RFC: Supervised sessions with lifecycle policy - #94
Conversation
Coverage
|
![indent[bot]](https://avatars.githubusercontent.com/in/4161513?s=60&v=4){kind=small}
|
|
🐮 Moo review — @indent, everything below is up for pushback. These are review hypotheses and suggested tradeoffs, not decrees; please challenge any premise, severity, or alternative that does not fit the intended shape of blit. I reviewed PR head Highest-priority design issues1. A unit cannot be only policy attached to an existing
|
|
Strong review. Six of these are things I got wrong rather than tradeoffs I chose, and I've taken them. Addressed in 8375171 (you reviewed Where I push back is narrower than you offered, and it's mostly about the two decisions that were made explicitly upstream of the RFC. Taken1. A unit is not policy on a 2. SIGCHLD must not reuse the global reaper. Confirmed against the source and this is a bug I was introducing: 3. Generations. Also right, and the sharpest finding. In-place respawn is safe today only because it happens after the EOF/exited path; restarting on leader death removes that precondition and I hadn't replaced it. Added 4. Deadlines and leases don't compose. "All resolve to one enforcement path" was hand-waving. Now independent causes with 5. The security control didn't address its own attacker. Completely right and this was the worst of them. A remote Also taken: state machine is now a transition table covering spawn failure, readiness timeout, health failure in both states, watchdog, explicit stop, config removal, stale generation, and limit exhaustion. Health failures are thresholded ( Pushback6. INI vs. TOML — the format is decided, the ambiguity is real. INI with systemd's key names was an explicit product decision upstream of this RFC, so I'm keeping it. But your substantive point stands and I've fixed it: "use So: only the vocabulary transfers, and there's now an explicit grammar table — no continuations, no inline comments, no quoting, Alternative B, partially. Deferring health checks and live reload conflicts with an explicit "readiness and health from day 1" decision, so I can't take it as written. But your underlying point — the first implementation should have a smaller race surface — is right, so I applied it as sequencing instead of scope: PR 4 is now unit core (registry, generations, strict parser, state machine, restart, Alternative A is already the plan — PRs 1-3 are exactly those primitives and land first. Alternative C is what the registry plus generations now implements. One correction to the review: the missing Pipe backing / Wire and CLI / Server restart sections were my splice error and were restored in |
|
Findings
What I’d Do Different Verification: fetched PR head |
27b2d76 to
fe69704
Compare
|
Nine of ten taken, one taken with a different fix. Pushed as Rebase and staleness (P2)Both claims verified, and there was a third. Git moved wholesale The The HELLO precedent is now cited as two append-only extensions rather than one, which is a stronger argument for the pattern than the RFC was making. Wire contracts (P1 + P2)
One thing I had to decide that you didn't ask: The unit record is pinned, one shape used by both
Leases (P2)You're right that this was a sketch. The
Operator intent in PR 4 (P1)Taken, and the contradiction is worse than "contradicting the stated contract" — a
|
|
|
||
| ## What exists today | ||
|
|
||
| Verified against `dc6a265`. |
There was a problem hiding this comment.
Stale verification base after the rebase. The branch was rebased from dc6a265 onto fb98a50, and the rebase commit re-anchored essentially every path:line citation to the new base's line numbers (e.g. lib.rs:1587→1593, C2S_KILL 8872→9177). But this header still says dc6a265, so a reader who trusts it and checks out dc6a265 will find every line number off. Change it to fb98a50. (Relatedly, the one citation the re-anchoring missed: line 49 cites pty_unix.rs:266-284 for reap_zombies, which is now at :297-315 — 266-284 is kill_pty today, and the doc already cites :297-315 for the same function at line 161.)
| Verified against `dc6a265`. | |
| Verified against `fb98a50`. |
pcarrier
changed the title
…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.
Formatting only, no behaviour change. These fail `prettier --check` on this branch and pass on `main`, which has reformatted them since; lint is red here purely because of that drift, so the RFC change this branch carries cannot go green on its own. They will be replaced wholesale when #94 rebases on main — this is to unblock CI in the meantime, not a claim about the right content.
| | `cleanup_pty_internal` sets `exited: true` and keeps the entry | `lib.rs:2483-2497` | | ||
| | The only `ptys.remove` in the server is in the `C2S_CLOSE` arm | `lib.rs:9188` | | ||
| | `max_ptys: 0` (unlimited), hardcoded, no flag, no env var | `crates/cli/src/main.rs:870` | | ||
| | Hitting the cap is a bare `continue` — no reply, and there is no error opcode in the protocol at all, so a nonce-bearing create hangs forever | `lib.rs:8120-8122` + 3 sites | |
There was a problem hiding this comment.
Baseline contradicts the code this branch is now based on. After the latest rebase onto post-#204 main, this row is false: S2C_CREATE_FAILED (0x10) shipped in #204, so there is an error opcode now — which the doc's own Primitives and Delivery sections describe. The two rows just above have the same problem: line 51 ("the only ptys.remove is in the C2S_CLOSE arm") is contradicted by GC's evict_exited, and line 52 ("max_ptys: 0, hardcoded, no flag, no env var") by the shipped BLIT_MAX_PTYS env var + CLI flag. The "What exists today" table describes the pre-#204 world but the branch now sits on post-#204 main, so it should be reconciled the way the rest of the doc already was.
This comment has been minimized.
This comment has been minimized.
A zero-timeout `wait_for_fences` reports `NOT_READY` both for the frame that is still rendering and for one whose channel took a GPU fault and will never complete. Both fence-poll sites treated them alike: re-stash `pending_submit` and return nothing. Since `render_tree_sized` early-returns while a submit is held, one NVRM Xid 73 on the compositor's queue meant no surface was ever composited again — every viewer black or frozen on its last sidebar thumbnail, `blit surface capture` answering "not found or has no buffer" for every surface at once, and not one line in the log to say why. Twice in two hours on the dev stack, each time recovered only by restarting the server (and the apps it hosts). Time tells the two apart. A submission unsignalled for 2s logs once, naming the surface and pointing at `dmesg`. At 5s — or immediately on `ERROR_DEVICE_LOST`, which is a verdict rather than a delay — it is abandoned: its client buffers are released and their explicit-sync release points signalled, so apps keep drawing, and the whole `PendingSubmit` is parked in `abandoned_submits` rather than destroyed, because a faulted context may still reference its command buffer and textures. Nothing it staged is published; a faulted submit's staging buffer is not a frame. The next composite then reaches the queue, which is the only way a fault that killed one channel but left the device usable ever produces a picture again. Live Vulkan Video sessions are handed back through the existing `vulkan_encode_giveups` path: a compositor-resident encoder cannot emit a bitstream from a queue that just faulted, and its subscribers are parked on `vulkan_await` waiting for one. After four abandoned submits the device is not coming back, so the renderer says so and stops submitting — which also bounds the leak at four submissions. `surface capture` names that state too, instead of sending whoever reads it to look at the app.
NVENC's engine has minimum dimensions — 192x128 for AV1, 145x49 for H.264 on this 4090 — and a sidebar preview target is under them: a 256x128 card of a 2318x2235 surface inscribes to 132x128. The preference walk skips every hardware entry on extent alone, so each preview took a scarce Vulkan Video session to encode a thumbnail, on the driver path least exercised by anything else. That is also where the Xid 73 in the previous commit came from, if it came from anything blit chose. Grow the extent to the floor instead: aspect preserved, never past native (the compositor downscales its composite into the target and cannot upscale), cheapest floor by area so H.264's 145x49 wins over AV1's 192x128 rather than whichever is listed first. A 132x74 target becomes 146x82 and lands on NVENC H.264; a surface smaller than every floor keeps its extent and belongs to the tier below. The growth only engages once VA-API has been written off for the host — until then a VA-API candidate outranks the tier and there is nothing to unlock, matching `outranking_encoder_pending`. The in-flight-configure check gets the same growth against the native it is heading for: comparing a grown target with an ungrown projection would read every thumbnail as "the configure will move this" and withhold its frames for as long as a resize was outstanding.
Design-only. Proposes declarative units (systemd-shaped, INI, on-disk, autostart) layered on three primitives that fix existing bugs: group kill, server-enforced deadlines/leases, and GC of exited PTY slots.
… weaknesses The worked-example commit accidentally deleted Pipe backing, Wire and CLI, and Server restart. Restores them, corrects the refuse_lsp_message citation, gives units an explicit identity field rather than reusing the client-chosen tag, and records the design's known weak points.
…ecStop/StartPre - Health gating moves out of Type= into ActiveWhenHealthy= so systemd's Type= semantics stay intact. - Requires= carries systemd's stop propagation rather than a narrowed meaning; a cyclic edit rejects one file, not the whole graph. - Adds ExecStartPre=/ExecStop= and defines helper-child accounting, including SIGCHLD pid dispatch against the existing pty pid registry. - Specifies ReadyMatch's stream, splitting, and bound. - Unit sessions are exempt from max_ptys; max-units bounds them instead. - Persists operator intent (disabled, manually stopped) in KV behind a new server-owned blit/ prefix. - Adds a platform support matrix and a testability section.
…rammar, threat model - A unit is not policy on a Pty: adds an explicit UnitRuntime registry, since units exist while inactive/blocked/RemainAfterExit. The PTY stays the only stream object. - Adds generations and an ordered restart sequence, so SIGCHLD-triggered restarts cannot let a retired reader mutate a new generation. - SIGCHLD wakes the supervisor for targeted waitpid over owned pids; the global waitpid(-1) drain is deleted, not rescheduled, so it cannot steal ExecHealthCheck/audio/LSP statuses. - Deadlines become independent causes with an enforced minimum, plus server-minted lease ids, epochs, and single-holder reclaim. - Replaces 'man systemd.service is the reference' with an explicit strict grammar; only the vocabulary transfers. - Security: unit dirs excluded from the fs family and install made a local capability. Mode checks do not distinguish a remote fs writer from the operator, which is the attacker the section named. - State machine becomes a transition table; health failures thresholded; After= documented as an initial gate; live-reload semantics defined; unsupported platform capabilities are load errors, not warnings. - Splits unit delivery into core and policy stages.
Unit directories are no longer excluded from the fs family and install is no longer a local-only capability: remote install is intended. The security section states the equivalence instead — fs write access to a unit directory is durable code execution — and keeps BLIT_UNITS=0, load-time hygiene, and audit logging as the controls that actually earn their place. Also drops every 'an earlier draft said' retrospective (that belongs in the review thread), folds prose into tables, and cuts repeated rationale: 7744 -> 5812 words with no section or contract removed.
… gaps Re-verified every citation against current main (git moved to 0xA0-0xB4, HELLO gained server_version). Adds S2C_UNIT_DONE, the exact unit record layout, UnitName grammar, lease op/status semantics, Job Object group kill on Windows, SCM_CREDENTIALS attribution for Type=notify, and moves operator-intent persistence into the same PR as autostart.
| 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.
Every file:line citation had drifted; #204 moved most of them and twelve more commits moved the rest. Refreshed all of them against 1919717 and rewrote 'What exists today' to say what is true after #204 rather than before it, marking which rows it changed. Three wire claims collided with merged reality: #167 took feature bit 11 and C2S/S2C 0x90-0x95 for extensions and channels, #173 took bit 13 and 0xC0-0xC6 for processes, and #260 took C2S 0x1E / S2C 0x11 for scroll-by. The unit family moves to 0xD0 on feature bit 20 and the lease pair to C2S 0x1F / S2C 0x12. Two corrections to what #212 recorded: the timed half of the C2S_CLOSE escalation did ship, on the abandoned pid rather than a 'closing' slot; and the create refusal reaches only C2S_CREATE2, so the silent drop survives on the three legacy create opcodes.
c0b5f5c to
82d70dc
Compare
|
Both reds are external to this PR, which only adds a markdown file.
|
176dc25 to
a0ccf8d
Compare
1 participant
Design only — no code. Adds
docs/design/units.md.What this proposes
A unit is a named, declarative supervisor for a blit session: an on-disk file saying what to run, when to start it, when to restart it, when it is ready, when it is healthy, and what it depends on. Systemd's model, down to the key names.
The framing that keeps it blit-shaped: a unit introduces no new object.
C2S_RESTARTalready respawns an exited child in place, reusing the same pty id and driver (lib.rs:8683,pty_unix.rs:570), so blit already has a session identity that outlives a process. A unit is that identity made declarative — policy on an existingPtyentry, keyed by its existing tag. Clients subscribe once and follow a unit across restarts, with continuous scrollback as the blit-native journal. The alternative, a second process model bolted alongside the PTY map, is what the design exists to avoid.Three primitives first
Units sit on three changes that fix real bugs and are worth having alone:
blanket_frame_intervalreturnsNonewith no clients (lib.rs:2731), so the tick loop'snext_deadlinegoesNoneand a silent runaway is never visited on any schedule. The 5 sreap_zombiestask becomes a 1 s state-carrying supervisor tick.C2S_KILLsignals the leader pid only andC2S_CLOSE's SIGHUP misses anything that changed process group, even though the code already pgrp-signals for SIGWINCH (pty_unix.rs:224-230).KillMode=process-groupbecomes the default.ptys.removein the server is in theC2S_CLOSEarm,lib.rs:8883) andmax_ptysis hardcoded unlimited (crates/cli/src/main.rs:802). Adjacent standalone bug: hitting the cap is a barecontinuewith no reply, and there is no error opcode in the protocol at all, so a nonce-bearing create hangs forever.Decisions baked in
INI with sections and systemd's exact key names (four documented deviations); readiness and health both in v1;
Backing=pipestill feeds the alacritty driver so scrollback/search/COPY_RANGE/rendering keep working; the full deadline surface including connection leases that resolve into the deadline primitive rather than adding a second kill path.Health gates activation — departing from systemd, a unit with
ExecHealthCheck=reachesactiveonly after its first probe passes. That single rule makesAfter=mean "healthy" with no second ordering keyword, and withType=oneshot/surfaceit makes this expressible:Three different definitions of "up" — a
sd_notifyhandshake plus a passing health probe, a Wayland surface, a command that exited zero — and the dependent expresses its requirement without knowing which.Delivery
One axis per PR: (1) group kill, (2) supervisor tick + deadlines + leases, (3) GC +
max_ptys+S2C_CREATE_FAILED, (4) units. The first three are the incident-shaped fixes and stand alone.Every code claim in the doc is cited to
path:lineand verified againstdc6a265. Open questions are called out inline, notablyReadySurface=matching byapp_idrather than by process — surfaces are keyed by Wayland object id and the compositor records no client credentials today.Tag
@indentto continue the conversation here.