fable5findings

FABLE_5_FINDINGS.md

@@ -0,0 +1,107 @@
+# Fable 5 findings
+
+Review of the issues Fable found after the Call to Play follow-up fixes. This
+document records the assessment only; it is not an implementation plan.
+
+## 1. IP-based actor verification can reject legitimate peers
+
+**Assessment: real and must-fix.**
+
+`handle_call_to_play_events` requires the QUIC connection's source IP to match
+the peer's advertised listener IP. That assumption is unreliable for a
+multi-homed LAN machine: routing may select Ethernet, Wi-Fi, a VPN, or a bridge
+address different from the advertised address. The receiver silently drops the
+event, while the one-way sender sees a successful write and does not invoke its
+resync fallback.
+
+The check is not strong authentication either. Two peers on one host share an
+IP, and the project currently uses a shared application TLS certificate rather
+than a cryptographic identity unique to each `peer_id`. A normal client still
+stamps its own peer ID, so same-host impersonation requires a modified client,
+but the IP comparison should not be presented as an authenticated identity
+boundary.
+
+A sound correction must either bind a peer ID to a connection through a
+same-connection protocol exchange, return an application-level
+acknowledgement/rejection that can trigger resync, or deliberately rely on the
+documented trusted-LAN model without the unreliable IP equality test.
+
+## 2. Evicted event IDs can prevent a revived call from healing
+
+**Assessment: real and must-fix together with finding 5.**
+
+Expiry removes call events from `events` but leaves their IDs in `event_ids`.
+If one peer expires a call and later receives an orphan `AddTime`, a subsequent
+handshake cannot restore the original `Create`: it is rejected forever as a
+duplicate. The call can therefore be alive on its creator while remaining
+invisible on the other peer. The ID set also grows without a bound for the
+session.
+
+Pruning expired IDs alone is not sufficient with the current `insert_all`
+behavior. A handshake commonly supplies `Create` followed by later actions;
+per-event compaction can expire and remove `Create` before the merge reaches
+the extending `AddTime`. Healing requires atomic batch semantics: validate and
+deduplicate the batch, merge it with retained events, then compact once using
+the complete history.
+
+Retained IDs should correspond to retained events and deliberate terminal
+tombstones, not every event ever observed.
+
+## 3. `insert` can accept an event that compaction immediately removes
+
+**Assessment: correct, lower-severity correctness/semantics issue.**
+
+An action targeting a call that expired more than five minutes ago can be
+inserted, removed by compaction in the same operation, and still return
+`Ok(true)`. It is then emitted to the local UI and broadcast to peers even
+though it has no durable effect.
+
+The presentation reducer normally hides the result, but the accepted signal is
+misleading and the work is wasted. Store results should distinguish retained,
+duplicate, rejected, and immediately obsolete events, and only retained events
+should be emitted or broadcast.
+
+## 4. "Add 5 more minutes" can shorten the deadline
+
+**Assessment: real user-visible bug.**
+
+The action currently sets the deadline to `now + 5 minutes`. If a roster fills
+early while the call still has more than five minutes remaining, the button
+shortens the call despite saying that it adds time.
+
+The intended calculation is `max(now, current deadline) + 5 minutes`. That also
+behaves naturally for an already expired call.
+
+## 5. Handshake history merge is quadratic
+
+**Assessment: correct, and part of the correctness fix for finding 2 rather
+than merely a performance nit.**
+
+`insert_all` calls `insert` for every incoming event, and each insertion rebuilds
+several maps over the growing store while holding its write lock. Merging a
+large handshake history is therefore O(n²).
+
+Compacting once after an atomic batch merge removes that cost and is also what
+allows `Create` plus a later `AddTime` to revive consistently. Findings 2 and 5
+should be treated as one store-semantics problem.
+
+## 6. The startup error points users at the wrong cause
+
+**Assessment: correct UX issue.**
+
+When `actorId` is still unavailable, the peer may simply be starting. Telling
+the user to choose a game folder is misleading unless the application actually
+knows that the folder is missing. The normal startup state should say that Call
+to Play is connecting and ask the user to try again shortly; folder guidance
+should be reserved for a known missing-folder condition.
+
+## Priority
+
+1. Replace the unreliable IP identity check or make delivery acknowledged.
+2. Correct batch merge, compaction, and event-ID retention as one change.
+3. Correct the deadline-extension calculation.
+4. Stop reporting and broadcasting immediately obsolete events.
+5. Correct the startup message.
+
+The existing QUIC connection must not be described as carrying an authenticated
+per-peer identity until the protocol actually establishes one.