fix: make artifact inventory consumption deterministic on download

[thymikee]

Problem

The unit test daemon artifact downloads can keep the source file while consuming the inventory entry (src/daemon/__tests__/http-server-artifacts.test.ts) fails flakily on CI, always in the slower instrumented Coverage job, on PRs unrelated to the artifact code:

• feat: accept tap as an alias of press #1053 (flagged in the PR comment thread)
• feat(interaction): opt-in --verify evidence for press/click/fill #1064 — run 28655306400 attempt 1 failed the Coverage job with this exact assertion, then passed on re-run

Failure: after downloading an artifact, GET /artifacts still lists the consumed artifact id (expected false, received true at line 261).

Root cause — a production race, not a test bug

In handleArtifactDownload (src/daemon/downloadable-artifact-http.ts), the inventory entry was consumed only from the source stream's 'end' / response 'finish' / 'close' handlers.

But the response carries an explicit content-length. The client's fetch body completes as soon as content-length bytes arrive — i.e. right after the final data chunk is written to the socket. At that point the server's fs.createReadStream still needs one more threadpool read() to detect EOF and emit 'end'. Under CPU contention (coverage instrumentation, busy CI runners) that threadpool completion can land after the client's follow-up GET /artifacts, which then still lists the consumed artifact.

This is a real client-observable contract violation, not just a test-timing issue: any client that finishes a download and immediately lists the inventory can transiently see the already-consumed artifact. The two sibling "consume" tests only dodge it by polling for the file-deletion side effect (waitFor(() => !fs.existsSync(...))); the retain-file variant keeps the file, so it had no signal to wait on — it exposed the race directly.

Fix

Consume the inventory entry as soon as the stream has emitted content-length bytes, in a 'data' listener registered before stream.pipe(res) (listeners run in registration order, so consumption happens before pipe writes the final chunk to the client). Any client that observed a complete download is now guaranteed to observe the entry as consumed. The existing 'end'/'finish'/'close' handlers stay as backstops (idempotent via didCleanupArtifact), covering edge cases like a file that shrank after stat. Failure semantics are unchanged: an aborted download still leaves the entry available for retry.

No test changes needed — the test was asserting the correct contract.

Verification

• Reproduced on unmodified main: 1 failure in 20 vitest run --coverage iterations under CPU contention, with the exact CI assertion (http-server-artifacts.test.ts:261, true !== false)
• With the fix: 0 failures in 25 coverage iterations under the same contention, plus 0/27 plain runs
• pnpm typecheck, pnpm lint, pnpm format:check all pass

(While verifying, request-handler-catalog.test.ts > specialized daemon routes... timed out when run alongside four other daemon test files — that reproduces identically on unmodified main and is the known contention flake, unrelated to this change.)

[github-actions]

Size Report

Metric	Base	Current	Diff
JS raw	1.5 MB	1.5 MB	+54 B
JS gzip	479.6 kB	479.6 kB	+15 B
npm tarball	581.1 kB	581.1 kB	+13 B
npm unpacked	2.1 MB	2.1 MB	+54 B

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	21.8 ms	22.1 ms	+0.3 ms
CLI --help	38.4 ms	37.8 ms	-0.6 ms

Top changed chunks:

Chunk	Raw diff	Gzip diff
dist/src/9722.js	+54 B	+15 B

[thymikee]

Reviewed by running. Verified independently:

• Mechanism confirmed in code: the data listener is registered before stream.pipe(res) in the same tick, so Node's listener-order guarantee runs the consume before pipe writes the final chunk — a client that observed content-length bytes cannot see stale inventory. The end/finish/close handlers remain as idempotent backstops (empty artifacts, sizeBytes mismatch, delivery-completion), and the consume-on-read-EOF timing matches the old stream.on('end') semantics — this moves it milliseconds earlier within the same contract, so aborted-download/retry behavior is unchanged.
• 15/15 green under CPU contention on the fix branch (6 yes spinners saturating cores), vs the 1-in-20 repro the PR documents on unmodified main.
• One nit on the verification claims in the PR body for the record: single-file --coverage runs fail the global coverage threshold regardless of test outcome, so "coverage-mode iterations" only count if you grep test results rather than exit codes — the test results themselves are what I re-verified.

This also explains the Coverage-only CI failures on #1053 and #1064 (instrumentation slows the threadpool EOF read enough to lose the race). LGTM.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-04 08:25 UTC