cold: unified handle architecture (supersedes #57)

[prestwich]

Summary

Collapse cold storage from channels+dispatcher+writer-task into a single ColdStorage<B: ColdStorageBackend> handle over Arc<Inner<B>>. All concurrency primitives (read_sem(64), write_sem(1), stream_sem(8), TaskTracker, CancellationToken) live on Inner; all backend work spawns into the shared tracker; spawned tasks hold permits for the real duration of the backend call.

Supersedes #57. Tracks Linear ENG-2198.

Fixes three PR #57 review issues:

1. Semaphore acquisition was scattered across three places — now all in the handle.
2. StreamLogs setup held the outer read permit across unbounded awaits — streams now acquire only stream_sem; setup reads are isolated from the read pool by design (a stream requesting "latest" should observe latest at setup).
3. Dispatcher-side timeout did not cancel backend work so permits released early — timeouts are now mandatory in the ColdStorageBackend trait and enforced at the backend layer, so permits honestly reflect in-flight work.

Shape:

• ColdStorage<B> is clone-cheap (one Arc refcount bump); replaces ColdStorageHandle + ColdStorageReadHandle + ColdStorageTask.
• ColdStorageWrite now takes &self across all backends (Mem / MDBX / SQL updated in lockstep).
• Reads acquire a read permit in the handle and spawn; cache-hit fast path returns in the caller's task without permit/spawn.
• Writes acquire write_sem then the drain barrier (read_sem.acquire_many_owned(64)); destructive writes invalidate the cache inside the spawned body while holding the drain.
• Streams acquire only stream_sem; producer runs in the shared tracker.
• Hidden shutdown coordinator task watches the cancel token and closes all three semaphores + the tracker; handle calls then fail fast with TaskTerminated.

Backend timeouts (mandatory in trait contract):

• MDBX: tokio::task::spawn_blocking for reads, tokio::task::block_in_place for writes, in-body deadline check between per-block iterations. Post-commit WARN on overrun (advisory — MDBX commits are uninterruptible).
• SQL: SET LOCAL statement_timeout = <ms> at the start of every Postgres transaction; SQLite skips (no equivalent).
• Both backends expose with_read_timeout / with_write_timeout builders on backend and connector. Defaults 500ms / 2s.

Observability:

• New crates/cold/src/metrics.rs following the ajj pattern.
• Gauges: cold.reads_in_flight, cold.writes_in_flight, cold.streams_active.
• Histograms: cold.op_duration_us, cold.permit_wait_us, cold.stream_lifetime_ms.
• Counter: cold.op_errors_total.
• #[tracing::instrument] on every handle method with stable op field; spans propagate into spawned tasks via .in_current_span().
• Alerting is external (Prometheus on gauges); no in-process watchdog.

Error variants removed: Timeout, Backpressure, Cancelled. Backend timeouts now map to Backend. TaskTerminated remains and is returned from every handle method when the semaphore is closed.

Rollout: 10 commits, one per phase, each building + testing clean in isolation:

1. `b539c8f` refactor(cold): ColdStorageWrite takes &self; all backends updated in lockstep
2. `31e1c13` refactor(cold): unify handle around Arc; remove channels and dispatcher
3. `b0d3063` fix(cold): stream permit acquired in handle; streams do not hold a read permit
4. `9881f03` feat(cold): drain barrier moves to handle write path
5. `9426d88` feat(cold): shutdown coordinator closes semaphores on cancel
6. `902aab6` refactor(cold-mdbx): spawn_blocking reads, block_in_place writes, in-body iterator deadline
7. `727c054` feat(cold-sql): mandatory statement_timeout; read_timeout and write_timeout builders
8. `978e2af` feat(cold): metrics and tracing spans across all operations
9. `3339c87` docs(cold): trait impl guide documents mandatory timeouts
10. `cb06741` test(cold): concurrency suite covers new architecture

Spec: `docs/superpowers/specs/2026-04-20-cold-unified-architecture-design.md` (local — `docs/` is gitignored).

Test plan

• `cargo +nightly fmt -- --check`
• `cargo clippy --workspace --all-targets --all-features -- -D warnings`
• `cargo clippy --workspace --all-targets --no-default-features -- -D warnings`
• `RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps`
• `cargo t --workspace` — all suites green, including:
  • `tests/handle_shape.rs` (clone-cheap handle)
  • `tests/stream_isolation.rs` (stream setup not blocked by saturated reads)
  • `tests/drain_barrier.rs` (write waits for reads to drain)
  • `tests/shutdown.rs` (acquire fails fast after cancel)
  • `tests/concurrency.rs` (7 scenarios: 256-way reads, fairness, cancel during backpressure, cancel during drain, stream permit cancel, cache-through-truncate)
  • `crates/cold-mdbx/tests/timeout.rs` (iterator deadline trips; point lookups bypass)
  • `crates/cold-sql/tests/statement_timeout.rs` (gated on Postgres; skip locally without DATABASE_URL)
• Reviewer validation path: cut a patch release, bump `init4tech/node-components`, rebuild `signet-sidecar:latest`, redeploy to dev mainnet, confirm no backpressure-induced crashes and no permit-wait regression on the new metrics.

🤖 Generated with Claude Code

[prestwich]

[Claude Code]

Code review

Went deep on the concurrency model. One real regression worth addressing, plus a resource-lifetime nit.

Found 2 issues:

1. Fast-path cache reads bypass the drain barrier. Withdrawn on review. Cache values are populated only from successful backend reads and are block-number-keyed; a fast-path read during a concurrent truncate returns a value that was real at some point, which is equivalent to serializing the read before the truncate. Caller has no happens-before with the concurrent writer, so this is linearizable. The drain barrier's purpose is to protect the backend from read-during-commit interference, not to globally serialize cache lookups.

2. Shutdown coordinator keeps Inner alive indefinitely if cancel never fires. ColdStorage::new spawns an untracked tokio::spawn that moves Arc<Inner<B>> and awaits cancel.cancelled(). If a caller drops all ColdStorage clones without firing the token, the backend (MDBX env, Postgres pool, file handles) is pinned until process exit. Fixed in 25b4b6c. Coordinator now holds Weak<Inner>; Inner carries a DropGuard on a child cancel token so shutdown fires on either user-side cancel or Inner drop, and the coordinator exits cleanly in both cases.

Separately, CI was red on signet-cold-mdbx::mdbx_backend_conformance — .map_err(ColdStorageError::backend) was unconditionally wrapping MdbxColdError::TooManyLogs as Backend(Box<_>), shadowing the From<MdbxColdError> impl that already translates it. Fixed in 09c2981 by routing all spawn_blocking result conversions through ::from.

🤖 Generated with Claude Code

_{- If this code review was useful, please react with 👍. Otherwise, react with 👎.}

[Evalir]

[Claude Code] Deep-read focused on invariants, panics, and perf regressions. Below are the actionable items I found, ordered roughly by severity.

---

1. block_in_place in MDBX writes panics on a current_thread runtime

crates/cold-mdbx/src/backend.rs:882,897,912,929 — every MDBX write (append_block, append_blocks, truncate_above, drain_above) now wraps its inner call in tokio::task::block_in_place. That API panics on a single-threaded runtime. Pre-PR, this work ran in the dispatcher task and never hit that requirement.

Reads in the same file correctly use spawn_blocking — it is only the writes. The new tests added for this path all pin #[tokio::test(flavor = "multi_thread", worker_threads = 2)], which hides the issue in CI. Any consumer of MdbxColdBackend on a current_thread runtime will panic on the first write.

Fix: either use spawn_blocking like the read path does, or loudly document the multi-threaded runtime requirement on MdbxColdBackend / ColdStorage::new.

2. Cache-hit readers bypass the drain barrier → return just-truncated blocks mid-write

crates/cold/src/handle.rs:243-248, 294-300, 367-373 — get_header, get_transaction, get_receipt check self.inner.cache before acquiring a read_sem permit. spawn_write invalidates the cache after the backend mutation inside the same spawned task:

self.spawn_write("truncate_above", move |inner| async move {
    let result = inner.backend.truncate_above(block).await;  // tens of ms for MDBX
    if result.is_ok() {
        inner.cache.lock().await.invalidate_above(block);
    }
    result
})

Ordering: drain barrier acquired → backend commits the truncate → cache invalidated. Between the backend commit and the cache invalidation, a cache-hit reader can still return a header/tx/receipt for a block the backend has already deleted. The cache_consistent_through_truncate test only checks the post-commit state — it does not exercise this window.

Fix: invalidate the cache before calling backend.truncate_above / backend.drain_above. The drain barrier already guarantees no spawn_read task is racing to re-populate, so pre-invalidation closes the window.

3. stream_logs setup holds stream_sem across an un-timeouted backend call

crates/cold/src/handle.rs:519-544 — the permit is acquired, then self.inner.backend.get_latest_block().await? runs before the task is spawned. The backend call has no timeout:

• SQL: begin_read → pool.begin() can park up to sqlx's acquire_timeout (default 30 s) if the pool is saturated.
• MDBX: get_latest_block is a point lookup which skips the deadline check entirely (see issue 5).

If 8 callers land here with a slow / hung backend, stream_sem is fully held and no new streams can start. The permit only exists to cap in-flight producer tasks; holding it across setup I/O is exactly the bug the PR #57 review (item 2) was meant to avoid.

Fix: either wrap the setup I/O in tokio::time::timeout(effective_deadline, …), or acquire the stream_sem permit after resolving to.

4. MDBX iterator timeouts are only checked between blocks, never within

crates/cold-mdbx/src/backend.rs:658, 483-487, 517-519, and the same pattern in produce_log_stream_blocking:125-131. get_logs_inner / collect_signet_events_in_range / collect_zenith_headers_in_range / the streaming producer check Instant::now() > deadline at the top of each block iteration — never inside the per-receipt or per-log loops. A single block with thousands of matching logs runs far past the 500 ms default before the next check.

In the streaming path it is worse: the deadline-error branch at block boundaries is the only place the deadline ever trips, and the inner blocking_send to a slow receiver can stall indefinitely per log.

Fix: add a bounded periodic check in the inner loops (every N logs/receipts), at minimum inside produce_log_stream_blocking's inner log loop so a slow consumer plus a big block cannot stall a spawn_blocking worker past the deadline.

5. MDBX point lookups have no timeout enforcement at all

crates/cold-mdbx/src/backend.rs:700-704, 726-730, 750-754, 795-799, 861-875 — get_header, get_transaction, get_receipt, get_zenith_header, get_latest_block all spawn_blocking their inner call with no deadline and no timeout wrapper. The trait doc exempts point lookups, but MDBX point reads can still block on disk I/O for arbitrary durations (cold page fetch, adjacent writer doing a page split, etc.).

A stuck point lookup holds a spawn_blocking worker and a read_sem permit. Enough stuck lookups ⇒ read_sem exhausted, and the handle cannot fail fast because nothing resolves.

Fix: wrap each spawn_blocking in tokio::time::timeout(self.read_timeout, …). The blocking thread still runs to completion (unavoidable), but the handle releases its permit promptly and returns an error to the caller.

6. In-flight gauges leak on panic

crates/cold/src/handle.rs:154-168, 211-226, 550-560 — inc_in_flight runs before the spawned body, dec_in_flight runs at the end. If anything between them panics (f(inner).await, the backend call, record_op_duration, the cache .lock()), the decrement is skipped and cold.reads_in_flight / cold.writes_in_flight / cold.streams_active drifts up permanently.

Semaphore permits are Drop-based and do release correctly; only the gauges leak. These are the gauges the PR pitches Prometheus alerts against, so drift poisons the signal.

Fix: wrap the in-flight bookkeeping in a small Drop guard struct.

7. Write-timeout WARN fires on error paths

crates/cold-mdbx/src/backend.rs:878-940 — pattern:

let result = tokio::task::block_in_place(|| …);
let elapsed = start.elapsed();
if elapsed > threshold {
    tracing::warn!(…, "mdbx … exceeded advisory write timeout");
}
Ok(result?)

If the write fails (disk full, corruption) after > 2 s, the caller sees ColdStorageError::Backend(…) and a misleading advisory-write-timeout WARN. Noisy for alerting, since the summary pitches this WARN as an SLO signal.

Fix: only warn on Ok.

---

Notes — not bugs, but worth an eye before merge

• tokio::sync::Mutex<ColdCache> (handle.rs:56) — the cache operations hold the lock only across a synchronous LRU op. A std::sync::Mutex / parking_lot::Mutex would avoid the async-runtime yield on every cache access. Minor perf.
• Removed error variants (Timeout, Backpressure, Cancelled) collapse into Backend for MDBX timeouts — downstream callers that used to match on Timeout now have to string-match. Worth confirming nothing does.
• MemColdBackend silently violates the new "Timeouts (mandatory)" contract. The trait doc says exempt backends "may document their exemption explicitly" — MemColdBackend's type doc does not.

[prestwich]

wrt #2, this is intentional. returning slightly stale data > blocking

[prestwich]

[Claude Code]

Thanks @Evalir — solid review. Pushed two commits addressing everything except #2 (already-considered; see below).

#	Finding	Action	Commit
1	block_in_place panics on current_thread runtime	Writes switched to spawn_blocking; added writes_work_on_current_thread_runtime regression test	7c9d50a
2	Cache-hit readers bypass drain barrier	Ignored. Identical to the finding I withdrew in the prior comment — cache values are block-number-keyed and populated only from successful backend reads, so a fast-path hit during a concurrent truncate linearizes before the truncate. The reader has no happens-before with the writer. Pre-invalidation would be cheap hardening but not a correctness fix.	—
3	stream_sem held across un-timeouted setup I/O	stream_logs now resolves to (incl. get_latest_block fallback) before acquiring the permit	4f67ca7
4	Iterator deadlines only checked between blocks	Added per-receipt check in get_logs_inner, per-event in collect_signet_events_in_range, per-receipt + per-log in produce_log_stream_blocking	7c9d50a
5	Point lookups have no timeout	Documented. Exemption is intentional (MDBX native latency profile), but the type-level docs + with_read_timeout docs now explicitly call out the spawn_blocking worker / read_sem permit implications so callers know to wrap at the call site when fail-fast matters.	7c9d50a
6	In-flight gauges leak on panic	Introduced InFlightGuard RAII wrapper; spawn_read / spawn_write / stream_logs all use it, so the decrement runs even on panic	4f67ca7
7	Advisory-write-timeout WARN fires on error paths	Overrun WARN now only fires on Ok; factored out to a small helper	7c9d50a

Note	Action	Commit
N1 — tokio::sync::Mutex for cache is overkill	Switched to parking_lot::Mutex — no await is ever held across the guard, so the async mutex was pure overhead	4f67ca7
N2 — Removed Timeout/Backpressure/Cancelled collapse into Backend	Promoted to a first-class ColdStorageError::DeadlineExceeded(Duration) variant. MDBX Timeout now routes through it. Audited downstream — no runtime code matched on the removed variants; fixed stale Backpressure references in the cold and storage READMEs and the signet-storage skill doc.	4f67ca7
N3 — MemColdBackend silently violates the "Timeouts (mandatory)" contract	Added an explicit # Timeout exemption section to the type docs, noting it's test-only and MUST NOT be used for SLO/prod	4f67ca7

Verified against the pre-push hook suite: cargo +nightly fmt --check, workspace clippy (--all-features and --no-default-features, -D warnings), RUSTDOCFLAGS="-D warnings" cargo doc, and the cold / cold-mdbx / storage test suites all green.

[Fraser999]

nit: std::time::Instant could be just Instant (same thing in a few places in this file).

[Fraser999]

[Claude Code]

Capturing start here means the SLO measurement only covers spawn_blocking queueing + the MDBX commit. It excludes the handle-side wait on write_sem and the drain barrier, which is precisely the failure shape that wedged production at #56 (slow readers blocking writes). A 3 s drain followed by a 100 ms commit measures 100 ms here, well under the 2 s threshold, and the WARN never fires.

Same applies to append_blocks, truncate_above, and drain_above.

Suggest hoisting the SLO timing into ColdStorage::spawn_write so it captures start before write_sem.acquire_owned, then reading the configured threshold via a new ColdStorageBackend::write_timeout(&self) -> Option<Duration> accessor (defaulting to None so MemColdBackend opts out cleanly). The backend method then becomes a thin spawn_blocking wrapper with no timing logic, and warn_on_overrun can be deleted.

Trade-off worth calling out in the WARN message or commit notes: an e2e measurement will surface reader-side contention as write SLO violations. That is arguably exactly the signal you want given #56, but it is a deliberate change in what the alert means.

[Fraser999]

[Claude Code]

String-matching here hides that PG statement_timeout (SQLSTATE 57014) reaches the caller as Backend(...), not DeadlineExceeded. MDBX timeouts route to DeadlineExceeded (crates/cold-mdbx/src/error.rs:33); PG doesn't, because From<SqlColdError> for ColdStorageError (crates/cold-sql/src/error.rs:15-19) blindly wraps every sqlx error as Backend. Result: callers can't match on timeout without string-matching, and cold.op_errors_total{error=backend} collapses "query too slow" with "backend down".

Suggest: add SqlColdError::Timeout(Duration), detect 57014 at the SQL boundary (carrying the configured read_timeout/write_timeout), map to DeadlineExceeded(d). Then this assertion becomes assert!(matches!(err, SqlColdError::Timeout(_))) and a future refactor that drops timeout detection actually fails the test.

[Fraser999]

[Claude Code]

In PG, SET LOCAL statement_timeout = 0 means "no timeout". The builders with_read_timeout / with_write_timeout (line 154, 165 here, plus the SqlConnector mirrors) accept any Duration including Duration::ZERO, so a caller passing zero (or computing one from a config that defaults to zero) silently disables the trait-level "Timeouts (mandatory)" contract at crates/cold/src/traits.rs:267-296.

Suggest either rejecting zero / sub-millisecond values at the builder (runtime check on first use, or non-const builder returning Result), or documenting the disable behaviour explicitly on with_read_timeout / with_write_timeout. Either way the trait-level "MUST enforce" wording should be reconciled with what the SQL backend actually allows.

[Fraser999]

[Claude Code]

This per-receipt check doesn't bound the inner per-log loop. A single receipt with thousands of matching logs (large mints, batched events) iterates here unchecked. The streaming path (produce_log_stream_blocking:159-194) checks per-log; the non-streaming path inconsistently doesn't.

[Fraser999]

[Claude Code]

This claim is wrong as written. The implementation at line 212 below uses self.cold.drain_above(block).await.unwrap_or_default(), which silently turns every cold error (TaskTerminated, Backend, DeadlineExceeded, etc.) into Vec::new(). StorageError::Cold is never returned from this method.

Pre-existing behaviour, but the doc rewrite in this PR added the claim, so the gap between docs and impl is new. Suggest either:

• Restore real error propagation (return Cold on at least non-TaskTerminated failures).
• Keep the silent-swallow behaviour and update the doc + the "best-effort - failure = normal cold lag" comment to admit it. Currently that comment conflates "cold returned an error" with "cold has no data above" - two different states that operators need to distinguish.

[Fraser999]

[Claude Code]

This JoinError only fires on panic in normal operation - tracked tasks aren't dropped by the runtime, so the only way to get one here is the spawned body panicking inside f(inner).await (i.e. backend code). Mapping it to TaskTerminated tells the caller "shutdown", which on-call will read as "graceful exit", not
"backend panicked". The original error is also swallowed silently - no log of the panic message or location.

Same pattern at line 231 (spawn_write).

Suggest at minimum logging the JoinError at ERROR before mapping. Optionally introduce a ColdStorageError::BackendPanic variant so panics are distinguishable from semaphore-close shutdown - the two failure modes have very different ops responses (panic = bug to chase, TaskTerminated = expected shutdown signal).

[Fraser999]

[Claude Code]

This setup get_latest_block runs before any permit acquisition (deliberate, per the comment above) AND without caller-side timeout protection. With MDBX, point lookups have no in-body deadline so it can stall on disk I/O indefinitely. With PG, pool.begin() inside begin_read can park for acquire_timeout (default 30 s) on a saturated pool. Either way, N concurrent stream_logs callers do uncapped backend reads concurrently with no concurrency cap.

tests/stream_isolation.rs:36-43 uses a bounded to_block, so this code path isn't regression-tested. With a GatedBackend that gates point lookups, you could pin a setup-stall scenario.

Suggest one of:

• Wrap this call in tokio::time::timeout(self.inner.setup_timeout, ...) so it can fail fast (with the standard caveat that the future drops but backend work continues - the same trade-off the rest of the design accepts).
• Move stream_sem.acquire_owned() to before this call. That re-introduces the original concern (a stuck setup pins stream permits), but bounds the call by stream_sem rather than leaving it completely uncapped.

[Fraser999]

[Claude Code]

Stale doc: the signatures below all take &self, and the backend is no longer "exclusively owned by the task runner" - the handle wraps it in Arc<Inner> and shares it across spawned tasks.

[prestwich]

[Claude Code]

Thanks @Fraser999 — pushed four commits addressing all nine inline comments. Summary:

#	Finding	Action	Commit
1	std::time::Instant::now() where Instant is already in scope	Folded both sites in produce_log_stream_blocking	e6d44e4
2	Write SLO start captured inside spawn_blocking, missing write_sem + drain wait	Hoisted timing into ColdStorage::spawn_write; added ColdStorageBackend::write_timeout() accessor; deleted MdbxColdBackend::warn_on_overrun. WARN now measures end-to-end (queue + drain + commit), so reader contention surfaces as a write-SLO violation — explicit signal for the #56 failure shape.	f5fdaae
3	PG SQLSTATE 57014 reaches caller as Backend(...), not DeadlineExceeded; test was string-matching	Added SqlColdError::Timeout; From<sqlx::Error> now detects 57014; From<SqlColdError> for ColdStorageError maps to DeadlineExceeded. Test now matches on the typed variant. The configured deadline is not threaded to this conversion boundary so the surfaced Duration is ZERO (variant correctness > duration precision; threading the real value is a separate refactor).	d3e3210
4	with_read_timeout(Duration::ZERO) silently disables PG statement_timeout	Builders are now non-const and assert! non-zero on both MDBX and SQL backends + connectors. Failure is loud at boot rather than silent at runtime.	e6d44e4
5	get_logs_inner per-receipt deadline check doesn't bound the inner per-log loop	Added per-log Instant::now() > deadline check inside the receipt loop, mirroring the streaming path	e6d44e4
6	unified::drain_above doc claims Cold error path; impl is unwrap_or_default	Doc + comment rewritten to admit silent-swallow behaviour. Propagation/deletion decision tracked in ENG-2210 under ENG-2198.	0b965bf
7	JoinError mapped to TaskTerminated swallows backend panics	spawn_read / spawn_write now log JoinError before mapping: panics fire ERROR with the panic message, cancellations fire DEBUG. Variant still collapses to TaskTerminated per design.	e6d44e4
8	stream_logs setup get_latest_block runs without permit cap or timeout	Wrapped in tokio::time::timeout(backend.read_timeout(), ...) with a 500 ms fallback for backends that don't advertise a timeout. Standard caveat applies (future drops on timeout, backend work continues).	f5fdaae
9	Stale trait doc on ColdStorageWrite ("exclusively owned by the task runner")	Rewritten to describe &self shared-via-Arc ownership	f5fdaae

Pre-push hook clean: cargo +nightly fmt --check, workspace clippy --all-features and --no-default-features (-D warnings), RUSTDOCFLAGS="-D warnings" cargo doc --workspace --no-deps, and the cold / cold-mdbx / cold-sql / storage test suites all green.

[Fraser999]

LGTM now. Would be nice to ticket threading the timeout value through to the actual error, since hard-coding ZERO isn't ideal, but this would be an easy standalone follow-up :)