Repository navigation
test(net): quarantine two load-sensitive js/net tests - #8287
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 54 seconds. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. |
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed 463f7e6. No actionable findings.
The diff quarantines exactly the two named tests and adds linked, indexed follow-ups requiring cause-based synchronization and removal of the skips. Checked the existing test bodies against the plans: the bandwidth test counts three ticks in 350 ms of Manual time, and the datagram test checks queue creation/priority and oversized-payload sequencing. No runtime implementation changes are included.
Verification limits: static GitHub review only; no loaded-machine reproduction, tests, or quest checks run. The documented scheduling causes remain hypotheses for the follow-up investigations.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor documentation correction inline.
Reviewed changes
- Quarantines both tests:
test.skipreplacestestinbandwidth.test.tsanddatagram-tracks.test.ts, each with a comment pointing at the owning child quest. - Plans both fixes: two new
[XS]children underquest/m0/test-flakes-2/, plus the epic README's 2026-11-08 note and two## Requiredentries. - Verified:
biome checkpasses on both test files,bun teston them reports 45 pass / 2 skip / 0 fail, and theQuestworkflow passed on463f7e6. Nosh/gh/test-inventory.allowentry applies, since that file only covers opt-in Rust#[ignore]and script targets.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
ℹ️ No critical issues — one minor documentation correction inline.
Reviewed changes
- Corrected the datagram quest's provenance:
datagram-queues.mdnow cites#7846as the lite-07 test's origin (verified: merged 2026-11-05, touched the file) and cites a merge-queue drop as the failure evidence. - Merged
origin/main: the branch now carries main's unrelated widened max-age re-drop note; no conflict resolution touched this PR's files.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed c5e80d0 incrementally from 463f7e6, accounting for the main merge. No new actionable findings.
The additional quarantine selects only lite-06 for the session-close-after-datagram-end test; lite-05 and lite-07 remain enabled. Its linked and indexed follow-up accurately identifies the existing 10,000-microtask drain and calls for investigating the end/close ordering rather than increasing the drain or retrying. The datagram-queue plan also corrects the failure attribution. No production behavior changes are included.
Verification limits: static GitHub review only; no tests, loaded-machine reproduction, or quest checks run. The reported failure's underlying cause remains unverified.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Changes since the prior pullfrog review at d30271b:
- Quarantined the lite-06 session-close case:
datagram-tracks.test.ts:473-475selectstest.skiponly forALPN_06viaconst run = alpn === ALPN_06 ? test.skip : test;, leaving the lite-05 and lite-07 cases enabled. - Added
session-close-end.md: a[S]child quest citing the actual failing merge-queue job (128758087035), identifying the pending step (SUBSCRIBE_FINISH, FIN, or the datagram wait) beforeclient.abort(), and requiring an awaited event rather than a longer drain. - Dropped the
#8280misattribution:datagram-queues.mdno longer claims the lite-07 queue test dropped#8280; that evidence now lives insession-close-end.md. - Updated the epic README: the 2026-11-08 note names the lite-06 case, and
## Requiredindexes the new child. - Verified:
bun test src/lite/datagram-tracks.test.tsreports 38 pass / 2 skip / 0 fail (lite-06 skipped, lite-05 and lite-07 pass, so the detachedtest.skipreference behaves), andbun run checkinjs/net(tsc forsrc,examples,bench) is clean. The quest'sSession: remote error: 0citation matchesSessioninjs/net/src/error.ts:151andcloseErroraterror.ts:567.
deepseek-v4.1-flash-1017 (free via Pullfrog for OSS) ｜ 𝕏
The test was quarantined in #8287 as flaky under load. Its suspected cause is an ordering assumption, not a timeout, and I could not reproduce a failure: it passed every run on the un-skipped test (about 6,000, most under heavy CPU load, including forced write-budget yields on every poll and five runs of the whole js/net suite). The quest stays open with the un-reproduced cause written down, and a Waiting item asks the maintainer whether to keep the test un-skipped. Co-Authored-By: Claude Haiku 5.5 <noreply@anthropic.com>
Skips two js/net tests that fail under load, per AGENTS.md's rule to disable flaky tests immediately, and plans each fix as a child of quest/m0/test-flakes-2:
bandwidth.test.ts: "a connection samples the transport only while a reservation reads the estimate" →bandwidth-sampling.mddatagram-tracks.test.ts: "lite-07 bounds and prioritizes independent createWritable datagram queues" →datagram-queues.mdquest checkandjust checkpass locally.(Written by Claude Opus 5.5 with low)
🤖 Generated with Claude Code
Review
Rewritten by
quest reviewafter each round.01a1bc5c-b771-7e71-81dd-2db1645b71a8, round 2The incremental change quarantines only the flaky lite-06 case and documents a scoped investigation, with no actionable defects found; builds and tests were not run as instructed.