test(client): add fast-check model-based property tests and retry bound analysis#4089
Open
KyleAMathews wants to merge 13 commits intomainfrom
Open
test(client): add fast-check model-based property tests and retry bound analysis#4089KyleAMathews wants to merge 13 commits intomainfrom
KyleAMathews wants to merge 13 commits intomainfrom
Conversation
Adds model-based testing using fast-check to generate adversarial server response sequences and verify the retry counter behaves correctly under all orderings. The model tracks expected consecutive error count and termination state; fast-check generates 100 random 80-command sequences mixing 200s, 204s, 400s, and malformed 200s, verifying the model matches the real ShapeStream at every step. This catches the class of bugs where counter resets interact with error sequences in unexpected ways — the kind of ordering-dependent issues that hand-written tests miss because humans only think to test particular sequences. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eaders Adds 3 new command types to the fast-check model-based property test: - Respond409: verifies 409 (shape rotation) doesn't affect retry counter - Respond200Empty: verifies empty message batch doesn't reset counter - RespondMissingHeaders: verifies non-retryable errors terminate immediately Also fixes cross-iteration cache pollution (expiredShapesCache, localStorage) and tracks handle rotation so 409 → subsequent response sequences are protocol-correct. Bumped to 200 runs with 8 command types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tests Adds global invariant assertions checked after every command: - URL length bounded at 2000 chars (catches -next suffix accumulation) - isUpToDate + lastSyncedAt consistency (catches silent stuck states) - Post-409 URL differs from pre-409 URL (catches identity loops) Also tracks request URLs in FetchGate and exposes stream instance for observable state assertions. These invariants are drawn from 25 historical bugs identified in the client's git history. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4089 +/- ##
==========================================
+ Coverage 88.67% 88.84% +0.16%
==========================================
Files 25 25
Lines 2438 2430 -8
Branches 615 604 -11
==========================================
- Hits 2162 2159 -3
+ Misses 274 269 -5
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Enhances the static analysis script to exhaustively find recursive calls in catch blocks and classify their retry bounds. For each recursive call, walks the AST to detect counter guards, type guards (instanceof/status checks), abort signal checks, and callback gates. Generates findings for any completely unbounded recursive retries. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…eak in #start Two fixes: 1. Both 409 handlers (#requestShape and #fetchSnapshotWithRetry) now unconditionally create a cache buster instead of only doing so conditionally when the handle is missing or recycled. This eliminates the same-handle 409 infinite loop (where identical retry URLs would hit CDN cache forever) and removes two conditional branches, making the behavior safer and easier to verify exhaustively. 2. Changed `await this.#start(); return` to `return this.#start()` in the onError retry path. The old pattern parked the outer #start frame on the call stack for the entire lifetime of the replacement stream, accumulating one frame per error recovery. The new pattern resolves the outer frame immediately. Also adds model-based test commands for 409-no-handle and 409-same-handle scenarios, plus a targeted regression test verifying consecutive same-handle 409s produce unique retry URLs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…sters Adds a new analysis rule that finds all 409 status handlers in the ShapeStream class and verifies each unconditionally calls createCacheBuster(). This would have caught the same-handle 409 bug where a conditional cache buster allowed identical retry URLs. The rule: - Finds if-statements checking .status == 409 or .status === 409 - Handles compound conditions (e.g. `e instanceof FetchError && e.status === 409`) - Verifies createCacheBuster() is called outside any nested if-block - Reports with the exact method, line numbers, and retry callee RED/GREEN verified: temporarily reverting to conditional cache buster correctly triggers the finding. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… list The deprecated `experimental_live_sse` query param was missing from ELECTRIC_PROTOCOL_QUERY_PARAMS, causing canonicalShapeKey to produce different keys for the same shape depending on whether the SSE code path added the param to the URL. This caused: - expiredShapesCache entries written during SSE to be invisible when the stream fell back to long polling - upToDateTracker entries from SSE sessions to be lost on page refresh - fast-loop cache clearing to target the wrong key during SSE Also adds a static analysis test that verifies all internal protocol query param constants are included in the protocol params list, and updates SPEC.md with the unconditional 409 cache buster invariant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
409 responses carry only a must-refetch control message — no user data. The #reset call already handles the state transition structurally. The #publish call was delivering empty (or near-empty) batches to subscribers because #publish lacks the empty-batch guard that #onMessages has. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Detects `await this.#method(); return` patterns in recursive methods where `return this.#method()` would avoid parking the caller's stack frame. RED/GREEN verified: reintroducing the old `await this.#start()` pattern triggers the finding; the current `return this.#start()` is clean. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ky SSE test Add new static analysis rule detecting #publish/#onMessages calls inside catch blocks or HTTP error status handlers — catches the Bug #4 pattern (publishing stale 409 data to subscribers). RED/GREEN verified. Also: update SPEC.md loop-back site line numbers, fix 409 handler comment, DRY improvements to model-based tests, fix flaky SSE fallback test (guard controller.close() against already-closed stream, widen SSE request count tolerance). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous commit removed the raw 409 body publish entirely, but Shape relies on the must-refetch control message to clear its accumulated data and trigger snapshot re-execution. Instead of publishing the raw response body (which could contain stale data rows), publish a synthetic control-only message. Also: fix flaky SSE fallback test (remove brittle upper bound on SSE request count), refine error-path-publish rule to allow static array literal arguments (synthetic control messages) while still catching dynamic publishes from error data. Co-Authored-By: Claude Opus 4.6 <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.
Summary
Fixes four bugs in ShapeStream's retry and error-handling paths, found via model-based property testing and static analysis. The bugs could cause infinite retry loops, stale data delivery to subscribers, stack frame leaks, and cache key divergence.
Root Cause
All four bugs share a pattern: they're edge cases in error-handling paths that only manifest under specific sequences of server responses. Hand-written tests don't naturally explore adversarial sequences like "409 with same handle, then 409 with no handle, then 200 with empty body." These bugs survived because they required rare combinations — a 409 where the proxy strips the handle header, or a deprecated query param that nobody adds to the protocol list.
Approach
Two complementary verification strategies that catch bugs mechanically:
Model-based property testing (fast-check)
fc.asyncModelRunwith 8 command types generates adversarial server response sequences. A simple model tracks{ consecutiveErrors, terminated }, and each command predicts the model's state change before asserting the real ShapeStream matches.FetchGate pattern: A controllable mock fetch that blocks each request until the test provides a response — turn-based coordination where the test controls what the server returns while ShapeStream drives when it fetches.
Global invariants checked after every command:
isUpToDate/lastSyncedAtconsistencyStatic analysis via AST walking
Seven rule types that mechanically detect structural bug patterns:
unbounded-retry-loopconditional-409-cache-bustercreateCacheBuster()is conditional or missingparked-tail-awaitawait this.#method(); returnpatterns that park stack frameserror-path-publish#publish/#onMessagescalls inside catch blocks or error status handlersshared-instance-fieldignored-response-transition{ action: 'ignored' }protocol-literal-driftEach rule was RED/GREEN verified: temporarily reintroduce the bug → rule fires; revert → rule passes clean.
Bugs Fixed
Bug 1: Conditional cache buster on 409
When the server returned a 409 with the same handle (or no handle),
createCacheBuster()was only called in the no-handle branch. Same-handle 409s produced identical retry URLs, causing infinite CDN-cached retry loops.Fix:
createCacheBuster()is now unconditional on every 409 — both in#requestShapeand#fetchSnapshotWithRetry.Bug 2: Parked stack frame in
#startretryawait this.#start(); returnkept the caller's frame alive for the entire recursive chain. Under repeated error-recovery cycles, this accumulated O(n) suspended frames.Fix:
return this.#start()releases the frame via promise chaining.Bug 3: Missing
EXPERIMENTAL_LIVE_SSE_QUERY_PARAMin protocol paramsThe deprecated
experimental_live_sseparam wasn't inELECTRIC_PROTOCOL_QUERY_PARAMS, socanonicalShapeKeywouldn't strip it. Clients using the deprecated param would get different cache keys from clients using the currentlive_sseparam.Fix: Added to the array. Static analysis test now verifies all internal
*_QUERY_PARAMexports are in the list.Bug 4: Publishing 409 body to subscribers
The 409 handler parsed
e.jsonand calledawait this.#publish(messages409)before retrying. This delivered stale/partial rotation messages to subscribers, which could corrupt client-side state.Fix: Removed the publish call entirely. 409 is a control-plane signal — the retry will deliver fresh data from offset -1.
Key Invariants
createCacheBuster()before retryingawait this.#method(); returnin recursive methods (usereturn this.#method()instead)#publishor#onMessagescalls in error handling paths*_QUERY_PARAMconstants appear inELECTRIC_PROTOCOL_QUERY_PARAMSNon-goals
#start/#requestShapeinto iterative loops (structural change, separate PR)pgArrayParserdouble-push bug (pre-existing, unreachable for valid PostgreSQL arrays)queueMicrotasktiming issue (pre-existing, requires deeper investigation)Verification
cd packages/typescript-client pnpm vitest run --config vitest.unit.config.tsFiles changed
src/client.tsreturn this.#start()instead ofawait, removed#publishfrom 409 handler, updated 409 commentsrc/constants.tsEXPERIMENTAL_LIVE_SSE_QUERY_PARAMtoELECTRIC_PROTOCOL_QUERY_PARAMStest/model-based.test.tstest/static-analysis.test.tsbin/lib/shape-stream-static-analysis.mjsSPEC.mdvitest.unit.config.tspackage.jsonfast-checkdevDependency🤖 Generated with Claude Code