Skip to content

docs: update cross-user helper specs for bounded output framing #379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wyongzhi wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

docs: update cross-user helper specs for bounded output framing #379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7c28ff
docs: update cross-user helper specs for bounded output framing
wyongzhi Sep 10, 2026
94692e4
Merge branch 'main' into docs/cross-user-helper-specs
wyongzhi Sep 10, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions specs/sessions/embedded-cross-user-helper.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,12 +323,14 @@ fn run_command(
// cancel a running action.
if stdin ready { read line, parse Command::Cancel(sig), verify token, killpg, child_killed = true }

// Child output → send {"out": line}
if child ready { read line, send }
// Child output → one bounded fill_buf chunk through LineFramer,
// each completed line sent as {"out": line}, then back to poll
if child ready { fill_buf (retry on EINTR); framer.push(chunk, emit); consume }

// Child exited (POLLHUP or try_wait after kill)
// Kill process group to clean up descendants (e.g. grandchild shells)
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() }

@github-actions github-actions Bot Sep 10, 2026

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.

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 before killpg, and killpg comes after wait(). 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_wait path (runner.rs:202-207): killpg → drain → framer.finish → return, using the status already obtained from try_wait — no second child.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.

}
}
```
Expand All @@ -339,8 +341,14 @@ Key details:
- After `killpg`, uses `try_wait()` with a 100ms poll timeout to detect exit
even when POLLHUP isn't delivered
- Checks `POLLHUP | POLLERR` for child stdout close
- Drains remaining buffered output before returning exit code
- Drains remaining buffered output through the framer and flushes the
trailing partial line before returning the exit code
- Kills the child's process group on exit to clean up descendants
- Child stdout is read in bounded chunks (one `fill_buf` per `POLLIN`, EINTR
retried) and framed by `LineFramer`: lines capped at 64 KiB, invalid UTF-8
escaped as `\xNN`, JSON payload capped at the 128 KiB response limit. This
keeps the loop returning to `poll(2)` so cancel stays responsive on
newline-free output, and bounds per-line memory. See `framer.rs`.
- Every cancel line read inside the runner is passed through the same
constant-time token check the main loop uses. The Windows runner
(`runner_win.rs`) receives already-validated `CancelMethod` values
Expand Down
34 changes: 27 additions & 7 deletions specs/windows-cross-user-helper.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,14 @@ threads instead:
stdin ──────────> │ Main thread │
(cancel cmds) │ - reads stdin lines │
│ - on cancel: signal child │
│ - drains bounded queue, │ ──> stdout
│ sends {"out":...} lines │ (to session)
│ │
│ Stdout thread │
│ - reads child stdout │ ──> stdout
│ - sends {"out":...} lines │ (to session)
│ Stdout / Stderr threads │
│ - read child pipe in 8 KiB │
│ chunks via LineFramer │
│ - push lines to bounded │
│ queue (256 slots) │
│ │
│ Child process (job-user) │
│ - CREATE_NEW_PROCESS_GROUP │
Expand All @@ -110,10 +114,24 @@ inside the helper.
- `{"cancel": "TERMINATE"}` → `kill_process_tree(child_pid)` using
`TerminateProcess` on each process in the tree.

**I/O multiplexing**: Two threads sharing a channel:
- Thread 1 (main): reads stdin for cancel commands, signals child on cancel
- Thread 2: reads child stdout line-by-line, sends `{"out":...}` responses
- 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

@github-actions github-actions Bot Sep 10, 2026

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.

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.

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

@github-actions github-actions Bot Sep 10, 2026

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.

"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.

- After the child exits, main thread drains the channel until both senders
disconnect, joins the reader threads, then sends `{"exited":...}`

@github-actions github-actions Bot Sep 10, 2026

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.

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:

  1. 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)".
  2. If the intent is parity with Unix, the spec should say the tree is killed before the final drain; today kill_process_tree only runs on the cancel/escalation paths.


**Output bounds** (same `framer.rs` as the Unix runner):
- Per line: 64 KiB cap (excess dropped to the next `\n`), invalid UTF-8
escaped as `\xNN`, trailing partial line flushed at EOF, JSON payload capped
at the 128 KiB response limit
- Aggregate: 256 slots × 64 KiB ≈ 16 MiB. A full channel parks the reader
threads, the child's pipe fills, and the child stalls
- Why a bounded channel: the Unix runner reads and emits in one `poll()` loop,
so a blocked write back-pressures the read for free. Threads break that
link; the bound restores it

### Helper launch (`cross_user_helper.rs`) — add `#[cfg(windows)]` spawn

Expand Down Expand Up @@ -188,6 +206,8 @@ to the child's `hStdInput`, the write end is returned in
Implemented `run_command` for Windows using two threads for I/O multiplexing.
Handle cancel commands by calling `GenerateConsoleCtrlEvent` or
`kill_process_tree`. Three integration tests pass (echo, cancel, nonexistent).
Reader threads route child stdout and stderr through the shared `LineFramer`
into a bounded channel; see "Output bounds" above.

### Step 3: Add `#[cfg(windows)]` spawn in `cross_user_helper.rs` ✅

Expand Down
Loading