Repository navigation
refactor(storage): run store writers and readers on one fixed pool - #4242
Conversation
Every attached SQLite file owned a writer thread and up to a budget of reader threads, about 244 threads for 30 projects at rest. Writers and readers are now tasks on one storage runtime of N tracedecay-store threads (TRACEDECAY_STORE_THREADS, default 6, clamped 2..16). The writer loop's channel wait, batch dwell and interactive exact-SQL transaction wait, the reader's command and snapshot waits, the deferred snapshot-end return and retire, and the requested checkpoint's wait for released snapshots all await instead of parking a thread. Synchronous waits that callers use refuse on a pool thread in debug builds, and a task keeps its own lock-work, VM and test-clock thread-locals.
…age-shared-executor
An async_channel receiver keeps its queued messages after it drops while any sender lives, unlike the std receivers the worker threads used. A caller whose command was still queued when a transaction expired or a snapshot ended then waited forever on a reply that would never come. Dropping the receiving end now discards them.
…age-shared-executor # Conflicts: # crates/tracedecay-rusqlite-runtime/src/reader/worker.rs
Busy-begin retries, WAL-recovery read retries and the writer's checkpoint timer now wait on the pool timer instead of sleeping the thread, and an early timer re-arms against the Instant deadline. Synchronous store waits (writer start, task join, writer replies, reader acquire/begin/spawn, wait_until) refuse on a pool thread in every build instead of parking it. native_runtime no longer mistakes a pool thread for a native one. TaskTransaction::new is unsafe with its contract at each call site, and a panicking poll restores the task's thread-locals.
The proof now runs alone in a child process and fails if 63 more stores grow the process by more than four threads, so an unnamed per-store thread fails it too. A second test shows a pool task's synchronous write and read are refused, not parked, and the write never runs.
…ared-executor # Conflicts: # crates/tracedecay-application/tests/application_suite/work_leaf_jobs.rs
Six threads capped SQL parallelism for a process with many stores opening at once: the tracedecay unit_test binary timed out cold opens in three of five runs. The default is now the available parallelism clamped to 6..=16; TRACEDECAY_STORE_THREADS still overrides it.
Also records the step 7 pool decisions in decisions.tsv.
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| pub fn store_threads() -> usize { | ||
| match std::env::var(STORE_THREADS_ENV) | ||
| .ok() | ||
| .and_then(|value| value.trim().parse::<usize>().ok()) | ||
| { | ||
| Some(threads) => threads.clamp(*STORE_THREAD_RANGE.start(), *STORE_THREAD_RANGE.end()), |
There was a problem hiding this comment.
🟡 Pool size reports the wrong count
When TRACEDECAY_STORE_THREADS changes after startup, store_threads() reports the new override instead of the running pool size. POOL retains the thread count chosen on first initialization.
Learn more
The pool is created once in pool, using store_threads() at creation time. Subsequent calls read the current environment again, but do not resize that pool. A caller using this public function to inspect pool capacity can therefore receive a false count.
Example: Start with TRACEDECAY_STORE_THREADS=2, open a store, then set the variable to 16. The pool still has two workers, while store_threads() returns 16.
Recommended fix: Cache the selected size alongside POOL, and make the public query return that selected size after initialization. Keep an internal configuration reader for the initial build if needed.
Was this helpful? React with 👍 or 👎 to provide feedback.
| Err(_) => Err(slot | ||
| .lock() | ||
| .unwrap_or_else(PoisonError::into_inner) | ||
| .take() | ||
| .expect("a refused task never ran its work")), |
There was a problem hiding this comment.
🟡 Pool task refusal can panic
If a refused task's slot is empty, spawn() panics instead of returning a typed failure. The new reader retry also uses expect for its transaction, violating the repository's production-failure rule.
Learn more
Repository guidance forbids new production expect calls and requires typed failures. A rejected spawn currently asserts that the saved future remains in its slot. The new reader retry makes a similar assertion about its active transaction at read_across_wal_recovery. These are internal invariants, but an invariant failure must not turn a storage failure into a process-level panic.
Example: If task rejection occurs after the saved future was taken, the spawn path panics before it can report that the runtime is unavailable.
Recommended fix: Make rejection and transaction lookup return typed errors, or structure the ownership so the invalid state is unrepresentable without adding an expect.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Why
Every attached SQLite file owned one writer thread and up to three reader threads, so 30 mounted projects cost about 244 threads at rest. Step 7 of the storage redesign (docs/plans/storage/README.md) runs every store on one fixed pool, so the thread count no longer grows with the number of files. This is the pool slice of that step. The fenced TRUNCATE, idle close, unlink probe and
ProfileLostfollow in later slices.Scope
storage_pool(new) runs one asupersync multi-thread runtime oftracedecay-store-Nthreads with no blocking pool. N is the host's available parallelism clamped to 6..=16;TRACEDECAY_STORE_THREADSoverrides it (clamped to 2..=16).Workerand each reader worker are pool tasks. Idle waits, batch dwell, snapshot polls, busy-begin retries and WAL-recovery prepare retries are awaits on the pool timer. Deferred snapshot ends run as pool tasks instead of-reader-returnand-reader-retirethreads.drain, andshutdown_and_join. Work the pool refuses to spawn is adopted by the task that is polling, so it is never dropped.native_runtime::handle,cxandon_native_threadtreat a pool thread as not native.Tradeoffs
The design's default of 6 threads failed measurement. At N=6 the
//crates/tracedecay:unit_testbinary, which opens many stores at once in one process, timed out cold opens in 3 of 5 runs and ran 24% slower than master. At N=16 it passed 3 of 3. No thread is reserved for foreground classes yet, because today's files mix classes (Work sharessessions.dbwith observation ingest); the reservation lands with the step 8 partition.Blast Radius
Every store open, read and write in the daemon and CLI now runs on the pool. A long statement holds a pool thread for its duration, so N concurrent long statements delay other stores until a thread frees.
Verification
sixty_four_attached_stores_run_on_the_fixed_storage_pool: 64 attached stores add exactly N threads, all of them pool threads. Mutation: one writer on an unnamedstd::threadfails it.a_writer_query_rides_out_a_file_another_connection_briefly_locksfor the WAL-recovery retry. Each fix was removed in turn and its test failed.//crates/tracedecay:unit_test--runs_per_test=3 --nocache_test_results: master 177 s avg, this head 171 s avg, 3 of 3 pass.//crates/tracedecayunit_test, daemon_suite, mcp_suite and session_suite all pass. Clippy (--config=clippy) and rustfmt_check_2024 pass on both touched crates.