sqlite: SAFETY comments on the substrate UDF get_connection blocks (D-40)

Review 004 item 15: the unsafe { ctx.get_connection() } UDF blocks
carried no SAFETY justification. Added the concrete soundness
invariant at all ten sites (the eight attach_alkstore_functions
registrations, the notify registration, the test-module savepoint
probe — same class) and registered the comment-only change as
PROVENANCE delta D-40 per ADR-012 §3's diff-fidelity posture.

Task: sqlite-substrate-safety-comments

alkstore-sqlite/src/substrate/PROVENANCE.md

@@ -132,6 +132,7 @@ Cherry-picks: empty at scaffold, append-only forever.
 | D-37 | re-derivation | visibility-only widening on the re-derived `@every` surface: `parse_every_interval` (`pub(crate)` → `pub`) and `skip_forward` (module-private → `pub`) in `queue_ops.rs`, plus this subtree's facade re-export (`mod.rs`: `pub use queue_ops::{parse_every_interval, skip_forward}`), so the engine crate root can `#[doc(hidden)] pub use` both as the wave-7 fuzzing differential target's drive surface (`fuzz-parser-exposure`, target `every_parse`). Zero logic change, zero signature change, zero name change; `mod substrate` stays private, so containment holds — nothing leaves the crate except the doc-hidden root re-exports. A `pub use` re-export mechanically requires item-level `pub` (E0364/E0603), which is why the facade line is in `mod.rs`; the module doc's "nothing re-exported" boundary sentence was amended in the same commit to name this sole exception | `queue_ops.rs` (two visibility keywords); `mod.rs` (facade re-export line + boundary-doc exception sentence) | ADR-012 §3 (fidelity governs the ported honker body — `watcher.rs`/`schema.rs`/`ops.rs` untouched; `queue_ops.rs` is the re-derived half, owned contract-v1 code, so a visibility-only delta is legitimate); docs/research/fuzzing.md §3.2 (option (b): the `#[doc(hidden)]` narrow exposure); ADR-017 §3 (doc-hidden test surface, no stability guarantee — the `Job::from_row` precedent) | ours |
 | D-38 | port delta | degenerate/adversarial-triple hardening on `skip_forward` (two legs, both aligning to the pg engine's committed clamp shape where it existed and extending pg itself): (1) the `interval` is clamped (`interval.max(1) as i128`) — pre-fix, `interval == 0` panicked on i128 division-by-zero, reachable only through degenerate/non-grammar callers (the parsers reject `n <= 0`, so every engine-internal call site passes `interval >= 1`); (2) the gap is computed in i128 (`now as i128 - next_fire_at as i128`) — pre-fix, `now - next_fire_at` was an **i64 subtraction** that panics in debug (overflow) / silently wraps in release when `(next_fire_at, now)` straddle the full domain (`now=i64::MIN`-adjacent, `next_fire_at=i64::MAX`-adjacent), feeding a wrapped gap to the 128-bit arithmetic and breaking the saturation guarantee. Found by the `every_parse` differential target (wave 7, tasks/fuzz-target-every-parse) driving the adversarial `(next_fire_at, now, interval)` space; pg needed leg (2) as well. Observable behavior unaffected in the reachable domain (|gap| < i64 range), saturation posture now holds over the whole domain. Zero signature change; lineage body untouched (`skip_forward` is the re-derived scheduler half, D-12's own) | `queue_ops.rs` (`skip_forward`, two lines) | docs/research/fuzzing.md §3.2 (the overflow-posture invariant: reject-or-saturate, never wrap or panic, over the whole domain); wave-6's register-overflow alignment (tasks/review-wave-6); alkstore-postgres/src/scheduler.rs `skip_forward` (the pinned clamp shape) | ours |
 | D-39 | port delta | watcher-death state machine (review 004 items 2+3, two legs on the ported watcher): (1) **identity-unknown baseline** — the loop's initial identity `stat` failure seeds an unknown baseline instead of the false `(0, 0)`; the first *successful* stat captures it without driving the death path (pre-fix, a transient startup stat hiccup killed the watcher — and closed every subscriber — when the first later stat succeeded); (2) **dead-watcher fail-closed subscribe** — the loop sets a `dead` sign on the caller (`SharedUpdateWatcher`, an `Arc<AtomicBool>`) before exiting through the watcher-fatal path, and `subscribe` becomes fallible, failing closed on the sign (or a taken watcher slot) checked atomically with the enrollment under the senders lock — new subscriptions after a death fail instead of enrolling onto a feed with no poller alive to park forever (the death-guard's clear of *pre-death* subscribers unchanged, D-07 still holds); the engine maps the failure into `Error::Database` at `listen`/stream `subscribe` (ADR-008 §5's opaque fallback). Lineage behavior for both legs was the (0,0)-seed and the infallible enroll | `watcher.rs` (`run_poll_loop`'s baseline/identity arms, `SharedUpdateWatcher`'s dead sign + fallible `subscribe`); `notify.rs`/`stream.rs` (the engine-side mapping) | engine-sqlite.md's watcher-death posture ("new subscriptions fail closed"; identity unknown until the first successful stat); review 004 items 2–3 (tasks/sqlite-fix-watcher-death-subscribe); ADR-006 (the close/fail-closed posture); ADR-012 §4 (delta discipline; D-07's death path refined) | ours |
+| D-40 | hygiene | `// SAFETY` comments installed at all ten `unsafe { ctx.get_connection() }` UDF sites — the eight `attach_alkstore_functions` registrations (stream/lock function set + `alkstore_bootstrap`), the `notify` registration in `attach_notify`, and the savepoint-panic scalar probe in `ops.rs`'s test module. Review 004 item 15 scoped the eight; the two others are the same class (identical rusqlite registration shape, same single-connection synchronous invocation) and joined the same delta. Each comment states the concrete soundness invariant — the UDF is registered on one connection and invoked only as a synchronous callback inside a statement on that very connection (SQLite hands back that handle mid-statement); each connection is one-thread-at-a-time so the cross-thread-alias hazard in rusqlite #643 cannot arise (rusqlite's `Connection` is !Sync; the reader pool transfers owned connections, the writer lease is exclusive); and the closure consumes the deref'd reference only within the callback's lifetime (nothing stored past return, no owned access, no close). Comment-only: zero behavior change. The deliberate D-15 exception — comments riding a delta, not a casual edit | `ops.rs` (the eight `attach_alkstore_functions` blocks + the test-module probe), `schema.rs` (`attach_notify`) | AGENTS.md's inline-comment carve-out ("when a non-obvious safety constraint would otherwise be missed"); review 004 §"Findings recorded, not fixed" item 15; rusqlite 0.40 `Context::get_connection` contract (functions.rs; issue #643) | ours |
 
 ### Cherry-picks
 

alkstore-sqlite/src/substrate/ops.rs

@@ -144,6 +144,16 @@ fn undo_savepoint_frame(conn: &Connection, name: &str, owns_transaction: bool) -
 
 pub(crate) fn attach_alkstore_functions(conn: &Connection) -> rusqlite::Result<()> {
     conn.create_scalar_function("alkstore_bootstrap", 0, FunctionFlags::SQLITE_UTF8, |ctx| {
+        // SAFETY: this UDF is registered on one connection and SQLite invokes it
+        // only as a synchronous callback inside a statement on that very
+        // connection, so the handle handed back is that connection mid-statement.
+        // rusqlite marks `get_connection` unsafe because the rebuilt view could
+        // alias a cross-thread access or outlive the callback; neither is
+        // reachable here — each connection is one-thread-at-a-time (rusqlite's
+        // Connection is !Sync; the reader pool transfers owned connections, the
+        // writer lease is exclusive) and the closure consumes the deref'd
+        // reference only within the callback's lifetime: nothing stored past
+        // return, no owned access, no close.
         let db = unsafe { ctx.get_connection() }?;
         super::schema::bootstrap_schema(&db).map_err(to_sql_err)?;
         Ok(1i64)
@@ -157,6 +167,16 @@ pub(crate) fn attach_alkstore_functions(conn: &Connection) -> rusqlite::Result<(
             let topic: String = ctx.get(0)?;
             let key: Option<String> = ctx.get(1)?;
             let payload: String = ctx.get(2)?;
+            // SAFETY: this UDF is registered on one connection and SQLite invokes it
+            // only as a synchronous callback inside a statement on that very
+            // connection, so the handle handed back is that connection mid-statement.
+            // rusqlite marks `get_connection` unsafe because the rebuilt view could
+            // alias a cross-thread access or outlive the callback; neither is
+            // reachable here — each connection is one-thread-at-a-time (rusqlite's
+            // Connection is !Sync; the reader pool transfers owned connections, the
+            // writer lease is exclusive) and the closure consumes the deref'd
+            // reference only within the callback's lifetime: nothing stored past
+            // return, no owned access, no close.
             let db = unsafe { ctx.get_connection() }?;
             stream_publish(&db, &topic, key.as_deref(), &payload).map_err(to_sql_err)
         },
@@ -170,6 +190,16 @@ pub(crate) fn attach_alkstore_functions(conn: &Connection) -> rusqlite::Result<(
             let topic: String = ctx.get(0)?;
             let offset: i64 = arg_i64(ctx, 1)?;
             let limit: i64 = arg_i64(ctx, 2)?;
+            // SAFETY: this UDF is registered on one connection and SQLite invokes it
+            // only as a synchronous callback inside a statement on that very
+            // connection, so the handle handed back is that connection mid-statement.
+            // rusqlite marks `get_connection` unsafe because the rebuilt view could
+            // alias a cross-thread access or outlive the callback; neither is
+            // reachable here — each connection is one-thread-at-a-time (rusqlite's
+            // Connection is !Sync; the reader pool transfers owned connections, the
+            // writer lease is exclusive) and the closure consumes the deref'd
+            // reference only within the callback's lifetime: nothing stored past
+            // return, no owned access, no close.
             let db = unsafe { ctx.get_connection() }?;
             stream_read_since(&db, &topic, offset, limit).map_err(to_sql_err)
         },
@@ -183,6 +213,16 @@ pub(crate) fn attach_alkstore_functions(conn: &Connection) -> rusqlite::Result<(
             let consumer: String = ctx.get(0)?;
             let topic: String = ctx.get(1)?;
             let offset: i64 = arg_i64(ctx, 2)?;
+            // SAFETY: this UDF is registered on one connection and SQLite invokes it
+            // only as a synchronous callback inside a statement on that very
+            // connection, so the handle handed back is that connection mid-statement.
+            // rusqlite marks `get_connection` unsafe because the rebuilt view could
+            // alias a cross-thread access or outlive the callback; neither is
+            // reachable here — each connection is one-thread-at-a-time (rusqlite's
+            // Connection is !Sync; the reader pool transfers owned connections, the
+            // writer lease is exclusive) and the closure consumes the deref'd
+            // reference only within the callback's lifetime: nothing stored past
+            // return, no owned access, no close.
             let db = unsafe { ctx.get_connection() }?;
             stream_save_offset(&db, &consumer, &topic, offset).map_err(to_sql_err)
         },
@@ -195,6 +235,16 @@ pub(crate) fn attach_alkstore_functions(conn: &Connection) -> rusqlite::Result<(
         |ctx| {
             let consumer: String = ctx.get(0)?;
             let topic: String = ctx.get(1)?;
+            // SAFETY: this UDF is registered on one connection and SQLite invokes it
+            // only as a synchronous callback inside a statement on that very
+            // connection, so the handle handed back is that connection mid-statement.
+            // rusqlite marks `get_connection` unsafe because the rebuilt view could
+            // alias a cross-thread access or outlive the callback; neither is
+            // reachable here — each connection is one-thread-at-a-time (rusqlite's
+            // Connection is !Sync; the reader pool transfers owned connections, the
+            // writer lease is exclusive) and the closure consumes the deref'd
+            // reference only within the callback's lifetime: nothing stored past
+            // return, no owned access, no close.
             let db = unsafe { ctx.get_connection() }?;
             stream_get_offset(&db, &consumer, &topic).map_err(to_sql_err)
         },
@@ -208,6 +258,16 @@ pub(crate) fn attach_alkstore_functions(conn: &Connection) -> rusqlite::Result<(
             let name: String = ctx.get(0)?;
             let owner: String = ctx.get(1)?;
             let ttl: i64 = arg_i64(ctx, 2)?;
+            // SAFETY: this UDF is registered on one connection and SQLite invokes it
+            // only as a synchronous callback inside a statement on that very
+            // connection, so the handle handed back is that connection mid-statement.
+            // rusqlite marks `get_connection` unsafe because the rebuilt view could
+            // alias a cross-thread access or outlive the callback; neither is
+            // reachable here — each connection is one-thread-at-a-time (rusqlite's
+            // Connection is !Sync; the reader pool transfers owned connections, the
+            // writer lease is exclusive) and the closure consumes the deref'd
+            // reference only within the callback's lifetime: nothing stored past
+            // return, no owned access, no close.
             let db = unsafe { ctx.get_connection() }?;
             lock_acquire(&db, &name, &owner, ttl).map_err(to_sql_err)
         },
@@ -220,6 +280,16 @@ pub(crate) fn attach_alkstore_functions(conn: &Connection) -> rusqlite::Result<(
         |ctx| {
             let name: String = ctx.get(0)?;
             let owner: String = ctx.get(1)?;
+            // SAFETY: this UDF is registered on one connection and SQLite invokes it
+            // only as a synchronous callback inside a statement on that very
+            // connection, so the handle handed back is that connection mid-statement.
+            // rusqlite marks `get_connection` unsafe because the rebuilt view could
+            // alias a cross-thread access or outlive the callback; neither is
+            // reachable here — each connection is one-thread-at-a-time (rusqlite's
+            // Connection is !Sync; the reader pool transfers owned connections, the
+            // writer lease is exclusive) and the closure consumes the deref'd
+            // reference only within the callback's lifetime: nothing stored past
+            // return, no owned access, no close.
             let db = unsafe { ctx.get_connection() }?;
             lock_release(&db, &name, &owner).map_err(to_sql_err)
         },
@@ -233,6 +303,16 @@ pub(crate) fn attach_alkstore_functions(conn: &Connection) -> rusqlite::Result<(
             let name: String = ctx.get(0)?;
             let owner: String = ctx.get(1)?;
             let ttl: i64 = arg_i64(ctx, 2)?;
+            // SAFETY: this UDF is registered on one connection and SQLite invokes it
+            // only as a synchronous callback inside a statement on that very
+            // connection, so the handle handed back is that connection mid-statement.
+            // rusqlite marks `get_connection` unsafe because the rebuilt view could
+            // alias a cross-thread access or outlive the callback; neither is
+            // reachable here — each connection is one-thread-at-a-time (rusqlite's
+            // Connection is !Sync; the reader pool transfers owned connections, the
+            // writer lease is exclusive) and the closure consumes the deref'd
+            // reference only within the callback's lifetime: nothing stored past
+            // return, no owned access, no close.
             let db = unsafe { ctx.get_connection() }?;
             lock_renew(&db, &name, &owner, ttl).map_err(to_sql_err)
         },
@@ -739,6 +819,16 @@ mod optional_error_tests {
             0,
             FunctionFlags::SQLITE_UTF8,
             |ctx| {
+                // SAFETY: this UDF is registered on one connection and SQLite invokes it
+                // only as a synchronous callback inside a statement on that very
+                // connection, so the handle handed back is that connection mid-statement.
+                // rusqlite marks `get_connection` unsafe because the rebuilt view could
+                // alias a cross-thread access or outlive the callback; neither is
+                // reachable here — each connection is one-thread-at-a-time (rusqlite's
+                // Connection is !Sync; the reader pool transfers owned connections, the
+                // writer lease is exclusive) and the closure consumes the deref'd
+                // reference only within the callback's lifetime: nothing stored past
+                // return, no owned access, no close.
                 let db = unsafe { ctx.get_connection() }?;
                 in_savepoint::<i64>(&db, "alkstore_panic_probe_sql", || panic!("inside the udf"))
             },

alkstore-sqlite/src/substrate/schema.rs

@@ -101,6 +101,16 @@ pub(crate) fn attach_notify(conn: &Connection) -> Result<(), Error> {
     conn.create_scalar_function("notify", 2, FunctionFlags::SQLITE_UTF8, |ctx| {
         let channel: String = ctx.get(0)?;
         let payload: String = ctx.get(1)?;
+        // SAFETY: this UDF is registered on one connection and SQLite invokes it
+        // only as a synchronous callback inside a statement on that very
+        // connection, so the handle handed back is that connection mid-statement.
+        // rusqlite marks `get_connection` unsafe because the rebuilt view could
+        // alias a cross-thread access or outlive the callback; neither is
+        // reachable here — each connection is one-thread-at-a-time (rusqlite's
+        // Connection is !Sync; the reader pool transfers owned connections, the
+        // writer lease is exclusive) and the closure consumes the deref'd
+        // reference only within the callback's lifetime: nothing stored past
+        // return, no owned access, no close.
         let db = unsafe { ctx.get_connection() }?;
         let mut ins = db.prepare_cached(
             "INSERT INTO __alkstore_notifications (channel, payload) VALUES (?1, ?2)",

tasks/sqlite-substrate-safety-comments.md

@@ -1,7 +1,7 @@
 ---
 id: sqlite-substrate-safety-comments
 name: SAFETY comments on the substrate UDF blocks, riding a PROVENANCE delta (review 004 item 15)
-status: pending
+status: completed
 depends_on: []
 scope: narrow
 risk: low
@@ -64,8 +64,33 @@ if not, leave them and record why.
 
 ## Notes
 
-> To be filled by implementation agent
+- Same-class neighbors **included**: `schema.rs:104` (the `notify`
+  registration in `attach_notify`) and `ops.rs`'s test-module scalar
+  probe (`a_panicking_savepoint_body_leaves_no_frame_on_the_scalar_path`)
+  are the identical rusqlite UDF shape — registered on one connection,
+  invoked only synchronously inside that connection's statement
+  evaluation — so both joined the delta; all **ten** sites carry the
+  comment.
+- The comment states the concrete invariant, derived from rusqlite
+  0.40's `Context::get_connection` contract (functions.rs; the issue
+  #643 analysis behind its unsafety): the rebuilt `ConnectionRef`
+  could alias a cross-thread access or outlive the callback — neither
+  reachable here (each connection one-thread-at-a-time: rusqlite
+  `Connection` is !Sync, the reader pool transfers owned connections,
+  the writer lease is exclusive; the closure consumes the deref'd
+  reference only within the callback's lifetime).
+- Delta registered as **D-40** in `PROVENANCE.md`, category `hygiene`
+  (comment-only, zero behavior change) — the deliberate exception to
+  D-15's comment elision, riding a delta per ADR-012 §3.
 
 ## Summary
 
-> To be filled on completion
+Added uniform `// SAFETY` comments at all ten `unsafe {
+ctx.get_connection() }` UDF sites (the eight review-scoped blocks in
+`attach_alkstore_functions`, plus the same-class `notify` registration
+and the test-module savepoint-panic probe) and registered the
+comment-only change as PROVENANCE delta D-40. Verified: `cargo test`
+workspace green (alkstore-sqlite 203 passed, all crates green
+unchanged), `cargo clippy --all-targets -- -D warnings` clean,
+`cargo fmt --check` clean, `cargo build` green, fuzz gate
+(`fuzz/shared`) green — comment-only diff, no behavior change.