Fix synced queue wipe replication and preserve dirty file session buffers#372
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@copilot review but do not make fixes |
arul28
force-pushed
the
cursor/critical-correctness-bugs-0cf0
branch
from
08f4177 to
020bdfd
Compare
|
@copilot review but do not make fixes |
Reviewed the last two commits without making fixes. Findings: no blocking issues found in the desktop/iOS queue-wipe + Files session-buffer changes. Validation run:
|
|
@copilot review but do not make fixes |
…fers The stacked-PR queue overhaul migration deleted queue_landing_state while CRR triggers were active, replicating DELETE tombstones to peers that had not upgraded yet. Strip CRR metadata before the one-shot local wipe and run it after migrations but before ensureCrrTables re-registers replication. Files tab session caching dropped unsaved buffers over 8 MiB when switching project/lane scope. Always retain dirty tabs in the session snapshot so large in-flight edits are not lost silently on tab reentry. Co-authored-by: Arul Sharma <arul28@users.noreply.github.com>
arul28
force-pushed
the
cursor/critical-correctness-bugs-0cf0
branch
from
b6d218a to
73fd643
Compare
|
@copilot review but do not make fixes |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review
Scope: 9 file(s), +446 / −53
Verdict: Minor issues
This PR stops the stacked-PR queue overhaul wipe from replicating over CRDT (local-only delete + filtered kv marker on desktop and iOS) and keeps unsaved Files tab buffers when switching lane scopes by dropping the dirty-tab size cap. The sync changes are well tested; the only notable tradeoff is unbounded in-memory retention for very large dirty edits.
⚡ Performance
[Low] Dirty session cache no longer caps oversized unsaved buffers
File: apps/desktop/src/renderer/components/files/FilesPage.tsx:L248-L257
Issue: cacheableSessionTabs now keeps every dirty tab regardless of size; only clean text tabs remain capped at 256 KiB. Scope eviction still respects unsaved tabs (MAX_FILES_PAGE_CACHED_SCOPES = 8), so several multi‑MiB dirty buffers can stay in the renderer at once.
Impact: Lane switching with multiple large unsaved files (the new test uses ~8 MiB + 1 char) can increase memory use and GC pressure compared with the previous 8 MiB dirty cap.
Fix: If needed, keep “never drop dirty edits” but bound cost—e.g. a higher dirty ceiling, spill oversized dirty content to dirtyWorkspaceBuffers, or retain only dirty metadata in the session cache and reload from the spill store on scope return.
Notes
- Desktop and iOS both run the queue wipe after bootstrap/migrations and before (re)enabling CRR, filter the marker on export/apply, and have targeted tests—good alignment for a local-only migration.
- Mixed-version rollout: upgraded peers no longer replicate wipe tombstones; peers still on an older build can still push existing
queue_landing_staterows until they upgrade—consistent with protecting not-yet-upgraded devices from a one-shot delete. - Removing
delete from queue_landing_statefrom iOSDatabaseBootstrap.sqlavoids a bootstrap-time wipe that could have been captured for sync on first open.
Sent by Cursor Automation: BUGBOT in Versic
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| dbVersion: versionBeforeApply + 1, | ||
| siteId: "ffffffffffffffffffffffffffffffff", | ||
| cl: 1, |
There was a problem hiding this comment.
"\(marker)" is interpolated directly into the SQL string. marker is a compile-time constant today so there is no injection risk, but the pattern could be copied into helpers that use dynamic values. Binding the key via executeSqlForTesting with a parameter slot (or using the existing kvValue helper to read back and a separate raw-SQL exec to write) would keep the test consistent with the parameterized style used everywhere else in this test suite.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/ios/ADETests/ADETests.swift
Line: 2972-2974
Comment:
**SQL string interpolation in test helper**
`"\(marker)"` is interpolated directly into the SQL string. `marker` is a compile-time constant today so there is no injection risk, but the pattern could be copied into helpers that use dynamic values. Binding the key via `executeSqlForTesting` with a parameter slot (or using the existing `kvValue` helper to read back and a separate raw-SQL exec to write) would keep the test consistent with the parameterized style used everywhere else in this test suite.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| /** Strip CRR triggers/metadata so row deletes stay local (no replicated tombstones). */ | ||
| function deleteAllRowsWithoutCrrReplication( | ||
| db: DatabaseSyncType, | ||
| tableName: string, | ||
| logger?: Logger, | ||
| ): void { | ||
| if (!rawHasTable(db, tableName)) return; | ||
|
|
||
| const clockTableName = `${tableName}__crsql_clock`; | ||
| const pksTableName = `${tableName}__crsql_pks`; | ||
| if (rawHasTable(db, clockTableName) || rawHasTable(db, pksTableName) || listCrrTriggers(db, tableName).length > 0) { | ||
| if (rawHasTable(db, "crsql_master") && rawHasColumn(db, "crsql_master", "tbl_name")) { | ||
| runStatement(db, "delete from crsql_master where tbl_name = ?", [tableName]); | ||
| } | ||
| if (rawHasTable(db, "crsql_changes") && rawHasColumn(db, "crsql_changes", "table")) { | ||
| runStatement(db, "delete from crsql_changes where [table] = ?", [tableName]); | ||
| } | ||
| try { | ||
| getRow(db, "select crsql_as_table(?) as ok", [tableName]); | ||
| } catch { | ||
| // Table may not be registered enough for crsql_as_table; shadow cleanup below still applies. | ||
| } | ||
| dropCrrTriggers(db, tableName, logger); | ||
| runStatement(db, `drop table if exists ${quoteIdentifier(clockTableName)}`); | ||
| runStatement(db, `drop table if exists ${quoteIdentifier(pksTableName)}`); | ||
| } | ||
|
|
||
| runStatement(db, `delete from ${quoteIdentifier(tableName)}`); |
There was a problem hiding this comment.
crsql_master/crsql_changes cleanup gated behind shadow-table presence
deleteAllRowsWithoutCrrReplication enters the CRR-cleanup branch only when a shadow table (__crsql_clock / __crsql_pks) or a CRR trigger is found. If a previous partial initialization left entries only in crsql_master (no shadow tables, no triggers), those entries survive the wipe and queue_landing_state rows deleted afterward could still be associated with stale master metadata. The sibling removeExcludedCrrMetadata function checks hasMasterRows independently — the same independent pre-check would make this function fully symmetric with it. Very unlikely in practice, but worth aligning for correctness.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/main/services/state/kvDb.ts
Line: 524-551
Comment:
**`crsql_master`/`crsql_changes` cleanup gated behind shadow-table presence**
`deleteAllRowsWithoutCrrReplication` enters the CRR-cleanup branch only when a shadow table (`__crsql_clock` / `__crsql_pks`) or a CRR trigger is found. If a previous partial initialization left entries only in `crsql_master` (no shadow tables, no triggers), those entries survive the wipe and `queue_landing_state` rows deleted afterward could still be associated with stale master metadata. The sibling `removeExcludedCrrMetadata` function checks `hasMasterRows` independently — the same independent pre-check would make this function fully symmetric with it. Very unlikely in practice, but worth aligning for correctness.
How can I resolve this? If you propose a fix, please make it concise.
Bug and impact
Synced PR queue wipe deleted in-flight queue state on peers that had not upgraded
The stacked-PR
queue_landing_stateoverhaul migration ranDELETEwhile CRR triggers were active, so upgrading Mac A replicated DELETE tombstones to Mac B before B ran the migration. B could lose an active merge-queue landing state without upgrading.Files tab silently dropped large unsaved buffers on project/lane reentry
Session caching excluded dirty tabs over 8 MiB when snapshotting scope changes. Switching tabs/lanes and returning restored a session without those tabs;
replaceDirtyBuffersForWorkspacethen cleared the in-memory dirty map, so unsaved edits were lost with no prompt.Root cause
migrate()while cr-sqlite was loaded andqueue_landing_statewas already CRR-registered from prior opens.cacheableSessionTabs()applied the 8 MiB cap to dirty tabs for memory bounds, but session restore treated the filtered list as authoritative.Fix
queue_landing_statelocally, then re-register CRR inensureCrrTables(wipe runs after migrations, before CRR marking).Validation
npx vitest run src/main/services/state/kvDb.migrations.test.ts(pass)kvDb.sync.test.tsadds regression coverage when cr-sqlite is available (macOS CI)Does not overlap open draft PRs #363–#371 (CRR retrofit, orchestration, App Control routing).
Greptile Summary
This PR fixes two correctness bugs: a CRR replication hazard where the
queue_landing_statewipe ran insidemigrate()while cr-sqlite triggers were active (causing DELETE tombstones to propagate to unupgraded peers), and a session-cache bug where dirty tabs over 8 MiB were silently dropped when switching lane scopes.migrate()into a newwipeQueueLandingStateForStackedOverhaulIfNeededfunction that runs post-migration and pre-ensureCrrTables, manually strips CRR metadata before deleting rows, and filters the upgrade marker key from both CRDT export and import on desktop and iOS.cacheableSessionTabsnow always retains dirty tabs regardless of size; the 8 MiB cap is removed entirely, while the 256 KB cap for clean tabs remains unchanged. Both fixes ship with direct regression tests covering the failure scenarios described in the PR.Confidence Score: 4/5
The sync fix is well-reasoned and the ordering of wipe-before-CRR-registration is correct; both platforms filter the marker from export and block it on import. The dirty-tab change removes the only memory guard on per-session dirty content, which is safe for correctness but leaves renderer memory consumption unbounded when many lanes hold large unsaved files simultaneously.
Both core fixes address real data-loss or data-corruption scenarios and are accompanied by targeted regression tests. The behavioral changes are narrow and the cross-platform symmetry (desktop TypeScript and iOS Swift both implement the same wipe/filter logic) is a good sign. The open questions are around memory growth with unbounded dirty tabs and a minor edge case in CRR metadata cleanup, neither of which causes incorrect behavior on the fixed path.
FilesPage.tsx deserves a second look on the memory side — removing all size bounds on dirty-tab caching combined with the existing never-evict-dirty-sessions guard could let the renderer accumulate significant memory in heavy multi-lane workflows.
Important Files Changed
Sequence Diagram
sequenceDiagram participant A as Mac A (upgraded) participant Host as Sync Host participant B as Mac B (not yet upgraded) Note over A: openKvDb() on upgrade A->>A: migrate() A->>A: removeExcludedCrrMetadata() A->>A: wipeQueueLandingStateForStackedOverhaulIfNeeded() Note over A: strips CRR triggers BEFORE delete A->>A: DELETE queue_landing_state (local only) A->>A: INSERT kv marker (filtered from export) A->>A: ensureCrrTables() A->>Host: exportChangesSince(version) marker+queue deletes filtered out Host->>B: applyChanges(filtered set) Note over B: queue_landing_state rows intact B->>B: (later) openKvDb() on own upgrade B->>B: wipeQueueLandingStateForStackedOverhaulIfNeeded() Note over B: wipes its own rows locallyComments Outside Diff (1)
apps/desktop/src/renderer/components/files/FilesPage.tsx, line 248-258 (link)With the size cap removed,
cacheableSessionTabsretains dirty buffers of any size. The eviction guard insetFilesPageSession(line 277) already refuses to evict sessions with unsaved tabs, so a user with dirty buffers across all eight cached scopes will push the renderer process above whatever total in-memory content those buffers hold — with no upper bound. This was the exact tradeoff the old 8 MiB limit addressed. Consider logging a warning when a dirty tab exceeds the old threshold so the renderer team can monitor real-world usage, or capping how many over-limit dirty tabs per scope are retained (e.g. keep the most-recently-written N).Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "Harden local-only queue wipe sync" | Re-trigger Greptile