RFC: Native non-PTY process protocol - #173
Conversation
Coverage
|
9a56e1f to
2efd517
Compare
|
|
Reviewing this against #94 (units — declarative process supervision: unit files, One thing prevents that today. Please add a detach/adopt path
For a client running a build or a test, that's the correct default and I wouldn't change it. For a supervisor it inverts the property that makes a supervisor worth having: the supervisor is supposed to be more reliable than the things it supervises. Here, updating the supervising extension, cancelling its attempt, or tripping a guest trap takes down every process it manages. A units extension could not ship a new version without killing production. It's also inconsistent with the neighbouring family, in a way that would surface as two backings with opposite crash semantics. #167 says PTYs "remain server-session objects, exactly as they do when an ordinary network client disconnects. Attempt cleanup does not invent extension-only PTY ownership." So a PTY-backed unit survives its supervisor restarting and a pipe-backed one does not — same declarative config, same restart, different outcome, for a reason nobody writing a unit file would predict. What I need is the weakest version of this that works: an opt-in spawn flag that makes a child outlive its endpoint, plus a way for a later endpoint to re-attach to it by a stable identity. Worth saying that this is useful beyond units. Any client that wants a long-running job to survive a reconnect wants it, and today the only way to get it is Please make the per-endpoint cap reachable for a supervisor
I'm not asking for a higher default — 16 is right for an interactive client. I'd like either the per-endpoint cap to be raisable for a specific endpoint, or an acknowledgement in the RFC that a supervising client should expect to hit it, so the units design sizes against it deliberately rather than discovering it in a crash loop. Two smaller notes
Everything else here I'd take as-is. |
2efd517 to
fe67fb5
Compare
|
Addressed in fe67fb5 and rebased onto current #167. The RFC now specifies opt-in DETACHABLE children with client-persisted 128-bit adoption tokens; atomic PROCESS_ADOPT / PROCESS_ADOPTED snapshot-to-live binding; bounded output gaps, stdin inheritance, detach cutoffs, terminal-result retention, and endpoint-loss races; explicit KILLED causes; supervisor BUDGET guidance; and the Unix custom-grace workaround / Windows limitation. It also serializes child registration against the existing waitpid(-1) backstop and makes process-lifetime tasks server-owned so an extension replacement can adopt a surviving child. |
fe67fb5 to
5493f9b
Compare
…dlines, retention (#204) 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: | Commit | #181 item | What it fixes | | --- | --- | --- | | `answer a refused create instead of dropping it` | 3 (half) | all four create arms refuse with a bare `continue`, so a nonce-bearing client waits forever | | `kill a terminal's process group, not just its leader` | 2 | `kill(pid)` / `kill(pid, SIGHUP)` reached the session leader alone — kill a shell, keep its children | | `detect a terminal's exit from the child, not from EOF` | 1, 2 | exit detection was EOF-on-master, which means "the slave closed", not "the child exited" | | `enforce opt-in terminal deadlines, and say when one fired` | 1 | every timeout was client-side, so none survived the client that set it | | `bound retained terminals, and count the cap against live ones` | 3 (rest) | nothing but an explicit `CLOSE` ever removed an exited entry | ## Relationship to #188 #188 made `BLIT_MAX_PTYS` reachable and argued — correctly — that unlimited is the right default. It also documented the gap this PR closes, in `allocate_pty_id`: *"the protocol has no 'create refused' message"*, settling for an `eprintln` so 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-ptys` is added alongside the env var for symmetry with the other server knobs; the `eprintln` stays, 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 N` is 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: - **Refusal**: server with `--max-ptys 1` refuses the second create in milliseconds with `budget exhausted (terminal cap reached (1); raise --max-ptys or close a terminal)` and exit 1. Previously: a 10s hang, then a generic socket timeout. - **Exit detection**: A/B'd against a pre-change server with the same command, `bash -c '(trap "" HUP; sleep N) & exit 7'` — a grandchild that ignores the hangup and keeps the slave open. Before: the terminal sits at `running` indefinitely. After: `exited(7)`. - **Deadlines**: a terminal created with `--deadline 5` and abandoned dies at ~5s with no client attached. `blit terminal wait` prints `signal(15) — killed by deadline` where a hand-rolled `kill 9` prints a bare `signal(9)`. Refreshed every 2s against a 4s deadline it survived 12s, then died 8s after the refreshes stopped. - **Retention**: with `--max-ptys 2 BLIT_MAX_EXITED=3`, six consecutive short commands all succeed and the list settles at the newest three. - **Group kill**: two tests pin both halves — one asserts a child survives a leader-only kill, the other that a group kill reaches it. Mutation-checked by flipping the second to leader-only and confirming it fails. Workspace clippy clean, `cargo fmt` clean, 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`' global `waitpid(-1)` drain — a strict improvement, since it was reaping other subsystems' children and discarding their statuses out from under the audio pipeline's own `try_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's `protocol.md` already 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/git`** are unrelated — identical 55-passed/5-failed on a tree with none of these changes (a local git config makes `git tag v1` demand a message). ## Follow-ups, deliberately not in here - `docs/design/units.md` (#94) needs reconciling before it merges: it allocates `S2C_LEASE = 0x10`, which this PR now uses for `CREATE_FAILED`; it gives `CREATE_FAILED` a different opcode *and* payload; and its "the `C2S_KILL` flags arm is `data.len() >= 7`" is off by one, since 7 is the existing message length. - The timed `C2S_CLOSE` escalation from units.md needs `CLOSE` to hold the entry in a "closing" state, which tangles with the retention path, and is not part of what #181 asks for. - Bounding the *aggregate* `S2C_LIST` size needs a logical-message ceiling that does not exist yet. The per-field `TOO_LARGE` check is in.
1 participant
Summary
Extract the non-PTY process design from #167 into a standalone, all-client protocol RFC.
0xC0through0xC6Network clients use their existing transport. In-process extensions use the same packets through the ordinary host ABI, but neither implementation depends on the other.
Review follow-up
The revised RFC makes detach/adopt ownership, outbox cutoffs, inherited stdin, output-credit reset, terminal expiry, and concurrent adoption linearization explicit. It also serializes Unix spawn registration against Blit's existing
waitpid(-1)backstop, documents expected supervisorBUDGEThandling, and gives supervisors an explicit Unix per-process grace-period recipe.Detached state is intentionally server-memory-only: it does not survive a Blit restart, and output produced while unbound is drained and represented as an offset gap rather than replayed.
Stack
This is intentionally based on #167 so the extension RFC can link to it while keeping the process family out of the parent PR. It should be reviewed and merged separately after the base lands.
Verification
./bin/lintpassescargo fmt --check, repository-wide Prettier, andgit diff --checkpass