test(agent-runs): stop a success-path assertion racing a 10ms deadline - #3710
Conversation
"successful strict project file calls dispose cancellation listeners and timers" built a client with operationTimeoutMs/timeoutMs of 10 around a stubbed fetch, then asserted the abort listener was added once and removed once. Nothing in it tests timeout behaviour; the deadline was incidental config that made a success-path assertion depend on wall clock. Under a loaded parallel run the deadline can elapse before the call settles, the timeout path runs, and the counts stop being 1/1. Raised to 30s. The tests in this file that do exercise deadlines keep their short ones -- those need the timeout to fire, and contention only makes them fire more reliably. Honest about what this does not establish: I could not reproduce the failure in isolation even under 12 saturating busy-loops (0/6), so the mechanism is plausible rather than proven, and the suite's failing test varies between runs. Four full-suite runs passed afterwards, but against a ~1-in-3 baseline that is a regression check, not evidence of a fix. Context in veryfront-issue-inbox#506.
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe cancellation-listener cleanup test now uses 30-second operation and request timeouts. Its fetch behavior and listener-disposal assertions remain unchanged. ChangesTest stability
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This localized test change removes an unrelated wall-clock dependency from a success-path assertion without changing production behavior. No actionable merge-blocking risk remains beyond normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
What
"successful strict project file calls dispose cancellation listeners and timers"built its client withoperationTimeoutMs: 10/timeoutMs: 10around a stubbed fetch, then asserted the abort listener was added once and removed once.Nothing in that test is about timeouts. The deadline was incidental config that made a success-path assertion depend on wall clock: under a loaded parallel run the 10ms can elapse before the call settles, the timeout path runs, and the counts stop being 1/1.
Raised to 30s, with the reasoning in a comment so it is not "tidied" back down.
What this deliberately does not change
The other 12 short deadlines in this file stay. Tests named "bounds a stalled terminal response" or "enforces its deadline after trace" need a 1ms budget — that is the behaviour under test, and contention only makes those fire more reliably, which is the outcome they assert.
That distinction matters: sweeping every short deadline in the suite would break a dozen legitimate timeout tests to chase a handful of real ones.
Honest status
This is a plausible fix for one instance, not a proven fix for the suite:
What it does establish: this test no longer depends on wall clock for an assertion that has nothing to do with time. That is correct regardless of whether it was the cause.
deno lintanddeno fmt --checkclean; the file passes 75/75.Background, including a correction to my own earlier over-count of the affected sites: veryfront-issue-inbox#506.
Summary by CodeRabbit