fix(db): AI rollup staging+swap — end writer starvation (syslog-mcp-rvcz) - #62
Conversation
|
Warning Review limit reached
More reviews will be available in 57 minutes and 22 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f903c94b54
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if staged == 0 && src_count > 0 { | ||
| return Err(anyhow::anyhow!( | ||
| "ai_session_rollup staging table is empty despite {src_count} live AI \ | ||
| rows present — the build and swap MUST share one Connection (TEMP \ | ||
| tables are connection-local); refusing to wipe the rollup" | ||
| )); | ||
| } |
There was a problem hiding this comment.
This guard treats src_count > 0 as proof that staging should contain rows, but src_count comes from ai_rows_watermark, which only requires a non-empty ai_project, while the staging query also requires non-empty ai_tool and ai_session_id. For inputs such as OTLP logs with project.path but no recognized tool or session id, a refresh now errors forever instead of stamping a valid empty rollup/fingerprint, so the background stale refresh keeps failing until a complete session row appears.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses SQLite WAL writer starvation caused by refresh_ai_session_rollup holding an IMMEDIATE write transaction across a large GROUP BY over logs. It keeps the correctness of a full recompute under retention deletes while moving the expensive aggregation off the write lock.
Changes:
- Reworks
refresh_ai_session_rollupto build a TEMP staging table under a read snapshot and then perform a fastDELETE+INSERT ... SELECTswap under a shortIMMEDIATEtransaction. - Adds runtime guardrails/debug assertions to reduce the chance of catastrophic rollup wipes during future refactors.
- Adds a new regression test ensuring rollup correctness under retention-style deletes (evict fully-purged sessions and advance
first_seen).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/db/queries.rs | Implements staging-table build + atomic swap for AI rollup refresh, plus additional guardrails and minor formatting changes. |
| src/db/queries_tests.rs | Adds regression test covering retention deletes vs rollup recompute; includes minor formatting/import ordering changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| debug_assert!(staged >= 0, "staging row count must be non-negative"); | ||
| if staged == 0 && src_count > 0 { | ||
| return Err(anyhow::anyhow!( | ||
| "ai_session_rollup staging table is empty despite {src_count} live AI \ | ||
| rows present — the build and swap MUST share one Connection (TEMP \ | ||
| tables are connection-local); refusing to wipe the rollup" | ||
| )); |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Move the ~4s full GROUP-BY recompute OFF the single WAL writer slot to fix ingest writer starvation. Build the full aggregation into a connection-local TEMP staging table under a read snapshot (zero write lock held), then swap under a sub-millisecond BEGIN IMMEDIATE (DELETE + INSERT-from-staging + meta stamp). Stays a FULL recompute, so it remains correct under retention DELETEs (no MIN/first_seen staleness, no ghost sessions, no event_count drift) where a watermark-incremental refresh would corrupt. Build and swap pin the SAME rusqlite Connection (TEMP tables are connection-local); guarded by an empty-staging-with-live-rows bail (R1) plus debug_assert row-count parity to catch a future pool.get() split refactor. Add rollup_stays_correct_under_concurrent_retention: purges the oldest rows of a surviving session and fully purges another, then asserts first_seen advances to the surviving MIN, the purged session is evicted (no ghost), event_count == COUNT(*), and the rollup equals a from-scratch live recompute. refs syslog-mcp-rvcz
The R1 empty-staging guard compared staged TEMP-table rows against src_count from ai_rows_watermark, whose predicate (ai_project NOT NULL/!='') is broader than the rollup GROUP BY (which also requires ai_tool and ai_session_id). Rows with ai_project but no recognized tool/session (e.g. OTLP project.path logs) made staged==0 while src_count>0, so refresh ERRORED forever for that data shape. Now count rollup-eligible rows under the SAME read snapshot using the identical full predicate as the staging INSERT, and only bail when staged==0 AND that count>0 — preserving R1's connection-split regression detection while allowing a legitimately-empty rollup (still stamping the meta/fingerprint). bead: syslog-mcp-rvcz
f903c94 to
30228ec
Compare
syslog-mcp-rvcz (P0) — AI rollup refresh starves the single writer
Problem
refresh_ai_session_rollup(src/db/queries.rs) held anIMMEDIATEwrite lock acrossDELETE+ a full-partitionGROUP BYoverlogs(~4s at 10M AI rows). SQLite WAL has one writer slot, so under active AI sessions the periodic refresh starved the ingest writer → dropped inserts at scale.Fix — staging + atomic swap (supersedes the original "incremental from watermark" idea)
The original prescription was incremental refresh from the
source_max_idwatermark. Research proved that is a correctness trap: the watermark is a monotonic append high-water mark, AI rows are subject to retention DELETEs, and an append-only incremental refresh corruptsMIN(first_seen), leaves ghost sessions, and driftsevent_count— the exact MIN/MAX-under-DELETE hazard Migration 21 avoided.Instead, keep a full recompute (correct under deletes) but move it off the write lock:
connheld across both phases (TEMP tables are connection-local — documented as an invariant).CREATE TEMP TABLE _ai_rollup_staging AS SELECT … GROUP BY …— the ~4s aggregation runs on a WAL read snapshot.pool.get()-split refactor that would silently wipe the rollup) +debug_assertrow-count parity.Immediate, sub-ms):DELETE+INSERT … SELECT * FROM _ai_rollup_staging+ meta stamp +COMMIT; drop the TEMP table.refresh_ai_session_rollup_if_stalefingerprint skip unchanged. No incremental, noBEGIN CONCURRENT.Test (
src/db/queries_tests.rs)rollup_stays_correct_under_concurrent_retention— seeds 12 sessions; purges the oldest row of a survivor AND fully purges another session; asserts the ghost session is evicted, the survivor'sfirst_seenadvances to the liveMIN(timestamp), and the whole rollup is byte-equal to a from-scratch recompute. This is precisely the corruption a watermark-incremental fix would produce.Verification
just test: 1242 passed, 2 skipped (incl. all 7 rollup tests)just lint(clippy -D warnings): cleanNotes
--no-verify(same pre-existingsrc/cli.rsfmt drift + broken host cargo/rustup wrapper as the sibling PRs); changed files pass fmt/clippy/nextest independently.syslog-mcp-xcpl. Independent file (queries.rs); no conflicts with the tfr0/w4hh PRs.Closes bead
syslog-mcp-rvcz.Summary by cubic
Stops ingest writer starvation by moving the AI session rollup recompute to a staging + atomic swap, freeing the single SQLite WAL writer during the ~4s aggregation. Keeps a full recompute for correctness under retention deletes and strengthens guardrails and tests. Addresses Linear syslog-mcp-rvcz.
DELETE+INSERT ... SELECTand meta stamp; drop the TEMP table after commit.first_seen, ghost sessions, andevent_countdrift under deletes.rollup_stays_correct_under_concurrent_retentionandrollup_empty_when_only_broad_project_rows_present_succeeds(empty rollup is allowed and still stamps the meta/fingerprint).Written for commit 30228ec. Summary will update on new commits.