Skip to content

test(installer): drive download fixtures on events, not one event-loop turn (HACK-2250) - #858

Merged
roodboi merged 3 commits into
nextfrom
test/hack-2250-download-cancel-bound
Nov 15, 2026
Merged

roodboi merged 3 commits into
nextfrom
test/hack-2250-download-cancel-bound

Conversation

@roodboi

@roodboi roodboi commented Nov 15, 2026 •

Copy link
Copy Markdown
Contributor

What

tests/python-free-download.test.ts, "bounded cancellation refuses uncertain read-error settlement and preserves the primary failure", hit Bun's 5 s default timeout on hosted macOS shard 1/8 (#853). The next scenario then failed toHaveBeenCalledTimes(1) with 2, and the global-state guard reported globalThis.fetch replaced.

What it was really waiting on

The test drove a fake clock after a single setTimeout(0):

await drain();      // one event-loop turn
clock.advance(5);   // expire the 5 ms cleanup grace

transferAttempt (src/lib/release-transfer.ts) awaits a real open() of the target file before reading. Only after the size failure does settleCancellation arm the 5 ms grace on the fake clock. If open() takes more than one turn (a loaded runner), the advance fires nothing. The grace is armed afterwards at the new time and never expires, so the download never settles. The test then hangs until the 5 s timeout, and its finally { mock.mockRestore() } never runs. The next scenario's spyOn(globalThis, "fetch") reuses the leaked spy, which already has one call: hence 2, and the guard error.

sequenceDiagram
  participant T as test
  participant D as download
  participant C as fake clock
  T->>D: start
  T->>T: one setTimeout(0)
  T->>C: advance(5): nothing armed yet
  D->>D: open() completes (real I/O, late)
  D->>C: arm 5 ms cleanup grace (never advanced)
  Note over T: hangs to the 5 s timeout, finally skipped, fetch spy leaks
Loading

The same assumption (one turn is enough) sat in three sibling tests in this file: "HTTP transfer budget … progress / total" and "five-file … cleanup". They fail the same way when open() is slow.

Fix (test-only)

  • until(ready, what) drains in a loop with a 2 s real-time bound, well under the 5 s default. A missed step fails with Fixture step did not happen within 2000 ms: <what> instead of a timeout. within(promise, what) bounds the final settlement the same way.
  • Before advancing the 5 ms cleanup grace, the test waits for clock.isArmed(5). FixtureClock now records each timer's delay; the other timers are 30 and 100 ms.
  • Before an inactivity tick, the test waits for an actual pending read. The fixture streams use highWaterMark: 0 and flag pull(), which runs only while the download is reading. A file-existence check was not enough: the file exists before open() returns its handle (found in review).
  • The five-file spin loops, capped at 100 turns, use the same real-time bound.
  • afterEach calls mock.restore(), so a test that fails before its finally cannot leak the fetch spy into the next test. The existing per-test mockRestore() calls stay.

No assertion is weakened or removed.

Validation (Linux, 16 cores)

Check Before After
-t "bounded cancellation" under 16-way parallel load 5 of 400 failed, with the hosted signature (5 s timeout, then 2 calls and the guard error) 400 of 400 pass at 5f16f8b
Whole file under 16-way parallel load 2 of 191 failed at an intermediate head (progress/total false stall, found in review) 208 of 208 pass (15/15 each) at 5f16f8b
Preload delaying open() by 50 ms 4 failures, incl. read-error and stalled-read timeouts with the guard error 15/15
Preload delaying open() by 200 ms — 15/15
Preload delaying after open() creates the file progress/total fail 15/15

tsc -p packages/cli (it includes tests/) and biome check pass.

Not verified: macOS. The lead runs the 200 on the Mac.

Release signal

test: test-only.

Refs HACK-2250.

🤖 Generated with Claude Code

roodboi and others added 2 commits November 15, 2026 02:09
…p turn (HACK-2250)

"bounded cancellation refuses uncertain read-error settlement" advanced the
fixture clock after a single setTimeout(0). The download opens its target
file (real I/O) before it reads, and only then arms the 5 ms cleanup grace
on the fixture clock. When open() took longer than one turn, the advance
fired nothing, the grace never expired, and the test hung until Bun's 5 s
timeout. Its finally never ran, so fetch stayed spied: the next scenario
saw two calls and the global-state guard reported fetch replaced.

The tests now wait, bounded at 2 s, for the event each step depends on:
the cleanup grace armed before advancing 5 ms, the target file open before
an inactivity tick, and the five-file sequence's spin loops use the same
real-time bound instead of 100 turns. Final settlement is awaited with the
same bound, and afterEach restores every spy so a failed test cannot leak
fetch into the next one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… clock ticks

Review of HACK-2250: the target file can exist before the awaited open()
returns its handle, so "file exists, then one turn" still let an inactivity
tick land before the download was reading (a false stall). The streams in
the transfer-budget and stalled-read fixtures now use highWaterMark 0 and
flag pull(), which runs only while the download has a read pending, and
each tick waits for that flag, bounded at 2 s.

Under 16-way parallel load the previous head failed 2 of 191 runs of the
file this way (progress and total); a delay injected after open() creates
the file reproduces it every time and passes with this change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vercel

vercel Bot commented Nov 15, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
hack-docs Ready Ready Preview Nov 15, 2026 2:29am UTC

Request Review

@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@linear-code

linear-code Bot commented Nov 15, 2026

Copy link
Copy Markdown

HACK-2250

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Nov 15, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-15T02:30:26.436082Z 0c33970 Manual request
🔒 Security Review ✅ Completed 2026-11-15T02:32:32.119223Z 0c33970 Manual request
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. Delightful!

Reviewed commit: 5f16f8be39

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 5f16f8be39

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 🎉

Reviewed commit: 0c33970830

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review · Automatically triggered

Security review completed. No security issues were found in this pull request.

Reviewed commit: 0c33970830

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

Ready for lead: head 0c339708308a79c9db4882a83fe56b2624618c73 (my changes at 5f16f8b, plus a normal merge of next at e47681e; #794 had added one assertion in this file).

Ran (Linux, 16 cores)

  • Before the fix, -t "bounded cancellation" with 16 runs in parallel: 5 of 400 runs failed with the hosted signature (read-error 5 s timeout, then toHaveBeenCalledTimes 2 and globalThis.fetch replaced).
  • After the fix, same setup at 5f16f8b: 400 of 400 pass. Whole file with 16 in parallel: 208 of 208 pass at 5f16f8b and 208 of 208 at this merge head (15/15 each, no timeouts or guard errors).
  • Preloads that delay open() by 50 ms or 200 ms before it opens the file, or by 50 ms after it creates the file: 15/15 at this head. The old test fails every time with the 50 ms delay.
  • tsc -p packages/cli (includes tests/) and biome check.
  • Hosted CI run 43667984279: success, both required checks pass. The file ran in Linux test shard 8/8. The change scope did not schedule the macOS shards for this test-only PR, so hosted macOS did not run this file here. Earlier runs 43666802013 and 43666807807 were cancelled when a newer run started on the same head, one of them after I edited the PR body; they are not test failures.
  • Independent Codex reviewer (same thread as the earlier units). Round 5: one P2 (file existence does not prove a read is pending), fixed with a pull() handshake. CLEAN at 5f16f8b and again at this merge head, including feat(native): guide first-use runtime setup #794's assertion.
  • Codex bot: no major issues at 0c33970, and its automatic security review found none. computer pr review-status: CLEAR at 0c33970.

Not verified

  • macOS: your 200-run on the Mac.

Also fixed in this file: the same "one event-loop turn is enough" assumption in "HTTP transfer budget … progress/total" and "five-file … cleanup". At an intermediate head these failed 2 of 191 runs under load (false stall), and they fail with the injected delays. Test-only; no assertion weakened.

@roodboi

roodboi commented Nov 15, 2026

Copy link
Copy Markdown
Contributor Author

Mac result at 0c33970 (Apple silicon): the four "bounded cancellation" tests pass 200 of 200 runs, and the whole file passes 20 of 20. This PR goes into the next combined Mac batch.

🤖 Generated with Claude Code

@roodboi
roodboi merged commit bf74aaf into next Nov 15, 2026
21 checks passed
@roodboi
roodboi deleted the test/hack-2250-download-cancel-bound branch November 15, 2026 05:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant