Ver código fonte

merge: lane/sqlite-d1-patterns

0xDarkMatter 1 mês atrás
pai
commit
9cb33d8e06

+ 16 - 0
CHANGELOG.md

@@ -24,6 +24,22 @@ feature releases live in the README "Recent Updates" section.
   `--local-upstream`). SKILL.md reference index + Common Gotchas pointer
   updated.
 
+### Changed
+- **`sqlite-ops`** — new `references/d1-production-patterns.md`: three
+  incident-derived Cloudflare D1 patterns (symptom → why → procedure) mined from
+  a production multi-tenant Worker. (1) `wrangler d1 migrations apply --remote`
+  can time out yet still apply — verify remote schema state read-only before
+  re-running; (2) `.batch()` rolls back on SQL error but a scoped
+  UPDATE/DELETE matching 0 rows is NOT an error — check `meta.changes`,
+  post-verify, compensate; treat 0 changes as 403/conflict, never success;
+  (3) read replication via the Sessions API as opt-in-to-replica — default
+  `first-primary`, replica only for allowlisted display-only GETs, bookmark
+  cookie for read-your-writes, so a misclassified route degrades to slower,
+  never staler. Cross-linked from `d1-edge.md` (batching, Sessions API, deploy
+  gate) and `migration-patterns.md`; indexed in SKILL.md with two new gotcha
+  rows; the reference-wiring test now gates the new file.
+
+
 ### Removed
 - **`fleetflow` skill extracted to its own repo** (`X:\Forge\fleetflow`) with
   full history via `git subtree split` (36+ commits) — an app with a live

+ 8 - 2
skills/sqlite-ops/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: sqlite-ops
-description: "SQLite across every host and engine - query performance, concurrency, schema, feature modules, operations. Triggers on: sqlite, slow query, EXPLAIN QUERY PLAN, query plan, SCAN vs SEARCH, covering index, index not used, rows read, rows_read, sql_duration_ms, ANALYZE, sqlite_stat1, LIKE performance, database is locked, SQLITE_BUSY, WAL, busy_timeout, STRICT tables, type affinity, foreign_keys, VACUUM, integrity_check, fts5, trigram, json_extract, D1, cloudflare d1, wrangler d1, node:sqlite, better-sqlite3, bun:sqlite, aiosqlite, libsql, turso, migration."
+description: "SQLite across every host and engine - query performance, concurrency, schema, feature modules, operations. Triggers on: sqlite, slow query, EXPLAIN QUERY PLAN, query plan, SCAN vs SEARCH, covering index, index not used, rows read, rows_read, sql_duration_ms, ANALYZE, sqlite_stat1, LIKE performance, database is locked, SQLITE_BUSY, WAL, busy_timeout, STRICT tables, type affinity, foreign_keys, VACUUM, integrity_check, fts5, trigram, json_extract, D1, cloudflare d1, wrangler d1, node:sqlite, better-sqlite3, bun:sqlite, aiosqlite, libsql, turso, migration, d1 batch, read replication, sessions api, d1 bookmark, migration timeout."
 license: MIT
 compatibility: "Guidance is engine-agnostic (SQLite 3.x semantics). Examples are labelled by host: sqlite3 CLI, Python sqlite3/aiosqlite, node:sqlite/better-sqlite3/bun:sqlite, Cloudflare D1 via wrangler, libSQL/Turso. scripts/eqp-triage.py needs Python 3.8+ (stdlib only)."
 allowed-tools: "Read Write Bash"
@@ -267,7 +267,10 @@ wrangler d1 time-travel info <db>                                      # 30-day
 ```
 
 `./references/d1-edge.md` covers those plus the rows-read economics, the verified limits
-table, the error catalogue, and import/export.
+table, the error catalogue, and import/export. For the production incident patterns —
+a timed-out `migrations apply --remote` that landed anyway, `batch()` treating a 0-row
+scoped UPDATE as success, and the opt-in-to-replica rollout shape for read replication —
+see `./references/d1-production-patterns.md`.
 
 ## Operations
 
@@ -322,6 +325,8 @@ python3 scripts/eqp-triage.py --db app.db --sql "SELECT ..." --json | jq '.data[
 | `VACUUM` to "speed things up" | Rewrites the whole file, needs 2x space, holds a lock | `PRAGMA optimize` / targeted index work |
 | Trusting one cold run | 1.5–1.7x first-run penalty is routine | Median of 10+, report the range |
 | Inlining literals to dodge a parameter cap | That is how injection happens | Chunk the work; keep bound parameters |
+| Re-running a timed-out remote migration | The apply may have landed; the error was about the response | Verify schema state read-only first — `./references/d1-production-patterns.md` |
+| Reading a committed `batch()` as per-statement success | A conditional UPDATE matching 0 rows is not an error | Check `meta.changes`; 0 on a scoped write = 403/conflict |
 
 ## Reference files
 
@@ -329,6 +334,7 @@ python3 scripts/eqp-triage.py --db app.db --sql "SELECT ..." --json | jq '.data[
 |---|---|
 | `./references/query-performance.md` | Any slow statement: EQP, index design, ANALYZE, planner defeats, measurement method |
 | `./references/d1-edge.md` | Cloudflare D1: rows-read economics, `d1 insights`, Sessions API/replication, Time Travel, limits, errors |
+| `./references/d1-production-patterns.md` | Running D1 in production: verifying a timed-out migration, `batch()` 0-row write verification, the opt-in-to-replica replication rollout |
 | `./references/concurrency-durability.md` | Locking, WAL, busy_timeout, transaction modes, checkpointing, durability |
 | `./references/schema-design.md` | Affinity, STRICT, foreign keys, generated columns, `WITHOUT ROWID`, constraints |
 | `./references/schema-patterns.md` | Ready-made table designs: state, cache, event log, queue, session, dedup |

+ 15 - 0
skills/sqlite-ops/references/d1-edge.md

@@ -365,6 +365,11 @@ const [a, b] = await env.DB.batch([
 each individually. A batch's cost is the *sum* of its statements' rows read. `d1 insights`
 sorted by `sum`/`reads` will surface these even when your own instrumentation cannot.
 
+Batching has a *correctness* trap as well as this observability one: the batch rolls back
+on a SQL error, but a conditional `UPDATE` matching 0 rows is **not** an error — see
+[`d1-production-patterns.md`](d1-production-patterns.md#batch-and-the-0-row-conditional-write)
+for the pre-check / post-verify / compensate shape.
+
 ---
 
 ## Sessions API and read replication
@@ -421,6 +426,11 @@ that is how you verify replication is doing anything.
 anyway), single-region traffic, and workloads that require the absolute latest data on every
 read.
 
+For the production rollout shape — default every request to `first-primary` and let only an
+enumerated allowlist of display-only GETs touch a replica, so a misclassified route degrades
+to slower rather than staler — see
+[`d1-production-patterns.md`](d1-production-patterns.md#read-replication-opt-in-to-replica-never-opt-out).
+
 ---
 
 ## Time Travel
@@ -577,6 +587,10 @@ An unverified-until-deploy conclusion is a legitimate deliverable: "this index i
 to cut the statement from 171 ms to ~7 ms based on a read-only proof; applying it needs a
 gated deploy" beats applying it to find out.
 
+And when the maintainer's `--remote` apply reports a timeout: the migration may have landed
+anyway — verify schema state read-only before re-running
+([`d1-production-patterns.md`](d1-production-patterns.md#migration-apply-can-time-out-yet-still-land)).
+
 ---
 
 ## libSQL and Turso
@@ -602,6 +616,7 @@ optionality, and the vast majority of application SQL never needs a vendor exten
 ## See also
 
 - [`query-performance.md`](query-performance.md) — the engine-level analysis this builds on
+- [`d1-production-patterns.md`](d1-production-patterns.md) — incident-derived procedures: timed-out migrations, `batch()` 0-row writes, the replication rollout
 - [`hosts.md`](hosts.md) — the D1 driver API alongside the other hosts
 - [`migration-patterns.md`](migration-patterns.md) — wrangler migrations and the deploy gate
 - [`feature-modules.md`](feature-modules.md) — why FTS5 on D1 is recorded as unknown

+ 247 - 0
skills/sqlite-ops/references/d1-production-patterns.md

@@ -0,0 +1,247 @@
+# D1 Production Patterns
+
+Three patterns learned running a multi-tenant production Worker on Cloudflare D1. Each is
+incident-shaped — **symptom → why → procedure** — and each closes a gap where D1's happy-path
+API reads as success while something else happened. [`d1-edge.md`](d1-edge.md) is the
+platform reference (billing, limits, meta, Sessions API mechanics); this file is what those
+mechanics do to you in production and the defensive shape that survives them.
+
+> **Sourcing note.** These patterns come from a live multi-tenant billing platform on one
+> Worker + one D1 database (2026-07/08): a migration-timeout incident, a conditional-write
+> race in the Xero push path, and a signed-off read-replication rollout (its ADR and the
+> ~100-line session module are distilled in pattern 3). The code shapes are generalised;
+> the failure modes are not hypothetical.
+
+## Contents
+
+- [Migration apply can time out yet still land](#migration-apply-can-time-out-yet-still-land)
+- [batch() and the 0-row conditional write](#batch-and-the-0-row-conditional-write)
+- [Read replication: opt-in-to-replica, never opt-out](#read-replication-opt-in-to-replica-never-opt-out)
+
+---
+
+## Migration apply can time out yet still land
+
+**Symptom.** `wrangler d1 migrations apply <db> --remote` reports a timeout or network
+error. The natural reflex — re-run it — is the trap.
+
+**Why.** The apply is an HTTP round trip to a remote engine, and the error you saw is about
+the *response*, not the *work*. The migration can execute and be recorded server-side while
+the CLI's connection dies waiting — so the client-visible outcome ("it failed") and the
+database's actual state ("it applied") disagree. A blind re-run then re-executes SQL against
+a database that already has it:
+
+- `CREATE TABLE` / `CREATE INDEX` without `IF NOT EXISTS` → the re-run fails, which at
+  least tells you the truth, confusingly.
+- Seed/backfill `INSERT`s → **duplicated data**, which tells you nothing until something
+  downstream breaks.
+- `ALTER TABLE ... ADD COLUMN` → fails with "duplicate column name" (SQLite has no
+  `IF NOT EXISTS` for column adds — this is why column-add migrations can never be made
+  fully idempotent and *must* go through the verify step).
+
+**Procedure — verify state before re-applying, read-only:**
+
+```bash
+# 1. What does wrangler think was applied? (D1 tracks applied migrations in its own table)
+wrangler d1 migrations list <db> --remote
+
+# 2. Does the schema object actually exist? Ask the database, not the CLI's last error.
+wrangler d1 execute <db> --remote --json \
+  --command "SELECT name, sql FROM sqlite_master WHERE name = 'new_table_or_index'"
+
+# 3. For a column add, inspect the table shape (pragma_* function form — see d1-edge.md)
+wrangler d1 execute <db> --remote --json \
+  --command "SELECT * FROM pragma_table_info('the_table')"
+```
+
+Decision table:
+
+| Observed | Meaning | Action |
+|---|---|---|
+| Object exists, migrations list shows it applied | Landed; only the response was lost | Nothing to do — do **not** re-run |
+| Object exists, migrations list does NOT show it | Landed but bookkeeping is behind | Reconcile deliberately (the migration's SQL must not run twice) — never a blind re-apply |
+| Object missing | Genuinely did not land | Re-apply |
+
+Two habits make the incident boring instead of dangerous:
+
+- **Write migrations idempotent-safe where the syntax allows it** — `IF NOT EXISTS` on
+  every `CREATE`, `INSERT OR IGNORE` for seeds — so an accidental double-apply is a no-op.
+  Where it doesn't (column adds), the verify-first procedure above is the whole protection.
+- **Treat "timeout" as "state unknown", never as "failed".** The same discipline as any
+  distributed write: an error after the request left the building tells you nothing about
+  what the server did.
+
+---
+
+## batch() and the 0-row conditional write
+
+**Symptom.** A scoped write inside a `batch()` — `UPDATE ... WHERE id = ? AND
+tenant_id = ?` — "succeeds": no error, batch commits, caller returns 200. But the row
+belongs to another tenant (or was already claimed, or doesn't exist), so the statement
+matched **zero rows** and changed nothing. The caller reported success for a write that
+never happened.
+
+**Why.** `batch()` wraps its statements in a transaction and rolls back on a **SQL
+error** — and a conditional `UPDATE`/`DELETE` matching 0 rows is *not* an error. It is a
+successful statement with `meta.changes === 0`. This is correct SQL semantics on every
+engine, but D1's batch framing makes it easy to read "the batch committed" as "every
+statement did what I meant". The scoping predicate that makes multi-tenant writes safe
+(`AND tenant_id = ?`) is exactly the predicate that turns an authorization failure into a
+silent no-op.
+
+**Procedure — pre-check, post-verify, compensate.** The rule in one line: **0 rows
+affected on a scoped UPDATE/DELETE means 403/404/conflict, never success.**
+
+The single-statement shape — check `meta.changes` on every conditional write:
+
+```ts
+const res = await db
+  .prepare(`UPDATE clients SET name = ?, updated_at = ? WHERE id = ? AND tenant_id = ?`)
+  .bind(name, now, clientId, tenantId)
+  .run();
+if ((res.meta?.changes ?? 0) === 0) throw NotFound('client not found');
+// 0 here means: wrong tenant, or no such row. Either way, NOT success.
+```
+
+The compare-and-set shape — a conditional claim where exactly one racer may win:
+
+```ts
+// Claim is a CAS on the ownership column, never an assumption. Two racers issue the
+// same UPDATE; the WHERE ... IS NULL lets exactly one match. The loser's statement
+// "succeeds" with 0 changes — the explicit count check is what turns that into Conflict.
+const claim = await db
+  .prepare(`UPDATE push_intents SET document_id = ?, updated_at = ?
+             WHERE id = ? AND tenant_id = ? AND document_id IS NULL`)
+  .bind(documentId, now, intentId, tenantId)
+  .run();
+if ((claim.meta?.changes ?? 0) !== 1) throw Conflict();
+```
+
+The batch shape — conditional statements inside a `batch()` need a **post-verify count
+and a compensating undo**, because the batch cannot fail itself on your behalf:
+
+```ts
+// lockStmts are conditional claims (`... WHERE owner_id IS NULL`): a racing writer
+// matches zero rows there and the batch still commits. The post-verify is the gate.
+await db.batch([docStmt, ...lineStmts, ...lockStmts]);
+
+const locked = await db
+  .prepare(`SELECT COUNT(*) AS n FROM entries WHERE tenant_id = ? AND owner_id = ?`)
+  .bind(tenantId, documentId)
+  .first<{ n: number }>();
+if ((locked?.n ?? 0) !== expectedCount) {
+  await db.batch(undoStmts);   // compensate: release partial claims, delete the doc
+  throw Conflict();            // a concurrent writer claimed some rows first
+}
+```
+
+`batch()` returns one result per statement, each with its own `meta` — so for batches
+where each statement's effect matters, walk the results and check `meta.changes`
+per-statement rather than trusting the commit. (Counting actual changes is also how you
+report an honest number for `INSERT OR IGNORE` batches: sum `meta.changes`, don't count
+statements.)
+
+When you *want* the batch to abort atomically on a precondition, invert the trick: make
+the guard statement **violate a constraint** on failure (e.g. an `INSERT` that collides
+with a `UNIQUE` index) — a real SQL error rolls the whole batch back. That is the one
+shape where "error aborts batch" works *for* you; a 0-row match never will.
+
+---
+
+## Read replication: opt-in-to-replica, never opt-out
+
+**Symptom class this prevents.** With D1 read replication enabled (see
+[`d1-edge.md`](d1-edge.md#sessions-api-and-read-replication) for the mechanics), the naive
+rollout routes *all* reads through replica-eligible sessions and then exempts the routes
+someone remembered are sensitive. Every route the exemption list misses is a **stale read
+feeding a write flow** — a balance check, a pre-push billing review, a lock check — and the
+failure only shows up as an occasional wrong decision under replica lag, which is the worst
+possible way to discover a route classification bug.
+
+**The pattern.** Invert the default so a classification mistake degrades to *slower*,
+never to *staler*:
+
+1. **Every request defaults to `first-primary`** — strongly consistent, identical to
+   pre-replication behaviour.
+2. A request may serve from a replica **only** when it is a `GET` **and** its path is
+   positively enumerated in a replica allowlist of display-only surfaces.
+3. A route missing from the allowlist — by oversight or by design — stays on the primary:
+   no latency win, no correctness risk. **A misclassification cannot corrupt data.** The
+   partition's safety is a property of the code shape, not of per-query vigilance.
+4. **Writes always hit the primary** regardless of session mode (D1 routes writes to the
+   primary itself) — so the only thing the allowlist can get wrong is letting a
+   *pre-write read* see stale data. Keep every entry display-only.
+5. **Carry the session bookmark in a cookie** so the caller's next request is bounded to
+   be at least as fresh as their own last write (read-your-writes), even when served by
+   a replica.
+
+The whole decision fits in ~100 lines, and isolating it in one module is part of the
+pattern — one place owns the safety decision, and the data layer just receives a session:
+
+```ts
+/** Display-only GET routes allowed to serve from a nearby read replica.
+ *  Two matching shapes, chosen deliberately per entry:
+ *   - Trailing '/': whole-subtree prefix. Safe ONLY because the method gate below
+ *     excludes every mutation under it anyway.
+ *   - No trailing slash: exact path only. Use this when the same mount point also
+ *     serves write-flow reads (e.g. '/api/time/dashboard' is listed, but the
+ *     billing-review reads under '/api/time/billing' must never become eligible —
+ *     a prefix would have silently swept them in).
+ *  When unsure, leave the route off — the default (primary) is always safe, just slower. */
+const REPLICA_ALLOWLIST: readonly string[] = [
+  '/api/analytics/',        // subtree: pure read-model surface
+  '/api/dashboard',         // exact: sibling routes include write-flow reads
+];
+
+function isReplicaEligible(method: string, path: string): boolean {
+  if (method !== 'GET') return false;
+  return REPLICA_ALLOWLIST.some((e) => (e.endsWith('/') ? path.startsWith(e) : path === e));
+}
+
+/** Argument for env.DB.withSession(...) for this request. */
+function pickSessionMode(method: string, path: string, bookmark: string | null): string {
+  if (!isReplicaEligible(method, path)) return 'first-primary';   // the safe default
+  // Replica-eligible: constrain to the caller's own last write if they have one.
+  return bookmark ?? 'first-unconstrained';
+}
+
+// In the request middleware: one session per request, bookmark round-tripped in a cookie.
+const bookmark = getCookie(request, 'd1_bookmark');
+const session = env.DB.withSession(pickSessionMode(request.method, url.pathname, bookmark));
+// ... handlers run against `session` instead of `env.DB` ...
+const newBookmark = session.getBookmark();
+if (newBookmark) setCookie(response, 'd1_bookmark', newBookmark);
+```
+
+Classification guidance from the production partition that shipped:
+
+| Surface | Classification | Why |
+|---|---|---|
+| Analytics, dashboards, index/list read-models | Replica-eligible | Seconds of lag is invisible on a display surface |
+| Money *display* (totals, statements) | Replica-eligible | Display-only; nothing decides on it |
+| Any read **inside a write flow** (balance/lock checks, review-before-push) | Primary-only | A stale read here feeds a mutation |
+| Read immediately after that user's own write | Covered by the bookmark | Read-your-writes without pinning the route to primary |
+
+Two operational notes:
+
+- **Local test environments have one D1 and no replicas**, so the *refactor* (threading a
+  session through the data layer) is testable locally but the *staleness behaviour* is
+  not — validate replication in a preview/staging environment, and verify routing in
+  production via `meta.served_by_primary` / `meta.served_by_region`
+  ([`d1-edge.md`](d1-edge.md#the-meta-object)).
+- Session mode only constrains the **first** query; within a session D1 guarantees
+  sequential consistency. The allowlist + bookmark pattern is about *choosing* the right
+  first-query constraint per request, cheaply and safely, for every route you have —
+  including the ones nobody thought about.
+
+---
+
+## See also
+
+- [`d1-edge.md`](d1-edge.md) — the D1 platform reference: billing, limits, `meta`,
+  Sessions API mechanics, Time Travel, error catalogue
+- [`migration-patterns.md`](migration-patterns.md) — wrangler migrations, numbering,
+  the deploy gate these apply-verification steps slot into
+- [`concurrency-durability.md`](concurrency-durability.md) — the engine-level locking
+  model behind the CAS/claim shapes
+- `cloudflare-ops` skill — Workers, bindings, wrangler configuration, deployment

+ 4 - 0
skills/sqlite-ops/references/migration-patterns.md

@@ -250,6 +250,10 @@ statement, 100 bound parameters — see [`d1-edge.md`](d1-edge.md)); a remote ap
 [Time Travel](d1-edge.md#time-travel) gives you a 30-day undo that local SQLite does not,
 so take a bookmark before applying.
 
+One more remote-apply trap: `--remote` can **report a timeout while the migration actually
+landed**, and blindly re-running is the mistake — verify the schema state read-only first
+([`d1-production-patterns.md`](d1-production-patterns.md#migration-apply-can-time-out-yet-still-land)).
+
 ```python
 # aiosqlite - same logic, awaited. Run migrations at startup, before serving.
 async def migrate(conn, directory: str) -> None:

+ 2 - 2
skills/sqlite-ops/tests/run.sh

@@ -127,8 +127,8 @@ done
 
 # ── references exist and are cited ──────────────────────────────────────────
 echo "-- references --"
-for ref in query-performance d1-edge concurrency-durability schema-design \
-           schema-patterns migration-patterns feature-modules hosts \
+for ref in query-performance d1-edge d1-production-patterns concurrency-durability \
+           schema-design schema-patterns migration-patterns feature-modules hosts \
            async-patterns operations testing; do
   if [[ -f "$R/$ref.md" ]]; then ok "reference exists: $ref.md"; else no "reference MISSING: $ref.md"; fi
   # SKILL-RESOURCE-PROTOCOL: an uncited reference is dead weight the router never finds.