fix(client): resync on failed patch instead of silently corrupting the cache - #3
Merged
Conversation
…e cache A failed incremental patch in the WebSocket data handler used to escape onmessage, leaving the cache partially mutated and the subscription silently frozen — no snapshot ever arrived, so the client stopped updating until something else forced a reconnect. Apply patches non-mutatingly (mutateDocument: false) so a failed patch leaves the cache untouched (all-or-nothing), and wrap decode+patch in try/catch: on failure clear cache/version and drive the existing forced reconnect, which redials without a version and receives a full snapshot. Recovery runs before the onerror callback so a throwing consumer handler cannot leave the subscription frozen; onmessage is never called with a corrupt or stale cache. Adds a "patch failure resync" spec and bumps the version to 1.0.1. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
CI was red for a pre-existing toolchain reason unrelated to the resync
diff: the workflow ran an unpinned `go get github.com/benitogf/ooo` on a
Go 1.19 matrix, but current ooo imports stdlib `slices` (Go 1.21+), so the
harness build died with `package slices is not in GOROOT`.
- Bump go.mod: `go 1.19` -> `go 1.25`; pin ooo to `8c38013e` (merge of
#154, post-#150/#153/#154) so the resync is tested against the fixed
server, not the 2022 init pin. `go mod tidy`.
- Rewrite .github/workflows/test.yml: go 1.25.x, node 20.x, actions
checkout@v4/setup-node@v4/setup-go@v5, checkout-first. Replace the
unpinned `go get` with `go mod download && go build ./...` so CI builds
reproducibly against the go.mod pin.
- Adapt client.stats() to the fixed server's API: the new ooo serves an
explorer SPA at `/` and moved key-listing to `?api=keys`
(KeysResponse{keys,...}); the old server returned stats JSON at `/`.
stats() now GETs `?api=keys`, still satisfying the `Stats{keys}` type.
npm test: 9/9 specs, 45 expects, 0 failures (incl. `patch failure resync`)
against the fixed server on Go 1.25 + node.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tly corrupt
Self-review found the resync guarantee didn't hold for the row-drift class:
with fast-json-patch's validateOperation off, an out-of-bounds positional
op (`remove /5`/`replace /5` on a shorter cache) either no-ops silently
(cache stays divergent forever, no resync) or commits a sparse
`[...,null,{...}]` array to onmessage — the exact "silently stops updating"
/ corrupt-render behavior the fix claims to eliminate.
- Pass validateOperation: true so an unresolvable op throws
OPERATION_PATH_UNRESOLVABLE and drives the existing resync path; valid
ops still apply, so the suite stays 9/9.
- Rework the resync spec to corrupt the cache to `[]` (a realistic emptied/
drifted cache) instead of `null`; an empty array has no index for the
server's positional patch, so it is unresolvable and throws under
validation — matching the sibling plan's literal `client.cache = []`.
- stats(): page through ?api=keys (limit 500) until all keys are collected,
so stats().keys lists every key like the pre-explorer server did instead
of the endpoint's default 50-key first page. Return shape stays {keys}.
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
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.
What & why
This PR does two things on one branch. The client fix is the deliverable; the CI repair is folded in because a red PR can't merge.
1. Resync on failed patch instead of silently corrupting the cache
When an incremental list/object update fails to apply, the JS client used to break in three ways at once: the exception escaped the WebSocket message handler (leaving the cache partially mutated), the consumer kept rendering stale state, and the subscription stayed connected on its old version so no snapshot ever arrived — the client silently stopped updating until something else forced a reconnect.
The Go client already recovers correctly (a patch error breaks the read loop and the reconnect delivers a fresh snapshot). This gives the JS client the same guarantee.
mutateDocument: falseandvalidateOperation: true, so a failed patch leaves the cache untouched (all-or-nothing) and an op the cache can't resolve (a positionalremove/replaceaddressing an index a drifted cache doesn't have) throws instead of silently no-op'ing or committing a sparse[…,null,{…}]array. Both are the row-drift class that caused silent divergence.cacheandversionand drives its existing forced-reconnect path (the same one thereconnectspec pins). Withversioncleared the redial carries no?v=, so the server answers with a full snapshot and the next message delivers correct state.onerrorfires, so a throwing consumer handler can't leave the subscription frozen.onmessageis never called with a corrupt or stale cache.Normal reconnects are unchanged — they still send
?v=(thereconnectspec passes unmodified).New
patch failure resyncspec: seeds 3 records, drifts the cache to[]in-page, deletes a record so the server emits a positional patch the drifted cache can't apply, then assertsonerrorfired once, the redial URL carries no?v=, and a subsequent message delivers the correct 2 survivors.Version bumped 1.0.0 → 1.0.1. No API surface change (
ooo.d.tsunchanged).2. CI / toolchain repair (folded in — a red PR can't merge)
CI was red for a pre-existing toolchain reason unrelated to the resync diff: the workflow ran an unpinned
go get github.com/benitogf/oooon a Go 1.19 matrix, but current ooo imports the stdlibslicespackage (Go 1.21+), so the test harness build died withpackage slices is not in GOROOT.go 1.19→go 1.25; ooo pinned to8c38013e(the merge of ooo #154, post-#150/#153/#154) so the resync is tested against the fixed server rather than the 2022 init pin.go mod tidy.checkout@v4,setup-node@v4,setup-go@v5), checkout-first. The unpinnedgo getis replaced withgo mod download && go build ./...so CI builds reproducibly against the go.mod pin, not "whatever is newest".client.stats()— adapted to the fixed server's API. The new ooo serves an explorer SPA at/and moved key-listing to?api=keys(KeysResponse{keys,…}); the old server returned stats JSON at/.stats()now pages through?api=keysand returns{keys}, still satisfying theStats{keys}type and preserving the old all-keys behavior.Tests
Full suite green against the fixed server on Go 1.25 + node: 9/9 specs, 45 expects, 0 failures — including
patch failure resyncandkeys.Known non-blockers (out of scope, tracked)
mutateDocument: falseall-or-nothing property (not observable through the client's public surface; harness is integration-only).ooo.d.tsonerrortype accuracy (the plan pinsooo.d.tsunchanged as an acceptance criterion).🤖 Generated with Claude Code