Repository navigation
design+phase1: local-first device list (Durable Object source + local SQLite cache + extensible sync protocol) - #6120
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a complete "sync/v1" local-first device-list substrate end-to-end. The TypeScript Changessync/v1 local-first device list
Sequence Diagram(s)sequenceDiagram
participant iOSApp as iOS App
participant SyncClient
participant SyncFrameApplier
participant CmuxSyncStore
participant TeamPresenceDO as TeamPresence DO
rect rgba(100, 149, 237, 0.5)
Note over iOSApp, CmuxSyncStore: Connection setup and hello
iOSApp->>SyncClient: run()
SyncClient->>SyncFrameApplier: cursor(collection), epoch(collection)
SyncFrameApplier->>CmuxSyncStore: cursor, epoch (per team/collection)
SyncClient->>CmuxSyncStore: encodeHello({collections, cursors, epochs})
SyncClient->>TeamPresenceDO: sync.hello (via WebSocket)
end
rect rgba(144, 238, 144, 0.5)
Note over TeamPresenceDO, CmuxSyncStore: Initial backfill and snapshot paging
TeamPresenceDO->>TeamPresenceDO: handleSyncHello → resolveHelloFrames
TeamPresenceDO->>SyncClient: SyncSnapshotFrame (complete=false) page 1
SyncClient->>SyncFrameApplier: apply(.snapshot page 1)
SyncFrameApplier->>SyncFrameApplier: buffer records
TeamPresenceDO->>SyncClient: SyncSnapshotFrame (complete=false) page N
SyncClient->>SyncFrameApplier: apply(.snapshot page N)
SyncFrameApplier->>SyncFrameApplier: buffer records
TeamPresenceDO->>SyncClient: SyncSnapshotFrame (complete=true) final page
SyncClient->>SyncFrameApplier: apply(.snapshot final)
SyncFrameApplier->>CmuxSyncStore: applySnapshot(teamID, collection, records)
SyncFrameApplier->>CmuxSyncStore: drain queued deltas → applyDelta(...)
SyncClient->>iOSApp: onApplied()
end
rect rgba(255, 165, 0, 0.5)
Note over TeamPresenceDO, iOSApp: Live delta broadcast
TeamPresenceDO->>TeamPresenceDO: heartbeat → reconcileSingleDevice
TeamPresenceDO->>TeamPresenceDO: upsertRecord(devices, deviceId, payload)
TeamPresenceDO->>SyncClient: SyncDeltaFrame
SyncClient->>SyncFrameApplier: apply(.delta(...))
SyncFrameApplier->>CmuxSyncStore: applyDelta(teamID, collection, frameRev, records)
SyncClient->>iOSApp: onApplied()
iOSApp->>iOSApp: DeviceSyncFacade.registryDevices(teamID:)
iOSApp->>CmuxSyncStore: liveRecords(teamID:, collection:)
CmuxSyncStore-->>iOSApp: [StoredSyncRecord]
iOSApp->>iOSApp: decode and render [RegistryDevice]
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // snapshot (possibly into a new history); discard the stale buffer. | ||
| if build.snapshotRev != snapshotRev || build.epoch != epoch { | ||
| build = SnapshotBuild(snapshotRev: snapshotRev, epoch: epoch) | ||
| } |
There was a problem hiding this comment.
Mid-paging restart drops queued deltas
Medium Severity
If a later snapshot page arrives with a different snapshotRev or epoch, the applier replaces the in-flight build with a fresh SnapshotBuild and discards any queuedDeltas accumulated during paging. Deletes or updates that arrived mid-snapshot are never applied after the new snapshot commits, so removed devices can reappear as ghosts.
Reviewed by Cursor Bugbot for commit b7f3c3d. Configure here.
![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: b7f3c3d260
ℹ️ 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".
| try transaction { | ||
| for record in records { | ||
| try applyOneRecord(teamID: teamID, collection: collection, record: record, sortKey: sortKeyFor(record)) |
There was a problem hiding this comment.
When a delayed or duplicate delta whose frameRev is already covered by the local cursor arrives after a snapshot/reconnect, this loop still applies each record before setCursor's monotone guard runs. For a device ID that was absent from the snapshot and has no local tombstone, applyOneRecord sees no localRev and inserts the stale live record, while the cursor remains at the newer rev, so a deleted device can reappear permanently. Check the current cursor and ignore frames/records at or below it before applying records.
Useful? React with 👍 / 👎.
Greptile SummaryPhase 1 of the local-first device list: a generic
Confidence Score: 5/5This PR is explicitly marked do-not-merge (design review + dogfood pending), the sync layer is isolated from presence heartbeat success, and legacy clients are shielded from sync frames by the sync.hello guard — so no existing client behaviour changes until the feature flag flips. The protocol invariants (monotone rev, atomic record+head writes, GC-floor-first crash safety, epoch-based reset detection, bounded buffers on both snapshot pages and queued deltas) are implemented carefully and matched by thorough unit and integration tests on both sides of the wire. The two new comments are quality/footgun concerns that don't affect correctness of the current code paths. The previously flagged message.length byte-count issue in do.ts is the most material concern still open, but it is already tracked in prior review threads. workers/presence/src/do.ts (inbound hello byte-limit uses UTF-16 length for text frames — tracked in prior threads); Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift (tombstoneAt non-monotone SQL; syncStoreLog unused — both tracked). Important Files Changed
Sequence DiagramsequenceDiagram
participant iOS as iOS SyncClient
participant WS as WebSocket
participant DO as TeamPresence DO
participant SS as SyncStorage (DO KV)
iOS->>iOS: Read cursor + epoch from SQLite (t0 launch render)
iOS->>WS: "sync.hello {collections, cursor, epoch}"
WS->>DO: webSocketMessage(hello)
DO->>SS: readBackfillDone()
alt First hello (backfill needed)
DO->>SS: "syncDeviceRecords() — project inst:* → synced:devices:*"
DO->>SS: markBackfillDone()
end
DO->>SS: resolveHelloFrames(cursor, epoch)
alt "cursor=0 or epoch mismatch or cursor>head"
SS-->>DO: snapshot pages (snapshotRev, epoch)
DO->>WS: "sync.snapshot {records, complete=false}*"
DO->>WS: "sync.snapshot {records, complete=true}"
iOS->>iOS: applySnapshot() — upsert+reconcile+forceCursor (SQLite transaction)
else cursor in (gcFloor, head]
SS-->>DO: "delta records (rev > cursor)"
DO->>WS: "sync.delta {records, rev}"
iOS->>iOS: applyDelta() — monotone upsert+setCursor (SQLite transaction)
end
iOS->>iOS: onApplied() → UI reload from SQLite
Note over iOS,DO: Steady-state heartbeat path
iOS-->>DO: (Mac sends heartbeat RPC)
DO->>SS: reconcileSingleDevice() — O(device instances)
alt List-shape changed
SS-->>DO: delta for changed device
DO->>WS: "sync.delta {records=[device], rev}"
iOS->>iOS: applyDelta() + onApplied()
end
Note over DO,SS: Alarm path (timeout/prune)
DO->>SS: reconcileDeviceRecords() — O(team)
DO->>SS: gcTombstones() — raise GC floor, delete expired tombstones
DO->>WS: sync.delta (per changed device)
Reviews (2): Last reviewed commit: "sync: extract SyncDatabase so the SQLite..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| public import Foundation | ||
| import SQLite3 | ||
| import os | ||
|
|
||
| private let syncStoreLog = Logger(subsystem: "com.cmuxterm.app", category: "CmuxSyncStore") | ||
|
|
||
| /// Local-first sync store: one raw-SQLite3 database backing the generic sync | ||
| /// substrate (DESIGN.md §4). This is a deliberate clone of | ||
| /// ``MobilePairedMacStore``'s pattern — an `actor` serializing a | ||
| /// `SQLITE_OPEN_FULLMUTEX` connection, `PRAGMA user_version` lazy migrations, a | ||
| /// `BindValue` binder — extended to one generic `sync_records` table keyed by | ||
| /// `(team_id, collection, record_id)` plus a `sync_cursors` table. Typed facades | ||
| /// (e.g. ``DeviceSyncFacade``) read/write through it; the store stays generic. | ||
| public actor CmuxSyncStore: CmuxSyncStoring { | ||
| public static let currentSchemaVersion: Int32 = 1 | ||
|
|
||
| private let dbPath: String | ||
| // `nonisolated(unsafe)` only so the nonisolated `deinit` can close the | ||
| // handle; every other access is actor-isolated and the connection is | ||
| // FULLMUTEX, matching MobilePairedMacStore. |
There was a problem hiding this comment.
At 619 lines CmuxSyncStore.swift handles at least four separate concerns: SQLite connection + schema migration (MARK: Open + migrate), read queries (MARK: Reads), frame-application logic (MARK: Frame application), and transparent-migration seeding + cursor management (MARK: Transparent migration + MARK: Internals). The cmux-swift-file-package-boundaries rule flags new production files over 400 lines without a single responsibility.
Each MARK section is independently testable; splitting along those boundaries (e.g., CmuxSyncStoreApply.swift, CmuxSyncStoreMigrations.swift, CmuxSyncStoreReads.swift) would keep the actor extensions together while bringing each file under the 400-line threshold.
Rule Used: Flag Swift changes that add too much unrelated res... (source)
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!
| /// substrate (DESIGN.md §4). This is a deliberate clone of | ||
| /// ``MobilePairedMacStore``'s pattern — an `actor` serializing a | ||
| /// `SQLITE_OPEN_FULLMUTEX` connection, `PRAGMA user_version` lazy migrations, a | ||
| /// `BindValue` binder — extended to one generic `sync_records` table keyed by |
There was a problem hiding this comment.
syncStoreLog is declared but never called
The file-level Logger is initialized but no syncStoreLog.debug/info/error(...) call appears anywhere in the 619-line file. A logger that is never invoked provides neither signal nor an escape valve when errors need tracing. Either add the log calls (e.g., on migration steps and store errors) or remove the declaration to avoid dead code in the production path.
Rule Used: Flag production Swift diagnostics that bypass unif... (source)
| } | ||
|
|
||
| /// The render-order hint for a device wire record: its newest instance's | ||
| /// `lastSeenAtAtRev` (epoch ms), used as the store `sort_key`. Decoding the | ||
| /// payload here keeps the sort rule in the facade, not the generic store. | ||
| public static func sortKey(for record: SyncWireRecord) -> Double { | ||
| guard let decoded = try? JSONDecoder().decode(SyncedDeviceRecord.self, from: record.payloadJSON) else { | ||
| return 0 | ||
| } |
There was a problem hiding this comment.
sortKey(for:) allocates a new JSONDecoder and fully decodes the payload on every record during batch apply
DeviceSyncFacade.sortKey(for:) is called once per record inside applyDelta/applySnapshot. Each call constructs a fresh JSONDecoder(), decodes the entire SyncedDeviceRecord (including the nested instances array with its failable CmxAttachRoute decode), and discards everything except lastSeenAtAtRev. For a 200-record snapshot page this is 200 full payload decodes just for sort keys. A lighter approach would decode only { var lastSeenAtAtRev: Double }, or carry the sort key as a top-level wire field so the client never needs to decode the payload for ordering.
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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.
Inline comments:
In `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift`:
- Around line 267-277: The snapshot reconciliation is performing an N+1 query
pattern by first fetching all record IDs with allRecordIDs(), then calling
recordRev() separately for each missing ID in the loop. To fix this, modify the
allRecordIDs() method to return both the record ID and its revision in a single
query, then update the loop that iterates over existing IDs to unpack both the
ID and rev from the query result and compute tombRev in-memory using
max(snapshotRev, localRev) without making the per-record recordRev() call.
- Around line 540-542: The jsonString method currently silently converts invalid
UTF-8 data to "{}" instead of raising an error, which masks data corruption.
Change the jsonString method to throw an error when String initialization with
utf8 encoding fails instead of returning the fallback "{}". Then update all call
sites (forceApplyRecord, applyOneRecord, and seedProvisional) to use try
jsonString(...) instead of jsonString(...) to propagate the error, allowing the
transaction to atomically rollback and surface the failure rather than storing
corrupted data.
- Around line 153-156: The while loop in the read path only checks for
SQLITE_ROW success, silently returning incomplete or default results when
sqlite3_step encounters error states like SQLITE_BUSY or SQLITE_ERROR. Replace
the simple while condition at lines 153–156, 169–173, 185–189, 440–444, and
459–464 with explicit error checking that matches the pattern used in
schemaVersion() and exec() methods. After each sqlite3_step call, check if the
return code is an error (neither SQLITE_ROW nor SQLITE_DONE) and throw an
appropriate error instead of allowing the loop to exit silently, ensuring the
sync state remains accurate by surfacing database errors rather than returning
partial or default values.
In `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/PairedMacMigration.swift`:
- Around line 17-19: The comment block describing the idempotency mechanism
incorrectly states that the marker is scoped as `migrated:<accountId>`, but the
actual implementation uses a team+account scoped key. Update this comment to
accurately reflect the current marker key scope and ensure it matches what the
implementation actually does, so future maintainers have correct documentation
of the migration-key structure.
In `@scripts/lint-ios-package-conventions.sh`:
- Around line 22-23: The script includes Packages/CmuxSyncStore in the SCOPES
array at the loop definition but excludes it from the global-state check pattern
at line 77 (which only specifies Packages/CMUXMobile* and Packages/CmuxMobile*),
allowing this package to bypass the global-state linting rule. Add
Packages/CmuxSyncStore to the global-state check pattern at line 77 to ensure
all packages in SCOPES are consistently subject to the global-state lint rule.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a964d43f-3502-443b-bbaf-6115224df98f
📒 Files selected for processing (21)
.github/workflows/test-ios.ymlPackages/CmuxSyncStore/Package.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStoreError.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStoring.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/DeviceSyncFacade.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/MobileDeviceListLocalFirst.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/PairedMacMigration.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/SyncClient.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/SyncFrameApplier.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/SyncProtocol.swiftPackages/CmuxSyncStore/Tests/CmuxSyncStoreTests/CmuxSyncStoreTests.swiftplans/feat-do-device-list/DESIGN.mdscripts/lint-ios-package-conventions.shworkers/presence/src/do.tsworkers/presence/src/sync.tsworkers/presence/src/syncDevices.tsworkers/presence/src/syncStorage.tsworkers/presence/test/sync.test.tsworkers/presence/test/syncStorage.test.tsworkers/presence/tsconfig.test.json
| while sqlite3_step(statement) == SQLITE_ROW { | ||
| out.append(readRecord(statement, teamID: teamID, collection: collection)) | ||
| } | ||
| return out |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /repo && wc -l Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 116
🏁 Script executed:
cd /repo && sed -n '150,175p' Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 116
🏁 Script executed:
cd /repo && sed -n '182,195p' Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 116
🏁 Script executed:
cd /repo && sed -n '437,470p' Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 116
🏁 Script executed:
cd /repo && rg "sqlite3_step" Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 116
🏁 Script executed:
wc -l Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 129
🏁 Script executed:
sed -n '150,175p' Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 1146
🏁 Script executed:
sed -n '182,195p' Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 525
🏁 Script executed:
sed -n '437,470p' Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 1765
🏁 Script executed:
rg "sqlite3_step" Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 2242
🏁 Script executed:
rg "enum CmuxSyncStoreError" Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift -A 15Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg "stepFailed" Packages/CmuxSyncStore/Sources/CmuxSyncStore/Repository: manaflow-ai/cmux
Length of output: 935
🏁 Script executed:
sed -n '1,100p' Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift | head -50Repository: manaflow-ai/cmux
Length of output: 2145
🏁 Script executed:
cat Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStoreError.swiftRepository: manaflow-ai/cmux
Length of output: 766
🏁 Script executed:
sed -n '100,200p' Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 4172
🏁 Script executed:
rg "lastErrorMessage" Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 3483
🏁 Script executed:
sed -n '430,475p' Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftRepository: manaflow-ai/cmux
Length of output: 2404
Handle sqlite3_step error states explicitly in read paths.
These sites treat any non-SQLITE_ROW result as "no data" and silently return defaults or partial results. When sqlite3_step returns SQLITE_BUSY, SQLITE_ERROR, or other error codes, the methods return incorrect values (cursor() and epoch() return 0, liveRecords() returns empty array, allRecordIDs() returns partial results) instead of throwing. This can cause the sync state to desynchronize with wrong cursor/epoch values. Use the same explicit error-checking pattern already present in schemaVersion() and exec():
Suggested fix pattern
-while sqlite3_step(statement) == SQLITE_ROW {
- out.append(readRecord(statement, teamID: teamID, collection: collection))
-}
+while true {
+ let step = sqlite3_step(statement)
+ if step == SQLITE_ROW {
+ out.append(readRecord(statement, teamID: teamID, collection: collection))
+ continue
+ }
+ guard step == SQLITE_DONE else {
+ throw CmuxSyncStoreError.stepFailed(step, lastErrorMessage())
+ }
+ break
+}Apply to all flagged locations: lines 153–156, 169–173, 185–189, 440–444, 459–464.
🤖 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 `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift` around
lines 153 - 156, The while loop in the read path only checks for SQLITE_ROW
success, silently returning incomplete or default results when sqlite3_step
encounters error states like SQLITE_BUSY or SQLITE_ERROR. Replace the simple
while condition at lines 153–156, 169–173, 185–189, 440–444, and 459–464 with
explicit error checking that matches the pattern used in schemaVersion() and
exec() methods. After each sqlite3_step call, check if the return code is an
error (neither SQLITE_ROW nor SQLITE_DONE) and throw an appropriate error
instead of allowing the loop to exit silently, ensuring the sync state remains
accurate by surfacing database errors rather than returning partial or default
values.
| let existing = try allRecordIDs(teamID: teamID, collection: collection, minRev: 1, maxRev: maxRev) | ||
| for id in existing where !present.contains(id) { | ||
| // Tombstone rev: normally snapshotRev. On a reset, a stale row can | ||
| // have a rev FAR ABOVE snapshotRev (from the old high-rev history); | ||
| // tombstoning at snapshotRev would let a queued old-history delta | ||
| // (rev > snapshotRev) pass the monotone guard and resurrect it. So | ||
| // tombstone at max(snapshotRev, localRev) to dominate any | ||
| // old-history delta for that id. | ||
| let localRev = try recordRev(teamID: teamID, collection: collection, recordID: id) ?? 0 | ||
| let tombRev = isReset ? max(snapshotRev, localRev) : snapshotRev | ||
| try tombstoneAt(teamID: teamID, collection: collection, recordID: id, rev: tombRev, now: now) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Avoid per-record re-query in snapshot reconciliation.
This block loads IDs, then re-queries recordRev per missing ID. For large collections this creates avoidable N+1 SQLite calls during snapshot apply. Fetch (record_id, rev) once and compute tombRev in-memory.
As per coding guidelines, production Swift paths over scalable data should avoid per-target rescans/batch re-queries (.github/review-bot-rules/algorithmic-complexity.md).
🤖 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 `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift` around
lines 267 - 277, The snapshot reconciliation is performing an N+1 query pattern
by first fetching all record IDs with allRecordIDs(), then calling recordRev()
separately for each missing ID in the loop. To fix this, modify the
allRecordIDs() method to return both the record ID and its revision in a single
query, then update the loop that iterates over existing IDs to unpack both the
ID and rev from the query result and compute tombRev in-memory using
max(snapshotRev, localRev) without making the per-record recordRev() call.
Source: Coding guidelines
| private func jsonString(_ data: Data) -> String { | ||
| String(data: data, encoding: .utf8) ?? "{}" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "CmuxSyncStore.swift" | head -5Repository: manaflow-ai/cmux
Length of output: 127
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" | sed -n '535,550p'Repository: manaflow-ai/cmux
Length of output: 843
🏁 Script executed:
rg "jsonString" "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" -B 2 -A 5Repository: manaflow-ai/cmux
Length of output: 1060
🏁 Script executed:
rg "CmuxSyncStoreError" "Packages/CmuxSyncStore/Sources/CmuxSyncStore/" -A 10Repository: manaflow-ai/cmux
Length of output: 16121
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStoreError.swift"Repository: manaflow-ai/cmux
Length of output: 864
🏁 Script executed:
rg "jsonString" "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" -B 5 -A 2Repository: manaflow-ai/cmux
Length of output: 1026
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" | sed -n '1,50p'Repository: manaflow-ai/cmux
Length of output: 2495
🏁 Script executed:
wc -l "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift"Repository: manaflow-ai/cmux
Length of output: 129
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" | sed -n '490,520p'Repository: manaflow-ai/cmux
Length of output: 1943
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" | sed -n '400,430p'Repository: manaflow-ai/cmux
Length of output: 1811
🏁 Script executed:
rg "func applyWireRecord|func applyAppliedRecord|func completeWireRecord" "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" -A 3Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
rg "applyWireRecord|applyAppliedRecord|completeWireRecord" "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" -B 2 -A 15 | head -60Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" | sed -n '200,260p'Repository: manaflow-ai/cmux
Length of output: 3901
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" | sed -n '250,310p'Repository: manaflow-ai/cmux
Length of output: 4182
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" | sed -n '310,360p'Repository: manaflow-ai/cmux
Length of output: 2572
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" | sed -n '320,380p'Repository: manaflow-ai/cmux
Length of output: 3022
🏁 Script executed:
rg "struct SyncWireRecord|SyncWireRecord {" "Packages/CmuxSyncStore/Sources/CmuxSyncStore/" -A 10Repository: manaflow-ai/cmux
Length of output: 200
🏁 Script executed:
rg "\.payloadJSON|payloadJSON:" "Packages/CmuxSyncStore/Sources/CmuxSyncStore/" -B 3Repository: manaflow-ai/cmux
Length of output: 6733
🏁 Script executed:
find "Packages/CmuxSyncStore" -name "*.swift" -type f | xargs wc -l | sort -nRepository: manaflow-ai/cmux
Length of output: 872
Make jsonString throw on invalid UTF-8 instead of silently coercing to {}.
Line 541 silently rewrites non-UTF8 payloads to {}. That can store a live record with empty payload instead of surfacing a hard failure, masking corruption and breaking typed decoding downstream.
All call sites (forceApplyRecord, applyOneRecord, seedProvisional) are already in throwing contexts and wrap the result in try exec(), so propagating the error will surface the failure and atomically rollback the transaction.
Suggested fix
-private func jsonString(_ data: Data) -> String {
- String(data: data, encoding: .utf8) ?? "{}"
+private func jsonString(_ data: Data) throws -> String {
+ guard let value = String(data: data, encoding: .utf8) else {
+ throw CmuxSyncStoreError.encodeFailed
+ }
+ return value
}Then update call sites to try jsonString(...).
🤖 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 `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift` around
lines 540 - 542, The jsonString method currently silently converts invalid UTF-8
data to "{}" instead of raising an error, which masks data corruption. Change
the jsonString method to throw an error when String initialization with utf8
encoding fails instead of returning the fallback "{}". Then update all call
sites (forceApplyRecord, applyOneRecord, and seedProvisional) to use try
jsonString(...) instead of jsonString(...) to propagate the error, allowing the
transaction to atomically rollback and surface the failure rather than storing
corrupted data.
| /// Idempotent two ways: a `migrated:<accountId>` marker short-circuits a | ||
| /// re-run, and `seedProvisional` is `INSERT OR IGNORE` so even without the | ||
| /// marker a re-seed never clobbers an existing record. |
There was a problem hiding this comment.
Update idempotency comment to match the current key scope.
The comment says the marker is migrated:<accountId>, but the implemented marker key is team+account scoped. Keeping this stale comment risks future regressions in migration-key cleanup behavior.
🤖 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 `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/PairedMacMigration.swift` around
lines 17 - 19, The comment block describing the idempotency mechanism
incorrectly states that the marker is scoped as `migrated:<accountId>`, but the
actual implementation uses a team+account scoped key. Update this comment to
accurately reflect the current marker key scope and ensure it matches what the
implementation actually does, so future maintainers have correct documentation
of the migration-key structure.
| for d in Packages/CMUXMobile* Packages/CmuxMobile* Packages/CmuxSyncStore ios/cmuxPackage/Sources ios/cmux; do | ||
| [ -d "$d" ] && SCOPES+=("$d") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Expand the global-state lint scope to include the new package.
After adding Packages/CmuxSyncStore in Line 22, the global-state check still excludes it at Line 77 (Packages/CMUXMobile* Packages/CmuxMobile* only), so this package bypasses that rule.
Suggested fix
-scan global WARN '\b(UserDefaults\.standard|FileManager\.default|Bundle\.main)\b' 1 Packages/CMUXMobile* Packages/CmuxMobile* 2>/dev/null || true
+scan global WARN '\b(UserDefaults\.standard|FileManager\.default|Bundle\.main)\b' 1 Packages/CMUXMobile* Packages/CmuxMobile* Packages/CmuxSyncStore 2>/dev/null || true🤖 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 `@scripts/lint-ios-package-conventions.sh` around lines 22 - 23, The script
includes Packages/CmuxSyncStore in the SCOPES array at the loop definition but
excludes it from the global-state check pattern at line 77 (which only specifies
Packages/CMUXMobile* and Packages/CmuxMobile*), allowing this package to bypass
the global-state linting rule. Add Packages/CmuxSyncStore to the global-state
check pattern at line 77 to ensure all packages in SCOPES are consistently
subject to the global-state lint rule.
Generic local SQLite <-> presence DO sync substrate; device list is the first consumer. Protocol nails snapshot+delta with a per-(team,collection) rev logical clock, contiguous-prefix cursor advanced per atomic frame, rev-filtered snapshots with concurrent deltas queued, tombstone GC floor for forced resync, server-authoritative LWW, and an extensibility contract. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…1 worker) The cloud half of the local-first sync layer designed in plans/feat-do-device-list/DESIGN.md. Additive to the live TeamPresence DO: new storage keys only, presence path untouched, old instances ignore sync.hello and clients fall back to the registry under the flag. - sync.ts: pure sync/v1 wire layer (SyncRecord, hello/snapshot/delta/tick frames, rev as a per-(team,collection) logical clock, snapshot paging, resolveHello floor decision, tombstone GC predicate, schemaVersion stamp). - syncStorage.ts: storage-bound orchestration over a minimal SyncStorage interface (unit-testable with a Map fake): per-collection rev head, upsert-if-shape-changed (quiet cursor on steady heartbeat), tombstone + rev-ordered synctomb: index, gcTombstones raising syncgcfloor:, rev-filtered snapshots, delta catch-up, schemaVersion lazy upgrade. - syncDevices.ts: the devices collection (first consumer). Derives a DeviceRecord projection from presence instances + owner pins; reconciles the whole collection on each write (upsert living, tombstone departed). - do.ts: wires reconciliation onto BOTH write paths (heartbeat + alarm), GC in the alarm, sync.hello over the existing presence WS, sync delta broadcast on the same socket. Single SyncStorage narrowing cast. - tests: 119 pass. syncStorage.test.ts covers the three protocol holes DESIGN flags (frame-atomic cursor, snapshot-races-delete, gc-floor forced resync) plus schemaVersion lazy upgrade, tombstone GC, and derivation idempotency (seen tick / online-offline flip do NOT bump rev; routes/identity/membership do). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The phone half of the local-first sync layer (DESIGN.md §4/§6/§9/§10/§12). A new raw-SQLite3 package mirroring MobilePairedMacStore exactly (actor over a FULLMUTEX connection, CmuxSyncStoring protocol seam, CmuxSyncStoreError enum, PRAGMA user_version lazy migrations, BindValue binder), generalized to one generic sync_records + sync_cursors schema. - CmuxSyncStore: generic local store. Atomic-per-frame apply (applyDelta / applySnapshot commit records + cursor in one transaction = the contiguous-prefix watermark), local.rev>=r.rev stale guard, snapshot rev>=1 reconciliation that EXEMPTS provisional rev=0 rows, monotone cursor, team-scoped keys, the single wire-ms -> stored-seconds boundary. - SyncProtocol: sync/v1 frame codec mirroring the worker; presence frames on the shared socket parse as .unknown (cleanly ignored). - SyncFrameApplier: client apply state machine — snapshot paging buffer + concurrent-delta queue so a delete racing a snapshot is applied after the commit (no ghost), tick advances cursor when idle. - SyncClient: generic transport-agnostic driver (sends sync.hello with the persisted cursors, feeds frames to the applier). - DeviceSyncFacade: typed devices facade -> SyncedDeviceRecord and the existing RegistryDevice UI shape (no new UI model); skips undecodable rows. - PairedMacMigration: transparent, idempotent local->local-cache seeding of existing paired Macs as provisional rev=0 records for instant first render. - MobileDeviceListLocalFirst: the flag (DEBUG-on/Release-off, env + UserDefaults overridable), same seam as PresenceServiceConfiguration. - tests: 27 pass. Covers apply guard, snapshot reconciliation (incl. the rev=0 exemption), cursor monotonicity, paging + concurrent-delete race, local-first render with no network, migration idempotency, ms/seconds boundary, frame codec, flag, and an end-to-end SyncClient hello->apply. Compiles for macOS and iOS arm64 simulator. Shell UI wiring (loadRegistryDevices local-first branch via DeviceSyncFacade.registryDevices, behind the flag with registry fallback) is the final integration step, landing on architecture approval; the facade mapping is in place and tested. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three fixes to the worker sync wiring (autoreview P1/P1/P2): 1. Sync frames no longer reach legacy presence-only sockets. A socket is marked sync-subscribed (its negotiated collections persisted on the WS attachment) only after it sends sync.hello; broadcastSync skips any socket not subscribed to the frame's collection. Without this, a list-shape heartbeat would push a sync.delta to old iOS clients whose PresenceUpdate decoder throws on unknown message types, killing their subscribe stream. 2. The heartbeat path reconciles ONLY the heartbeating device (reconcileSingleDevice over its own inst:<deviceId>: instances + owner), not the whole team. Previously every ~15s beat scanned all instances, all owners, and all stored sync records — O(team) per beat / O(N^2) per interval on a hot DO path. Full-collection reconcile (with the pruned- device tombstone sweep) stays on the periodic alarm only. 3. The alarm now includes the next tombstone-GC deadline (nextTombstoneGcTime) in its next-fire calculation, so a fully-offline team still wakes to GC tombstones and advance syncgcfloor past the 7-day retention window. Previously, with no instances left, no alarm was scheduled and tombstones lingered forever. Tests: 124 pass (added reconcileSingleDevice bounded-work + tombstone + single-device-isolation cases, and nextTombstoneGcTime). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Three CmuxSyncStore fixes (autoreview P1/P2/P2): 1. Malformed sync frames no longer silently advance the cursor. A sync.delta/sync.snapshot whose `records` field is missing or not an array now throws (SyncFrameCodec.requireRecords) instead of being treated as an empty frame. Previously a broken delta at rev=N could move the durable cursor to N without applying records, and a broken snapshot could reconcile against an empty set — durably losing records. The client now reconnects/resyncs instead. 2. The sync UI-invalidation callback fires only on an actual commit. SyncFrameApplier.apply now returns whether the store was written / cursor advanced; SyncClient gates onApplied on it. A presence frame (.unknown), an incomplete snapshot page, or a delta queued during paging returns false, so high-frequency presence `seen` traffic on the shared socket no longer drives spurious SQLite reloads and UI invalidations. 3. The migration marker is keyed by (account, team), matching the team scope of the rows it seeds. Previously a single account-only marker suppressed seeding for the same account in a different team, and survived clear(teamID). The marker key is now `migrated:<teamId>:<accountId>` and clear(teamID) deletes the team's markers, so a different team seeds and a re-sign-in after sign-out re-seeds the fallback. Tests: 31 pass (added malformed-frame-throws, apply commit-flag, cross-team re-seed, and clear-removes-marker cases). iOS arm64 + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… writes Round 2 of review fixes (autoreview P1/P1/P1): 1. SyncClient resyncs on a malformed sync frame instead of skipping it. run() now rethrows a SyncFrameParseError.malformed (a frame that claims to be sync but is structurally broken) after resetting in-flight state, so the session reconnects and re-hellos to fill the gap. Only .notJSON / presence noise is skipped. Skipping a malformed delta could leave a rev permanently absent while a later tick advanced the cursor past it. 2. The device facade decodes routes failably per entry. A future route kind or one malformed route no longer drops the whole device row (which would hide a device the registry/presence paths still render). SyncedDeviceRecord .InstanceRecord has a custom decoder that keeps valid routes and skips bad ones, matching the existing per-route decode contract. 3. The DO sync writes are atomic. upsertRecord / tombstoneRecord / lazyUpgradeRecord now commit the record + head (+ tombstone GC index) via a single DurableObjectStorage.put(entries) multi-key write, so storage can never hold a record whose rev exceeds the head (which would make it invisible to catch-up deltas and rev-filtered snapshots). SyncStorage gained the batched-put overload; the Map fake mirrors it. Tests: 124 worker + 32 swift pass (added malformed-frame-resync, one-bad-route keeps device, atomic-write coverage). iOS arm64 + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 3 of review fixes (autoreview P1/P2): 1. Tombstone GC raises the resync floor BEFORE deleting any tombstone (and only when it advances). If the alarm is interrupted between the floor write and the deletes, the tombstones linger and are re-GC'd next pass (idempotent) while the floor already forces a client whose cursor predates a GC'd deletion onto a full snapshot — so a missed delete can never be silently lost. Previously the floor was raised last, so a crash after the delete left a stale floor and a permanent ghost device. GC is now a decide-then-mutate two pass. 2. The migration marker stores the RAW team id in its sync_meta key and escapes the team id only when building the LIKE pattern in clear(teamID). Previously the key stored an escaped team id AND clear escaped again, so a team id with `_`/`%`/`\` (e.g. team_1) never matched its own stored key and clear left the marker behind, blocking re-seed on re-sign-in. Tests: 124 worker + 33 swift pass (added a clear-re-seeds test for a team id with LIKE metacharacters). iOS arm64 + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 4 review fix (autoreview P1): snapshot missing-record reconciliation preserves the per-record rev watermark. When a completed snapshot omits a local authoritative record, the store now writes a TOMBSTONE at rev = snapshotRev for that id instead of hard-deleting the row. A hard delete dropped the local.rev watermark applyOneRecord relies on, so a delayed/duplicate delta with rev <= snapshotRev (e.g. a queued delta from a reconnect/snapshot overlap) would resurrect the record the snapshot just proved deleted, producing a ghost device. The tombstone is excluded from the live read and its rev makes the guard ignore any later rev <= snapshotRev delta, while a genuinely newer delta (rev > snapshotRev) can still legitimately bring the device back. Provisional rev = 0 rows remain exempt. Tests: 34 swift pass (added staleDeltaCannotResurrectSnapshotDeletedRecord). iOS arm64 + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nto CI Round 5 review fixes (autoreview P2/P2/P2): 1. Rollout backfill on sync.hello. An existing DO has inst:* presence but no synced:devices:* projection until a heartbeat/alarm rebuilds it after this deploys. handleSyncHello now backfills the projection from the live presence map (syncDeviceRecords) when the devices head is still 0, before resolving frames, so a client subscribing in that window sees currently- present devices instead of an empty snapshot. Additive + idempotent. 2. Steady-state heartbeats do zero sync storage work. The heartbeat path now calls syncOneDevice only when the beat could change list-shape (heartbeatMayChangeListShape: new instance, owner pin, routes/identity change). A pure `seen` tick on a known instance with unchanged identity — the common ~15s beat for every instance — skips the prefix-list + owner read + compare entirely, instead of doing it per instance per interval for no possible delta. 3. CmuxSyncStore tests run in CI. Added a `swift test --package-path Packages/CmuxSyncStore` step to test-ios.yml and added the package to the should_run change detector, so the new sync-store coverage is a real PR gate (the worktree CLAUDE.md warns that unwired tests pass with 0 executed). Tests: 124 worker pass; CmuxSyncStore swift test (34) green via the exact CI command. Worker bundles; typecheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 6 review fix (autoreview P1/P1): the new public all-static namespace types SyncFrameCodec and MobileDeviceListLocalFirst failed the repo-wide namespace-type rule that package-conventions-lint enforces for any Packages/ change (now triggered for this PR by the test-ios workflow edit). - SyncFrameCodec is now an instantiable `struct` with `init()` and instance parse/encodeHello methods (matching CmxAttachTicketCompactCoder). Callers hold one instance (SyncClient gained a stored codec). - MobileDeviceListLocalFirst is now a resolved value: `struct` with an `isEnabled` property and a `resolved(environment:defaults:isDebugBuild:)` factory, instead of a static `isEnabled(...)` namespace. `./scripts/lint-ios-package-conventions.sh` passes. Tests: 34 swift pass. iOS arm64 + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 7 review fixes (autoreview P1/P2): 1. The rollout backfill now uses an explicit one-time marker (syncbackfill:<collection>), not head !== 0. A single device's list-shape change makes the head nonzero while other devices that only seen-heartbeat were never projected, so head !=0 did not prove the projection was complete and a sync.hello could serve a partial device set. handleSyncHello now runs the full syncDeviceRecords backfill once, gated on the marker, so every pre-existing presence instance is projected before the first hello resolves. 2. resolveHello forces a snapshot when cursor > head. A client whose cursor exceeds the current DO head (storage reset, rollback, or a cache from a previous DO history) was treated as current — delta mode sent nothing (head <= cursor) and stale/deleted devices persisted forever. cursor > head now triggers a full snapshot + reconciliation so the client converges to current state. Tests: 127 worker pass (added cursor>head resnapshot and backfill-marker independence cases). Typecheck clean; worker bundles. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 8 review fixes (autoreview P1/P2): 1. SyncFrameCodec.intValue no longer traps on an out-of-range JSON number. A valid JSON sync frame with a huge `rev`/`snapshotRev` (e.g. 1e100) used to crash on Int(d); it now goes through intFromDouble which requires finite, integral, in-Int-range values and returns nil otherwise, so the frame surfaces .malformed and the client resyncs (the broken-sync-frame contract). 2. The heartbeat sync gate compares routes directly instead of relying on the `routes` event. A stopping goodbye emits only an `offline` event but can carry new routes (e.g. an empty set); the old gate skipped sync on it, leaving the synced record's attach routes stale until a later alarm. heartbeatMayChangeListShape now returns true when existing.routes != instance.routes (via core routesEqual), so a goodbye-with-routes projects. Tests: 127 worker + 35 swift pass (added huge-rev-is-malformed case). Typecheck clean; iOS arm64 + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 9 review fixes (autoreview P1/P2): 1. resolveHello forces a snapshot for cursor 0 (was returning a delta when the GC floor was also 0). A first-time client has nothing and needs the paged snapshot + reconciliation, not an unpaged catch-up delta. This also matches the documented protocol (DESIGN.md §3.5) and fixes a stale-row hole: a client whose cursor was reset to 0 while local records survived now gets a full snapshot that tombstones the stale authoritative rows. 2. intFromDouble compares against the exactly-representable 2^63 with a strict `<`, not `Double(Int.max)`. Int.max (2^63-1) rounds UP to 2^63 as a Double, so `9223372036854775808` passed the old `<= Double(Int.max)` guard and then trapped on Int(d). The new bound rejects it as malformed. Tests: 127 worker + 35 swift pass (updated the cursor-0 resolveHello / resolveHelloFrames expectations to snapshot; added the 2^63 boundary case). Typecheck clean; iOS + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 10 review fix (autoreview P1): the worker forces a snapshot when a client's cursor is ahead of the DO head (a storage reset/rollback), but the client's monotone apply path could not consume that lower-rev snapshot — present records were ignored (localRev >= snapshotRev), absent records fell outside the [1, snapshotRev] reconciliation, and setCursor's MAX kept the stale ahead cursor — so stale/deleted devices survived forever behind an unrecoverable cursor. applySnapshot now detects a reset (local cursor > snapshotRev) and treats the snapshot as the new ground truth: it force-applies snapshot records unconditionally, reconciles ALL authoritative rows (any rev, not capped at snapshotRev) absent from the snapshot into tombstones, and forces the cursor DOWN to snapshotRev. Provisional rev-0 rows stay exempt. The normal (non-reset) path is unchanged. Tests: 37 swift pass (added reset-recovers-from-ahead-cursor and reset-keeps-provisional). iOS arm64 + macOS compile clean; conventions lint passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 11 review fix (autoreview P1): a DO storage reset/rollback that backfills to the SAME head as a client's cached old history aliased the old rev space — resolveHello returned an empty delta (cursor == head), the client never reconciled, and stale devices survived forever. Adds a collection-history epoch (syncepoch:<collection>, minted once per DO- storage lifetime, re-minted on a reset). It rides every sync.snapshot frame and is sent back in sync.hello: - Worker: readOrMintEpoch/readEpoch; resolveHello forces a snapshot when the client epoch != the server epoch even at an equal head; snapshot frames and resolveHelloFrames carry the epoch; the hello parses an optional epoch. - iOS: SyncWireRecord snapshot frame carries epoch; sync_cursors gains an epoch column; applySnapshot treats an epoch change (or cursor > snapshotRev) as a reset (force-apply records, reconcile all authoritative rows, force cursor + epoch down); the store exposes epoch(); SyncClient sends cursor + epoch in the hello. Tests: 132 worker + 38 swift pass (added epoch mint/stability, snapshot-carries- epoch, resolveHello epoch-mismatch, and the iOS equal-head-epoch-change reset). Typecheck/lint/bundle clean; iOS arm64 + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…y revs Round 12 review fixes (autoreview P1/P1): 1. The collection epoch is minted on the FIRST collection write (prior head 0), atomically with the head, not only lazily on a snapshot read. After a reset wipes storage to head 0, the rollout backfill / first heartbeat rebuild now mints a fresh epoch immediately, so a stale-epoch client at an equal head is still force-snapshotted (previously serverEpoch could read 0, disabling the guard). resolveHelloFrames also mints when head > 0 but epoch 0, covering pre-epoch records written before this shipped. 2. Reset reconciliation tombstones a stale row at max(snapshotRev, localRev), not snapshotRev. A reset drops to a low head while old-history rows carry high revs; tombstoning at the low snapshotRev let a queued old-history delta (rev > snapshotRev) pass the monotone guard and resurrect the row. The higher tombstone rev now dominates any old-history delta for that id. Tests: 134 worker + 39 swift pass (added first-write-mints-epoch, pre-epoch-record minting, and reset-tombstone-blocks-old-history-delta). Typecheck/lint/bundle clean; iOS arm64 + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 13 review fix (autoreview P1): reset detection now triggers on any nonzero incoming snapshot epoch that differs from the local epoch, INCLUDING when the local epoch is 0 (a pre-epoch cache or pre-migration state). The worker force-snapshots a clientEpoch-0 client against a real (epoch-aware) server, but the client previously treated that snapshot as non-reset, so a same-id/same-rev record with a changed payload was skipped by the monotone guard and stale routes/metadata survived the forced resync. The snapshot is now applied authoritatively in that case. A pure first sync (no local rows) is unaffected, and provisional rev-0 rows stay exempt. Tests: 40 swift pass (added nonzero-epoch-vs-local-epoch-0 same-rev-changed- payload reset). iOS arm64 + macOS compile clean; lint passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n guard
Round 14 review fixes (autoreview P1/P?):
1. A LIVE (non-deleted) wire record whose payload is missing or unserializable
now throws .malformed instead of being stored as `{}`. The `{}` fallback
produced a row the device facade cannot decode (hidden from the list) while
the cursor advanced past it — a durably lost row with no resync. The client
now resyncs. Tombstones legitimately keep `{}` and are unaffected.
2. Added Packages/CmuxSyncStore to the lint-ios-package-conventions.sh SCOPES so
the architecture guard (singleton/Combine/locks/Dispatch/timers/KVO/
free-function/untyped rules) covers the new package, not just the repo-wide
namespace-type check. Lint passes (only the expected [String: Any] WARNs for
JSON wire parsing, matching the existing MobileCoreRPCSession pattern).
Tests: 41 swift pass (added live-record-without-payload-is-malformed; tombstone
without payload still parses). iOS arm64 + macOS compile clean; lint passes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 15 review fix (autoreview P1): the DO's webSocketMessage parsed client-controlled JSON (the sync.hello) on the live presence DO without an input size bound, a resource-exhaustion vector. The message byte length is now checked against MAX_SYNC_HELLO_BYTES (4 KiB) before JSON.parse; an over-large frame is dropped silently like any other non-hello message. A real hello (a few short collection names + integer cursors/epochs) is well under the cap, and parseHello already bounds the collection-list count, so the two caps together bound the work the DO does per inbound frame. Typecheck/test/bundle clean (134 worker tests pass). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 16 review fix (autoreview P2): a repeated sync.hello for an already-subscribed collection is now ignored. Previously every well-formed hello ran the full resolution path (backfill check, storage scan, snapshot serialization + send), so an authenticated member could spam tiny <4 KiB hellos and force repeated full device snapshots on the live DO. The socket already records its subscribed collections on the attachment; handleSyncHello now skips a collection already present there. A client resubscribes/resyncs by reconnecting, which the snapshot-first-on-connect protocol already supports. Typecheck/test/bundle clean (134 worker tests pass). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 17 review fixes (autoreview P1/P2): 1. Sync projection is best-effort and cannot fail the presence path. The heartbeat wraps syncOneDevice in try/catch (presence already succeeded, so a DO storage hiccup or bad stored payload must not turn the live heartbeat RPC into a 5xx for existing hosts). The alarm wraps the sync projection + GC so a sync failure never aborts the alarm before it closes expired subscribers and reschedules — the presence-critical alarm duties. Matches the DESIGN §5 "presence path untouched / additive" guarantee. 2. The Swift codec rejects boolean and negative integer fields. rev/snapshotRev/ cursor/epoch are non-negative; a JSON boolean (bridged to a CFBoolean NSNumber) no longer parses as 1, and a negative value is rejected. These drive SQLite revs and cursor advancement, so an invalid value now forces .malformed/resync instead of persisting an impossible cursor. Tests: 134 worker + 42 swift pass (added boolean/negative-rev malformed cases). Typecheck/lint/bundle clean; iOS arm64 + macOS compile clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A compromised or misbehaving DO could stream an endless run of `complete: false` snapshot pages, or flood deltas while stalling a never-completing snapshot, growing SyncFrameApplier's in-flight buffers without limit. Cap both (defaults far above the server-bounded record cardinality) and on overflow drop the in-flight build + surface a malformed frame so the transport tears down and re-hellos, the same recovery path as any other structurally broken frame. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the low-level SQLite statement helpers (BindValue, exec/bind/ transaction, user_version + cursor/tombstone writers) into CmuxSyncStore+SQLite.swift, and split the test suites into SyncFrameAndProtocolTests.swift, so both the store (490) and each test file (<500) stay under the swift-file-length-budget threshold without a budget bump. Pure code-motion; behavior unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
b7f3c3d to
ad5d964
Compare
| const byteLength = typeof message === "string" | ||
| ? message.length // chars; ASCII JSON, so ~bytes, and an over-count only tightens the cap | ||
| : message.byteLength; | ||
| if (byteLength > MAX_SYNC_HELLO_BYTES) return; |
There was a problem hiding this comment.
Hello size uses character count
Low Severity
For string WebSocket payloads, inbound size is capped with message.length (UTF-16 code units), not UTF-8 byte length. Non-ASCII sync.hello JSON can exceed MAX_SYNC_HELLO_BYTES in bytes while passing the check and still be parsed.
Reviewed by Cursor Bugbot for commit ad5d964. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad5d964cc8
ℹ️ 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 (err) { | ||
| console.error("sync projection/GC failed (alarm); presence unaffected", err); | ||
| } |
There was a problem hiding this comment.
When syncDeviceRecords or gcTombstones throws after this alarm has pruned the last stored instance, all can be empty and tombGc remains null, so the candidates list below is empty and no future alarm is scheduled. In that transient-failure path, the stale live synced:devices:* row never gets tombstoned, and later sync.hello snapshots keep showing a device that presence already pruned until some unrelated heartbeat/subscriber happens to schedule another alarm; set a retry deadline in the catch to preserve the best-effort isolation without making the stale state permanent.
Useful? React with 👍 / 👎.
1. Reject records whose rev exceeds the frame head (SyncProtocol). A forged delta carrying rev=5 with a record at rev=1000000 would persist the poison-high rev and the per-record monotone guard would then ignore every legitimate future update for that id until the server head caught up (durable local-cache poisoning). requireRecords now throws .malformed when any record.rev > head, forcing a clean resync. 2. Dedup repeated collection names within one sync.hello (parseHello). The DO's per-connection guard only dedups across separate hellos; a single hello repeating 'devices' N times amplified into N backfills + N snapshot serializations. parseHello now keeps the first occurrence per name; handleSyncHello also marks each name seen immediately (defense in depth). 3. Bound queued deltas by total RETAINED RECORDS, not frame count (SyncFrameApplier). The prior frame-count bound let one oversized multi-record delta blow past the ceiling. Now sums records across the queue and rejects before append. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift`:
- Around line 85-90: Remove the fallthrough statement from case 0 in the
migration switch statement. Since case 1 only contains a break statement and
performs no additional operations, the fallthrough is unnecessary. After calling
migrateToV1() and setUserVersion(1) in case 0, the case will naturally exit the
switch without explicit fallthrough, achieving the same behavior while removing
the SwiftLint warning and simplifying the control flow.
- Around line 379-389: The migrationCompleted function currently treats all
non-SQLITE_ROW results from sqlite3_step as false, which masks database errors
like SQLITE_BUSY and SQLITE_ERROR. Modify the return statement at the end of
migrationCompleted to properly check the sqlite3_step result: store it in a
variable, then check if it equals SQLITE_ROW (return true), SQLITE_DONE (return
false), or anything else (throw CmuxSyncStoreError.stepFailed with the result
code and lastErrorMessage()) to match the error handling pattern used elsewhere
in the package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d23d737e-d29e-4d07-bd25-03d85acc2236
📒 Files selected for processing (14)
.github/workflows/test-ios.ymlPackages/CmuxSyncStore/Package.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore+SQLite.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStoreError.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStoring.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/DeviceSyncFacade.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/MobileDeviceListLocalFirst.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/PairedMacMigration.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/SyncClient.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/SyncFrameApplier.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/SyncProtocol.swiftPackages/CmuxSyncStore/Tests/CmuxSyncStoreTests/CmuxSyncStoreTests.swiftPackages/CmuxSyncStore/Tests/CmuxSyncStoreTests/SyncFrameAndProtocolTests.swift
| case 0: | ||
| try migrateToV1() | ||
| try setUserVersion(1) | ||
| fallthrough | ||
| case 1: | ||
| break |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Remove fallthrough in migration switch to satisfy lint and simplify control flow.
This switch can express the same behavior without fallthrough, and it clears the existing SwiftLint warning.
Suggested change
switch version {
case 0:
try migrateToV1()
try setUserVersion(1)
- fallthrough
case 1:
break
default:
throw CmuxSyncStoreError.unknownSchemaVersion(Int(version))
}🧰 Tools
🪛 SwiftLint (0.63.3)
[Warning] 88-88: Fallthrough should be avoided
(fallthrough)
🤖 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 `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift` around
lines 85 - 90, Remove the fallthrough statement from case 0 in the migration
switch statement. Since case 1 only contains a break statement and performs no
additional operations, the fallthrough is unnecessary. After calling
migrateToV1() and setUserVersion(1) in case 0, the case will naturally exit the
switch without explicit fallthrough, achieving the same behavior while removing
the SwiftLint warning and simplifying the control flow.
Source: Linters/SAST tools
| public func migrationCompleted(accountID: String, teamID: String) throws -> Bool { | ||
| try ensureReady() | ||
| var statement: OpaquePointer? | ||
| defer { sqlite3_finalize(statement) } | ||
| let rc = sqlite3_prepare_v2(db, "SELECT value FROM sync_meta WHERE key = ?;", -1, &statement, nil) | ||
| guard rc == SQLITE_OK else { | ||
| throw CmuxSyncStoreError.prepareFailed(rc, lastErrorMessage()) | ||
| } | ||
| try bind(statement: statement, parameters: [.text(migrationKey(accountID: accountID, teamID: teamID))]) | ||
| return sqlite3_step(statement) == SQLITE_ROW | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "CmuxSyncStore.swift" -path "*/CmuxSyncStore/*"Repository: manaflow-ai/cmux
Length of output: 127
🏁 Script executed:
cat -n "Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift" | sed -n '370,400p'Repository: manaflow-ai/cmux
Length of output: 1577
🏁 Script executed:
# Check the error type definition
rg "enum CmuxSyncStoreError" -A 20 "Packages/CmuxSyncStore"Repository: manaflow-ai/cmux
Length of output: 1452
🏁 Script executed:
# Search for other sqlite3_step patterns to see how they're handled
rg "sqlite3_step" "Packages/CmuxSyncStore" -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 4912
🏁 Script executed:
# Check where migrationCompleted is called
rg "migrationCompleted" "Packages/CmuxSyncStore" -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 1854
🏁 Script executed:
# Check broader codebase usage
rg "migrationCompleted" --type swift -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 1854
Handle non-SQLITE_ROW/SQLITE_DONE step results in migration lookup.
migrationCompleted treats all non-row results as false, masking database failures like SQLITE_BUSY/SQLITE_ERROR as "not migrated". This can cause the migration to be re-run when a transient database issue occurs. Other functions in the same package check sqlite3_step results and throw CmuxSyncStoreError.stepFailed on unexpected values; this function should follow the same pattern.
Suggested fix
- return sqlite3_step(statement) == SQLITE_ROW
+ let step = sqlite3_step(statement)
+ if step == SQLITE_ROW { return true }
+ if step == SQLITE_DONE { return false }
+ throw CmuxSyncStoreError.stepFailed(step, lastErrorMessage())🤖 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 `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/CmuxSyncStore.swift` around
lines 379 - 389, The migrationCompleted function currently treats all
non-SQLITE_ROW results from sqlite3_step as false, which masks database errors
like SQLITE_BUSY and SQLITE_ERROR. Modify the return statement at the end of
migrationCompleted to properly check the sqlite3_step result: store it in a
variable, then check if it equals SQLITE_ROW (return true), SQLITE_DONE (return
false), or anything else (throw CmuxSyncStoreError.stepFailed with the result
code and lastErrorMessage()) to match the error handling pattern used elsewhere
in the package.
Queued deltas during snapshot paging are transient overhead the completing snapshot subsumes, so cut a stalled-snapshot producer off an order of magnitude sooner here than on the snapshot pages. The legitimate count is tiny (devices collection is presence-capped well under 10k). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| routes.append(route) | ||
| } else { | ||
| _ = try? unkeyed.decode(AnyDecodableSkip.self) | ||
| } |
There was a problem hiding this comment.
Route skip decode may hang
Medium Severity
In decodeRoutesFailably, when CmxAttachRoute decoding fails, the loop relies on AnyDecodableSkip to consume the bad element. If that skip decode also fails, the unkeyed container index never advances and the while !unkeyed.isAtEnd loop never terminates, hanging the actor that loads the device list.
Reviewed by Cursor Bugbot for commit 9959b7f. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9959b7ffe2
ℹ️ 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".
| ownersFromList(owners), | ||
| nowMs, | ||
| ); | ||
| for (const delta of deltas) this.broadcastSync(delta); |
There was a problem hiding this comment.
When syncDeviceRecords reconciles multiple devices, reconcileDeviceRecords has already upserted/tombstoned earlier rows before it returns this deltas array. If a later storage operation in that same reconcile throws, this loop is never reached, so those earlier committed revs are never sent to subscribed sockets; a subsequent delta for another device can then advance clients' cursors past the missed rev, leaving that row absent from the local SQLite cache until a forced snapshot. Emit each delta as its write commits, or force subscribed clients to resync when a reconcile fails after partial writes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@Packages/CmuxSyncStore/Sources/CmuxSyncStore/SyncFrameApplier.swift`:
- Around line 175-182: The current check on queuedRecords + records.count only
bounds the total number of records across deltas, but empty deltas (where
records.count equals zero) can be appended indefinitely without triggering this
limit. Add an additional check that also bounds the count of deltas themselves,
not just the records they contain, to prevent a stalled snapshot stream from
queuing unbounded empty tuples in queuedDeltas. Modify the condition before
appending to queuedDeltas to ensure both the record count and the number of
deltas are bounded against maxQueuedDeltaRecords or an appropriate limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2129ebc3-7a36-4640-bb7a-854b057b6afd
📒 Files selected for processing (6)
Packages/CmuxSyncStore/Sources/CmuxSyncStore/SyncFrameApplier.swiftPackages/CmuxSyncStore/Sources/CmuxSyncStore/SyncProtocol.swiftPackages/CmuxSyncStore/Tests/CmuxSyncStoreTests/SyncFrameAndProtocolTests.swiftworkers/presence/src/do.tsworkers/presence/src/sync.tsworkers/presence/test/sync.test.ts
… splitting Autoreview flagged that splitting the store into a +SQLite extension forced the nonisolated(unsafe) sqlite3 handle from file-private to module-internal, weakening the actor-isolation invariant (any future module file could touch the raw handle off-actor). Revert the store split to keep `db` private to the actor, and accept the 619-line file as documented known debt in swift-file-length-budget.tsv (the guard's explicit escape). The test-file split is kept (pure test code, no concurrency invariant). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… bypass) Autoreview P1: switching the queued-delta guard to a records-only bound opened a bypass — a producer can hold a snapshot open and flood empty (records: []) deltas, each growing queuedDeltas by one entry while adding 0 to the record count, so the bound never trips (unbounded memory). Add an independent frame-count bound (default 10k) enforced alongside the record bound; either overflow drops the build and forces a resync. Test covers the empty-delta flood. Rebalanced the two test files to stay under the 500-line guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ion growth) Autoreview P2: the per-collection buffer/cursor bounds did not bound the NUMBER of collections, so a misbehaving endpoint could stream incomplete snapshots/deltas/ticks for many distinct collection names (each just under the per-collection ceiling) and grow `builds` + create local cursor state for collections the client never requested. SyncFrameApplier now takes an allowedCollections set and rejects any frame outside it as .malformed (routing through SyncClient's reset+rethrow). SyncClient documents that the composition root must build the applier with allowedCollections matching its subscribed list. Test covers rejection + no leaked cursor state. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview P2 follow-up: the applier's allowlist defaulted to accept-all, making the unrequested-collection bound opt-in (a production caller could forget to pass it). SyncClient now derives the allowlist from its subscribed `collections` and rejects any inbound frame for a collection outside that set in run() directly, independent of the injected applier's config — the safety invariant is enforced by the client API, not left to each caller. Test uses a default (accept-all) applier and proves the client still rejects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…iles split Resolves the tension between the two prior review preferences: a +SQLite extension widened the raw handle to module-internal (actor-isolation weakening), while keeping one 619-line file tripped the file-length guard. Extract a SyncDatabase type that owns the raw sqlite3 OpaquePointer as a PRIVATE member and exposes only the binder/exec/transaction/prepare helpers; CmuxSyncStore holds one as a private let. The handle is now never module-visible (isolation invariant holds by construction), and the package splits into focused files (store 492, SyncDatabase 120, row-decode 32) so no budget exception is needed. Pure code-motion + indirection; behavior unchanged, all 49 package tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
There are 5 total unresolved issues (including 3 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 52d9534. Configure here.
| ws.serializeAttachment({ expiresAt, syncCollections: merged } satisfies WsAttachment); | ||
| } catch { | ||
| // attachment write failed; the socket is likely gone | ||
| } |
There was a problem hiding this comment.
Sync subscription after snapshot send
High Severity
handleSyncHello sends snapshot/delta frames before persisting syncCollections on the socket attachment, and a failed serializeAttachment is ignored. The client can commit the hello response locally while broadcastSync skips that socket, so live device-list deltas never arrive until reconnect.
Reviewed by Cursor Bugbot for commit 52d9534. Configure here.
| } | ||
| const hello = parseHello(body); | ||
| if (hello === null) return; // not a sync.hello; ignore | ||
| await this.handleSyncHello(ws, hello.collections); |
There was a problem hiding this comment.
Unhandled sync hello errors
Medium Severity
webSocketMessage awaits handleSyncHello without the same best-effort isolation used on heartbeat and alarm sync projection. A storage or serialization error during backfill or resolveHelloFrames can abort hello handling after partial work, leaving the socket unsubscribed and no catch-up frames.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 52d9534. Configure here.
This branch was successfully deployed
1 participant
Phase 1 of the device-list refactor Lawrence requested: make the iOS device list driven by the per-team presence Durable Object (cloud source of truth) with a local SQLite cache for instant startup, over a general, extensible local-first sync protocol. Design doc: plans/feat-do-device-list/DESIGN.md (836 lines) — the primary thing to review.
Requirements captured (all four):
Plus: transparent MobilePairedMacStore→DO migration; additive/schemaVersion'd/lazy changes to the LIVE presence DO (per docs/presence-service.md migration discipline); Aurora kept as fallback this phase behind flag
mobileDeviceListLocalFirst(DEBUG-on/Release-off).STATUS: the implementing agent stalled on an infra stream-watchdog after 21 commits (sync handshake, sync isolated from the presence path, bounded inbound size, rev validation). This PR is opened to preserve the work, make the design reviewable, and let CI validate the build — it may need a finishing pass (remaining sync tests, CI green, rebase). Do NOT merge: architecture change on a live service, awaits Lawrence's design review + dogfood.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Changes a live presence Durable Object (new WebSocket handling and device projection) and introduces durable on-device sync state with subtle cursor/epoch/reconciliation rules; incorrect behavior could miss deletions or show stale devices until dogfood proves the flag path.
Overview
Phase 1 adds a generic local-first sync stack (
sync/v1on the presence WebSocket plus a newPackages/CmuxSyncStoreSQLite package) so the iOS device list can render from disk and reconcile with the per-team Durable Object.plans/feat-do-device-list/DESIGN.mddocuments the protocol, cursor/epoch semantics, and rollout.iOS (
CmuxSyncStore): Actor-backed raw SQLite3 store (sync_records,sync_cursors, migration meta) with atomic delta/snapshot apply, reset handling (epoch mismatch, ahead cursor), tombstone reconciliation, and provisionalrev=0seeding.SyncClient+SyncFrameApplierhandle paged snapshots, queued mid-snapshot deltas, malformed-frame resync, and collection allowlists.DeviceSyncFacademapsdevicespayloads toRegistryDevice;PairedMacMigrationseeds fromMobilePairedMacStore;MobileDeviceListLocalFirstis DEBUG-on / Release-off. Extensive Swift tests cover apply, codec, and client behavior.Worker (
TeamPresenceDO): Additive sync on the existing socket—sync.hello(size-bounded), one-time devices backfill, snapshot vs delta from cursor/GC floor/epoch, heartbeat projection only when list-shape changes, alarm-path full reconcile + tombstone GC. Sync frames go only to sockets that subscribed; presence-only clients unchanged.CI/lint: iOS workflow detects
Packages/CmuxSyncStore, runsswift testfor it, and includes the package inlint-ios-package-conventions.sh.Reviewed by Cursor Bugbot for commit 52d9534. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Phase 1 of the local‑first device list: iOS now syncs the team device list from the Presence Durable Object over
sync/v1, with a local raw SQLite cache for instant startup. BehindmobileDeviceListLocalFirst, Aurora remains a fallback.New Features
sync/v1substrate (snapshot + delta, per‑record rev/tombstone, cursor + epoch, reset recovery) on the DO and iOS.Packages/CmuxSyncStore(SQLite3) withSyncClientandDeviceSyncFacade; renders from cache, then reconciles; provisional seed fromMobilePairedMacStoreasrev=0.sync.hello; epoch minted per collection; snapshot forced on cursor=0, cursor>head, or epoch mismatch; atomic multi‑key writes; tombstone GC raises floor before delete; heartbeat syncs only the changed device; goodbye‑with‑routes projected; sync frames only to sockets that sentsync.hello; inbound hello size bounded; sync isolated from presence.Packages/CmuxSyncStoretests; include the package inscripts/lint-ios-package-conventions.sh.Bug Fixes
revexceeds the frame head (forces resync).sync.helloand ignores already‑subscribed collections on that connection.SyncClientnow rejects frames for unrequested collections by construction (independent of applier config);SyncFrameApplieralso allowlists; malformed frames trigger resync.SyncDatabaseto own the rawsqlite3handle (file‑private) and split store helpers into focused files, preserving actor isolation; no behavior change.Written for commit 52d9534. Summary will update on new commits.
Summary by CodeRabbit
devicesusing snapshot/delta frames with bounded paging and cursor/epoch handling.