From 2071810dab749f15cb8be48cf499a9d5906c292e Mon Sep 17 00:00:00 2001 From: Yicong Huang <17627829+Yicong-Huang@users.noreply.github.com> Date: Wed, 29 Jul 2026 23:01:22 +0000 Subject: [PATCH] fix(local-dev): rebuild a source edited in the stamp's timestamp tick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dirty-source fast filter treated "source mtime equal to the build stamp's" as clean, so an edit landing in the same filesystem timestamp tick as the stamp write was never rebuilt: `auto` reported `everything up-to-date` and bounced nothing. The window is one tick wide, but the stamp is written at the end of a build and the natural next action is editing the file you were just building, so it is hit in normal use — how wide the tick is depends on the filesystem and kernel clock granularity, not on how fast anyone types. Both implementations had it. `svc_src_changed` used `find -newer "$stamp"`, where `-newer` is strictly newer; `tui.py`'s `_newest_mtime_after` used a strict `>`. The shell side is the consequential half — it is what gates the rebuild, while tui.py only colours the SRC column. It now compares against a throwaway marker one second behind the stamp, because `find` has no portable "not older than". The stamp itself keeps its real mtime, so the mtime refresh at the end of the slow path still converges. tui.py can say what it means, so it uses `>=`. Both directions stay conservative rather than wrong: widening the filter only enlarges the candidate set, and the content hash underneath — which was always correct — makes the actual decision. It also self-heals, because the first tick after a build takes the hash path once, finds the content unchanged, and bumps the stamp past the sources. This also fixes `test_is_dirty_after_seed_then_edit`, which had been failing on any filesystem whose granularity is coarser than its two consecutive writes; the new test forces the colliding mtime with `os.utime` so it no longer depends on the platform. Closes #7075 --- bin/local-dev/main.sh | 29 +++++++++- bin/local-dev/tests/test_local_dev_sh.sh | 67 +++++++++++++++++++++++ bin/local-dev/tests/test_local_dev_tui.py | 32 +++++++++++ bin/local-dev/tui.py | 14 ++++- 4 files changed, 140 insertions(+), 2 deletions(-) diff --git a/bin/local-dev/main.sh b/bin/local-dev/main.sh index 8ee11a256bd..a1240208ac7 100755 --- a/bin/local-dev/main.sh +++ b/bin/local-dev/main.sh @@ -1971,6 +1971,17 @@ svc_source_hash() { } # Per-service dirty check (the SRC * indicator). Two-stage: +# Set a file's mtime one second into the past, in whichever `touch` dialect is +# present. Used to build the comparison reference for the fast path below; see +# there for why the second of slack is needed. A missing path is a quiet no-op. +_stamp_backdate() { + [[ -f "${1:-}" ]] || return 0 + # GNU coreutils, then BSD/macOS `-A` (adjust the timestamps by -1 second). + touch -d '1 second ago' "$1" 2>/dev/null \ + || touch -A -01 "$1" 2>/dev/null \ + || true +} + # Fast path (~22 ms): is any tracked source newer than the stamp file's # mtime? If not, definitely clean. # Slow path (~100 ms): compute current source hash and compare to the hash @@ -2003,10 +2014,26 @@ svc_src_changed() { while IFS= read -r d; do [[ -n "$d" ]] && dirs+=("$d") done < <(_svc_src_dirs "$svc") + # `find -newer` is *strictly* newer, and the stamp is written at the + # end of a build — right before you edit the file you were just + # building. An edit inside the filesystem's timestamp granularity + # therefore shares the stamp's mtime exactly and used to be + # invisible here, so `auto` skipped the rebuild (#7075). Compare + # against a throwaway marker one second behind the stamp instead. + # The slack only widens the candidate set; the content hash below + # still decides. The stamp itself keeps its real mtime, so the + # refresh at the end of the slow path converges as before. + local cmp_ref="$stamp" + local marker="$BUILD_STAMP_DIR/.${svc}.cmp" + if touch -r "$stamp" "$marker" 2>/dev/null; then + _stamp_backdate "$marker" + cmp_ref="$marker" + fi local newer="" newer=$(find "${dirs[@]}" \ \( -name "*.scala" -o -name "*.java" -o -name "*.proto" \) \ - -newer "$stamp" -type f -print 2>/dev/null | head -1) + -newer "$cmp_ref" -type f -print 2>/dev/null | head -1) + rm -f "$marker" if [[ -z "$newer" ]]; then return 1 # nothing changed since last stamp → clean fi diff --git a/bin/local-dev/tests/test_local_dev_sh.sh b/bin/local-dev/tests/test_local_dev_sh.sh index c8d5e1d4179..06a340b3722 100755 --- a/bin/local-dev/tests/test_local_dev_sh.sh +++ b/bin/local-dev/tests/test_local_dev_sh.sh @@ -593,5 +593,72 @@ else _fail "changelog references missing files:$missing_files" fi +# 29) Regression for #7075: `find -newer` is *strictly* newer, so a source whose +# mtime equals the build stamp's is invisible to the fast filter and `auto` +# reports "everything up-to-date" without rebuilding it. The read side +# therefore compares against a throwaway marker one second behind the stamp. +# Deterministic: the colliding mtime is forced with `touch -r`, not raced. +stamp_fn=$(awk 'index($0, "_stamp_backdate()") == 1 {f=1} f{print} f && /^}/{exit}' "$MAIN_SH") +if [[ -z "$stamp_fn" ]]; then + _fail "_stamp_backdate helper missing" +else + _sd=$(mktemp -d 2>/dev/null || mktemp -d -t ldsd) + mkdir -p "$_sd/src" + # Older.scala must end up comfortably behind the stamp, further back than + # the one second of slack the marker adds — hence a real wait rather than a + # computed timestamp, which has no portable spelling. + : > "$_sd/src/Older.scala" + sleep 2 + : > "$_sd/stamp" + : > "$_sd/src/Same.scala" + touch -r "$_sd/stamp" "$_sd/src/Same.scala" # exactly the stamp's mtime + + # The bug itself, characterised: comparing against the stamp misses it. + naive=$(find "$_sd/src" -name '*.scala' -newer "$_sd/stamp" -print 2>/dev/null) + if [[ "$naive" != *"Same.scala"* ]]; then + _pass "stamp backdate: bare \`-newer \$stamp\` misses an equal mtime (the bug)" + else + _fail "stamp backdate: premise no longer holds — -newer saw an equal mtime" \ + "found: $naive" + fi + + # The fix: a marker one second behind the stamp sees it. + marker="$_sd/marker" + ( eval "$stamp_fn" + touch -r "$_sd/stamp" "$marker" && _stamp_backdate "$marker" ) 2>/dev/null + fixed=$(find "$_sd/src" -name '*.scala' -newer "$marker" -print 2>/dev/null) + if [[ "$fixed" == *"Same.scala"* ]]; then + _pass "stamp backdate: backdated marker sees the equal-mtime edit" + else + _fail "stamp backdate: backdated marker still misses the edit" "found: '$fixed'" + fi + # Negative: the slack must not drag genuinely older sources in, or every + # tick pays the content hash forever. + if [[ "$fixed" != *"Older.scala"* ]]; then + _pass "stamp backdate: a clearly older source stays clean" + else + _fail "stamp backdate: slack flagged an older source" "found: $fixed" + fi + # Negative: a missing path must be a quiet no-op, not an error spray. + err=$( ( eval "$stamp_fn"; _stamp_backdate "$_sd/nope" ) 2>&1 ); rc=$? + if (( rc == 0 )) && [[ -z "$err" ]]; then + _pass "stamp backdate: missing file is a quiet no-op" + else + _fail "stamp backdate: missing file was noisy" "rc=$rc err='$err'" + fi + rm -rf "$_sd" +fi + +# 30) Wiring: the jvm dirty check must compare against the backdated marker +# rather than the stamp, or #7075 is only half fixed (the shell path is the +# one that gates the rebuild; tui.py only colours the SRC column). +src_changed_body=$(awk 'index($0, "svc_src_changed()") == 1 {f=1} f{print} f && /^}/{exit}' "$MAIN_SH") +if [[ "$src_changed_body" == *"_stamp_backdate"* ]] \ + && ! printf '%s\n' "$src_changed_body" | grep -qE '\-newer "\$stamp"'; then + _pass "svc_src_changed compares against the backdated marker" +else + _fail "svc_src_changed still compares directly against \$stamp" +fi + printf "\n%d passed, %d failed\n" "$PASS" "$FAIL" (( FAIL == 0 )) diff --git a/bin/local-dev/tests/test_local_dev_tui.py b/bin/local-dev/tests/test_local_dev_tui.py index 0679ef76848..ae8d1b57514 100644 --- a/bin/local-dev/tests/test_local_dev_tui.py +++ b/bin/local-dev/tests/test_local_dev_tui.py @@ -175,6 +175,38 @@ def test_is_dirty_after_seed_then_edit(tmp_path, monkeypatch, tui): assert tui.is_dirty(svc) is True +def test_is_dirty_when_edit_shares_the_stamp_mtime(tmp_path, monkeypatch, tui): + """An edit landing in the same filesystem timestamp tick as the stamp write + must still be seen. + + `test_is_dirty_after_seed_then_edit` above hits this by accident wherever + the filesystem's granularity is coarser than its two consecutive writes; + here the collision is forced with os.utime, so it holds on every platform. + The fast mtime filter must not answer "definitely clean" without consulting + the content hash, or `auto` silently skips the rebuild.""" + monkeypatch.setattr(tui, "REPO_ROOT", tmp_path) + monkeypatch.setattr(tui, "BUILD_STAMP_DIR", tmp_path / "stamps") + (tmp_path / "stamps").mkdir() + _seed_jvm_layout(tmp_path, "config-service/src") + + svc = tui.SERVICES_BY_NAME["config-service"] + jar = tmp_path / svc.artifact_jar + jar.parent.mkdir(parents=True, exist_ok=True) + jar.write_bytes(b"fake-jar-bytes") + + assert tui.is_dirty(svc) is False + stamp = tmp_path / "stamps" / svc.name + + # Change the content, then force the source's mtime to exactly the stamp's. + src = tmp_path / "config-service/src/Main.scala" + src.write_text("object Main { def y = 2 }\n") + st = stamp.stat() + os.utime(src, ns=(st.st_atime_ns, st.st_mtime_ns)) + assert src.stat().st_mtime_ns == stamp.stat().st_mtime_ns + + assert tui.is_dirty(svc) is True + + def test_is_dirty_mtime_bump_without_content_change_stays_clean(tmp_path, monkeypatch, tui): """Robustness against `git checkout` touching mtimes — the whole reason we moved off pure-mtime detection. After seeding the stamp, simulating a diff --git a/bin/local-dev/tui.py b/bin/local-dev/tui.py index 305244307e4..e3892cc030e 100644 --- a/bin/local-dev/tui.py +++ b/bin/local-dev/tui.py @@ -592,9 +592,21 @@ def source_hash(svc: Service, files: Optional[list[Path]] = None) -> str: def _newest_mtime_after(files: list[Path], stamp_mtime: float) -> bool: + """Any source at or after the stamp's mtime? + + `>=`, not `>`: the stamp is written at the end of a build and the natural + next action is to edit the file you were just building, so an edit landing + inside the filesystem's timestamp granularity shares the stamp's mtime + exactly. With a strict `>` the filter answered "definitely clean" and the + content hash was never consulted, so the rebuild was skipped (#7075). + + Equality costs at most one extra hash comparison: when it finds the content + unchanged, `_jvm_is_dirty` bumps the stamp's mtime past the sources, and + later ticks take the cheap path again. + """ for f in files: try: - if f.stat().st_mtime > stamp_mtime: + if f.stat().st_mtime >= stamp_mtime: return True except OSError: continue