From f12b42b67d7097b42cd35d980c5bbb210dd17c8c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 19:58:58 +0000 Subject: [PATCH 1/2] A store's database closes the writer a previous JS runtime left open before opening its own When the JS runtime reloads inside the same process, nitro-sqlite's native registry keeps the previous runtime's writer. nitro's open() registers a new handle under the same name without closing the old one, so a transaction the old runtime had in flight can stay open on the leaked handle and hold the write lock, and the new runtime's writes fail as "database is locked". openNitroConnection now closes a writer registered under the name by a previous runtime, the way openReader already reclaims the reader. When nothing is registered the close throws, which is swallowed. A reopen within one runtime is unchanged. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J5R4szy5V6TVSutrikAztt --- src/nitro/nitro_connection.test.ts | 46 ++++++++++++++++++++++++++++-- src/nitro/nitro_connection.ts | 17 +++++++++-- 2 files changed, 58 insertions(+), 5 deletions(-) diff --git a/src/nitro/nitro_connection.test.ts b/src/nitro/nitro_connection.test.ts index c8e79822..e231c2b5 100644 --- a/src/nitro/nitro_connection.test.ts +++ b/src/nitro/nitro_connection.test.ts @@ -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..ddbbdb23 100644 --- a/src/nitro/nitro_connection.ts +++ b/src/nitro/nitro_connection.ts @@ -141,9 +141,9 @@ function isHandleInUse(error: unknown): boolean { * * {@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. + * 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); From 24864b7370b9645b9cd1f885e39fc2b535c48a31 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 28 Oct 2026 20:24:47 +0000 Subject: [PATCH 2/2] Describe reload handling without app-specific details Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01J5R4szy5V6TVSutrikAztt --- src/nitro/nitro_connection.test.ts | 2 +- src/nitro/nitro_connection.ts | 2 +- src/tests/superseded_connection.test.ts | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/nitro/nitro_connection.test.ts b/src/nitro/nitro_connection.test.ts index e231c2b5..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', () => { diff --git a/src/nitro/nitro_connection.ts b/src/nitro/nitro_connection.ts index ddbbdb23..c74d8a8b 100644 --- a/src/nitro/nitro_connection.ts +++ b/src/nitro/nitro_connection.ts @@ -140,7 +140,7 @@ 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 + * 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. 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(() => {