sqlite: panic-safety armor for the auto-commit seams (review 004 item 1)
with_writer/with_reader/blocking_read release their slot/pool connection only on the normal return — a panic inside the closure wedged the single writer slot forever (Writer::acquire waits on a condvar) and permanently shrank the reader pool (outstanding never decremented on abandonment). The seams now ride RAII drop-guards (ConnLease's idiom): the normal arm defuses and releases unchanged; the unwind arm drops the unknown-state connection and takes the established abandonment dispositions — Writer::replenish() (the tick's poisoned arm as a substrate method, the writer now holding its path) and Readers::abandon() (outstanding decrement + pool-locked notify). Three panic-injection tests pin the properties; no reachable panic path exists in production — this is the defense-in-depth armor the review scoped. Task: sqlite-fix-seam-panic-safety.
This commit is contained in:
1 parent
9bbe645558
commit
22f25ffafd
5 files changed
+403
-16
No files matched your search
@@ -1,7 +1,7 @@
|
||||
---
|
||||
id: sqlite-fix-seam-panic-safety
|
||||
name: Panic-safety armor for the sqlite auto-commit seams (review 004 item 1)
|
||||
status: pending
|
||||
status: completed
|
||||
depends_on: []
|
||||
scope: moderate
|
||||
risk: medium
|
||||
@@ -77,8 +77,86 @@ restored and a later reader acquires.
|
||||
|
||||
## Notes
|
||||
|
||||
> To be filled by implementation agent
|
||||
**Design of record** (decided before implementation):
|
||||
|
||||
- **Mechanism**: RAII drop-guards in `seam.rs`, following the tx
|
||||
seam's established `ConnLease` idiom (`tx.rs::ConnLease`). Two
|
||||
guards: `WriterLease` (for `with_writer`) and a shared `ReaderLease`
|
||||
(for `with_reader` and `blocking_read`), each holding the lease
|
||||
connection in an `Option<rusqlite::Connection>` plus an owned
|
||||
`Arc` of the substrate object. The normal arm defuses the guard
|
||||
(`lease.defuse()` yields the connection; the caller releases it to
|
||||
the slot/pool exactly as before the armor — normal paths are
|
||||
behaviorally unchanged). The unwind arm is `Drop`: the connection
|
||||
is **dropped, not released** (a panicked op leaves its statement
|
||||
state unknown; SQLite rolls back its uncommitted state at close —
|
||||
no rollback attempt is made, unlike `ConnLease`, these are
|
||||
auto-commit ops with no intentional transaction), then the
|
||||
abandonment disposition runs.
|
||||
- **Writer disposition**: drop + replenish, per the tick's
|
||||
established poisoned arm (review 004 inline fix 1,
|
||||
`scheduler.rs:235-244`). The replenish lives on the substrate:
|
||||
`Writer` gains a `path: String` field (symmetric with `Readers`,
|
||||
which already held its path — this avoided threading a reopen
|
||||
closure through ~20 `with_writer` call sites) and a
|
||||
`Writer::replenish()` method that opens a fresh bootstrapped
|
||||
connection from its own path and `release`s it into the slot; on
|
||||
open failure the slot stays empty with an `eprintln` for
|
||||
diagnosability — the established arms' tolerance (`ConnLease`'s
|
||||
drop and the tick's arm both take `if let Ok(fresh)`).
|
||||
`Writer::new(path, conn)` now takes the path (call site:
|
||||
`store.rs` only). A failed replenish with a `:memory:` path opens a
|
||||
fresh private db — acceptable, since `:memory:` connections are
|
||||
documented as each-private anyway (the store open test's posture).
|
||||
- **Reader disposition**: outstanding decrement, connection dropped
|
||||
not released. `Readers::abandon()` decrements `outstanding`
|
||||
(saturating — the accounting precises out: every connection carries
|
||||
its fresh-open increment for life, release/pop never decrement, so
|
||||
`outstanding` counts connections in existence and an abandonment is
|
||||
exactly one destruction) and wakes one waiter **under the pool
|
||||
lock** (`Readers::available` waits on the pool mutex — the notify
|
||||
must be race-safe against a waiter between its pool-empty check and
|
||||
`wait`, matching `release`'s notify discipline); the woken waiter
|
||||
re-evaluates pool-miss + under-`max` and opens fresh, restoring
|
||||
capacity. No immediate replacement open — the next acquire repairs
|
||||
lazily.
|
||||
- **Testability**: panics injected by passing panicking closures
|
||||
directly to the `pub(crate)` seam functions from `store/seam_tests.rs`
|
||||
(same crate). `with_writer`/`with_reader` panics surface as
|
||||
`Err(Database)` via the existing `JoinError` mapping — the unwound
|
||||
panic payload is consumed by tokio; `blocking_read` is synchronous
|
||||
(propagates past the seam), so its test wraps the call in
|
||||
`catch_unwind(AssertUnwindSafe(...))`. Post-unwind assertions run
|
||||
behaviorally: slot reacquired, `max_readers: 1` reader re-acquired
|
||||
(the pre-armor state would hang the acquire on both — the condvars
|
||||
have no timeout), and a full store enqueue/read-back succeeds.
|
||||
|
||||
## Summary
|
||||
|
||||
> To be filled on completion
|
||||
Landed the auto-commit seams' panic-safety armor (review 004
|
||||
"Findings recorded, not fixed" item 1):
|
||||
|
||||
- `alkstore-sqlite/src/substrate/schema.rs`:
|
||||
`Writer` carries its path and gained `replenish()` (the tick's
|
||||
poisoned-disposition arm as a substrate method); `Readers` gained
|
||||
`abandon()` (outstanding decrement + pool-lock-held notify_one).
|
||||
- `alkstore-sqlite/src/seam.rs`: `WriterLease` and `ReaderLease`
|
||||
drop-guards now own the connection across all three seams
|
||||
(`with_writer`, `with_reader`, `blocking_read`); the normal arm
|
||||
defuses and releases exactly as before; the unwind arm drops the
|
||||
connection and takes the replenish/decrement disposition. The three
|
||||
seams' doc comments name the armor.
|
||||
- `alkstore-sqlite/src/store/seam_tests.rs` (new): three
|
||||
panic-injection tests pinning writer-slot grantability (re-acquire
|
||||
+ full store op after), reader-pool capacity restoration
|
||||
(`max_readers: 1` re-acquire + full store op), and the synchronous
|
||||
`blocking_read` path via `catch_unwind`.
|
||||
|
||||
Verified: `cargo test -p alkstore-sqlite` (197 lib + 25 suite, green —
|
||||
the pre-existing seam/slot/pool tests untouched and passing, normal
|
||||
paths behaviorally unchanged), `cargo test --workspace` (all columns
|
||||
green server-less), `cargo clippy --all-targets -- -D warnings`, `cargo
|
||||
fmt --check`, the fuzz corpus replay (`fuzz/shared`, 6/6), and the doc
|
||||
gate (`RUSTDOCFLAGS="-D warnings" cargo doc --no-deps` — zero warnings).
|
||||
No reachable panic path exists in production today; this is the
|
||||
defense-in-depth armor the review scoped.
|
||||
Reference in new issue
Block a user