Skip to content

Fix primary lease arbitration treating all clients as zombied when LocalStorage is unavailable - #10181

Open
nbransby wants to merge 1 commit into
firebase:mainfrom
GitLiveApp:fix/zombied-client-without-webstorage
Open

Fix primary lease arbitration treating all clients as zombied when LocalStorage is unavailable#10181
nbransby wants to merge 1 commit into
firebase:mainfrom
GitLiveApp:fix/zombied-client-without-webstorage

Conversation

@nbransby

Copy link
Copy Markdown

isClientZombied() used this.webStorage?.getItem(...) !== null, which evaluates to true when webStorage is null (undefined !== null). On platforms without LocalStorage, every client was therefore considered zombied, so a second persistence client would ignore a live client's primary lease and take it over instead of failing with the exclusive-access error. Treat "no WebStorage" as "no zombie markers"; stale clients still age out via their lease/metadata timestamps.

Also removes the start() debug assertion that window is non-null: window is legitimately null on LocalStorage-less platforms and every use of it is already null-guarded.

isClientZombied() used `this.webStorage?.getItem(...) !== null`, which
evaluates to true when webStorage is null (undefined !== null). On
platforms without LocalStorage, every client was therefore considered
zombied, so a second persistence client would ignore a live client's
primary lease and take it over instead of failing with the
exclusive-access error. Treat "no WebStorage" as "no zombie markers";
stale clients still age out via their lease/metadata timestamps.

Also removes the start() debug assertion that window is non-null:
window is legitimately null on LocalStorage-less platforms and every
use of it is already null-guarded.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GCwoo1meAkgcbiJKZUqyyp
@nbransby
nbransby requested review from a team as code owners July 17, 2026 14:53
@changeset-bot

changeset-bot Bot commented Jul 17, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: a5953f3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
firebase Patch
@firebase/firestore Patch
@firebase/firestore-compat Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@google-cla

google-cla Bot commented Jul 17, 2026

Copy link
Copy Markdown

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request fixes an issue on platforms without LocalStorage (such as Node.js) where clients were incorrectly treated as zombied during primary lease arbitration, allowing unauthorized takeover of the primary lease. The fix ensures that when WebStorage is unavailable, clients are not marked as zombied, and a new unit test has been added to verify this behavior. The review feedback recommends wrapping the test execution in a try-finally block to guarantee proper cleanup of the persistence clients and prevent potential resource leaks if the test fails.

Comment on lines +1574 to +1580
const db1 = newWebStorageLessPersistence('clientA');
await db1.start();
const db2 = newWebStorageLessPersistence('clientB');
await expect(db2.start()).to.eventually.be.rejectedWith(
'Failed to obtain exclusive access to the persistence layer.'
);
await db1.shutdown();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

To prevent resource leaks and ensure proper cleanup of client metadata in IndexedDB, wrap the test execution in a try...finally block. If the assertion fails or if db2.start() unexpectedly succeeds, db1 and db2 might not be shut down, leading to leaked timers or dirty database state. Since shutdown() is idempotent, calling it on both clients in a finally block is safe and ensures a clean state for subsequent tests.

    const db1 = newWebStorageLessPersistence('clientA');
    const db2 = newWebStorageLessPersistence('clientB');
    try {
      await db1.start();
      await expect(db2.start()).to.eventually.be.rejectedWith(
        'Failed to obtain exclusive access to the persistence layer.'
      );
    } finally {
      await db1.shutdown();
      await db2.shutdown();
    }

@cherylEnkidu cherylEnkidu self-assigned this Jul 17, 2026
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.

3 participants