cr-sqlite retention + paced deletes, and a reproduced CRR rollout wedge - #1063
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
📝 WalkthroughWalkthroughThe PR adds bounded pruning for AI usage and selected event logs, integrates the operations with storage maintenance and the journal, skips excluded incoming tables, and expands mobile outbound changeset exclusions. ChangesRetention API and database maintenance
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
0d5f32f to
f81574e
Compare
|
@codex review |
![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: f81574e8e1
ℹ️ 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".
| } catch (error) { | ||
| logger.warn("db.maintenance_failed", { | ||
| action, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| }); | ||
| return unsupportedMaintenanceResult(); |
There was a problem hiding this comment.
When a batched prune encounters an operational SQLite error—such as a lock, disk failure, or malformed table—the new async wrapper converts it to skippedReason: "unsupported" instead of rethrowing. runStep therefore records no error, so the maintenance journal, analytics, and Settings UI report a completed/tidy run; for pruneEventLogs, earlier table batches may already have committed while their affected-row count is also discarded. Preserve genuine unsupported results for missing tables, but let operational failures reach runStep so the partial failure is reported.
AGENTS.md reference: AGENTS.md:L61-L62
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 65f67df360
ℹ️ 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".
| pruneAiUsageLog: () => runMaintenanceSafelyAsync("pruneAiUsageLog", async () => { | ||
| if (!rawHasTable(db, "ai_usage_log")) return unsupportedMaintenanceResult(); | ||
| const cutoff = new Date( | ||
| Date.now() - AI_USAGE_LOG_RETENTION_DAYS * 24 * 60 * 60 * 1_000, |
There was a problem hiding this comment.
For projects with more than 90 days of AI activity, the storage doctor now irreversibly deletes the only database rows used by collectAdeDatabaseUsageStats to calculate tracked tokens, calls, duration, provider/model totals, features, and active dates. The Stats UI still offers both Year and All ranges—and describes the latter as “lifetime”—so after the first sweep those ranges silently report only the retained 90-day subset. Preserve durable daily aggregates before deleting these rows, or constrain and relabel the affected ranges so the displayed period remains accurate.
AGENTS.md reference: AGENTS.md:L61-L62
Useful? React with 👍 / 👎.
b92f142 to
5cbf440
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/ade-cli/src/services/sync/syncHostService.ts (1)
316-339: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd named mobile sync regression coverage for the new outbound exclusions.
The new table names are in the mobile outbound filter and also affect the mobile replica reseed cache, but the current test only covers an already-empty filtered batch. Add cases that include mobile rows for these tables and unrelated mobile rows so the omitting behavior and still-synced table behavior are both asserted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/ade-cli/src/services/sync/syncHostService.ts` around lines 316 - 339, Add named mobile sync regression tests covering all newly excluded tables in the outbound filter and mobile replica reseed cache. Include batches containing rows from each excluded table alongside rows from an allowed table, then assert excluded rows are omitted while unrelated allowed rows still synchronize; retain coverage for the empty filtered-batch case.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/ade-cli/src/services/sync/syncHostService.ts`:
- Around line 316-339: Add named mobile sync regression tests covering all newly
excluded tables in the outbound filter and mobile replica reseed cache. Include
batches containing rows from each excluded table alongside rows from an allowed
table, then assert excluded rows are omitted while unrelated allowed rows still
synchronize; retain coverage for the empty filtered-batch case.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ae1f4fed-f89d-449f-a854-44d5f0e647ef
⛔ Files ignored due to path filters (3)
docs/features/storage-and-recovery/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/README.mdis excluded by!docs/**docs/features/sync-and-multi-device/crdt-model.mdis excluded by!docs/**
📒 Files selected for processing (9)
apps/ade-cli/src/services/sync/syncHostService.tsapps/desktop/src/main/services/ai/aiIntegrationService.tsapps/desktop/src/main/services/state/dbMaintenanceApi.test.tsapps/desktop/src/main/services/state/dbMaintenanceApi.tsapps/desktop/src/main/services/state/kvDb.test.tsapps/desktop/src/main/services/state/kvDb.tsapps/desktop/src/main/services/storage/storageInsightsService.tsapps/desktop/src/main/services/storage/storageLedger.tsapps/desktop/src/renderer/components/settings/storage/storageView.ts
|
Good catch — confirmed and fixed by removing the retention rather than working around it.
Event-log retention stands: none of those four tables feeds a lifetime-labelled surface. |
The research framed this as "make cr-sqlite tombstone compaction run on paired machines." Investigating it produced the opposite conclusion, and the measurements are why. **Tombstones are under 4% of the metadata.** On the live 28.7 MB project DB, `__crsql_clock`/`__crsql_pks` is 5.7 MB (20%) — but ~1,950 of ~53,000 clock rows are tombstones. `ai_usage_log`, the single worst table at 0.93 MB of metadata for 0.27 MB of data, has **exactly zero**. The bloat is structural — one clock row per live row per non-PK column — not garbage. A perfect compaction would recover ~220 KB of 28.7 MB. **And it cannot be made safe.** Reproduced against the vendored crsqlite extension: `rebuildCrrTableWithBackfill` drops and recreates the clock table, resetting `col_version` 3→1 and destroying `cl=-1` tombstones. A peer that has not acked then rejects the host's push as older and diverges permanently, and a row the host deleted is resurrected by the peer pushing it back. Silently, with no error and no resync. A watermark would not fix it either: no durable host-owned per-peer ack version exists (the number lives on the client), and peer seeding replays `crsql_changes` from version 0, so any clock row removed is absent from every future seed. **The `has_peers` guard is therefore correct and is left exactly as it is.** The measured 4% figure is now in the docs so nobody re-litigates it. What this actually ships: - **A reproduced rollout wedge, fixed.** `applyChanges` threw `could not find the schema information` for a table that is local-only here but still a CRR on a peer running an older build. That rolls back the whole `BEGIN IMMEDIATE` — and a batch is ordered by db_version, so one usage row took that peer's chats, lanes, and commits with it. The peer's outbound cursor only advances on an ok ack, so it re-exported the same poisoned range forever. Inbound changes for local-only tables are now skipped. This latent hazard fires on any future local-only move. - **Paced deletes.** Every existing prune is one unbounded DELETE on a fully synchronous driver. `pruneRowsInBatches` does 2,000 rows per batch with a `setImmediate` yield between them, bounded at 200 batches. - **Retention** for `ai_usage_log` (90d) and four event logs (30d), wired into the doctor sweep and registered in the storage ledger so Settings shows a real policy chip instead of "Ai usage log". - **Mobile exclusion** for all seven event logs — an outbound filter only, with zero Swift read paths (they exist solely in DatabaseBootstrap.sql). Three of the seven proposed tables are deliberately NOT pruned, each documented: `linear_ingress_events` is the webhook replay guard (pruning it re-dispatches automations — re-running agent work and re-posting Linear comments), `cto_session_logs` is two-way reconciled from an append-only jsonl so a prune is undone by the next read while writing more CRR metadata, and `worker_agent_runs` is a lifecycle table whose prune would delete pending runs and orphan live references. Measured on a copy of the real DB: ai_usage_log 2,235 -> 1,313 rows, its clock rows 20,115 -> 12,739 (-37%), 688 KB off the file after vacuum. The clock arithmetic confirms the model exactly — 922 rows x 9 columns = 8,298 clock rows removed, 922 tombstones added, net -7,376. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Scoping the "make it local-only, serve the budget from a slim synced aggregate" plan turned up three things that make it larger than it looks, so the raw table keeps replicating and only retention ships. The aggregate as originally specified would not have worked: `ai.budgets.<feature>.dailyLimit` is enforced by counting *successful requests per feature* for today, not spend or tokens, and not per provider. It also has to be keyed (day, feature, site) and summed at read. A shared (day, feature) counter cannot work — cr-sqlite is last-writer-wins per column, so one machine's upsert discards another's count, which is the same silent under-count by a different route. And it cannot ship in one release. The moment raw rows stop replicating, a machine on the new build sees nothing from a peer still on the old one — the inbound local-only skip added in the previous commit drops those changes — so it under-counts and overruns the account cap during exactly the window a rollout guarantees. Safe sequencing is two releases. Recorded at both places someone would act on it: beside the exclusion set in kvDb, and above countDailyUsage, whose row count is what makes the cap account-wide in the first place. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`runMaintenanceSafelyAsync` swallowed any error into an `unsupported` result, copying the synchronous wrapper's shape. But "unsupported" means "this handle does not implement the step" — reporting a locked database or a malformed table that way makes the maintenance journal, the analytics event, and the Settings run summary all show a tidy completed sweep, and for the multi-table `pruneEventLogs` it hides a partial sweep entirely. `runStep` already catches and records `error` per action, so rethrowing is what surfaces it, and a failing step still does not abort the rest of the run. Rows deleted before the throw stay deleted and the next sweep continues from there. Genuinely-unsupported handles still report `unsupported`; the test pins both halves. Raised independently by Greptile and Codex. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ActivityModule` defaults to the All range and renders a "lifetime tokens" total computed from `ai_usage_log`, and the Stats surface offers a Year range too. A 90-day prune would have silently turned both into trailing-90-day figures after the first doctor sweep — the same class of quiet behavior loss as the per-machine budget, and equally invisible. So `ai_usage_log` now gets neither the CRR exclusion nor retention in this PR. Both wait on the durable daily aggregate; whoever builds it can add retention safely in the same change, because the aggregate is what makes a lifetime total survive pruning the raw rows. Event-log retention is unaffected — none of those four tables feeds a lifetime-labelled surface. Raised by Codex. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bf8209b to
ca1d97e
Compare
1 participant
Item 3 of three hygiene fixes from the 2026-08-09 t3code research (§3.3). Stacked on #1061; shares no files with #1062.
This PR does not do what the research asked for, and the measurements are why. The ask was "make
compactCrsqlTombstonesrun on paired machines." Investigating it produced the opposite conclusion.Why the proposed fix was dropped
Tombstones are under 4% of the metadata. On the live 28.7 MB project DB,
__crsql_clock/__crsql_pksreally is 5.7 MB (20% — the research's number is right). But only ~1,950 of ~53,000 clock rows are tombstones.ai_usage_log, the single worst table at 0.93 MB metadata for 0.27 MB data (3.5×, exactly as reported), has zero. The bloat is structural — one clock row per live row per non-PK column — not garbage awaiting collection. A perfect, safe compaction would recover ~220 KB of 28.7 MB.And it cannot be made safe. Reproduced against the repo's own vendored
crsqlite.dylib:rebuildCrrTableWithBackfilldrops and recreates the clock table. Measured:col_version3 → 1,cl2 → 1, thecol_name='-1'tombstone gone,db_versionmoved forward, site id unchanged.col_version=6, peer holding its own write at 4. Control run converges. With compaction the host pushescol_version=1, the peer rejects it as older, and the two never converge again. No error, no resync.A watermark would not rescue it. There is no durable host-owned per-peer ack version —
PeerState.lastKnownServerDbVersionis in-memory, initialized to0, and the authoritative copy lives on the client (hello.peer.dbVersionBySite). And peer seeding replayscrsql_changesfrom version 0 (mobileReplicaReseed.ts), so any clock row removed is permanently absent from every future seed — the watermark describes existing peers and says nothing about ones that don't exist yet.So
has_peersstays exactly as it is. It is correctly polarized (it means "ever paired", not "currently connected", and fails closed). The measured 4% figure is now indocs/features/storage-and-recovery/README.mdso nobody re-litigates removing the guard for a 220 KB prize.What this actually ships
A reproduced rollout wedge, fixed
applyChangesthrewcrsql - could not find the schema information for table Xwhen a peer on an older build sent changes for a table this machine has since made local-only. The base table still exists, so the "unknown table" guard passes. That throw rolls back the entireBEGIN IMMEDIATE— and a batch is ordered bydb_version, so one usage row took that peer's chats, lanes, and commits down with it. The peer's outbound cursor only advances on anok:trueack, so it re-exported the same poisoned range forever; on restart the un-acked window was silently skipped instead.Inbound changes for local-only tables are now skipped. This is latent for every future local-only move, not just this one — which is exactly why it lands here.
Paced deletes
Every existing prune is a single unbounded
DELETEon a fully synchronous driver (node:sqliteDatabaseSync), so a large one stalls UI, IPC, and the sync pump for its full duration.pruneRowsInBatchesdoes 2,000 rows per batch with asetImmediateyield between them, bounded at 200 batches so one sweep cannot run away.Retention, and three tables deliberately left alone
Retained:
ai_usage_log(90d),linear_sync_events,linear_workflow_run_events,worker_agent_cost_events,pack_events(30d). Registered inSTORAGE_LEDGER+LEDGER_LABELS, so Settings shows a real policy chip rather than falling through to dev-shaped copy ("Ai usage log").Not retained, each documented in code:
linear_ingress_eventspersistRecordrefuses to dispatch adelivery_idit has stored, and the cursor is explicitly reset oncursorExpired. Pruning it lets a backlog drain re-dispatch automations — re-running agent work and re-posting Linear comments.cto_session_logs.ade/cto/sessions.jsonlwith no retention of its own. Every prune is undone by the next CTO read, and each cycle writes a tombstone plus a fresh set of clock rows — net negative on the exact metric this PR exists to improve.worker_agent_runscreated_atwould delete a still-pendingrun and orphanworker_agent_cost_events.run_idandautomation_runs.worker_run_id.worker_agent_cost_eventskeys oncreated_at, notoccurred_at— the latter is event time and can be backdated, which would age a row out the moment it is written.Timestamps are matched with
like '____-__-__%'. On atext-affinity column an epoch number stores as its digits and'1767225600000' < '2026-…'is true as a string, and CRR repair appendsdefault ''to NOT NULL text columns — so both a numeric and a defaulted timestamp would otherwise be born expired.Mobile exclusion
All seven event logs added to
MOBILE_CHANGESET_EXCLUDED_TABLES— verified zero Swift read paths (they exist only inDatabaseBootstrap.sql). This is an outbound filter only: it never touches CRR metadata, so unlike a local-only conversion it carries none of the apply hazard above.Measured (copy of the real project DB, through ADE's own prune path)
ai_usage_logrows__crsql_clockrowsThe clock arithmetic confirms the model exactly: 922 rows × 9 non-PK columns = 8,298 clock rows removed, 922 tombstones added, net −7,376. Deletes on a CRR table are a net win, not "worse before better" — worth stating because the research assumed the opposite.
ai_usage_logstays synced — decision recordedThe local-only conversion was approved conditionally, paired with a slim synced daily aggregate so
dailyLimitwould stay account-wide. Scoping it against that condition, it falls back: the aggregate is not a small, low-risk change, and the forgone win is not worth loosening a cost control.What syncs, what doesn't, what
dailyLimitreads — unchanged by this PR:ai_usage_lograw rowsMOBILE_CHANGESET_EXCLUDED_TABLES)ai.budgets.<feature>.dailyLimitcount(*) where feature = ? and success = 1for today, summed across every machine on the accountWhy the aggregate didn't fit in one change. Three findings, in order of how much they cost:
(day, provider).dailyLimitcounts successful requests perfeature— not spend, not tokens, not keyed on provider. Building it as specified would have preserved the wrong number.(day, feature, site)and summed at read. cr-sqlite is last-writer-wins per column, so one machine upserting a shared(day, feature)total discards the other's — silently under-counting, which is the same loosening by a different route.applyChangesfix in this very PR — so it under-counts and overruns the account cap during exactly the window a rollout guarantees. There is no fail-stricter reading available: the data simply isn't there to be strict about. Safe sequencing is two releases (ship the aggregate while the table still replicates, then flip).A fourth, smaller cost: a new CRR table must exist in every peer's schema or
unknown_sync_tablewedges apply — the same hazard this PR fixes.Forgone win: ~1.2 MB of CRR metadata, ~2.8 MB off the file after vacuum, roughly 10% of the database. Recorded in code at both places someone would act on it (beside the exclusion set in
kvDb.ts, and abovecountDailyUsage) and indocs/features/storage-and-recovery/README.md, with the corrected(day, feature, site)design and the two-release sequencing, so the planned cross-machine-usage initiative can pick it up properly rather than rediscover it.Verification
/quality: 1 Blocker (reproduced), 1 High, 5 Medium, 2 Low. All fixed except the gated item above. The High and two Mediums were tables I had wrongly included in retention.crdt-model.md(the local-only skip is now a stated CRR invariant), sync README.validate-docs.mjs: 228 files.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Greptile Summary
The PR adds paced retention for selected replicated event logs, excludes unread event tables from mobile changesets, and safely ignores inbound changes for tables converted to local-only storage. It also updates storage policy presentation and documents the CRR rollout and compaction constraints.
Confidence Score: 5/5
The PR appears safe to merge.
No blocking failure remains; the previously reported event-log pruning error is now propagated and recorded as a failed maintenance action, while the AI usage-log pruning path was removed.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Storage maintenance sweep] --> B[Prune retained event logs] B --> C[Delete up to 2,000 eligible rows] C --> D{Batch full?} D -->|Yes| E[Yield to event loop] E --> C D -->|No| F[Record successful result] C -->|SQLite error| G[Propagate error] G --> H[Record failed maintenance action]Reviews (8): Last reviewed commit: "review: drop ai_usage_log retention — it..." | Re-trigger Greptile
Context used:
ade codeTUI