From 22dcd953abb5400cde87d09ebfa96958cd6dcd6c Mon Sep 17 00:00:00 2001 From: Jihoon Kang Date: Fri, 18 Sep 2026 00:26:39 +0000 Subject: [PATCH] Record per-file mtimes in .kati_stamp for accurate regen detection Previously, Kati recorded a single process start time (gen_time) in .kati_stamp and compared every makefile and extra file dependency against it during --regen check. This caused false invalidations in two common scenarios: 1. Virtual or on-demand filesystems where uncached source files are materialized on first read during evaluation, assigning them an mtime of TimeNow() which is later than Kati's start time. 2. Build-generated intermediate staging files written during Make evaluation with an mtime later than Kati's start time. On subsequent builds, Kati would see file.mtime > gen_time and falsely conclude that the files were modified, triggering unnecessary full reanalysis. This change stores each file's observed mtime alongside its path in .kati_stamp (both in ckati and rkati). During --regen, each file's current mtime is compared against its recorded mtime. If the file's mtime has not advanced, it is recognized as clean. --- src-rs/file.rs | 23 +++++--- src-rs/file_cache.rs | 29 ++++++---- src-rs/ninja.rs | 7 ++- src-rs/regen.rs | 15 +++-- src-rs/regen_dump.rs | 8 ++- src/file.cc | 2 +- src/file.h | 3 +- src/file_cache.cc | 10 ++-- src/file_cache.h | 5 +- src/ninja.cc | 9 ++- src/regen.cc | 11 +++- src/regen_dump.cc | 14 ++++- testcase/ninja_regen_stamp_mtime.sh | 89 +++++++++++++++++++++++++++++ 13 files changed, 183 insertions(+), 42 deletions(-) create mode 100755 testcase/ninja_regen_stamp_mtime.sh diff --git a/src-rs/file.rs b/src-rs/file.rs index 79404942..4c7f5001 100644 --- a/src-rs/file.rs +++ b/src-rs/file.rs @@ -14,7 +14,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -use std::{ffi::OsStr, os::unix::ffi::OsStrExt, sync::Arc}; +use std::{ffi::OsStr, io::Read, os::unix::ffi::OsStrExt, sync::Arc, time::SystemTime}; use anyhow::Result; use bytes::Bytes; @@ -28,20 +28,29 @@ use crate::{ pub struct Makefile { pub filename: Symbol, + pub mtime: SystemTime, pub stmts: Arc>>, } impl Makefile { pub fn from_file(filename: &OsStr) -> Result>> { - if !std::fs::exists(filename)? { - return Ok(None); - } - - let buf = Bytes::from(std::fs::read(filename)?); + let mut file = match std::fs::File::open(filename) { + Ok(f) => f, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None), + Err(e) => return Err(e.into()), + }; + let mtime = file.metadata()?.modified()?; + let mut buf = Vec::new(); + file.read_to_end(&mut buf)?; + let buf = Bytes::from(buf); let filename = intern(filename.as_bytes().to_vec()); let stmts = parse_file(&buf, filename)?; - Ok(Some(Arc::new(Makefile { filename, stmts }))) + Ok(Some(Arc::new(Makefile { + filename, + mtime, + stmts, + }))) } } diff --git a/src-rs/file_cache.rs b/src-rs/file_cache.rs index d2e18b28..70489db1 100644 --- a/src-rs/file_cache.rs +++ b/src-rs/file_cache.rs @@ -15,9 +15,10 @@ limitations under the License. */ use std::{ - collections::{HashMap, HashSet}, + collections::HashMap, ffi::{OsStr, OsString}, sync::{Arc, LazyLock}, + time::{SystemTime, UNIX_EPOCH}, }; use anyhow::Result; @@ -28,13 +29,13 @@ use crate::file::Makefile; static CACHE: LazyLock> = LazyLock::new(|| { Mutex::new(MakefileCacheManager { cache: HashMap::new(), - extra_file_deps: HashSet::new(), + extra_file_deps: HashMap::new(), }) }); struct MakefileCacheManager { cache: HashMap>>, - extra_file_deps: HashSet, + extra_file_deps: HashMap, } impl MakefileCacheManager { @@ -54,17 +55,25 @@ pub fn get_makefile(filename: &OsStr) -> Result>> { } pub fn add_extra_file_dep(filename: OsString) { - CACHE.lock().extra_file_deps.insert(filename); + let mtime = std::fs::metadata(&filename) + .and_then(|m| m.modified()) + .unwrap_or(UNIX_EPOCH); + CACHE.lock().extra_file_deps.insert(filename, mtime); } -pub fn get_all_filenames() -> HashSet { +pub fn get_all_filenames() -> HashMap { let manager = CACHE.lock(); - let mut ret = HashSet::new(); - for p in manager.cache.keys() { - ret.insert(p.clone()); + let mut ret = HashMap::new(); + for (p, mk) in &manager.cache { + let mtime = mk + .as_ref() + .map(|m| m.mtime) + .or_else(|| std::fs::metadata(p).and_then(|m| m.modified()).ok()) + .unwrap_or(UNIX_EPOCH); + ret.insert(p.clone(), mtime); } - for f in &manager.extra_file_deps { - ret.insert(f.clone()); + for (f, mtime) in &manager.extra_file_deps { + ret.insert(f.clone(), *mtime); } ret } diff --git a/src-rs/ninja.rs b/src-rs/ninja.rs index bc1e8f89..297b80f4 100644 --- a/src-rs/ninja.rs +++ b/src-rs/ninja.rs @@ -743,8 +743,13 @@ impl<'a> NinjaGenerator<'a> { let makefiles = file_cache::get_all_filenames(); dump_usize(&mut out, makefiles.len() + 1)?; dump_string(&mut out, self.kati_binary.as_bytes())?; - for makefile in makefiles { + let kati_binary_ts = std::fs::metadata(&self.kati_binary) + .and_then(|m| m.modified()) + .unwrap_or(self.start_time); + dump_systemtime(&mut out, &kati_binary_ts)?; + for (makefile, mtime) in makefiles { dump_string(&mut out, makefile.as_bytes())?; + dump_systemtime(&mut out, &mtime)?; } dump_usize(&mut out, Evaluator::used_undefined_vars().len())?; diff --git a/src-rs/regen.rs b/src-rs/regen.rs index 8d01cf2d..16ab76c8 100644 --- a/src-rs/regen.rs +++ b/src-rs/regen.rs @@ -148,10 +148,15 @@ impl StampChecker { println!("Generated time: {:?}", self.gen_time); } - let files = load!(load_vec_string(fp)); - for s in files { + let num_files = load!(load_usize(fp)); + for _ in 0..num_files { + let s = load!(load_string(fp)); + let recorded_ts = load!(load_systemtime(fp)); let ts = std::fs::metadata(&s).and_then(|m| m.modified()); - if ts.as_ref().is_ok_and(|ts| gen_time >= *ts) { + if ts + .as_ref() + .is_ok_and(|ts| recorded_ts + std::time::Duration::from_micros(1) >= *ts) + { if FLAGS.dump_kati_stamp { println!("file {s:?}: clean ({:?})", ts.unwrap()) } @@ -162,12 +167,12 @@ impl StampChecker { } if should_ignore_dirty(s.as_bytes()) { if FLAGS.regen_debug { - println!("file {s:?}: ignored ({:?})", ts.unwrap()); + println!("file {s:?}: ignored ({:?})", ts.ok()); } continue; } if FLAGS.dump_kati_stamp { - println!("file {s:?}: dirty ({:?})", ts.unwrap()); + println!("file {s:?}: dirty ({:?} > {:?})", ts, recorded_ts); } else { eprintln!("{} was modified, regenerating...", s.to_string_lossy()); } diff --git a/src-rs/regen_dump.rs b/src-rs/regen_dump.rs index 81a3b5b6..7c5a6e8f 100644 --- a/src-rs/regen_dump.rs +++ b/src-rs/regen_dump.rs @@ -85,9 +85,11 @@ fn inner( // { - let files = load_vec_string(fp)?; - if dump_files { - for file in files { + let num_files = load_usize(fp)?; + for _ in 0..num_files { + let file = load_string(fp)?; + let _ts = load_systemtime(fp)?; + if dump_files { println!("{}", file.display()); } } diff --git a/src/file.cc b/src/file.cc index dfae6f8a..be51ff10 100644 --- a/src/file.cc +++ b/src/file.cc @@ -39,7 +39,7 @@ Makefile::Makefile(const std::string& filename) } size_t len = st.st_size; - mtime_ = st.st_mtime; + mtime_ = GetTimestampFromStat(st); buf_.resize(len); exists_ = true; size_t remaining = len; diff --git a/src/file.h b/src/file.h index 757d7e75..bfb6e08d 100644 --- a/src/file.h +++ b/src/file.h @@ -34,10 +34,11 @@ class Makefile { std::vector* mutable_stmts() { return &stmts_; } bool Exists() const { return exists_; } + double mtime() const { return mtime_; } private: std::string buf_; - uint64_t mtime_; + double mtime_; std::string filename_; std::vector stmts_; bool exists_; diff --git a/src/file_cache.cc b/src/file_cache.cc index 465c45a4..19cc8761 100644 --- a/src/file_cache.cc +++ b/src/file_cache.cc @@ -18,6 +18,7 @@ #include "file.h" #include "file_cache.h" +#include "fileutil.h" const Makefile& MakefileCacheManager::ReadMakefile( const std::string& filename) { @@ -29,15 +30,16 @@ const Makefile& MakefileCacheManager::ReadMakefile( } void MakefileCacheManager::GetAllFilenames( - std::unordered_set* out) { + std::unordered_map* out) const { for (const auto& p : cache_) - out->insert(p.first); + (*out)[p.first] = p.second.mtime(); for (const auto& f : extra_file_deps_) - out->insert(f); + (*out)[f.first] = f.second; } void MakefileCacheManager::AddExtraFileDep(std::string_view dep) { - extra_file_deps_.emplace(dep); + std::string s(dep); + extra_file_deps_[s] = GetTimestamp(s); } MakefileCacheManager& MakefileCacheManager::Get() { diff --git a/src/file_cache.h b/src/file_cache.h index a17c861c..ba176d85 100644 --- a/src/file_cache.h +++ b/src/file_cache.h @@ -16,6 +16,7 @@ #define FILE_CACHE_H_ #include +#include #include #include "file.h" @@ -23,7 +24,7 @@ class MakefileCacheManager { public: const Makefile& ReadMakefile(const std::string& filename); - void GetAllFilenames(std::unordered_set* out); + void GetAllFilenames(std::unordered_map* out) const; void AddExtraFileDep(std::string_view dep); static MakefileCacheManager& Get(); @@ -33,7 +34,7 @@ class MakefileCacheManager { MakefileCacheManager(const MakefileCacheManager&) = delete; MakefileCacheManager(MakefileCacheManager&&) = delete; std::unordered_map cache_; - std::unordered_set extra_file_deps_; + std::unordered_map extra_file_deps_; }; #endif // FILE_CACHE_H_ diff --git a/src/ninja.cc b/src/ninja.cc index cd3c6abd..acfef402 100644 --- a/src/ninja.cc +++ b/src/ninja.cc @@ -674,12 +674,17 @@ class NinjaGenerator { size_t r = fwrite(&start_time_, sizeof(start_time_), 1, fp); CHECK(r == 1); - std::unordered_set makefiles; + std::unordered_map makefiles; MakefileCacheManager::Get().GetAllFilenames(&makefiles); DumpInt(fp, makefiles.size() + 1); DumpString(fp, kati_binary_); - for (const std::string& makefile : makefiles) { + double kati_binary_ts = GetTimestamp(kati_binary_); + r = fwrite(&kati_binary_ts, sizeof(kati_binary_ts), 1, fp); + CHECK(r == 1); + for (const auto& [makefile, mtime] : makefiles) { DumpString(fp, makefile); + r = fwrite(&mtime, sizeof(mtime), 1, fp); + CHECK(r == 1); } DumpInt(fp, Evaluator::used_undefined_vars().size()); diff --git a/src/regen.cc b/src/regen.cc index c5835567..4009defe 100644 --- a/src/regen.cc +++ b/src/regen.cc @@ -160,10 +160,15 @@ class StampChecker { int num_files = LOAD_INT(fp); for (int i = 0; i < num_files; i++) { LOAD_STRING(fp, &s); + double recorded_ts; + if (fread(&recorded_ts, sizeof(recorded_ts), 1, fp) != 1) { + fprintf(stderr, "incomplete kati_stamp, regenerating...\n"); + RETURN_TRUE; + } double ts = GetTimestamp(s); // GetTimestamp returns < 0 when there's an error reading the file, like - // when its been removed. - if (gen_time < ts || ts < 0) { + // when its been removed. Allow 1e-6s tolerance for float representation. + if (recorded_ts + 1e-6 < ts || ts < 0) { if (g_flags.regen_ignoring_kati_binary) { if (s == GetExecutablePath()) { fprintf(stderr, "%s was modified, ignored.\n", s.c_str()); @@ -176,7 +181,7 @@ class StampChecker { continue; } if (g_flags.dump_kati_stamp) - printf("file %s: dirty (%f)\n", s.c_str(), ts); + printf("file %s: dirty (%f > %f)\n", s.c_str(), ts, recorded_ts); else fprintf(stderr, "%s was modified, regenerating...\n", s.c_str()); RETURN_TRUE; diff --git a/src/regen_dump.cc b/src/regen_dump.cc index bea12960..a5a144e1 100644 --- a/src/regen_dump.cc +++ b/src/regen_dump.cc @@ -95,9 +95,17 @@ int stamp_dump_main(int argc, char* argv[]) { // { - auto files = LoadVecString(fp); - if (dump_files) { - for (const auto& f : files) { + int num_files = LoadInt(fp); + if (num_files < 0) + ERROR("Incomplete stamp file"); + for (int i = 0; i < num_files; i++) { + std::string f; + if (!LoadString(fp, &f)) + ERROR("Incomplete stamp file"); + double ts; + if (fread(&ts, sizeof(ts), 1, fp) != 1) + ERROR("Incomplete stamp file"); + if (dump_files) { printf("%s\n", f.c_str()); } } diff --git a/testcase/ninja_regen_stamp_mtime.sh b/testcase/ninja_regen_stamp_mtime.sh new file mode 100755 index 00000000..f9101e62 --- /dev/null +++ b/testcase/ninja_regen_stamp_mtime.sh @@ -0,0 +1,89 @@ +#!/bin/sh +# +# Copyright 2026 Google Inc. All rights reserved +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http:#www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -eu + +log=stderr_log +mk="$@" + +touch dep.mk extra.txt +# Set mtime to a future timestamp (later than Kati's process start time). +# Previously, Kati compared file mtime against the generation start time, +# falsely marking files with mtime > gen_time as modified on subsequent runs. +touch -d "2050-01-01 00:00:00" dep.mk extra.txt + +cat < Makefile +include dep.mk +EXTRA_DEPS := extra.txt +\$(KATI_extra_file_deps \$(EXTRA_DEPS)) +all: + echo foo +EOF + +${mk} 2> ${log} +if [ -e ninja.sh ]; then + ./ninja.sh +fi + +# A second run without changes should NOT regenerate, even though the file +# mtimes are in the future relative to when Kati started. +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if grep -q regenerating ${log}; then + echo 'Should not be regenerated' + fi + ./ninja.sh +fi + +# Advancing the mtime of an included makefile should trigger regeneration. +touch -d "2051-01-01 00:00:00" dep.mk + +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if ! grep -q regenerating ${log}; then + echo 'Should have regenerated due to touched makefile' + fi + ./ninja.sh +fi + +# Running again without changes should NOT regenerate. +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if grep -q regenerating ${log}; then + echo 'Should not be regenerated after makefile regen' + fi + ./ninja.sh +fi + +# Advancing the mtime of an extra file dependency should trigger regeneration. +touch -d "2051-01-01 00:00:00" extra.txt + +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if ! grep -q regenerating ${log}; then + echo 'Should have regenerated due to touched extra file dependency' + fi + ./ninja.sh +fi + +# Running again without changes should NOT regenerate. +${mk} 2> ${log} +if [ -e ninja.sh ]; then + if grep -q regenerating ${log}; then + echo 'Should not be regenerated after extra file dep regen' + fi + ./ninja.sh +fi