diff --git a/src/selfhost/backend-concurrency-model.md b/src/selfhost/backend-concurrency-model.md new file mode 100644 index 0000000000..8bc0695a1c --- /dev/null +++ b/src/selfhost/backend-concurrency-model.md @@ -0,0 +1,97 @@ +# 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) against the real shared seam, 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, runs in every CI pass) and the +`PG_TEST_URL`-gated `test/integration/selfhost-pg.test.ts` (Postgres, needs a live server). + +## The seam + +Both adapters implement one contract, `SelfHostD1Database` (`src/selfhost/backend-contracts.ts:87-89`): +`prepare` / `batch` / `exec` / `dump`, where `batch(statements)` is documented as running "a batch +atomically, one result per statement, in order" (`src/selfhost/d1-adapter.ts:75`). Every data-access call +site in loopover — the ~171 drizzle-orm repository sites plus every raw +`env.DB.prepare(sql).bind(...).all()/.first()/.run()/.batch()` call — goes through this one surface, so +its atomicity is the guarantee the whole application actually leans on. + +- **SQLite adapter** — `createD1Adapter(driver)` (`src/selfhost/d1-adapter.ts:70`) over the synchronous + `SqliteDriver` primitive (`d1-adapter.ts:20-22`); 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 already-resolved values, so there is no real preemption inside a single statement. +- **Postgres adapter** — `createPgAdapter(pool)` (`src/selfhost/pg-adapter.ts`) 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: `installation-concurrency-admission.ts` states outright that +"single-process-per-deployment is already the supported topology for the whole admission system (the +SQLite backend structurally cannot share state across processes at all)". "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-88`). Because the driver is synchronous, a `batch()` runs its `BEGIN` through its +`COMMIT`/`ROLLBACK` with no `await` in between, 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 + lose no updates")_ +- `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: "a committed batch applies every statement, + in order")_ +- A read interleaved with a batch never observes an uncommitted intermediate state — only the pre- or + post-batch value. _(test: "a read concurrent with a batch 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 pre-write value before + any write lands, losing all but one update. _(test: "concurrent non-atomic read-modify-write loses + updates")_ Callers must use a single atomic statement, a `batch()`, or a `UNIQUE`-constrained upsert — + never a bare read-then-write pair. +- **Cross-process sharing is out of scope** for this backend. `nodeSqliteDriver` itself sets no PRAGMAs; + the production open path (`src/server.ts:266`) applies + `PRAGMA journal_mode = WAL; PRAGMA foreign_keys = ON; PRAGMA busy_timeout = 5000;`, which lets a single + deployment's short serialized write windows resolve without `SQLITE_BUSY`, 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 same +client, then `COMMIT` — or `ROLLBACK` and rethrow on error — before releasing the connection back to the +pool (`pg-adapter.ts`, `async batch(statements)`). This is real cross-connection transactional isolation: +concurrent tenants run on distinct pooled connections, and each `batch()` is its own isolated transaction. + +**What is guaranteed** + +- Each `batch()` is an isolated transaction on its own connection; a failure rolls the whole batch back + without touching any other in-flight connection's work. _(test, `PG_TEST_URL`-gated: "batch() rolls back + the whole transaction on a failing statement")_ +- Distinct pooled connections give genuine parallelism across tenant sessions, unlike the SQLite backend's + single-connection topology. + +**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`/upsert constraint, or fold the + read and write into 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 `test:coverage` 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 (`test/integration/selfhost-pg.test.ts`) rather than being faked +with a scripted mock pool, which cannot exhibit real multi-connection race behavior. The shared takeaway +for callers is backend-independent: **atomicity is a property of the statement or `batch()` you write, not +something either backend adds to a read-modify-write pair for free.** diff --git a/test/integration/selfhost-pg.test.ts b/test/integration/selfhost-pg.test.ts index 794cb99e0c..a13254442c 100644 --- a/test/integration/selfhost-pg.test.ts +++ b/test/integration/selfhost-pg.test.ts @@ -220,6 +220,46 @@ suite("Postgres backend (#977) — real Postgres", () => { expect(await getGateBlockOutcome(env, "owner/repo", 43)).toMatchObject({ overridden: true }); }); + // #4942: real cross-connection Postgres concurrency guarantees, verified against the actual pooled backend + // (mirrors the SQLite side's own in-process concurrency guarantees in test/unit/selfhost-d1-concurrency.test.ts + // -- see src/selfhost/backend-concurrency-model.md for the combined write-up). A scripted mock pool cannot + // exhibit real multi-connection race behavior, so this needs the live server this whole suite is gated on. + it("GUARANTEE (#4942): batch() rolls back the whole transaction on a failing statement, on its own pooled connection", async () => { + const db = createPgAdapter(pool); + await pool.query("CREATE TABLE IF NOT EXISTS concurrency_counters (id TEXT PRIMARY KEY, value INTEGER NOT NULL)"); + await pool.query("DELETE FROM concurrency_counters"); + await db.prepare("INSERT INTO concurrency_counters (id, value) VALUES ('c', 0)").run(); + + // Second statement violates the PRIMARY KEY, so the whole batch -- on its own dedicated connection -- must + // ROLLBACK, leaving the first statement's UPDATE un-applied too. + await expect( + db.batch([ + db.prepare("UPDATE concurrency_counters SET value = 99 WHERE id = 'c'"), + db.prepare("INSERT INTO concurrency_counters (id, value) VALUES ('c', 1)"), // duplicate PK -> throws + ]), + ).rejects.toThrow(); + + const row = await db.prepare("SELECT value FROM concurrency_counters WHERE id = 'c'").first<{ value: number }>(); + expect(row?.value).toBe(0); + }); + + it("GUARANTEE (#4942): N concurrent atomic increments across distinct pooled connections lose no updates", async () => { + const db = createPgAdapter(pool); + await pool.query("CREATE TABLE IF NOT EXISTS concurrency_counters (id TEXT PRIMARY KEY, value INTEGER NOT NULL)"); + await pool.query("DELETE FROM concurrency_counters"); + await db.prepare("INSERT INTO concurrency_counters (id, value) VALUES ('n', 0)").run(); + + const N = 25; + // Each single self-contained UPDATE is atomic on whichever pooled connection runs it -- real parallelism + // across connections, unlike the SQLite backend's single-connection topology, still loses no updates. + await Promise.all( + Array.from({ length: N }, () => db.prepare("UPDATE concurrency_counters SET value = value + 1 WHERE id = 'n'").run()), + ); + + const row = await db.prepare("SELECT value FROM concurrency_counters WHERE id = 'n'").first<{ value: number }>(); + expect(row?.value).toBe(N); + }); + it("tunes github_rate_limit_observations autovacuum below Postgres's default, idempotently (#2543)", async () => { const db = createPgAdapter(pool); diff --git a/test/unit/selfhost-d1-concurrency.test.ts b/test/unit/selfhost-d1-concurrency.test.ts new file mode 100644 index 0000000000..f18d7e34ed --- /dev/null +++ b/test/unit/selfhost-d1-concurrency.test.ts @@ -0,0 +1,105 @@ +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 +// originally designed for two local processes sharing one SQLite file; #7175 migrated that layer onto the +// shared SelfHostD1Database seam (src/selfhost/backend-contracts.ts, #4010), 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, never real OS-level multi-connection contention (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; raw: DatabaseSync } { + // The production open path (src/server.ts:266) sets these exact PRAGMAs; matching them here keeps the seam + // under test aligned with the deployed configuration. An in-memory db is a single connection -- the SQLite + // backend's real topology (single process, one file/connection) -- so "concurrency" here is event-loop + // interleaving of the async D1 surface, not OS-level multi-connection contention. + const raw = new DatabaseSync(":memory:"); + raw.exec("PRAGMA journal_mode = WAL; PRAGMA foreign_keys = ON; PRAGMA busy_timeout = 5000;"); + return { d1: createD1Adapter(nodeSqliteDriver(raw as never)), raw }; +} + +async function readCounter(d1: D1Database): Promise { + return (await d1.prepare("SELECT value FROM counters WHERE id = 'c'").first("value")) ?? -1; +} + +let d1: D1Database; +let raw: DatabaseSync; + +beforeEach(async () => { + ({ d1, raw } = 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(() => { + raw.close(); // release the SQLite handle so nothing is left 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 is a single statement on the synchronous driver -- it runs to completion + // before the next call resumes, so every increment is applied; none can interleave mid-statement. + 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: concurrent non-atomic read-modify-write loses updates -- the documented hazard, not a bug", async () => { + const N = 50; + // Splitting the increment into an awaited read then an awaited write lets every one of the N sequences + // observe the same pre-write value before any write lands, so all but one update is lost. This is + // deterministic here (every read resolves before the first write, since the read is issued synchronously + // at the top of each async callback) -- the exact reason callers must use a single atomic statement or a + // batch(), never a bare read-then-write pair, on ANY backend. + 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: a failing statement rolls back the whole batch (no partial write)", async () => { + // The second statement violates the PRIMARY KEY, so the whole batch must ROLLBACK, leaving the first + // statement's UPDATE un-applied too. + 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, in the order given + }); + + it("GUARANTEE: a read concurrent with a batch never observes a rolled-back intermediate state", async () => { + // The batch runs BEGIN..COMMIT/ROLLBACK synchronously with no await in between (the driver is sync), so an + // interleaved read can only ever 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" as const); + const observedDuring = await readCounter(d1); + await failing; + expect(observedDuring).toBe(0); // never the uncommitted 77 + expect(await readCounter(d1)).toBe(0); // rolled back cleanly + }); +});