fix(test): the 504 this test looked for was in a random id

[lidge-jun]

The failure

dev CI went red on run 30967162304 with a single failing test:

(fail) runWithImageBridge > retry wait longer than connectTimeoutMs restarts the header deadline (no 504)
error: expect(received).not.toContain(expected)

The assertion was expect(sse).not.toContain("504"), and the stream it rejected looked like this:

"id":"resp_612a6504e8ea4cf095d0f02d4f828c80"
             ^^^

That stream carried recovered, ended in response.completed, and contained no failure frame. The only 504 anywhere in it was three characters of a random identifier.

About one 32-hex id in 137 contains 504, and a stream carries two of them, so this reddened roughly one run in 69 for no reason. Reproduced locally: 1 failure in 37 runs, which matches.

The larger half

The flake is not the interesting part. Removing the deadline re-arm that this test is named after left it green:

Mutation in src/images/loop.ts	Old assertion	New assertion
drop the post-wait re-arm	passes	fails
drop the pre-wait clear()	passes	passes (see below)
drop both — the real cumulative-deadline regression	passes	fails

Two reasons it could not work. The mock adapter ignored ctx.abortSignal and answered 200 no matter what the deadline said. And a genuine first-iteration timeout is answered eagerly with an HTTP 504 whose JSON body does not contain the number 504 at all — so even a real one would have slipped past a substring search.

So the regression this test exists to prevent was never actually guarded.

The change

tests/images/loop.test.ts only. No production code.

• the mock now calls ctx.abortSignal?.throwIfAborted(), so the deadline means something
• expect(response.status).toBe(200) plus the content type, checked before the body is consumed, because an eager 504 never produces a stream to inspect
• terminal events by name (event: response.completed present, event: response.failed absent) instead of a substring hunt
• signal identity captured at fetch time, which is the only way to tell a fresh deadline from the disarmed remains of the old one — neither expires, so nothing else distinguishes them

tests/web-search.test.ts has a test of the same name proving the same thing, and it was already written this way. It passed in the same CI run.

What is deliberately not claimed

Dropping only the pre-wait clear() still passes. That call guards a race between a stale expiry and the client-cancel path, which this test does not exercise. Saying it is covered would be the same kind of overclaim this PR is fixing, so it is left unclaimed and noted in the commit message.

Verification

• remote Linux suite: 8412 pass / 0 fail / 10 skip across 540 files
• bun x tsc --noEmit: clean
• the fixed test: 30 consecutive runs, 0 failures (it fails about 1 in 37 before)
• mutation results in the table above, each restored to a clean tree afterwards

Summary by CodeRabbit

• Tests
  • Strengthened coverage for connection retries and timeout handling.
  • Added validation that each retry receives a fresh, active timeout signal.
  • Improved checks for response status, streaming headers, terminal events, and successful retry completion.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 833e0721-174f-486e-8c7e-8f67df576e2c

📥 Commits

Reviewing files that changed from the base of the PR and between 8608b18 and 479e537.

📒 Files selected for processing (1)

• tests/images/loop.test.ts

---

📝 Walkthrough

Walkthrough

The retry regression test now verifies that each fetch attempt receives a distinct, live abort signal. It also validates the SSE response status, headers, successful completion, and absence of a failure event.

Changes

Retry deadline validation

Layer / File(s)	Summary
Retry signals and response assertions tests/images/loop.test.ts:254-274, tests/images/loop.test.ts:292-326	The test captures abort signals for both attempts, rejects expired deadlines, and verifies distinct un-aborted signals. It replaces "504" substring matching with SSE status, header, completion, and terminal-event assertions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: ingwannu, harryzhou2000, wibias

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes the flaky 504 assertion fixed in tests/images/loop.test.ts, which is a central change in the pull request.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/loop-test-504-collision

---

_{Comment @coderabbitai help to get the list of available commands.}