diff --git a/src/nitro/nitro_connection.test.ts b/src/nitro/nitro_connection.test.ts index c8e79822..02bcff20 100644 --- a/src/nitro/nitro_connection.test.ts +++ b/src/nitro/nitro_connection.test.ts @@ -154,7 +154,7 @@ describe('openNitroConnection — the dedicated read handle', () => { expect(lastReport().context).toContain('a reopen and a refetch'); }); - // An iOS CodePush reload replaces the JS runtime inside the running process, so `openConnections` comes back empty + // A JS reload replaces the JS runtime inside the running process, so `openConnections` comes back empty // while nitro still holds every handle the previous runtime opened. The writer re-registers by name; the reader is // the one whose name is exclusive, and losing it is what puts the store's whole working set back on the JS heap. it('reclaims a reader handle a previous JS runtime left open, rather than giving it up', () => { @@ -180,8 +180,9 @@ describe('openNitroConnection — the dedicated read handle', () => { throw handleInUse(); }); mockOpenSecondary.mockReturnValue(reader as never); - mockNativeClose.mockImplementationOnce(() => { - throw new Error('not ours to close'); + // Keyed by name: the stale writer's close, which runs first when this runtime does not hold the name, goes through. + mockNativeClose.mockImplementation((handle: string) => { + if (handle === 'things:reader') throw new Error('not ours to close'); }); const conn = openNitroConnection('things', { dedicatedReader: true }); @@ -189,6 +190,7 @@ describe('openNitroConnection — the dedicated read handle', () => { expect(conn.reader).toBeDefined(); expect(String(mockOpenSecondary.mock.lastCall?.[0].handle)).toMatch(/^things:reader:.+/); expect(captureException).not.toHaveBeenCalled(); + mockNativeClose.mockImplementation(() => undefined); }); }); @@ -327,6 +329,46 @@ describe('binding a store', () => { * untidy: they are what makes the *next* attempt — a retry, or a Fast Refresh re-running the same init — report a * handle collision on top of the failure that actually happened. */ +// The writer half of the same reload. nitro's `open` registers over a name it already holds without closing the old +// handle, and every statement of a batch looks its handle up by name, so a batch the old runtime had in flight would +// finish on the new handle and leave its `BEGIN EXCLUSIVE` open on the leaked one: every write after it fails as +// `database is locked` until the app is killed. +describe('openNitroConnection — the writer a previous JS runtime left open', () => { + it('closes it by name before opening its own, so it cannot keep the lock', () => { + mockOpen.mockReturnValue(fakeHandle() as never); + + openNitroConnection('reloaded.db'); + + expect(mockNativeClose).toHaveBeenCalledWith('reloaded.db'); + expect(mockNativeClose.mock.invocationCallOrder[0]).toBeLessThan(mockOpen.mock.invocationCallOrder[0]); + }); + + it('opens as usual when nothing is registered under the name, which is a cold start', () => { + mockOpen.mockReturnValue(fakeHandle() as never); + mockNativeClose.mockImplementationOnce(() => { + throw new Error('Database reloaded2.db is not open'); + }); + + const conn = openNitroConnection('reloaded2.db'); + + expect(conn).toBeDefined(); + expect(captureException).not.toHaveBeenCalled(); + }); + + it('leaves the close to its own handles when this runtime already holds the name, as on a reopen', () => { + const first = fakeHandle(); + mockOpen.mockReturnValue(first as never); + openNitroConnection('held.db'); + mockNativeClose.mockClear(); + + mockOpen.mockReturnValue(fakeHandle() as never); + openNitroConnection('held.db'); + + expect(first.close).toHaveBeenCalledTimes(1); + expect(mockNativeClose).not.toHaveBeenCalledWith('held.db'); + }); +}); + describe('binding a store — the handles a failure opened', () => { const failingBind = (name: string, writer: FakeHandle, reader: FakeHandle) => { mockOpen.mockReturnValue(writer as never); diff --git a/src/nitro/nitro_connection.ts b/src/nitro/nitro_connection.ts index 89c50fa1..c74d8a8b 100644 --- a/src/nitro/nitro_connection.ts +++ b/src/nitro/nitro_connection.ts @@ -140,10 +140,10 @@ function isHandleInUse(error: unknown): boolean { * Opens `name`'s dedicated reader, reclaiming the handle if a previous JS runtime left it open. * * {@linkcode closeNitroConnection} can only hand back handles this module's own map knows about, and that map lives in - * the JS heap. An iOS CodePush reload replaces the JS runtime in the same native process, so the new runtime starts - * with an empty map while nitro's registry still holds every handle the old one opened. The writer survives that, - * because `open` addresses a database by name and re-registers it; a secondary handle's name is exclusive, so the - * reader is the one that collides. + * the JS heap. A JS reload replaces the JS runtime in the same native process, so the new runtime starts + * with an empty map while nitro's registry still holds every handle the old one opened. `open` registers a writer over + * the old one without complaint, which leaks it ({@linkcode reclaimStaleWriter} closes it first); a secondary handle's + * name is exclusive, so the reader is the one that collides. * * Losing it is not the small thing it reads as. {@linkcode readRows} falls back to the writer handle, so the ranker's * multi statement `TEMP` work starts interleaving with an ingest's savepoint on one connection, SQLite refuses the @@ -171,6 +171,15 @@ function openReader(name: string): ReturnType | undefined return openSecondary({ name, handle: `${preferred}:${Date.now().toString(36)}` }); } +/** A JS reload leaves the previous runtime's writer registered natively; close it before opening our own. */ +function reclaimStaleWriter(name: string): void { + try { + NitroSQLite.native.close(name); + } catch { + /* nothing registered under the name: a cold start */ + } +} + /** Options for {@linkcode openNitroConnection}. */ export interface NitroConnectionOptions { /** @@ -193,7 +202,9 @@ export function openNitroConnection(name: string, opts?: NitroConnectionOptions) // Reopening a database this process already holds — a Fast Refresh re-running init, or a store rebound after a // schema change — has to hand the previous handles back first. Registering over them would leak them, and because a // secondary handle's name is exclusive, the reader is the one that would not come back. + const heldByThisRuntime = openConnections.has(name); closeNitroConnection(name); + if (!heldByThisRuntime) reclaimStaleWriter(name); const writer = open({ name }); applyPragmas(writer, name); diff --git a/src/tests/superseded_connection.test.ts b/src/tests/superseded_connection.test.ts index 1e631d66..7b423507 100644 --- a/src/tests/superseded_connection.test.ts +++ b/src/tests/superseded_connection.test.ts @@ -49,8 +49,8 @@ const flush = () => new Promise((resolve) => queueMicrotask(resolve)); /** * A store on its in-memory fallback that a retry binds back onto its file still has the fallback's writes in flight. - * Each resumes on the fallback's connection, and in the app on whatever nitro now holds under that name — such as a - * fresh in-memory database with none of its `TEMP` tables (SLEEPER-PROD-84T1, SLEEPER-PROD-84GX). + * Each resumes on the fallback's connection, and on a device on whatever nitro now holds under that name — such as a + * fresh in-memory database with none of its `TEMP` tables. */ describe('defineSqliteStore — a failure on a connection the store has moved off of', () => { beforeEach(() => {