A paste the test delivers in two writes is not a paste - #414
Conversation
`aid/tests/interactive.rs` failed on a loaded `cargo test --workspace` about one run in seven, and always on the same assertion: the pasted prompt reached the agent cut at its first line. #401 offered three candidate mechanisms -- a lost wakeup between the pty reader thread and `wait_for`, a reader thread that died early, or 60s genuinely elapsing under contention. It is none of them, and the deadline is left alone. `send_line` wrote the line and its Enter as two `write_all` calls on the pty master. A master write is copied into the line discipline in one go, so two writes are two of those with a schedulable gap between them. In `a_pasted_multi_line_prompt_arrives_whole_rather_than_leaking` the first write carries `fix this\nand then that`, which completes line one and nothing else, and that alone is enough to wake `aid`'s read. `read_terminal_submission` then drains what the terminal holds right now -- by design, since its zero-timeout poll is what keeps somebody who typed one line from being waited on for a second -- and while the second write is still in flight the tail is not a completed line, so canonical mode will not hand it over, so the prompt is `fix this`. The test's own comment claimed "one write, as a terminal delivers a paste"; the helper it called had never made that true. Confirmed rather than argued. Widening the gap to a deliberate 20ms sleep between the two writes moved the failure from 14 runs in 100 to 19 in 20, which is the mechanism named and nothing else. One write makes every completed line of the paste readable at the same instant, which is also what a terminal delivering a paste actually does. Measured on 8 cores under a load average of 15 to 21, running the suite binary back to back: before: 86 green in 100, 14 failures, every one this assertion after: 100 green in 100 The 60s deadline in `wait_for` stays. The symptom #401 recorded, `"press Enter" never appeared`, did not reproduce once in 280 runs of the suite; raising a deadline for a mechanism nothing confirmed would buy only a slower failure. Three other flakes surfaced in the full-workspace runs and belong to their own tickets, not this one: `flows::launch::tests::a_contended_up_of_a_running_workspace_runs_no_up_at_all`, `timing::tests::stages_closed_out_of_order_do_not_stay_on_the_clock`, and `dl/tests/interrupt.rs`'s `closing_the_terminal_mid_up_removes_the_token_file_and_kills_the_up`.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFixes the flaky multi-line paste test by making the pty helper model a real paste: the complete input is copied into the terminal line discipline in one write, preventing File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
blooop
left a comment
blooop
left a comment
There was a problem hiding this comment.
This was generated by AI during review.
Reviewed at merge-base 57955a3...5f9d40c. Every number below was measured in a scratch worktree in this devcontainer, idle apart from the run.
Preflight: ci, rust, e2e, prek, public-api, packaging, coverage green. review/gate are red only for the missing wf-review report, not for the code.
Standards
The mechanism argument is sound, and I reproduced it rather than agreeing with it.
Reading read_terminal_submission (rust/dl/src/lib.rs:265-295) confirms the shape the PR describes: bytes are read one at a time up to the first \n, and only then does if ended_with_newline && stdin_readable_now() drain the tail. stdin_readable_now is a zero-timeout poll(POLLIN), and in canonical mode a line discipline reports readable only for a completed line — so a first write that ends mid-line-two leaves the tail invisible to that poll, and the submission is line one. The mechanism is exactly as claimed.
Reproduced by knob, on this branch, restoring the two writes with a deliberate 20ms gap between them:
test a_pasted_multi_line_prompt_arrives_whole_rather_than_leaking ... FAILED
left: "... claude --dangerously-skip-permissions '\"'\"'fix this'\"'\"''"
right: "... claude --dangerously-skip-permissions '\"'\"'fix this\nand then that'\"'\"''"
And the natural rate, on an otherwise idle machine (the PR measured under load average 12-25, so these are not comparable to its 86/100 — they are a floor):
| helper | runs | failures |
|---|---|---|
| two writes, no sleep (base) | 60 | 2, both the paste assertion |
| this PR's one write | 40 | 0 |
The fix changes only test code. The diff is rust/aid/tests/interactive.rs and nothing else. Confirmed.
The most important question — does the test still test what it claims? Yes, CONFIRMED. The concern is that a test rewritten to match the implementation can go green because it stopped asking anything. It did not. I neutered the production drain on this branch — rust/dl/src/lib.rs:287, if ended_with_newline && ... → if false && ended_with_newline && ... — and the test failed with precisely the truncation it asserts against:
thread 'a_pasted_multi_line_prompt_arrives_whole_rather_than_leaking' panicked at aid/tests/interactive.rs:302:5
left: "... '\"'\"'fix this'\"'\"''"
right: "... '\"'\"'fix this\nand then that'\"'\"''"
So the production drain path — the zero-timeout poll, the re-pushed \n, the byte loop — is genuinely exercised, and a regression in it still fails this test. That is the whole of what the change had to preserve, and it does.
Findings, both non-blocking:
- SUSPECTED — the fix narrows the window rather than closing it by construction. The test still depends on the kernel pushing all 23 bytes through the line discipline before
aid's poll runs. In practice a single smallwrite(2)on a pty master does that under oneatomic_write_lock(0/40 here, 0/100 in the PR), but it is an empirical property, not one the test asserts. I would not act on it. - SUSPECTED — a coverage loss worth a line in #416, not a change here. The two-write helper was, by accident, the only thing in the suite exercising a paste that arrives split.
read_terminal_submission's own doc says an unterminated tail "stays queued, and reaches the agent's session as typed-ahead input" — which is real user-visible behaviour for a paste larger than one write (N_TTY_BUF_SIZE, or a terminal that chunks). After this PR nothing covers it. The PR's reason for leavingread_terminal_submissionalone is right for #401's scope; the gap is worth naming somewhere.
Nit, no action: format!("{line}\n") allocates per call. Irrelevant in a test, mentioned only so the next reader does not re-derive it.
Spec
Against #401.
- "Worth establishing first whether the deadline is simply too tight under load, or whether there is a genuine lost-wakeup" — established as neither, with the alternative named and rejected on evidence rather than on preference. This is what the ticket asked for and the PR delivers it in the order the ticket set.
- "Options in rough order of cost: raise the deadline, serialise the pty tests, or hand the reader thread a condvar" — all three declined, each with a stated reason. Declining a ticket's suggested options after finding the actual cause is the right outcome, not a deviation.
- "The failure seen was
a_pasted_multi_line_prompt_arrives_whole_rather_than_leaking(aid/tests/interactive.rs:315), with"press Enter" never appearedafter the 60s deadline" — this is the one place the PR does not close the ticket as written. The test named is the one fixed; the symptom recorded is not the one reproduced or fixed, and the PR says so plainly and carries it to #416. That is the honest disposition, but it means #401 is only partly discharged here: if the recorded symptom was real, its cause is still unknown. Whoever closes #401 should close it against #414 and #416, not against #414 alone. Flagging for the human, not as a change to this diff.
Verdict
Approve. No blocking findings. The mechanism, the fix, and — the thing most worth checking — the test's continued grip on the production drain path are all confirmed by re-running rather than by reading. Two follow-ups suggested for #416: the split-paste coverage gap, and the disposition of #401's original symptom.
1 participant
Closes the diagnosis in #401. Map: #406.
What was actually wrong
Not a lost wakeup, not a dead reader thread, not a deadline that is too tight.
send_lineinrust/aid/tests/interactive.rswrote the typed line and its Enter as twowrite_allcalls on the pty master:A write to a pty master is copied into the line discipline in one go, so two writes are two of those with a schedulable gap between them. In
a_pasted_multi_line_prompt_arrives_whole_rather_than_leakingthe first write carriesfix this\nand then that, which completes line one and nothing else, and that alone wakesaid's blocking read.read_terminal_submission(rust/dl/src/lib.rs:265) then drains what the terminal holds right now by design: its zero-timeoutpollis what keeps somebody who typed one line from being waited on for a second. While the second write is still in flight the tail is not a completed line, canonical mode will not hand it over, and the prompt reaching the agent isfix this.The test's own comment claimed "One write, as a terminal delivers a paste: both lines arrive together". The helper it called had never made that true.
How the mechanism was confirmed
Not by argument. Widening the gap between the two writes with a deliberate 20ms sleep moved the failure rate from 14 in 100 to 19 in 20 - the named mechanism and nothing else responds to that knob.
Numbers
Method: run the built
interactivetest binary back to back, 100 times, on 8 cores under a load average of 12 to 25 (other agents were runningcargo test --workspacein sibling worktrees throughout, which is the loaded condition #401 describes).interactive.rs:291(paste truncated to line one)An earlier 60-run baseline agreed: 52/60 green, all 8 failures the same assertion.
What was rejected
wait_for. Left untouched. The symptom The aid pty tests flake on a loaded cargo test --workspace run #401 recorded,"press Enter" never appeared, did not reproduce once in 280 runs of this suite, nor in any of the 6 full-workspace runs measured. Raising a deadline for a mechanism nothing has confirmed buys only a slower failure.read_terminal_submission. Its drain-what-is-queued contract is deliberate and documented, and a real terminal delivering a paste writes it whole. The test was the thing that was wrong.Gate
pixi run cargo test --workspacegreenpixi run cargo clippy --locked --all-targets -- -D warningscleanpixi run cargo fmt --checkcleanOut of scope, seen while measuring
Three other tests failed intermittently in the full-workspace runs, none of them in this file, and none touched here:
flows::launch::tests::a_contended_up_of_a_running_workspace_runs_no_up_at_all(Ok(Started)whereOk(SkippedSiblingWon)was expected)timing::tests::stages_closed_out_of_order_do_not_stay_on_the_clock(twice)dl/tests/interrupt.rs'sclosing_the_terminal_mid_up_removes_the_token_file_and_kills_the_upAlso noted and not acted on:
a_ctrl_c_at_the_editor_tears_the_whole_boot_downstakes its "the orphaneddevpod upmust have been killed" assertion on await_forwhose subject is asleep 30. A kill that never happened still satisfies it 30 seconds later, which is where the occasional 30s run of this suite comes from. It is a separate weakness and a separate ticket.Summary by Sourcery
Fix the interactive paste test to send a line and its newline atomically, preventing multiline input from being truncated under load.
Bug Fixes:
Tests: