Keep optimistic usage updates in track when the replicator is not ready - #202
Merged
Merged
Conversation
track only bumped the cached company metric when isConnected() was true. In replicator mode that is the health endpoint's ready field, which goes false when Schematic is unreachable or the account is closed while the replicator's Redis cache stays intact. Flag checks keep evaluating from that cache, so usage stopped counting locally and numeric limits stopped moving. The event itself was still enqueued either way. The gate has no recorded rationale. It came from the Go SDK's optimistic metric updates, written before replicator mode existed, when "connected" was the only signal that the DataStream was in use at all. Replicator mode later redefined isConnected() as replicator readiness without revisiting it. The bump only touches a company already in the cache, and the next company the server sends replaces the metric value instead of adding to it, so running it while not ready cannot double count. Drop the isConnected() gate so the local update runs whenever a DataStream client exists, in both modes. In WebSocket mode a disconnected client also keeps evaluating cached companies, so the same reasoning applies. Add a replicator mode test with a real DataStream client, fake Redis seeded in the replicator's key layout, the real WASM engine, and a health endpoint reporting ready: false. track bumps the cached metric, a metric-gated flag flips once the threshold is reached, and a company that is not cached is left alone. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2 of 3 tasks
bpapillon
approved these changes
Sep 29, 2026
ryanechternacht
added a commit
that referenced
this pull request
Sep 30, 2026
The test from #202 expected a metric-gated flag check to flip from the cache while the replicator is not ready. Flag checks now skip the cache until it is ready, so the test tracks while not ready and checks the flag after the replicator reports ready. Co-Authored-By: Claude Opus 5.5 (1M context) <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.
Problem
trackenqueues the event and then optimistically bumps the cached company metric viaupdateCompanyMetrics. That second step was gated ondatastreamClient.isConnected(), which in replicator mode is the health endpoint'sreadyfield. When a customer's Schematic account is closed or Schematic is unreachable, the replicator stays up with its Redis cache intact but reportsready: false. Flag checks keep evaluating from that cache, but usage stopped counting locally, so numeric limits never moved for as long as the replicator was not ready. The event was still enqueued to the API either way.What I found
History of the gate:
body.company && this.useDataStream() && this.datastreamClient!.isConnected()with the comment "if available and connected". Later edits (Chris/schy 380 sdk entitlement usage does not update from datastream #127, credit lease SDK primitives: check / trackWithReservation / prewarm #121, Close the lease-client gaps the port reviews found #194) carried the condition forward unchanged; Close the lease-client gaps the port reviews found #194 added theupdateMetricsflag. No PR discussion or commit message explains the connected check.body.Company != nil && c.datastreamConnected. At that pointuseDataStream()itself returneddatastreamConnected, so "connected" was simply the signal that the DataStream was in use. 🌿 Fern Regeneration -- February 25, 2026 #82 split it mechanically intouseDataStream() && IsConnected(). Replicator mode came later (SCHY-148 Use miniflare to test builds in a cloudflare environment #88, September 2025) and redefinedIsConnected()as replicator readiness without revisiting this call site. Go still has the same gate inemitTrack.Correctness checks:
updateCompanyMetricsreads the company from the cache and returns without writing if it is not there, so a not ready replicator with an empty or partial cache is a no-op. The test covers this.trackWithReservationpassesupdateMetrics = falsewhen the settle did not land locally (Close the lease-client gaps the port reviews found #194).So I found no correctness reason for the gate.
One related issue this does not change:
updateCompanyMetricsrewrites the company and its lookup keys with the SDK'scacheTTL(24 hours by default), while the replicator writes them with no TTL by default. That already happens today whenevertrackruns with the replicator ready, including for events that match no metric (the company is rewritten regardless). With this change it can also happen while the replicator is not ready, and during an outage longer thancacheTTL, a company that was tracked and then went idle forcacheTTLwould drop out of Redis until the replicator resyncs. I'd suggest a follow-up so the metric write in replicator mode keeps the existing expiry and skips the write when no metric matched.Change
The
isConnected()condition is removed from the metric update inemitTrack, so the local update runs whenever a DataStream client exists, in both modes. In WebSocket mode a disconnected client also keeps evaluating cached companies incheckFlag, so skipping the bump there would under count in the same way. The cache is the SDK's own in that mode, so the TTL point above does not apply.Test
New
tests/unit/replicator/track-not-ready.test.ts(and a.fernignoreentry for the directory). It uses a real DataStream client in replicator mode, the real WASM rules engine, and the in-memory fake Redis seeded the way the replicator writes it (snake_case JSON under versioned keys).fetchis stubbed so the health endpoint returns{ ready: false, cache_version: "v-test" }.trackwith quantity 2 moves the cachedapi_callmetric from 3 to 5.api_call >= 5evaluates false, stays false after onetrack, and flips to true after the second, with the API mocked to fail.The first two fail on main and pass with this change.
yarn buildandyarn testpass (101 suites; the Redis integration suite is skipped locally withoutTEST_REDIS_URL).This branch and #201 each add the same
tests/unit/replicator/line to.fernignoreat the same spot, so they merge cleanly in either order.🤖 Generated with Claude Code