fix(pty): stabilize non-ASCII output round-trip test - #12848
Merged
Conversation
The round-trips non-ASCII output test printed the fixture and exited immediately. On loaded CI runners bun-pty can observe the child-exit End message before its reader thread has drained the last output bytes, dropping the trailing UTF-8 marker and timing the test out. Keep the child alive after printing (printf ...; sleep 5), matching the sibling explicit-arguments test, so output is fully consumed before the process can exit. Also include the received output in the waitForOutput timeout error to make any future failure self-diagnosing.
marius-kilocode
marked this pull request as ready for review
August 4, 2026 10:22
marius-kilocode
enabled auto-merge
August 4, 2026 10:22
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (1 file)
Reviewed by kimi-k3 · Input: 88K · Output: 5K · Cached: 290.7K Review guidance: REVIEW.md from base branch |
johnnyeric
approved these changes
Aug 4, 2026
This was referenced Aug 5, 2026
t7tran
pushed a commit
to t7tran/kilocode
that referenced
this pull request
Aug 14, 2026
fix(pty): stabilize non-ASCII output round-trip test
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
pty > round-trips non-ASCII output byte-identicallytest intermittently fails on the macOS unit CI job with a 15s timeout waiting forcafé-über-北京-🚀. Previous mitigations (bumping the wait timeouts, feeding replayed output into the wait helper, and switching the fixture to raw octal escapes) made the test slower or more tolerant without removing the underlying race, so it kept flaking.Root cause
The fixture command prints the marker and exits immediately:
The Bun runtime PTY driver (
packages/core/src/pty/pty.bun.ts, backed bybun-pty) bridges a Rust reader thread to the JS event loop. The Rust side emitsMsg::Datachunks and, once the child exits,Msg::End. When the JS read loop observesMsg::End, the RustReaderwaits only 20ms for trailing data before reporting child exit; anything the reader thread pushes later is discarded.On a loaded CI runner the reader thread can be scheduled more than 20ms behind the wait-thread that signals the child exit, so the final output bytes never reach the test. This test's marker sits at the very end of the output, so
waitForOutputruns out its 15s timeout. The flake hits this test and not its siblings because every sibling either keeps the child alive after printing (printf ...; sleep 5) or has no output that can be lost.Fix
Keep the child alive until the test is torn down, matching the sibling
preserves explicit command argumentstest:While the child is alive no
Msg::Endis produced, so the output is always fully consumed before the process can exit and the race window cannot exist. Teardown already kills the process group (Pty.remove->KiloPtyTermination), so the test still completes in milliseconds and leaks no processes. The byte-identity assertion is unchanged; only the exit timing is decoupled from output delivery, which the test never meant to be sensitive to.waitForOutputnow includes the received output in its timeout error, so any future failure is self-diagnosing (for example a truncated tail shows exactly which bytes were dropped).Why this is not flaky anymore
sleep 5pattern has never flaked since these tests were introduced.CI verification
The first CI attempt's macOS unit job failed on an unrelated, pre-existing flake (
v2 pty HttpApi > applies plugin shell environment before forced PTY values, a different test in the CLI package that already usessleep 5and a 30s timeout). It passed on re-run, and the full workflow is green with this fix in place.