diff --git a/src/selfhost/backend-concurrency-model.md b/src/selfhost/backend-concurrency-model.md new file mode 100644 index 0000000000..9bd1732f71 --- /dev/null +++ b/src/selfhost/backend-concurrency-model.md @@ -0,0 +1,90 @@ +# Shared backend concurrency model — verification & design doc (#4942) + +The AMS local-store concurrency guarantees were originally designed for **two local processes sharing one +SQLite file**. #7175 migrated that layer off `node:sqlite`-directly and onto the shared +`SelfHostD1Database` seam (`src/selfhost/backend-contracts.ts`, #4010), which has two interchangeable +adapters — a SQLite one and a Postgres one. This doc records what concurrency the two adapters actually +guarantee (and what they don't), so the hosted service's assumptions are stated explicitly instead of +being inherited implicitly from the old local-file design. The claims below are pinned by +`test/unit/selfhost-d1-concurrency.test.ts` (SQLite) and the `PG_TEST_URL`-gated +`test/integration/selfhost-pg.test.ts` (Postgres). + +## The seam + +Both adapters implement one contract, `SelfHostD1Database` (`backend-contracts.ts:87-94`): +`prepare` / `batch` / `exec` / `dump`, where `batch(statements)` (`backend-contracts.ts:89`) is documented +as running "a batch atomically, one result per statement, in order" (`d1-adapter.ts:76`). Every ~171 +data-access call site in loopover goes through this one surface, so its atomicity is the guarantee the +whole application leans on. + +- **SQLite adapter** — `createD1Adapter(driver)` (`src/selfhost/d1-adapter.ts:70`) over the synchronous + `SqliteDriver` primitive (`d1-adapter.ts:20-23`); the default driver is `nodeSqliteDriver` over + `node:sqlite` (`d1-adapter.ts:116`). The D1 API is async, but the driver is **synchronous** — the async + methods only wrap resolved values, so there is no real preemption inside a single statement. +- **Postgres adapter** — `createPgAdapter(pool)` (`src/selfhost/pg-adapter.ts:62`) over a `node-postgres` + `Pool`; a real pooled, async, multi-connection client. + +## SQLite backend + +**Topology.** One process, one connection, one file. This is not incidental — it is the supported topology +for the whole admission system: "single-process-per-deployment is already the supported topology […] the +SQLite backend structurally cannot share state across processes at all" +(`installation-concurrency-admission.ts:10-16`). "Concurrency" against this backend therefore means +**event-loop interleaving of the async D1 surface within one process**, not OS-level multi-connection +contention. + +**Atomicity.** `batch()` wraps its statements in `BEGIN` / `COMMIT`, with `ROLLBACK` on any error +(`d1-adapter.ts:75-90`). Because the driver is synchronous, a `batch()` runs start-to-finish with no +`await` between `BEGIN` and `COMMIT`, so no other operation can observe a partially-applied batch. + +**What is guaranteed** + +- A single self-contained write statement (e.g. `UPDATE … SET value = value + 1`) is applied in full; N + such concurrent statements lose no updates (final value == N). _(test: "N concurrent atomic increments")_ +- `batch()` is all-or-nothing: a failing statement rolls back the entire batch, leaving no partial write. + _(test: "a failing statement rolls back the whole batch")_ +- A committed batch applies every statement, in order. _(test: "applies every statement, in order")_ +- A read interleaved with a batch sees only the pre- or post-batch state, never an uncommitted + intermediate. _(test: "never observes a rolled-back intermediate state")_ + +**What is NOT guaranteed** + +- **Non-atomic read-modify-write is not safe**, exactly as on any backend. Splitting an increment into an + awaited read then an awaited write lets concurrent sequences all read the same value before any write + lands, losing all but one update _(test: "concurrent non-atomic read-modify-write … DO lose updates")_. + Callers must use a single atomic statement, a `batch()`, or a `UNIQUE`-constrained upsert — not a + read-then-write pair. +- **Cross-process sharing is out of scope** for this backend. `nodeSqliteDriver` sets no PRAGMAs itself; + the production open path (`src/server.ts:266`) applies `PRAGMA journal_mode = WAL; PRAGMA busy_timeout = +5000;`, which lets a single deployment's short serialized write windows resolve without `SQLITE_BUSY` + (`src/selfhost/sqlite-queue.ts:142`), but multi-writer cross-process durability is a Postgres concern, + not a SQLite one. + +## Postgres backend + +`batch()` acquires a dedicated pooled connection, runs `BEGIN`, executes each statement on that client, +then `COMMIT` — or `ROLLBACK` and rethrow on error — before releasing the connection +(`pg-adapter.ts:65-83`). This is real cross-connection transactional isolation: concurrent tenants run on +distinct pooled connections, and each `batch()` is an isolated transaction. + +**What is guaranteed** + +- Each `batch()` is an isolated transaction; a failure rolls the whole batch back on its connection. _(the + `PG_TEST_URL`-gated batch-rollback test in `test/integration/selfhost-pg.test.ts`)_ +- Distinct connections give genuine parallelism across tenant sessions. + +**What is NOT guaranteed** + +- Application-level lost-update protection for a read-then-write spanning two separate statements — the + same rule as SQLite. Use row locking (`SELECT … FOR UPDATE`), a `UNIQUE` constraint, or a single atomic + statement inside the batch. + +## Why the tests are split this way + +The SQLite guarantees are verified deterministically **in-process** (the backend's real topology), so they +run in the standard `validate-tests` suite with no external dependency and no flakiness. Real +multi-connection Postgres concurrency needs a live server, so it stays behind the existing +`PG_TEST_URL`-gated integration suite rather than being faked with a scripted mock pool (which cannot +exhibit real race behavior). The shared takeaway for callers is backend-independent: **atomicity is a +property of the statement/`batch()` you write, not something either backend adds to a read-modify-write +pair for free.** diff --git a/test/unit/selfhost-d1-concurrency.test.ts b/test/unit/selfhost-d1-concurrency.test.ts new file mode 100644 index 0000000000..769f98ebdd --- /dev/null +++ b/test/unit/selfhost-d1-concurrency.test.ts @@ -0,0 +1,121 @@ +import { DatabaseSync } from "node:sqlite"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + createD1Adapter, + nodeSqliteDriver, +} from "../../src/selfhost/d1-adapter"; + +// Concurrency-model verification for the shared SQLite backend (#4942). The AMS local-store guarantees were +// designed for two local processes sharing one file; #7175 migrated that layer onto the shared +// pg-adapter/SqliteDriver seam, so the guarantees the hosted service now actually relies on need to be +// verified against the real seam and documented, not assumed to still hold implicitly. This file pins down +// the SQLite side's guarantees under concurrent access from the async D1 surface (the model the SQLite +// backend actually has: a single process, a synchronous driver, operations serialized on the event loop -- +// see src/selfhost/backend-concurrency-model.md). The Postgres side's real cross-connection concurrency is +// exercised by the PG_TEST_URL-gated test/integration/selfhost-pg.test.ts, since it needs a live server. + +function makeDb(): { d1: D1Database; db: DatabaseSync } { + // The production open path (src/server.ts) sets these exact PRAGMAs; use them here so the seam under test + // matches the deployed configuration. An in-memory db is a single connection, which is the SQLite backend's + // real topology (single process, one file/connection) -- concurrency here is event-loop interleaving of the + // async D1 surface, not OS-level multi-connection contention. + const db = new DatabaseSync(":memory:"); + db.exec( + "PRAGMA journal_mode = WAL; PRAGMA foreign_keys = ON; PRAGMA busy_timeout = 5000;", + ); + return { d1: createD1Adapter(nodeSqliteDriver(db as never)), db }; +} + +async function readCounter(d1: D1Database): Promise { + return ( + (await d1 + .prepare("SELECT value FROM counters WHERE id = 'c'") + .first("value")) ?? -1 + ); +} + +let d1: D1Database; +let rawDb: DatabaseSync; + +beforeEach(async () => { + ({ d1, db: rawDb } = makeDb()); + await d1.exec( + "CREATE TABLE counters (id TEXT PRIMARY KEY, value INTEGER NOT NULL);", + ); + await d1.prepare("INSERT INTO counters (id, value) VALUES ('c', 0)").run(); +}); + +afterEach(() => { + rawDb.close(); // release the SQLite handle so the worker leaves nothing open between tests +}); + +describe("shared SQLite backend concurrency guarantees (#4942)", () => { + it("GUARANTEE: N concurrent atomic increments lose no updates (final value == N)", async () => { + const N = 50; + // A single self-contained UPDATE acquires the write path atomically; the synchronous driver runs each to + // completion before the next resumes, so every increment is applied. + await Promise.all( + Array.from({ length: N }, () => + d1 + .prepare("UPDATE counters SET value = value + 1 WHERE id = 'c'") + .run(), + ), + ); + expect(await readCounter(d1)).toBe(N); + }); + + it("BOUNDARY: N concurrent non-atomic read-modify-write across awaits DO lose updates", async () => { + const N = 50; + // The documented hazard: splitting the increment into an awaited read then an awaited write lets every + // sequence read the same pre-write value before any write lands, so all but one update is lost. This is + // deterministic here (the read executes synchronously when first() is called, so all N observe 0), and is + // the exact reason callers must use a single atomic statement or a batch()/transaction -- not because the + // backend is "broken", but because read-modify-write is not atomic on any backend without one. + await Promise.all( + Array.from({ length: N }, async () => { + const current = await readCounter(d1); + await d1 + .prepare("UPDATE counters SET value = ? WHERE id = 'c'") + .bind(current + 1) + .run(); + }), + ); + const final = await readCounter(d1); + expect(final).toBeLessThan(N); + expect(final).toBe(1); + }); + + it("GUARANTEE: batch() is atomic -- a failing statement rolls back the whole batch (no partial write)", async () => { + // Second statement violates the PRIMARY KEY, so the batch must ROLLBACK and leave the counter untouched. + await expect( + d1.batch([ + d1.prepare("UPDATE counters SET value = 99 WHERE id = 'c'"), + d1.prepare("INSERT INTO counters (id, value) VALUES ('c', 1)"), // duplicate PK -> throws + ]), + ).rejects.toThrow(); + expect(await readCounter(d1)).toBe(0); + }); + + it("GUARANTEE: a committed batch applies every statement, in order", async () => { + await d1.batch([ + d1.prepare("UPDATE counters SET value = value + 10 WHERE id = 'c'"), + d1.prepare("UPDATE counters SET value = value * 2 WHERE id = 'c'"), + ]); + expect(await readCounter(d1)).toBe(20); // (0 + 10) * 2 + }); + + it("GUARANTEE: a read concurrent with an atomic batch never observes a rolled-back intermediate state", async () => { + // The batch runs BEGIN..COMMIT/ROLLBACK synchronously with no await inside, so an interleaved read can + // only see the pre-batch or post-batch value, never a partially-applied one. + const failing = d1 + .batch([ + d1.prepare("UPDATE counters SET value = 77 WHERE id = 'c'"), + d1.prepare("INSERT INTO counters (id, value) VALUES ('c', 2)"), // duplicate PK -> rollback + ]) + .catch(() => "rolled-back"); + const observed = await readCounter(d1); + await failing; + expect(observed).toBe(0); // never the uncommitted 77 + expect(await readCounter(d1)).toBe(0); + }); +});