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

[v0.1/P1] Close on shell exit and reconnect the same session after transport loss #22

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

Parent: #12 (P1.3). Supersedes the lifecycle decision in #6.

Why

Normal shell exit currently moves a window to an unrelated session, while transport loss closes the window. Silent selection can cross privilege, host, or working-directory boundaries, and the disconnect behavior contradicts the update guide.

Scope

  • On persistent-session exit, apply and paint the final grid state once, then close the window.
  • Keep explicit next/previous as the only unrelated-session navigation.
  • Preserve return-to-origin only when the initiating transient or cross-carrier operation established it.
  • On transport loss, keep the shadow visible with a disconnected indicator and retry the same carrier/session with bounded exponential delay.
  • Treat unknown session, auth failure, major refusal, and retry exhaustion as actionable terminal states without creating or selecting a replacement.

Acceptance criteria

  • Normal exit never lands on another persistent session.
  • The final queued grid state is painted before close.
  • Disconnect retries only the original carrier and session.
  • Retry bounds and terminal states are visible and tested.
  • Lifecycle explanation, update guide, reference behavior, and changelog agree.
Parent: #12 (P1.3). Supersedes the lifecycle decision in #6. ## Why Normal shell exit currently moves a window to an unrelated session, while transport loss closes the window. Silent selection can cross privilege, host, or working-directory boundaries, and the disconnect behavior contradicts the update guide. ## Scope - On persistent-session exit, apply and paint the final grid state once, then close the window. - Keep explicit next/previous as the only unrelated-session navigation. - Preserve return-to-origin only when the initiating transient or cross-carrier operation established it. - On transport loss, keep the shadow visible with a disconnected indicator and retry the same carrier/session with bounded exponential delay. - Treat unknown session, auth failure, major refusal, and retry exhaustion as actionable terminal states without creating or selecting a replacement. ## Acceptance criteria - [ ] Normal exit never lands on another persistent session. - [ ] The final queued grid state is painted before close. - [ ] Disconnect retries only the original carrier and session. - [ ] Retry bounds and terminal states are visible and tested. - [ ] Lifecycle explanation, update guide, reference behavior, and changelog agree.
natsukium commented 2026-09-03 20:35:13 +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

Mostly accurate; one overstatement.

  • Shell exit moves the window to another session. crates/felis-client/src/event_handler.rs:754-786: AppEvent::SessionExited for the current session ends in self.begin_exit_switch(false) (:785); crates/felis-client/src/app_methods.rs begin_exit_switch dials Landing::PickExit { anchor, previous } (:976-979); crates/felis-client-core/src/dial.rs:302-309 picks via pick_exit_switch_target(&sessions, anchor, previous) and attaches live-only. Only when nothing is left does continue_exit_ladder (app_methods.rs:1101-1113) return to origin or close. This is documented as intended: docs/explanation/architecture/session-lifecycle.md:402 (table row "switches to another session") and :420-431 (the rationale: "keeps its window open ... prefers the session the window was attached to before"). So the issue reverses a recorded design decision, and the report must say so.
  • Transport loss closes the window. event_handler.rs on_daemon_closed: ClosedOutcome::Exit => { info!("daemon disconnected; exiting"); event_loop.exit(); } (via daemon_closed_outcome, tested at crates/felis-client/src/tests.rs:1530). No retry, no indicator.
  • The update guide contradicts this. docs/how-to/update-felis.md:64-65: "A window left open across the restart reconnects on its own, and a newer daemon does not need it rebuilt." False today, and after a daemon restart the session is gone anyway (crates/felis-daemon/src/main.rs:31-34: no signal handling, PTY masters close; #32 deferred). This sentence is wrong regardless of what #22 decides and can be fixed on its own.
  • Overstated: "Silent selection can cross privilege ... boundaries." The pick is drawn from the roster of the daemon the window is already attached to, which is per-UID (peer-uid checked in felis-transport), so no privilege boundary is crossed. Host and working-directory boundaries are real: a ctrl-D lands on whatever unrelated project's shell sorts next in the ring.
  • Final grid state. crates/felis-daemon/src/serve/session_task.rs:1201-1211: notify_shell_exit pushes SessionExited after the queued grid events and just before evict_all, and the pump flushes in order, so the shadow has applied the final rows by the time the client handles SessionExited. "Painted" is a client-side redraw ordering question only.

Verdict

accept-with-changes.

Principle check for the reversal: the exit-driven pick is the client choosing a session for the user on a rule they did not state (principle 4's spirit; the roster order is not something the user sees, since there is no in-window picker), and it makes "a window holds exactly one shell" (principle 1) hold only until that shell exits, after which the window silently becomes another shell's. Closing on exit is what every non-multiplexing terminal does, and the session's own recovery story (post-exit grace, felis attach) is untouched. Accept the close.

Scope changes:

  1. Reconnect after transport loss is only meaningful over --host. On the local socket a close is daemon death, and until #32 that session no longer exists; a retry of Attach { id } can only end in UnknownSession (or a fresh autospawned daemon that has never heard of it). Over SSH the relay dies with the connection but the session survives (crates/felis-daemon/src/relay.rs:5-7), and retrying the same id is exactly right. Implement one retry ladder for both carriers (the local case terminates on the first UnknownSession), but size the schedule for the SSH case and say in the docs that local-carrier loss is daemon loss.
  2. Terminal states end in a closed window, not a parked one. "Actionable" means the log line and exit status name the reason and the remedy (felis attach <id> once the daemon is back), not an in-window message: felis has no dialog surface, and a window with no session is the thing principle 1 forbids. During retries the shadow stays visible with an indicator (see approach).
  3. "Paint the final grid state once" → "apply, then close on the next event-loop turn." Since the window closes, a paint is invisible; the requirement that matters is ordering (no close before the queued rows are applied), which the pump already guarantees. Test the ordering, not the paint.
  4. The update guide sentence is fixed now, independent of the code, because it currently promises behaviour that does not exist.

Approach

Client (crates/felis-client)

  • event_handler.rs:754-786 SessionExited: keep the transient branch (PipeState::Active → begin_pipe_return) and the in-flight-switch guard; replace the final else (pending_switch.cancel(); begin_exit_switch(false)) with if !self.continue_exit_ladder("shell exited") { … } else { event_loop.exit() }, i.e. return-to-origin when self.origin was set by a cross-carrier retarget (:591-597) or a transient, else exit.
  • Delete begin_exit_switch, Landing::PickExit (felis-client-core/src/dial.rs:228, :251, :286, :302-309), pick_exit_switch_target, SwitchIntent::Exit (switch_intent.rs:31), and the exit_driven/retry fields of SwitchState::InFlight; SwitchState::carry_exit collapses to "an in-flight landing that observes the exit closes the window if it lands on the dead session, else proceeds".
  • on_daemon_closed ClosedOutcome::Exit: becomes ClosedOutcome::Reconnect when the close was not a switch, not a transient, and not typed (Evicted, SessionExited already handled). New App::reconnect_state: Option<Reconnect { attempt, deadline }>; dial_in_background(dial_and_land(carrier, offer, Landing::Attach { id: current, intent: Automatic })) with felis_transport::retry::RetryPolicy (already used by connect_carrier_with_retry, connector.rs:218-223) bounded to, say, 6 attempts / 30 s. Landing failure classification: AttachFailed { UnknownSession | SessionExited | SessionEnding }, auth/major refusal (ConnectError variants that connect_carrier_with_retry already treats as non-retryable, connector.rs:1848-1887 tests), and budget exhaustion → event_loop.exit() with a tracing::error! naming the reason and remedy, and a nonzero process exit.
  • Indicator: while reconnect_state is Some, set the window title to "<title> — disconnected" and stop the cursor blink (both client-local presentation, principle 3); no daemon change.

Client-core: Reconnector already carries carrier+offer; add RetryPolicy::reconnect() beside the existing connect_carrier_with_retry policy so both are in one place.

Docs cascade

  • docs/explanation/architecture/session-lifecycle.md:390-458: rewrite the table row and the two paragraphs after it (close on exit; the only automatic navigation left is return-to-origin for transients and cross-carrier windows; record the rejected alternative, the roster pick, and why: unstated selection rule, host/cwd surprise; "Revisit if" a user asks for a configured exit action, which would be a typed Action, not a heuristic). Update the "Picking the session a chord lands on" section (:595-630) to drop the shell-exit exception. Add a "Transport loss" subsection with the retry bounds and the local-vs-SSH note.
  • docs/how-to/update-felis.md:64-65 (remove the reconnect claim; state that windows close on daemon restart and sessions do not survive it until #32), and :41-52.
  • docs/reference/cli.md window/exit-status section (the client's nonzero exit on a terminal reconnect state), docs/reference/ipc.md:636-642 (the live_only sentence lists "the exit-driven switch" as a user; drop it), docs/reference/spec.md if a REQ covers the exit behaviour (grep exit-driven), skills/felis/SKILL.md where it describes what a window does on exit, CHANGELOG.md (user-visible behaviour change).

Tests

  • crates/felis-client/src/tests.rs:1691-1720 (an_in_flight_landings_retry_becomes_the_exit_ladders, a_spent_retry_budget_is_not_refilled_by_the_exit) are rewritten for the new ladder; :1530-1560 gain the Reconnect outcome cases.
  • Pure tests for the retry schedule (bounded count and total delay) and for the terminal-state classifier (each ConnectError/AttachFailure → retry or terminal).
  • Ordering test: a stub pump delivering RowDelta, RowDelta, SessionExited; assert the shadow holds both rows when the exit handler runs.
  • Daemon side: no change; session_task.rs:3640-3665 already pins the push.

Dependencies

  • None hard. It is client-only and does not touch the wire; it does not need #20/#46 (the tracker's step 6 ordering is convenience, not necessity). The doc fix for update-felis.md can land today.
  • Coordinate with #24 only in prose (superseded verdicts must not be reported for a window that closed on exit).

Risk/effort

L. The switch/landing state machine in felis-client (switch_state, pending_switch, pipe_state, origin) is the densest code in the client and every transient path (run, pipe, cross-carrier retarget) rides it. Main risk: breaking transient return-to-origin while removing the pick. Mitigation: delete PickExit in one commit with its tests, add reconnect in a second.

Labels

Keep priority/P1, release/v0.1.0: "observable session/window behaviour" is inside the freeze boundary, and the update guide is wrong today. Suggest splitting the update-felis.md correction into its own docs(manual) commit immediately.

Review amendments (round 1)

  • Requirement change is explicit. The issue's "paint the final grid state once before closing" is replaced by "apply every queued row (through SessionExited) before closing, then close on the next event-loop turn". Rationale to record in the issue and the session-lifecycle explanation twin: the window closes, so a paint has no observer; the pump's in-order flush already guarantees the apply ordering. The client still calls request_redraw before deferring the close so any frame that does get presented is the final state; the test asserts the shadow ordering, not pixels.
  • Exit status must propagate. event_loop.exit() alone leaves main returning Ok(()) (felis-client/src/main.rs:951). App records terminal_exit: Option<ExitReason> before calling exit(); after run_app returns, main maps Some(reason) to a nonzero ExitCode (and logs the reason and the remedy). Reasons: reconnect budget exhausted, UnknownSession, auth/major refusal. Test: the fake-daemon harness drops the transport and never comes back; the client process exits nonzero within the retry budget.
## 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 Mostly accurate; one overstatement. - **Shell exit moves the window to another session.** `crates/felis-client/src/event_handler.rs:754-786`: `AppEvent::SessionExited` for the current session ends in `self.begin_exit_switch(false)` (`:785`); `crates/felis-client/src/app_methods.rs` `begin_exit_switch` dials `Landing::PickExit { anchor, previous }` (`:976-979`); `crates/felis-client-core/src/dial.rs:302-309` picks via `pick_exit_switch_target(&sessions, anchor, previous)` and attaches live-only. Only when nothing is left does `continue_exit_ladder` (`app_methods.rs:1101-1113`) return to origin or close. This is *documented as intended*: `docs/explanation/architecture/session-lifecycle.md:402` (table row "**switches to another session**") and `:420-431` (the rationale: "keeps its window open ... prefers the session the window was attached to before"). So the issue reverses a recorded design decision, and the report must say so. - **Transport loss closes the window.** `event_handler.rs` `on_daemon_closed`: `ClosedOutcome::Exit => { info!("daemon disconnected; exiting"); event_loop.exit(); }` (via `daemon_closed_outcome`, tested at `crates/felis-client/src/tests.rs:1530`). No retry, no indicator. - **The update guide contradicts this.** `docs/how-to/update-felis.md:64-65`: "A window left open across the restart reconnects on its own, and a newer daemon does not need it rebuilt." False today, and after a daemon restart the session is gone anyway (`crates/felis-daemon/src/main.rs:31-34`: no signal handling, PTY masters close; #32 deferred). This sentence is wrong regardless of what #22 decides and can be fixed on its own. - **Overstated:** "Silent selection can cross privilege ... boundaries." The pick is drawn from the roster of the daemon the window is already attached to, which is per-UID (peer-uid checked in `felis-transport`), so no privilege boundary is crossed. Host and working-directory boundaries are real: a ctrl-D lands on whatever unrelated project's shell sorts next in the ring. - **Final grid state.** `crates/felis-daemon/src/serve/session_task.rs:1201-1211`: `notify_shell_exit` pushes `SessionExited` *after* the queued grid events and just before `evict_all`, and the pump flushes in order, so the shadow has applied the final rows by the time the client handles `SessionExited`. "Painted" is a client-side redraw ordering question only. ## Verdict **accept-with-changes.** Principle check for the reversal: the exit-driven pick is the client choosing a session for the user on a rule they did not state (principle 4's spirit; the roster order is not something the user sees, since there is no in-window picker), and it makes "a window holds exactly one shell" (principle 1) hold only until that shell exits, after which the window silently becomes another shell's. Closing on exit is what every non-multiplexing terminal does, and the session's own recovery story (post-exit grace, `felis attach`) is untouched. Accept the close. Scope changes: 1. **Reconnect after transport loss is only meaningful over `--host`.** On the local socket a close is daemon death, and until #32 that session no longer exists; a retry of `Attach { id }` can only end in `UnknownSession` (or a fresh autospawned daemon that has never heard of it). Over SSH the relay dies with the connection but the session survives (`crates/felis-daemon/src/relay.rs:5-7`), and retrying the same id is exactly right. Implement one retry ladder for both carriers (the local case terminates on the first `UnknownSession`), but size the schedule for the SSH case and say in the docs that local-carrier loss is daemon loss. 2. **Terminal states end in a closed window, not a parked one.** "Actionable" means the log line and exit status name the reason and the remedy (`felis attach <id>` once the daemon is back), not an in-window message: felis has no dialog surface, and a window with no session is the thing principle 1 forbids. During retries the shadow stays visible with an indicator (see approach). 3. **"Paint the final grid state once" → "apply, then close on the next event-loop turn."** Since the window closes, a paint is invisible; the requirement that matters is ordering (no close before the queued rows are applied), which the pump already guarantees. Test the ordering, not the paint. 4. **The update guide sentence is fixed now**, independent of the code, because it currently promises behaviour that does not exist. ## Approach **Client (`crates/felis-client`)** - `event_handler.rs:754-786` `SessionExited`: keep the transient branch (`PipeState::Active` → `begin_pipe_return`) and the in-flight-switch guard; replace the final `else` (`pending_switch.cancel(); begin_exit_switch(false)`) with `if !self.continue_exit_ladder("shell exited") { … } else { event_loop.exit() }`, i.e. return-to-origin when `self.origin` was set by a cross-carrier retarget (`:591-597`) or a transient, else exit. - Delete `begin_exit_switch`, `Landing::PickExit` (`felis-client-core/src/dial.rs:228`, `:251`, `:286`, `:302-309`), `pick_exit_switch_target`, `SwitchIntent::Exit` (`switch_intent.rs:31`), and the `exit_driven`/`retry` fields of `SwitchState::InFlight`; `SwitchState::carry_exit` collapses to "an in-flight landing that observes the exit closes the window if it lands on the dead session, else proceeds". - `on_daemon_closed` `ClosedOutcome::Exit`: becomes `ClosedOutcome::Reconnect` when the close was not a switch, not a transient, and not typed (`Evicted`, `SessionExited` already handled). New `App::reconnect_state: Option<Reconnect { attempt, deadline }>`; `dial_in_background(dial_and_land(carrier, offer, Landing::Attach { id: current, intent: Automatic }))` with `felis_transport::retry::RetryPolicy` (already used by `connect_carrier_with_retry`, `connector.rs:218-223`) bounded to, say, 6 attempts / 30 s. Landing failure classification: `AttachFailed { UnknownSession | SessionExited | SessionEnding }`, auth/major refusal (`ConnectError` variants that `connect_carrier_with_retry` already treats as non-retryable, `connector.rs:1848-1887` tests), and budget exhaustion → `event_loop.exit()` with a `tracing::error!` naming the reason and remedy, and a nonzero process exit. - Indicator: while `reconnect_state` is `Some`, set the window title to `"<title> — disconnected"` and stop the cursor blink (both client-local presentation, principle 3); no daemon change. **Client-core:** `Reconnector` already carries carrier+offer; add `RetryPolicy::reconnect()` beside the existing `connect_carrier_with_retry` policy so both are in one place. **Docs cascade** - `docs/explanation/architecture/session-lifecycle.md:390-458`: rewrite the table row and the two paragraphs after it (close on exit; the only automatic navigation left is return-to-origin for transients and cross-carrier windows; record the rejected alternative, the roster pick, and why: unstated selection rule, host/cwd surprise; "Revisit if" a user asks for a configured exit action, which would be a typed `Action`, not a heuristic). Update the "Picking the session a chord lands on" section (`:595-630`) to drop the shell-exit exception. Add a "Transport loss" subsection with the retry bounds and the local-vs-SSH note. - `docs/how-to/update-felis.md:64-65` (remove the reconnect claim; state that windows close on daemon restart and sessions do not survive it until #32), and `:41-52`. - `docs/reference/cli.md` window/exit-status section (the client's nonzero exit on a terminal reconnect state), `docs/reference/ipc.md:636-642` (the `live_only` sentence lists "the exit-driven switch" as a user; drop it), `docs/reference/spec.md` if a REQ covers the exit behaviour (grep `exit-driven`), `skills/felis/SKILL.md` where it describes what a window does on exit, `CHANGELOG.md` (user-visible behaviour change). **Tests** - `crates/felis-client/src/tests.rs:1691-1720` (`an_in_flight_landings_retry_becomes_the_exit_ladders`, `a_spent_retry_budget_is_not_refilled_by_the_exit`) are rewritten for the new ladder; `:1530-1560` gain the `Reconnect` outcome cases. - Pure tests for the retry schedule (bounded count and total delay) and for the terminal-state classifier (each `ConnectError`/`AttachFailure` → retry or terminal). - Ordering test: a stub pump delivering `RowDelta`, `RowDelta`, `SessionExited`; assert the shadow holds both rows when the exit handler runs. - Daemon side: no change; `session_task.rs:3640-3665` already pins the push. ## Dependencies - None hard. It is client-only and does not touch the wire; it does not need #20/#46 (the tracker's step 6 ordering is convenience, not necessity). The doc fix for `update-felis.md` can land today. - Coordinate with #24 only in prose (`superseded` verdicts must not be reported for a window that closed on exit). ## Risk/effort **L.** The switch/landing state machine in `felis-client` (`switch_state`, `pending_switch`, `pipe_state`, `origin`) is the densest code in the client and every transient path (`run`, `pipe`, cross-carrier retarget) rides it. Main risk: breaking transient return-to-origin while removing the pick. Mitigation: delete `PickExit` in one commit with its tests, add reconnect in a second. ## Labels Keep `priority/P1`, `release/v0.1.0`: "observable session/window behaviour" is inside the freeze boundary, and the update guide is wrong today. Suggest splitting the `update-felis.md` correction into its own `docs(manual)` commit immediately. ## Review amendments (round 1) - **Requirement change is explicit.** The issue's "paint the final grid state once before closing" is replaced by "apply every queued row (through `SessionExited`) before closing, then close on the next event-loop turn". Rationale to record in the issue and the session-lifecycle explanation twin: the window closes, so a paint has no observer; the pump's in-order flush already guarantees the apply ordering. The client still calls `request_redraw` before deferring the close so any frame that does get presented is the final state; the test asserts the shadow ordering, not pixels. - **Exit status must propagate.** `event_loop.exit()` alone leaves `main` returning `Ok(())` (`felis-client/src/main.rs:951`). `App` records `terminal_exit: Option<ExitReason>` before calling `exit()`; after `run_app` returns, `main` maps `Some(reason)` to a nonzero `ExitCode` (and logs the reason and the remedy). Reasons: reconnect budget exhausted, `UnknownSession`, auth/major refusal. Test: the fake-daemon harness drops the transport and never comes back; the client process exits nonzero within the retry budget.
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#22
No description provided.