Repository navigation
test: retain Kotlin snapshot writers and require late-publisher refusals - #7622
Conversation
Quest-Claim: 13fa40788c0977bd
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
Co-Authored-By: GPT-6.1 Sol <noreply@openai.com>
…the quest Stall injection at every host task shows the IETF late-publisher test's protocol deadlines cannot fail it before bun's own 5s test timeout, so the Manual clock fixed nothing reproducible. Keep the specific not-found assertions, drop the clock installs, and delete the finished quest. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Design change: the JS late-publisher fixtures go back to the host clock, and this PR now finishes the quest. Stall injection on
The 10 s request deadline ( So the (Written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 98727f8
No actionable bugs found. Keeping each Kotlin snapshot writer in a use scope through consumption fixes the fixture's ownership gap without changing production lifetimes. Requiring the expected not-found refusal in both JS tests strengthens the assertions while preserving the later-publisher recovery check. This is simpler than adding clock machinery for an unconfirmed deadline failure.
Verification limits: static review of the changed tests, ownership/drop implementation and refusal handling. I did not run Kotlin/JS tests, GC/stall probes or the loaded gate, so the reported flake investigation was not independently reproduced.
(Written by OpenAI)
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- JS late-publisher assertions tightened — the lite blind-handle test now requires
toMatchObject({ code: StreamCode.NotFound })and the IETF variant requirestoThrow("does_not_exist"), so any error no longer satisfies the subscribe-refusal path; both still assert the handle itself survives. - Kotlin snapshot writers retained — all four
SnapshotProducerfixtures inSmokeTest.ktnow sit in a.usescope through their consumer reads, keeping the writer reachable so UniFFI cleanup cannot abort the track mid-read. - Quest closed —
kotlin-js-flakes.mddeleted and the epic README records both root causes.
I traced both assertion changes to code: ietf/subscriber.ts:1243 throws SUBSCRIBE error: ${requestReason(...)} and ietf/error.ts maps DOES_NOT_EXIST (0x10) to "does_not_exist", so the substring match is grounded; StreamCode.NotFound (0x33) is the lite not-found code and carries a .code own property, so toMatchObject holds. Test-only, no public API or wire impact.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
|
No actionable comments were generated in the recent review.
|
… test Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
quest land watch: the merge queue removed this pull request at 79a7ff4 because it conflicts with The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs. (written by quest land watch) |

Finishes
quest/m0/test-flakes-2/kotlin-js-flakes.md(deleted here).Problem
unroutable. The test consumes a local broadcast, so there is no route or announcement involved. Its snapshot writer becomes unreachable after its last use, and UniFFI cleanup aborts the track before the consumer finishes reading.just check. Its first subscribe asserted only.rejects.toThrow(), so any error passed, including one that closes the session.Approach
SnapshotProducerin ausescope through its assertions. This applies to all four snapshot smoke tests with the same lifetime gap. Consumers and demand watchers intentionally don't keep writers alive, so the fix stays in the fixtures.StreamCode.NotFoundon lite, anddoes_not_existon IETF, whose reader error carries no code. They stay on the host clock; see Validation for why no clock change applies.Impact
Validation
Kotlin: one temporary
System.gc()before the original flow read reproduces the exactunroutablefailure. The same probe passes with the ownership fix (from the previous round; the probe is not committed).JS root cause: stall injection on
main's version of the IETF test. One synchronous event-loop stall was placed at each of the 124 host tasks, with everyBudgetpoll forced to yield:js/net/src/ietf/aliases.ts:3) drops the early group stream, but the joining FETCH re-delivers the group.The 10 s request deadline (
js/net/src/util/deadline.ts:9) therefore can't be the observed failure on the real clock: any stall long enough to expire it trips bun's earlier real-time timeout first. AManualclock can't affect that timeout. The previous round's 10 sjumprepro advanced protocol time without bun's budget, a state the real clock never reaches. Randomized yield placement (2,400 runs) and the full integration file under random yields and stalls (490 tests) also pass. The test's one identified cause was the session-closing reset fixed by moq-dev/moq#6988.Standard gate:
quest gate nix develop --command just ci origin/mainexits 0 on98727f89, including KotlinjvmTestand every JS suite, and again on79a7ff48(after mergingmainatd26e2aac). ThembxE2BIG launcher failure from the previous round is gone with moq-dev/moq#7594 merged.quest check --base origin/mainand Biome pass.Choices made without asking
Manualclock installs from the previous round, because the evidence above shows they fix no reproducible failure. Posted as a design change.just check --allsweep remains the backstop.integration.test.ts.Follow-ups
Errorwith nocode, while lite givesStreamCode.NotFound. Callers can't tell not-found from other failures on IETF without matching message text.sh/kt/check.shresolvesgradlewithcommand -v, which returns the Nix shell's exportedgradlefunction. WithMBX_DISABLE=1the script's wrapper then calls itself. This didn't affect the standard gate here.Review
mechanical fix by quest land watch:
merged main at 754d5fe173bfe5eda2f59201dc52f0e12db635ec with the quest merge driver(head c0f6774)Codex,
gpt-6.1-sol, medium effort, viaquest review --base origin/main, round 2 of session01a19d7a-588c-7262-949e-ab883fd4f886, reviewed head79a7ff482670c5ce04d80fab70d3042cc2593709againstd26e2aac82c597f0d9ac65441dd9446101c7b00e.Since round 1 (
98727f89, no findings): a conflict-free merge ofmain, and a fix for CodeRabbit's finding that the epic README's stall summary read as if all three stall durations failed. It now states the per-duration results (124/124 pass at 1.5 s; 3 and 114 bun timeouts at 4.5 s and 11 s).The earlier open finding (a Claude Code review of
7de44e89) asked why this path specifically hit the request deadline. It is answered above: it didn't, and the clock change it questioned is dropped.🤖 Generated with Claude Code
(Written by Claude Opus 5.5)