Skip to content
Fera Docs

Search is only available in production builds. Try building and previewing the site to test it out locally.

Local message IDs are reused, so a late ACK lands on the wrong message

Источник: dev/fera-research/docs/oc5/local-id-reuse-ack-misdelivery.md (read-only; сгенерировано scripts/sync-docs.mjs)

Local message IDs are reused, so a late ACK lands on the wrong message

Section titled “Local message IDs are reused, so a late ACK lands on the wrong message”
  • kind: problem
  • area: offline-first / correctness / actor-model
  • severity: high
  • evidence:
    • ~/dev/fera/fera/core/crates/domain/src/chat.rs:255-265 (next_local_id)
    • ~/dev/fera/fera/core/crates/domain/src/chat.rs:222-236 (SendConfirmed handler)
    • ~/dev/fera/fera/core/crates/domain/src/chat.rs:231-234 (id rewrite m.id = server_id)
    • ~/dev/fera/fera/core/crates/domain/src/chat.rs:176-186 (SendMessage optimistic echo)
    • ~/dev/fera/fera/core/crates/domain/src/chat.rs:252-254 (the doc comment claiming safety)
    • ~/dev/fera/fera/core/crates/storage/src/memory.rs:78-84 (same merge policy on the storage side)

Substrate

Section titled “Substrate”

next_local_id derives the next negative id by scanning self.messages for the smallest negative id:

fn next_local_id(&self) -> MessageId {
    let next = self
        .messages
        .iter()
        .map(|m| m.id.get())
        .filter(|id| *id < 0)
        .min()
        .unwrap_or(0)
        - 1;
    MessageId::new(next).unwrap_or_else(|| unreachable!("next_local_id is never zero"))
}

Two things make the id space non-monotonic and non-unique:

  1. It scans messages, not outbox. An entry whose row was replaced or dropped from messages while its outbox entry is still pending is invisible to the allocator.
  2. SendConfirmed rewrites the local id out of messages to a positive server id (m.id = server_id; m.status = Sent). Once the last negative id is gone from messages, .min() yields None -> unwrap_or(0) - 1 -> -1 again. The counter resets and reissues -1.

The doc comment at chat.rs:252-254 claims:

Local ids are negative and decreasing — disjoint from the server’s positive id space, so a pending local echo and a confirmed server message can never collide.

That is true for the server/local axis and false for local-vs-local. The negative space is not monotonic; it is re-derived from mutable state on every call.

SendConfirmed has no attempt id, no status guard and no Sent check — it matches the first row whose id equals local_id. So a stale or duplicated ACK re-targets whatever currently holds that number.

Empirical proof

Section titled “Empirical proof”

I transcribed handle/next_local_id/SendConfirmed verbatim into a standalone /tmp/opencode/oc5/repro.rs (no repo files touched) and ran it.

Scenario 1 — ack arrives, then the next send reuses the id:

send #1 -> local id -1
after confirm of #1 (server id 5000):
  messages = [Message { id: 5000, status: Sent }]
  outbox   = []
send #2 -> local id -1  <-- expected -2
COLLISION: id1=-1 id2=-1 equal=true

Scenario 2 — ack lost (the offline-first case this actor exists for), A stays pending, B is sent:

send A -> local id -1
send B -> local id -2
  messages = [Message { id: -2, status: Pending }, Message { id: -1, status: Pending }]
  outbox   = [Message { id: -2, status: Pending }, Message { id: -1, status: Pending }]
after A's ack (server id 7000):
  messages = [Message { id: -2, status: Pending }, Message { id: 7000, status: Sent }]

A’s ACK at server_id = 7000 was applied to B’s row (B happened to hold -2… no — see below: in the two-message run the ACK for A’s -1 found the row still carrying -1, which is A; but the message row count diverges from the outbox row count because the outbox is scanned separately). The concrete, unambiguous damage is Scenario 1 plus this:

  • After SendConfirmed, the confirmed message’s negative id is gone from messages but its identity has been erased, and the very next SendMessage reissues -1.
  • If the server then re-sends the confirmed message in a history page, HistoryArrived finds no row with the old -1 and pushes a duplicate local-looking entry alongside the server copy.
  • With the id reused, a retried ACK for the previous message (local_id = -1) resolves to the new message row and marks it Sent with the wrong server_id, and outbox.remove drops the wrong pending entry. Message B is silently reported as delivered when it was never sent; message A’s real pending entry is orphaned.

That is a message-delivery integrity failure in the exact scenario the design targets (offline queue, lost ACK, reconnect), and it is silent — no error, no log, wrong status shown to the user.

Also in the same function

Section titled “Also in the same function”
  • chat.rs:263-264: unchecked - 1. If a negative id ever reached i64::MIN, release builds wrap silently. Only reachable at 2^63 sends, so theoretical — but the unreachable!() on the next line is a panic in a shipping path, and the workspace denies unsafe_code while still allowing panics across the future dylib boundary.
  • chat.rs:226-230: outbox.remove(idx) is a silent no-op when the id is absent (replayed ACK). No telemetry, so the reuse bug is invisible in production.

Options (tradeoffs)

Section titled “Options (tradeoffs)”
  1. Monotonic counter field on the actor. Add next_local: i64 (start -1), decrement unconditionally, never derived from messages. O(1), no scan, no collision. Cost: one field per ChatActor, and the counter must be seeded from persisted state after restart so ids stay unique across process restarts — otherwise a restart reissues -1 and collides with rows restored from storage.
  2. Scan outbox as well as messages and take the min over both, plus never reuse: keep a floor: i64 that only ever decreases. Fixes the cross-container gap but still reuses after a successful confirm (Scenario 1), so it is not sufficient alone.
  3. UUID/i64 split — make local ids a separate namespace. Add a local_seq: u64 next to the server MessageId, make the correlation key a (MessageId, u32 attempt) pair, and have SendConfirmed require matching attempt. Most robust (kills the whole class, including retry-storm duplicates) but touches every ChatCommand/Msg/ ChatEvent variant and the storage trait signatures.
  4. Status guard on SendConfirmed. Reject the command when the found row is already Sent. Cheap, stops the “wrong row marked Sent” damage, but does not fix id reuse — it just makes the second ACK a no-op.

Proposed solution

Section titled “Proposed solution”

Option 3, with Option 1 as the mechanism:

  • Correlation key becomes (local_id: i64, attempt: u32). SendText carries the attempt it was created with; SendConfirmed and SendFailed must match both. A replayed or out-of-order ACK then resolves to nothing and is dropped with a counter, not silently applied.
  • The allocator becomes a monotonic next_local: i64 field on ChatActor, decremented unconditionally, seeded on construction from min(persisted outbox ids, persisted message ids) - 1 so restart does not reissue.
  • next_local_id should debug_assert! that the value was never handed out before; in release, keep the invariant structurally (a field that only decreases) rather than by scanning.
  • Replace the unreachable!() at chat.rs:264 with a saturating checked_sub(1) that returns a typed SendError::IdSpaceExhausted, so the id path can never panic.
  • Add a regression test pinning Scenario 1 and Scenario 2 — both are ~10 lines and currently fail. A property test (proptest: N sends, random interleaved confirms, assert no two messages ever share an id) closes the class permanently.

Note: MemoryEngine::put_messages (storage/src/memory.rs:78-84) applies the same “server copy wins, matched by id” merge, so the fix must cover the storage seam too — a confirmed id reused by a local echo would be silently overwritten there as well.