fix(brain): compact sync log on installs with no brain-sync peers - #5462
Conversation
…omantic#5439) - Compact to getCurrentSeq() in syncAllPeers() when no brain-sync peers are enabled, keeping the last entry so initSyncLog() recovers the sequence counter across restarts - Add early return in brainSyncLog.compactLog() when dropped is 0 to avoid rewriting the log file on idle cycles - Add unit and regression tests in syncOrchestrator.test.js and brainSyncLog.test.js
atomantic
left a comment
There was a problem hiding this comment.
Reviewed by /do:review — 2 critical, 1 improvement, 0 nits.
Highlights
- server/services/syncOrchestrator.js:881 — the compaction floor can exceed the durable sequence after a failed append, emptying the log and resetting sequence recovery.
- server/services/syncOrchestrator.js:878 — local pull categories do not account for inbound/asymmetric or pre-reconcile delta consumers.
Coherence check
The patch matches issue #5439, but its later-peer reconciliation claim is false for supported pre-#1077 delta-only peers.
Generated by /do:review
…c#5439) - Use compatibility-preserving terminal replay compaction in brainSyncLog - Determine compaction floor from durable disk state under log mutex - Update syncAllPeers to pass minSeq floor without destructive truncation - Add spy assertion on atomicWrite for idle-rewrite regression in tests
|
Thanks for the review! I have addressed all the feedback in the latest commit: Compatibility-preserving compaction: Switched from single newest delta truncation to terminal LWW replay compaction per (type, id), preserving active entity state for delta-only/pre-#1077 consumers and retaining max sequence numbers. Mutex-guarded durable floor: Compaction floor and sequence recovery are now determined under the log mutex from disk state, preventing skew after failed appends. Idle-write test assertion: Added a spy on atomicWrite to verify no writes occur when dropped === 0. |
atomantic
left a comment
There was a problem hiding this comment.
Reviewed by /do:review — 1 critical, 1 improvement, 0 nits.
Highlights
server/services/brainSyncLog.js:332— tail membership proves only a later sequence, not that the tail operation wins the LWW replay.
Original thread resolution
- Durable floor after a failed append: resolved.
- Idle no-rewrite side-effect assertion: resolved.
- Legacy/asymmetric convergence: partially resolved; floor-0 replay works, but a positive floor can still discard the true LWW winner.
Coherence check
The live PR description still documents the superseded else if / compactLog(getCurrentSeq()) newest-only design and says 71 tests. The current patch retains one terminal entry per (type, id) and the two focused files contain 72 tests. Please update the description and acceptance wording to match the chosen compatibility-preserving bound.
Generated by /do:review
…ale (atomantic#5439) - Compare LWW timestamps across pre-floor history and tail in brainSyncLog.compactLog - Retain replayed pre-floor winner when tail carries only stale operations - Add test fixture verifying pre-floor winner is retained under positive floor
|
Addressed the latest review feedback:
|
Superseded: subsequent commits resolved the durable-floor and legacy/asymmetric compatibility findings.
atomantic
left a comment
There was a problem hiding this comment.
Reviewed the updated head 76775fe with /do:review — no blocking findings.
The latest commit resolves the positive-floor LWW issue: a pre-floor winner is retained for equal or stale tail timestamps, while any strictly newer tail entry correctly makes that baseline redundant. The earlier durable-floor, idle-write, and legacy/asymmetric compatibility findings also remain resolved, so I resolved their threads and dismissed the superseded change-request reviews.
Non-blocking follow-up: index preserved tail entries by record key during compaction instead of filtering the whole tail once per overlapping key. That will make large positive-floor peer compactions linear and avoid unnecessarily long log-mutex holds; it does not affect the no-peer path fixed here.
The PR description now matches the compatibility-preserving terminal-per-key design and current focused coverage. Required current-head CI is still pending authorization.
Generated by /do:review
Description
Fixes #5439.
On installs without enabled brain-sync peers (the default single-machine posture),
compactLogwas previously never called because compaction only ran withinif (brainPeers.length > 0). As a result, mutations across brain types continuously appended entries todata/brain/sync_log.jsonlwithout bound, increasing startup latency and memory spikes ininitSyncLog().Key Changes
Compatibility-Preserving Terminal LWW Replay Compaction (
server/services/brainSyncLog.js):(type, id)(followingapplyRemoteRecordtie-break rules and Migration 080 replay).GET /api/brain/sync?since=0receive all records.minSeq > 0), retains pre-floor winning state if tail operations are stale (tailTs <= olderWinner.updatedAt), preventing record resurrection on fresh/delta-only peers.maxDurableSeq) on the survivor to anchor peer cursors and sequence recovery.Durable Sequence Resolution Under Log Mutex (
server/services/brainSyncLog.js&server/services/syncOrchestrator.js):maxDurableSeqstrictly from on-disk entries underwithLockinsidecompactLog(), guarding against index/sequence skew if an append previously failed.syncOrchestrator.js,syncAllPeers()passesminSeq(minimum reported peer cursor, or0when no brain peers exist) rather than an unmutexedgetCurrentSeq().Avoid Idle Rewrites (
server/services/brainSyncLog.js):if (dropped <= 0) return 0;beforeatomicWrite(), avoiding disk rewrites and log noise on idle sync cycles.Tests & Verification (
server/services/brainSyncLog.test.js&server/services/syncOrchestrator.test.js):atomicWriteasserting absence of writes whendropped === 0.syncOrchestrator.test.jsto assertcompactLog(0)is invoked when no brain peers exist.Testing & Verification
npx vitest run server/services/brainSyncLog.test.js server/services/syncOrchestrator.test.jspassed (73 tests).