general review 004 (pre-release, whole workspace): one HIGH slot-poisoning fix, two contract-observable divergence fixes, robustness + doc corrections — all gates re-run green
- sqlite scheduler tick: a failed COMMIT (or ROLLBACK) left the connection released with a stuck-open transaction, wedging the writer slot forever — the one commit site missing the drop-+-reopen-replenish arm; tick now takes the conn by value with a poisoned disposition (drop + replenish), the commit-fault seam reaches the tick, and failed_tick_commit_replenishes_the_writer_slot pins the arm - pg save_offset_tx stored GREATEST($3,0) where the wave-8 gate's negative-seed fix (raw seed, ADR-023 §2 2026-10-11 annotation) covered only the auto-commit form; mem's merged in-tx read clamped a staged negative seed against a phantom zero; both fixed and the tx-form totality pinned by a new stamped suite leg (in_tx_reads_see_own_writes) green on all three columns vs the live harness server - pg: a failed forwarder register leaked the channel refcount forever (no receiver to drop it) — listen/spawn_bridge now unregister on the register error, mirroring the subscribe-failure arm; register doc corrected to put the failure disposition at the call site - pg forwarder: nine poisoned-mutex expect() panics converted to the mem crate's into_inner recovery idiom (no-expect-outside-tests rule) - core encode_payload: a deeply-nested Value aborted the process in serde_json's unguarded serializer recursion and the fuzz pin blessed storing bytes no decoder could read back; encode now rejects nesting past 127 containers (the empirically-verified decode limit) with typed Codec — decode-aligned boundary pins added to the fuzz invariant and value_type_tests, applied pre-publish within the open class-1 window - scheduler sleep deadlines: Instant + Duration overflow caps (MAX_SLEEP_S, tick-and-resleep) on all three engines for far-future boundaries; sqlite tick claim stamp now saturating_add - docs read true again: Error::Closed scope (stream try_recv arm), validation whitespace wording, sqlite lib.rs phantom durability knobs, pg lock.rs honest autocommit wording, mem tx/stream merge + wake- granularity wording; wakes.rs stale allow(dead_code) removed - tests: value_type_tests exercised its own map_err instead of the function under test (fixed); unwrap_or_default reference encodes panic with cause - docs/reviews/004-pre-release-general-review.md records nine fixed + sixteen parked findings (sqlite seam panic-safety armor, watcher post-death-subscribe hang + initial-stat self-kill, pg forwarder TOCTOU/hang/diagnosability, pg BIGINT server-side stamp overflow, mem growth/outbox notes, pg identifier-truncation doc note, smaller) and two false positives caught on verification; implementation.md review-rounds entry + core-contract backlog rows updated - gates: build, workspace tests (sqlite 194, core 27, suite legs new), full live pg column 127+25+9, clippy -D warnings, fmt, fuzz replay 6/6, wasm32 check, doc gate -D warnings — all green
This commit is contained in:
1 parent
5385f6bee3
commit
029f885aa1
24 files changed
+738
-103
No files matched your search
@@ -136,7 +136,7 @@ impl ChannelSet {
|
||||
let mut map = self
|
||||
.channels
|
||||
.lock()
|
||||
.expect("channel registry mutex poisoned");
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
let is_new = !map.contains_key(channel);
|
||||
*map.entry(channel.to_string()).or_insert(0) += 1;
|
||||
is_new
|
||||
@@ -150,7 +150,7 @@ impl ChannelSet {
|
||||
let mut map = self
|
||||
.channels
|
||||
.lock()
|
||||
.expect("channel registry mutex poisoned");
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
let Some(count) = map.get_mut(channel) else {
|
||||
return false;
|
||||
};
|
||||
@@ -168,7 +168,7 @@ impl ChannelSet {
|
||||
pub(crate) fn snapshot(&self) -> Vec<String> {
|
||||
self.channels
|
||||
.lock()
|
||||
.expect("channel registry mutex poisoned")
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.keys()
|
||||
.cloned()
|
||||
.collect()
|
||||
@@ -414,7 +414,7 @@ impl Forwarder {
|
||||
pub(crate) fn subscribe(&self) -> Option<broadcast::Receiver<RawNotification>> {
|
||||
self.fanout
|
||||
.lock()
|
||||
.expect("fanout mutex poisoned")
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.as_ref()
|
||||
.map(|s| s.subscribe())
|
||||
}
|
||||
@@ -433,8 +433,14 @@ impl Forwarder {
|
||||
///
|
||||
/// Fails `Database` when no listener connection is currently live
|
||||
/// (mid-reconnect) or the LISTEN itself failed — transient; a
|
||||
/// retry recovers it (the registry entry stays either way and the
|
||||
/// re-issue covers the channel on reconnect).
|
||||
/// retry recovers it (the caller's retry re-adds the reference).
|
||||
/// The failure disposition is the **call site's**: a failed
|
||||
/// register leaves no receiver whose drop would decrement the
|
||||
/// registry, so the failing site must `unregister` the reference
|
||||
/// (the notify `listen` and stream `spawn_bridge` paths do
|
||||
/// exactly that) — a phantom entry here would wedge the
|
||||
/// last-subscriber UNLISTEN rule and keep the channel LISTENing
|
||||
/// after every receiver is gone.
|
||||
pub(crate) async fn register(&self, channel: &str) -> Result<(), alkstore::Error> {
|
||||
self.channels.add_subscriber(channel);
|
||||
if !self.connected.load(Ordering::Acquire) {
|
||||
@@ -493,7 +499,10 @@ impl Forwarder {
|
||||
// Take the sender even if a receiver holds this Arc: the
|
||||
// receivers' terminal `None` must fire at engine shutdown
|
||||
// whatever lifetime stands on the handle.
|
||||
*self.fanout.lock().expect("fanout mutex poisoned") = None;
|
||||
*self
|
||||
.fanout
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner) = None;
|
||||
}
|
||||
|
||||
/// Aim the reconnect arm's connect step at `config` (the failed-
|
||||
@@ -505,7 +514,7 @@ impl Forwarder {
|
||||
*self
|
||||
.reconnect_config
|
||||
.lock()
|
||||
.expect("reconnect config mutex poisoned") = config;
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner) = config;
|
||||
}
|
||||
|
||||
/// The reconnect config's current content (the failed-connect
|
||||
@@ -514,7 +523,7 @@ impl Forwarder {
|
||||
pub(crate) fn reconnect_config_snapshot(&self) -> tokio_postgres::Config {
|
||||
self.reconnect_config
|
||||
.lock()
|
||||
.expect("reconnect config mutex poisoned")
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.clone()
|
||||
}
|
||||
|
||||
@@ -581,7 +590,10 @@ pub(crate) struct FanoutRelease {
|
||||
|
||||
impl Drop for FanoutRelease {
|
||||
fn drop(&mut self) {
|
||||
*self.slot.lock().expect("fanout mutex poisoned") = None;
|
||||
*self
|
||||
.slot
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner) = None;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -819,7 +831,9 @@ async fn forwarder_loop(
|
||||
let attempt = {
|
||||
let slot = reconnect_config.clone();
|
||||
move || {
|
||||
let guard = slot.lock().expect("reconnect config mutex poisoned");
|
||||
let guard = slot
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
let config = guard.clone();
|
||||
drop(guard);
|
||||
async move { config.connect(NoTls).await.ok() }
|
||||
|
||||
@@ -61,11 +61,12 @@ fn table(schema: &str, name: &str) -> String {
|
||||
/// the holder column decides, not the insert's row count (the D-4
|
||||
/// swallow's lesson: never trust a write result for ownership).
|
||||
///
|
||||
/// One statement sequence on one pool connection's implicit
|
||||
/// transaction (auto-committed as a unit on return-to-pool; the
|
||||
/// expiry-delete + insert + read-back race window is closed by the
|
||||
/// PK constraint — a concurrent grant of the same name either
|
||||
/// lands first or reads as foreign).
|
||||
/// One statement sequence on one pool connection in autocommit mode
|
||||
/// (each statement commits as it lands; the acquire is **not** a
|
||||
/// single transaction): the correctness guarantee is the PK
|
||||
/// constraint, not statement atomicity — a concurrent grant of the
|
||||
/// same name either lands first (the read-back then shows the foreign
|
||||
/// owner) or reads as foreign.
|
||||
pub(crate) async fn lock_acquire(
|
||||
client: &tokio_postgres::Client,
|
||||
schema: &str,
|
||||
|
||||
@@ -107,7 +107,14 @@ pub(crate) fn listen(
|
||||
if closed.load(Ordering::Acquire) {
|
||||
return Err(database_error("the store is closed"));
|
||||
}
|
||||
forwarder.register(&channel).await?;
|
||||
// A failed registration leaves no receiver whose drop would
|
||||
// decrement the registry — unregister here, mirroring the
|
||||
// subscribe-failure arm below (no refcount leak past a failed
|
||||
// listen; a retry re-adds the reference).
|
||||
if let Err(e) = forwarder.register(&channel).await {
|
||||
forwarder.unregister(&channel);
|
||||
return Err(e);
|
||||
}
|
||||
let broadcast_rx = forwarder.subscribe();
|
||||
let Some(broadcast_rx) = broadcast_rx else {
|
||||
// The subscribe raced (or arrived past) a shutdown: fail
|
||||
|
||||
@@ -148,6 +148,15 @@ const SLEEP_SLICE_S: u64 = 1;
|
||||
/// The idle sleep when nothing is scheduled (soonest = 0).
|
||||
const IDLE_SLEEP_S: u64 = 60;
|
||||
|
||||
/// The upper bound on one sleep deadline, in seconds: a schedule whose
|
||||
/// next boundary is further out sleeps this long, ticks (a no-op), and
|
||||
/// sleeps again — the tick recomputes the soonest every iteration.
|
||||
/// The cap keeps `Instant + Duration` arithmetic on the sleep deadline
|
||||
/// inside every platform's `Instant` range (a far-future boundary's
|
||||
/// raw wait can exceed a platform's representable span; a bounded
|
||||
/// duration never does).
|
||||
const MAX_SLEEP_S: u64 = 60 * 60 * 24 * 30;
|
||||
|
||||
/// The tick's transient-error retry budget: a tick failed by a
|
||||
/// pool/database error (pool checkout, `BEGIN`, a statement, `COMMIT`)
|
||||
/// retries this many times before the loop exits `Err` (the module
|
||||
@@ -784,7 +793,7 @@ async fn keep_awake_renewing(lease: &mut Box<dyn alkstore::Lock>, stop: &StopTok
|
||||
// due immediately (fire on the next iteration, don't sleep).
|
||||
(soonest - now_unix()).max(0) as u64
|
||||
};
|
||||
let deadline = started + Duration::from_secs(wait_s);
|
||||
let deadline = started + Duration::from_secs(wait_s.min(MAX_SLEEP_S));
|
||||
let slice = Duration::from_secs(SLEEP_SLICE_S);
|
||||
loop {
|
||||
let now = tokio::time::Instant::now();
|
||||
|
||||
@@ -577,8 +577,13 @@ async fn spawn_bridge(
|
||||
) -> Result<Box<dyn EventReceiver>> {
|
||||
// Registration first — a failed registration here (never: LISTEN
|
||||
// failures carry the registry entry's recovery, but a shutdown
|
||||
// race can kill the command channel) fails the whole subscribe.
|
||||
forwarder.register(&stream).await?;
|
||||
// race can kill the command channel) fails the whole subscribe,
|
||||
// unregistered (a failed register leaves no receiver whose drop
|
||||
// would decrement the registry — the notify path's same rule).
|
||||
if let Err(e) = forwarder.register(&stream).await {
|
||||
forwarder.unregister(&stream);
|
||||
return Err(e);
|
||||
}
|
||||
let Some(broadcast_rx) = forwarder.subscribe() else {
|
||||
// The subscribe raced (or arrived past) a shutdown: fail
|
||||
// closed — no receiver can ever deliver again. Unregister
|
||||
|
||||
@@ -437,14 +437,16 @@ impl TxHandle for PgTxHandle {
|
||||
Box::pin(async move {
|
||||
let client = self.client()?;
|
||||
// Monotone upsert (one owner): a first save seeds the
|
||||
// checkpoint — clamped at 0, the absent-consumer=0 rule
|
||||
// (ADR-019 §1) — and a save at-or-below the stored one
|
||||
// updates nothing (ADR-019 §6's silent no-op).
|
||||
// checkpoint — raw, no clamp (ADR-023 §2's 2026-10-11
|
||||
// boundary/offset annotation: the argument is an offset,
|
||||
// a negative is stored as the checkpoint) — and a save
|
||||
// at-or-below the stored one updates nothing (ADR-019 §6's
|
||||
// silent no-op).
|
||||
client
|
||||
.execute(
|
||||
&format!(
|
||||
"INSERT INTO {} AS o (stream, consumer, \"offset\")
|
||||
VALUES ($1, $2, GREATEST($3::bigint, 0))
|
||||
VALUES ($1, $2, $3::bigint)
|
||||
ON CONFLICT (stream, consumer) DO UPDATE
|
||||
SET \"offset\" = EXCLUDED.\"offset\"
|
||||
WHERE EXCLUDED.\"offset\" > o.\"offset\"",
|
||||
|
||||
Reference in new issue
Block a user