From 696b83fb10afabee9fe47b3d78f1e238ed61d008 Mon Sep 17 00:00:00 2001 From: le king fu Date: Thu, 8 Oct 2026 21:05:14 -0400 Subject: [PATCH] fix(onboarding): skip the D9 transaction once its flag is set `initializeGuidedPath` opened BEGIN, read the flag, then COMMIT at every mount of the provider: start-up, and every profile switch, creation or deletion. While a `withTransaction` callback runs, `db.ts` lets every other query bypass its lock, the overlap its serialization exists to prevent. The flag is now read first, outside any transaction, through the queued connection: once D9 ran, an opening costs that one read. Without it, the transaction re-reads the flag before writing, so two first loads still cannot apply the rule twice (CWE-362). Pinned: no transaction and no write with the flag set; the flag counts only for "1", on both reads; a first load that sets the flag between the two reads wins; the in-transaction reads still follow BEGIN. Review of #396 (PR #402). Co-Authored-By: Claude Opus 5.5 --- src/services/onboardingService.test.ts | 60 +++++++++++++++++++++++--- src/services/onboardingService.ts | 17 ++++++-- 2 files changed, 68 insertions(+), 9 deletions(-) diff --git a/src/services/onboardingService.test.ts b/src/services/onboardingService.test.ts index 104be6b..e6a561d 100644 --- a/src/services/onboardingService.test.ts +++ b/src/services/onboardingService.test.ts @@ -260,9 +260,19 @@ describe("initializeGuidedPath — rule D9 (#396)", () => { ["INSERT", ONBOARDING_GUIDED_INITIALIZED_KEY], ["COMMIT", undefined], ]); - // The flag and the transaction count are read after BEGIN. - const firstSelect = fake.db.select.mock.invocationCallOrder[0]; - expect(firstSelect).toBeGreaterThan(fake.db.execute.mock.invocationCallOrder[0]); + // The flag is read once outside any transaction, then again after BEGIN, + // with the transaction count. + const begin = fake.db.execute.mock.invocationCallOrder[0]; + const reads = fake.db.select.mock.calls.map(([sql], i) => ({ + sql: String(sql), + order: fake.db.select.mock.invocationCallOrder[i], + })); + const flagReads = reads.filter((r) => r.sql.includes("FROM user_preferences WHERE key = $1")); + expect(flagReads).toHaveLength(2); + expect(flagReads[0].order).toBeLessThan(begin); + expect(flagReads[1].order).toBeGreaterThan(begin); + const count = reads.find((r) => r.sql.includes("COUNT(*) AS transactions FROM transactions"))!; + expect(count.order).toBeGreaterThan(begin); expect(fake.prefs()).toEqual({ [ONBOARDING_CHECKLIST_HIDDEN_KEY]: "1", [ONBOARDING_GUIDED_INITIALIZED_KEY]: "1", @@ -278,17 +288,57 @@ describe("initializeGuidedPath — rule D9 (#396)", () => { expect(logInfo).not.toHaveBeenCalled(); }); - it("with the flag already set: no write at all, even with transactions", async () => { + it("with the flag already set: no transaction and no write, even with transactions", async () => { const fake = useFakeDb({ transactions: 3, prefs: { [ONBOARDING_GUIDED_INITIALIZED_KEY]: "1" }, }); await expect(initializeGuidedPath()).resolves.toEqual({ hidden: false }); - expect(fake.statements).toEqual(["BEGIN", "COMMIT"]); + expect(withTransaction).not.toHaveBeenCalled(); + expect(fake.statements).toEqual([]); + expect(fake.db.select).toHaveBeenCalledTimes(1); expect(fake.prefs()).toEqual({ [ONBOARDING_GUIDED_INITIALIZED_KEY]: "1" }); expect(logInfo).not.toHaveBeenCalled(); }); + it('counts the flag as set only for the value "1", outside and inside the transaction', async () => { + for (const value of ["0", "true", ""]) { + vi.mocked(withTransaction).mockClear(); + const fake = useFakeDb({ + transactions: 3, + prefs: { [ONBOARDING_GUIDED_INITIALIZED_KEY]: value }, + }); + await expect(initializeGuidedPath()).resolves.toEqual({ hidden: true }); + expect(withTransaction).toHaveBeenCalledTimes(1); + expect(fake.statements).toEqual([ + "BEGIN", + "INSERT INTO user_preferences", + "INSERT INTO user_preferences", + "COMMIT", + ]); + expect(fake.prefs()[ONBOARDING_GUIDED_INITIALIZED_KEY]).toBe("1"); + } + }); + + it("re-reads the flag inside the transaction: a first load that set it in between wins (CWE-362)", async () => { + const fake = useFakeDb({ transactions: 3 }); + const read = fake.db.select.getMockImplementation()!; + let flagReads = 0; + fake.db.select.mockImplementation(async (sql: string, params?: unknown[]) => { + const rows = await read(sql, params); + if (params?.[0] === ONBOARDING_GUIDED_INITIALIZED_KEY && ++flagReads === 1) { + // Another opening runs D9 to its end right after this read. + fake.prefs()[ONBOARDING_GUIDED_INITIALIZED_KEY] = "1"; + } + return rows; + }); + await expect(initializeGuidedPath()).resolves.toEqual({ hidden: false }); + expect(flagReads).toBe(2); + expect(fake.statements).toEqual(["BEGIN", "COMMIT"]); + expect(fake.prefs()[ONBOARDING_CHECKLIST_HIDDEN_KEY]).toBeUndefined(); + expect(logInfo).not.toHaveBeenCalled(); + }); + it("a profile shown again after D9 is not hidden a second time", async () => { // D9 hid it, the user showed it again from Settings → Users. const fake = useFakeDb({ transactions: 3 }); diff --git a/src/services/onboardingService.ts b/src/services/onboardingService.ts index bdd78cc..3a0fc29 100644 --- a/src/services/onboardingService.ts +++ b/src/services/onboardingService.ts @@ -123,15 +123,24 @@ export async function getOnboardingSnapshot(): Promise { * Rule D9, once per profile: at the first opening under this version, a * profile that already has transactions gets the path hidden (same preference * as "Hide", so Settings → Users shows it again); an empty or new profile sees - * it. Runs as one transaction under the DB lock, flag re-read inside it, so two - * concurrent first loads cannot apply it twice (CWE-362): the hidden preference - * is written first, then the flag, then COMMIT; any failure rolls both back and - * leaves the flag unset, so the next opening tries again. + * it. + * + * The flag is read first, outside any transaction: once D9 ran, every later + * opening costs that one queued read and opens no transaction (while a + * `withTransaction` callback runs, `db.ts` lets every other query bypass its + * lock). Without the flag, D9 runs as one transaction under the DB lock, flag + * re-read inside it, so two concurrent first loads cannot apply it twice + * (CWE-362): the hidden preference is written first, then the flag, then + * COMMIT; any failure rolls both back and leaves the flag unset, so the next + * opening tries again. Only the value "1" counts as set, on both reads. * * Returns `{ hidden: true }` only when this call hid the path, which it also * writes to the in-app log. Not announced on the data channel. */ export async function initializeGuidedPath(): Promise<{ hidden: boolean }> { + if ((await getPreference(ONBOARDING_GUIDED_INITIALIZED_KEY)) === "1") { + return { hidden: false }; + } const hidden = await withTransaction(async (db) => { await db.execute("BEGIN"); try {