natsukium/felis
Watch
1
Code Issues 27 Pull requests 1 Projects Releases Packages Wiki Activity Actions

[v0.1/P1] Move detached spawn to correlated Ops and make create atomic #20

New issue
Closed
opened 2026-09-03 16:18:03 +09:00 by natsukium · 1 comment
natsukium commented 2026-09-03 16:18:03 +09:00
Owner
Copy link

Parent: #12 (P1.1). Related to #5 and #6.

Why

Detached creation is a pool operation but currently uses uncorrelated Session::Create, forcing the bridge to serialize spawns positionally. Window creation also exposes a pool-visible unattached gap between create and attach.

Scope

  • Add correlated Ops::Spawn, Spawned, and typed failure messages.
  • Make window Session::Create atomically create and attach.
  • Replace shared Ready { created }/AttachFailed ambiguity with distinct typed outcomes.
  • Reserve correlation field 100 on Session if no steady-state arm needs it.

Acceptance criteria

  • Two concurrent bridge spawns with reordered replies are attributed by request id.
  • Window creation emits one success acknowledgement and is attached when sent.
  • Failed create/attach cannot leave a new orphan session.
  • Old positional bridge state is removed.
  • Protocol vectors, minor ledger, CLI/bridge docs, and skills/felis are updated.
Parent: #12 (P1.1). Related to #5 and #6. ## Why Detached creation is a pool operation but currently uses uncorrelated `Session::Create`, forcing the bridge to serialize spawns positionally. Window creation also exposes a pool-visible unattached gap between create and attach. ## Scope - Add correlated `Ops::Spawn`, `Spawned`, and typed failure messages. - Make window `Session::Create` atomically create and attach. - Replace shared `Ready { created }`/`AttachFailed` ambiguity with distinct typed outcomes. - Reserve correlation field 100 on `Session` if no steady-state arm needs it. ## Acceptance criteria - [ ] Two concurrent bridge spawns with reordered replies are attributed by request id. - [ ] Window creation emits one success acknowledgement and is attached when sent. - [ ] Failed create/attach cannot leave a new orphan session. - [ ] Old positional bridge state is removed. - [ ] Protocol vectors, minor ledger, CLI/bridge docs, and `skills/felis` are updated.
natsukium commented 2026-09-03 20:35:12 +09:00
Author
Owner
Copy link

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 against HEAD (69076d42). Nothing in the three post-snapshot commits touches this path.

  • Create is detached and answered positionally. crates/felis-daemon/src/serve.rs:467-554: SessionMsg::Create reserves a slot (:485-489), spawns, registers via session_task::spawn_session (:530-538), writes Ready { info, created: true } uncorrelated (:545-549), then loops to wait for a follow-up Attach (:551-552 comment: "Detached creation: loop to honor a follow-up Attach (GUI window) or EOF (sessions spawn)"). docs/reference/ipc.md:643-650 documents exactly this ("always detached ... A caller that wants a live view sends Attach next").
  • The Session family carries an unused correlation slot. crates/felis-protocol/proto/felis.proto:1072-1085: Correlation correlation = 100; with the comment "The slot exists for the family, and no arm uses it". Ready/AttachFailed are shared by Attach and Create and distinguished only by the created bool (felis.proto:1123-1134).
  • The window path has a pool-visible unattached gap. crates/felis-client-core/src/connector.rs create_with: spawn_session(args) (one round trip) then attach(created.id, AttachIntent::Deliberate) (a second). Between the daemon's Ready write and the Attach arriving, the session is registered, idle, and listable; a client crash or a failed Ready write (serve.rs:545-549 returns Err after registration) leaves a session nobody was ever told about.
  • The bridge serializes spawns positionally. crates/felis-cli/src/cli_bridge.rs:1440-1443 (Registry.session: Option<oneshot::Sender<…>>, "attributed positionally: one slot, held for the whole round trip") and :1583-1600 (create_session refuses a second spawn with invalid_request "another spawn is already in flight on this bridge"). Acceptance criterion 1 (two concurrent bridge spawns attributed by request id) is currently impossible, not merely untested.
  • Concurrency at the daemon is already right for the cap. serve/tests.rs:4353 concurrent_creates_never_admit_past_the_session_cap covers the reservation; this issue is about attribution and atomicity, not admission.

One nuance the issue's wording hides: "Failed create/attach cannot leave a new orphan session" must be read narrowly. Once a session has been attached, losing its window is the product (principle 3, session-lifecycle.md:333-338), not an orphan. The orphan is a session whose creator never learned its id. With create+attach atomic, the id is only ever delivered in the attached state, and an Ops::Spawn whose reply is lost is a detached session that sessions list shows, which is what a headless spawn wants anyway.

Verdict

accept-with-changes. The direction is right and the change is breaking, so it belongs in the pre-2.0 window (#30). Changes to the written scope:

  1. The typed failure for Ops::Spawn should be an outcome oneof in the reply, not a new failure arm: OpsSpawned { oneof outcome { SessionInfo ok = 1; SpawnRefused refused = 2; } } with SpawnRefused { AttachFailure reason; string detail }. This mirrors ResolvedId (felis.proto:1243-1255) and keeps ConnMsg::Error's StreamErrorReason set (InvalidRequest/TooManyStreams/Unavailable/Internal) from having to grow spawn-specific reasons.
  2. Replace Ready { created } with two arms rather than one flag: SessionMsg::Attached { info } (answers Attach) and SessionMsg::Created { info } (answers Create, already attached when sent). AttachFailed stays for both; the reason enum already distinguishes them.
  3. Do not reserve field 100 on Session by hand before #48 decides the correlation shape; reserve it as part of #30's renumbering so the ledger records one reason.

Principle 1 is not in question: this adds no capability, it fixes attribution of an existing one.

Approach

Protocol (crates/felis-protocol)

  • proto/felis.proto: add OpsSpawn { SpawnArgs args = 1; } / OpsSpawned (above) as OpsMsg arms 13/14; change SessionMsg arms per verdict item 2; move SpawnArgs doc from the Session section to shared structs (it already lives at :500).
  • src/messages.rs / src/convert.rs: domain enums and conversions; codec::Correlated impl for OpsMsg::Spawn is free (the whole OpsMsg family is correlated).

Daemon (crates/felis-daemon/src/serve.rs)

  • Extract the body of the Create arm (:467-538: admit dims, try_reserve, mint id, spawn_with_args, spawn_session) into fn create_session(...) -> Result<(SessionId, SessionInfo), (AttachFailure, String)>.
  • route_ops (:916 area): add OpsMsg::Spawn { args } => create_session(...) mapped to OpsSpawned; gate it with the same mode rule as Destroy (mutating ops need ConnectionMode::Ops, :788).
  • wait_for_attach Create arm: call create_session, then fall straight into the existing Attach subscribe path (:555-600) with the fresh handle and live_only: false, and answer Created { info } only after SubscribeReq succeeds. Drop the "loop to honor a follow-up Attach" behaviour; a Session::Attach after Created is then a phase violation (#46).

Client core (crates/felis-client-core/src/connector.rs)

  • CreateSessionReq becomes a CorrelatedRequest on OpsMsg::Spawn (used by felis sessions spawn and the bridge). create_with sends one SessionMsg::Create and interprets Created. AttachSessionReq::interpret matches Attached.

Bridge (crates/felis-cli/src/cli_bridge.rs)

  • Delete Registry.session (:1440-1443), the positional branch in dispatch_payload, and create_session (:1583-1627); op_spawn (:834-852) becomes self.anchor.request(&OpsMsg::Spawn { args }) and maps SpawnRefused to at_capacity / invalid_request exactly as :1611-1622 does today.

Docs cascade

  • docs/reference/ipc.md Session section (:643-650, :730-740), Ops section (new Spawn/Spawned bullet beside Destroy), the minor ledger (:1738+; if this lands before #30 it is a minor 6 row whose "older peer" column is cannot be sent, like Status; if after, it is 2.0 base).
  • docs/explanation/architecture/ipc.md: record why create+attach is one message (the unattached gap) and why headless spawn moved to Ops (attribution); "Revisit if" a client ever needs create-without-attach on a window connection.
  • docs/explanation/architecture/session-lifecycle.md:141 (the sessions spawn sentence), docs/reference/cli.md spawn row, skills/felis/SKILL.md bridge sessions.spawn text (drop any "one spawn at a time" caveat), CHANGELOG.md (wire + bridge behaviour).

Tests

  • serve/tests.rs: (a) two Ops::Spawn on one connection with a factory that delays the first; replies arrive reversed and each request_id maps to its own SessionInfo; (b) a window Create is followed immediately by RehydrateBegin on the same connection with no Attach sent; (c) a_create_past_the_session_cap_is_refused_and_executes_nothing (:4300) and the geometry test (:4199) re-pointed at both entry points; (d) a Create whose subscribe fails leaves pool.len() unchanged.
  • Bridge: two sessions.spawn requests pipelined get two result objects with their own ids.
  • Protocol compat fixtures consumed by #19's check are regenerated in the same commit.

Dependencies

  • Decide first: #47 (arm-vs-kind: Spawn as an OpsMsg arm is the "new operation inside an existing conversation" case) and #48 (correlation shape; the reply must be a request reply, nothing else). Decisions only; their implementation can follow.
  • Coordinate: #46 (the post-Created phase is Attached; land #20 first so #46 models the final flow, as #52 recommends).
  • Land before: #21 (removes the positional slot the bridge bounds would otherwise have to cap), #30.
  • Tracker order (step 5) still holds.

Risk/effort

L. Touches the daemon attach phase, the client-core request layer, and the bridge at once, with #46 reshaping the same driver. Main risk: regressing the window-launch path (felis, felis -- cmd, transient run/pipe which also create) — every landing in dial.rs:295-330 passes through create_with.

Labels

Keep priority/P1, release/v0.1.0. It is a wire break and must precede #30.

Review amendments (round 1)

  • Rollback on attach failure. spawn_session registers the session in the pool and starts its owner task before subscription. If the subscribe/attach step fails, the daemon must pool.remove(id) and send SessionCmd::Shutdown on the handle (the same path sessions kill uses at serve.rs:808) and await the owner task's exit, so the PTY child is reaped. Implement this as an RAII guard (Registered { pool, id, handle }) disarmed only after the subscription succeeds, so every early-return path rolls back. Test (d) becomes: a Create whose subscribe fails leaves pool.len() unchanged and the session task has exited (join handle resolved) with the child reaped.

Review amendments (round 2)

  • Rollback is an explicit async step, not Drop. spawn_owned (session_task.rs:330-436) hands the owner JoinHandle to a detached supervisor and SessionHandle (pool.rs:331-337) has no completion handle, and Drop cannot await. Refactor: spawn_owned returns a SessionLifecycle { id, info, done: watch::Receiver<bool> } (the supervisor sets done when the owner task exits and the child is reaped). Create/attach has a single failure exit that calls async fn rollback(&mut self) on a Registered guard: pool.remove(id), cmd.send(SessionCmd::Shutdown), then done.changed().await under a bounded timeout (log and continue if the child ignores SIGHUP within it). Drop remains only as a nonblocking fallback (try_send(Shutdown)) for panics. Test: the attach-failure test awaits create returning the error, then asserts done is already true, pool.len() unchanged, and the fixture child's PID is gone.
## 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 against HEAD (69076d42). Nothing in the three post-snapshot commits touches this path. - **Create is detached and answered positionally.** `crates/felis-daemon/src/serve.rs:467-554`: `SessionMsg::Create` reserves a slot (`:485-489`), spawns, registers via `session_task::spawn_session` (`:530-538`), writes `Ready { info, created: true }` uncorrelated (`:545-549`), then *loops* to wait for a follow-up `Attach` (`:551-552` comment: "Detached creation: loop to honor a follow-up `Attach` (GUI window) or EOF (`sessions spawn`)"). `docs/reference/ipc.md:643-650` documents exactly this ("always **detached** ... A caller that wants a live view sends `Attach` next"). - **The Session family carries an unused correlation slot.** `crates/felis-protocol/proto/felis.proto:1072-1085`: `Correlation correlation = 100;` with the comment "The slot exists for the family, and no arm uses it". `Ready`/`AttachFailed` are shared by Attach and Create and distinguished only by the `created` bool (`felis.proto:1123-1134`). - **The window path has a pool-visible unattached gap.** `crates/felis-client-core/src/connector.rs` `create_with`: `spawn_session(args)` (one round trip) then `attach(created.id, AttachIntent::Deliberate)` (a second). Between the daemon's `Ready` write and the `Attach` arriving, the session is registered, idle, and listable; a client crash or a failed `Ready` write (`serve.rs:545-549` returns `Err` *after* registration) leaves a session nobody was ever told about. - **The bridge serializes spawns positionally.** `crates/felis-cli/src/cli_bridge.rs:1440-1443` (`Registry.session: Option<oneshot::Sender<…>>`, "attributed positionally: one slot, held for the whole round trip") and `:1583-1600` (`create_session` refuses a second spawn with `invalid_request` "another spawn is already in flight on this bridge"). Acceptance criterion 1 (two concurrent bridge spawns attributed by request id) is currently impossible, not merely untested. - **Concurrency at the daemon is already right for the cap.** `serve/tests.rs:4353` `concurrent_creates_never_admit_past_the_session_cap` covers the reservation; this issue is about attribution and atomicity, not admission. One nuance the issue's wording hides: "Failed create/attach cannot leave a new orphan session" must be read narrowly. Once a session *has* been attached, losing its window is the product (principle 3, `session-lifecycle.md:333-338`), not an orphan. The orphan is a session whose creator never learned its id. With create+attach atomic, the id is only ever delivered in the attached state, and an `Ops::Spawn` whose reply is lost is a detached session that `sessions list` shows, which is what a headless spawn wants anyway. ## Verdict **accept-with-changes.** The direction is right and the change is breaking, so it belongs in the pre-2.0 window (#30). Changes to the written scope: 1. The typed failure for `Ops::Spawn` should be an *outcome oneof* in the reply, not a new failure arm: `OpsSpawned { oneof outcome { SessionInfo ok = 1; SpawnRefused refused = 2; } }` with `SpawnRefused { AttachFailure reason; string detail }`. This mirrors `ResolvedId` (`felis.proto:1243-1255`) and keeps `ConnMsg::Error`'s `StreamErrorReason` set (`InvalidRequest/TooManyStreams/Unavailable/Internal`) from having to grow spawn-specific reasons. 2. Replace `Ready { created }` with two arms rather than one flag: `SessionMsg::Attached { info }` (answers `Attach`) and `SessionMsg::Created { info }` (answers `Create`, already attached when sent). `AttachFailed` stays for both; the reason enum already distinguishes them. 3. Do not reserve field 100 on `Session` by hand before #48 decides the correlation shape; reserve it as part of #30's renumbering so the ledger records one reason. Principle 1 is not in question: this adds no capability, it fixes attribution of an existing one. ## Approach **Protocol (`crates/felis-protocol`)** - `proto/felis.proto`: add `OpsSpawn { SpawnArgs args = 1; }` / `OpsSpawned` (above) as `OpsMsg` arms 13/14; change `SessionMsg` arms per verdict item 2; move `SpawnArgs` doc from the Session section to shared structs (it already lives at `:500`). - `src/messages.rs` / `src/convert.rs`: domain enums and conversions; `codec::Correlated` impl for `OpsMsg::Spawn` is free (the whole `OpsMsg` family is correlated). **Daemon (`crates/felis-daemon/src/serve.rs`)** - Extract the body of the `Create` arm (`:467-538`: admit dims, `try_reserve`, mint id, `spawn_with_args`, `spawn_session`) into `fn create_session(...) -> Result<(SessionId, SessionInfo), (AttachFailure, String)>`. - `route_ops` (`:916` area): add `OpsMsg::Spawn { args } => create_session(...)` mapped to `OpsSpawned`; gate it with the same mode rule as `Destroy` (mutating ops need `ConnectionMode::Ops`, `:788`). - `wait_for_attach` `Create` arm: call `create_session`, then fall straight into the existing `Attach` subscribe path (`:555-600`) with the fresh handle and `live_only: false`, and answer `Created { info }` only after `SubscribeReq` succeeds. Drop the "loop to honor a follow-up Attach" behaviour; a `Session::Attach` after `Created` is then a phase violation (#46). **Client core (`crates/felis-client-core/src/connector.rs`)** - `CreateSessionReq` becomes a `CorrelatedRequest` on `OpsMsg::Spawn` (used by `felis sessions spawn` and the bridge). `create_with` sends one `SessionMsg::Create` and interprets `Created`. `AttachSessionReq::interpret` matches `Attached`. **Bridge (`crates/felis-cli/src/cli_bridge.rs`)** - Delete `Registry.session` (`:1440-1443`), the positional branch in `dispatch_payload`, and `create_session` (`:1583-1627`); `op_spawn` (`:834-852`) becomes `self.anchor.request(&OpsMsg::Spawn { args })` and maps `SpawnRefused` to `at_capacity` / `invalid_request` exactly as `:1611-1622` does today. **Docs cascade** - `docs/reference/ipc.md` Session section (`:643-650`, `:730-740`), Ops section (new `Spawn`/`Spawned` bullet beside `Destroy`), the minor ledger (`:1738+`; if this lands before #30 it is a minor 6 row whose "older peer" column is *cannot be sent*, like `Status`; if after, it is 2.0 base). - `docs/explanation/architecture/ipc.md`: record why create+attach is one message (the unattached gap) and why headless spawn moved to `Ops` (attribution); "Revisit if" a client ever needs create-without-attach on a window connection. - `docs/explanation/architecture/session-lifecycle.md:141` (the `sessions spawn` sentence), `docs/reference/cli.md` spawn row, `skills/felis/SKILL.md` bridge `sessions.spawn` text (drop any "one spawn at a time" caveat), `CHANGELOG.md` (wire + bridge behaviour). **Tests** - `serve/tests.rs`: (a) two `Ops::Spawn` on one connection with a factory that delays the first; replies arrive reversed and each `request_id` maps to its own `SessionInfo`; (b) a window `Create` is followed immediately by `RehydrateBegin` on the same connection with no `Attach` sent; (c) `a_create_past_the_session_cap_is_refused_and_executes_nothing` (`:4300`) and the geometry test (`:4199`) re-pointed at both entry points; (d) a `Create` whose subscribe fails leaves `pool.len()` unchanged. - Bridge: two `sessions.spawn` requests pipelined get two `result` objects with their own ids. - Protocol compat fixtures consumed by #19's check are regenerated in the same commit. ## Dependencies - **Decide first:** #47 (arm-vs-kind: `Spawn` as an `OpsMsg` arm is the "new operation inside an existing conversation" case) and #48 (correlation shape; the reply must be a request reply, nothing else). Decisions only; their implementation can follow. - **Coordinate:** #46 (the post-`Created` phase is `Attached`; land #20 first so #46 models the final flow, as #52 recommends). - **Land before:** #21 (removes the positional slot the bridge bounds would otherwise have to cap), #30. - Tracker order (step 5) still holds. ## Risk/effort **L.** Touches the daemon attach phase, the client-core request layer, and the bridge at once, with #46 reshaping the same driver. Main risk: regressing the window-launch path (`felis`, `felis -- cmd`, transient `run`/`pipe` which also create) — every landing in `dial.rs:295-330` passes through `create_with`. ## Labels Keep `priority/P1`, `release/v0.1.0`. It is a wire break and must precede #30. ## Review amendments (round 1) - **Rollback on attach failure.** `spawn_session` registers the session in the pool and starts its owner task before subscription. If the subscribe/attach step fails, the daemon must `pool.remove(id)` *and* send `SessionCmd::Shutdown` on the handle (the same path `sessions kill` uses at `serve.rs:808`) and await the owner task's exit, so the PTY child is reaped. Implement this as an RAII guard (`Registered { pool, id, handle }`) disarmed only after the subscription succeeds, so every early-return path rolls back. Test (d) becomes: a `Create` whose subscribe fails leaves `pool.len()` unchanged *and* the session task has exited (join handle resolved) with the child reaped. ## Review amendments (round 2) - **Rollback is an explicit async step, not `Drop`.** `spawn_owned` (`session_task.rs:330-436`) hands the owner `JoinHandle` to a detached supervisor and `SessionHandle` (`pool.rs:331-337`) has no completion handle, and `Drop` cannot await. Refactor: `spawn_owned` returns a `SessionLifecycle { id, info, done: watch::Receiver<bool> }` (the supervisor sets `done` when the owner task exits and the child is reaped). Create/attach has a single failure exit that calls `async fn rollback(&mut self)` on a `Registered` guard: `pool.remove(id)`, `cmd.send(SessionCmd::Shutdown)`, then `done.changed().await` under a bounded timeout (log and continue if the child ignores `SIGHUP` within it). `Drop` remains only as a nonblocking fallback (`try_send(Shutdown)`) for panics. Test: the attach-failure test awaits `create` returning the error, then asserts `done` is already `true`, `pool.len()` unchanged, and the fixture child's PID is gone.
Sign in to join this conversation.
No Branch/Tag specified
Branches Tags
main
ci/darwin-smoke
No results found.
Labels
Clear labels   
priority/P0
Must be resolved before the v0.1.0 tag

  
priority/P1
Must be settled before the first public contract freezes

  
priority/P2
Explicitly deferred until after v0.1.0

  
release/v0.1.0
Work tracked for the first tagged release

  
status/blocked
Cannot start now: waits on an upstream release, another issue, or a recorded revisit trigger

  
status/planned
Triaged with a source-grounded plan and a passed pi review; ready to implement

  
type/bug
Shipped behavior that contradicts the docs or the code's own intent; needs a product code change

  
type/design
An open scope or trade-off question; needs a recorded decision in the owning explanation doc before code

  
type/test-gap
Documented or reviewed behavior that no test pins; usually split from a review finding, needs no product code change

  
type/tracker
Parent issue that tracks child work

No labels
priority/P0
priority/P1
priority/P2
release/v0.1.0
status/blocked
status/planned
type/bug
type/design
type/test-gap
type/tracker
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
natsukium/felis#20
No description provided.