RFC: Supervised sessions with lifecycle policy

[pcarrier]

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_RESTART already 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 existing Pty entry, 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:

• Deadlines — every timeout is client-side today, so a hung command outlives a disconnected orchestrator forever. Worse than it looks: blanket_frame_interval returns None with no clients (lib.rs:2731), so the tick loop's next_deadline goes None and a silent runaway is never visited on any schedule. The 5 s reap_zombies task becomes a 1 s state-carrying supervisor tick.
• Group kill — C2S_KILL signals the leader pid only and C2S_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-group becomes the default.
• GC — nothing frees an exited PTY slot (the only ptys.remove in the server is in the C2S_CLOSE arm, lib.rs:8883) and max_ptys is hardcoded unlimited (crates/cli/src/main.rs:802). Adjacent standalone bug: hitting the cap is a bare continue with 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=pipe still 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= reaches active only after its first probe passes. That single rule makes After= mean "healthy" with no second ordering keyword, and with Type=oneshot/surface it makes this expressible:

# open-url.unit
[Unit]
Requires=api chromium
After=api chromium
[Service]
Type=oneshot
RemainAfterExit=yes
ExecStart=<browser-driver> open http://localhost:8080

Three different definitions of "up" — a sd_notify handshake 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:line and verified against dc6a265. Open questions are called out inline, notably ReadySurface= matching by app_id rather than by process — surfaces are keyed by Wayland object id and the compositor records no client credentials today.

Tag @indent to continue the conversation here.

[github-actions]

Coverage

Crate	Lines	Functions	Regions
alacritty-driver	69.7% (698/1002)	72.0% (54/75)	71.7% (1050/1464)
browser	0.0% (0/807)	0.0% (0/65)	0.0% (0/1370)
cli	17.3% (1521/8803)	28.1% (200/711)	20.2% (2730/13540)
compositor	1.0% (93/9248)	2.0% (8/400)	1.2% (146/12403)
fonts	81.4% (721/886)	88.6% (70/79)	83.0% (1427/1719)
fssync	92.5% (4876/5274)	94.1% (445/473)	92.5% (8920/9647)
gateway	26.1% (375/1437)	29.9% (38/127)	19.9% (470/2360)
git	87.3% (4165/4771)	88.4% (329/372)	87.0% (6594/7582)
lsp	76.0% (2503/3295)	78.2% (248/317)	73.8% (3887/5266)
proxy	19.3% (172/892)	20.5% (26/127)	21.2% (293/1381)
remote	90.0% (8545/9497)	92.7% (614/662)	87.7% (14147/16134)
sd-notify	73.9% (68/92)	100.0% (6/6)	83.2% (109/131)
server	36.2% (6164/17050)	49.5% (592/1195)	38.8% (10516/27087)
ssh	1.9% (7/374)	3.2% (1/31)	0.7% (4/613)
upsidedown	31.4% (391/1247)	27.8% (55/198)	34.8% (797/2287)
webrtc-forwarder	2.7% (72/2624)	2.1% (4/187)	1.2% (50/4335)
webserver	59.8% (1032/1726)	64.6% (155/240)	62.8% (1782/2836)
Total	45.5% (31403/69025)	54.0% (2845/5265)	48.0% (52922/110155)

[indent]

PR Summary

Design PR — adds docs/design/units.md, an RFC proposing systemd-style units: a declarative supervisor layer over blit's existing PTY sessions. The three foundational primitives it was built on (group kill, the reactive supervisor loop + deadlines, and GC + an error opcode) have since shipped in #204; this PR now carries only the RFC doc plus prettier-only reformatting of a few js/ui files. The remaining proposal is the unit layer itself.

• Defines a UnitRuntime registry (the PTY stays the only stream object), per-restart generations for race-safe restart-on-leader-death, unit identity via a server-owned non-wire unit field, a strict execve-direct INI grammar, the state machine, readiness/health probes with thresholds, faithful Requires= stop-propagation, Backing=pipe, a 0x90 wire family (gated on FEATURE_UNITS bit 17), and the CLI.
• Reconciles the doc with how server: own the PTY lifecycle — refusals, group kill, supervisor, deadlines, retention #204 actually landed: S2C_CREATE_FAILED at 0x10 with a common status registry (so S2C_LEASE moves to 0x11), opt-in refusal via CREATE2_WANT_STATUS/FEATURE_CREATE_STATUS, max_ptys kept at 0, the C2S_KILL flags arm at len >= 8, and KillMode shipped as KILL_LEADER_ONLY/FEATURE_KILL_MODE. All verified against main. Delivery now marks items 1-3 shipped, leaving the lease family and the timed C2S_CLOSE escalation open.
• Group kill is portable (Windows Job Object); security model settled (install through fs, boundary at the socket + BLIT_UNITS=0).
• The doc's central claim is that every path:line citation is verified against its stated base; one open nit remains: the "Verified against dc6a265" header and one reap_zombies citation still point at the old base's line numbers after the rebase.

Issues

1 potential issue found:

• The doc's "Verified against dc6a265" header (line 39) is stale: its path:line citations were re-anchored to a newer base during the rebase, and the doc now also asserts protocol facts from server: own the PTY lifecycle — refusals, group kill, supervisor, deadlines, retention #204 (which postdates dc6a265), so the header names a base matching neither. Separately, line 49 still cites pty_unix.rs:266-284 for reap_zombies, which is at :297-315 (266-284 is now kill_pty; the doc cites :297-315 correctly at line 161). → Autofix

2 issues already resolved

• The "flesh out the worked example" commit (174388e) also deleted the Pipe backing, Wire and CLI (the 0x90 opcode table, FEATURE_UNITS bit, CLI subcommands, learn.md), and Server restart sections — but Delivery item 4 and Constraints still reference that removed content, and the reworked example now says "setpgid(0,0) noted above" with no such note left, indicating the deletion was unintentional. (fixed by commit 23f3032)
• Citation drift: the doc cites the refuse_lsp_message refusal pattern at lib.rs:7411-7422, but those lines are the git open-rebase path; the actual refusal dispatch is lib.rs:7431-7439 and the function is defined at lib.rs:6928. (fixed by commit 23f3032)

⚡ Autofix All Issues

_{View session}

[pcarrier]

🐮 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 4ee4989 against base dc6a265. The motivation is strong, and the three standalone primitives—group kill, server-side deadlines, and bounded exited-session retention—look independently valuable. Before implementing the unit layer, I think the RFC should resolve the lifecycle and trust-boundary questions below.

Highest-priority design issues

1. A unit cannot be only policy attached to an existing Pty

The central claim is that a unit “introduces no new object” and is keyed by the PTY’s existing tag. That seems incompatible with the rest of the design:

• An inactive, failed-to-load, dependency-blocked, or autostarting unit exists before any PTY does.
• A unit with RemainAfterExit=yes exists after its process exits.
• Reloading a definition requires retaining configuration and runtime state independently of a process generation.
• Existing tags are arbitrary, optional, and not unique. Creation accepts empty or duplicate tags without checking existing PTYs (crates/server/src/lib.rs:7777-7817).

I would introduce a small explicit registry:

UnitRuntime {
    name: UnitName,
    definition: UnitDefinition,
    state: UnitState,
    current_pty: Option<PtyId>,
    generation: u64,
}

The PTY can remain the sole stream/terminal object. A registry pointing to the current PTY is orchestration state, not a competing process model. Unit identity should be a validated unique name or reserved tag namespace, rather than an unconstrained client tag. The RFC should also define what happens if an ordinary client creates a PTY whose tag matches a loaded unit.

2. SIGCHLD must not blindly reuse the current global reaper

The RFC replaces polling with a reactive SIGCHLD handler, but does not specify the reaping algorithm. Today reap_zombies() calls waitpid(-1, …) and discards statuses for children not registered as PTYs (pty_unix.rs:246-284). Running that immediately on every SIGCHLD would make it much more likely to steal exit statuses from health-check helpers, audio children, LSP processes, and future Command-owned children. That directly conflicts with periodic ExecHealthCheck processes.

The signal handler could wake the supervisor, which then calls waitpid(known_pty_pid, WNOHANG) for each registered PTY child. A global waitpid(-1) drain seems safe only if child ownership is centralized for the whole server.

3. Leader death before PTY EOF creates stale-generation races

The RFC correctly identifies the case where the session leader dies while a grandchild keeps the slave open. Restarting immediately on SIGCHLD creates another problem:

• the old PTY reader can remain alive;
• old descendants can keep producing output;
• respawn_child opens a new master and starts another reader;
• eventual EOF/output from the old reader can arrive against the same PTY ID and mutate the new generation.

Today in-place respawn is safe largely because it occurs after the old PTY reaches the EOF/exited path. The proposal removes that precondition without defining replacement cleanup.

Every asynchronous event should probably carry (pty_id, generation). Before restart, the RFC should define a sequence such as:

1. Record leader exit status.
2. Terminate the remaining old process group.
3. Close the old master.
4. Retire/join the old reader.
5. Increment generation.
6. Spawn the replacement.
7. Ignore late events from previous generations.

4. Deadlines and leases do not yet compose

The RFC says all bounds resolve to one enforcement path, while lease disconnect arms a deadline and reconnect clears it. With one apparent deadline field, several races are undefined:

• An explicit C2S_DEADLINE can be overwritten by a lease-disconnect deadline.
• Reclaim could clear an unrelated explicit or RuntimeMaxSec deadline.
• An old connection can disconnect after a replacement connection reclaims the same lease, rearming its deadline.
• Two simultaneous connections presenting one lease ID have no ownership semantics.
• S2C_EXITED attribution is ambiguous when multiple constraints expire together.

I would model independent causes and enforce their minimum:

effective_deadline =
  min(explicit_deadline,
      lease_deadline_for_current_epoch,
      runtime_max_deadline,
      stop_escalation_deadline)

Reclaim should remove only the matching lease constraint. A connection epoch or holder count is needed so stale disconnects cannot revoke newer claims. A server-minted unguessable token may be safer than a client-selected bare u64; either way, issuance, collision, and concurrent-holder semantics should be explicit.

5. The persistence security control does not address the stated attacker

The RFC correctly says units add persistence and can be written through the filesystem protocol, then proposes rejecting group/world-writable files and directories. But a remote filesystem client writing through the server will normally create a server-user-owned, non-world-writable file in the user directory, so it passes those checks. Mode checks protect against another Unix account; they do not distinguish a trusted local operator from the remote client in the threat model.

Possible policies:

1. Exclude unit directories from the remote filesystem API.
2. Require unit installation through a local-only administrative CLI/control socket.
3. Autostart only system-owned /etc/blit/units by default; gate user autostart behind an explicit server option.
4. Treat remote unit installation as a separate capability from C2S_CREATE.
5. At minimum, document that filesystem capability grants durable code-execution persistence and that mode checks do not mitigate that case.

Symlinks, ownership, atomic open/validation, and writable parent-directory traversal also need definition.

6. “Use man systemd.service as the reference” is too ambiguous for a small custom parser

The proposed roughly 60-line parser plus systemd-compatible names leaves material questions:

• Is ExecStart shell-interpreted? systemd’s generally is not /bin/sh -c.
• Are quoting, escaping, continuations, specifiers, and environment expansion supported?
• Do repeated keys replace or append?
• Are dependency lists whitespace-separated?
• Are duration suffixes (30s, 500ms, infinity) accepted?
• Are unknown sections/keys fatal, warnings, or ignored?
• What are duplicate-section, malformed-line, and inline-comment semantics?

Because these files persistently execute code, permissive or surprising parsing is risky. I would either define a deliberately small grammar and say only the vocabulary transfers, use a mature parser for actual systemd semantics, or use a typed format such as TOML and consider an import tool later. “No parser crate currently exists in the workspace” does not seem like a strong reason to own a compatibility-sensitive parser.

Other clarifications worth making

Reject unsupported readiness modes instead of silently weakening them

Type=notify falling back to simple on Windows breaks the core promise that After= waits for real readiness. Prefer a load error, or require an explicit fallback. The RFC also needs a platform matrix covering process-tree kill, cgroups/Job Objects, notify sockets, and reactive child exit.

Make the state machine an event/transition table

The diagram omits restart backoff, reload, stop escalation, readiness failure, and generations. It also says Restart= applies on active → inactive|failed, but separately applies it to activation timeout. Please define transitions for spawn failure, readiness timeout, health failure while activating/active, watchdog expiry, explicit stop, config removal, stale-generation EOF, and start-limit exhaustion.

Clarify that health-based dependencies are only an initial gate

The headline can read as a maintained invariant—“that unit is answering its health check”—but v1 waits only for the first passing probe and explicitly excludes propagation. That is a reasonable policy, but I would state it directly:

After= gates initial activation on the dependency’s first healthy state; it does not maintain a continuous health invariant.

Consecutive success/failure thresholds may also be useful; restarting on one transient failed curl seems aggressive.

Document live-reload semantics

Define whether changes to command/environment/working directory, dependencies, health intervals, restart limits, autostart, removal, and parse failures apply immediately, on the next restart, or after an explicit restart. A safe pattern is to parse and validate a complete new registry, then atomically swap definitions; an invalid edit should not partially mutate live policy.

Existing inline findings

I agree with the existing review notes that:

• the worked-example commit appears to have removed the Pipe backing, Wire and CLI, and Server restart sections while Delivery still promises them; and
• the refuse_lsp_message citation has drifted.

The missing server-restart and wire sections are especially relevant because persistence and capability negotiation affect several issues above.

Possible narrower alternatives

A. Land only the incident-shaped primitives first

Proceed with group/process-tree kill, per-session monotonic deadlines, nonce-correlated create failures, and bounded exited-session retention. Defer leases and units until timer provenance, reconnect ownership, and process-generation handling have been exercised.

B. Start with a smaller unit MVP

Use an explicit UnitRegistry, initially supporting only:

• Type=simple | oneshot | notify;
• ExecStart;
• After and Requires;
• restart/backoff;
• start/stop timeouts;
• PTY backing only.

Defer surface matching, regex matching, health checks, pipe backing, leases, live reload, and watchdogs. This keeps the core value while reducing the first implementation’s race surface.

C. Separate stable unit identity from terminal/process generations

Unit → current ProcessGeneration → PTY stream

The existing terminal driver can remain attached across generations where safe, but lifecycle events carry a generation number. This preserves continuous scrollback without requiring unit identity, PTY identity, and process identity to be identical.

Again, @indent: all of this is open for pushback. The RFC has a compelling core; my main ask is to make the ownership, generation, composition, parsing, and persistence contracts explicit before implementation locks them in.

[indent]

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 4ee4989; 23f3032 had already fixed the two inline findings).

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.

Taken

1. A unit is not policy on a Pty. You're right and my patch two commits ago was only half a fix — I'd moved identity off tag onto a unit: Option<String> field, which still can't represent a unit that is inactive, dependency-blocked, failed-to-load, or RemainAfterExit=yes past its process. Adopted UnitRuntime with current: Option<(PtyId, Generation)>. What survives is the narrower claim you granted: the PTY stays the only stream object and the registry points at it. A client PTY whose tag collides with a unit name is now explicitly left alone — not adopted, not refused, never in S2C_UNIT_LIST.

2. SIGCHLD must not reuse the global reaper. Confirmed against the source and this is a bug I was introducing: reap_zombies drains waitpid(-1, WNOHANG) and discards statuses for pids outside pty_pids() (pty_unix.rs:266-284). The audio pipeline already lives with that race at 5s; running it per-SIGCHLD widens it sharply, and I was adding periodic ExecHealthCheck= children straight into its path. Now: the handler only wakes the supervisor, which does targeted waitpid(pid, WNOHANG) over pids it owns (PTY + helper children, via the existing register_pty_pid/pty_pids() registry generalized). The global drain is deleted rather than rescheduled — Command-owned children keep being reaped by their own owners, and no status is collected by a party that didn't spawn it. Strictly safer than today.

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 Generation on every unit-owned PTY, (pty_id, generation) on every async event, and the ordered sequence you sketched — record status, kill old group, close master, retire and join the reader, bump generation, spawn, drop stale-generation events. That last step is the cheap backstop that makes the rest safe to get slightly wrong.

4. Deadlines and leases don't compose. "All resolve to one enforcement path" was hand-waving. Now independent causes with effective = min(explicit, lease[current_epoch], runtime_max, stop_escalation), each armed and cleared only by itself. Leases get server-minted ids (a client-chosen u64 is a namespace anyone on the socket can guess, for something that is a kill switch on other people's sessions), an epoch bumped on reclaim so a stale disconnect can't revoke a newer claim, and single-holder semantics. S2C_EXITED attributes to the cause that produced the minimum, ties broken in a fixed order, so it's deterministic rather than whichever timer fired first.

5. The security control didn't address its own attacker. Completely right and this was the worst of them. A remote fs client writes as the server's user, 0644, in the user's own directory — it passes every mode check. Mode bits distinguish another Unix account, not a socket from a keyboard. Rewrote around the distinction that actually matters: C2S_CREATE is ephemeral execution scoped to a connection, a unit file is durable execution that outlives the connection, the client, and the process. Treating them as one capability is how a session compromise becomes permanent. So — your options 1, 2, 3 and 5 together: unit directories excluded from the fs family; install is a local capability (START/STOP/RESTART stay on the wire, since exercising already-installed policy isn't introducing code); /etc/blit/units autostarts by default with the user directory gated behind an explicit option; mode/symlink/parent-traversal checks demoted to depth; and the equivalence documented plainly.

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 (HealthCheckFailureThreshold, default 3) — you're right that restarting on one lost curl race is worse than the problem. After= is documented as an initial gate, in your words. Live reload defined as validate-whole-registry-then-atomic-swap, with a per-key table of what applies now vs. at next start. Unsupported platform capabilities are now load errors, not warnings — you're right that Type=notify silently degrading breaks the one promise After= makes, and I'd traded a loud failure for a silent one.

Pushback

6. 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 man systemd.service as the reference" is not implementable, and I've deleted that sentence. systemd's syntax carries specifiers, ExecStart= prefixes (-, @, +, !), continuations, its own quoting, drop-in merge order and EnvironmentFile= expansion; implementing a subset of that in 60 lines while claiming compatibility is exactly how a code-executing file format acquires surprises.

So: only the vocabulary transfers, and there's now an explicit grammar table — no continuations, no inline comments, no quoting, argv split on whitespace and execve'd directly (never /bin/sh -c), no specifiers, repeated scalar keys are an error rather than last-wins, list keys append, durations take ms/s/m/h, unknown section or key is an error. Strict beats permissive throughout, and it can be relaxed later where permissive can't be tightened. I'd also push back on "use a mature parser for actual systemd semantics": that buys a compatibility surface we've deliberately scoped out, and the failure mode isn't parse errors, it's users reasonably expecting %i and drop-ins to work.

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, Requires=/After=, Type=simple|oneshot|notify, PTY backing only) and PR 5 is unit policy (health + thresholds + ActiveWhenHealthy=, watchdog, Type=match, Type=surface, Backing=pipe, live reload, KV operator intent). Nothing is dropped, but the generation and ownership contracts get exercised by a PTY-only, probe-free core before a second spawn path and live reload land on top.

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 23f3032, before this round. (I then managed to repeat the identical mistake while editing this revision and caught it on re-read — the failure mode is real enough that I'm now diffing section headers before every push.)

[pcarrier]

Findings

• [P1] UNIT_START|STOP|RESTART|RELOAD are nonce-bearing, but only UNIT_LIST has a nonce-correlated response. UNIT_STATE is async and has no nonce/status, so the CLI cannot reliably report not found, disabled, dependency failed, parse failure, etc. Add something like S2C_UNIT_DONE [nonce:2][status:1][detail_len:2][detail:N]. docs/design/units.md:580-589

• [P1] Delivery order splits persisted operator intent from the feature that needs it. PR 4 includes autostart/stop, but PR 5 adds the reserved KV prefix. That means blit unit stop api can re-autostart after a server restart during PR 4, contradicting the stated contract. Move operator-intent persistence into unit core, or defer autostart/stop until KV reservation lands. Also define behavior when BLIT_KV=0. docs/design/units.md:595-615, docs/design/units.md:861-867

• [P2] KillMode=process-group is the default, but Windows marks it unsupported and unavailable capabilities are load errors. A portable unit omitting KillMode= would fail on Windows. Either make the default platform-specific, introduce a portable “process-tree” abstraction using Job Objects on Windows, or require KillMode=process in portable units. docs/design/units.md:92-106, docs/design/units.md:617-638

• [P2] The grammar rejects inline comments, but the reference example uses inline comments on value lines, making the example invalid and ReadyMatch materially wrong. Either remove those comments from the INI block or support inline comments explicitly. docs/design/units.md:317-331, docs/design/units.md:337-376

• [P2] The unit wire format is not yet implementable enough. S2C_UNIT_LIST says “name, state, pty id, generation, restart count, last exit, health” without lengths, enum values, sentinels for “no PTY”, status codes, or health encoding. UnitName is “validated” but no grammar/length/case/path rules are given. Define the exact binary schema now; the wire is version-stable. docs/design/units.md:249-271, docs/design/units.md:573-589

• [P2] The RFC is stale relative to current origin/main. It says git occupies 0x50; current main moved git to 0xA0/0xB0. S2C_HELLO also now includes trailing server_version, not just boot generation. The proposed 0x90 block still looks free, but the rationale/citations should be refreshed before merge. docs/design/units.md:68-82

• [P2] Lease creation/reclaim semantics are incomplete. The server “mints lease_id”, but C2S_LEASE already carries [lease_id:8]; flags is unnamed; release/create/reclaim behavior is not specified. This matters because leases are the reconnect/dead-man-switch primitive. docs/design/units.md:187-204

• [P3] Type=notify should define sender attribution. A per-generation NOTIFY_SOCKET is not enough if any same-user process can send READY=1; use peer credentials where available and define main|group|all semantics. Also, the existing crates/sd-notify client is Linux-only, so “Unix yes” may be too broad. docs/design/units.md:455-461, docs/design/units.md:626

• [P3] Type=surface admits false readiness when two units use the same app_id. Until compositor pid attribution exists, consider rejecting duplicate ReadySurface= values or documenting it as best-effort. docs/design/units.md:470-479

• [P3] The RFC date is in the future: it says 2026-08-04; today is 2026-08-01. docs/design/units.md:3-4

What I’d Do Different
I’d keep the core direction: explicit UnitRuntime, targeted SIGCHLD, generations, strict parser, and staged delivery are the right shape. Before accepting the RFC as implementation-ready, I’d tighten the wire/status contracts, move persistence into the first unit PR, and make platform semantics explicit. Those are the places where ambiguity becomes expensive later.

Verification: fetched PR head 27b2d76, compared against merge-base dc6a265 and current origin/main 257c254; gh pr checks 94 is green; git diff --check origin/main...origin/pr/94 is clean.

[indent]

Nine of ten taken, one taken with a different fix. Pushed as fe69704, on top of a rebase onto aae38a8 — the branch was based on dc6a265, which is why the staleness finding was right about more than it said.

Rebase and staleness (P2)

Both claims verified, and there was a third. Git moved wholesale 0x50-0x5E → 0xA0-0xB4 and gained DISCOVER/BLAME/REFLOG/FETCH (crates/remote/src/git.rs); S2C_HELLO gained a trailing [ver_len:u16][server_version:N] after the boot generation (crates/remote/src/lib.rs:1601-1613). I re-read all 29 citations line by line rather than re-deriving the ones that looked stable — 27 had moved, including every lib.rs line (C2S_KILL 8872→9177, C2S_CREATE 7777→8082, max_ptys main.rs 802→870) and both BlitConnection.ts ones. config.rs:632 was also just wrong; it's crates/webserver/src/config.rs:701.

The 0x90 claim survives, and I've stated the free space instead of the pattern, since "the next 0x?0 block" stopped being a rule the moment git jumped to 0xA0. Verified free on current main: C2S 0x1D-0x1F, S2C 0x10-0x1F, and all of 0x90-0x9F, 0xC0-0xFF. FEATURE_* bits 0-10 are contiguously allocated, so bit 11 holds.

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)

S2C_UNIT_DONE added, [nonce:2][status:1][detail_len:2][detail:N], statuses ok / not-found / invalid / permission / unloadable / dependency-failed / timeout / conflict / internal, detail UTF-8 capped at 1 KiB. UNIT_LIST answers S2C_UNIT_LIST; everything else answers UNIT_DONE, so the family finally satisfies the refusal rule its own Constraints section states.

One thing I had to decide that you didn't ask: UNIT_DONE fires on the terminal outcome, not on acceptance. blit unit start api should block and exit nonzero when api cannot come up, and that is only knowable after readiness resolves — acceptance-only would make dependency-failed and timeout unreachable, which is the case you raised. It's bounded by TimeoutStartSec plus each dependency's, so it always arrives; --no-block returns at acceptance for callers that want the old shape.

The unit record is pinned, one shape used by both UNIT_LIST and UNIT_STATE so there's one decoder:

[name_len:1][name:N][state:1][health:1][pty_id:2][generation:8]
[restarts:4][exit_kind:1][exit_status:1][autostart:1][enabled:1]

pty_id: 0 is the no-PTY sentinel, which is free because allocate_pty_id rotates from 1 and wraps u16::MAX → 1 (lib.rs:1769, asserted at :12437). state is the five states the state-machine table actually uses; the draft's list and the table's had drifted apart. exit_kind separates "never exited" from "exited 0". Every field fixed-width, so a later field appends and old clients length-gate past it.

UnitName = [A-Za-z0-9_.-]{1,64}, ASCII, case-sensitive, not . or ... No /, so traversal is unrepresentable; the name comes from the filename, never from a key inside the file, and two directories offering the same name is a load error naming both paths rather than a silent shadow.

Leases (P2)

You're right that this was a sketch. The flags byte is now an op byte, because the three operations are mutually exclusive and a flag set invites the combinations:

op	lease_id in	Effect
0 CREATE	must be 0	mint, hold, epoch 1
1 RECLAIM	the lease	take holder, bump epoch, replace grace_ms, disarm the lease cause
2 RELEASE	the lease	drop holder without arming grace, disarm the lease cause

S2C_LEASE gains an opcode (0x10) and a status byte (ok / unknown-lease / invalid-op / permission) — no nonce needed since the reply carries the lease_id. And a lease with no holder and no tagged session is dropped, otherwise CREATE in a reconnect loop leaks table entries. S2C_CREATE_FAILED got its missing opcode too (0x11).

Operator intent in PR 4 (P1)

Taken, and the contradiction is worse than "contradicting the stated contract" — a blit unit stop that un-stops itself at the next restart isn't a missing feature, it's a broken promise, so shipping autostart without persistence ships a bug on purpose. Persistence and the blit/ reservation moved into PR 4 alongside autostart.

BLIT_KV=0: the supervisor reaches the store in process through the OnceLock<Mutex<Store>> at kv.rs:556, not over the wire. BLIT_KV=0 withholds the feature bit and refuses KV_* at dispatch (lib.rs:7287, :7772) — a control on the client-facing surface, and units have no client-facing KV surface to withhold, so it doesn't disable them. What does defeat persistence is no resolvable state dir, where the store is memory-only (kv.rs:324-326); that's now logged once at load rather than discovered when a stopped unit comes back.

KillMode on Windows (P2) — taken, different fix

Real contradiction, and neither of your three options is what I'd do. A platform-specific default means the same file kills differently on two machines, and requiring KillMode=process in portable units makes the safe thing opt-in.

Windows gets a Job Object. CreateProcessW (pty_windows.rs:287) creates the child suspended, AssignProcessToJobObject, resume; TerminateJobObject is the group kill and JOB_OBJECT_LIMIT_KILL_ON_JOB_CLOSE gives close_pty the containment the Unix side gets from SIGHUP-to-the-group. That is also strictly better than today's bare TerminateProcess (pty_windows.rs:70-77), which orphans every grandchild — so it's worth doing whether or not units land. Moved into PR 1.

That still left the general trap, so the rule is now explicit: defaults are resolved per platform before the capability check; only keys written in the file can produce a load error. Otherwise the next default with a platform gap rejects every unit that omits it.

Type=notify attribution (P3)

Right on both halves, and the second one changes the answer. A per-generation socket path is not attribution — anything running as the same user can send READY=1. Added SO_PASSCRED with an SCM_CREDENTIALS pid check and NotifyAccess=main|all (main = the leader pid, all = the unit's process group; systemd's exec omitted, there's no ExecStartPost to scope it to).

But SCM_CREDENTIALS is Linux-only, and macOS gives peer credentials for SOCK_STREAM only — a datagram readiness protocol there is unauthenticated by construction. Combined with crates/sd-notify already being #[cfg(target_os = "linux")] with a no-op stub, Type=notify is Linux-only in the matrix now. Rejecting the key beats shipping readiness anything on the box can forge.

Smaller ones

Inline comments (P2). Straightforwardly broken: ReadyMatch=^Listening on # Type=match only parses as that whole string. Comments moved to their own lines rather than relaxing the grammar — a value-terminating # in a file whose values are argv and regexes is a footgun, and strict can be relaxed later.

Duplicate ReadySurface= (P3). Now a load error, so unit B's window can't mark unit A ready. The residual — a non-unit client owning a matching app_id — is documented, not defended, until SO_PEERCRED reaches Surface.

Date (P3). Fixed, 2026-08-01.

Only substantive thing I chose against your framing is the Windows one, and it argues for more work rather than less. CI is green; the doc is 1020 lines.

[indent]

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

Suggested change

	Verified against `dc6a265`.
	Verified against `fb98a50`.