fiddlerwoaroof/improvise
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

pr-reviews re-polls after a slow tick; the harness lock fails loudly

Browse Source 
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) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VdfEGXA7aJ6ftMsCc9PJze
This commit is contained in:
Edward Langley
2026-09-22 23:36:00 -07:00
parent 4b880846b9
commit 1550d390cb
6 changed files with 94 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
context/test-evidence/tooling.md
+44
View File
@@ -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.
harness/build.sh
+11
View File
@@ -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
harness/opfs-contract/Cargo.lock
Generated
+8 -8
View File
@@ -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",
]
harness/tests/opfs.spec.js
+5 -1
View File
@@ -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.
scripts/pr-reviews
+6 -2
View File
@@ -9,7 +9,8 @@ usage() {
usage: pr-reviews <pr-number> [--wait <sec>] [--interval <sec>]
--wait <sec> 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 <sec> 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"
scripts/pr-reviews-test.sh
+20
View File
@@ -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