refactor(memory): replace Arc<Mutex<Connection>> with r2d2 connection pool (#3378 part 2) by houko · Pull Request #4685 · librefang/librefang

houko · 2026-05-06T03:57:25Z

Replaces Arc<Mutex<Connection>> with an r2d2 + r2d2_sqlite pool across the
memory substrate, runtime audit log, kernel approval manager, and the
prompt/experiment store. Closes the #3378 root cause: every SQLite call —
session save/load, audit appends, usage records, idempotency lookups, KV
ops — used to serialise on a single std::sync::Mutex<Connection> taken
from tokio tasks, so one slow INSERT (FTS5 tokenization, transactional
cascades, large UPDATE plans) parked the whole tokio worker thread. The
pool unlocks WAL multi-reader concurrency and removes the global lock.

Closes #3378 (part 2 of N). Part 1 (#4544) wrapped sync calls in
spawn_blocking so the lock no longer parked the runtime worker; this PR
removes the lock itself.

Substantive changes

librefang-memory::substrate — MemorySubstrate.conn: Arc<Mutex<Connection>> → pool: Pool<SqliteConnectionManager>. PRAGMAs
(journal_mode=WAL, busy_timeout=5000, cache_size=-2000,
mmap_size=0, foreign_keys=ON, synchronous=NORMAL) are reapplied to
every checkout via SqliteConnectionManager::with_init. Migrations run
once on a dedicated connection before the pool serves traffic.
:memory: pools force max_size=1 because each rusqlite in-memory
connection is a separate database.
10 stores (session, usage, semantic, consolidation, prompt,
structured, decay, knowledge, idempotency, roster_store) —
hold Pool instead of Arc<Mutex<Connection>>; lock() →
pool.get()?; transaction scope preserved by holding the
PooledConnection for the full unchecked_transaction /
transaction_with_behavior(Immediate) lifecycle. Poison-recovery
branches dropped (a non-poisoning pool does not surface
PoisonError).
librefang-runtime::audit — AuditLog.db: Option<Pool<...>>. The
Merkle-chain INSERT is now wrapped in
transaction_with_behavior(Immediate) so chain ordering stays linear
even if the in-memory entries/tip Mutex scope is later narrowed,
or another process holds its own pooled connection.
librefang-kernel::approval — ApprovalManager.audit_db: Option<Pool<...>>. Pending approvals + TOTP lockout restore on boot
use the same pool.
Configurability + observability (#4685 review follow-up):
- MemoryConfig.pool_size: u32 (default 8) wired through
  config.toml: [memory] pool_size. Doc explains the trade-off
  (per-connection page cache cap of 2 MiB, lockstep with
  queue.concurrency.trigger_lane). Clamped to >= 1 so a typo
  pool_size = 0 does not panic r2d2.
- MemorySubstrate::open_with_pool_size and
  PromptStore::new_with_path(db_path, pool_size) accept the value;
  open_with_chunking keeps its old signature for tests via
  DEFAULT_POOL_SIZE = 8.
- librefang_memory_pool_get_failed_total{store=,op=} Prometheus
  counter incremented in audit (record), idempotency
  (lookup/put/prune_expired), and roster
  (upsert/members/remove_member/member_count). Existing
  tracing lines kept so operators get both metric + context.
Pool exhaustion behaviour —
- production pool.get() failures: LibreFangError::memory(...)
  propagation in substrate/session/decay/consolidation/
  idempotency; graceful degradation (tracing::warn! + early
  return) in roster_store; chain NOT advanced (entry dropped) in
  audit. Zero .unwrap() / .expect() on pool.get() outside
  #[cfg(test)].

Regression tests

librefang-memory::substrate::tests::pool_enables_concurrent_reads —
4 concurrent pool.get() holders, ordering-based proof via
AtomicUsize that max_concurrent ≥ 2 (no wall-clock dependency).
librefang-runtime::audit::tests::audit_chain_holds_under_concurrent_record —
8 threads × 50 appends through a real file-backed pool with
max_size = 8. Asserts: row count matches call count; no two rows
share prev_hash (direct fork detector); exactly one genesis row;
every row's prev_hash matches the prior row's hash; reload via
with_db() still passes verify_integrity(). Stable across 10
consecutive runs.
The pre-existing
async_wrappers_do_not_park_current_thread_runtime ordering proof
(PR fix(memory): async wrappers for kernel substrate calls (#3378 part 1) #4544) was rewritten to saturate a max_size = 1 pool from
outside tokio, preserving the original assertion under the pool.

Verification

cargo check --workspace --lib — OK
cargo clippy --workspace --all-targets -- -D warnings — 0 warnings
cargo test -p librefang-memory — 211 passed
cargo test -p librefang-runtime — 1521 passed (incl. audit
contention regression)
cargo test -p librefang-kernel — passes (incl. 81 approval tests
exercising the pool-backed audit_db)
CI (28 / 30 green at the time of writing): Coverage, Quality,
Security, OpenAPI Drift, Live Smoke, all 4 Ubuntu shards, macOS, Test
Unit, Workspace coverage (llvm-cov). Windows job ran 7021/7021
passing then was cancelled in the cache-upload step (CI infra
flake); rerun in flight.

Risk audit

Busy-timeout / WAL preservation: GOOD. PRAGMAs reapplied per checkout.
Migration runner reentrancy: GOOD. Runs on dedicated connection
before stores serve traffic.
Transaction-scope drift: GOOD per call. save_session,
delete_session, delete_agent_sessions, load_canonical,
save_canonical, audit append all hold one PooledConnection for
the full transaction.
Cross-call atomicity: WEAKENED but no concrete bug. Callers that
did lookup → mutate on what they thought was the same connection
now run on two different pool connections; under WAL + IMMEDIATE
this is generally fine. The audit chain (one concrete instance of
this class) is closed by the IMMEDIATE-transaction commit
(6af88ca) + the contention regression test.
FTS5 reconcile: GOOD. MemorySubstrate::open still calls
sessions.reconcile_fts_index() after pool init and before stores
serve traffic. The v33 backfill test was reworked under the pool
and still passes the same end-to-end save-reflow assertions.

Out-of-scope follow-ups

Bump cache_size / mmap_size PRAGMAs from their pre-pool
defaults — separate decision; left identical for parity.
Audit BEGIN IMMEDIATE is defense-in-depth on the hot path; if
audit ever becomes a measurable bottleneck (the contention test
proves correctness, not throughput), batch INSERTs or a
channel-backed audit writer would be the next move.
Extend the pool-exhaustion counter to high-traffic stores not yet
instrumented (session, usage) if the metric ever fires in
production.

CHANGELOG

Entry on [Unreleased] with (@houko) attribution.

… pool (refs #3378 part 2) PR #4544 (part 1) wrapped sync substrate calls in spawn_blocking but kept the single Mutex<Connection>, so all SQLite I/O was still serialised on one lock. This commit replaces the Mutex with an r2d2 + r2d2_sqlite connection pool (max_size = 8 file-backed, 1 in-memory; WAL + busy_timeout=5000), unlocking WAL multi-reader concurrency and removing the single point of serialisation called out by issue #3378. Mutex<Connection> count in crates/ after this commit: 0. Changes: - workspace + librefang-memory Cargo.toml: r2d2 + r2d2_sqlite (workspace deps); compatible with rusqlite 0.39 + bundled feature - librefang-memory/src/substrate.rs: Pool<SqliteConnectionManager> replaces Arc<Mutex<Connection>>; init pragmas via with_init; the schema migration runs once on a dedicated connection before the pool serves request traffic; in-memory test path uses max_size(1) because each rusqlite in-memory connection is a separate database - 10 stores (session, usage, semantic, consolidation, prompt, structured, decay, knowledge, idempotency, roster_store): replace conn: Arc<Mutex<Connection>> with pool: Pool<...>; lock() -> pool.get()?; preserve transaction scope for unchecked_transaction call sites; drop poison-recovery branches (a non-poisoning pool does not surface PoisonError) - librefang-runtime/src/audit.rs: Option<Arc<Mutex<Connection>>> -> Option<Pool<SqliteConnectionManager>> on AuditLog; with_db / with_db_anchored consume the pool from MemorySubstrate::pool().clone() - librefang-kernel + librefang-api routes: substrate access points adapted to the pool API where they directly held the connection - librefang-testing/test_app.rs: in-memory pool fixture - New regression: substrate::tests::pool_enables_concurrent_reads acquires 4 pooled connections concurrently, each holding for 50 ms; serialised execution would take >= 200 ms, the assertion expects < 160 ms (CI-jitter tolerant) and locally completes in well under 100 ms — proves the pool actually parallelises readers under WAL - The PR #4544 ordering proof (async_wrappers_do_not_park_current_thread_runtime) is preserved Verified locally: cargo check --workspace --lib OK (modulo pre-existing macOS libdbus-sys gap in transitive deps; unrelated to this PR; CI Linux runs full) cargo test -p librefang-memory 211 passed; 0 failed (210 existing + 1 new pool_enables_concurrent_reads) Refs #3378 (part 2 of N).

chatgpt-codex-connector · 2026-05-06T03:57:31Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

…p fixture (refs #3378) Follow-up to c062984: - substrate.rs: add pool_enables_concurrent_reads regression test that acquires 4 pool connections concurrently (each holding 50 ms) and asserts the batch completes in < 160 ms, proving WAL multi-reader concurrency is unlocked by the r2d2 pool - substrate.rs: fix clippy::explicit_auto_deref -- &*migration_conn -> &migration_conn (PooledConnection derefs to Connection automatically) - agent_loop.rs: fix test fixture still using Arc<Mutex<Connection>> for SessionStore::new; replace with r2d2 max_size(1) in-memory pool to match the new SessionStore::new(Pool<...>) signature - agent_loop.rs: fix clippy::explicit_auto_deref and remove unused rusqlite::Connection + std::sync::Mutex imports Verified: cargo check --workspace --lib OK (libdbus-sys macOS gap unrelated) cargo clippy -p librefang-memory -p librefang-runtime -- -D warnings OK cargo test -p librefang-memory 211 passed; 0 failed cargo test -p librefang-runtime 1521 passed; 0 failed

…art 2) Independent review of PR #4685 (commit c062984) flagged seven .expect() sites that would panic on pool exhaustion / SQLITE_BUSY in HTTP and channel hot paths, plus stale documentation referring to the removed usage_conn accessor and an over-loose timing bound in the new pool concurrency regression test. - H1 idempotency.rs (3 sites): replace .expect("idempotency pool get") with ? propagation via IdempotencyError::Pool(r2d2::Error). Pool saturation no longer crashes the request thread. - H2 roster_store.rs (4 sites): replace .expect("roster pool get") with let Ok(c) = ... else { warn; return } early-return for all methods. Matches the file's existing swallow-on-error style. - M1 idempotency.rs:89, migration.rs:81: update comments referring to the removed MemorySubstrate::usage_conn to point to MemorySubstrate::pool() instead. - M3 substrate.rs pool_enables_concurrent_reads: replaced the wall-clock assertion with an ordering-based proof: an AtomicUsize tracks the number of tasks holding a pooled connection concurrently, and the test asserts max_concurrent >= 2 inside the 50 ms hold window. No timing dependency. - L doc cleanup: substrate.rs remove_agent_inner / vacuum_inner doc no longer references "the connection mutex"; session.rs:367 explains unchecked_transaction safety in terms of PooledConnection ownership. Verified locally: cargo clippy -p librefang-memory --all-targets -- -D warnings OK cargo test -p librefang-memory 211 passed Refs #3378 (part 2 review fix).

…GMAs - approval.rs test module: re-add use std::sync::Arc; — the Arc<Mutex<Connection>> audit_db field was migrated to a Pool but ~30 surviving tests still hold Arc<ApprovalManager> for thread-spawn fan-out. Removing the import broke every one of them with E0433 on Unit/Quality/Ubuntu/Windows/macOS jobs. - substrate.rs: restore PRAGMA cache_size=-2000 and PRAGMA mmap_size=0 in open_with_chunking. The pool refactor silently dropped both. cache_size capped per-connection page cache at 2 MiB pre-pool; with the new pool defaulting to 8 connections, the explicit cap matters more, not less. mmap_size=0 stays for parity — flipping it is a separate decision. - substrate.rs / prompt.rs: rewrite collapsed-to-one-line PRAGMA strings back to one-PRAGMA-per-line with line continuations. Functionally equivalent, but readable when the next reviewer scans pool init.

… [skip ci]

houko

Review for the r2d2 pool migration (#3378 part 2). Read-only review against the diff; not running CI.

Top findings

[HIGH] `runtime/src/audit.rs` — Merkle chain `tip` ↔ DB INSERT no longer atomic under contention

The pre-pool design serialised the entire record() path through Arc<Mutex<Connection>>, which by side effect serialised both self.tip mutation and the INSERT that depends on the same prev_hash. With the pool, self.tip: Mutex<String> only guards the in-memory read/update; two threads can each acquire a tip snapshot, build entries with the same prev_hash, then both successfully INSERT on different pooled connections. Result: divergent chain forks the verifier won't catch until verify_integrity() runs. The audit_entries.seq INTEGER PRIMARY KEY collision will only catch one direction of the race (same seq), not the prev_hash mismatch when seqs differ.
Fix: hold the tip MutexGuard across the DB INSERT (move the db.get() + execute inside the existing tip lock scope), or wrap entry-build + INSERT in transaction_with_behavior(Immediate) and re-read the latest seq/hash from audit_entries inside the tx.

[HIGH] No integration-level concurrency test for the kernel approval / audit DB

The single new pool concurrency test (pool_enables_concurrent_reads in substrate.rs) only proves that multiple readers can hold pooled connections at once — it does not exercise approvals or audit writes under contention, which is exactly where the cross-connection ordering risk above lives. Recommend adding #[tokio::test] cases that fire ~16 parallel record() / db_insert_pending calls and assert chain-integrity / row counts after.

[MEDIUM] `max_size=8` hardcoded; pool exhaustion silently degrades

substrate.rs:open builds Pool::builder().max_size(8). With the per-agent trigger lane already capped at 8 (queue.concurrency.trigger_lane) plus channel bridges, cron, audit writes and idempotency lookups, a busy daemon can saturate the pool. roster_store.rs degrades to a tracing::warn no-op on pool.get() failure (silent data loss for upsert_member/remove_member); other call sites surface LibreFangError::memory to callers but no metric is emitted. Recommend (a) make max_size configurable via config.toml: memory.pool_size with sensible default, (b) emit a metrics counter on pool.get() failure so operators can see exhaustion before users do.

Regression risk audit

Busy-timeout / WAL preservation: GOOD. PRAGMAs (journal_mode=WAL, busy_timeout=5000, foreign_keys=ON, synchronous=NORMAL, cache_size=-2000, mmap_size=0) are reapplied to every checkout via SqliteConnectionManager::file(...).with_init(|c| c.execute_batch(...)). The in-memory variant correctly drops WAL/busy_timeout (irrelevant for :memory:) and forces max_size=1 because each :memory: connection is a separate DB — the comment makes that explicit.
Migration runner reentrancy: GOOD. run_migrations runs once on a dedicated checkout in a scope block before any store is constructed; no other thread can race because the substrate hasn't been published yet.
Transaction-scope drift: GOOD within a single function. save_session, delete_session, delete_agent_sessions, load_canonical, save_canonical all hold the PooledConnection for the full unchecked_transaction() / transaction_with_behavior(Immediate) lifecycle, so atomicity is preserved per call.
Cross-call atomicity: WEAKENED but no concrete bug found in this diff. Callers that did lookup → mutate on what they thought was the same connection now run on two different pool connections; with WAL + IMMEDIATE this is generally fine, but the audit chain finding above is a real instance of this class.
FTS5 trigger / reconcile: GOOD. MemorySubstrate::open still calls sessions.reconcile_fts_index() after pool init and before stores serve traffic. The v33 backfill test (test_fts_v33_backfill_then_save_reflows_content) was reworked to use the pool and still passes the same end-to-end save-reflow assertions.

`pool.get()` failure handling

Production paths: .map_err(LibreFangError::memory)? everywhere in substrate.rs, session.rs, decay.rs, consolidation.rs, idempotency.rs. New IdempotencyError::Pool variant added with From<r2d2::Error> impl — good. roster_store.rs chooses graceful degradation (tracing::warn! + early return) which is appropriate for that path. runtime/src/audit.rs::record() logs pool get failed; chain NOT advanced and propagates a false persisted. No .unwrap() / .expect() on pool.get() outside #[cfg(test)] modules — verified.

Verdict

REQUEST CHANGES for the audit-chain race (HIGH). The pool migration itself is well-executed — PRAGMA preservation, transaction handling, in-memory max_size=1, the new concurrency test, and complete elimination of .unwrap() from production pool acquisitions all show care. The remaining concern is that the previous Arc<Mutex<Connection>> was acting as a load-bearing serialiser for the audit Merkle chain, and the migration removed that serialisation without replacing it with a transaction-scoped equivalent. Adding an integration test under contention will likely demonstrate the issue and guide the fix.

Positive observations: the pool_enables_concurrent_reads test is the right shape (proves max_concurrent ≥ 2 rather than wall-clock timing); the rewrite of the existing _async wrapper test to use a saturated max_size=1 pool to prove blocking happens in the spawn_blocking thread, not on the runtime worker, is a clean way to preserve the original assertion under the new design; the doc comment on MemorySubstrate::open explaining the per-connection PRAGMA cache-size ceiling math (8 conns × 2 MiB = 16 MiB) is excellent.

…review) Wrap the audit `record_with_context` INSERT in `transaction_with_behavior(Immediate)` so the Merkle chain stays linear even if the in-memory `entries` / `tip` Mutex scope is later narrowed, or if a separate process holds its own pooled connection. IMMEDIATE acquires a SQLite RESERVED lock at the storage layer, which is the strongest guarantee available without an external coordinator and is free under WAL. Adds a contention regression test (`audit_chain_holds_under_concurrent_record`) that fires 8 threads x 50 appends through a real file-backed pool with `max_size=8` and asserts: total persisted rows match the call count, no two rows share a `prev_hash` (direct fork detector), exactly one genesis row exists, every row's `prev_hash` matches the prior row's `hash`, and a `with_db()` reload still passes `verify_integrity()`. Stable across 10 consecutive runs. The fix is defense-in-depth - today's `entries` / `tip` Mutex still covers the same critical section, but the previous code relied on that implicit serialisation as a load-bearing invariant the type system did not enforce. Pushing the guarantee down into SQLite means the chain stays correct under r2d2 pool fan-out regardless of how the in-memory locking evolves. Refs: review of PR #4685.

The PR #4685 r2d2 migration hardcoded `max_size(8)` in two places (`MemorySubstrate::open_with_chunking` and `PromptStore::new_with_path`) and surfaced pool exhaustion only via `tracing::warn!`/`error!`. With the trigger lane already capped at 8 plus channel bridges, cron, audit writes, and idempotency lookups all sharing the same pool, a busy daemon can saturate it and operators see no signal until users notice. This was flagged as MEDIUM in the PR self-review and is the last outstanding follow-up before the migration is fully wrapped. - librefang-types: `MemoryConfig.pool_size: u32` (default 8) wired through `config.toml: [memory] pool_size`. Doc comment explains the trade-off (per-connection page cache cap of 2 MiB, lockstep with `queue.concurrency.trigger_lane`). - librefang-memory: - new `MemorySubstrate::open_with_pool_size(...)` taking explicit `pool_size`. `open_with_chunking` keeps its old signature and delegates with `DEFAULT_POOL_SIZE = 8` so existing test fixtures and ad-hoc tools compile unchanged. - `PromptStore::new_with_path(db_path, pool_size)` likewise accepts the value rather than hardcoding 8 — the kernel boot path passes `config.memory.pool_size` for both so a single config knob tunes both pools. - both clamp `max_size = pool_size.max(1)` so a deserialised `pool_size = 0` (operator typo) fails soft instead of crashing boot via r2d2's "max_size = 0" panic. - `metrics` workspace dep added; `pool.get()` failures in `idempotency.rs` (lookup/put/prune_expired), `roster_store.rs` (upsert/members/remove_member/member_count), and the audit append in `runtime/src/audit.rs` now increment the `librefang_memory_pool_get_failed_total` counter labelled with `store=` and `op=`. Existing `tracing` lines are kept so operators get both the metric and the contextual log. - librefang-kernel: substrate + prompt store init at boot now reads `config.memory.pool_size`. Verified locally: cargo check -p librefang-types -p librefang-memory -p librefang-runtime -p librefang-kernel --lib OK cargo clippy -p librefang-types -p librefang-memory -p librefang-runtime -p librefang-kernel --all-targets -- -D warnings OK (0 warnings) cargo test -p librefang-types --lib 734 passed cargo test -p librefang-memory --lib 211 passed (incl. existing pool concurrency regressions) cargo test -p librefang-runtime --lib audit 29 passed (incl. audit_chain_holds_under_concurrent_record) cargo test -p librefang-kernel --lib approval 81 passed cargo fmt --all -- --check clean Refs #3378 (part 2 review fix), PR #4685.

The merge that pulled main into the PR branch (b3f1a8d) brought in three KernelConfig fields without updating `routes/config.rs::ui_sections_overlay()` or the `every_kernel_config_struct_field_is_exposed_via_overlay` test guard introduced by #4682: - `cron_session_compaction_mode` and `cron_session_compaction_keep_recent` (#3693): flat scalars on KernelConfig that belong next to the existing `cron_session_max_*` / `cron_session_warn_*` knobs. - `tool_exec` (tool-exec backend selection): a sub-config struct that needs its own section descriptor. Without this, Coverage CI on the PR's merge-with-main commit fails the new dashboard-coverage guard. Fixes: - `routes/config.rs::ui_sections_overlay()`: - append the two compaction scalars to the synthetic `general` section's `fields` list. - add a `{"key": "tool_exec", "struct_field": "tool_exec"}` entry so the dashboard ConfigPage picks the new sub-section up. - `tests/config_schema_overlay.rs`: extend the `EXCLUDED` allowlist with the two compaction scalars (root-level scalars are tracked via the synthetic general section, not via `struct_field`). - `tests/fixtures/kernel_config_schema.golden.json`: regenerated via `cargo test -p librefang-api --test config_schema_golden -- --ignored regenerate_golden`. Diff captures the new tool_exec sub-tree, the two compaction fields, and `MemoryConfig.pool_size` from cdce7bf. Verified locally: cargo test -p librefang-api --test config_schema_overlay 4 passed cargo test -p librefang-api --test config_routes_integration 26 passed cargo clippy -p librefang-api --all-targets -- -D warnings 0 warnings Refs PR #4685 CI fix.

… compaction The Phase 1 mechanical-move subagent worked off a state that pre-dated four recent main commits, silently reverting their changes during the trait-impl extraction: - #4685 (prompt store r2d2 pool): mod.rs called `memory.usage_conn()` and passed 1 arg to `PromptStore::new_with_path`. Both APIs had been replaced with `memory.pool()` / 2-arg `new_with_path(&db_path, pool_size)`. Restored. - #3329 (memory_wiki vault): `kernel_handle::WikiAccess` is a `KernelHandle` super-trait but the `wiki_vault` field is not yet on `LibreFangKernel` in this branch. Replace the broken impl with an empty `impl WikiAccess for LibreFangKernel {}` so every method falls through to the trait default (`KernelOpError::unavailable("wiki_*")`) until the rebase-on-main work pulls in the full vault wiring. - #4683 (cron summarize-trim compaction): `kernel::tests` references `cron_compute_keep_count` / `cron_clamp_keep_recent` / `cron_resolve_compaction_mode` / `try_summarize_trim` that were dropped from mod.rs. Cherry-pick the four helpers into a new `kernel::cron_compaction` sub-module + re-export from mod.rs so tests resolve. The matching cron-tick body change still needs to be ported into `kernel::cron_tick` — that's a separate item. Verified: cargo check -p librefang-kernel --lib + cargo clippy -p librefang-kernel --lib -- -D warnings + cargo check -p librefang-kernel --lib --tests + rustfmt --check all green. Note for reviewers: this PR's branch is "behind 4" of origin/main. The proper fix is a rebase, which will involve genuine merge work in the giant impl block that the refactor sliced. This commit is the minimum to keep HEAD compiling against the current workspace deps; it does NOT restore #4683's runtime changes (the SummarizeTrim path inside the cron tick body) — only the helpers + their tests.

…review) The kernel/mod split landed type-clean but silently dropped four behavioural hooks that `cargo check` / `clippy` could not see. All four fail open / fail-permissive (rather than fail-closed), which is why CI green doesn't catch them — they're only visible by exercising the boot path or the wiki tools at runtime. Restored to parity with `origin/main` (pre-split): - `boot.rs`: re-add `config.tool_exec.validate()` after the workspace-wide `config.validate()` warning loop. Without it, a deployment with `[tool_exec] kind = "ssh"` or `"daytona"` but missing the matching subtable boots silently and fails on first tool call instead of aborting at boot. - `boot.rs`: `MemorySubstrate::open_with_chunking(...)` → `open_with_pool_size(..., config.memory.pool_size)`. The 3-arg form forces the r2d2 default pool size of 8 regardless of operator config, so `[memory] pool_size` (just landed in #4685) was a no-op for the primary substrate; only the prompt store init below this line still honoured the setting, leaving the two SQLite pools inconsistently sized. - `spawn.rs::spawn_agent_inner`: re-add the `manifest.tool_exec_backend` override validation block after `validate_manifest_module_path`. Without it, an agent whose manifest pins `tool_exec_backend = "ssh"` against a daemon lacking `[tool_exec.ssh]` enters the registry and fails every tool invocation thereafter, instead of being rejected pre-registry. - `mod.rs` + `boot.rs` + `handles/wiki_access.rs`: re-add the `wiki_vault: Option<Arc<librefang_memory_wiki::WikiVault>>` field on `LibreFangKernel`, the boot-time `WikiVault::new` initializer with the same fail-open warning behaviour as main, the `wiki_vault: wiki_vault.clone()` in the struct construction, and the concrete `wiki_get` / `wiki_search` / `wiki_write` method bodies (ported verbatim from the pre-split mod.rs). Without these three pieces, every `tool_wiki_*` call on a deployment with `[memory_wiki] enabled = true` flowed to the trait default and returned `KernelOpError::unavailable("wiki_*")`, silently disabling the just-landed #3329 feature. Phase 1 had left an empty `impl WikiAccess for LibreFangKernel {}` with a header comment claiming \"will arrive with the #3329 main-side merge\" — but #3329 was already on `origin/main` (#4712) at the time of the branch. Verified locally: cargo check -p librefang-kernel --lib OK cargo clippy -p librefang-kernel --lib -- -D warnings 0 warnings rustfmt --check --edition 2021 (4 modified files) clean Refs Codex review on commits 594aa42 / 0107d06 / 79eb683 of the parent PR — findings #2-#5 (the four P2 items still open at HEAD) addressed here. Codex P1 finding #1 (cron SummarizeTrim) was addressed earlier in 0107d06 and is unaffected by this commit.

…hases 1-3) (#4713) * refactor(kernel): extract 16 kernel_handle trait impls into kernel::handles Phase 1 of the kernel/mod.rs split. Move every `impl kernel_handle::* for LibreFangKernel` block out of the 20K-line god-file into per-trait files under `kernel/handles/`. The submodules are descendants of `kernel`, so they retain access to LibreFangKernel's private fields and inherent methods without any visibility surgery. Mechanical, behavior-preserving move — no method bodies were touched. mod.rs drops from ~20.6k to ~18.8k lines (-2073). Three imports that became unused after the move were removed (kernel_handle self-alias, ToolApprovalSubmission, std::str::FromStr) and re-imported locally in the consuming handle files. Traits moved: AgentControl, MemoryAccess, TaskQueue, EventBus, KnowledgeGraph, CronControl, HandsControl, ApprovalGate, A2ARegistry, ChannelSender, PromptStore, WorkflowRunner, GoalControl, ToolPolicy, ApiAuth, SessionWriter Verified: cargo check -p librefang-kernel --lib + cargo clippy -p librefang-kernel --lib -- -D warnings both green. * refactor(kernel): phase 2 — extract free-fn / cron-bridge / probe helpers Move the cohesive bottom-of-file helpers out of kernel/mod.rs into thematic sub-modules of `kernel`: mcp_summary.rs – mcp_summary_cache_key, render_mcp_summary reviewer_sanitize.rs – sanitize_reviewer_line, sanitize_reviewer_block cron_script.rs – cron_script_wake_gate, atomic_write_toml, parse_wake_gate cron_bridge.rs – `impl CronChannelSender for KernelCronBridge`, CRON_EMPTY_OUTPUT_HEARTBEAT, cron_fan_out_targets, cron_deliver_response provider_probe.rs – probe_and_update_local_provider, probe_all_local_providers_once Mechanical, behavior-preserving move. mod.rs drops by ~636 lines (now ~18.2k). The struct `KernelCronBridge` itself stays in mod.rs; only its trait impl moves. Free fns formerly visible only to mod.rs are now `pub(super)` so the (still-resident) `LibreFangKernel` methods that call them keep compiling. mod.rs adds a `use cron_bridge::{cron_deliver_response, cron_fan_out_targets};` re-export so existing call sites in mod.rs and `kernel::tests` resolve byte-for-byte. Also restores `kernel_handle::self` alongside the prelude wildcard import — phase 1 dropped the self alias as "newly unused", missing the fact that `kernel::tests` uses it via `kernel_handle::ApprovalGate::...`. Without this, `cargo test -p librefang-kernel --lib --no-run` fails. * refactor(kernel): phase 3a — extract accessor / lifecycle impl block to kernel::accessors Move the first `impl LibreFangKernel { ... }` block (formerly mod.rs lines 1106..2598, ~1500 lines, ~85 methods — accessors like `home_dir`, `config_ref`, `cron`, `pairing_ref`, plus operational helpers like `spawn_key_validation`, `spawn_approval_sweep_task`, `record`, `auto_dream_*`, etc.) into `crates/librefang-kernel/src/kernel/accessors.rs`. Sibling submodule of `kernel::mod`, so retains private-field/method access into `LibreFangKernel` without any visibility surgery (same pattern as the Phase 1 trait-impl moves). Mechanical, behavior-preserving move — method bodies untouched. mod.rs drops from ~18.2k to ~16.7k lines (-1500). 4 inherent `impl LibreFangKernel` blocks remain in mod.rs (down from 5); phase 3b/3c will continue chipping. Verified: cargo check / clippy -- -D warnings / cargo check --tests / rustfmt --check all green. * refactor(kernel): phase 3b — extract cron tick loop closure to kernel::cron_tick The cron scheduler tick loop was historically the longest closure in mod.rs (~528 lines, the landing zone for #4683 et al.). Lift the body out of `spawn_logged("cron_scheduler", async move { … })` into a free `pub(super) async fn run_cron_scheduler_loop(kernel: Arc<LibreFangKernel>)` in `crates/librefang-kernel/src/kernel/cron_tick.rs`. Behaviour-preserving — body moved byte-for-byte; only the outer wrapper changed (closure → free fn). Captured state was just `kernel`, which becomes the single fn parameter; per-tick `cron_sem` and per-job clones are still constructed inside the loop body, unchanged. mod.rs drops by ~528 lines (now ~16.2k). Three Phase 2 re-exports (`cron_deliver_response`, `cron_fan_out_targets`, `cron_script_wake_gate`) that mod.rs introduced for the inline closure are no longer needed — the only call sites moved with the loop — so they're gone from mod.rs's `use` block. Verified: cargo check / clippy -- -D warnings / cargo check --tests all green. * fix(kernel): repair phase 1 regressions on prompt store / wiki / cron compaction The Phase 1 mechanical-move subagent worked off a state that pre-dated four recent main commits, silently reverting their changes during the trait-impl extraction: - #4685 (prompt store r2d2 pool): mod.rs called `memory.usage_conn()` and passed 1 arg to `PromptStore::new_with_path`. Both APIs had been replaced with `memory.pool()` / 2-arg `new_with_path(&db_path, pool_size)`. Restored. - #3329 (memory_wiki vault): `kernel_handle::WikiAccess` is a `KernelHandle` super-trait but the `wiki_vault` field is not yet on `LibreFangKernel` in this branch. Replace the broken impl with an empty `impl WikiAccess for LibreFangKernel {}` so every method falls through to the trait default (`KernelOpError::unavailable("wiki_*")`) until the rebase-on-main work pulls in the full vault wiring. - #4683 (cron summarize-trim compaction): `kernel::tests` references `cron_compute_keep_count` / `cron_clamp_keep_recent` / `cron_resolve_compaction_mode` / `try_summarize_trim` that were dropped from mod.rs. Cherry-pick the four helpers into a new `kernel::cron_compaction` sub-module + re-export from mod.rs so tests resolve. The matching cron-tick body change still needs to be ported into `kernel::cron_tick` — that's a separate item. Verified: cargo check -p librefang-kernel --lib + cargo clippy -p librefang-kernel --lib -- -D warnings + cargo check -p librefang-kernel --lib --tests + rustfmt --check all green. Note for reviewers: this PR's branch is "behind 4" of origin/main. The proper fix is a rebase, which will involve genuine merge work in the giant impl block that the refactor sliced. This commit is the minimum to keep HEAD compiling against the current workspace deps; it does NOT restore #4683's runtime changes (the SummarizeTrim path inside the cron tick body) — only the helpers + their tests. * fix(kernel): port #4683 cron-tick SummarizeTrim path into kernel::cron_tick Phase 3b's `cron_tick.rs` body was the pre-#4683 closure (the version my Phase 1 subagent saw); the SummarizeTrim runtime path was lost when the Phase 1 work happened to omit #4683's mod.rs hunk. The earlier repair commit restored the helper fns into `kernel::cron_compaction` but left their call site missing. Replace the body of `run_cron_scheduler_loop` with the post-#4683 version copied from origin/main's mod.rs (the `spawn_logged("cron_scheduler", async move { … })` block at lines 14386..15064). Now the cron tick body actually calls: - `cron_compute_keep_count` to size the keep window without mutating - `cron_resolve_compaction_mode` to fall back from SummarizeTrim → Prune when keep_count < 2 - `cron_clamp_keep_recent` to enforce `[summary] + tail` cap - `try_summarize_trim` (aux LLM compaction) on the SummarizeTrim path - `apply_cron_prune` for both the configured Prune path and the SummarizeTrim → Prune fallback `apply_cron_prune` was bumped to `pub(super)` so cron_tick.rs (sibling submodule of cron_compaction) can call it. Verified: cargo check / clippy -- -D warnings / cargo check --tests / rustfmt --check all green. * refactor(kernel): phase 3c — extract 5 thematic clusters from giant impl Slice the still-resident giant `impl LibreFangKernel { ... }` body into five cohesive sub-modules under `kernel/`: messaging.rs — 26 send_message* / send_message_streaming_* variants + send_message_full / send_message_full_with_upstream + resolve_agent_home_channel + run_forked_agent_streaming (~2 670 lines) assistant_routing.rs — 12 helpers driving the @-mention / specialist routing path: notify_owner_bg, llm_classify_intent, resolve_or_spawn_specialist, send_message_streaming_resolved, resolve_assistant_target, route_assistant_by_metadata, etc. (~460 lines) hands_lifecycle.rs — 14 hand-management methods: activate_hand, deactivate_hand, reload_hands, apply_hand_agent_runtime_override_to_registry, resolve_hand_agent_model_defaults, etc. (~990 lines) mcp_setup.rs — 6 MCP wiring fns: connect_mcp_servers, reload_mcp_servers, retry_mcp_connection, reconnect_mcp_server, run_mcp_health_loop, disconnect_mcp_server (~610 lines) prompt_context.rs — 7 cached-summary builders for prompt assembly: cached_workspace_metadata, cached_skill_metadata, active_goals_for_prompt, build_skill_summary_from_skills, build_mcp_summary, collect_prompt_context, etc. (~340 lines) Mechanical, behavior-preserving move — method bodies untouched. mod.rs drops from ~16.2k to ~11.3k lines (-4 950). Combined with phases 1+2+3a+3b that's a 9 350-line reduction (-45%) on the original 20 609-line god-file. Visibility surgery (only what the compiler demanded; private inherent methods are not visible across sibling submodules without it): - messaging::send_message_full / send_message_streaming_with_sender_and_session bumped to pub(crate) — called from cron_tick + assistant_routing. - assistant_routing::{notify_owner_bg, send_message_streaming_resolved, resolve_assistant_target, assistant_route_key, should_skip_intent_classification} bumped to pub(crate) — called from messaging + tests. - mcp_setup::run_mcp_health_loop bumped to pub(crate) — kept-resident spawn_logged callsite in mod.rs needs it. - prompt_context::{cached_workspace_metadata, cached_skill_metadata, active_goals_for_prompt, build_mcp_summary} bumped to pub(crate) for cross-module consumers. - mod.rs structs CachedWorkspaceMetadata + CachedSkillMetadata bumped to pub(crate) struct (private fields kept) so the pub(crate) accessor signatures don't trip the private-interfaces lint. Verified across all 5 cluster checkpoints + final pass: cargo check -p librefang-kernel --lib + clippy -- -D warnings + check --tests + rustfmt --check all green. * refactor(kernel): phase 3d — extract 5 more clusters from giant impl Continue slicing the still-resident `impl LibreFangKernel { ... }` body into thematic sub-modules: boot.rs — session_stream_hub, boot, boot_with_config (~2 005 lines — `boot_with_config` is the single largest method extracted so far) spawn.rs — spawn_agent + 4 spawn_agent_with_* + spawn_agent_inner + verify_signed_manifest + trusted_manifest_signer_keys (~410 lines) agent_execution.rs — execute_wasm_agent, execute_python_agent, should_reuse_cached_route, is_brief_acknowledgement, execute_llm_agent (~1 195 lines) session_ops.rs — inject_message + inject_message_for_session + setup/teardown_injection_channel + resolve_module_path + reset/reboot_session + clear_agent_history + list/create/switch/export/import_session + inject_reset_prompt + evaluate_condition + save_session_summary (~820 lines) agent_state.rs — persist_manifest_to_disk, set_agent_model, reload_agent_from_disk, update_manifest, set_agent_skills, set_agent_mcp_servers, set_agent_tool_filters (~530 lines) Mechanical move — method bodies untouched. mod.rs drops from ~11.3k to ~6.4k lines (-4 870). Cumulative across phases 1+2+3a+3b+3c+3d the original 20 609-line god-file is now ~6 385 lines (-69%). Visibility surgery (only what the compiler demanded for cross-submodule call sites): - spawn::spawn_agent_inner → pub(crate) (called by hands_lifecycle) - agent_execution::execute_wasm_agent / execute_python_agent / execute_llm_agent / should_reuse_cached_route → pub(crate) (called by messaging + assistant_routing) - session_ops::{setup_injection_channel, teardown_injection_channel, resolve_module_path, inject_reset_prompt, evaluate_condition} → pub(crate) (called by messaging / spawn / tests) No struct or enum visibility bumps were needed — `boot.rs` builds the kernel via struct literal because descendant submodules retain private-field access. Verified: cargo check / clippy -- -D warnings / cargo check --tests / rustfmt --check all green at every cluster checkpoint and the final pass. * refactor(kernel): phase 3e/1 — extract agent_runtime cluster Move runtime-control methods (cost/stop/list/suspend/resume/compact/ context_report/kill + watchers) from the giant `impl LibreFangKernel` into `kernel/agent_runtime.rs`. mod.rs drops from 6 385 to 5 825 (-560). Methods: session_usage_cost, stop_agent_run, stop_session_run, list_running_sessions, agent_has_active_session, running_session_ids, suspend_agent, resume_agent, persist_agent_enabled, compact_agent_session, compact_agent_session_with_id, context_report, register_agent_watcher, abort_agent_watchers, kill_agent, kill_agent_with_purge. Verified: cargo check / clippy -- -D warnings / cargo check --tests all green. * refactor(kernel): phase 3e/2-7 — extract 6 remaining clusters from giant impl Continue slicing the still-resident `impl LibreFangKernel { ... }` body into thematic submodules. After this commit the giant first impl block is fully drained. bindings_and_handle.rs — set_log_reloader, set_self_handle, kernel_handle, list_bindings, add_binding, remove_binding (~95 lines) config_reload_ops.rs — reload_config + apply_hot_actions_inner (~480 lines) triggers_and_workflow.rs — spawn_session_label_generation, one_shot_llm_call, publish_event + publish_event_inner, register_trigger + register_trigger_with_target + remove_trigger / set_trigger_enabled / list_triggers / get_trigger / update_trigger, register_workflow, run_workflow, dry_run_workflow (~640 lines) background_lifecycle.rs — start_background_agents, start_ofp_node, self_arc, start_heartbeat_monitor, start_background_for_agent, shutdown (~1 270 lines — the longest single extraction in this PR) llm_drivers.rs — lookup_provider_url, resolve_driver (~250 lines) tools_and_skills.rs — available_tools, reload_skills, try_claim_skill_review_slot, summarize_traces_for_review, background_skill_review, is_transient_review_error, extract_json_from_llm_response, context_engine_for_agent (~1 080 lines) Mechanical move — method bodies untouched. mod.rs drops from ~5.8k to ~2.0k lines (-3 800). Combined with phases 1+2+3a+3b+3c+3d the original 20 609-line god-file is now at ~2 000 lines (-90%). Visibility surgery (only what the compiler demanded): - tools_and_skills::{context_engine_for_agent, try_claim_skill_review_slot, summarize_traces_for_review, background_skill_review, extract_json_from_llm_response, is_transient_review_error, MAX_INFLIGHT_SKILL_REVIEWS} → pub(crate) (cross-submodule + tests) - mod.rs `enum ReviewError` → pub(crate) enum (private-interfaces lint once tools_and_skills::background_skill_review's signature went pub(crate)) Plus a cosmetic doc-paragraph rewrap in `llm_drivers.rs` to satisfy `clippy::doc_lazy_continuation`. Verified: cargo check / clippy -- -D warnings / cargo check --tests all green. * fix(kernel): repair four behavioural regressions from mod split (#4713 review) The kernel/mod split landed type-clean but silently dropped four behavioural hooks that `cargo check` / `clippy` could not see. All four fail open / fail-permissive (rather than fail-closed), which is why CI green doesn't catch them — they're only visible by exercising the boot path or the wiki tools at runtime. Restored to parity with `origin/main` (pre-split): - `boot.rs`: re-add `config.tool_exec.validate()` after the workspace-wide `config.validate()` warning loop. Without it, a deployment with `[tool_exec] kind = "ssh"` or `"daytona"` but missing the matching subtable boots silently and fails on first tool call instead of aborting at boot. - `boot.rs`: `MemorySubstrate::open_with_chunking(...)` → `open_with_pool_size(..., config.memory.pool_size)`. The 3-arg form forces the r2d2 default pool size of 8 regardless of operator config, so `[memory] pool_size` (just landed in #4685) was a no-op for the primary substrate; only the prompt store init below this line still honoured the setting, leaving the two SQLite pools inconsistently sized. - `spawn.rs::spawn_agent_inner`: re-add the `manifest.tool_exec_backend` override validation block after `validate_manifest_module_path`. Without it, an agent whose manifest pins `tool_exec_backend = "ssh"` against a daemon lacking `[tool_exec.ssh]` enters the registry and fails every tool invocation thereafter, instead of being rejected pre-registry. - `mod.rs` + `boot.rs` + `handles/wiki_access.rs`: re-add the `wiki_vault: Option<Arc<librefang_memory_wiki::WikiVault>>` field on `LibreFangKernel`, the boot-time `WikiVault::new` initializer with the same fail-open warning behaviour as main, the `wiki_vault: wiki_vault.clone()` in the struct construction, and the concrete `wiki_get` / `wiki_search` / `wiki_write` method bodies (ported verbatim from the pre-split mod.rs). Without these three pieces, every `tool_wiki_*` call on a deployment with `[memory_wiki] enabled = true` flowed to the trait default and returned `KernelOpError::unavailable("wiki_*")`, silently disabling the just-landed #3329 feature. Phase 1 had left an empty `impl WikiAccess for LibreFangKernel {}` with a header comment claiming \"will arrive with the #3329 main-side merge\" — but #3329 was already on `origin/main` (#4712) at the time of the branch. Verified locally: cargo check -p librefang-kernel --lib OK cargo clippy -p librefang-kernel --lib -- -D warnings 0 warnings rustfmt --check --edition 2021 (4 modified files) clean Refs Codex review on commits 594aa42 / 0107d06 / 79eb683 of the parent PR — findings #2-#5 (the four P2 items still open at HEAD) addressed here. Codex P1 finding #1 (cron SummarizeTrim) was addressed earlier in 0107d06 and is unaffected by this commit. * fix(kernel): repair phase 3 regressions on reload_config + NO_REPLY allow-list (#4713) Phase 3e/3 moved Kernel::reload_config into kernel/config_reload_ops.rs but the body that landed there was the pre-#4664 tolerant version: it called crate::config::load_config(...) (which silently returns KernelConfig::default() on parse / migration / deserialize / include failures) and wrapped validator errors as "Validation failed: ..." without the "live config unchanged" reload-boundary pledge. The strict-loader function crate::config::try_load_config and its 7 direct unit tests in config.rs are still present (orphaned), but the 4 api integration tests + 1 kernel integration test that drive reload_config end-to-end all fail because the strict path was never re-wired after the move. Restore parity with #4664: - swap load_config for try_load_config and propagate the Err through the same "Config reload failed; live config unchanged: {e}" wrapper - wrap the validator branch with the same prefix so every reload-rejection path satisfies the "live config unchanged" invariant the integration helper relies on Also: phase 3 split Kernel::cron_tick into a sibling kernel/cron_tick.rs file that carries one inline comment mentioning the NO_REPLY literal. The silent_response_single_source_of_truth grep-guard in librefang-runtime allow-lists the literal in selected files; add cron_tick.rs next to the existing mod.rs allow entry so the guard stops flagging it. Verified locally: kernel + api + runtime tests pass; clippy clean.

github-actions Bot added size/XL 1000+ lines changed area/runtime Agent loop, LLM drivers, WASM sandbox area/kernel Core kernel (scheduling, RBAC, workflows) labels May 6, 2026

houko and others added 5 commits May 6, 2026 13:03

Merge branch 'main' into refactor/sqlite-conn-pool

308f910

chore(codegen): auto-regenerate openapi.json + sdk + schema baselines…

4f5b8f2

… [skip ci]

houko commented May 6, 2026

View reviewed changes

houko and others added 5 commits May 6, 2026 18:34

Merge branch 'main' into refactor/sqlite-conn-pool

86ff2e9

Merge branch 'main' into refactor/sqlite-conn-pool

b3f1a8d

houko merged commit e5b9093 into main May 6, 2026
49 of 50 checks passed

houko deleted the refactor/sqlite-conn-pool branch May 6, 2026 23:40

This was referenced May 6, 2026

feat(kernel,memory): migrate workflow persistence to SQLite #4710

Closed

fix(api): expose cron_session_compaction_* + tool_exec via ui_sections_overlay #4723

Closed

houko mentioned this pull request May 7, 2026

refactor(kernel): split kernel/mod.rs into per-cluster files (#3744 phases 1-3) #4713

Merged

7 tasks

houko mentioned this pull request May 8, 2026

release: v2026.5.8-beta.10 #4802

Merged

DaBlitzStein mentioned this pull request May 9, 2026

feat(kernel,memory): persist workflow runs to SQLite #4838

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

refactor(memory): replace Arc<Mutex<Connection>> with r2d2 connection pool (#3378 part 2)#4685

refactor(memory): replace Arc<Mutex<Connection>> with r2d2 connection pool (#3378 part 2)#4685
houko merged 11 commits into
mainfrom
refactor/sqlite-conn-pool

houko commented May 6, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot commented May 6, 2026

Uh oh!

houko left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

houko commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Substantive changes

Regression tests

Verification

Risk audit

Out-of-scope follow-ups

CHANGELOG

Uh oh!

chatgpt-codex-connector Bot commented May 6, 2026

Uh oh!

houko left a comment

Choose a reason for hiding this comment

Top findings

[HIGH] runtime/src/audit.rs — Merkle chain tip ↔ DB INSERT no longer atomic under contention

[HIGH] No integration-level concurrency test for the kernel approval / audit DB

[MEDIUM] max_size=8 hardcoded; pool exhaustion silently degrades

Regression risk audit

pool.get() failure handling

Verdict

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

houko commented May 6, 2026 •

edited

Loading

[HIGH] `runtime/src/audit.rs` — Merkle chain `tip` ↔ DB INSERT no longer atomic under contention

[MEDIUM] `max_size=8` hardcoded; pool exhaustion silently degrades

`pool.get()` failure handling