This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-7079-0834f25d605a611ed9130fd05c7a7e6f702254d1 in repository https://gitbox.apache.org/repos/asf/texera.git
commit ebc8c64411149c71f5f9d467badb3fbe9339fd8f Author: Yicong Huang <[email protected]> AuthorDate: Wed Jul 29 20:02:07 2026 -0400 fix(local-dev): rebuild a source edited in the stamp's timestamp tick (#7079) ### What changes were proposed in this PR? 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, which sounds negligible but isn't: the stamp is written at the end of a build, and the natural next action is editing the file you were just building. How wide the tick is depends on the filesystem and kernel clock granularity, not on how fast anyone types. Both implementations had it, and the shell one is the consequential half — it gates the rebuild, while `tui.py` only colours the `SRC` column: | Where | Was | Now | | --- | --- | --- | | `main.sh` `svc_src_changed` | `find … -newer "$stamp"` (strictly newer) | compares against a throwaway marker one second behind the stamp | | `tui.py` `_newest_mtime_after` | `st_mtime > stamp_mtime` | `>=` | ``` Before: build -> edit within the same tick -> auto: "everything up-to-date" After: build -> edit within the same tick -> hash compared -> rebuilt ``` The two fixes differ because the two languages can express different things: `find` has no portable "not older than", so the shell borrows one second of slack via a marker; Python can say exactly what it means, so it uses `>=`. The marker is a throwaway — the stamp itself keeps its real mtime, so the mtime refresh at the end of the slow path converges exactly as before. Both directions stay conservative rather than wrong. Widening the filter only enlarges the *candidate* set; the content hash underneath — which was always correct — still makes the decision. And it self-heals: the first tick after a build takes the hash path once, finds the content unchanged, and bumps the stamp past the sources, so later ticks are cheap again. A source that is genuinely older than the stamp is not dragged in by the slack (asserted below). ### Any related issues, documentation, discussions? Closes #7075 ### How was this PR tested? The bug's own symptom was an existing test that failed only on some filesystems. This PR adds a deterministic version of it — `os.utime` forces the colliding mtime instead of racing for it — so it holds on every platform: ``` $ python -m pytest bin/local-dev/tests/ -q 44 passed ``` That count matters: before this change the suite was `1 failed, 42 passed` on this machine, the failure being `test_is_dirty_after_seed_then_edit`, which had been hitting the same bug by accident wherever the filesystem granularity was coarser than its two consecutive writes. It now passes for the right reason rather than by platform luck. The shell side is covered by the mechanism it actually uses, with the colliding mtime forced by `touch -r`: ``` $ bash bin/local-dev/tests/test_local_dev_sh.sh ... ✓ stamp backdate: bare `-newer $stamp` misses an equal mtime (the bug) ✓ stamp backdate: backdated marker sees the equal-mtime edit ✓ stamp backdate: a clearly older source stays clean ✓ stamp backdate: missing file is a quiet no-op ✓ svc_src_changed compares against the backdated marker 55 passed, 0 failed ``` The first of those characterises the bug itself, so it will start failing if a future `find` learns to include equal mtimes — at which point the marker can go. Reproduction outside the suite, for the record: ``` $ D=$(mktemp -d); mkdir -p $D/src; : > $D/stamp; : > $D/src/A.scala $ touch -r $D/stamp $D/src/A.scala # identical mtimes $ find $D/src -name '*.scala' -newer $D/stamp -print # empty — A.scala is invisible ``` Not covered: an end-to-end `auto` that rebuilds off a same-tick edit. Forcing that on the real stack means winning the same race the test now sidesteps, so the suite's deterministic version is the check that carries the weight here. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Code (Claude Opus 5) --- 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 8ee11a256b..a1240208ac 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 c8d5e1d417..06a340b372 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 0679ef7684..ae8d1b5751 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 305244307e..e3892cc030 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
