feat: durable store + delivery idempotency — #2 Phase 1#45
Closed
BunsDev wants to merge 4 commits into
Closed
Conversation
Phase 1 of docs/durable-task-store.md: new crates/store on rusqlite (bundled, WAL) with user_version migrations for the webhook_deliveries / tasks / task_attempts schema. Exposes delivery insert-or-dedup, routing updates, and queued-task persistence — the durable claim loop and recovery follow in Phase 2. Signed-off-by: Val Alexander <bunsthedev@gmail.com>
New StorageConfig.path (default data/coven-github.db) locates the durable SQLite state; the section is serde-default so existing configs keep working. Doctor errors when the path's parent exists as a non-directory and warns when it is absent (created on first run). Signed-off-by: Val Alexander <bunsthedev@gmail.com>
handle_webhook now requires X-GitHub-Delivery (400 if absent), records the delivery as an idempotency claim before any work, and returns 200 duplicate:true on redelivery without re-running. Accepted tasks are persisted queued and their routing recorded before the mpsc dispatch (kept for Phase 1); a failed task insert releases the delivery claim and returns 500 so GitHub retries rather than losing the work. Signed-off-by: Val Alexander <bunsthedev@gmail.com>
Opens the SQLite store from storage.path at startup (failing fast on a bad path) and passes it into AppState so the webhook can persist deliveries and tasks. Signed-off-by: Val Alexander <bunsthedev@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Phase 1 implementation of issue #2’s durable state plan: introduces an embedded SQLite-backed store to persist webhook delivery records (for idempotency) and accepted tasks (queued) before acknowledging GitHub, and wires the server/webhook/config plumbing to use it.
Changes:
- Added
coven-github-storecrate (rusqlite + migrations) to persistwebhook_deliveriesandtasksfor delivery dedup + queued task durability. - Updated webhook route to require
X-GitHub-Delivery, claim/dedup deliveries, persist tasks before dispatch, and record routing outcomes. - Added
[storage] pathconfig with defaults + doctor diagnostics; server opens the store at startup and passes it into webhook state.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/worker/src/lib.rs | Updates test config builders to include the new storage config field. |
| crates/webhook/src/routes.rs | Enforces delivery id, adds delivery idempotency + persist-then-ack behavior, records routing, and adds route-level tests. |
| crates/webhook/Cargo.toml | Adds dependency on the new store crate. |
| crates/store/src/lib.rs | Implements SQLite store, migrations, delivery recording, task persistence, and related tests/helpers. |
| crates/store/Cargo.toml | Defines new store crate dependencies/dev-dependencies. |
| crates/server/src/main.rs | Opens durable store at startup and injects it into webhook state. |
| crates/server/Cargo.toml | Adds dependency on the new store crate. |
| crates/config/src/lib.rs | Adds [storage] config with default path plus doctor validation/diagnostics and tests. |
| config/example.toml | Documents and demonstrates [storage] path configuration. |
| Cargo.toml | Adds crates/store workspace member and workspace rusqlite dependency. |
| Cargo.lock | Locks new dependencies introduced by rusqlite / store crate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+224
to
+226
| let version: i64 = conn.query_row("PRAGMA user_version", [], |row| row.get(0))?; | ||
| if version < 1 { | ||
| conn.execute_batch( |
Comment on lines
+89
to
+92
| tokio::task::spawn_blocking(move || { | ||
| let guard = conn.lock().expect("store connection mutex poisoned"); | ||
| f(&guard) | ||
| }) |
Comment on lines
+132
to
+136
| conn.execute( | ||
| "UPDATE webhook_deliveries SET routing = ?2 WHERE delivery_id = ?1", | ||
| rusqlite::params![delivery_id, routing], | ||
| )?; | ||
| Ok(()) |
Comment on lines
+170
to
+173
| if let Err(e) = state.store.insert_task(&task, &delivery_id, supersede_key).await { | ||
| warn!(task_id = %task.id, "failed to persist task: {e:#}"); | ||
| state.store.delete_delivery(&delivery_id).await.ok(); | ||
| return ( |
Comment on lines
+127
to
+132
| match state.store.record_delivery(record).await { | ||
| Ok(DeliveryOutcome::New) => {} | ||
| Ok(DeliveryOutcome::Duplicate) => { | ||
| info!(delivery_id, "duplicate webhook delivery — skipping"); | ||
| return (StatusCode::OK, Json(json!({"ok": true, "duplicate": true}))).into_response(); | ||
| } |
Comment on lines
+333
to
+340
| Err(_) => out.push(Diagnostic::warning( | ||
| "storage.path", | ||
| format!( | ||
| "storage directory '{}' does not exist yet — it will be created on first run.", | ||
| parent.display() | ||
| ), | ||
| )), | ||
| }, |
Member
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase 1 of the merged design (
docs/durable-task-store.md, #43). Advances #2; does not close it — Phases 2 (durable claim loop + restart recovery + retire the in-memoryTaskStore) and 3 (compose volume + README/HOSTED status flip + demo assertion) follow.What this delivers
crates/store— new crate onrusqlite(bundled SQLite, WAL,synchronous=NORMAL,busy_timeout=5s, foreign keys on).PRAGMA user_versionforward-only migrations create the fullwebhook_deliveries/tasks/task_attemptsschema from the design doc. Sync connection guarded by a mutex, bridged to async viaspawn_blocking. Exposes what Phase 1 uses: delivery insert-or-dedup, routing updates, queued-task persistence, and a compensating delete.handle_webhooknow requiresX-GitHub-Delivery(400 if absent), records the delivery as a claim before any work, and returns 200{duplicate:true}on redelivery without re-running. This is the direct fix for GitHub redeliveries creating duplicate sessions/comments/PRs.state='queued') and their routing recorded before dispatch. If task persistence fails, the delivery claim is released and the handler returns 500 so GitHub retries rather than the work being deduped away and lost. The mpsc channel stays for dispatch in this phase (per the doc); a full channel now logs against a durable row instead of silently dropping.[storage] pathconfig (defaultdata/coven-github.db, serde-default so existing configs keep working) with adoctorcheck that errors on a non-directory parent and warns when it's absent.Scope boundaries (per the phased design)
Phase 1 deliberately does not yet: replace the mpsc with a durable claim loop, reload queued/running tasks on restart, move supersession into the store, retire the in-memory
TaskStore, flip the README status, or add the compose volume. Those are Phases 2–3.Tests (map to the issue's criteria)
task:<id>routing; replayed delivery id does not create a second task (duplicate:true); unactionable event recordsignored:routing with no task.Local gates:
cargo check --all-targets+cargo clippy --all-targets -- -D warnings+cargo test --all(145 passed, 0 failed).🤖 Generated with Claude Code