From 1550d390cbc6ea180d5ae0f71fb4d4f8ffe6f973 Mon Sep 17 00:00:00 2001 From: Edward Langley Date: Tue, 22 Sep 2026 23:36:00 -0700 Subject: [PATCH] pr-reviews re-polls after a slow tick; the harness lock fails loudly improvise-qnpi: `pr-reviews --wait` checked its deadline before the first re-poll, so a second boundary between two reads of the clock skipped the only poll (exit 1 with polls=1, reproduced under CPU load). The deadline is now checked after each poll. A new case drives it with a PATH shim whose `date` ticks one second per read, so it fails deterministically on the old loop rather than under load. improvise-mkbp: harness/opfs-contract/Cargo.lock pinned pest 2.9.1 while its path deps require 2.9.2, so every build rewrote it. The resolved lock is committed and build.sh starts with `cargo metadata --locked`; the wasm-pack `-- --locked` route was tried and does not work (wasm-pack re-resolves first). opfs.spec.js now names the fix when harness/serve/opfs is missing, which is what running web/assemble.sh alone leaves. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze --- context/test-evidence/tooling.md | 44 ++++++++++++++++++++++++++++++++ harness/build.sh | 11 ++++++++ harness/opfs-contract/Cargo.lock | 16 ++++++------ harness/tests/opfs.spec.js | 6 ++++- scripts/pr-reviews | 8 ++++-- scripts/pr-reviews-test.sh | 20 +++++++++++++++ 6 files changed, 94 insertions(+), 11 deletions(-) diff --git a/context/test-evidence/tooling.md b/context/test-evidence/tooling.md index cd8ff234..3304bd5e 100644 --- a/context/test-evidence/tooling.md +++ b/context/test-evidence/tooling.md @@ -719,3 +719,47 @@ every target-specific entry, whose **normal** serde_json dependency lacks The `checked.len() >= 2` assertion is the positive signal. If a cargo change renamed the metadata fields, the check would find no serde_json dependency and pass vacuously without it. + +### The re-poll a second boundary could skip — `scripts/pr-reviews-test.sh` (improvise-qnpi) + +**What broke.** "an inline comment arriving mid-wait is seen" failed once in +two identical runs of `checkers.sh` on a loaded machine. `pr-reviews --wait` +computed `deadline = now + WAIT` and then tested `now < deadline` *before* its +first re-poll. With `--wait 1`, a second boundary falling between those two +reads of the clock made the loop run zero times: exit 1 with one poll, whatever +the second poll would have shown. + +**Reproduced under load.** A loop running that fixture with one busy loop per +core failed once, as `exit 1, polls=1`, around run 52; the fixed script ran +200 times under the same load with no failure. That is evidence of the +mechanism, not proof: the window is a few milliseconds per run. + +**The case that proves it.** "a second passing before the first re-poll does +not skip it" puts a `date` on `PATH` that advances one second per read (a +loaded machine, compressed) and a no-op `sleep`. Against the old loop it fails +every time, exit 1 (22 passed, 1 failed). The fix checks the deadline *after* +each poll, so any `--wait` above 0 polls at least once more. A larger timeout +would not have helped: the case uses the shimmed clock, where only the order of +check and poll matters. + +### The harness lock that every build rewrote — `harness/build.sh` (improvise-mkbp) + +**What broke.** `harness/opfs-contract/Cargo.lock` pinned `pest 2.9.1` while +its path dependencies require `pest = "2.9.2"`, so the committed lock was +invalid for its own manifest and every cargo invocation rewrote it. + +**Why both halves.** The resolved lock is committed, and `build.sh` now starts +with `cargo metadata --locked` against it. The lock alone would repeat the bug +at the next workspace dependency bump; `--locked` alone fails today. The check +is its own step because `wasm-pack build … -- --locked` does not work: with the +stale lock restored, that version exited 0 and still rewrote the lock, because +wasm-pack re-resolves before the flag reaches `cargo build`. The standalone +check, against the same stale lock, exits 101 in seconds and leaves the file +untouched. + +**The assemble half.** `web/assemble.sh` starts with `rm -rf` of its output, +which is what keeps the deployed tree free of stale files, so it is not taught +to spare a harness directory. `opfs.spec.js` instead asserts `/opfs/index.html` +is served, naming the fix. Injected by running `web/assemble.sh harness/serve` +alone: 4 chromium failures, each carrying that message, where it used to fail +with no hint of the cause. diff --git a/harness/build.sh b/harness/build.sh index f4e21ffa..ed73ca64 100755 --- a/harness/build.sh +++ b/harness/build.sh @@ -9,10 +9,21 @@ # half is the *deployed* arrangement rather than `web/` itself, because a # harness that runs against a different tree from the one that ships is # testing something nobody deploys (improvise-vvl3). +# +# Re-run THIS script, never `web/assemble.sh harness/serve` alone: assembling +# starts with `rm -rf` of its output, which deletes opfs/ and the fixtures +# (improvise-mkbp). opfs.spec.js says so if it finds opfs/ missing. set -eu root=$(cd "$(dirname "$0")/.." && pwd) serve="$root/harness/serve" +# A stale opfs-contract/Cargo.lock fails HERE rather than being rewritten in +# every agent's tree (improvise-mkbp). wasm-pack cannot enforce this: it +# re-resolves the lock itself before any `-- --locked` reaches cargo build. +# Fix a failure with `cargo update -w` in harness/opfs-contract, and commit it. +cargo metadata --locked --format-version 1 \ + --manifest-path "$root/harness/opfs-contract/Cargo.toml" >/dev/null + "$root/web/build.sh" >/dev/null "$root/web/assemble.sh" "$serve" harness >/dev/null diff --git a/harness/opfs-contract/Cargo.lock b/harness/opfs-contract/Cargo.lock index 08a283d2..d20a9d11 100644 --- a/harness/opfs-contract/Cargo.lock +++ b/harness/opfs-contract/Cargo.lock @@ -308,9 +308,9 @@ dependencies = [ [[package]] name = "pest" -version = "2.9.1" +version = "2.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6d45aeb61b4bf818e12d4205f2466f8c4748f85f4fce0146d1c03d69d753f0ad" +checksum = "45d3aca230fad2e6f6317ca0a72724338c4960cb97168a85cdee66df4a9a21a8" dependencies = [ "memchr", "ucd-trie", @@ -318,9 +318,9 @@ dependencies = [ [[package]] name = "pest_derive" -version = "2.9.1" +version = "2.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "89cc5a242e25ed4e7704d0be240f2cfbe20a8c27e7e252d94835be93d92dc39f" +checksum = "284b60557f2c4a2e72ad3f2d34d42685a2fa4a6a61d0d2a10c0ae2a5e916c2cf" dependencies = [ "pest", "pest_generator", @@ -328,9 +328,9 @@ dependencies = [ [[package]] name = "pest_generator" -version = "2.9.1" +version = "2.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7abf21475cc3820fe4b2ca2dc2142902f67a02189f3b5b3a229f4febc01a43e5" +checksum = "1d9d1f08a115309ee99268cf85e5228e0e56aa9caf8841ec12866b6be07c3109" dependencies = [ "pest", "pest_meta", @@ -341,9 +341,9 @@ dependencies = [ [[package]] name = "pest_meta" -version = "2.9.1" +version = "2.9.2" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "adba4db388f687393c18c51348d44a41d870ca9df71a2c98172ea3035dc6936e" +checksum = "ed93ba1a9ffcca32130a5188701c81c0c49cf00d4b7c5007d5148951d743adcb" dependencies = [ "pest", ] diff --git a/harness/tests/opfs.spec.js b/harness/tests/opfs.spec.js index 5d480441..83b2e556 100644 --- a/harness/tests/opfs.spec.js +++ b/harness/tests/opfs.spec.js @@ -23,7 +23,11 @@ const EXPECTED = [ test.describe("OpfsFs in a real browser", () => { test.beforeEach(async ({ page, browserName }, testInfo) => { - await page.goto("/opfs/index.html"); + const served = await page.goto("/opfs/index.html"); + expect( + served?.ok(), + "harness/serve/opfs is missing: re-run harness/build.sh, not web/assemble.sh alone", + ).toBe(true); // The skip is conditional on the PROBE, not on the browser name, so it // cannot quietly swallow a real regression. See clients.md. diff --git a/scripts/pr-reviews b/scripts/pr-reviews index cf46427d..55de7185 100755 --- a/scripts/pr-reviews +++ b/scripts/pr-reviews @@ -9,7 +9,8 @@ usage() { usage: pr-reviews [--wait ] [--interval ] --wait wait up to this long for a review or an inline review - comment whose commit is the current head + comment whose commit is the current head; any --wait + above 0 polls at least once more after the first read (default 0 = print what exists and exit) --interval seconds between polls (default 30) @@ -157,8 +158,10 @@ report "$json" if has_head_review "$json"; then exit 0; fi +# The deadline is checked AFTER each poll: checked first, a second ticking over +# between two reads of the clock skipped the only re-poll (improvise-qnpi). deadline=$(( $(date +%s) + WAIT )) -while [ "$(date +%s)" -lt "$deadline" ]; do +while :; do sleep "$INTERVAL" json=$(fetch) if has_head_review "$json"; then @@ -166,6 +169,7 @@ while [ "$(date +%s)" -lt "$deadline" ]; do report "$json" exit 0 fi + [ "$(date +%s)" -lt "$deadline" ] || break done printf '\npr-reviews: waited %ss; nothing against the current head.\n' "$WAIT" diff --git a/scripts/pr-reviews-test.sh b/scripts/pr-reviews-test.sh index a591f760..520315fe 100755 --- a/scripts/pr-reviews-test.sh +++ b/scripts/pr-reviews-test.sh @@ -197,6 +197,26 @@ mk "$d" last.issue.json <<'EOF' EOF run_case "an inline comment arriving mid-wait is seen" 0 "$d" --wait 1 --interval 1 +# The same fixture on a clock that ticks one second per read: a loaded machine, +# compressed. A second passing between two reads must not cost the only poll. +clock="$work/clock" +mkdir -p "$clock" +cat > "$clock/date" <<'EOF' +#!/usr/bin/env bash +[ "${1:-}" = "+%s" ] || exec /bin/date "$@" +n=$(( $(cat "$CLOCK_TICKS" 2>/dev/null || echo 0) + 1 )) +echo "$n" > "$CLOCK_TICKS" +echo $(( 1000 + n )) +EOF +printf '#!/usr/bin/env bash\nexit 0\n' > "$clock/sleep" +chmod +x "$clock/date" "$clock/sleep" +rm -f "$d/.poll" "$work/ticks" +CLOCK_TICKS="$work/ticks" PATH="$clock:$PATH" PR_REVIEWS_FIXTURE_DIR="$d" \ + "$REVIEWS" 1 --wait 1 --interval 1 > "$work/out" 2>&1 +got=$? +[ "$got" = 0 ] && ok "a second passing before the first re-poll does not skip it" \ + || bad "a second passing before the first re-poll does not skip it" "exit $got, wanted 0" + # --------------------------------------------------------------------------- # `gh api --paginate` emits one array PER PAGE, so a thread past the page size # arrives as several concatenated documents. Passing that straight to