Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 45 additions & 3 deletions src/nitro/nitro_connection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -180,15 +180,17 @@ 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 });

expect(conn.reader).toBeDefined();
expect(String(mockOpenSecondary.mock.lastCall?.[0].handle)).toMatch(/^things:reader:.+/);
expect(captureException).not.toHaveBeenCalled();
mockNativeClose.mockImplementation(() => undefined);
});
});

Expand Down Expand Up @@ -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);
Expand Down
19 changes: 15 additions & 4 deletions src/nitro/nitro_connection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -171,6 +171,15 @@ function openReader(name: string): ReturnType<typeof openSecondary> | 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 {
/**
Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions src/tests/superseded_connection.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ const flush = () => new Promise<void>((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(() => {
Expand Down
Loading