Skip to content

Commit 36d1c25

Browse files
KyleAMathewsclaude
andcommitted
fix(client): warn loudly when self-healing accepts stale proxy data
Addresses three findings from external review of the self-healing PR: 1. Detect and warn when a post-self-heal response carries the same handle we just marked expired. Previously the client silently accepted stale data with no operator signal — now it emits a targeted warning naming the handle and pointing at the proxy cache-key misconfiguration that causes this. 2. Tighten the recovery-guard clearing check from `accepted + kind === live` to an explicit `status === 204`, matching the comment's intent and removing latent fragility if the state machine ever starts transitioning to live for non-204 responses. 3. Update the `#reset()` comment to list all three callers (#requestShape's 409 handler, #checkFastLoop, and stale-retry self-healing) instead of only the 409 handler. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 1f7a5f4 commit 36d1c25

File tree

2 files changed

+55
-8
lines changed

2 files changed

+55
-8
lines changed

packages/typescript-client/src/client.ts

Lines changed: 37 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -624,6 +624,7 @@ export class ShapeStream<T extends Row<unknown> = Row>
624624
#pendingRequestShapeCacheBuster?: string
625625
#maxSnapshotRetries = 5
626626
#expiredShapeRecoveryKey: string | null = null
627+
#pendingSelfHealCheck: { shapeKey: string; staleHandle: string } | null = null
627628

628629
constructor(options: ShapeStreamOptions<GetExtensions<T>>) {
629630
this.options = { subscribe: true, ...options }
@@ -1225,6 +1226,25 @@ export class ShapeStream<T extends Row<unknown> = Row>
12251226
? expiredShapesCache.getExpiredHandle(shapeKey)
12261227
: null
12271228

1229+
// If this response is the first one after a self-healing retry, check
1230+
// whether the proxy/CDN returned the exact handle we just marked expired.
1231+
// If so, the client is about to accept stale data silently — loudly warn
1232+
// so operators can detect and fix the proxy misconfiguration.
1233+
if (this.#pendingSelfHealCheck) {
1234+
const { shapeKey: healedKey, staleHandle } = this.#pendingSelfHealCheck
1235+
this.#pendingSelfHealCheck = null
1236+
if (shapeKey === healedKey && shapeHandle === staleHandle) {
1237+
console.warn(
1238+
`[Electric] Self-healing retry received the same handle "${staleHandle}" that was just marked expired. ` +
1239+
`This means your proxy/CDN is serving a stale cached response and ignoring cache-buster query params. ` +
1240+
`The client will proceed with this stale data to avoid a permanent failure, but it may be out of date until the cache refreshes. ` +
1241+
`Fix: configure your proxy/CDN to include all query parameters (especially 'handle' and 'offset') in its cache key. ` +
1242+
`For more information visit the troubleshooting guide: ${TROUBLESHOOTING_URL}`,
1243+
new Error(`stack trace`)
1244+
)
1245+
}
1246+
}
1247+
12281248
const transition = this.#syncState.handleResponseMetadata({
12291249
status,
12301250
responseHandle: shapeHandle,
@@ -1239,9 +1259,9 @@ export class ShapeStream<T extends Row<unknown> = Row>
12391259

12401260
this.#syncState = transition.state
12411261

1242-
// Clear recovery guard when response transitions directly to live (e.g. 204),
1243-
// since #onMessages won't run for empty bodies.
1244-
if (transition.action === `accepted` && this.#syncState.kind === `live`) {
1262+
// Clear recovery guard on 204 (no-content), since the empty body means
1263+
// #onMessages won't run to clear it via the up-to-date path.
1264+
if (status === 204) {
12451265
this.#expiredShapeRecoveryKey = null
12461266
}
12471267

@@ -1265,6 +1285,16 @@ export class ShapeStream<T extends Row<unknown> = Row>
12651285
new Error(`stack trace`)
12661286
)
12671287
this.#expiredShapeRecoveryKey = shapeKey
1288+
// Arm a post-self-heal check: if the next response comes back
1289+
// with the same handle we just marked expired, the proxy/CDN is
1290+
// still serving stale data and we'll warn loudly instead of
1291+
// accepting it silently.
1292+
if (shapeHandle) {
1293+
this.#pendingSelfHealCheck = {
1294+
shapeKey,
1295+
staleHandle: shapeHandle,
1296+
}
1297+
}
12681298
this.#reset()
12691299
throw new StaleCacheError(
12701300
`Expired handle entry evicted for self-healing retry`
@@ -1770,9 +1800,10 @@ export class ShapeStream<T extends Row<unknown> = Row>
17701800
#reset(handle?: string) {
17711801
this.#syncState = this.#syncState.markMustRefetch(handle)
17721802
this.#connected = false
1773-
// releaseAllMatching intentionally doesn't fire onReleased — it's called
1774-
// from within the running stream loop (#requestShape's 409 handler), so
1775-
// the stream is already active and doesn't need a resume signal.
1803+
// releaseAllMatching intentionally doesn't fire onReleased — every caller
1804+
// (#requestShape's 409 handler, #checkFastLoop, and stale-retry
1805+
// self-healing in #onInitialResponse) runs inside the active stream loop,
1806+
// so the stream is already active and doesn't need a resume signal.
17761807
this.#pauseLock.releaseAllMatching(`snapshot`)
17771808
}
17781809

packages/typescript-client/test/expired-shapes-cache.test.ts

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -673,11 +673,13 @@ describe(`ExpiredShapesCache`, () => {
673673
expect(expiredShapesCache.getExpiredHandle(expectedShapeUrl)).toBeNull()
674674
})
675675

676-
it(`self-healing accepts stale data when proxy always serves expired handle (by design)`, async () => {
676+
it(`self-healing accepts stale data when proxy always serves expired handle (by design) but warns loudly`, async () => {
677677
// Finding 1 from external review: after self-healing clears the expired
678678
// entry, if the proxy serves the same stale response with up-to-date,
679679
// the client accepts it. This is by design — stale data is better than
680-
// permanent 502 for one-shot streams.
680+
// permanent 502 for one-shot streams — but the client MUST warn loudly
681+
// so operators can detect and fix the proxy misconfiguration.
682+
const warnSpy = vi.spyOn(console, `warn`).mockImplementation(() => {})
681683
const expectedShapeUrl = `${shapeUrl}?table=test`
682684
const staleHandle = `expired-handle`
683685

@@ -744,6 +746,20 @@ describe(`ExpiredShapesCache`, () => {
744746
expect(stream.isUpToDate).toBe(true)
745747
// 4 stale retries + 1 self-healing = 5
746748
expect(requestCount).toBe(5)
749+
750+
// CRITICAL: silent acceptance of stale data would be a bug. The client
751+
// MUST emit a warning naming the stale handle so operators can detect
752+
// and fix the proxy misconfiguration. Without this signal, apps would
753+
// sit on stale data with no way to know.
754+
const staleAcceptWarning = warnSpy.mock.calls.find(
755+
(args) =>
756+
typeof args[0] === `string` &&
757+
args[0].includes(
758+
`Self-healing retry received the same handle "${staleHandle}"`
759+
)
760+
)
761+
expect(staleAcceptWarning).toBeTruthy()
762+
warnSpy.mockRestore()
747763
})
748764

749765
it(`should clear recovery guard after 204 so self-healing works again`, async () => {

0 commit comments

Comments
 (0)