Skip to content

fix: run reconcileSubscription after every fetch attempt - #27

Merged
jimmy-phantom merged 1 commit into
mainfrom
jimmy/reconcile-on-applydata-throw
May 18, 2026
Merged

fix: run reconcileSubscription after every fetch attempt#27
jimmy-phantom merged 1 commit into
mainfrom
jimmy/reconcile-on-applydata-throw

Conversation

@jimmy-phantom

@jimmy-phantom jimmy-phantom commented May 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Reactive getConfig() that switches subscribe to undefined on an error response (e.g. poll only while response.ok) silently failed when the error body did not match the result entity shape.

runQuery only called reconcileSubscription after applyData succeeded (QueryResult.ts:337), so parseEntities throwing on the body shape skipped the reconcile, the inner reactiveSignal never re-read this.config, and the running poll subscriber kept ticking against stale config.

Fix

Move the reconcileSubscription call into a finally block so it fires after every fetch attempt, regardless of whether parsing succeeds.

     return withRetry(
       async () => {
-        const freshData = await adapter.send(ctx, signal);
-        this.updatedAt = Date.now();
-
-        const result = this.applyData(freshData, true);
-        this.saveQueryMetadata();
-
-        this.reconcileSubscription();
-
-        return result;
+        try {
+          const freshData = await adapter.send(ctx, signal);
+          this.updatedAt = Date.now();
+
+          const result = this.applyData(freshData, true);
+          this.saveQueryMetadata();
+
+          return result;
+        } finally {
+          this.reconcileSubscription();
+        }
       },

On adapter-level failures where ctx.response was never updated this is a no-op: _resolvedOptions returns its cached value, the subscribe ref hasn't changed, and the ref check inside reconcileSubscription short-circuits.

Why the existing test missed this

stops polling when getConfig() switches subscribe to undefined after an error used result = { n: t.number } with a 404 body of { n: callCount }. The body matched the result schema, applyData succeeded, and the reconcile path ran normally. The test passed even though the bug was present for the entity-shape-mismatch case.

That test is rewritten to use t.entity() with a 404 body that does not match the entity shape ({error: 'Not found'}). It fails on main and passes with this change.

Test plan

  • Repro test fails on main, passes with the fix
  • Full unit suite: 1135 passed, 2 skipped
  • Manual verification by a consumer that hit this in a runtime where the existing test's "lucky" body shape didn't paper over the bug

🤖 Generated with Claude Code

Reactive getConfig() switching subscribe to undefined on an error response
(e.g. poll only while response.ok) silently failed when the error body
did not match the result entity shape. runQuery only called
reconcileSubscription after applyData succeeded, so parseEntities throwing
on the body shape skipped the reconcile, the inner reactiveSignal never
re-read this.config, and the running poll subscriber kept ticking against
stale config.

Move the call into a finally block so it fires after every fetch attempt.
On adapter-level failures where ctx.response was never updated this is a
no-op: the cached config is returned and the ref check inside
reconcileSubscription short-circuits.

The existing poll.test "stops polling when getConfig() switches subscribe
to undefined after an error" is rewritten to use t.entity() with an
unparseable 404 body, which fails on main and passes with this change.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jimmy-phantom
jimmy-phantom requested a review from pzuraq May 18, 2026 20:51
@jimmy-phantom
jimmy-phantom merged commit f7aaa5c into main May 18, 2026
1 check passed
@jimmy-phantom
jimmy-phantom deleted the jimmy/reconcile-on-applydata-throw branch May 22, 2026 01:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants