Skip to content

test: retain Kotlin snapshot writers and require late-publisher refusals - #7622

Merged
moq-bot[bot] merged 10 commits into
mainfrom
quest/m0/test-flakes-2/kotlin-js-flakes
Nov 2, 2026
Merged

moq-bot[bot] merged 10 commits into
mainfrom
quest/m0/test-flakes-2/kotlin-js-flakes

Conversation

@kixelated

@kixelated kixelated commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Finishes quest/m0/test-flakes-2/kotlin-js-flakes.md (deleted here).

Problem

  • Kotlin: "cancelling a flow collection cancels its native handle" fails intermittently with 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.
  • JS: "integration: ietf blind handle picks up a publisher that arrives late" failed under a loaded just check. Its first subscribe asserted only .rejects.toThrow(), so any error passed, including one that closes the session.

Approach

  • Keep each Kotlin SnapshotProducer in a use scope 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.
  • Both late-publisher tests (lite and IETF) now require the specific missing-publisher refusal: StreamCode.NotFound on lite, and does_not_exist on IETF, whose reader error carries no code. They stay on the host clock; see Validation for why no clock change applies.
  • Delete the finished quest. The epic README records both findings.

Impact

  • Public API: none.
  • Wire: none. Production deadlines, cancellation, and ownership are unchanged.

Validation

  • Kotlin: one temporary System.gc() before the original flow read reproduces the exact unroutable failure. 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 every Budget poll forced to yield:

    • 1.5 s: 124/124 pass. The 1 s track-alias wait (js/net/src/ietf/aliases.ts:3) drops the early group stream, but the joining FETCH re-delivers the group.
    • 4.5 s: 121 pass, 3 hit bun's 5 s test timeout.
    • 11 s: 114 hit bun's 5 s test timeout, 10 pass, and none fails with ControlTimeout or any other error.

    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. A Manual clock can't affect that timeout. The previous round's 10 s jump repro 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/main exits 0 on 98727f89, including Kotlin jvmTest and every JS suite, and again on 79a7ff48 (after merging main at d26e2aac). The mbx E2BIG launcher failure from the previous round is gone with moq-dev/moq#7594 merged.

  • quest check --base origin/main and Biome pass.

Choices made without asking

  • Dropped the Manual clock installs from the previous round, because the evidence above shows they fix no reproducible failure. Posted as a design change.
  • Finished the quest rather than keeping it open for the JS test. The original 2026-10-31 failure text was not recorded anywhere this session could find (land runs are local). The epic's own loaded just check --all sweep remains the backstop.
  • Inspected moq-dev/moq#7485 and moq-dev/moq#7597: both edit other blocks of integration.test.ts.

Follow-ups

  • An IETF SUBSCRIBE refusal reaches the reader as a plain Error with no code, while lite gives StreamCode.NotFound. Callers can't tell not-found from other failures on IETF without matching message text.
  • sh/kt/check.sh resolves gradle with command -v, which returns the Nix shell's exported gradle function. With MBX_DISABLE=1 the 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, via quest review --base origin/main, round 2 of session 01a19d7a-588c-7262-949e-ab883fd4f886, reviewed head 79a7ff482670c5ce04d80fab70d3042cc2593709 against d26e2aac82c597f0d9ac65441dd9446101c7b00e.

  • No findings.

Since round 1 (98727f89, no findings): a conflict-free merge of main, 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)

kixelated and others added 4 commits November 2, 2026 03:27
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>
@kixelated kixelated changed the title A Kotlin cancel test and a JS late-publisher test hold up test: retain Kotlin snapshot writers and pause late-publisher time Nov 2, 2026
kixelated and others added 3 commits November 2, 2026 09:32
…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>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Design change: the JS late-publisher fixtures go back to the host clock, and this PR now finishes the quest.

Stall injection on main's version of the IETF test (one synchronous event-loop stall at each of the 124 host tasks, with every Budget poll forced to yield):

  • 1.5 s stall: 124/124 pass. The 1 s track-alias wait (js/net/src/ietf/aliases.ts:3) does drop the early group stream, but the joining FETCH re-delivers the group.
  • 4.5 s stall: 121 pass; 3 hit bun's own 5 s test timeout.
  • 11 s stall: 114 hit bun's 5 s test timeout, 10 pass; none fails with ControlTimeout or any other error.

The 10 s request deadline (js/net/src/util/deadline.ts:9) can't be the observed failure on the real clock: any stall long enough to expire it trips bun's earlier real-time test timeout first. A Manual clock can't affect that timeout. The earlier 10 s jump repro advanced protocol time without advancing 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.

So the Manual installs are dropped. The specific not-found assertions stay: the old .rejects.toThrow() accepted a session-closing error. The quest is deleted, and the epic README records the finding. Its loaded just check --all sweep remains the backstop if the test fails again.

(Written by Claude Opus 5.5)

@kixelated kixelated changed the title test: retain Kotlin snapshot writers and pause late-publisher time test: retain Kotlin snapshot writers and require late-publisher refusals Nov 2, 2026
@kixelated
kixelated marked this pull request as ready for review November 2, 2026 18:22

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

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)

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ 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 requires toThrow("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 SnapshotProducer fixtures in SmokeTest.kt now sit in a .use scope through their consumer reads, keeping the writer reachable so UniFFI cleanup cannot abort the track mid-read.
  • Quest closed — kotlin-js-flakes.md deleted 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.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a494e51a-2ce8-409f-b73d-4d6cd0b4835d
Commits

Reviewing files that changed from the base of the PR and between 98727f8 and c0f6774.

Files selected for processing: 1
  • quest/m0/test-flakes-2/README.md
Files skipped from review as they are similar to previous changes: 1
  • quest/m0/test-flakes-2/README.md

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 0 remain after this review.

Walkthrough

Kotlin tests now scope SnapshotProducer instances with use. JavaScript blind-handle tests now assert specific missing-broadcast errors. The flake README explains the identified Kotlin and JavaScript failure causes and removes the Kotlin/JS item from the required list. The related planning document was deleted.

Merge Risk: Minimal · up to c0f67

The stall results are now stated accurately. No actionable merge-blocking issue remains.

Pre-merge checks: Passed 5
Check name Status Explanation
Title check Passed The title clearly summarizes the main changes: retaining Kotlin snapshot writers and requiring specific late-publisher refusal errors.
Description check Passed The description directly explains the Kotlin and JavaScript test changes, their causes, validation results, and documentation updates.
Docstring Coverage Passed No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat…
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.
Full details: Docstring Coverage

Explanation

No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files. (1 skipped: 1 unsupported.)

Finishing Touches
Simplify code
  • Commit to this branch
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

Share

@coderabbitai coderabbitai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Fix comments: Fix CodeRabbit comments on this PR

Comment thread quest/m0/test-flakes-2/README.md Resolved
@kixelated
kixelated added this pull request to the merge queue Nov 2, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 2, 2026
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at 79a7ff4 because it conflicts with main (merge_conflict).

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)

@moq-bot
moq-bot Bot enabled auto-merge November 2, 2026 21:10
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown
Contributor

quest land watch: armed auto-merge on this pull request at c0f6774, so it joins the back of the merge queue once its required checks pass, with main at 754d5fe merged in by the quest merge driver.

(written by quest land watch)

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 2, 2026
Merged via the queue into main with commit 2ba6d46 Nov 2, 2026
8 of 10 checks passed
@moq-bot
moq-bot Bot deleted the quest/m0/test-flakes-2/kotlin-js-flakes branch November 2, 2026 22:19
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.

1 participant