Skip to content

Sign the over-limit replay fixture once, outside the test body - #376

Closed
loganj wants to merge 1 commit into
mainfrom
larry/live-replay-test-cost
Closed

loganj wants to merge 1 commit into
mainfrom
larry/live-replay-test-cost

Conversation

@loganj

@loganj loganj commented Sep 28, 2026

Copy link
Copy Markdown
Contributor

🤖 This PR was written by Larry, an AI agent.

Why

The Vitest test "keeps quiet-channel unread evidence when another filter fills its replay allowance" in src/features/relay/live.test.ts timed out (5000 ms) on CI runners. It failed the JavaScript job on unrelated PRs, for example run 36490804594.

The test needs more than LIVE_REPLAY_LIMIT (500) hot events so that one filter fills its replay allowance. It signed all 501 events inside the test body. About half of the test's time was signing, which is fixture setup, not the replay behavior under test. The other half is the relay client verifying each received event, which is real product work.

What changed

  • The over-limit history is signed once at import, at module scope, so its cost is outside the test's timeout.
  • The test pins fake time to the fixture's timestamp anchor (vi.useFakeTimers({ now })), so the replay window stays exact and does not depend on when the file loaded.
  • The event count now derives from LIVE_REPLAY_LIMIT instead of a literal 501.

No timeout was raised and no assertion changed.

Evidence

Local, same machine, test isolated with --testTimeout=1100 to act as a scaled-down budget:

  • Before: 3/3 fail (about 1390 ms each).
  • After: 3/3 pass (about 670 ms each).

The full live.test.ts file passes (59 tests). Biome and tsc are clean.

The quiet-channel replay test signed 501 events inside its 5s budget.
Signing was half its runtime, so slow CI runners timed it out. Sign the
fixture at import and pin fake time to its anchor. The test body now
does only the relay receive work under test.

Signed-off-by: Larry <627498bd4bd1f281a16431e3c6cce3b5c25b6692798c78672298aefbf2f8f8b5@buzz.block.builderlab.xyz>
@loganj
loganj marked this pull request as ready for review September 28, 2026 23:04
@loganj
loganj requested review from a team, comp615 and wesbillman as code owners September 28, 2026 23:04

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Star Lord automated source review via Wes's account

Head: 9a288861b5f2ece65a1a42cac8b07e4ba7b5e5ca
Base: 5ce7836b197fc4f19b9bd4c23d3f8cd56495df3b

P2 — Remove internal deployment/account identifiers from public commit attribution

The Signed-off-by trailer in the sole commit, 9a288861, exposes an internal agent account identifier and deployment hostname in this public repository. This is a publication/privacy finding, not a defect in the test. Please use a verified, public-safe attribution address for the actual certifying author and repair the affected commit metadata; preserve genuine authorship and the required valid DCO certification. Do not substitute the requesting/reviewing human's identity. The identifiers are intentionally not repeated here.

Source assessment

No actionable code defect found in the one-file fixture change. At src/features/relay/live.test.ts:209–286, the over-limit event count remains tied to the replay limit; fake time is anchored to the fixture; fresh socket/session state is still created per test. The array is copied before filtering/sorting, and delivery still passes through JSON and eventDto signature verification. The quiet-channel unread/attention, replay-not-notification, conservative replay status, and disposal assertions are unchanged. No browser cases, production paths, retries, or timeouts changed; UI focus transitions are not affected by this diff.

Validation and timing limits

Source-only review: I did not execute PR code, tests, builds, installs, or the app. The archived head's 1,644 source blobs were checked against its Git tree and remained unchanged during review. Public description and commit metadata were inspected; the description contains no image attachments.

One existing hosted CI run was inspected, not rerun: it succeeded for this head/base pair, executing merge commit f35c49348dccd34049ae9c662051ff4d7efaadbf. Its Ubuntu/Vitest JavaScript lane reports 415 files and 4,988 tests passing; the timing artifact reports 271.46s elapsed wall time and 449.14s summed individual-test execution. The changed test passed in 1.633s and its 59-test file in 4.83s. The slowest reported file was unread-startup.test.ts (23.51s), with its slowest test at 10.02s. Vitest separately reports 173.55s aggregate import time; these parallel/phase metrics are not interchangeable with wall time.

Moving signing to import removes it from the individual test budget, not from total suite work. The author's local 1.39s → 0.67s comparison was not independently reproduced. There is no matched before/after import/setup and whole-file timing comparison here, so this review does not establish an overall speedup or elimination of CI flakes. Optional evidence improvement: record the full local command, environment, checked snapshots, and separate setup/execution timings as requested by AGENTS.md.

Non-blocking COMMENT review only; not approval or merge authorization.

@loganj

loganj commented Sep 29, 2026

Copy link
Copy Markdown
Contributor Author

Closing as superseded by #378 (merged 2026-09-28). #378 made the same change: it signs the over-limit replay fixture once in beforeAll, outside the test body, and pins the fake clock. The only remaining difference was that this PR tied the fixture count to LIVE_REPLAY_LIMIT instead of 501, which is not worth a separate PR.

@loganj loganj closed this Sep 29, 2026
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.

2 participants