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

[v0.1/P1] Make correlation an exact per-arm identity #48

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

Problem

The correlation schema can express states the documented request/stream model does not allow, and validation is split across handlers.

At review snapshot 5077d74b:

  • Correlation contains two independent scalar fields. The conversion rejects an envelope with neither id but accepts one carrying both request_id and stream_id.
  • No current arm legitimately belongs to a point request and a stream simultaneously.
  • ConnectionDriver::account_for handles only stream_id. Required request ids are recovered later by ops_request_of, stream_of, or family-specific branches.
  • match_reply checks only request_id, so an envelope can also name an unrelated live stream.
  • The client allocates request ids monotonically, but the daemon does not enforce the documented next-request sequence or duplicate/reuse rule.

This weakens field 100 from an exact identity into an optional-field bag. A public non-Rust implementation has to reproduce call-site behavior rather than one schema rule.

Required change

Make correlation an exclusive identity and centralize exact per-arm validation in the shared protocol/driver layer.

The protocol-2.0 baseline should use a protobuf oneof and a domain enum such as Correlation::Request(RequestId) | Correlation::Stream(StreamId). If a future operation truly needs both concepts, it should define a deliberate conversation shape rather than making every current envelope ambiguous.

Acceptance criteria

  • The schema cannot encode both ids in one Correlation; zero/unset and unknown oneof arms are malformed.
  • Every correlated message arm declares exactly one of: uncorrelated, request opener, request reply, stream opener, or stream item.
  • ConnectionDriver::decode validates that declaration before returning a deliverable message. Handlers no longer recover mandatory correlation ad hoc.
  • The daemon enforces the request-id sequence promised by the reference contract; the client still matches replies against its outstanding set and rejects duplicate/late replies.
  • Request and stream sequences remain independent and non-zero unless the owning explanation is deliberately changed with equivalent ABA/cancel analysis.
  • Tests cover missing ids, the old both-ids encoding, wrong identity class on every conversation family, skipped/reused request ids, out-of-order valid replies, and stream cancellation races.
  • felis.proto, docs/reference/ipc.md, and the explanation rationale agree on the exclusive shape.
  • Coordinate with #20, which moves detached spawn into correlated Ops, then land the breaking schema in #30.
## Problem The correlation schema can express states the documented request/stream model does not allow, and validation is split across handlers. At review snapshot `5077d74b`: - `Correlation` contains two independent scalar fields. The conversion rejects an envelope with neither id but accepts one carrying both `request_id` and `stream_id`. - No current arm legitimately belongs to a point request and a stream simultaneously. - `ConnectionDriver::account_for` handles only `stream_id`. Required request ids are recovered later by `ops_request_of`, `stream_of`, or family-specific branches. - `match_reply` checks only `request_id`, so an envelope can also name an unrelated live stream. - The client allocates request ids monotonically, but the daemon does not enforce the documented next-request sequence or duplicate/reuse rule. This weakens field 100 from an exact identity into an optional-field bag. A public non-Rust implementation has to reproduce call-site behavior rather than one schema rule. ## Required change Make correlation an exclusive identity and centralize exact per-arm validation in the shared protocol/driver layer. The protocol-2.0 baseline should use a protobuf `oneof` and a domain enum such as `Correlation::Request(RequestId) | Correlation::Stream(StreamId)`. If a future operation truly needs both concepts, it should define a deliberate conversation shape rather than making every current envelope ambiguous. ## Acceptance criteria - The schema cannot encode both ids in one `Correlation`; zero/unset and unknown oneof arms are malformed. - Every correlated message arm declares exactly one of: uncorrelated, request opener, request reply, stream opener, or stream item. - `ConnectionDriver::decode` validates that declaration before returning a deliverable message. Handlers no longer recover mandatory correlation ad hoc. - The daemon enforces the request-id sequence promised by the reference contract; the client still matches replies against its outstanding set and rejects duplicate/late replies. - Request and stream sequences remain independent and non-zero unless the owning explanation is deliberately changed with equivalent ABA/cancel analysis. - Tests cover missing ids, the old both-ids encoding, wrong identity class on every conversation family, skipped/reused request ids, out-of-order valid replies, and stream cancellation races. - `felis.proto`, `docs/reference/ipc.md`, and the explanation rationale agree on the exclusive shape. - Coordinate with #20, which moves detached spawn into correlated `Ops`, then land the breaking schema in #30.
natsukium commented 2026-09-03 20:35:20 +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.

Claim check

Accurate against HEAD.

  • Schema: message Correlation { uint64 request_id = 1; uint64 stream_id = 2; } (crates/felis-protocol/proto/felis.proto:461-467), two independent scalars. Domain: Correlation { request_id: Option<RequestId>, stream_id: Option<StreamId> } (crates/felis-protocol/src/messages.rs:421-447). The conversion (crates/felis-protocol/src/convert.rs:263-275) rejects neither-set and accepts both-set; codec::peek_correlation (codec.rs:110-115) lifts it via CorrelationSlot (:96-100).
  • No arm needs both: the family table at docs/reference/ipc.md:356-364 assigns exactly one id kind per arm.
  • ConnectionDriver::account_for (crates/felis-transport/src/driver.rs:414-434) looks only at stream_id; a request_id on an arm that should carry none passes silently. Required request ids are recovered afterwards by ops_request_of (crates/felis-daemon/src/serve.rs:720-731, called at :628 and :1221), stream_of (:1247-1258, called at :643, :1154, :1188), and the inline Region::Request branch (:1162-1171).
  • match_reply (driver.rs:255-275) reads only request_id; a reply naming an outstanding request and a live stream is accepted as a reply.
  • The client allocates request_id sequentially (issue_request, driver.rs:244-253); the daemon's StreamTable.next_request (:147) is never advanced or compared on the daemon side. The reference promises "two independent sequences that start at 1, count strictly upward" (docs/reference/ipc.md:373-379), and the explanation says both ends keep the counters in step (docs/explanation/architecture/ipc.md:613-615) but only the stream counter is (accept_open, driver.rs:438-471).
  • Tests: an_empty_correlation_envelope_closes_the_connection (driver/tests.rs:470), a_stream_id_on_an_arm_that_opens_none_closes_the_connection (:658), replies_match_their_requests_in_any_order (:593); nothing on both-set, wrong class, or request-sequence gaps.

Verdict

accept-with-changes: the decision is requested; recommend the issue's oneof, with one implementation detail the issue does not mention (below) that is needed for its "the old both-ids encoding is rejected" criterion to be true.

Decision: Correlation becomes a protobuf oneof id { uint64 request_id = 1; uint64 stream_id = 2; }; the domain type becomes enum Correlation { Request(RequestId), Stream(StreamId) }; every arm declares one CorrelationClass in the #47 ArmMeta; the driver validates envelope against class in decode.

Why the oneof rather than a wider redesign: keeping tags 1 and 2 makes every honest frame on the wire today byte-identical after the change (a oneof of scalars encodes exactly as the single scalar did), so #30's golden conversations need no regeneration for this item; a sender physically cannot set both; and the two independent sequences, the ABA argument (docs/explanation/architecture/ipc.md:599-607), and Subject (messages.rs:451-456, already an exclusive enum) all stay as they are. The "deliberate conversation shape" for a hypothetical arm needing both (a request that also opens a stream) is a future arm carrying a second, named field, not a widened envelope.

The detail: prost decodes a oneof with last-field-wins, so a both-set body would silently parse as Stream. Keep the private CorrelationSlot mirror in codec.rs:96-100 as two plain scalars (it is already separate from the generated type) and have peek_correlation reject request_id != 0 && stream_id != 0 as WireError::AmbiguousCorrelation. The public schema forbids both; the peek proves a peer did not send both anyway. Document that in the CorrelationSlot comment.

Approach

Protocol (felis-protocol):

  1. felis.proto:461-467: oneof; comment states exclusivity and that 0/unset is malformed on every correlated arm that declares a class other than Uncorrelated. Regenerate (just proto).
  2. messages.rs:417-447: enum Correlation { Request(RequestId), Stream(StreamId) }; keep the request()/stream() constructors as const fn for call-site compatibility; describe (driver.rs:648-656) prints the variant.
  3. convert.rs:252-275: From/TryFrom over the oneof; unset arm → MissingOneof("Correlation.id"); zero value in the set arm → MissingField.
  4. CorrelationClass { Uncorrelated, RequestOpener, RequestReply, StreamOpener, StreamItem } as the correlation column of ArmMeta (#47). Fill it from the table at docs/reference/ipc.md:356-364: Ops verbs RequestOpener, Ops replies RequestReply; Region::Request/Reply likewise, Region::Rows StreamOpener, Row/RowsDone StreamItem; Notify::Subscribe opener, Subscribed/Event/Lagged items; Search::Query opener, Match item; Session all Uncorrelated (until #20 decides; if #20 makes Session::Create correlated the class changes there); Input/Grid/Image/Push/Conn Uncorrelated (their wrappers reserve 100, so the class is also asserted by MessageKind::is_correlated, lib.rs:114-119; add a test that no arm of a non-correlated kind declares a class other than Uncorrelated).

Driver (felis-transport/src/driver.rs):

  1. decode (:383-409): after direction, match (meta.correlation, payload.correlation): (Uncorrelated, None) deliver; (RequestOpener, Some(Request(id))) daemon side → accept_request(id): must equal table.next_request, advance it, insert into outstanding_requests (the daemon's set of requests it owes a reply to; Ops reply writers then retire_request(id) like retire_stream); (RequestReply, Some(Request(id))) client side → match_reply; (StreamOpener, Some(Stream(id))) → accept_open; (StreamItem, Some(Stream(id))) → accept_item; anything else → DriverError::Correlation { expected: class name, found: describe(..) }. Delivery::Deliver then carries the validated identity: Delivery::Deliver { msg, correlation: Correlation } or a Delivered<M> struct, so ops_request_of, stream_of, and the Region::Request branch are deleted and the daemon reads delivered.request() / .stream() typed accessors that cannot be None for the class the arm declared.
  2. match_reply (:255-275): takes Correlation, matches Request(id) only; a Stream envelope on a reply arm is now unreachable because step 5 rejects it first.
  3. The client's is_reply_to peek (connector.rs:868-898) keeps working on the enum (Correlation::Request(named)).

Tests (driver/tests.rs, plus protocol unit tests):

  • envelope missing on each correlated class; both-ids encoding (hand-built CorrelationSlot bytes) rejected at peek_correlation; zero in the set arm; wrong class on every family (Ops::List with a stream id, Search::Query with a request id, Region::Row with a request id, Notify::Event with a request id, Grid::RowDelta with any envelope); daemon request sequence: skipped id (1 then 3), reused id, out-of-order valid replies (existing :593), stream cancel racing a terminal (existing :170, :197); client rejects a duplicate reply for a retired id (existing :613 covers unattributable; add duplicate).
  • serve/tests.rs:3623 an_uncorrelated_region_request_closes_the_connection moves to the driver table.

Docs: docs/reference/ipc.md:338-352 (Correlation shape: oneof, exclusive), :356-364 table gains the class column, :373-379 add "the daemon checks the request sequence like the stream sequence"; docs/explanation/architecture/ipc.md:544-615 add the rejected alternative (two optional scalars: an optional-field bag a non-Rust peer must reproduce by call-site behavior) and the prost last-wins note as the reason the peek keeps two scalars; felis.proto:446-467. CHANGELOG: "Wire: correlation is an exclusive request-or-stream identity; both-ids envelopes are malformed" (under the #30 protocol-2.0 entry). skills/felis: none (bridge JSON exposes ids? check docs/reference/control-surfaces.md bridge epoch; if the bridge JSON echoes request_id/stream_id as two keys, the JSON shape is unchanged because each object still carries one key).

Dependencies

Decide with #47 (the class lives on ArmMeta). #20 changes the Session classes; land #20 after this decision and before #46. #30 freezes last. #52's order (item 1) holds.

Risk/effort

M (1-2 days). Main risk: the daemon's new outstanding_requests set must be retired on every reply path (ops_reply in both loops, Region::Reply, Conn::Error { Request }), or the set grows for the connection's life; add a retire_request counterpart to retire_stream and a test that a reply for an unknown request is a driver error on the daemon's own writer path.

Labels

Keep priority/P1, release/v0.1.0.

## 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. ## Claim check Accurate against HEAD. - Schema: `message Correlation { uint64 request_id = 1; uint64 stream_id = 2; }` (`crates/felis-protocol/proto/felis.proto:461-467`), two independent scalars. Domain: `Correlation { request_id: Option<RequestId>, stream_id: Option<StreamId> }` (`crates/felis-protocol/src/messages.rs:421-447`). The conversion (`crates/felis-protocol/src/convert.rs:263-275`) rejects neither-set and accepts both-set; `codec::peek_correlation` (`codec.rs:110-115`) lifts it via `CorrelationSlot` (`:96-100`). - No arm needs both: the family table at `docs/reference/ipc.md:356-364` assigns exactly one id kind per arm. - `ConnectionDriver::account_for` (`crates/felis-transport/src/driver.rs:414-434`) looks only at `stream_id`; a `request_id` on an arm that should carry none passes silently. Required request ids are recovered afterwards by `ops_request_of` (`crates/felis-daemon/src/serve.rs:720-731`, called at `:628` and `:1221`), `stream_of` (`:1247-1258`, called at `:643`, `:1154`, `:1188`), and the inline `Region::Request` branch (`:1162-1171`). - `match_reply` (`driver.rs:255-275`) reads only `request_id`; a reply naming an outstanding request *and* a live stream is accepted as a reply. - The client allocates `request_id` sequentially (`issue_request`, `driver.rs:244-253`); the daemon's `StreamTable.next_request` (`:147`) is never advanced or compared on the daemon side. The reference promises "two independent sequences that start at 1, count strictly upward" (`docs/reference/ipc.md:373-379`), and the explanation says both ends keep the counters in step (`docs/explanation/architecture/ipc.md:613-615`) but only the stream counter is (`accept_open`, `driver.rs:438-471`). - Tests: `an_empty_correlation_envelope_closes_the_connection` (`driver/tests.rs:470`), `a_stream_id_on_an_arm_that_opens_none_closes_the_connection` (`:658`), `replies_match_their_requests_in_any_order` (`:593`); nothing on both-set, wrong class, or request-sequence gaps. ## Verdict **accept-with-changes**: the decision is requested; recommend the issue's oneof, with one implementation detail the issue does not mention (below) that is needed for its "the old both-ids encoding is rejected" criterion to be true. **Decision: `Correlation` becomes a protobuf `oneof id { uint64 request_id = 1; uint64 stream_id = 2; }`; the domain type becomes `enum Correlation { Request(RequestId), Stream(StreamId) }`; every arm declares one `CorrelationClass` in the #47 `ArmMeta`; the driver validates envelope against class in `decode`.** Why the oneof rather than a wider redesign: keeping tags 1 and 2 makes every honest frame on the wire today byte-identical after the change (a oneof of scalars encodes exactly as the single scalar did), so #30's golden conversations need no regeneration for this item; a sender physically cannot set both; and the two independent sequences, the ABA argument (`docs/explanation/architecture/ipc.md:599-607`), and `Subject` (`messages.rs:451-456`, already an exclusive enum) all stay as they are. The "deliberate conversation shape" for a hypothetical arm needing both (a request that also opens a stream) is a future arm carrying a second, named field, not a widened envelope. The detail: prost decodes a oneof with **last-field-wins**, so a both-set body would silently parse as `Stream`. Keep the private `CorrelationSlot` mirror in `codec.rs:96-100` as two plain scalars (it is already separate from the generated type) and have `peek_correlation` reject `request_id != 0 && stream_id != 0` as `WireError::AmbiguousCorrelation`. The public schema forbids both; the peek proves a peer did not send both anyway. Document that in the `CorrelationSlot` comment. ## Approach Protocol (`felis-protocol`): 1. `felis.proto:461-467`: oneof; comment states exclusivity and that 0/unset is malformed on every correlated arm that declares a class other than `Uncorrelated`. Regenerate (`just proto`). 2. `messages.rs:417-447`: `enum Correlation { Request(RequestId), Stream(StreamId) }`; keep the `request()`/`stream()` constructors as `const fn` for call-site compatibility; `describe` (`driver.rs:648-656`) prints the variant. 3. `convert.rs:252-275`: `From`/`TryFrom` over the oneof; unset arm → `MissingOneof("Correlation.id")`; zero value in the set arm → `MissingField`. 4. `CorrelationClass { Uncorrelated, RequestOpener, RequestReply, StreamOpener, StreamItem }` as the `correlation` column of `ArmMeta` (#47). Fill it from the table at `docs/reference/ipc.md:356-364`: `Ops` verbs `RequestOpener`, `Ops` replies `RequestReply`; `Region::Request`/`Reply` likewise, `Region::Rows` `StreamOpener`, `Row`/`RowsDone` `StreamItem`; `Notify::Subscribe` opener, `Subscribed`/`Event`/`Lagged` items; `Search::Query` opener, `Match` item; `Session` all `Uncorrelated` (until #20 decides; if #20 makes `Session::Create` correlated the class changes there); `Input`/`Grid`/`Image`/`Push`/`Conn` `Uncorrelated` (their wrappers reserve 100, so the class is also asserted by `MessageKind::is_correlated`, `lib.rs:114-119`; add a test that no arm of a non-correlated kind declares a class other than `Uncorrelated`). Driver (`felis-transport/src/driver.rs`): 5. `decode` (`:383-409`): after direction, `match (meta.correlation, payload.correlation)`: `(Uncorrelated, None)` deliver; `(RequestOpener, Some(Request(id)))` daemon side → `accept_request(id)`: must equal `table.next_request`, advance it, insert into `outstanding_requests` (the daemon's set of requests it owes a reply to; `Ops` reply writers then `retire_request(id)` like `retire_stream`); `(RequestReply, Some(Request(id)))` client side → `match_reply`; `(StreamOpener, Some(Stream(id)))` → `accept_open`; `(StreamItem, Some(Stream(id)))` → `accept_item`; anything else → `DriverError::Correlation { expected: class name, found: describe(..) }`. `Delivery::Deliver` then carries the validated identity: `Delivery::Deliver { msg, correlation: Correlation }` or a `Delivered<M>` struct, so `ops_request_of`, `stream_of`, and the `Region::Request` branch are deleted and the daemon reads `delivered.request()` / `.stream()` typed accessors that cannot be `None` for the class the arm declared. 6. `match_reply` (`:255-275`): takes `Correlation`, matches `Request(id)` only; a `Stream` envelope on a reply arm is now unreachable because step 5 rejects it first. 7. The client's `is_reply_to` peek (`connector.rs:868-898`) keeps working on the enum (`Correlation::Request(named)`). Tests (driver/tests.rs, plus protocol unit tests): - envelope missing on each correlated class; both-ids encoding (hand-built `CorrelationSlot` bytes) rejected at `peek_correlation`; zero in the set arm; wrong class on every family (`Ops::List` with a stream id, `Search::Query` with a request id, `Region::Row` with a request id, `Notify::Event` with a request id, `Grid::RowDelta` with any envelope); daemon request sequence: skipped id (1 then 3), reused id, out-of-order valid replies (existing `:593`), stream cancel racing a terminal (existing `:170`, `:197`); client rejects a duplicate reply for a retired id (existing `:613` covers unattributable; add duplicate). - `serve/tests.rs:3623` `an_uncorrelated_region_request_closes_the_connection` moves to the driver table. Docs: `docs/reference/ipc.md:338-352` (`Correlation` shape: oneof, exclusive), `:356-364` table gains the class column, `:373-379` add "the daemon checks the request sequence like the stream sequence"; `docs/explanation/architecture/ipc.md:544-615` add the rejected alternative (two optional scalars: an optional-field bag a non-Rust peer must reproduce by call-site behavior) and the prost last-wins note as the reason the peek keeps two scalars; `felis.proto:446-467`. CHANGELOG: "Wire: correlation is an exclusive request-or-stream identity; both-ids envelopes are malformed" (under the #30 protocol-2.0 entry). `skills/felis`: none (bridge JSON exposes ids? check `docs/reference/control-surfaces.md` bridge epoch; if the bridge JSON echoes `request_id`/`stream_id` as two keys, the JSON shape is unchanged because each object still carries one key). ## Dependencies Decide with #47 (the class lives on `ArmMeta`). #20 changes the `Session` classes; land #20 after this decision and before #46. #30 freezes last. #52's order (item 1) holds. ## Risk/effort **M** (1-2 days). Main risk: the daemon's new `outstanding_requests` set must be retired on every reply path (`ops_reply` in both loops, `Region::Reply`, `Conn::Error { Request }`), or the set grows for the connection's life; add a `retire_request` counterpart to `retire_stream` and a test that a reply for an unknown request is a driver error on the daemon's own writer path. ## Labels Keep `priority/P1`, `release/v0.1.0`.
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#48
No description provided.