From cb199de04e7e109df81959c14bd042c2475cf57c Mon Sep 17 00:00:00 2001
From: Prasad Pilla <prasad@supatest.ai>
Date: Mon, 20 Apr 2026 10:25:58 +0530
Subject: [PATCH] fix(electric-db-collection): guard markReady against
 post-cleanup onError race
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Problem

When `collection.cleanup()` is called (e.g. on logout or team switch),
the `AbortController` is signalled to stop the `ShapeStream`. However,
there is a race condition: `onError` can fire after `cleanup()` has run
but before the `ShapeStream` observes the abort signal.

The `onError` handler unconditionally calls `markReady()`, which throws:

```
CollectionStateError: Invalid collection status transition
  from "cleaned-up" to "ready"
```

This surfaces as an unhandled exception in applications (e.g. captured
by PostHog exception autocapture) and pollutes error tracking dashboards
with noise that has no actionable fix from the application side.

## Fix

Add an `abortController.signal.aborted` guard before calling `markReady()`
in the `onError` handler. `abortController` is already in scope and its
`abort()` method is already called during `cleanup()`, so the guard is
zero-overhead and correct:

```typescript
// Before
markReady()

// After
if (!abortController.signal.aborted) markReady()
```

## Test

Added regression test:
"should not throw CollectionStateError when onError fires after cleanup"

The test wires a real `AbortController` into the mock, calls `cleanup()`,
then fires `onError` and asserts no exception is thrown.

## Related

- PR #1174 fixed similar abort-race issues in `requestSnapshot` /
  `fetchSnapshot`, but the `onError` → `markReady()` path was not covered.
---
 .../electric-db-collection/src/electric.ts    |  6 ++-
 .../tests/electric.test.ts                    | 40 +++++++++++++++++++
 2 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/packages/electric-db-collection/src/electric.ts b/packages/electric-db-collection/src/electric.ts
index 72a9a072e..7f3818b8a 100644
--- a/packages/electric-db-collection/src/electric.ts
+++ b/packages/electric-db-collection/src/electric.ts
@@ -1422,7 +1422,11 @@ function createElectricSync<T extends Row<unknown>>(
           // Note that Electric sends a 409 error on a `must-refetch` message, but the
           // ShapeStream handled this and it will not reach this handler, therefor
           // this markReady will not be triggers by a `must-refetch`.
-          markReady()
+          // Guard against the race condition where collection.cleanup() aborts
+          // the stream but onError fires before the ShapeStream observes the
+          // abort signal. Calling markReady() on a cleaned-up collection throws
+          // CollectionStateError ("cleaned-up" → "ready" is invalid).
+          if (!abortController.signal.aborted) markReady()
 
           if (shapeOptions.onError) {
             return shapeOptions.onError(errorParams)
diff --git a/packages/electric-db-collection/tests/electric.test.ts b/packages/electric-db-collection/tests/electric.test.ts
index 30b15887b..a27579437 100644
--- a/packages/electric-db-collection/tests/electric.test.ts
+++ b/packages/electric-db-collection/tests/electric.test.ts
@@ -1917,6 +1917,46 @@ describe(`Electric Integration`, () => {
       expect(mockAbortController.abort).toHaveBeenCalledTimes(1)
     })
 
+    it(`should not throw CollectionStateError when onError fires after cleanup`, async () => {
+      // Regression test: cleanup() aborts the stream, but onError can still fire
+      // before the ShapeStream observes the abort signal. Without the guard,
+      // calling markReady() on a cleaned-up collection throws CollectionStateError
+      // ("cleaned-up" → "ready" is an invalid lifecycle transition).
+      const realAbortController = new AbortController()
+      mockAbortController = {
+        abort: vi.fn(() => realAbortController.abort()),
+        signal: realAbortController.signal,
+      }
+      global.AbortController = vi.fn().mockImplementation(() => mockAbortController)
+
+      let capturedOnError: ((params: unknown) => void) | undefined
+      const { ShapeStream } = await import(`@electric-sql/client`)
+      vi.mocked(ShapeStream).mockImplementation((options: any) => {
+        capturedOnError = options.onError
+        return mockStream as any
+      })
+
+      const config = {
+        id: `on-error-after-cleanup-test`,
+        shapeOptions: {
+          url: `http://test-url`,
+          params: { table: `test_table` },
+        },
+        getKey: (item: Row) => item.id as number,
+        startSync: true,
+      }
+
+      const testCollection = createCollection(electricCollectionOptions(config))
+      expect(capturedOnError).toBeDefined()
+
+      // Simulate cleanup (this aborts the controller)
+      await testCollection.cleanup()
+      expect(testCollection.status).toBe(`cleaned-up`)
+
+      // Simulate onError firing after cleanup — should not throw CollectionStateError
+      expect(() => capturedOnError!({ status: 500, json: {} })).not.toThrow()
+    })
+
     it(`should restart stream when collection is accessed after cleanup`, async () => {
       const config = {
         id: `restart-stream-test`,