natsukium/felis

Watch

[v0.1/P1] Add portable daemon stop and drain operations #25

New issue

Closed

opened 2026-09-03 16:18:05 +09:00 by natsukium · 1 comment

natsukium commented

2026-09-03 16:18:05 +09:00

Owner

Parent: #12 (P1.6). Related to #10.

Why

The update guide relies on pkill felis-daemon, which is Unix-only, races new session creation, and can destroy every session accidentally. The daemon needs a portable typed control path.

Scope

Add felis daemon stop over the normal IPC carrier.
By default, atomically refuse while sessions remain and return the count.
Add --force for explicit destruction and stop.
Add --when-empty to enter draining state, reject new creates, and exit after the last session.
Support global local/remote carrier flags and Windows.

Acceptance criteria

The emptiness decision cannot race a successful new create.
Default stop preserves existing sessions.
Draining state and refusal are typed and observable.
Force semantics are explicit in human and machine output.
Update guide no longer instructs users to use pkill.
IPC, CLI, completions, man pages, skills/felis, and changelog are updated.

Parent: #12 (P1.6). Related to #10. ## Why The update guide relies on `pkill felis-daemon`, which is Unix-only, races new session creation, and can destroy every session accidentally. The daemon needs a portable typed control path. ## Scope - Add `felis daemon stop` over the normal IPC carrier. - By default, atomically refuse while sessions remain and return the count. - Add `--force` for explicit destruction and stop. - Add `--when-empty` to enter draining state, reject new creates, and exit after the last session. - Support global local/remote carrier flags and Windows. ## Acceptance criteria - [ ] The emptiness decision cannot race a successful new create. - [ ] Default stop preserves existing sessions. - [ ] Draining state and refusal are typed and observable. - [ ] Force semantics are explicit in human and machine output. - [ ] Update guide no longer instructs users to use `pkill`. - [ ] IPC, CLI, completions, man pages, `skills/felis`, and changelog are updated.

natsukium added the

priority/P1

release/v0.1.0

labels

2026-09-03 16:18:06 +09:00

natsukium referenced this issue

2026-09-03 16:18:49 +09:00

[v0.1/Tracker] Close release blockers before v0.1.0 #12

natsukium commented

2026-09-03 20:35:14 +09:00

Author

Owner

Triage plan (2026-09-03)

Source-grounded triage against main at 69076d42, reviewed through seven rounds of an independent reviewer (pi sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. Where a "Review amendments" section below conflicts with an earlier section, the amendment is the decision.

Claim check

Accurate.

The update guide relies on pkill. docs/how-to/update-felis.md:58-61 (pkill felis-daemon) and :92 (ssh user@remote pkill felis-daemon). The race is acknowledged in the same guide, :48-52: "Nothing stops you starting a new session between the check and the restart: felis has no quiesce mode". CHANGELOG.md:493, :903 teach the same.
Unix-only and abrupt. crates/felis-daemon/src/main.rs:31-34: "Runs until the process is killed: there is no signal handling, and an abrupt death closes the PTY masters, which SIGHUPs the children". The daemon builds and is CI-gated on Windows (memory: 2026-09-03 Windows runtime gate green; felis-pty ConPTY backend per CLAUDE.md), where there is no pkill and no SIGTERM.
No stop verb exists. crates/felis-cli/src/cli_daemon.rs:20-33: DaemonOp has only Status. OpsMsg (felis.proto:1143-1160) has no stop arm. docs/explanation/architecture/session-lifecycle.md:471-475 lists idle-daemon exit as a revisit item ("the daemon process has no idle self-exit").
The emptiness race is closable where the count lives. crates/felis-daemon/src/pool.rs:483-490 try_reserve(max) checks sessions.len() + reserved under the pool mutex, and serve.rs:485-489 calls it under pool.lock().await. A draining flag checked inside try_reserve makes "refuse new creates" and "count is zero" one critical section.
Global carrier flags already exist. crates/felis-cli/src/main.rs:348-448: --host, --ssh-arg, --socket resolve through conn::resolve for every verb including daemon status (:434), so felis --host x daemon stop needs no new plumbing.
The serve loop has no exit path. serve.rs:222-246 serve_unix_with_factory runs an accept loop forever; :241 notes the socket file has "no shutdown hook to clear it from".

Verdict

accept. Principle 1 test: is there a dedicated tool that does this as well or better? pkill/kill does it worse (non-portable, races creation, cannot refuse, cannot drain) and does not exist on one supported build target; the OS's process tools cannot express "stop when empty" for a process that owns other processes. The verb is typed data over the existing IPC surface, which is the sanctioned extension surface. The default-refuse posture matches the read-side "no silent resurrection" stance already recorded for the CLI.

One addition to scope: daemon status should report draining so the state is observable (acceptance criterion 3); coordinate the field with #26.

Approach

Protocol

OpsMsg arms: OpsStop { StopMode mode } where StopMode is a three-arm oneof (IfEmpty {} default, Force {}, WhenEmpty {}) and OpsStopReply { oneof outcome { Stopping {}; Refused { uint32 sessions }; Draining { uint32 sessions } } }. A second Stop while draining answers Draining again (idempotent); Force while draining escalates.
AttachFailure::DAEMON_DRAINING for a Create refused while draining (and the Ops::Spawn refusal after #20); older-peer rule follows session_limit_refusal (serve.rs:1022-1028): send SPAWN_FAILED with the reason in detail.
OpsStatusReply.draining: bool (with #26).

Daemon

pool.rs: draining: bool on SessionPool; try_reserve returns None when set (:483-490), and the caller distinguishes "full" from "draining" by reading the flag under the same lock (the ok_or_else(|| guard.admitted()) at serve.rs:488 becomes a small enum).
serve.rs: DaemonCaps or the serve entry gains a shutdown: tokio::sync::watch::Sender<bool> created in serve_unix_with_factory; the accept loop (:246) selects on it and, on fire, drops the listener, unlinks the socket path (closing the :241 gap), and returns. route_ops handles Stop: IfEmpty → under the pool lock, if len() + reserved == 0 set draining and fire, else Refused { sessions }; Force → destroy every session via the existing OpsDestroy path (SessionCmd destroy), then fire; WhenEmpty → set draining, and the reaper (post-exit grace path in session_task/pool remove, pool.rs:514) fires shutdown when the pool becomes empty. The reply is written before the shutdown fires so the CLI reads it; the process exits after the accept loop returns and the session tasks are joined (force) or already gone (when-empty).
Windows: the accept loop is the cfg(windows) named-pipe variant in felis-transport; the same watch arm applies; no unlink.
main.rs doc comment (:31-34) updated: "no signal handling" stays true; the typed stop is the lifecycle path.

Client core / CLI

connector.rs: daemon_stop(mode) -> StopOutcome as a CorrelatedRequest; gate on the effective minor as daemon_status does (connector.rs:84 message pattern).
cli_daemon.rs: DaemonOp::Stop { force: bool, when_empty: bool (conflicts_with force), output: PointFormat }; exit 0 stopping/draining, 1 refused (typed refused kind with sessions in the object), 2 unreachable/too old. Human output states the mode taken and the count. Dial::Ops (conn.rs:44), never autospawn.
Completions and man pages are clap-derived (cli_completions.rs, cli_mangen.rs); regenerate whatever just recipe snapshots them.

Docs cascade

docs/reference/ipc.md Ops section + ledger/2.0 base + the AttachFailure list (:740+); docs/reference/cli.md new "Daemon stop" subsection under "Daemon status" (:253) and the verb table (:76); docs/reference/control-surfaces.md:33 row; docs/reference/spec.md new REQ beside REQ-1102 (:262); docs/explanation/architecture/control-surfaces.md:132-160 ("Diagnostic verbs" → daemon verbs; record why default-refuse, why --force is a flag not a verb, why no signal handler was added instead: not portable, cannot refuse); docs/explanation/architecture/session-lifecycle.md "Daemon updates" and :460-478 (draining is the first half of idle-daemon exit; keep that revisit); docs/how-to/update-felis.md:41-61, :88-95 (replace both pkill blocks; the drain becomes felis daemon stop --when-empty, then the next launch autospawns the new binary); skills/felis/SKILL.md:213-237 (new section or extend status); CHANGELOG.md (CLI + wire + update procedure).

Tests

serve/tests.rs beside :4432: stop with sessions → Refused { sessions: n } and the pool untouched; --force → pool empty and serve_unix_with_factory returns; --when-empty → a subsequent Create gets DAEMON_DRAINING, and after the last session's post-exit grace the serve future completes and the socket path is gone.
Race: spawn N concurrent creates and one IfEmpty stop against an empty pool; assert exactly one of {all creates succeed and stop is Refused, stop is Stopping and every create is DAEMON_DRAINING}; never a session registered after Stopping.
CLI: exit codes and JSON golden for the three outcomes; --force --when-empty is a clap conflict (exit 2).

Dependencies

#14 (aggregate admission object): if it lands first, draining belongs in that object, not the pool. Tracker step 8 (after #14, #23) holds.
#23 for the exit-code classes (typed refusal = 1, transport = 2, already consistent with the proposal).
#26 for the draining field in StatusReply.
#47 for arm placement. Does not need #20, but #20's Ops::Spawn must consult the same flag; land #25 after #20 or add the check in both.

Risk/effort

M. Main risk: exit ordering on the daemon (reply flushed before the listener drops; force-destroy waiting on PTY children that ignore SIGHUP; a Windows named-pipe listener that does not wake on the watch). Second: the --host path, where the relay (relay.rs) must survive long enough to carry the reply.

Labels

Keep priority/P1, release/v0.1.0: the update procedure is user-facing and the pkill instruction is wrong on a supported target.

Review amendments (round 3)

Reservations join the shutdown state machine. SessionSlot (pool.rs:399-408) is an atomic counter released on drop with no notification, so draining alone cannot settle in-flight creates. Change: reserved becomes pool state under the mutex plus a Notify; SessionSlot::drop and registration both re-evaluate draining and notify. WhenEmpty completes only when sessions.len() == 0 && reserved == 0. Force first sets draining (no new reservations), then destroys every registered session and waits for every outstanding reservation to resolve (register, then be destroyed like the others; or drop), then awaits every owner task's completion handle (#20) before the accept loop returns and the process exits. IfEmpty counts reservations as non-empty. Tests: a create paused between reserve and register while WhenEmpty is issued, resolved both ways (success → the session is drained and the daemon exits after it ends; failure → the drop wakes the drain and the daemon exits); Force during a paused create never leaves a child alive after exit.

## Triage plan (2026-09-03) Source-grounded triage against `main` at `69076d42`, reviewed through seven rounds of an independent reviewer (`pi` sol/luna) until it passed with no findings. The dependency order that supersedes the tracker's is posted on #12. Where a "Review amendments" section below conflicts with an earlier section, the amendment is the decision. ## Claim check Accurate. - **The update guide relies on `pkill`.** `docs/how-to/update-felis.md:58-61` (`pkill felis-daemon`) and `:92` (`ssh user@remote pkill felis-daemon`). The race is acknowledged in the same guide, `:48-52`: "Nothing stops you starting a new session between the check and the restart: felis has no quiesce mode". `CHANGELOG.md:493`, `:903` teach the same. - **Unix-only and abrupt.** `crates/felis-daemon/src/main.rs:31-34`: "Runs until the process is killed: there is no signal handling, and an abrupt death closes the PTY masters, which SIGHUPs the children". The daemon builds and is CI-gated on Windows (memory: 2026-09-03 Windows runtime gate green; `felis-pty` ConPTY backend per `CLAUDE.md`), where there is no `pkill` and no SIGTERM. - **No stop verb exists.** `crates/felis-cli/src/cli_daemon.rs:20-33`: `DaemonOp` has only `Status`. `OpsMsg` (`felis.proto:1143-1160`) has no stop arm. `docs/explanation/architecture/session-lifecycle.md:471-475` lists idle-daemon exit as a revisit item ("the daemon *process* has no idle self-exit"). - **The emptiness race is closable where the count lives.** `crates/felis-daemon/src/pool.rs:483-490` `try_reserve(max)` checks `sessions.len() + reserved` under the pool mutex, and `serve.rs:485-489` calls it under `pool.lock().await`. A `draining` flag checked inside `try_reserve` makes "refuse new creates" and "count is zero" one critical section. - **Global carrier flags already exist.** `crates/felis-cli/src/main.rs:348-448`: `--host`, `--ssh-arg`, `--socket` resolve through `conn::resolve` for every verb including `daemon status` (`:434`), so `felis --host x daemon stop` needs no new plumbing. - **The serve loop has no exit path.** `serve.rs:222-246` `serve_unix_with_factory` runs an `accept` loop forever; `:241` notes the socket file has "no shutdown hook to clear it from". ## Verdict **accept.** Principle 1 test: is there a dedicated tool that does this as well or better? `pkill`/`kill` does it worse (non-portable, races creation, cannot refuse, cannot drain) and does not exist on one supported build target; the OS's process tools cannot express "stop when empty" for a process that owns other processes. The verb is typed data over the existing IPC surface, which is the sanctioned extension surface. The default-refuse posture matches the read-side "no silent resurrection" stance already recorded for the CLI. One addition to scope: `daemon status` should report `draining` so the state is observable (acceptance criterion 3); coordinate the field with #26. ## Approach **Protocol** - `OpsMsg` arms: `OpsStop { StopMode mode }` where `StopMode` is a three-arm oneof (`IfEmpty {}` default, `Force {}`, `WhenEmpty {}`) and `OpsStopReply { oneof outcome { Stopping {}; Refused { uint32 sessions }; Draining { uint32 sessions } } }`. A second `Stop` while draining answers `Draining` again (idempotent); `Force` while draining escalates. - `AttachFailure::DAEMON_DRAINING` for a `Create` refused while draining (and the `Ops::Spawn` refusal after #20); older-peer rule follows `session_limit_refusal` (`serve.rs:1022-1028`): send `SPAWN_FAILED` with the reason in `detail`. - `OpsStatusReply.draining: bool` (with #26). **Daemon** - `pool.rs`: `draining: bool` on `SessionPool`; `try_reserve` returns `None` when set (`:483-490`), and the caller distinguishes "full" from "draining" by reading the flag under the same lock (the `ok_or_else(|| guard.admitted())` at `serve.rs:488` becomes a small enum). - `serve.rs`: `DaemonCaps` or the serve entry gains a `shutdown: tokio::sync::watch::Sender<bool>` created in `serve_unix_with_factory`; the accept loop (`:246`) selects on it and, on fire, drops the listener, unlinks the socket path (closing the `:241` gap), and returns. `route_ops` handles `Stop`: `IfEmpty` → under the pool lock, if `len() + reserved == 0` set `draining` and fire, else `Refused { sessions }`; `Force` → destroy every session via the existing `OpsDestroy` path (`SessionCmd` destroy), then fire; `WhenEmpty` → set `draining`, and the reaper (post-exit grace path in `session_task`/pool `remove`, `pool.rs:514`) fires shutdown when the pool becomes empty. The reply is written *before* the shutdown fires so the CLI reads it; the process exits after the accept loop returns and the session tasks are joined (force) or already gone (when-empty). - Windows: the accept loop is the `cfg(windows)` named-pipe variant in `felis-transport`; the same `watch` arm applies; no unlink. - `main.rs` doc comment (`:31-34`) updated: "no signal handling" stays true; the typed stop is the lifecycle path. **Client core / CLI** - `connector.rs`: `daemon_stop(mode) -> StopOutcome` as a `CorrelatedRequest`; gate on the effective minor as `daemon_status` does (`connector.rs:84` message pattern). - `cli_daemon.rs`: `DaemonOp::Stop { force: bool, when_empty: bool (conflicts_with force), output: PointFormat }`; exit `0` stopping/draining, `1` refused (typed `refused` kind with `sessions` in the object), `2` unreachable/too old. Human output states the mode taken and the count. `Dial::Ops` (`conn.rs:44`), never autospawn. - Completions and man pages are clap-derived (`cli_completions.rs`, `cli_mangen.rs`); regenerate whatever `just` recipe snapshots them. **Docs cascade** - `docs/reference/ipc.md` Ops section + ledger/2.0 base + the `AttachFailure` list (`:740+`); `docs/reference/cli.md` new "Daemon stop" subsection under "Daemon status" (`:253`) and the verb table (`:76`); `docs/reference/control-surfaces.md:33` row; `docs/reference/spec.md` new REQ beside REQ-1102 (`:262`); `docs/explanation/architecture/control-surfaces.md:132-160` ("Diagnostic verbs" → daemon verbs; record why default-refuse, why `--force` is a flag not a verb, why no signal handler was added instead: not portable, cannot refuse); `docs/explanation/architecture/session-lifecycle.md` "Daemon updates" and `:460-478` (draining is the first half of idle-daemon exit; keep that revisit); `docs/how-to/update-felis.md:41-61`, `:88-95` (replace both `pkill` blocks; the drain becomes `felis daemon stop --when-empty`, then the next launch autospawns the new binary); `skills/felis/SKILL.md:213-237` (new section or extend status); `CHANGELOG.md` (CLI + wire + update procedure). **Tests** - `serve/tests.rs` beside `:4432`: stop with sessions → `Refused { sessions: n }` and the pool untouched; `--force` → pool empty and `serve_unix_with_factory` returns; `--when-empty` → a subsequent `Create` gets `DAEMON_DRAINING`, and after the last session's post-exit grace the serve future completes and the socket path is gone. - Race: spawn N concurrent creates and one `IfEmpty` stop against an empty pool; assert exactly one of {all creates succeed and stop is `Refused`, stop is `Stopping` and every create is `DAEMON_DRAINING`}; never a session registered after `Stopping`. - CLI: exit codes and JSON golden for the three outcomes; `--force --when-empty` is a clap conflict (exit 2). ## Dependencies - **#14** (aggregate admission object): if it lands first, `draining` belongs in that object, not the pool. Tracker step 8 (after #14, #23) holds. - **#23** for the exit-code classes (typed refusal = 1, transport = 2, already consistent with the proposal). - **#26** for the `draining` field in `StatusReply`. - **#47** for arm placement. Does not need #20, but #20's `Ops::Spawn` must consult the same flag; land #25 after #20 or add the check in both. ## Risk/effort **M.** Main risk: exit ordering on the daemon (reply flushed before the listener drops; force-destroy waiting on PTY children that ignore SIGHUP; a Windows named-pipe listener that does not wake on the watch). Second: the `--host` path, where the relay (`relay.rs`) must survive long enough to carry the reply. ## Labels Keep `priority/P1`, `release/v0.1.0`: the update procedure is user-facing and the `pkill` instruction is wrong on a supported target. ## Review amendments (round 3) - **Reservations join the shutdown state machine.** `SessionSlot` (`pool.rs:399-408`) is an atomic counter released on drop with no notification, so `draining` alone cannot settle in-flight creates. Change: `reserved` becomes pool state under the mutex plus a `Notify`; `SessionSlot::drop` and registration both re-evaluate draining and notify. `WhenEmpty` completes only when `sessions.len() == 0 && reserved == 0`. `Force` first sets `draining` (no new reservations), then destroys every registered session *and* waits for every outstanding reservation to resolve (register, then be destroyed like the others; or drop), then awaits every owner task's completion handle (#20) before the accept loop returns and the process exits. `IfEmpty` counts reservations as non-empty. Tests: a create paused between reserve and register while `WhenEmpty` is issued, resolved both ways (success → the session is drained and the daemon exits after it ends; failure → the drop wakes the drain and the daemon exits); `Force` during a paused create never leaves a child alive after exit.

natsukium added the

status/planned

label

2026-09-03 20:35:14 +09:00

natsukium referenced this issue

2026-09-03 20:35:15 +09:00

[v0.1/P1] Make daemon status resource scopes unambiguous #26

natsukium referenced this issue

2026-09-03 20:35:16 +09:00

[v0.1/Release] Cut and publish v0.1.0 from the gated revision #31

natsukium referenced this issue

2026-09-03 20:35:16 +09:00