pty: escalate CLOSE to SIGKILL, and bound the aggregate S2C_LIST - #255
Merged
Conversation
|
Coverage
|
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.
#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.
The lint gate has been red on main since 1cddeae: prettier --check rejects docs/server.md, js/core/src/BlitConnection.ts, and js/core/src/__tests__/audio-skip-servo.test.ts. None of them are touched by this branch — CI checks the merge with main, so the failure lands here anyway. Purely mechanical, and version-independent: prettier 3.8.2 and 3.9.6 produce byte-identical output for all three, and 3.8.2 reports the whole tree clean afterwards.
indent
Bot
force-pushed
the
eng/pty-close-escalation-and-list-bound
branch
from
August 7, 2026 18:09
352be38 to
140cb49
Compare
|
🔗 Preview: https://blit-ako0d6io6-indent.vercel.app |
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.
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.
CLOSEDstill arrives the instantCLOSEis handled; the escalation is a server-side timer a client never sees. TheS2C_LISTbound is a refusal at creation, andCREATE_FAILEDonly reaches a client that setCREATE2.WANT_STATUS. Old client / new server and new client / old server both behave exactly as before, except that a process which used to surviveCLOSEnow dies and a create that used to corrupt the catalog is now refused.#213 —
C2S_CLOSEescalates to a group SIGKILLclose_ptyhas signalled the process group with SIGHUP since #204, but stopped there, so the issue's repro survived:docs/design/units.mdspecifies the rest — SIGHUP to the group, waitTimeoutStopSec, SIGKILL to the group — andenforce_deadlinesalready implements that shape for expiry.The design call: the escalation rides on the pid, not on a retained terminal. #213 framed this as
CLOSEholding the slot in a "closing" state, and that is where all the cost is: such an entry is neither live nor exited, solive_ptys()counts it against--max-ptyswhileevict_exited(which keys offexited_at) never sees it, andCLOSEDwould have to start meaning "going" instead of "gone". None of that is needed to kill a process.CLOSEkeeps removing the slot and broadcastingCLOSEDsynchronously;abandon_pty_pidtakes the deadline for the SIGKILL, and the supervisor — which already wakes on armed timers — firesescalate_abandonedwhen 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_ptydrops the last handle to aKILL_ON_JOB_CLOSEjob, 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 1returnsclose#214 — the aggregate
S2C_LISTboundThe 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_messagein blit-cli,BlitConnectionin@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 withBUDGETand a detail naming the number.Creation is the whole surface:
tagandcommandare fixed once a terminal exists (nothing renames one,RESTARTclones both), so the catalog only ever grows by an entry a create put there. The projection is derived fromptyson 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_bytesandpush_list_entryare paired and pinned by a test, plus adebug_assertin the encoder against the projection it gets checked with.Two gaps the audit turned up, both fixed here:
C2S_CREATE,CREATE_NandCREATE_ATnever got server: own the PTY lifecycle — refusals, group kill, supervisor, deadlines, retention #204's per-field guard at all, and theircommandfield 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 onmain. They refuse now too; having no failure reply, they refuse to the server log, asallocate_pty_idalready does for the cap.blit: server closed connection(exit 1) and the server stays healthy.End-to-end with a temporary 40-byte ceiling, four terminals live:
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_msghas the identical unbounded shape (title.len() as u16,app_id.len() as u16, no surface cap), and it is worse:titleandapp_idcome fromxdg_toplevel.set_titleon 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_createdinblit-remotecasts the same way. Untouched here.Verification
cargo test -p blit-server --lib330 passed;cargo fmt --all --check;cargo clippy --workspace -- -D warnings, plus the-p blit-server --all-targetsand--no-default-featurespasses CI runs;prettier --check docs/protocol.md. Both new escalation tests were confirmed non-vacuous by removing thekilland watching them fail in 5 s (and they now carry aDropguard, so a failure kills its forked group instead of leaving paused children holding the harness's stdout open).Tag
@indentto continue the conversation here.