docs(selfhost): verify + document the shared backend's concurrency model

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.**

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);
 

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<number> {
+  return (await d1.prepare("SELECT value FROM counters WHERE id = 'c'").first<number>("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
+  });
+});