Skip to content

Commit 26090d0

Browse files
BunsDevBunsDev
authored
docs: durable task store + delivery idempotency design (#2) (#43)
Proposes embedded SQLite (rusqlite, WAL) behind a new crates/store crate: persist-then-ack webhook handling keyed by X-GitHub-Delivery, a tasks table as the queue (atomic claims, startup recovery, supersession tombstones), task_attempts audit records, and Cave list continuity across restarts. Phased into three separately-mergeable PRs. Signed-off-by: Val Alexander <bunsthedev@gmail.com>
1 parent 30b84d6 commit 26090d0

1 file changed

Lines changed: 254 additions & 0 deletions

File tree

‎docs/durable-task-store.md‎

Lines changed: 254 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,254 @@
1+
# Durable task store and delivery idempotency — design (issue #2)
2+
3+
Status: **proposed** — this document is the review surface for the design;
4+
implementation follows in phased PRs once it is agreed.
5+
6+
## Problem
7+
8+
The adapter accepts GitHub webhooks, maps them to tasks, and pushes them into
9+
an in-process `tokio::mpsc` channel consumed by the worker pool
10+
(`crates/webhook/src/routes.rs` → `crates/worker/src/lib.rs`). Three failure
11+
modes make this unacceptable for a hosted App that promises reliable work:
12+
13+
1. **Silent drops.** A full channel logs `task queue full — dropping task`
14+
and still returns `200 OK`. GitHub treats 200 as delivered and never
15+
retries; the user sees nothing.
16+
2. **Restart amnesia.** Queued and running tasks, task history, and the
17+
supersession registry all live in process memory
18+
(`crates/github/src/tasks.rs`). A deploy or crash loses everything, and
19+
Cave cannot reconstruct state.
20+
3. **Duplicate deliveries.** GitHub redelivers webhooks (manual redelivery,
21+
timeouts, retries). Nothing deduplicates by `X-GitHub-Delivery`, so a
22+
redelivered event re-runs the task: duplicate sessions, comments, PRs.
23+
24+
## Goals
25+
26+
Mapped from issue #2's acceptance criteria:
27+
28+
- Every accepted webhook has a durable record before GitHub sees success.
29+
- Replaying a delivery id never creates a duplicate task.
30+
- Process restart loses no queued task and re-queues interrupted running
31+
tasks.
32+
- A saturated worker pool delays work instead of dropping it.
33+
- Task lifecycle states are explicit and queryable:
34+
`received`, `queued`, `running`, `completed`, `failed`, `ignored`
35+
(plus the existing `superseded` terminal state from #8/#10).
36+
- Cave's `/api/github/tasks` survives restarts.
37+
38+
## Non-goals
39+
40+
- Multi-node worker fleets and distributed queues. One process owns the
41+
store; the hosted fleet architecture arrives with #5/#15 and can graduate
42+
the trait implementation without changing call sites.
43+
- Tenant auth on the task API (#3), audit/retention policy (#12), and
44+
installation-scoped routing (#7) — they build on this store but are
45+
separate issues.
46+
- Payload archival. We persist routing coordinates and a payload hash, not
47+
full webhook bodies (see `docs/security.md`; bodies can embed user
48+
content we don't want retained by default).
49+
50+
## Storage choice: embedded SQLite via `rusqlite`
51+
52+
- **Zero-infra self-hosting.** The compose stack stays single-container;
53+
self-hosters get durability for free with a mounted volume. This matches
54+
the repo's "honest self-hosted adapter" posture.
55+
- **WAL mode** gives concurrent readers with a single writer — exactly the
56+
adapter's shape (webhook writes, worker claims, Cave reads).
57+
- **`rusqlite` over `sqlx`:** the adapter's query surface is small and
58+
hand-written; rusqlite is synchronous (wrapped in `spawn_blocking`),
59+
adds no proc-macro build cost, and avoids an async pool we don't need
60+
at this concurrency.
61+
- **Graduation path.** All access goes through one `TaskStore` type in a
62+
new `crates/store` crate. If hosted scale demands Postgres, the type
63+
becomes a trait with a second backend; call sites don't change.
64+
65+
Connection discipline: one writer connection guarded by a mutex, WAL,
66+
`busy_timeout=5s`, `synchronous=NORMAL`, foreign keys on. Reads may share
67+
the writer connection initially — the volume is tiny; optimizing read
68+
concurrency is premature until Cave polling proves otherwise.
69+
70+
Schema versioning via `PRAGMA user_version` and in-crate forward-only
71+
migrations run at startup.
72+
73+
## Config surface
74+
75+
```toml
76+
[storage]
77+
# Directory for durable adapter state (SQLite database + WAL files).
78+
# The doctor command checks it is creatable/writable.
79+
path = "data/coven-github.db"
80+
```
81+
82+
Default `data/coven-github.db` relative to the working directory; compose
83+
gains a named volume. `doctor` validates the parent directory exists or is
84+
creatable and warns when the path sits on tmpfs.
85+
86+
## Schema
87+
88+
```sql
89+
CREATE TABLE webhook_deliveries (
90+
delivery_id TEXT PRIMARY KEY, -- X-GitHub-Delivery
91+
event TEXT NOT NULL, -- X-GitHub-Event
92+
action TEXT, -- payload action, if any
93+
installation_id INTEGER,
94+
repo TEXT, -- owner/name when parseable
95+
payload_hash TEXT NOT NULL, -- sha256 of raw body
96+
routing TEXT NOT NULL, -- 'task:<id>' | 'ignored:<reason>'
97+
received_at TEXT NOT NULL -- RFC 3339
98+
);
99+
100+
CREATE TABLE tasks (
101+
id TEXT PRIMARY KEY, -- uuid (also the session id)
102+
delivery_id TEXT REFERENCES webhook_deliveries(delivery_id),
103+
installation_id INTEGER NOT NULL,
104+
repo TEXT NOT NULL, -- owner/name
105+
familiar_id TEXT NOT NULL,
106+
kind TEXT NOT NULL, -- serde_json of TaskKind
107+
commander TEXT, -- issue #13 permission gate
108+
state TEXT NOT NULL, -- queued|running|completed|failed|ignored|superseded
109+
supersede_key TEXT, -- 'owner/repo#pr' for PR reviews
110+
attempts INTEGER NOT NULL DEFAULT 0,
111+
-- Result surface for Cave (nullable until terminal):
112+
branch TEXT,
113+
pr_number INTEGER,
114+
check_run_url TEXT,
115+
summary TEXT,
116+
created_at TEXT NOT NULL,
117+
updated_at TEXT NOT NULL
118+
);
119+
CREATE INDEX tasks_state_created ON tasks(state, created_at);
120+
CREATE INDEX tasks_supersede ON tasks(supersede_key)
121+
WHERE supersede_key IS NOT NULL;
122+
123+
CREATE TABLE task_attempts (
124+
task_id TEXT NOT NULL REFERENCES tasks(id),
125+
attempt INTEGER NOT NULL,
126+
started_at TEXT NOT NULL,
127+
ended_at TEXT,
128+
outcome TEXT, -- 'completed' | failure category
129+
detail TEXT, -- redacted error text
130+
PRIMARY KEY (task_id, attempt)
131+
);
132+
```
133+
134+
Notes:
135+
136+
- `tasks.kind` stores the existing `TaskKind` serde JSON — no parallel type
137+
hierarchy, and the wire shape is already versioned by the enum tags.
138+
- `supersede_key` replaces the in-memory `review_heads` map: the newest
139+
review task for a PR supersedes older *queued* rows in the same
140+
transaction that inserts it (mid-flight staleness stays with the #8
141+
re-fetch gate).
142+
- The Cave list (`TaskListItem`) becomes a straight query over `tasks`;
143+
the in-memory `TaskStore` in `crates/github/src/tasks.rs` is retired.
144+
- `task_attempts.detail` passes through the existing `redact` scrubbing
145+
before persistence.
146+
147+
## Webhook path
148+
149+
```
150+
1. Validate HMAC (unchanged)
151+
2. Read X-GitHub-Delivery.
152+
Missing → 400 {"error":"missing delivery id"}.
153+
GitHub always sends it; a caller that doesn't is not GitHub.
154+
3. BEGIN IMMEDIATE
155+
INSERT webhook_deliveries ... ON CONFLICT(delivery_id) DO NOTHING
156+
If the row already existed → COMMIT, return 200 {"ok":true,
157+
"duplicate":true}. Nothing else happens: idempotency.
158+
4. Route the event (existing event_to_task logic).
159+
Not actionable → record routing='ignored:<reason>', COMMIT, 200.
160+
5. INSERT tasks (state='queued', supersede_key when review)
161+
+ tombstone older queued reviews with the same supersede_key
162+
(state='superseded').
163+
Record routing='task:<id>'. COMMIT.
164+
6. Notify the worker pool (tokio::sync::Notify — a wake-up, not a queue).
165+
7. Return 200. Durable state exists before GitHub hears success.
166+
```
167+
168+
The `mpsc` channel disappears. There is no "queue full": the queue is the
169+
`tasks` table, and backpressure is worker-side (semaphore), not
170+
acceptance-side.
171+
172+
Failure mode: if SQLite is unavailable the route returns **500**, GitHub
173+
retries the delivery later, and the operator sees it in logs — strictly
174+
better than acknowledging work we can't hold.
175+
176+
## Worker path
177+
178+
```
179+
loop:
180+
claim = UPDATE tasks
181+
SET state='running', attempts=attempts+1, updated_at=now
182+
WHERE id = (SELECT id FROM tasks
183+
WHERE state='queued' ORDER BY created_at LIMIT 1)
184+
RETURNING *;
185+
none → wait on Notify with a 5s timeout (poll fallback), continue.
186+
some → INSERT task_attempts row; execute (existing execute_task body);
187+
terminal update: state=completed|failed|superseded (+ result
188+
columns), close the attempt row.
189+
```
190+
191+
Concurrency stays a semaphore around claims. Claims are atomic under
192+
SQLite's writer lock; `RETURNING` (SQLite ≥ 3.35, bundled) keeps it one
193+
statement.
194+
195+
**Restart recovery:** on startup, before serving —
196+
197+
```sql
198+
UPDATE tasks SET state='queued'
199+
WHERE state='running';
200+
```
201+
202+
One process owns the store, so any `running` row at boot is an orphan of a
203+
dead process. The attempt row stays closed as `interrupted`; the task gets
204+
a fresh attempt when re-claimed. Tasks whose `attempts` already exceed
205+
`worker.max_retries + 1` go to `failed` instead, so a crash-looping task
206+
cannot poison the queue. (A re-run task re-uses its marker-backed status
207+
comment (#13), so the user surface stays deduplicated even across a
208+
restart.)
209+
210+
## What changes where
211+
212+
| Component | Change |
213+
|---|---|
214+
| new `crates/store` | `Store`: open/migrate, delivery insert-or-dup, task enqueue+tombstone, claim, terminal updates, attempt records, Cave list query, startup recovery |
215+
| `crates/webhook/routes.rs` | Delivery-id gate; persist-then-ack; drop `task_tx`; `list_tasks` reads the store |
216+
| `crates/worker/lib.rs` | Claim loop replaces `mpsc` recv; terminal states write to the store; supersession check reads `state='superseded'` instead of the in-memory registry |
217+
| `crates/github/tasks.rs` | In-memory `TaskStore` retired; `TaskListItem`/`TaskListStatus` stay as the API projection |
218+
| `crates/config` | `[storage] path` + doctor checks |
219+
| `crates/server/main.rs` | Open store, run recovery, wire Notify |
220+
| `compose.yaml` | Named volume for `/data` |
221+
| `README.md` | Durable queue row: planned → implemented (only once true) |
222+
223+
## Test plan (maps to the issue's criteria)
224+
225+
- **Duplicate delivery:** same delivery id twice → one `tasks` row; second
226+
response flags `duplicate`; no second worker execution (wiremock: no
227+
second Check Run POST).
228+
- **Missing delivery id:** 400, nothing persisted.
229+
- **Queue-full behavior:** N tasks with concurrency 1 → all N eventually
230+
complete; none dropped (the old `try_send` drop test inverts).
231+
- **Restart recovery:** enqueue, claim, drop the store handle mid-run,
232+
reopen → task is `queued` again with a closed `interrupted` attempt;
233+
exhausted-attempts variant lands in `failed`.
234+
- **Supersession:** two review events for the same PR → older queued row
235+
`superseded`, only the newer claims.
236+
- **Ignored routing:** unsupported event → delivery recorded with
237+
`ignored:` routing, no task row.
238+
- **Cave continuity:** list query returns pre-restart terminal tasks.
239+
- Demo (`examples/demo/run-demo.sh`) keeps passing — it exercises the full
240+
loop through the real binary.
241+
242+
## Phased PRs
243+
244+
1. **`crates/store` + delivery idempotency.** Store crate, migrations,
245+
config/doctor, webhook persist-then-ack with dedup. The mpsc stays for
246+
dispatch in this PR (tasks additionally recorded durably).
247+
2. **Durable claims + recovery.** Replace mpsc with claim loop + Notify;
248+
startup recovery; supersession moves into the store; retire the
249+
in-memory `TaskStore`; Cave list reads SQLite.
250+
3. **Truth pass.** README/HOSTED status updates, compose volume, demo
251+
assertion that a redelivered webhook does not duplicate comments.
252+
253+
Each PR keeps `cargo check/clippy/test` green and lands separately
254+
mergeable.

0 commit comments

Comments
 (0)