From 8a35a838d0f05176007aa28894d29d24a283ce0c Mon Sep 17 00:00:00 2001 From: dimgigov Date: Thu, 30 Jul 2026 18:58:47 +0300 Subject: [PATCH] docs: SQL writes through raft (C3b) spec + plan --- .../plans/2026-07-30-raft-sql-writes.md | 223 ++++++++++++++++++ .../2026-07-30-raft-sql-writes-design.md | 109 +++++++++ 2 files changed, 332 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-30-raft-sql-writes.md create mode 100644 docs/superpowers/specs/2026-07-30-raft-sql-writes-design.md diff --git a/docs/superpowers/plans/2026-07-30-raft-sql-writes.md b/docs/superpowers/plans/2026-07-30-raft-sql-writes.md new file mode 100644 index 0000000..ffe8cff --- /dev/null +++ b/docs/superpowers/plans/2026-07-30-raft-sql-writes.md @@ -0,0 +1,223 @@ +# SQL Writes Through Raft (C3b) Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** When Raft is enabled, SQL writes commit through the Raft log before the client sees success; followers reject writes naming the leader and apply committed entries via the existing applyCommand loop. + +**Architecture:** Intercept at the server-level `executeQuery` (core/server.nim:206-227) — same hook point and same data source (`res.keyValuePairs`) the legacy ReplicationManager uses. Leader: execute locally, append one raft entry per KV pair (`"put"` = `key\x00value`, `"delete"` = `key` — the exact format baradadb.nim's applyCommand already consumes), poll `node.commitIndex` until the last appended index commits (no raft.nim changes — state machine stays TLA-frozen). Follower: reject before execution. + +**Tech Stack:** Nim 2.2.10, ARC, unittest, real server processes for E2E. + +## Global Constraints + +- Spec: `docs/superpowers/specs/2026-07-30-raft-sql-writes-design.md` (read first). +- Test command per task: `nim c -d:ssl --threads:on --path:src -o:tests/test_all tests/test_all.nim && ./tests/test_all` — exit 0. Final task: full `nimble test` (673+ `[OK]`). +- **raft.nim stays FROZEN** (state machine, handlers, message format, serialization — TLA-faithfulness tests pin them). All changes in server.nim / config.nim / exec/params.nim / executor.nim / baradadb.nim / tests. +- Non-raft deployments (`server.raftNode == nil`): byte-identical behavior — verify the raft code path is fully gated. +- No new dependencies. Commits per task after green; source files only; no push (controller merges+pushes). +- Verified facts to use: + - Server hook: `core/server.nim:206-227` — parse at :213, `executor.executeQuery` at :218, legacy replication ship at :219-227 (`replication.writeLsn(data)` with `key\0value`). `executeQuery(db, ctx, query, params, replication)` is called from handleClient (:567 plain, :587 prepared). + - Statement kinds: `nkInsert/nkUpdate/nkDelete/nkMerge` write directly; `nkCommitTxn` emits kvPairs at COMMIT (executor.nim:968-979); BEGIN/ROLLBACK are connection-local. + - appendLog (raft.nim:355): `appendLog*(node, command, data): LogEntry` — returns empty entry (index 0) when not leader; replication ships on the next heartbeat (50ms); commit advances via handleAppendReply → applyCommitted (raft.nim:195-215, 407-421). + - applyCommand format (baradadb.nim:336-343): `cmd == "put"` → data = `key \x00 value`; `cmd == "delete"` → data = `key`. + - Delete kvPair convention: non-txn DELETE emits `(fullKey, @[])` (exec/dml.nim:241). **OPEN ITEM for Task 2: txn COMMIT emits `(key, version.value)` even when `version.isDelete` (executor.nim:971-976) — check whether version.value is empty for deletes (read core/mvcc.nim writeSet/VersionedValue); if not, change the COMMIT loop to emit `(key, @[])` for deletes so the raft "empty value = delete" rule holds.** + - Config env parsing: core/config.nim:177-206 (BARADB_RAFT_* section). + +--- + +### Task 1: isWrite helper + Server.raftNode + follower rejection + +**Files:** +- Modify: `src/barabadb/query/exec/params.nim` (isWrite helper next to isDDL) +- Modify: `src/barabadb/core/server.nim` (raftNode field + rejection in executeQuery) +- Modify: `src/baradadb.nim` (assign server.raftNode when raft enabled) +- Test: `tests/bugfix_test.nim` + +**Interfaces:** +- Produces: + - `proc isWrite*(stmt: Node): bool` in exec/params.nim — true for `nkInsert, nkUpdate, nkDelete, nkMerge, nkCommitTxn` (check exact enum names in query/ast.nim; nkCommitTxn included because COMMIT emits kvPairs). + - `Server.raftNode*: RaftNode` (nil default) — server.nim imports core/raft (verify no cycle: raft.nim must not import server.nim). + - Rejection in server-level executeQuery (core/server.nim:206+): right after parse and the empty-stmts check, BEFORE `executor.executeQuery`: + ```nim + if server.raftNode != nil and isWrite(astNode.stmts[0]): + let node = server.raftNode + if node.state != rsLeader: + let who = if node.leaderId.len > 0: node.leaderId else: "none elected" + return (false, QueryResult(), "not leader; leader is '" & who & "'") + ``` + (Adjust to the proc's actual error-return convention and to how it accesses the Server — if executeQuery is not a method, thread the raft node the same way `replication` is threaded; check the handleClient call sites at :567/:587 first and pick the smaller change.) + +- [ ] **Step 1: Failing tests** + +New suite in tests/bugfix_test.nim: + +```nim +suite "Raft write classification": + test "isWrite classifies DML and COMMIT": + check isWrite(parse("INSERT INTO t (id) VALUES (1)").stmts[0]) + check isWrite(parse("UPDATE t SET id = 2").stmts[0]) + check isWrite(parse("DELETE FROM t WHERE id = 1").stmts[0]) + check isWrite(parse("COMMIT").stmts[0]) + check not isWrite(parse("SELECT * FROM t").stmts[0]) + check not isWrite(parse("CREATE TABLE t (id INT)").stmts[0]) + check not isWrite(parse("BEGIN").stmts[0]) + check not isWrite(parse("ROLLBACK").stmts[0]) +``` + +(Adapt to bugfix_test.nim's imports — it has parser already.) + +- [ ] **Step 2: Run, watch it fail** + +Expected: compile error — `isWrite` undeclared. + +- [ ] **Step 3: Implement** + +The helper, the Server field, the rejection, the baradadb.nim assignment (`tcpServer.raftNode = raftNode` — check the actual server variable name and that raftNet/raftNode are in scope; they were hoisted in the C3a fix wave). + +- [ ] **Step 4: Run, watch it pass** + +bugfix_test green; test_all green (461+ [OK]). + +- [ ] **Step 5: Commit** + +```bash +git add src/barabadb/query/exec/params.nim src/barabadb/core/server.nim src/baradadb.nim tests/bugfix_test.nim +git commit -m "feat(raft): classify writes, reject them on follower nodes" +``` + +--- + +### Task 2: Leader write path — append + wait-for-commit + +**Files:** +- Modify: `src/barabadb/core/server.nim` (raft append + commit wait, replacing the legacy ship when raft active) +- Modify: `src/barabadb/core/config.nim` (raftWriteTimeoutMs + env) +- Modify: possibly `src/barabadb/query/executor.nim` (COMMIT delete kvPair convention — see OPEN ITEM) +- Test: `tests/test_all.nim` (in-process leader path) + +**Interfaces:** +- Consumes: Task 1's isWrite + server.raftNode; appendLog; applyCommand format. +- Produces: + - In server executeQuery after `res.success` (replacing the legacy replication ship when raftNode != nil): + ```nim + if server.raftNode != nil and res.keyValuePairs.len > 0: + let node = server.raftNode + var lastIdx = 0'u64 + for (key, value) in res.keyValuePairs: + let entry = if value.len > 0: + node.appendLog("put", cast[seq[byte]](key & "\x00" & cast[string](value))) + else: + node.appendLog("delete", cast[seq[byte]](key)) + if entry.index == 0: + return (false, QueryResult(), "lost leadership during raft append") + lastIdx = entry.index + # wait for commit + let deadline = getMonoTime() + initDuration(milliseconds = config.raftWriteTimeoutMs) + while node.commitIndex < lastIdx and getMonoTime() < deadline: + await sleepAsync(10) # or the sync equivalent — check whether executeQuery is async; if it is NOT async, use os.sleep in a bounded loop + if node.commitIndex < lastIdx: + return (false, QueryResult(), "raft commit timeout") + else if replication != nil and res.keyValuePairs.len > 0: + + ``` + CRITICAL: check whether server-level executeQuery (server.nim:206) is a sync proc under withStorageGate — if sync, the wait must not block the async event loop; use short os.sleep polling and keep the timeout small, or document. Match the codebase's reality, not this sketch. + - `raftWriteTimeoutMs*: int` on BaraConfig (default 5000) + `BARADB_RAFT_WRITE_TIMEOUT_MS` env parsing in the raft section of loadConfigFromEnv. + - COMMIT delete convention resolved (OPEN ITEM above): version.value empty for deletes, or executor.nim COMMIT loop fixed to emit `(key, @[])` for isDelete entries. + +- [ ] **Step 1: Failing test** + +In tests/test_all.nim (raft suites): + +```nim + test "leader append+commit wait round-trips through applyCommand": + # 3 in-process nodes over TCP (reuse the "timerLoop elects a leader" setup from C3a Task 2) + # wait for a leader; on the leader: appendLog("put", "users.1\x00alice") + # poll leader.commitIndex until >= entry.index (deadline 3s) + # assert a follower's applyCommand got invoked with ("put", data) + # (wire applyCommand on each node to record calls — check how baradadb.nim wires it) + test "txn COMMIT delete kvPairs are empty-valued": + # embedded ctx: BEGIN; INSERT; DELETE same row; COMMIT + # assert res.keyValuePairs for the deleted key has value.len == 0 +``` + +- [ ] **Step 2: Run, watch them fail** + +Expected: first — no server-side helper exists yet (compile error or assertion); second — may already pass or fail depending on the OPEN ITEM finding; record which. + +- [ ] **Step 3: Implement** + +Per Produces. Resolve the sync/async question by reading server.nim:206-227 and its callers first; keep the wait bounded and simple. + +- [ ] **Step 4: Run, watch them pass** + +test_all green. + +- [ ] **Step 5: Commit** + +```bash +git add src/barabadb/core/server.nim src/barabadb/core/config.nim src/barabadb/query/executor.nim tests/test_all.nim +git commit -m "feat(raft): leader appends writes to raft log and waits for commit" +``` + +--- + +### Task 3: E2E — replicated writes across a real 3-node cluster + +**Files:** +- Create: `tests/raft_writes_e2e_test.nim` (or extend tests/raft_e2e_test.nim — pick the cleaner; new file preferred to keep runtimes isolated) +- Modify: `baradadb.nimble` (wire into test task) + +**Interfaces:** +- Consumes: Tasks 1-2; the E2E process harness from tests/raft_e2e_test.nim (port base, env setup, leader detection via the "became leader" log line, teardown); the Nim client via `adaptors/nim/baradb_sqlite` (pattern from tests/nimforum_smoke_test.nim: `open("127.0.0.1:" & $port, "", "", "default")`, `db.exec(sql"...")`, `db.getAllRows`). +- Produces: suite "Raft replicated writes E2E": + 1. Boot 3 nodes (raft enabled), wait for leader (log line). + 2. On the LEADER's client port: CREATE TABLE + INSERT a row (expect success). + 3. On a FOLLOWER's client port: poll `SELECT` until the row appears (deadline 5s; follower applies committed entries via applyCommand to its default DB). + 4. On a FOLLOWER: `INSERT` → expect an error containing "not leader". + 5. Kill the leader; wait for new leader; INSERT on the new leader succeeds; the row becomes visible on the remaining follower. + 6. Teardown all processes; dump captured output on any failure. + +- [ ] **Step 1: Write the test** + +Follow raft_e2e_test.nim conventions (O_NONBLOCK output drains, deadlines, skip-with-reason if build/baradadb missing, different port base from both nimforum_smoke_test and raft_e2e_test). Note: applyCommand applies to the DEFAULT database — use database "default" in the client. + +- [ ] **Step 2: Run standalone (twice)** + +`nim c -d:ssl --threads:on --path:src -o:tests/raft_writes_e2e_test tests/raft_writes_e2e_test.nim && ./tests/raft_writes_e2e_test` +Expected: PASS twice consecutively. If the follower never sees the row, debug order: raft commit wait (Task 2) → applyCommand wiring → default-DB targeting. Dump output on failure. + +- [ ] **Step 3: Wire into nimble test + full suite** + +Add to baradadb.nimble test list after raft_e2e_test; add the binary name to .gitignore next to tests/raft_e2e_test. Run `nimble test` — exit 0, 673+ `[OK]`. + +- [ ] **Step 4: Commit** + +```bash +git add tests/raft_writes_e2e_test.nim baradadb.nimble .gitignore +git commit -m "test(raft): E2E replicated writes, follower rejection, failover writes" +``` + +--- + +### Task 4: Docs + close-out + +**Files:** +- Modify: `docs/superpowers/specs/2026-07-30-raft-sql-writes-design.md` (status → done) +- Modify: README.md only if it claims raft/replication behavior that changed (check `grep -n -i 'raft\|replicat' README.md | head -20` — update only now-false lines, minimally) + +- [ ] **Step 1: Update docs** + +- [ ] **Step 2: Commit** + +```bash +git add docs/ README.md +git commit -m "docs: SQL writes through raft (C3b) done" +``` + +--- + +## Self-Review Notes + +- Spec coverage: write classification → T1; follower rejection → T1; leader append+wait → T2; config → T2; E2E → T3; docs → T4. Non-goals honored (no forwarding, no DDL replication, raft.nim frozen). +- The COMMIT delete-convention open item is the main correctness risk (a deleted key resurrected on followers) — T2's second test pins it before it can ship. +- Double application on the leader is idempotent by KV semantics; T3's follower-visibility assertions prove the real path end-to-end. diff --git a/docs/superpowers/specs/2026-07-30-raft-sql-writes-design.md b/docs/superpowers/specs/2026-07-30-raft-sql-writes-design.md new file mode 100644 index 0000000..a357f8a --- /dev/null +++ b/docs/superpowers/specs/2026-07-30-raft-sql-writes-design.md @@ -0,0 +1,109 @@ +# SQL Writes Through Raft (C3b) — Design + +Date: 2026-07-30 +Status: Approved direction (user: "ок ... продължавай"); implementation follows. + +## Problem + +With C3a a real Raft cluster elects a leader over TCP, but SQL writes still +bypass Raft entirely: they go straight to the local LSM and (optionally) to +the legacy `ReplicationManager` (unconnected in production). Committed Raft +entries can be applied (`applyCommand` is wired and invoked on commit), yet +nothing ever calls `appendLog` from the write path. + +## Goal + +When Raft is enabled, SQL writes are committed through the Raft log before +the client sees success: + +- Leader: execute locally (constraints validated as today), append the + resulting KV pairs to the Raft log, wait for majority commit, then return. +- Followers: reject write statements with a clear "not leader" error naming + the known leader; apply committed entries via the existing `applyCommand` + loop (raft.nim:195-215). +- Non-raft deployments: byte-identical behavior to today. + +Non-goals (C3c+): leader forwarding/proxy, read consistency levels, +schema/DDL replication (DDL produces no keyValuePairs today — out of scope), +membership, snapshots. + +## Design + +### Write interception point + +`core/server.nim:206-227` — the server-level `executeQuery` parses the AST +(server.nim:213) and, after successful execution, already ships +`res.keyValuePairs` to the ReplicationManager (server.nim:219-227). The Raft +path slots into the same place with the same data source: + +1. Classify the statement right after parse: write = `stmt.kind in {nkInsert, + nkUpdate, nkDelete, nkMerge}` (DDL is out of scope; SELECT unaffected). + New helper `isWrite(stmt)` in `exec/params.nim` next to `isDDL`. +2. If Raft is active for this server (`server.raftNode != nil`): + - Not leader (`node.state != rsLeader`): return error + `not leader; leader is ''` (or `no leader elected`). + The statement is NOT executed. + - Leader: execute as today. If `res.success` and + `res.keyValuePairs.len > 0`: for each `(key, value)` append one log + entry — `cmd = "put"` with data `key \x00 value`, or `cmd = "delete"` + with data `key` when value is empty (this is EXACTLY the format the + existing `applyCommand` in baradadb.nim:336-343 already consumes — no + format changes, frozen). Then wait until `node.commitIndex` reaches the + last appended index (see below) before returning `res`. + - The legacy `replication.writeLsn` hook is skipped when the Raft write + path handled the statement (no double shipping). +3. Single-node raft (enabled, no peers): majority is self — appends commit + immediately; behavior is correct with negligible overhead. + +### Wait-for-commit + +No raft.nim changes (the state machine stays TLA-frozen). Poll from +server.nim: after appending, `while node.commitIndex < lastIdx`: sleep 10ms, +up to `config.raftWriteTimeoutMs` (new env `BARADB_RAFT_WRITE_TIMEOUT_MS`, +default 5000). Commit advances on the 50ms heartbeat cadence via the +existing `handleAppendReply`/`applyCommitted` path, so typical latency is +one heartbeat. On timeout: return an error (`raft commit timeout`) — the +entry may still commit later; documented limitation. Local execution already +happened (it validates constraints and produces kvPairs) — leader double- +applies via applyCommand on commit, which is idempotent for put/delete KV. + +### Server wiring + +`Server` gains `raftNode*: RaftNode` (nil by default) in core/server.nim; +baradadb.nim assigns it when raft is enabled (the node already exists there). +server.nim imports core/raft (no cycle: raft.nim does not import server). + +### Known v1 limitations (documented in code) + +- Leader losing leadership between local execution and appendLog (narrow + race): appendLog returns the empty entry (index 0) — surfaced as a + `lost leadership` error; the local write already applied (uncommitted). +- Transactional writes: kvPairs are emitted at COMMIT (executor.nim:970-979) + — the COMMIT statement is the raft-replicated unit (classify nkCommitTxn? + — resolve in planning: COMMIT's kvPairs flow through the same hook since + the existing replication hook already catches them). +- DDL is not replicated (no kvPairs today). + +## Testing + +1. Unit (`tests/bugfix_test.nim`): `isWrite` classification for + INSERT/UPDATE/DELETE/MERGE vs SELECT/DDL. +2. Follower rejection (in-process or E2E): write on a follower returns the + "not leader" error naming the leader; the statement is not executed. +3. E2E (extend `tests/raft_e2e_test.nim` or sibling): 3 real nodes; write a + row on the leader via the Nim client (adaptors/nim/baradb_sqlite pattern + from nimforum_smoke_test); poll a SELECT on a follower until the row + appears (deadline ~5s — follower applies on commit via applyCommand); + write on a follower → rejected; kill leader → re-election → writes work + on the new leader. +4. Full `nimble test` green (673+ `[OK]`). + +## Risks + +- Double application on the leader (executor + applyCommand): idempotent + KV semantics — verify put/delete idempotency holds for the value encoding + (same key → same value; delete of existing key). +- `node.log[idx-1]` positional indexing in the leader commit loop + (raft.nim:407) — pre-existing, out of scope unless the E2E trips it. +- Apply lag on followers (heartbeat cadence): tests poll with deadlines, + never fixed sleeps.