|
| 1 | +# AMS storage-abstraction research — replacing the `node:sqlite` local-store for a hosted, tenant-scoped datastore |
| 2 | + |
| 3 | +Research spike for **#5216**. AMS's local stores (`lib/local-store.js` and its siblings) use `node:sqlite`'s |
| 4 | +`DatabaseSync` with one file per machine and no tenant concept; a hosted AMS needs a shared, tenant-scoped |
| 5 | +datastore. This document compares realistic replacement backends **against AMS's existing read/write patterns** |
| 6 | +— not a green-field storage design — so the follow-up (maintainer-owned) storage-abstraction design issue can |
| 7 | +reuse an established adapter pattern rather than invent one. **Research and writeup only — no schema, storage |
| 8 | +code, or `local-store.js` shape changes here; the recommendation is non-binding.** It intentionally does not |
| 9 | +contradict the AMS Cloud Readiness architecture / reuse-boundary spec; where that spec later lands, it governs. |
| 10 | + |
| 11 | +## Summary |
| 12 | + |
| 13 | +**Recommendation (non-binding): adopt ORB's existing `SqliteDriver` adapter seam (`src/selfhost/d1-adapter.ts` |
| 14 | ++ `src/selfhost/pg-adapter.ts`) rather than inventing a new one, and lead with managed Postgres for the hosted |
| 15 | +tenant datastore, keeping Cloudflare D1 as the edge-native alternative.** ORB already abstracts *D1 or Postgres* |
| 16 | +behind one async, D1-shaped interface (`prepare().bind().all()/.first()/.run()/.batch()`), with a |
| 17 | +SQLite→Postgres SQL translator (`pg-dialect`), so AMS does not need a bespoke abstraction. |
| 18 | + |
| 19 | +Two properties of AMS's current access patterns dominate the decision: |
| 20 | + |
| 21 | +1. **AMS uses interactive, read-then-conditional-write transactions** — `portfolio-queue.js`'s `batchClaim` |
| 22 | + (`BEGIN IMMEDIATE` → read active rows → per-row conditional claim → `COMMIT`, `lib/portfolio-queue.js:334-351`) |
| 23 | + and `attempt-log.js:145-165`. **Postgres supports this natively** (a pinned `PoolClient`, real transactions — |
| 24 | + `pg-adapter.ts:56` `runOn(client)`); **D1 does not** — it offers only an atomic `batch()` of *predetermined* |
| 25 | + statements, so the read result cannot drive the writes inside one transaction. This is the single |
| 26 | + backend-specific rework cost, and it is **larger for D1 than for Postgres**. |
| 27 | +2. **The `DatabaseSync` API is synchronous; every hosted backend is async.** Converting the stores and their |
| 28 | + callers from sync `.prepare().run()/.get()/.all()` to `await` is the dominant, backend-independent migration |
| 29 | + cost. ORB's `SqliteDriver` keeps a synchronous `query()` core with async `all()/run()/first()` wrappers |
| 30 | + (`d1-adapter.ts:20-26`), which bounds the change to the store layer's own call sites. |
| 31 | + |
| 32 | +A KV / document store is a poor fit and is **not recommended**: AMS's correctness relies on relational, |
| 33 | +single-statement atomicity (conditional `UPDATE … RETURNING`, composite-key `ON CONFLICT`) that KV cannot |
| 34 | +express without app-level optimistic-concurrency scaffolding. |
| 35 | + |
| 36 | +## What the datastore must support — AMS's actual access patterns |
| 37 | + |
| 38 | +Enumerated from the stores (`lib/local-store.js`, `run-state.js`, `claim-ledger.js`, `portfolio-queue.js`, |
| 39 | +`event-ledger.js`, `governor-state.js`), each with the property a replacement must preserve: |
| 40 | + |
| 41 | +1. **Synchronous open + access (`DatabaseSync`).** `openLocalStoreDb` (`local-store.js:45-51`) does |
| 42 | + `new DatabaseSync(path)`, `mkdirSync(…, 0o700)` + `chmodSync(…, 0o600)`, `PRAGMA busy_timeout`. Every store |
| 43 | + call is synchronous. **A hosted backend is async → the store layer and its callers must become async.** |
| 44 | +2. **Race-free single-statement claims.** Claims use `INSERT … ON CONFLICT … DO UPDATE … WHERE status <> |
| 45 | + 'in_progress'` and atomic `UPDATE … WHERE rowid = (SELECT … ORDER BY priority DESC … LIMIT 1) RETURNING *` |
| 46 | + (portfolio-queue dequeue; `claim-ledger.js` `INSERT OR IGNORE` / `ON CONFLICT`). No read-then-write. **Needs |
| 47 | + single-statement conditional writes with `RETURNING`.** |
| 48 | +3. **Interactive multi-statement transactions.** `batchClaim` and `attempt-log` use `BEGIN IMMEDIATE` → read → |
| 49 | + conditional per-row writes → `COMMIT`/`ROLLBACK`. **Needs interactive transactions** (see Summary #1). |
| 50 | +4. **Append-only ledgers with a stable total order.** `event-ledger`/`claim-ledger` use |
| 51 | + `id INTEGER PRIMARY KEY AUTOINCREMENT` and sequential `seq`-based reads. **Needs a monotonic sequence and |
| 52 | + ordered range reads.** |
| 53 | +5. **In-place schema migrations.** `schema-version.js` gates migrations on `PRAGMA user_version`; PK reshapes |
| 54 | + are done by *table rebuild* ("SQLite cannot ALTER a PRIMARY KEY in place", governor-state / claim-ledger |
| 55 | + `_v2`/`_v3` rebuilds). **A shared RDBMS replaces `user_version` with a real migration model and alters keys |
| 56 | + in place; the rebuild dance disappears.** |
| 57 | +6. **A proto-tenant scope key already exists.** Every store was widened to composite keys prefixed by |
| 58 | + `api_base_url` (forge scoping, #5563): `PRIMARY KEY (api_base_url, repo_full_name, identifier)`, |
| 59 | + `UNIQUE (api_base_url, repo_full_name, issue_number)`. **This is the natural insertion point for a |
| 60 | + `tenant_id` column** — tenant scoping extends an existing composite-key pattern rather than adding a new axis. |
| 61 | +7. **Machine-local lease liveness.** Stuck in-flight rows are reclaimed by age via `leased_at` (plus PID-liveness |
| 62 | + elsewhere). **A hosted, multi-worker service must reclaim by lease *expiry time*, not by a local PID** — |
| 63 | + independent of backend, but it interacts with #3's transaction model. |
| 64 | +8. **Single-writer, file-per-machine concurrency.** `PRAGMA busy_timeout` + one file per process is the current |
| 65 | + concurrency model. **A shared datastore introduces genuine cross-worker concurrency**, which is exactly why |
| 66 | + the atomic-claim (#2) and interactive-transaction (#3) guarantees must be preserved, not weakened. |
| 67 | + |
| 68 | +## Backend options |
| 69 | + |
| 70 | +Each evaluated against the patterns above (not generic pros/cons). |
| 71 | + |
| 72 | +### Option A — Managed Postgres (via ORB's `pg-adapter.ts`) — *recommended lead* |
| 73 | + |
| 74 | +- **Interactive transactions (#3):** native — `pg-adapter.ts` runs a `batch` on a pinned `PoolClient` |
| 75 | + (`runOn(client)`, `:56-65`); real `BEGIN … COMMIT` with read-then-write inside. Best fit for `batchClaim`. |
| 76 | +- **Single-statement claims (#2):** native `INSERT … ON CONFLICT … RETURNING` and conditional `UPDATE … |
| 77 | + RETURNING`. SQLite→Postgres differences (e.g. `RETURNING *`, autoincrement→`GENERATED`/`SERIAL`, |
| 78 | + boolean/text affinity) are handled by the existing `translateSql`/`translateDdl` (`pg-dialect`). |
| 79 | +- **Append-only order (#4):** `BIGSERIAL`/identity + `ORDER BY` — direct. |
| 80 | +- **Migrations (#5):** real DDL migrations; PK reshapes in place — the `_v2/_v3` rebuild dance is retired. |
| 81 | +- **Tenant scope (#6):** add `tenant_id` to the existing composite keys; optional row-level security. |
| 82 | +- **Cost:** the async refactor (universal) + SQL-dialect coverage (mostly already in `pg-dialect`). Highest |
| 83 | + operational weight (a managed PG instance), but the strongest correctness match to AMS's claim/lease semantics. |
| 84 | + |
| 85 | +### Option B — Cloudflare D1 (via ORB's `d1-adapter.ts`) — *edge-native alternative* |
| 86 | + |
| 87 | +- **SQL compatibility:** D1 *is* SQLite, so AMS's SQL and `PRAGMA`-adjacent shapes port with the least |
| 88 | + rewriting; ORB's `SqliteDriver`/`Statement` is D1-shaped already. |
| 89 | +- **Interactive transactions (#3):** **not supported** — D1 offers atomic `batch()` of predetermined |
| 90 | + statements only. `batchClaim`'s read-then-conditional-write must be **reworked** to either a single |
| 91 | + conditional `UPDATE … RETURNING` per claim or an optimistic-concurrency loop. This is the main D1-specific cost. |
| 92 | +- **Single-statement claims (#2):** supported (`INSERT … ON CONFLICT`, `UPDATE … RETURNING`). |
| 93 | +- **Tenant scope (#6):** either a `tenant_id` column or per-tenant database; per-DB size/write limits and the |
| 94 | + lack of interactive transactions make it better for smaller/edge tenants than for the busiest. |
| 95 | +- **Cost:** async refactor (universal) + rework of the interactive-transaction claim paths (#3). Lowest SQL |
| 96 | + translation cost, highest transaction-model cost. |
| 97 | + |
| 98 | +### Option C — KV / document store (Workers KV, DynamoDB-style) — *not recommended* |
| 99 | + |
| 100 | +- **Atomicity (#2, #3):** no relational single-statement conditional writes and no general multi-key |
| 101 | + transactions; the ordered-priority dequeue and `ON CONFLICT` claim would need app-level optimistic |
| 102 | + concurrency / conditional-put scaffolding, re-implementing what the RDBMS gives for free — and risking the |
| 103 | + exact double-claim races the current `INSERT … ON CONFLICT` design was built to prevent. |
| 104 | +- **Append-only order (#4):** no server-assigned monotonic sequence; requires an external counter. |
| 105 | +- **Verdict:** a semantic mismatch with AMS's relational, atomic-claim core. Not pursued further. |
| 106 | + |
| 107 | +### Fit matrix |
| 108 | + |
| 109 | +| AMS pattern | Postgres | D1 | KV/document | |
| 110 | +|---|---|---|---| |
| 111 | +| Interactive txn `batchClaim` (#3) | native | **rework (batch-only)** | not viable | |
| 112 | +| Single-statement claim + `RETURNING` (#2) | native (via translate) | native | app-level only | |
| 113 | +| Append-only monotonic order (#4) | native | native | external counter | |
| 114 | +| In-place migrations / PK reshape (#5) | native | SQLite-style | n/a | |
| 115 | +| Tenant scope via composite key (#6) | column + optional RLS | column or per-tenant DB | key-prefix only | |
| 116 | +| SQL rewrite cost | medium (`pg-dialect`) | **lowest** | n/a | |
| 117 | +| Async refactor cost (#1) | required | required | required | |
| 118 | + |
| 119 | +## Migration cost (backend-independent + per-option) |
| 120 | + |
| 121 | +- **Universal:** `DatabaseSync` (sync) → async adapter converts every store method and caller to `await` |
| 122 | + (#1). Bounding this to the store layer via the `SqliteDriver` seam is the main lever to keep it tractable. |
| 123 | +- **Postgres-specific:** SQL-dialect coverage — largely already implemented in `pg-dialect` (`translateSql`, |
| 124 | + `translateDdl`); verify AMS-only constructs (e.g. `PRAGMA busy_timeout`, `RETURNING *`, `AUTOINCREMENT`). |
| 125 | +- **D1-specific:** rework the interactive-transaction claim paths (`batchClaim`, `attempt-log`) into D1's |
| 126 | + atomic `batch()` or a single conditional statement — the largest single behavioral change. |
| 127 | + |
| 128 | +## Tenant-scoping fit |
| 129 | + |
| 130 | +All three inherit AMS's existing `api_base_url` composite-key scoping (#5563) as the seam: add `tenant_id` |
| 131 | +alongside it. Postgres additionally offers row-level security for defense-in-depth; D1 can alternatively give |
| 132 | +each tenant its own database (strong isolation, at the cost of cross-tenant queries and per-DB limits). The |
| 133 | +lease-liveness reclaim (#7) must move from machine/PID-local to lease-expiry-by-time for any shared backend. |
| 134 | + |
| 135 | +## Consistency with ORB's adapter precedent |
| 136 | + |
| 137 | +ORB already ships the exact seam AMS should reuse: `SqliteDriver` (`d1-adapter.ts:20`) with a `Statement` that |
| 138 | +exposes `prepare/bind/all/first/run/raw/batch` and a `pg-adapter.ts` implementing the same surface over a PG |
| 139 | +pool with `translateSql`/`translateDdl`. Reusing this means AMS gets **D1-or-Postgres behind one interface** |
| 140 | +for free, and the follow-up design issue chooses the deployment target without a second abstraction. The |
| 141 | +recommendation here — Postgres-lead, D1-alternative, KV-excluded — is a non-binding input to that |
| 142 | +maintainer-owned design issue. |
0 commit comments