Skip to content

Commit eb99805

Browse files
committed
WORKSPACES-INIT (factory iter 2): clear _initPromise in try/finally so cache-reset triggers a real reload
bg/workspaces.js _init() guarded the cold-start race via _initPromise but only cleared it on FAILURE (in the .catch handler). After a successful init, _initPromise retained a resolved-promise reference forever. Two concrete issues: 1. Memory: one orphan resolved-promise per WorkspaceManager instance across the SW lifetime. Cosmetic. 2. Functional: a subsequent `this._cache = null` (factory reset, test isolation) intends to force a fresh load on the next _init. Pre-fix, the next _init hit `if (!this._initPromise) ...`, found the stale resolved promise still cached, awaited it (no-op since it was already settled), and returned WITHOUT re-reading from chrome.storage.local. The cache stayed null — subsequent reads crashed via `this._cache!.list`. Fix: mirror the modules/storage.js init pattern — wrap the await in try/finally that clears _initPromise on both success and failure. async _init() { if (this._cache !== null) return; if (!this._initPromise) { this._initPromise = (async () => { ... })(); } try { return await this._initPromise; } finally { this._initPromise = null; } } 3 new regression cases in tests/workspaces.test.js: - clears _initPromise after successful init - clears _initPromise after failed init - null-ing _cache forces a real storage reload on the next _init (would have failed on the pre-fix implementation) 45 test files, 769 cases green, tsc strict clean.
1 parent f9dc4bd commit eb99805

2 files changed

Lines changed: 48 additions & 6 deletions

File tree

bg/workspaces.js

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,19 +8,25 @@ const WorkspaceManager = {
88
async _init() {
99
if (this._cache !== null) return;
1010
// Serialize concurrent cold-start callers so a late-resolving get()
11-
// can't clobber mutations already applied to _cache by an earlier caller.
11+
// can't clobber mutations already applied to _cache by an earlier
12+
// caller. WORKSPACES-INIT — clear _initPromise in a try/finally on
13+
// BOTH success and failure (mirrors modules/storage.js init pattern).
14+
// Without the success-side clear, a test or factory-reset that nulls
15+
// _cache to force a reload would see the stale resolved promise and
16+
// no-op without re-loading from storage.
1217
if (!this._initPromise) {
1318
this._initPromise = (async () => {
1419
const data = await chrome.storage.local.get('workspaces');
1520
if (this._cache === null) {
1621
this._cache = data.workspaces || { active: null, list: [] };
1722
}
18-
})().catch(e => {
19-
this._initPromise = null;
20-
throw e;
21-
});
23+
})();
24+
}
25+
try {
26+
return await this._initPromise;
27+
} finally {
28+
this._initPromise = null;
2229
}
23-
return this._initPromise;
2430
},
2531

2632
async _save() {

tests/workspaces.test.js

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -188,4 +188,40 @@ describe('WorkspaceManager', () => {
188188

189189
expect((await WorkspaceManager.getAll()).list[0].snapshot.s1).toBe(true);
190190
});
191+
192+
// WORKSPACES-INIT — _initPromise must clear on BOTH success and failure
193+
// so a subsequent _cache null-out (factory reset, test isolation) gets a
194+
// fresh storage load. Pre-fix the resolved promise stuck around forever,
195+
// and any caller after `_cache = null` saw the stale promise and no-op'd.
196+
describe('WORKSPACES-INIT — _initPromise lifecycle', () => {
197+
it('clears _initPromise after successful init', async () => {
198+
await WorkspaceManager.getAll(); // triggers _init
199+
expect(WorkspaceManager._initPromise).toBeNull();
200+
});
201+
202+
it('clears _initPromise after failed init', async () => {
203+
chrome.storage.local.get.mockRejectedValueOnce(new Error('STORAGE_FAIL'));
204+
await expect(WorkspaceManager.getAll()).rejects.toThrow('STORAGE_FAIL');
205+
expect(WorkspaceManager._initPromise).toBeNull();
206+
});
207+
208+
it('null-ing _cache forces a real storage reload on the next _init', async () => {
209+
// Seed storage; cache populates from it.
210+
chrome.storage.local.set({ workspaces: { active: null, list: [{ id: 'ws1', name: 'First' }] } });
211+
const r1 = await WorkspaceManager.getAll();
212+
expect(r1.list).toHaveLength(1);
213+
expect(r1.list[0].name).toBe('First');
214+
215+
// Mutate storage out-of-band, null the cache to force a reload.
216+
chrome.storage.local.set({ workspaces: { active: null, list: [{ id: 'ws2', name: 'Second' }] } });
217+
WorkspaceManager._cache = null;
218+
219+
// Pre-fix: this would return the OLD cached value because
220+
// _initPromise still pointed at a resolved (now stale) promise
221+
// and short-circuited the storage re-read.
222+
const r2 = await WorkspaceManager.getAll();
223+
expect(r2.list).toHaveLength(1);
224+
expect(r2.list[0].name).toBe('Second');
225+
});
226+
});
191227
});

0 commit comments

Comments
 (0)