Fix primary lease arbitration treating all clients as zombied when LocalStorage is unavailable - #10181
Conversation
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
🦋 Changeset detectedLatest commit: a5953f3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
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. |
There was a problem hiding this comment.
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.
| 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(); |
There was a problem hiding this comment.
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();
}
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.