State sync and persistence issues#3
Open
pmitros wants to merge 16 commits into
Open
Conversation
Fixes the delete-before-send loss path: the durable queue deleted an event
in the same transaction it read it for sending, and socket.send() is
fire-and-forget, so an event closed-tab in that window was lost silently
(3/73 pilot students lost the tail of a final session).
- Queue backend gains a non-destructive lease discipline alongside the
legacy destructive dequeue: leaseNext()/confirm(uptoSeq)/rewind()/
unconfirmedCount() in both memoryQueue and indexeddbQueue. Nothing is
deleted until the server acks its seq; rewind() resends everything
unconfirmed on reconnect (survives reload via IndexedDB autoIncrement id
as the durable seq).
- queue.ts: opt-in lease loop (onLease) + confirm/rewind/unconfirmedCount
passthroughs.
- websocketLogger: seq-tagged sends; {status:'ack',seq} -> confirm;
rewind on (re)connect; capability handshake (hello) gates ack mode.
Against ack-less servers (writing_observer, pre-upgrade lo-blocks)
behavior is byte-identical: send-and-delete.
- Capability resolution is a GATE: sends are held until hello (or a grace
timeout -> legacy), so the open->hello window can't send the rewind
backlog in delete-on-send mode against an ack-capable server.
- loEvent.unackedCount(): precise "is anything unsaved?" signal for the
beforeunload warning (begins retiring the blob-based heuristic).
Server half (hello + ack after durable write) and the adversarial
kill-the-tab integration test land next, on the server side. No npm
publish until end-to-end tested.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Hardening (Hub, canonical doc §3a). A require-ack client silently running legacy on a server without the ack half sends events un-seq'd and loses them on tab-close — the exact bug, masked by the blob path. Fail fast instead. - New websocketLogger option requireAck (default false; lo-blocks sets true, writing_observer and other ack-less consumers leave it off). - When capability resolves to no-ack (no hello / hello without ack / grace window expires): requireAck=false keeps today's legacy fallback; requireAck=true fails loud — console.error + a `lo_fatal` (code ACK_REQUIRED) CustomEvent for a visible banner, and does NOT open the send gate, so events accumulate in the durable queue instead of going out un-acked. The error is thrown once from the next wsLogData() (after enqueue) so the mis-deploy surfaces to the app. - A late hello WITH ack still recovers (opens the gate); a slow ack server isn't permanently broken. Wire contract unchanged (client-only). No npm publish until end-to-end tested. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Surfaces the fatal condition through lo_event's status external store, the
same path useConnected/useSaved use — so lo-blocks reads it via a hook,
fully reactive, instead of an imperative consumeCustomEvent (which would
bypass React).
- reduxLogger: sticky _fatal external-store field + getFatal(); consumes
the lo_fatal CustomEvent ({code,message} to set, null to clear).
- hooks: export useFatal() (useSyncExternalStore) returning
{ code, message } | null, and the FatalState type.
- websocketLogger: dispatch stays via util.dispatchCustomEvent; a late
ack-capable hello after a fatal now clears the sticky banner (recovery).
lo_fatal remains the internal transport; the consumer contract is the
hook. Also leaves a TODO to decide the fate of the stable-algorithm queue
tests at final PR review.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…scream) Reviewing to the standard "every event must make it into a log file": the requireAck throw-once from wsLogData was a delivery hazard. sendEvent (loEvent) re-throws any non-BlockError out of its logger fan-out, which runs over a DESTRUCTIVE front-desk queue — so the throw skipped every logger after websocketLogger and lost that event for them. And because sendEvent runs in the front-desk dequeue loop (not the caller's stack), the throw was swallowed by the loop's onError and never surfaced anyway. No visibility benefit, real delivery risk. The reactive useFatal() hook + console.error already provide the loud, visible surface. So capture stays unconditional (wsLogData always enqueues), and the fatal is surfaced only via the hook. Removes ackRequiredError entirely; fatalActive is the single banner guard. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
On a notable failure, log to all three of console, localStorage, and a consumer surface. Adds util.recordFailure() — a bounded, ring-buffered NDJSON log (lo_event_failures), capped by calculation (~128K chars ≈ 5% of the ~5MB browsers guarantee) so it can never grow toward the quota; not wired to every debug.error. Wired into the ACK_REQUIRED path alongside console.error and the useFatal dispatch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Expand the README from the original telemetry doc into the architecture/ philosophy home: the multi-source mission (diverse events into learning record stores; lo-blocks one source among many, engineered-data and messy-ingestion both first-class), the event format (NDJSON, ~80% xAPI/Caliper, layers/onions grounded in the tincan user/timestamp pain, send-once/denormalize), the two workflows incl. a fire-and-forget usage walkthrough (init/lockFields/go/logEvent), the delivery standard (capture hard / transmission soft / UX soft), durability + ack protocol + capability negotiation + requireAck, client state-sync + the three planes, React hooks, failure handling, and the testing philosophy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both reviewers (correctly) caught a deadlock the gate introduced. capsGate /
openCapsGate are module-level and resetCaps() reassigns them on every
newWebsocket(). If a send parked on `await capsGate` in the pre-hello window
and the connection dropped before hello, the next connection's resetCaps()
orphaned the old gate's resolver — the parked sendLeased awaited a promise
nothing could ever resolve, hanging the whole lease loop (events still
enqueued durably, but nothing sent until reload). Reachable on essentially
every reconnect with a backlog — the exact resend case this feature targets.
Fix:
- Capture THIS connection's gate resolver locally (myOpen) and, on disconnect
(after READY=false, before the next resetCaps), resolve it — unblocking any
parked sendLeased before the gate is orphaned. Idempotent if hello/grace
already opened it.
- sendLeased bails if !READY after the gate resolves (the disconnect case):
no send, no confirm — the item stays leased-but-unconfirmed and rewind()
resends it on reconnect. Prevents sending on a dead socket / confirm-
deleting an unsent event.
Also fixes the README websocketLogger example: pass the URL as a string (the
{url} object form re-prepends the page protocol → wss://wss://...).
Gate-across-reconnect coverage belongs in the server-side integration harness
(open, park a send by withholding hello, drop, reconnect, assert delivery
resumes) rather than a committed socket mock.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two pre-existing P2s from review, both on the no-server / ack-less paths that Writing Observer exercises — folded in now to avoid a separate release round. - Initial WS-connect failures no longer masquerade as "no websocket configured": the connection loop dispatches connected:false at start, so useConnected() reads false (offline) instead of null while never-yet- connected. setConnected dedupes, so it's a no-op once connected. - Local-only redux stores now load and persist: new opt-in ReduxLoggerOptions .localOnly resolves IS_LOADED at init when no fetch_blob server will arrive, so localStorage saves run and useLoaded() becomes true. Opt-in — remote- backed consumers are unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round-2 review (both P1s valid, verified against the code): - Stale grace timer could resolve the NEXT connection as legacy. connGen was bumped only on connection open, so a previous connection's pending 3s timer, firing while the next connection was still connecting, passed its `myGen === connGen` guard and operated on the new connection's helloSeen/gate — opening it legacy pre-hello (data-loss window with requireAck:false). Fix: bump connGen on disconnect too, invalidating the old connection's timer/ gate-then before the next one is established. - Back out localOnly. It marked IS_LOADED without restoring: reduxLogger only WRITES localStorage (no read/restore path — loadState is commented out), so localOnly flipped loaded and the first change overwrote the saved blob with default state, and save status stayed 'modified' forever (it reused the server-save path awaiting an ack a local-only config can't produce). Half-done it's misleading and data-lossy. Real local-only support is a feature (restore path + local save-status + skip the server leg) — a designed follow-up, noted in the code. The offline-status fix from the same review round stays (f6eb92d). Also: note HELLO_GRACE_MS could become a per-logger setting when needed (YAGNI). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Round-3 review nits: - Document `seq` as a reserved top-level transport field (README + tagSeq comment). It's the wire-contract field the server acks against; apps must not use a top-level `seq`. No runtime collision guard — a documented reserved key, not a per-event phantom case to police. (Namespacing it is a contract change; Hub's call if desired.) - Fix useLoaded()/getLoaded() docs: they still claimed "no persistence configured" resolves loaded, contradicting the (now unsupported) local-only note. Loaded resolves on a fetch_blob load cycle. - Fix README: only the IndexedDB-backed queue survives reloads/restart; the in-memory fallback survives in-process outages/reconnects but not a reload. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
package.json declares react (>=18.0.0, optional) as a peer dependency for the hooks entry; the lockfile hadn't been regenerated to match. Minimal sync, no dependency changes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses several issues with state synchronization:
predicateThis allows for much more robust state management.