From c60193ad5ccccc3a38a9c36605d71fc18dcc9894 Mon Sep 17 00:00:00 2001 From: "glm-5.3-flash" Date: Sun, 11 Oct 2026 11:23:45 +0000 Subject: [PATCH] sqlite: SAFETY comments on the substrate UDF get_connection blocks (D-40) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 1 + alkstore-sqlite/src/substrate/ops.rs | 90 +++++++++++++++++++++ alkstore-sqlite/src/substrate/schema.rs | 10 +++ tasks/sqlite-substrate-safety-comments.md | 31 ++++++- 4 files changed, 129 insertions(+), 3 deletions(-) diff --git a/alkstore-sqlite/src/substrate/PROVENANCE.md b/alkstore-sqlite/src/substrate/PROVENANCE.md index bf2b75c..e1a1ce1 100644 --- a/alkstore-sqlite/src/substrate/PROVENANCE.md +++ b/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`) 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 diff --git a/alkstore-sqlite/src/substrate/ops.rs b/alkstore-sqlite/src/substrate/ops.rs index fdce827..58a3648 100644 --- a/alkstore-sqlite/src/substrate/ops.rs +++ b/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 = 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::(&db, "alkstore_panic_probe_sql", || panic!("inside the udf")) }, diff --git a/alkstore-sqlite/src/substrate/schema.rs b/alkstore-sqlite/src/substrate/schema.rs index da2b12f..6a05769 100644 --- a/alkstore-sqlite/src/substrate/schema.rs +++ b/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)", diff --git a/tasks/sqlite-substrate-safety-comments.md b/tasks/sqlite-substrate-safety-comments.md index 8fa1c86..0df5941 100644 --- a/tasks/sqlite-substrate-safety-comments.md +++ b/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 \ No newline at end of file +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. \ No newline at end of file