fix(persistence): bound local write retries and set a busy timeout - #734
Merged
Merged
Conversation
The 2026-09-17 scale evidence recorded a 90 % error rate for eight concurrent writers on one local database, and 47 % even with a caller-side retry loop (plans/evidence/scale_2026_09_17/persistence_scale.json), because csm-persistence configured neither a lock timeout nor a retry. ADR-0095 requires persistence concurrency to have bounded retries/timeouts and to report them. - `persistence_retry`: `LOCAL_BUSY_TIMEOUT_MS = 5000`, `WRITE_RETRY_LIMIT = 5`, transient-error classification (`SQLITE_BUSY`/`SQLITE_LOCKED` families) and a deterministic 2-32 ms exponential backoff. - `connect()` sets `PRAGMA busy_timeout` on local connections, so a writer waits for a competing writer instead of failing immediately. - The idempotent write paths (`save_concept`, `save_concepts`, `save_association`) retry a transient failure within that budget by re-running their upsert transaction; non-idempotent statements are not retried. `tests/persistence_concurrency.rs` guards it: 8 writers x 25 single saves and 8 writers x (5-concept batch + 5 associations) must all succeed and stay readable.
Contributor
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 67 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
added 7 commits
September 17, 2026 22:41
…racking The inline retry loops pushed persistence.rs to 518 lines, over the 500-LOC gate that tests/arch_fitness.rs enforces for crates/*/src (the gate failed in both the lint and test jobs). - persistence_retry gains with_retry(): the three write paths call it instead of repeating the loop. - The AbsenceStore impl and its row helper move to persistence_absence.rs (132 lines), taking persistence.rs to 387.
The extracted module took the file-level csm_traits imports with it, so the in-file test module (and the orphaned async_trait attribute) needed repair.
…rison The 2026-09-17 artifact recorded the state before bounded retries existed. Rename it persistence_scale_pre_fix.json (+ its manifest) so the post-fix run in this PR can sit beside it and the before/after is visible.
…fest The manifest's dirty flag described the evidence files the run itself writes. Exclude plans/evidence from the git status probe so the flag answers the question that matters: was the measured code committed?
…ies in place Produced on commit 7bf03c6 (clean tree): error rate 0.905 -> 0.000 without caller retries and 0.470 -> 0.000 with them; writers wait instead of failing. The pre-fix artifact and manifest stay beside it for the comparison.
SonarCloud flagged the new renderer: two path-traversal vulnerabilities for taking a filesystem path from argv, and cognitive complexity 36 in render(). - Resolve the argument as a *name* under plans/evidence/ (validated against [A-Za-z0-9_-]+), so no caller-supplied path reaches a file operation. - Split render() into render_header/render_ann/render_persistence/ render_memory/render_contention.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the ADR-0095 persistence acceptance criterion that the new scale evidence showed failing: bounded retries/timeouts for concurrent local writes.
The measured problem
From
plans/evidence/scale_2026_09_17/persistence_scale.json(produced by #733): 8 concurrent writers × 25 round-trips on one local database →csm-persistenceconfigured neither a busy timeout nor a retry, so writers failed withdatabase is lockedimmediately. The evidence harness (andbenches/persistence_benchmark.rs) had to hand-roll a retry loop.The change
persistence_retry(new module):LOCAL_BUSY_TIMEOUT_MS = 5_000,WRITE_RETRY_LIMIT = 5, textual classification of the transientSQLITE_BUSY/SQLITE_LOCKEDfamilies (the driver exposes no extended result code), and a deterministic 2–32 ms exponential backoff.connect()issuesPRAGMA busy_timeout = 5000on local connections, so a writer waits for a competing writer instead of failing; bounded at 5 s.save_concept,save_concepts,save_association) re-run their upsert transaction within the retry budget. Non-idempotent statements are deliberately not retried.Both write entry points keep their existing signatures and error types; a failure that exhausts the budget still returns
MemoryError::database.Verification
tests/persistence_concurrency.rs: 8 writers × 25 single saves and 8 writers × (5-concept batch + 5 associations) must all succeed and remain readable — passes in 0.70 s (the same workload that failed 90 % of operations before).cargo clippy -p csm-persistence --all-featuresclean;cargo fmt --all -- --checkclean;validate-changelog.shpasses.Fixedentry recording the before/after error rate and pointing at the evidence artifact.Follow-up
Once #733 is merged this branch gets a re-run of the persistence artifact (
scripts/scale-evidence.sh persistence --tasks 8 --ops 25) so the committed evidence shows the post-fix numbers; the pre-fix JSON stays in the same directory for comparison.