Conversation
Describe the LineFramer-based reads (64 KiB per-line cap, invalid UTF-8 escaping, trailing partial-line flush, 128 KiB response cap) in both the Unix and Windows runner specs, and document the Windows bounded channel and why it is needed there but not on Unix. Signed-off-by: wyongzhi <276409147+wyongzhi@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - Thread 1 (main): polls cancel commands and the channel, sends each queued | ||
| line as `{"out":...}`, signals child on cancel | ||
| - Threads 2 and 3: read child stdout and stderr in 8 KiB chunks through the | ||
| shared `LineFramer`, push framed lines to the channel |
There was a problem hiding this comment.
"shared LineFramer" reads as one framer instance shared by both reader threads, but runner_win.rs gives each thread its own: frame_child_output calls LineFramer::new() per invocation (runner_win.rs:164), and it is spawned twice (runner_win.rs:93, :96).
That distinction is load-bearing, not cosmetic. A genuinely shared LineFramer would be a correctness bug: its buf/discarding state is per-stream, so interleaved push calls from stdout and stderr would splice bytes from the two pipes into single lines and corrupt the discarding flag (also it is not Sync-shareable without a lock). The same wording appears in the Step 2 note at line 209.
Suggest "the shared LineFramer type" / "each through its own LineFramer" so a future reader does not try to consolidate them.
| if child done { killpg(child_pgid, SIGKILL); return child.wait().code() } | ||
| // Kill process group first (closes write ends held by group members), | ||
| // then drain buffered output and flush the trailing partial line | ||
| if child done { killpg(child_pgid, SIGKILL); drain; framer.finish(emit); return child.wait().code() } |
There was a problem hiding this comment.
This collapses two exit paths that use opposite orderings, and the "kill process group first" rationale only holds for one of them.
- POLLHUP/POLLERR path (
runner.rs:185-190):drain→framer.finish→child.wait()→killpg→ return. Drain happens beforekillpg, andkillpgcomes afterwait(). That ordering is deliberate and safe (POLLHUP means all writers already closed, so EOF is guaranteed), but it is the reverse of what this line says. try_waitpath (runner.rs:202-207):killpg→drain→framer.finish→ return, using the status already obtained fromtry_wait— no secondchild.wait()call.
So on the POLLHUP path the stated reason ("closes write ends held by group members" so the drain can reach EOF) does not apply at all, and a reader implementing from this pseudocode would move killpg ahead of wait() in the POLLHUP branch. Suggest splitting into the two branches, e.g.
// POLLHUP/POLLERR: writers already closed, so drain then reap
if child hungup { drain; framer.finish(emit); code = child.wait().code(); killpg(SIGKILL); return code }
// try_wait after kill: killpg first so group members close their write ends
if try_wait -> Some(status) { killpg(SIGKILL); drain; framer.finish(emit); return status.code() }
The "Key details" bullet at line 344-346 has the same ambiguity.
| - Main thread joins stdout thread after child exits, then sends `{"exited":...}` | ||
| **I/O multiplexing**: Three threads sharing a bounded channel | ||
| (`sync_channel`, 256 slots): | ||
| - Thread 1 (main): polls cancel commands and the channel, sends each queued |
There was a problem hiding this comment.
Thread count is off by one now that the inventory is being rewritten: Windows actually runs four threads. main.rs:186 spawns a dedicated stdin reader thread that parses commands and forwards CancelMethod over cancel_tx; the main thread never touches stdin — it only try_recvs on cancel_rx (runner_win.rs:104). The diagram box above (line 89) still says the main thread "reads stdin lines", which was already stale and this hunk keeps.
Suggest: "Four threads — main (drains the output channel, try_recvs cancels), stdin reader (parses commands, forwards cancels), stdout reader, stderr reader" and drop "reads stdin lines" from the main-thread box.
| - Threads 2 and 3: read child stdout and stderr in 8 KiB chunks through the | ||
| shared `LineFramer`, push framed lines to the channel | ||
| - After the child exits, main thread drains the channel until both senders | ||
| disconnect, joins the reader threads, then sends `{"exited":...}` |
There was a problem hiding this comment.
The drain is described as unconditionally terminating, but it can block forever, and the new "Output bounds" rationale is what makes that reachable.
runner_win.rs:142 is a blocking while let Ok(line) = out_rx.recv(), which returns Err(Disconnected) only after both reader threads drop their senders — i.e. only after both child pipes reach EOF. On Windows Stdio::piped() handles are inheritable, so any grandchild the child spawned holds a duplicate of the stdout/stderr write end. If such a grandchild outlives the child (the loop broke on child.try_wait() at :123, not on tree death), EOF never arrives: recv() parks forever, child.wait() at :147 is never reached, no exited response is sent, and the loop that was servicing cancel_rx is already gone — so the helper can no longer be cancelled via the protocol from that point on.
The Unix runner documents exactly this hazard rather than claiming termination (runner.rs:198-201: "A grandchild that left the group (setsid) can still hold the pipe open and stall this drain — a pre-existing limitation"), and it at least issues killpg(SIGKILL) on the whole group before draining. The Windows path issues no kill_process_tree on the normal-exit route, so it is strictly more exposed than the Unix one, not equivalent.
Two things worth doing:
- State the bound honestly, e.g. "drains until both senders disconnect — i.e. until both child pipes reach EOF, which a surviving grandchild holding an inherited write handle can delay indefinitely (same limitation as the Unix drain)".
- If the intent is parity with Unix, the spec should say the tree is killed before the final drain; today
kill_process_treeonly runs on the cancel/escalation paths.
1 participant
Follow-up to #366 and #369: brings the cross-user helper specs in line with the bounded output framing those PRs shipped. Docs only, no code changes.
What was the problem/requirement? (What/Why)
specs/windows-cross-user-helper.mdstill described the pre-#369 design: line-by-line reads over an unbounded channel, joining the reader thread before emitting.specs/sessions/embedded-cross-user-helper.mdstill showed the Unix runner reading withread_line, which #366 replaced. Review on #369 asked whether the spec reflects the bounded-channel design choice; it did not.What was the solution? (How)
Update both specs to describe the
LineFramer-based reads: 64 KiB per-line cap, invalid UTF-8 escaped as\xNN, trailing partial-line flush at EOF, 128 KiB response cap. On the Windows spec, add the stderr reader thread and the 256-slot bounded channel, and state why it is needed there and not on Unix: the Unix runner reads and emits in onepoll()loop so a blocked write back-pressures the read for free; the Windows reader threads break that link, and the bound restores it.What is the impact of this change?
None at runtime. Spec text only.
How was this change tested?
runner.rs,runner_win.rs, andframer.rsonmain(constants, channel size, shutdown order).Was this change documented?
Is this a breaking change?
No.
Does this change impact security?
No. Documents existing behavior; no code or file-permission changes.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.