server: own the PTY lifecycle — refusals, group kill, supervisor, deadlines, retention - #204
Merged
Conversation
|
|
🔗 Preview: https://blit-rmwdrblun-indent.vercel.app |
Coverage
|
![indent[bot]](https://avatars.githubusercontent.com/in/4161513?s=60&v=4){kind=small}
pcarrier
force-pushed
the
eng/pty-lifecycle
branch
from
August 6, 2026 00:35
d88a096 to
6fca345
Compare
pcarrier
changed the title
pcarrier
force-pushed
the
eng/pty-lifecycle
branch
2 times, most recently
from
August 6, 2026 01:10
c8b92bf to
d664384
Compare
Contributor
Author
|
All six fixed in 1. Reproduced and A/B'd on a running server, six create+close cycles: 6 zombies before (including the 2. Restart race — confirmed, fixed. Terminals carry a 3. Windows exit statuses — confirmed, fixed. Note this one is pre-existing on 4. Job-assign BOOL — confirmed, fixed. Nulls the handle on failure so 5. Reconnect attribution — confirmed, fixed. Initial burst uses 6. The doc was wrong and that's on me: Kept group as the default because that is the bug #181 describes, and a client that never reads the bit is precisely the client that wants "stop this terminal" to mean it. But "no way to ask for it" was fair, so Smaller items: On One I found, not in your listNarrowing So the trade is sharper than "narrowing is a real win": it fixes the status theft that broke audio's own I have not fixed that here. It's a different subsystem with its own ownership, I can't easily force the audio startup to fail on demand to verify a fix, and patching the one obvious site would look like a fix without being one. The options as I see them:
I'd take (1), as a separate PR. Flagging rather than deciding since it's outside what #181 asked for and you may know which audio paths are reachable in practice. Verified after the fixes: The fixes are one commit on top rather than folded into the five. Two earlier rebases on this branch silently dropped hunks that I only caught via clippy and a symbol audit, and the five commits each build standalone today; I'd rather not re-run that surgery under review. Say the word and I'll autosquash them in before merge. |
`BLIT_MAX_PTYS` became reachable in #188, but it could not be used safely: all four create arms refuse with a bare `continue` and no reply. `allocate_pty_id` says so itself — "the protocol has no 'create refused' message" — and settles for an `eprintln` so the cap at least leaves a trace in the server log. The client still sees nothing. `blit terminal start` hits its 10s socket timeout and reports "timeout waiting for server response"; the browser client's promise simply never settles. Turning the cap on traded an unbounded terminal count for a client that hangs. Give the protocol the missing message. `CREATE2(WANT_STATUS)` gets exactly one outcome — `CREATED_N` or `CREATE_FAILED [nonce:2][status:1][detail:N]` — negotiated through a new HELLO bit, with statuses from a common registry this also introduces. `BUDGET` names which exhaustion was hit, since raising a cap and waiting for ids to free up are different operator actions. Deliberately opt-in: `CREATE`, `CREATE_AT`, `CREATE_N`, and unflagged `CREATE2` keep their success-only contract, so a legacy client cannot mistake a refusal for PTY zero. That leaves `CREATE_N` unable to learn why it was refused — it has no feature byte to carry the request — and the `eprintln` stays for those arms. Both shipped clients use `CREATE2`. Every refusal added here replaces a path that was already dropping the request, except one: a tag or command over `u16::MAX` now returns `TOO_LARGE` instead of truncating through `pty_list_msg`'s `as u16` casts and desynchronizing `S2C_LIST` for every client. `--max-ptys` is added alongside the env var, for symmetry with the other server knobs. The default stays 0 — #188 argued unlimited is right, since a client that can open a terminal can already spend the machine from inside it, and nothing here changes that. Verified end to end against a server run with `--max-ptys 1`: the second create is refused in milliseconds with "budget exhausted (terminal cap reached (1); raise --max-ptys or close a terminal)" and exit 1, and the terminal list still holds exactly one.
`C2S_KILL` was `kill(child_pid, sig)` and `C2S_CLOSE` was `kill(child_pid, SIGHUP)` plus a master-fd close. Both reach the session leader and nothing else, so killing a shell left its children running and killing chromium left the zygote and every renderer. On close it is worse than a leak: exit detection is EOF on the master fd, and a surviving grandchild holds the slave open, so no EOF ever arrives, `exited` never flips, and the slot stays live with `S2C_EXITED` never sent. Every blit child is already a `setsid()` session leader, so its pgid equals its pid and `kill(-pid, sig)` needs no new bookkeeping — the tree just was not using it. `resize_pty_os` has delivered `SIGWINCH` this way, including the `TIOCGPGRP` hop to the foreground group, since forever; this generalizes that into `kill_pty`. Windows had the same shape for a different reason: `TerminateProcess` on the leader handle orphans the tree. `CreateProcessW` now creates the child suspended, assigns it to a job object, and resumes it — suspended because assigning a running child races anything it spawns in between, and those grandchildren would land outside the job. `JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE` gives the containment SIGHUP-to-the- group gives on Unix. The default flips to group delivery, which is the actual fix — no client change needed to stop leaking children. `KILL` takes an optional trailing flags byte (armed at length 8, HELLO bit 15) to opt back into leader-only, which is right when emulating a keystroke rather than stopping a terminal. Scope, stated plainly: this reaches the leader's group and the foreground group. A job backgrounded by an interactive shell is in neither and survives; bounding that needs a cgroup. Two tests pin both halves — one asserts the child survives a leader-only kill, the other that a group kill reaches it — and I checked they are not vacuous by flipping the second to leader-only and watching it fail. The Windows half is unverified locally: this checkout has no Windows toolchain (Nix, no rustup), so it rests on CI's windows build.
Exit detection had exactly one trigger: EOF on the master fd, drained
inside the delivery tick. But EOF means "the last fd on the slave
closed", not "the child exited". A grandchild that survives the terminal
hangup — one that ignores SIGHUP, or was setsid into another session —
holds the slave open, so the master never reaches EOF, `exited` never
flips, `S2C_EXITED` is never sent, and `blit terminal wait` blocks until
its own client-side timeout while the command has been dead for hours.
Worse, the tick that does the draining only schedules itself while a
client is attached: `blanket_frame_interval` returns `None` on an empty
client map and every other deadline it computes is client-gated, so a
server nobody is watching parks on `delivery_notify` indefinitely. That
is exactly when an abandoned command needs supervising.
Add a supervisor loop with the opposite duty cycle — its own task, woken
by SIGCHLD, sweeping every 5s as a backstop — that asks each live
terminal whether its child has exited and runs the exit path for the ones
that have. `poll_child_exited` is a targeted non-blocking `waitpid` that
parks the status the way the existing backstop does, so
`collect_exit_status` still reports the real code.
That let `reap_zombies` stop draining `waitpid(-1)`. The global drain
reaped *every* child of the process and discarded anything it did not
own, which meant it was stealing statuses from the audio pipeline's own
`try_wait` and from language-server engines — a race the audio code was
documented as living with. It is now a targeted sweep over owned pids:
still a backstop against a missed SIGCHLD, no longer anyone else's
problem.
Two smaller things fixed on the way:
- `C2S_CLOSE` on a live terminal never collected an exit status, which
is the only path that deregisters a pid, so every close leaked one
into the owned set and parked a status nobody drained.
- Windows `collect_exit_status` blocked up to a second under the
session mutex. The supervisor only calls it once the process is known
dead, so the wait drops to 50ms and stays only as a safety net for
the EOF path racing the kernel.
A/B'd against a pre-change server, same command both sides —
`bash -c '(trap "" HUP; sleep N) & exit 7'`, whose grandchild ignores the
hangup and keeps the slave open. Before: the terminal sits at `running`
indefinitely. After: `exited(7)`, with the grandchild still alive, which
is the documented limit of what a signal can contain.
Every timeout in blit is client-side, so none of them survives the client
that set it. `blit terminal wait --timeout` returns 124 and leaves the
command running. That is the shape of the incident this fixes: an agent
spawns a terminal per tool call, dies, and leaves a process burning a
machine nobody is looking at.
Add `C2S_DEADLINE [pty_id:2][ms:4]` and a `CREATE2` flag carrying the same
`[ms:4]`, enforced by the supervisor whether or not a client is attached.
Three properties, each load-bearing:
- Armable at create, so there is no window where a client that dies
immediately after creating a terminal leaves it unbounded. The field
goes after any cwd and *before* the command, because the command has
no length prefix and runs to the end of the message — a trailing
deadline would be swallowed into it.
- Refreshable, because `ms` counts from receipt. Repeat it on an
interval and it is a dead-man switch: the terminal outlives the
orchestrator by at most one period. `ms = 0` clears it, and also
stands down an in-flight stop sequence.
- Attributed. Expiry is SIGTERM to the group, 5s grace, then SIGKILL,
which lands on the wire as `signal(15)` — indistinguishable from
someone running `blit terminal kill`. `S2C_EXITED` gains a `reason`
byte, appended and length-gated so a short message from an older
server still reads as a normal exit. Values are numbered to leave the
lease and unit-stop causes from docs/design/units.md room to land
later without renumbering.
Deliberately still no default: unbounded stays the default, because
detaching and coming back is what a multiplexer is for. This is opt-in
per terminal.
`blit terminal start --deadline SECONDS` and `blit terminal deadline ID
SECONDS` expose it. A restart clears the deadline and the attribution —
it is a new command, not a continuation of the one the deadline was armed
for.
Verified against a running server: a terminal created with `--deadline 5`
and then abandoned dies at ~5s with no client attached; `blit terminal
wait` prints "signal(15) — killed by deadline" where a hand-rolled
`kill 9` on the same server prints a bare "signal(9)"; and a terminal
refreshed every 2s against a 4s deadline survived 12s, then died 8s after
the refreshes stopped.
`cleanup_pty_internal` marks a terminal exited and keeps its entry so the output stays readable. Nothing but an explicit `C2S_CLOSE` ever removed one — it was the only `ptys.remove` in the tree — so a client that creates a terminal per task and never closes it grew the map until the `u16` id space ran out, holding a full scrollback per dead command. Keep at most `BLIT_MAX_EXITED` of them (default 1024), oldest first. Eviction takes the same path a `C2S_CLOSE` would and broadcasts the same `S2C_CLOSED`, so no client learns a new message. It only ever touches terminals whose command has already exited; a live one is never reclaimed on a timer, because detaching and coming back is what a multiplexer is for. `BLIT_EXITED_LINGER` adds a time bound, off by default. That asymmetry is deliberate — a count bound reclaims only when there is pressure, while a time bound throws away output on a schedule, and how long a result stays interesting is a policy question the server cannot answer. The count bound alone is enough to keep the map bounded. This also corrects the cap semantics from the earlier commit in this series. `max_ptys` counted exited-but-retained terminals, which meant a client running short commands under `--max-ptys 256` would be refused after 256 of them with nothing running. It now counts live terminals only, with retention as the separate bound on the dead ones — which is what makes both numbers mean something. The eviction policy is a pure function of (id, exited_at) pairs, so the bounds are tested without standing up a PTY. Verified on a server run with `--max-ptys 2 BLIT_MAX_EXITED=3`: six consecutive short commands all succeed where the old counting would have refused the third, and the list settles at the newest three.
Six fixes, all local, none changing the shape of the change.
The blocker was mine, and it was in the cleanup of the commit that
otherwise fixes the most: `C2S_CLOSE` hung a live child up and then
dropped its pid from `pty_pids`, which after narrowing `reap_zombies` to
that set is the only thing that would ever wait it. The comment even
argued for it — and had the trade backwards. Keeping the pid costs a
hashmap entry; forgetting it costs a process slot, permanently, on a
server that agents cycle terminals through. `abandon_pty_pid` now moves
it to a set the reaper drains and discards, so the SIGHUP still gets
followed by a wait. A/B'd: six close cycles leave six zombies before,
zero after, and the unit test fails if the sweep is removed.
The rest:
- A restart landing inside the tick's 50ms deferred EOF cleanup used to
be impossible; the supervisor now reaches the same terminal within a
millisecond of SIGCHLD, so a client that sees `S2C_EXITED` and
restarts immediately can have a fresh child in the slot when the
deferred call lands — which then dropped its fd, hung it up, and
broadcast a second `EXITED` with an unknown status. Terminals carry a
generation, and cleanup queued against one will not fire on the next.
The `pty_fds` removal moves inside that guard too, where the session
lock already claims to own it.
- Windows read the exit code through a handle `close_pty` had just
closed, so every terminal reported an unknown status — or, once the
handle value was recycled, somebody else's. Pre-existing, but this
series leans on exit statuses, so: `close_pty` leaves `process` open
and `collect_exit_status` releases it, which also matches the Unix
twin's "collecting is what finishes with a child".
- A failed `AssignProcessToJobObject` left a non-null empty job, and
`kill_pty` routes group kills to `TerminateJobObject` — a terminal
nothing could kill, deadline included. It now nulls the handle and
falls back to the leader.
- The initial burst replayed `EXITED` with the reason hardcoded to
normal, so arm-a-deadline / disconnect / reconnect-to-collect — the
case the byte exists for — showed a bare `signal(15)`.
- `S2C_CREATE_FAILED` was never added to the opcode table, and
`msg_create2_full` did not warn that `HAS_DEADLINE` against a server
without the feature bit is the one optional field an old server
cannot safely ignore: it reads the four bytes as the start of the
command and spawns something else.
Also corrects the `KILL_LEADER_ONLY` doc, which claimed it was the right
choice for emulating a keystroke. It is not — the kernel sends `^C` to
the terminal's foreground process group, which is what the new default
already does, and signalling the shell alone mostly gets ignored. The
flag is for addressing the leader itself. The JS client can now pass it,
gated on the feature bit, so the browser has a way to ask.
pcarrier
force-pushed
the
eng/pty-lifecycle
branch
from
August 6, 2026 02:19
8251e9b to
80dadf2
Compare
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
Follow-up to #204, which I flagged there while verifying it. #204 narrowed `reap_zombies` from draining `waitpid(-1)` to sweeping only PTY-owned pids. That was the right call — the global drain was reaping other subsystems' children and discarding their statuses, which is what broke the audio pipeline's own `try_wait`. But it also stopped it covering for two places where the audio pipeline does not collect its own children, and both are live on `main` now that #204 has merged. **A missed wait.** The `dbus-daemon exited without printing an address` bail-out kills the child and returns without waiting it. Every other bail-out in `spawn` waits what it kills; this one was missed. **A duty-cycle gap** — the same one #204 fixed for terminals. `is_alive` is what collects dead sub-processes, as a side effect of the `try_wait` calls it makes to decide whether to heal, and it is only reached from the delivery tick. The tick does not run while no client is attached, so an idle server whose PipeWire died kept the corpse until somebody connected. `reap_children` collects and does nothing else — none of `is_alive`'s restart behaviour, which is not something to run on a timer nobody asked for — and the supervisor calls it, that loop existing precisely because it runs when the tick does not. ## Verification The dbus path is A/B'd with a fake `dbus-daemon` on `PATH` that exits without printing anything, which drives exactly that branch: - **before:** log shows `[audio] failed to start pipeline: dbus-daemon exited without printing an address`, and the server is left with a `dbus-daemon <defunct>` child - **after:** same log line, no children at all **The `reap_children` half is not verified end-to-end.** PipeWire does not start on the machine I have (`pipewire exited before creating its socket`), so there is no live pipeline whose child I can kill and watch get collected. What it does is a `try_wait` on each stored `Child` with no other state change, called from a loop that already runs every 5s — but that is an argument, not a test, and someone with working audio should confirm it. `nix run .#lint` exit 0, `cargo test -p blit-server` 287 passing. ## Note on scope There is a third option I did not take: registering the audio children with the same owned-pid mechanism PTYs use, so one reaper covers everything. That is the tidier end state, but it means moving ownership out of `AudioPipeline`, and this fix is small enough to stand alone and stop the bleeding first.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
The primitives section proposed wire that has since landed, and in two
places landed differently. Left as written it now contradicts the
protocol.
- `S2C_LEASE` moves to `0x11`. `0x10` was free when this was written;
#204 shipped `S2C_CREATE_FAILED` there.
- `S2C_CREATE_FAILED` is `[0x10][nonce:2][status:1][detail:N]`, not
`[0x11][nonce:2][reason:1]` — the common status registry rather than
a message-local reason byte, matching what #167's protocol.md had
already allocated. It is also opt-in per request via
`CREATE2_WANT_STATUS`, so a legacy client cannot read a refusal as
PTY zero.
- `max_ptys` kept its `0` default rather than gaining a real one.
#188 landed the env var in the meantime and argued unlimited is
right, and that argument holds: a client that can open a terminal
can already spend the machine from inside it.
- `FEATURE_UNITS` moves to bit 17. 11-13 are reserved for the
extension, channel, and process families; 14-16 shipped with #204.
- The `C2S_KILL` flags arm is `data.len() >= 8`, not `>= 7`. Seven is
the existing message length, so arming there reads a byte that is
not there.
Delivery marks 1-3 shipped and narrows 2 to what is actually left: the
lease family, and the timed `C2S_CLOSE` escalation, which needs `CLOSE`
to hold a "closing" state and tangles with the retention path.
This was referenced Aug 6, 2026
Contributor
Author
|
Follow-ups from the description are all filed, off latest
Nothing else outstanding from this PR that I'm aware of. |
This was referenced Aug 6, 2026
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
Follow-up to #204, from reviewing the merged PTY lifecycle work. ## The bug `CREATE2(WANT_STATUS)` promises exactly one outcome, and `docs/protocol.md` says a malformed field answers `INVALID`. Four of the six fields do. The tag did not: ```rust let tag = if data.len() >= 10 + tag_len { std::str::from_utf8(&data[10..10 + tag_len]).unwrap_or_default() } else { "" }; ``` An out-of-range `tag_len` and a non-UTF-8 tag both fell back to an empty tag and let the create **succeed**. The first is worse than a mislabelled terminal. `cursor` becomes `10 + tag_len` regardless, so an overrunning length leaves it past the end of the message. The cwd and deadline arms bounds-check what they read and would refuse — but a `CREATE2` carrying a command and neither of those has nothing left to catch it: `data.get(cursor..)` returns `None`, `create_payload` is `None`, and **the server spawns the default shell instead of the command it was sent**. A client that asked to run one thing silently gets a prompt. The non-UTF-8 case is quieter: the terminal exists with an empty tag, so a client correlating by tag can never match it and has no refusal to react to. ## The fix Pull the read into `create2_tag`, which returns the tag or the detail string to refuse with, and refuse on both. Extracted rather than inlined so the two failures are testable — the create arms need an `AppState` that nothing in the suite constructs, which is the same reason `armed_deadline` and `slots_to_evict` are pure. ## Verification Three tests: a well-formed tag (including one followed by later fields), a length past the end (off by one and far), and a non-UTF-8 tag. Checked they are not vacuous by restoring the empty-tag fallback and confirming both refusal tests fail. Workspace clippy and `cargo fmt` clean, 290 blit-server tests passing.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
…#215) Follow-up to #204, from reviewing the merged PTY lifecycle work. ## The bug `supervise` opened each pass with `enforce_deadlines`, which signals any terminal whose deadline or stop grace has come due. That is the wrong end of the pass, because `reap_zombies` — at the tail of the same pass — waits a child *without* marking its terminal exited; it has no way to reach the session. So a child that dies between the exit scan and `reap_zombies` gets waited there, freeing its pid, while its `Pty` stays `exited: false`. The next pass then ran `enforce_deadlines` first, saw a live terminal, and — if the stop grace was due — sent `kill(-pid, SIGKILL)` at a pid the kernel had already released. The distinction that matters: a **zombie pins its process group**, so signalling a not-yet-reaped child is safe. Signalling a *reaped* one aims at a pgid that is free to be reused. That is the only window where the `!pty.exited` guard does not hold, and `reap_zombies` is the only thing that opens it. ## The fix Move the call after the exit scan and its cleanup, still ahead of `reap_zombies`. Anything the previous pass waited is now marked exited by `poll_child_exited` — which reports a parked status — before deadlines are considered, and `enforce_deadlines` skips exited entries. That closes the window in both directions: a pid freed by this pass's `reap_zombies` is marked exited by the next pass's scan before this call can see it. No behaviour change for a terminal that is actually alive. The deadline still fires in the pass its instant falls in, only later within it, and `earliest_armed_deadline` computes the wakeup independently. ## Verification Not covered by a test, deliberately stated rather than glossed: `supervise` needs an `AppState`, which nothing in the suite constructs — the deadline and retention policies are unit tested through the pure `armed_deadline` and `slots_to_evict` for exactly that reason, and neither is where this ordering lives. Verified by reading the pass against `reap_zombies`, `poll_child_exited` and `enforce_deadlines`. Workspace clippy and `cargo fmt` clean, 287 blit-server tests passing.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
#204 landed the RFC's primitives (delivery items 1-3, tracked as #181). Two of them landed differently from what the RFC proposed, and the RFC now contradicts the shipped protocol in ways that would mislead whoever implements the unit layer on top. | RFC said | Shipped | Why | | --- | --- | --- | | `S2C_LEASE [0x10]` | must move to `0x11` | `0x10` was free when written; #204 put `S2C_CREATE_FAILED` there | | `S2C_CREATE_FAILED [0x11][nonce:2][reason:1]` | `[0x10][nonce:2][status:1][detail:N]` | common status registry rather than a message-local byte, matching #167's `protocol.md` | | `max_ptys` gets a real default | kept `0` | #188 landed the env var meanwhile and argued unlimited is right | | `FEATURE_UNITS` bit 11 | bit 17 | 11-13 reserved for extension/channel/process, 14-16 shipped with #204 | | `C2S_KILL` flags arm at `len >= 7` | `>= 8` | 7 is the existing message length — arming there reads a byte that isn't present | Also worth knowing for the layer above: the refusal is **opt-in per request**. A client sets `CREATE2_WANT_STATUS` (bit 3) after seeing `FEATURE_CREATE_STATUS` (HELLO bit 14), so `CREATE`, `CREATE_AT`, `CREATE_N` and unflagged `CREATE2` keep their success-only contract and a legacy client can't read a refusal as PTY zero. Delivery now marks 1-3 shipped and narrows item 2 to what's actually left: the lease family, and the timed `C2S_CLOSE` escalation. That second one needs `CLOSE` to hold the entry in a "closing" state, which tangles with the retention path #204 added — it was deliberately out of scope for #181 and is still open. No changes to the unit layer itself; how the primitives landed doesn't affect it.
pcarrier
added a commit
that referenced
this pull request
Aug 6, 2026
…217) Follow-up to #204/#211, found while auditing which subsystems still reap their own children. #204 narrowed the daemon's backstop from draining `waitpid(-1)` to sweeping PTY-owned pids only. It no longer touches LSP children at all, and three things in this crate still describe the world before that: - **`reap_backstop_status`** was already an empty stub (`fn reap_backstop_status(_pid: u32) {}`) whose doc comment explained a pid-recycling collision it no longer prevents. It and its two call sites go, along with the `pid` bindings that existed only to feed it. - **The `try_wait` comment** still warned that *"the daemon's global reaper may win the race and steal the status"*. It cannot. What's left is the ordinary case — an `Err` means the child is already gone — so the comment says that. - **`docs/design/lsp.md` § Reaping** described the backstop as reaping every child while parking statuses selectively. It reaps selectively now too. No behaviour change; the stub did nothing. ## On the doc I rewrote that bullet rather than deleting it, because the consequence is worth keeping and is not obvious: now that the backstop only sweeps what it owns, a subsystem that doesn't wait its own children **leaks zombies** instead of being quietly mopped up. That makes the engine's every-path `wait()` load-bearing rather than defensive. That's not hypothetical — it's exactly what #211 had to fix for the audio pipeline, where `is_alive` was the only thing collecting children and it only ran from the delivery tick. The LSP engine was already correct here; the note is so the next person adding a subprocess knows the floor moved. `nix run .#lint` exit 0, `cargo test -p blit-lsp` 39 passing.
indent Bot
pushed a commit
that referenced
this pull request
Aug 7, 2026
CLOSE has signalled the child's process group with SIGHUP since #204, but stopped there, so a child that ignores SIGHUP outlived its terminal: blit terminal start -- bash -c 'trap "" HUP; sleep 600' blit terminal close <id> # the sleep survives docs/design/units.md specifies the rest of the sequence — SIGHUP to the group, wait TimeoutStopSec, SIGKILL to the group — and the deadline path already implements that shape for expiry. The escalation rides on the pid, not on a retained terminal. #213 framed this as CLOSE holding the slot in a "closing" state, which is where the design cost is: such an entry is neither live nor exited, so it counts against --max-ptys through live_ptys() while evict_exited never sees it (that keys off exited_at), and CLOSED would have to start meaning "going" instead of "gone". None of that is needed to kill a process. CLOSE keeps removing the slot and broadcasting CLOSED synchronously; abandon_pty_pid takes the deadline for the SIGKILL, the supervisor arms its timer off it and fires escalate_abandoned when it comes due. Reaping beats escalating: a waited pid may already name an unrelated process group, so the reaper dropping a registration is also what disarms its pending kill, and the PID-1 orphan drain forgets the pid along with the status. The window where the group signal is valid is exactly the window where the child is still ours. Windows needs none of it — close_pty drops the last handle to a kill-on-job-close job, so the hangup is already the kill — and the wire is untouched in both directions: no new frame, no changed layout, and CLOSED arrives when it always did.
indent Bot
pushed a commit
that referenced
this pull request
Aug 7, 2026
#204 refused a create whose tag or command cannot round-trip S2C_LIST's u16 length prefix, and left the total unbounded. docs/protocol.md already promised the other half — "a projected LIST overflow ... return BUDGET" — and admitted it needed a logical-message ceiling that did not exist. It does exist, and it is not the 16 MiB frame size: the server fragments anything over 4 KiB, so a logical message is not frame-bounded. What binds is reassembly, where MAX_DECOMPRESSED (64 MiB) is already enforced by every client that reassembles — read_message in blit-cli and BlitConnection in @blit-sh/core both abort the connection past it, with no diagnostic at either end. A catalog the server cannot describe under that is a catalog nobody can be told about, so that is what a create is projected against, refused with BUDGET and a detail naming the number. Creation is the whole surface. tag and command are fixed once a terminal exists — nothing in the protocol renames one, and RESTART clones both — so the catalog only grows by an entry a create put there. The projection is derived from ptys on every create rather than carried in a running total: a counter is a second record of the same fact, it drifts the first time a removal path forgets it, and a catalog that reports itself smaller than it encodes is precisely the desynchronizing frame this prevents. list_entry_bytes and push_list_entry are paired and pinned by a test, with a debug_assert in the encoder against the projection it is checked with. Two gaps found on the way. C2S_CREATE, CREATE_N and CREATE_AT never got #204's guard at all, and their command field has no length prefix — it runs to the end of a frame that may be 16 MiB — so a >64 KiB command still truncated into a corrupt catalog for every client. They now refuse too; having no failure reply, they refuse to the log, as allocate_pty_id already does for the cap. And connecting preflights the same projection before registering the client, refusing the connection with a diagnostic rather than building a burst that would make the client hang up silently. Replaces the vacuous pty_list_msg_includes_tags, which built its expected bytes by hand and asserted them against themselves without ever calling the encoder. No wire change in either direction: same frames, same layouts, and a refusal that a client which never asked for CREATE_FAILED cannot see.
pcarrier
added a commit
that referenced
this pull request
Aug 7, 2026
Fixes #213 and #214, one commit each. Wire compat first: **no protocol change in either direction.** No new frame, no changed layout, no new feature bit. `CLOSED` still arrives the instant `CLOSE` is handled; the escalation is a server-side timer a client never sees. The `S2C_LIST` bound is a refusal at creation, and `CREATE_FAILED` only reaches a client that set `CREATE2.WANT_STATUS`. Old client / new server and new client / old server both behave exactly as before, except that a process which used to survive `CLOSE` now dies and a create that used to corrupt the catalog is now refused. --- ## #213 — `C2S_CLOSE` escalates to a group SIGKILL `close_pty` has signalled the process group with SIGHUP since #204, but stopped there, so the issue's repro survived: ``` blit terminal start -- bash -c 'trap "" HUP; sleep 600' blit terminal close <id> # the sleep keeps running ``` `docs/design/units.md` specifies the rest — SIGHUP to the group, wait `TimeoutStopSec`, SIGKILL to the group — and `enforce_deadlines` already implements that shape for expiry. **The design call: the escalation rides on the pid, not on a retained terminal.** #213 framed this as `CLOSE` holding the slot in a "closing" state, and that is where all the cost is: such an entry is neither live nor exited, so `live_ptys()` counts it against `--max-ptys` while `evict_exited` (which keys off `exited_at`) never sees it, and `CLOSED` would have to start meaning "going" instead of "gone". None of that is needed to kill a process. `CLOSE` keeps removing the slot and broadcasting `CLOSED` synchronously; `abandon_pty_pid` takes the deadline for the SIGKILL, and the supervisor — which already wakes on armed timers — fires `escalate_abandoned` when it comes due. **Reaping beats escalating.** A waited pid may already name an unrelated process group, so the reaper dropping a registration is also what disarms its pending kill, and the PID-1 orphan drain forgets the pid along with the status. The window in which `kill(-pid)` is valid is exactly the window in which the child is still ours. Windows needs none of this: `close_pty` drops the last handle to a `KILL_ON_JOB_CLOSE` job, so the hangup is already the kill, and the two new entry points are no-ops there. Verified against a real binary on an isolated socket: | | | |---|---| | `blit terminal close 1` returns | 13 ms | | HUP-ignoring child at t+0.5s, t+3s | alive | | at t+7s | gone | | catalog after `close` | empty immediately | | child that *does* answer SIGHUP | dead and reaped inside 0.7 s, no zombie | ## #214 — the aggregate `S2C_LIST` bound The ceiling the issue says is missing does exist, and it is not the 16 MiB frame size — the server fragments anything over 4 KiB, so a logical message is not frame-bounded. What binds is reassembly: `MAX_DECOMPRESSED` (64 MiB) is already enforced by every client that reassembles (`read_message` in blit-cli, `BlitConnection` in `@blit-sh/core`), and both abort the connection past it with no diagnostic at either end. A catalog the server cannot describe under that is a catalog nobody can be told about, so that is what a create is projected against, refused with `BUDGET` and a detail naming the number. Creation is the whole surface: `tag` and `command` are fixed once a terminal exists (nothing renames one, `RESTART` clones both), so the catalog only ever grows by an entry a create put there. The projection is **derived** from `ptys` on each create rather than carried in a running total — a counter is a second record of the same fact, it drifts the first time a removal path forgets it, and a catalog that reports itself smaller than it encodes is precisely the desynchronizing frame this is meant to prevent. `list_entry_bytes` and `push_list_entry` are paired and pinned by a test, plus a `debug_assert` in the encoder against the projection it gets checked with. Two gaps the audit turned up, both fixed here: - **`C2S_CREATE`, `CREATE_N` and `CREATE_AT` never got #204's per-field guard at all**, and their `command` field has no length prefix — it runs to the end of a frame that may be 16 MiB. So a >64 KiB command still truncated into a corrupt catalog for every client on `main`. They refuse now too; having no failure reply, they refuse to the server log, as `allocate_pty_id` already does for the cap. - **Connecting preflights the same projection** before the client is registered, refusing the connection with a diagnostic instead of building a burst that would make the client hang up silently. Verified with a temporarily-shrunk ceiling: the client fails immediately with `blit: server closed connection` (exit 1) and the server stays healthy. End-to-end with a temporary 40-byte ceiling, four terminals live: ``` blit: server refused to create terminal: budget exhausted (catalog would reach 43 bytes, over the 40-byte S2C_LIST ceiling) ``` Also replaces `pty_list_msg_includes_tags`, which was vacuous: it built its expected bytes by hand and asserted them against themselves without ever calling the encoder. ## Out of scope, worth its own issue `surface_list_msg` has the identical unbounded shape (`title.len() as u16`, `app_id.len() as u16`, no surface cap), and it is worse: `title` and `app_id` come from `xdg_toplevel.set_title` on a Wayland client, not from a blit request, so there is nothing to refuse and the fix has to be truncation or elision at encode time. `msg_surface_created` in `blit-remote` casts the same way. Untouched here. ## Verification `cargo test -p blit-server --lib` 330 passed; `cargo fmt --all --check`; `cargo clippy --workspace -- -D warnings`, plus the `-p blit-server --all-targets` and `--no-default-features` passes CI runs; `prettier --check docs/protocol.md`. Both new escalation tests were confirmed non-vacuous by removing the `kill` and watching them fail in 5 s (and they now carry a `Drop` guard, so a failure kills its forked group instead of leaving paused children holding the harness's stdout open). [](https://app.indent.com/c/019fdd2c-c70f-7a05-90ca-72eae84a2204) Tag `@indent` to continue the conversation here.
](https://app.indent.com/c/019fdd2c-c70f-7a05-90ca-72eae84a2204){kind=link}
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
1 participant
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.
Closes #181.
Implements all three gaps in #181. They share one cause — nothing in the server owned a PTY's lifetime — and one implementation: a supervisor loop that runs when the delivery tick does not.
Five commits, each building standalone and separately verified:
answer a refused create instead of dropping itcontinue, so a nonce-bearing client waits foreverkill a terminal's process group, not just its leaderkill(pid)/kill(pid, SIGHUP)reached the session leader alone — kill a shell, keep its childrendetect a terminal's exit from the child, not from EOFenforce opt-in terminal deadlines, and say when one firedbound retained terminals, and count the cap against live onesCLOSEever removed an exited entryRelationship to #188
#188 made
BLIT_MAX_PTYSreachable and argued — correctly — that unlimited is the right default. It also documented the gap this PR closes, inallocate_pty_id: "the protocol has no 'create refused' message", settling for aneprintlnso the cap at least leaves a trace in the server log.So the cap could be set but not safely used: turning it on traded an unbounded terminal count for a client that hangs. This adds the missing message and leaves the default at 0.
--max-ptysis added alongside the env var for symmetry with the other server knobs; theeprintlnstays, because the older create opcodes still drop the request silently by design.The last commit does change the cap's counting to live terminals only, so a client running short commands under
--max-ptys Nis not refused after N of them with nothing running. Exited terminals get their own bound instead.Verification
Each commit message records its own check. The load-bearing ones, all re-run after the rebase onto main:
--max-ptys 1refuses the second create in milliseconds withbudget exhausted (terminal cap reached (1); raise --max-ptys or close a terminal)and exit 1. Previously: a 10s hang, then a generic socket timeout.bash -c '(trap "" HUP; sleep N) & exit 7'— a grandchild that ignores the hangup and keeps the slave open. Before: the terminal sits atrunningindefinitely. After:exited(7).--deadline 5and abandoned dies at ~5s with no client attached.blit terminal waitprintssignal(15) — killed by deadlinewhere a hand-rolledkill 9prints a baresignal(9). Refreshed every 2s against a 4s deadline it survived 12s, then died 8s after the refreshes stopped.--max-ptys 2 BLIT_MAX_EXITED=3, six consecutive short commands all succeed and the list settles at the newest three.Workspace clippy clean,
cargo fmtclean, 556 Rust tests and 812 JS tests passing, JS typecheck clean.Review notes
Two things not verified here. The Windows job-object half has no toolchain in this checkout (Nix, no rustup) and rests on CI's windows build. And the third commit removes
reap_zombies' globalwaitpid(-1)drain — a strict improvement, since it was reaping other subsystems' children and discarding their statuses out from under the audio pipeline's owntry_wait, but it is a change outside the PTY family.Group kill's limit, stated rather than papered over. It reaches the leader's process group and, via
TIOCGPGRP, the terminal's foreground group. A job backgrounded by an interactive shell is in neither and survives. Bounding that needs a cgroup, not a signal.Feature bits 11–13 are left unallocated for the extension, channel, and process families under review in #167 and #173. This takes 14 (
CREATE_STATUS), 15 (KILL_MODE), and 16 (PTY_DEADLINE), matching the allocation #167'sprotocol.mdalready proposes for 14. The common status registry this introduces is #167's design; landing it here means #167 can drop that section rather than restate it.Five pre-existing test failures in
crates/gitare unrelated — identical 55-passed/5-failed on a tree with none of these changes (a local git config makesgit tag v1demand a message).Follow-ups, deliberately not in here
docs/design/units.md(RFC: Supervised sessions with lifecycle policy #94) needs reconciling before it merges: it allocatesS2C_LEASE = 0x10, which this PR now uses forCREATE_FAILED; it givesCREATE_FAILEDa different opcode and payload; and its "theC2S_KILLflags arm isdata.len() >= 7" is off by one, since 7 is the existing message length.C2S_CLOSEescalation from units.md needsCLOSEto hold the entry in a "closing" state, which tangles with the retention path, and is not part of what pty: exited sessions are never freed, nothing enforces a deadline, and kill misses the process group #181 asks for.S2C_LISTsize needs a logical-message ceiling that does not exist yet. The per-fieldTOO_LARGEcheck is in.