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

[v0.1/P1] Bound bridge lifecycle and stop on stdout failure #21

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

Parent: #12 (P1.2). Related to #3 and #5.

Why

The bridge has unbounded stdout/stream channels and one task per request with no in-flight cap. When stdout closes, its writer exits but the bridge can keep stdin and daemon links alive.

Scope

  • Cap stdin line bytes before JSON parsing.
  • Restrict ids to strings or safe integers; reject fractions and unknown fields.
  • Cap in-flight ids and daemon links; bound the stdout queue.
  • Reserve terminal capacity when admitting an operation.
  • Apply backpressure to finite lossless streams while retaining lag only for the intentionally lossy stream.
  • Resolve local relative cwd before sending it.
  • On stdout failure, notify the main loop, cancel work, close links, and exit nonzero in a specified order.

Acceptance criteria

  • Input, work, and output memory remain bounded under a blocked consumer.
  • Every accepted stream still emits exactly one terminal when output remains available.
  • Closed stdout shuts down all daemon links without waiting on open stdin.
  • Overload and shutdown errors are part of the JSON contract.
  • Concurrency, cancellation, daemon-loss, and stdout-loss tests cover the contract.
Parent: #12 (P1.2). Related to #3 and #5. ## Why The bridge has unbounded stdout/stream channels and one task per request with no in-flight cap. When stdout closes, its writer exits but the bridge can keep stdin and daemon links alive. ## Scope - Cap stdin line bytes before JSON parsing. - Restrict ids to strings or safe integers; reject fractions and unknown fields. - Cap in-flight ids and daemon links; bound the stdout queue. - Reserve terminal capacity when admitting an operation. - Apply backpressure to finite lossless streams while retaining `lag` only for the intentionally lossy stream. - Resolve local relative `cwd` before sending it. - On stdout failure, notify the main loop, cancel work, close links, and exit nonzero in a specified order. ## Acceptance criteria - [ ] Input, work, and output memory remain bounded under a blocked consumer. - [ ] Every accepted stream still emits exactly one terminal when output remains available. - [ ] Closed stdout shuts down all daemon links without waiting on open stdin. - [ ] Overload and shutdown errors are part of the JSON contract. - [ ] Concurrency, cancellation, daemon-loss, and stdout-loss tests cover the contract.
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

Accurate against HEAD; every cited gap is present in crates/felis-cli/src/cli_bridge.rs.

  • Unbounded stdout queue. :206-214 Out(mpsc::UnboundedSender<Option<String>>), with the doc comment defending it ("dropping a protocol object would break the 'exactly one terminal' contract").
  • Unbounded per-stream channels. :1651 mpsc::unbounded_channel() in open_stream for every capture/search/notification stream; the pump delivers into it synchronously (dispatch is sync by design, :1721-1723).
  • One task per request, no in-flight cap. :696-727: every accepted line becomes a tokio::spawn and an active HashMap entry; nothing counts them. Per-session attached links (Core::sessions, :632) and per-subscription observer dials (op_notifications, :1203-1210) are likewise uncapped.
  • No stdin line cap. :118 BufReader::new(tokio::io::stdin()).lines() then serde_json::from_str on the whole line (:404). A line without a newline grows without bound before parsing.
  • Loose ids and unknown fields. :420-421 accepts any is_number() (fractions, negatives, floats); parse_request uses object.get only, so unknown top-level fields pass silently.
  • stdout failure is not fatal. :218-226: the writer task breaks and logs "further protocol output is dropped"; serve's select loop (:119-129) only watches stdin and anchor.lost(), so stdin, the anchor, session links, and observers stay alive until the client closes stdin.
  • Relative cwd passes through. :847-849 sends it verbatim; the daemon refuses it (crates/felis-daemon/src/serve/tests.rs:4121 spawn_with_args_refuses_a_relative_cwd, "cwd must be absolute"), so a bridge client that passes "cwd":"build" gets SpawnFailed, not the editor-relative directory the comment promises.
  • lag only on the lossy stream. :1243 maps NotifyMsg::Lagged to lag; capture/search have no lag path and no backpressure.

Nothing here was fixed by the post-snapshot commits.

Verdict

accept-with-changes. All of it is bridge-local hardening with no principle question. Changes to the written scope:

  1. The numbers belong to #16. #16's scope already names "semantic limits for bridge lines"; #21 owns the enforcement sites and takes the values from wherever #16 puts them (one felis_client_core or felis_cli constants module), so the two issues do not each pick a line cap.
  2. Reuse at_capacity for the in-flight and link caps rather than minting an overloaded kind. at_capacity is already in the closed error.kind set (docs/reference/cli.md:109-112) with the meaning "the request was fine, retry after reaping"; adding a token touches #23's contract for no distinct consumer branch. The one new kind that is needed is output_failed for the exit-reason taxonomy (Shutdown at :143-176 has Eof/InputFailed/DaemonLost), and even that surfaces only in the exit code and stderr, since stdout is the thing that broke.
  3. "Every accepted stream emits exactly one terminal" cannot hold once stdout is dead; the acceptance criterion's qualifier ("when output remains available") is the contract. With a bounded stdout queue the writer must never drop a terminal: bound by count of items, and let terminals bypass the bound (a second sender or a priority flag on Out).
  4. Backpressure for capture/search should be end-to-end, not a second buffer. When the per-stream channel is full, the pump stops reading that link (try_send fails → park the reader on a Notify), so the daemon's outbox for that subscriber fills toward SUBSCRIBER_BUFFER_CAP (session_task.rs:46, :520) and the daemon evicts with a typed terminal. That makes a blocked consumer cost bounded memory on both ends and still yields one terminal; it is the existing eviction contract, not a new one.

Approach

crates/felis-cli/src/cli_bridge.rs

  • Input: replace .lines() with a length-limited reader (read_until(b'\n') into a Vec capped at the #16 line limit; on overflow, discard to the next newline and emit error_object(&Value::Null, malformed_request "line exceeds N bytes")). In parse_request (:404-460): ids must be a string (cap bytes, e.g. the same limit tags get) or an integer in 0..=2^53-1 (as_u64 and range check; reject is_f64); reject any key outside {v,id,op,params} with malformed_request. Reject unknown keys inside params per op too, since Params currently ignores them (:462+).
  • Admission: Core::accept (:660-727): a MAX_IN_FLIGHT check on active.len() before insert, answered with at_capacity; Core::sessions and the observer dial gated by MAX_LINKS the same way. Both checks happen before the tokio::spawn, so a refused request costs no task.
  • Output: Out becomes a bounded mpsc::channel for items plus an unbounded (or separately bounded) terminal lane; emit_open for items awaits capacity on the stream task (it is async there), so a slow consumer stalls the stream task, which stalls the per-stream channel, which parks the pump per verdict item 4. Out::start stores an AtomicBool dead and a Notify; the writer sets both on the first write error.
  • Shutdown on stdout loss: add Shutdown::OutputFailed(String) (code EXIT_FAILED); the serve select (:119-129) gains an out.dead() arm. Core::shutdown (:1260-1272) order for this reason: cancel every stream (existing), settle with SHUTDOWN_GRACE but skip writing synthesized terminals (stdout is gone), then anchor.shutdown(), every sessions link shutdown(), and observer links (they die with their tasks' abort). Stdin is not awaited.
  • cwd: in op_spawn (:834-852), std::path::absolute(cwd) (or env::current_dir().join) before sending, so the bridge's promise (":847 the bridge's working directory is the editor's") is what the daemon sees. Keep the daemon's absolute-only rule.

Docs cascade

  • docs/reference/cli.md bridge section: the id grammar, the unknown-field rule, the line cap (cross-reference #16's table), at_capacity on the bridge, and the exit-reason table (0 EOF, 1 stdin/stdout failed, 2 daemon lost).
  • docs/reference/ipc.md "CLI clients" if the exit-code sentence lives there (cli_bridge.rs:77-78 cites it).
  • docs/explanation/architecture/control-surfaces.md (bridge rationale): record why stdout death is fatal and why capture backpressure rides the daemon's eviction instead of a bridge buffer; "Revisit if" an editor needs a lossy capture.
  • skills/felis/SKILL.md bridge section: id rules, at_capacity, no unknown fields. CHANGELOG.md (bridge contract change).

Tests (in cli_bridge.rs tests, alongside :2114)

  • Overlong line → malformed_request with id: null, next line still served.
  • Fractional/negative id → refused; 2^53 refused; 2^53-1 accepted and echoed as an integer.
  • In-flight cap: N+1 pipelined sessions.list against a stub link → one at_capacity.
  • Blocked consumer: an Out whose receiver never drains; a stream of M items keeps memory bounded (channel capacity) and the pump stops reading (observable via the stub link's unread count).
  • stdout death: writer receiver dropped mid-stream; serve returns EXIT_FAILED without stdin EOF; every link's is_lost() is true.
  • Daemon-loss and cancellation tests exist (:2114-2170); extend with concurrency (two streams, one canceled).

Dependencies

  • #16 first (defines the line and payload limits the bridge enforces).
  • #20 first (deletes Registry.session and the positional spawn; otherwise this issue's link/in-flight bounds have to special-case it and then be rewritten).
  • #23/#29 coordinate: the error-kind set and the bridge JSON schema; land #21 before #29 generates schemas.
  • Tracker step 6 holds.

Risk/effort

M. Contained to one file plus docs. Main risk: the bounded-output change interacting with the shutdown settle path (a bounded send inside shutdown is the deadlock the current comment warns about, :207-209); the terminal lane must stay unbounded or the shutdown path must use try_send. Second risk: timing-based tests being flaky; use stub links, not real daemons.

Labels

Keep priority/P1, release/v0.1.0 (bridge epoch 1 freezes at the tag).

Review amendments (round 2)

  • The terminal lane is bounded too. Output memory is bounded only if terminals are counted. Admission of every operation reserves one terminal slot from a bounded pool (MAX_TERMINALS_PENDING, sized to the in-flight cap plus the stream cap, so admission never waits on a slot it already accounted for); the reservation is released when the terminal is written (not merely queued). When no reservation is available, the bridge stops reading stdin (does not admit) until one frees. The terminal lane may bypass item ordering (a terminal jumps the queue ahead of buffered items) but not the total bound. The shutdown settle path uses the same reservations it already holds, so the deadlock the :207-209 comment warns about cannot occur. A stdout consumer that stays open but stops reading therefore stalls admission rather than growing memory; the existing write-error path (Shutdown::OutputFailed) still covers a closed stdout.
  • Test: with MAX_IN_FLIGHT = 4 and a stdout sink that never drains, 1000 point requests on stdin leave at most MAX_IN_FLIGHT + MAX_TERMINALS_PENDING items in memory and stdin unread beyond that.

Review amendments (round 3)

  • Per-stream ordering is preserved; the terminal never overtakes its own items. Withdraw "a terminal may jump the queue". Out is one bounded FIFO of Line entries whose capacity is split into two accounted classes: item capacity (bounded, backpressures the stream task) and terminal capacity (one reserved slot per admitted operation, released when the line is written). An item send can never consume a terminal reservation, and a terminal send never waits on item capacity because its slot was reserved at admission, so the stream task's item, item, terminal enqueue order is the write order (ipc.md:400-405: an item after its terminal is corruption). Test: a stream with N items and a stalled stdout drains in exact order with the terminal last; a property test over interleaved streams asserts every stream's terminal is the last line carrying its id.
## 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; every cited gap is present in `crates/felis-cli/src/cli_bridge.rs`. - **Unbounded stdout queue.** `:206-214` `Out(mpsc::UnboundedSender<Option<String>>)`, with the doc comment defending it ("dropping a protocol object would break the 'exactly one terminal' contract"). - **Unbounded per-stream channels.** `:1651` `mpsc::unbounded_channel()` in `open_stream` for every capture/search/notification stream; the pump delivers into it synchronously (`dispatch` is sync by design, `:1721-1723`). - **One task per request, no in-flight cap.** `:696-727`: every accepted line becomes a `tokio::spawn` and an `active` HashMap entry; nothing counts them. Per-session attached links (`Core::sessions`, `:632`) and per-subscription observer dials (`op_notifications`, `:1203-1210`) are likewise uncapped. - **No stdin line cap.** `:118` `BufReader::new(tokio::io::stdin()).lines()` then `serde_json::from_str` on the whole line (`:404`). A line without a newline grows without bound before parsing. - **Loose ids and unknown fields.** `:420-421` accepts any `is_number()` (fractions, negatives, floats); `parse_request` uses `object.get` only, so unknown top-level fields pass silently. - **stdout failure is not fatal.** `:218-226`: the writer task breaks and logs "further protocol output is dropped"; `serve`'s select loop (`:119-129`) only watches stdin and `anchor.lost()`, so stdin, the anchor, session links, and observers stay alive until the client closes stdin. - **Relative `cwd` passes through.** `:847-849` sends it verbatim; the daemon refuses it (`crates/felis-daemon/src/serve/tests.rs:4121` `spawn_with_args_refuses_a_relative_cwd`, "cwd must be absolute"), so a bridge client that passes `"cwd":"build"` gets `SpawnFailed`, not the editor-relative directory the comment promises. - **`lag` only on the lossy stream.** `:1243` maps `NotifyMsg::Lagged` to `lag`; capture/search have no lag path and no backpressure. Nothing here was fixed by the post-snapshot commits. ## Verdict **accept-with-changes.** All of it is bridge-local hardening with no principle question. Changes to the written scope: 1. **The numbers belong to #16.** #16's scope already names "semantic limits for bridge lines"; #21 owns the enforcement sites and takes the values from wherever #16 puts them (one `felis_client_core` or `felis_cli` constants module), so the two issues do not each pick a line cap. 2. **Reuse `at_capacity` for the in-flight and link caps** rather than minting an `overloaded` kind. `at_capacity` is already in the closed `error.kind` set (`docs/reference/cli.md:109-112`) with the meaning "the request was fine, retry after reaping"; adding a token touches #23's contract for no distinct consumer branch. The one new kind that is needed is `output_failed` for the exit-reason taxonomy (`Shutdown` at `:143-176` has `Eof`/`InputFailed`/`DaemonLost`), and even that surfaces only in the exit code and stderr, since stdout is the thing that broke. 3. **"Every accepted stream emits exactly one terminal" cannot hold once stdout is dead**; the acceptance criterion's qualifier ("when output remains available") is the contract. With a bounded stdout queue the writer must never drop a *terminal*: bound by count of *items*, and let terminals bypass the bound (a second sender or a priority flag on `Out`). 4. **Backpressure for capture/search should be end-to-end, not a second buffer.** When the per-stream channel is full, the pump stops reading that link (`try_send` fails → park the reader on a `Notify`), so the daemon's outbox for that subscriber fills toward `SUBSCRIBER_BUFFER_CAP` (`session_task.rs:46`, `:520`) and the daemon evicts with a typed terminal. That makes a blocked consumer cost bounded memory on both ends and still yields one terminal; it is the existing eviction contract, not a new one. ## Approach **`crates/felis-cli/src/cli_bridge.rs`** - **Input:** replace `.lines()` with a length-limited reader (`read_until(b'\n')` into a `Vec` capped at the #16 line limit; on overflow, discard to the next newline and emit `error_object(&Value::Null, malformed_request "line exceeds N bytes")`). In `parse_request` (`:404-460`): ids must be a string (cap bytes, e.g. the same limit `tags` get) or an integer in `0..=2^53-1` (`as_u64` and range check; reject `is_f64`); reject any key outside `{v,id,op,params}` with `malformed_request`. Reject unknown keys inside `params` per op too, since `Params` currently ignores them (`:462+`). - **Admission:** `Core::accept` (`:660-727`): a `MAX_IN_FLIGHT` check on `active.len()` before insert, answered with `at_capacity`; `Core::sessions` and the observer dial gated by `MAX_LINKS` the same way. Both checks happen before the `tokio::spawn`, so a refused request costs no task. - **Output:** `Out` becomes a bounded `mpsc::channel` for items plus an unbounded (or separately bounded) terminal lane; `emit_open` for items awaits capacity on the *stream task* (it is async there), so a slow consumer stalls the stream task, which stalls the per-stream channel, which parks the pump per verdict item 4. `Out::start` stores an `AtomicBool dead` and a `Notify`; the writer sets both on the first write error. - **Shutdown on stdout loss:** add `Shutdown::OutputFailed(String)` (code `EXIT_FAILED`); the `serve` select (`:119-129`) gains an `out.dead()` arm. `Core::shutdown` (`:1260-1272`) order for this reason: cancel every stream (existing), `settle` with `SHUTDOWN_GRACE` but skip writing synthesized terminals (stdout is gone), then `anchor.shutdown()`, every `sessions` link `shutdown()`, and observer links (they die with their tasks' abort). Stdin is *not* awaited. - **cwd:** in `op_spawn` (`:834-852`), `std::path::absolute(cwd)` (or `env::current_dir().join`) before sending, so the bridge's promise (":847 the bridge's working directory is the editor's") is what the daemon sees. Keep the daemon's absolute-only rule. **Docs cascade** - `docs/reference/cli.md` bridge section: the id grammar, the unknown-field rule, the line cap (cross-reference #16's table), `at_capacity` on the bridge, and the exit-reason table (`0` EOF, `1` stdin/stdout failed, `2` daemon lost). - `docs/reference/ipc.md` "CLI clients" if the exit-code sentence lives there (`cli_bridge.rs:77-78` cites it). - `docs/explanation/architecture/control-surfaces.md` (bridge rationale): record why stdout death is fatal and why capture backpressure rides the daemon's eviction instead of a bridge buffer; "Revisit if" an editor needs a lossy capture. - `skills/felis/SKILL.md` bridge section: id rules, `at_capacity`, no unknown fields. `CHANGELOG.md` (bridge contract change). **Tests (in `cli_bridge.rs` tests, alongside `:2114`)** - Overlong line → `malformed_request` with `id: null`, next line still served. - Fractional/negative id → refused; `2^53` refused; `2^53-1` accepted and echoed as an integer. - In-flight cap: N+1 pipelined `sessions.list` against a stub link → one `at_capacity`. - Blocked consumer: an `Out` whose receiver never drains; a stream of M items keeps memory bounded (channel capacity) and the pump stops reading (observable via the stub link's unread count). - stdout death: writer receiver dropped mid-stream; `serve` returns `EXIT_FAILED` without stdin EOF; every link's `is_lost()` is true. - Daemon-loss and cancellation tests exist (`:2114-2170`); extend with concurrency (two streams, one canceled). ## Dependencies - **#16** first (defines the line and payload limits the bridge enforces). - **#20** first (deletes `Registry.session` and the positional spawn; otherwise this issue's link/in-flight bounds have to special-case it and then be rewritten). - **#23/#29** coordinate: the error-kind set and the bridge JSON schema; land #21 before #29 generates schemas. - Tracker step 6 holds. ## Risk/effort **M.** Contained to one file plus docs. Main risk: the bounded-output change interacting with the shutdown `settle` path (a bounded send inside shutdown is the deadlock the current comment warns about, `:207-209`); the terminal lane must stay unbounded or the shutdown path must use `try_send`. Second risk: timing-based tests being flaky; use stub links, not real daemons. ## Labels Keep `priority/P1`, `release/v0.1.0` (bridge epoch 1 freezes at the tag). ## Review amendments (round 2) - **The terminal lane is bounded too.** Output memory is bounded only if terminals are counted. Admission of every operation reserves one terminal slot from a bounded pool (`MAX_TERMINALS_PENDING`, sized to the in-flight cap plus the stream cap, so admission never waits on a slot it already accounted for); the reservation is released when the terminal is *written* (not merely queued). When no reservation is available, the bridge stops reading stdin (does not admit) until one frees. The terminal lane may bypass item ordering (a terminal jumps the queue ahead of buffered items) but not the total bound. The shutdown `settle` path uses the same reservations it already holds, so the deadlock the `:207-209` comment warns about cannot occur. A stdout consumer that stays open but stops reading therefore stalls admission rather than growing memory; the existing write-error path (`Shutdown::OutputFailed`) still covers a closed stdout. - Test: with `MAX_IN_FLIGHT = 4` and a stdout sink that never drains, 1000 point requests on stdin leave at most `MAX_IN_FLIGHT + MAX_TERMINALS_PENDING` items in memory and stdin unread beyond that. ## Review amendments (round 3) - **Per-stream ordering is preserved; the terminal never overtakes its own items.** Withdraw "a terminal may jump the queue". `Out` is one bounded FIFO of `Line` entries whose capacity is split into two accounted classes: item capacity (bounded, backpressures the stream task) and terminal capacity (one reserved slot per admitted operation, released when the line is written). An item send can never consume a terminal reservation, and a terminal send never waits on item capacity because its slot was reserved at admission, so the stream task's `item, item, terminal` enqueue order is the write order (`ipc.md:400-405`: an item after its terminal is corruption). Test: a stream with N items and a stalled stdout drains in exact order with the terminal last; a property test over interleaved streams asserts every stream's terminal is the last line carrying its id.
natsukium commented 2026-09-05 15:28:50 +09:00
Author
Owner
Copy link

Implemented in commit 07a77ae4. The bridge now bounds operations, auxiliary links, stream queues, and stdout backlog; reserves terminal output capacity; validates requests before admission; and shuts down promptly on stdout or daemon-link failure. The full just check gate passes, and an independent Sol review returned PASS.

Implemented in commit 07a77ae4. The bridge now bounds operations, auxiliary links, stream queues, and stdout backlog; reserves terminal output capacity; validates requests before admission; and shuts down promptly on stdout or daemon-link failure. The full `just check` gate passes, and an independent Sol review returned PASS.
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#21
No description provided.