4.9 KiB
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
- Replace the unreliable IP identity check or make delivery acknowledged.
- Correct batch merge, compaction, and event-ID retention as one change.
- Correct the deadline-extension calculation.
- Stop reporting and broadcasting immediately obsolete events.
- 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.