Skip to content

fix(migration): always close leader-election broadcastChannel#8780

Open
pubkey wants to merge 3 commits into
masterfrom
claude/leader-elector-channel-cleanup-g4elu8
Open

fix(migration): always close leader-election broadcastChannel#8780
pubkey wants to merge 3 commits into
masterfrom
claude/leader-elector-channel-cleanup-g4elu8

Conversation

@pubkey

@pubkey pubkey commented Jul 13, 2026

Copy link
Copy Markdown
Owner

This PR contains:

  • A BUGFIX
  • IMPROVED TESTS

Describe the problem you have without this PR

During a schema migration, RxMigrationState.startMigration() creates a per-collection BroadcastChannel and acquires leadership through a leaderElector (src/plugins/migration-schema/rx-migration-state.ts, at the await leaderElector.awaitLeadership() line). The channel was only closed on the success path.

If the migration threw anywhere after leadership was acquired, the channel stayed open and the tab remained leader forever, blocking other tabs from ever running the migration. The leaking paths were:

  1. Errors before the inner try block. oldCollectionMeta, createStorageInstance(), getConnectedStorageInstances(), countAllDocuments(), and the initial updateStatus() all ran before the try. Any rejection propagated out with the channel still open (and startMigration is called fire-and-forget with a swallowed catch, so the failure was silent).
  2. The migration catch block. On a migration failure it closed oldStorageInstance and set status ERROR, but never touched broadcastChannel.
  3. The old-collection-meta removal loop. A non-conflict error there re-threw past the success-path close.

Worse, the onClose safety net that would release the channel on database/collection close was registered inside migrateStorage(), so on path 1 (an error before migrateStorage was ever reached) it was never registered, leaking the leader even beyond database close.

Fix

  • Wrap the migration body in try/finally so the broadcastChannel is closed on every code path (success, migration error, pre-migration error, meta-removal error). The body was extracted into a private runMigration() method to keep the diff readable.
  • Register the onClose cleanup up-front in startMigration() (right after started = true) instead of inside migrateStorage(), so it also runs when an error happens before the replication is set up. This also removes the previous duplicate registration (migrateStorage() is called once per connected storage plus once for the main collection).

BroadcastChannel.close() is idempotent, so the finally close and the existing cancel() close paths do not conflict.

Test

Added #7827 should close the broadcastChannel/leader-election when the migration errors to test/unit/migration-schema.test.ts. It runs a multiInstance migration whose migrationStrategy throws, captures the created broadcastChannel, and asserts that after the failed migration the channel is closed (isClosed === true) and released (broadcastChannel === undefined).

Verified the test fails without the fix (20s timeout, channel never closes) and passes with it. All 50 tests in migration-schema.test.ts pass; lint and check-types are clean.

Todos

  • Tests
  • Documentation
  • Typings
  • Changelog

🤖 Generated with Claude Code


Generated by Claude Code

claude added 2 commits July 13, 2026 09:28
The schema migration acquires leadership through a per-collection
BroadcastChannel/leaderElector but only closed it on the success path.
When startMigration threw (a failing migrationStrategy, an error while
setting up the storage instances, or a non-conflict error while removing
the old collection meta doc) the channel stayed open, so the tab remained
leader forever and blocked other tabs from ever running the migration.

Wrap the migration body in try/finally so the channel is closed on every
code path, and register the onClose cleanup up-front (instead of inside
migrateStorage) so it also runs when an error happens before the
replication is set up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UYki1roCApd2AaTxZ8ZJxj
The test attached the .catch handler to migratePromise() only after an
intervening `await waitUntil(...)`. When the migration rejected before the
handler was attached (timing-dependent, hit on the CI runners), the test
harness treated it as an unhandledRejection and exited with code 5.

Catch the rejection synchronously at the call site and assert that the
broadcastChannel is released (set back to undefined, which the fix only
does after close() has run). This removes the capture-while-open race and
keeps the test a valid guard for the leak.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UYki1roCApd2AaTxZ8ZJxj
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

✅ Verify Test Reproduction: Tests FAILED without the fix (expected)

This confirms the changed tests correctly reproduce the bug that the source changes fix.

This workflow runs the changed tests without the source fix to verify they reproduce the bug.

Show output
...(truncated, showing last 200 of 3677 lines)
        �[32m✓ �[39mshould have exactly one master jakviuf - ihjovgm
    basic CRUD
      �[32m✓ �[39mshould stream changes over the replication to other collections

  encryption.test.ts
    init
      �[32m✓ �[39mcreate storage
    basics
      .encryptString()
        �[32m✓ �[39mstring
      .decryptString()
        �[32m✓ �[39mstring
        �[32m✓ �[39mshould encrypt and decrypt an extremely long string
        �[32m✓ �[39mshould encrypt and decrypt an extremely long password
    RxDatabase creation
      �[32m✓ �[39mshould crash with invalid password (empty object)
      �[32m✓ �[39mBUG: should have stored the password hash when creating the database
      �[32m✓ �[39mprevent 2 instances with different passwords on same adapter
    RxCollection creation
      �[32m✓ �[39mcreate encrypted collection
    RxCollection.insert()
      �[32m✓ �[39mshould insert one encrypted value (string)
      �[32m✓ �[39mshould insert one encrypted value (object)
      �[32m✓ �[39m#5624 insert with really big encrypted string
    RxDocument.save()
      �[32m✓ �[39mshould save one encrypted value (string)
      �[32m✓ �[39mshould save one encrypted value (object)
    replication
      �[32m✓ �[39mreplication state meta should not contain a secret in cleartext
    ISSUES
      �[32m✓ �[39m#837 Recover from wrong database password
      �[32m✓ �[39m#917 Unexpected end of JSON input
      �[32m✓ �[39mshould work with encrypted fields that have maxLength in schema
      �[32m✓ �[39mshould work with encrypted object fields that have nested properties
      �[32m✓ �[39mshould correctly encrypt and decrypt nested fields with dot-notation paths and non-string types
      �[32m✓ �[39mshould throw when encrypted contains overlapping parent and child paths
      #157 Cannot sort on field(s) "XXX" when using the default index
        �[32m✓ �[39mschema example 1
        �[32m✓ �[39mschema example 2
    SECURITY
      �[32m✓ �[39mshould not expose the database password as an enumerable property
      �[32m✓ �[39mshould not leak the password in error parameters when password is too short

  rx-state.test.ts (useSchemaValidator: true)
    helper
      .nextRxStateId()
        �[32m✓ �[39mshould increment in sort order
    creation
      �[32m✓ �[39mcalling addState twice should give the same instance
    write state data
      �[32m✓ �[39mshould write without error
      �[32m✓ �[39mwrite multiple times at once
      �[32m✓ �[39mupdate nested
      �[32m✓ �[39mupdate complete state at once
      �[32m✓ �[39mdoing many writes should end up in a single persistence write to the storage
    .get()
      �[32m✓ �[39mshould get root state
      �[32m✓ �[39mshould get the updated value
      �[32m✓ �[39mshould get nested values
      �[32m✓ �[39mshould not throw on undefined values
    .get$()
      �[32m✓ �[39mshould emit the correct data
      �[32m✓ �[39mshould emit the correct data via proxy-getter a$
    .get$$()
      �[32m✓ �[39mshould get the correct object
    cleanup
      �[32m✓ �[39mshould merge the state documents data on cleanup
    multiInstance
      �[32m✓ �[39mwrite with two states at once
      �[32m✓ �[39mwrite with two states to nested at once
      �[32m✓ �[39mget changes from other state
      �[32m✓ �[39mshould have a deterministic output when 2 instances write at the same time
      �[32m✓ �[39mshould have a deterministic output when 2 instances write to different fields
      �[32m✓ �[39mshould recover the same state from disc on the other side
      �[32m✓ �[39mshould emit the correct data for all states
    issues
      �[32m✓ �[39mRxState.property$ should be stable for initial synchronous get and subsequent subscription
      �[32m✓ �[39mset() with empty path should pass the current state to the modifier
      �[32m✓ �[39mshould recover correct state from disk after full-state replacement with set('')
      �[32m✓ �[39mbad rx-state after cleanup
      �[32m✓ �[39m$ observable should not emit duplicate values on a single write
      �[32m✓ �[39mget$() should emit the current value when subscribed, not a stale value from creation time
      �[32m✓ �[39m_cleanup() should return true to signal completion
      �[32m✓ �[39mwrite queue should recover after a modifier throws

  rx-state.test.ts (useSchemaValidator: false)
    helper
      .nextRxStateId()
        �[32m✓ �[39mshould increment in sort order
    creation
      �[32m✓ �[39mcalling addState twice should give the same instance
    write state data
      �[32m✓ �[39mshould write without error
      �[32m✓ �[39mwrite multiple times at once
      �[32m✓ �[39mupdate nested
      �[32m✓ �[39mupdate complete state at once
      �[32m✓ �[39mdoing many writes should end up in a single persistence write to the storage
    .get()
      �[32m✓ �[39mshould get root state
      �[32m✓ �[39mshould get the updated value
      �[32m✓ �[39mshould get nested values
      �[32m✓ �[39mshould not throw on undefined values
    .get$()
      �[32m✓ �[39mshould emit the correct data
      �[32m✓ �[39mshould emit the correct data via proxy-getter a$
    .get$$()
      �[32m✓ �[39mshould get the correct object
    cleanup
      �[32m✓ �[39mshould merge the state documents data on cleanup
    multiInstance
      �[32m✓ �[39mwrite with two states at once
      �[32m✓ �[39mwrite with two states to nested at once
      �[32m✓ �[39mget changes from other state
      �[32m✓ �[39mshould have a deterministic output when 2 instances write at the same time
      �[32m✓ �[39mshould have a deterministic output when 2 instances write to different fields
      �[32m✓ �[39mshould recover the same state from disc on the other side
      �[32m✓ �[39mshould emit the correct data for all states
    issues
      �[32m✓ �[39mRxState.property$ should be stable for initial synchronous get and subsequent subscription
      �[32m✓ �[39mset() with empty path should pass the current state to the modifier
      �[32m✓ �[39mshould recover correct state from disk after full-state replacement with set('')
      �[32m✓ �[39mbad rx-state after cleanup
      �[32m✓ �[39m$ observable should not emit duplicate values on a single write
      �[32m✓ �[39mget$() should emit the current value when subscribed, not a stale value from creation time
      �[32m✓ �[39m_cleanup() should return true to signal completion
      �[32m✓ �[39mwrite queue should recover after a modifier throws

  migration-schema.test.ts
    .create() with migrationStrategies
      positive
        �[32m✓ �[39mok to create with strategies
        �[32m✓ �[39mcreate same collection with different schema-versions
      negative
        �[32m✓ �[39mshould throw when array
        �[32m✓ �[39mshould throw when property no number
        �[32m✓ �[39mshould throw when property no non-float-number
        �[32m✓ �[39mshould throw when property-value no function
        �[32m✓ �[39mshould report the type of the non-function strategy value
        �[32m✓ �[39mthrow when strategy missing
    getOldCollectionMeta()
      �[32m✓ �[39mshould NOT get an older version
      �[32m✓ �[39mshould get an older version
    migration basics
      .remove()
        �[32m✓ �[39mshould delete the old storage instance with all its content
      .migrate()
        �[32m✓ �[39mshould resolve finished when no docs
        �[32m✓ �[39mshould resolve finished when some docs are in the collection
        �[32m✓ �[39mshould emit status updates
        �[32m✓ �[39mshould remove the document when migration-strategy returns null
        �[32m✓ �[39mshould throw when document cannot be migrated
      .migratePromise()
        positive
          �[32m✓ �[39mshould resolve when nothing to migrate
          �[32m✓ �[39mshould resolve when migrating data
        negative
          �[32m✓ �[39mshould reject when migration fails
      writes during migration
        �[32m✓ �[39mshould block writes while a migration is running and allow them again after it finishes
        �[32m✓ �[39mshould block bulkInsert that races with a starting migration (no waitUntil)
        �[32m✓ �[39mshould block writes when a migration is needed but not yet started
        �[32m✓ �[39mshould reset migrationInProgress on migration error
    integration into collection
      run
        �[32m✓ �[39mshould auto-run on creation
        �[32m✓ �[39mshould be able to change the primary key during migration
        �[32m✓ �[39mshould auto-run on creation (async)
        �[32m✓ �[39mshould keep the _meta.lwt value of the documents
      .migrationNeeded()
        �[32m✓ �[39mreturn true if schema-version is 0
        �[32m✓ �[39mreturn false if nothing to migrate
        �[32m✓ �[39mreturn true if something to migrate
    RxDatabase.migrationStates()
      �[32m✓ �[39mshould emit the ongoing migration state
    migration and replication
      �[32m✓ �[39mshould have migrated the replication state
      �[32m✓ �[39missue #7204 schema migration failing when returning null
    issues
      �[32m✓ �[39m#7226 db.addCollections fails after it failed for a missing migration strategy
      �[32m✓ �[39mduplicate meta states found if migration is canceled in between and restarted with a new version
      �[32m✓ �[39m#7791 should delete old collection meta doc even when _rev has changed
      �[32m✓ �[39m#7786 should properly cancel internal replication after migratePromise completes
      �[31m✗ �[39m�[31m#7827 should close the broadcastChannel/leader-election when the migration errors�[39m
	Error: AsyncTestUtil.waitUntil(): reached timeout of 5000ms
	    at runLoopOnce (webpack://rxdb/./node_modules/async-test-util/dist/es/wait-until.js?:34:16)
	    at eval (webpack://rxdb/./node_modules/async-test-util/dist/es/wait-until.js?:46:18)


Chrome Headless 150.0.0.0 (Linux 0.0.0): Executed 1163 of 1343�[31m (1 FAILED)�[39m (55.697 secs / 55.167 secs)
�[31mTOTAL: 1 FAILED, 1162 SUCCESS�[39m


�[31m1) #7827 should close the broadcastChannel/leader-election when the migration errors
�[39m�[31m     migration-schema.test.ts issues
�[39m�[90m     Error: AsyncTestUtil.waitUntil(): reached timeout of 5000ms
    at runLoopOnce (webpack://rxdb/./node_modules/async-test-util/dist/es/wait-until.js?:34:16)
    at eval (webpack://rxdb/./node_modules/async-test-util/dist/es/wait-until.js?:46:18)
�[39m



View full workflow run

async-test-util waitUntil() has no default timeout and polls forever. In
the reproduction run (test executed without the source fix) the channel
never becomes undefined, so the test hung until the browser CI runner was
disconnected after ~26 minutes instead of failing cleanly.

Pass an explicit 5s timeout so a regression fails fast with a clear
"reached timeout" error. With the fix the channel is released in well
under that bound.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UYki1roCApd2AaTxZ8ZJxj
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