fix(duckdb-watch): tombstone deletions on unwatched/deferred roots - #31
Conversation
Local duckdb watch omitted the probe-gated authoritative poller that pg watch uses for deferred/unwatched roots. Deletions under those scopes were never tombstoned in SQLite or the DuckDB mirror because interval pushes only run SyncAll, which does not prove missed deletions. Mirror pg watch: share one sync engine with the watcher, register polling obligations for unwatched roots, and reconcile them via ReconcileWatchRoots.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesDuckDB watch synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DuckDBPushWatch
participant syncpkg.Engine
participant startArchivePushWatcher
participant syncWatchBatch
participant unwatchedPoller
participant duckDBPusher
DuckDBPushWatch->>syncpkg.Engine: create shared watch engine
DuckDBPushWatch->>startArchivePushWatcher: start watcher
startArchivePushWatcher->>syncWatchBatch: synchronize recovered watch scope
syncWatchBatch-->>DuckDBPushWatch: notify push loop
startArchivePushWatcher->>unwatchedPoller: register fallback obligation
unwatchedPoller-->>DuckDBPushWatch: deliver authoritative poll tick
DuckDBPushWatch->>duckDBPusher: push synchronized changes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/periscope/archive_write_backend.go (1)
831-836: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNitpick: PG-named function used as the default DuckDB startup sync.
runPGWatchStartupSyncis the fallback when neitherduckDBStartupSyncnorpgStartupSynchooks are set. This is intentional per the new test's comment (DuckDB mirrors PG's SyncAll-based startup), but the name may confuse future readers of the DuckDB path. Consider a small doc comment here explaining why the PG-named function is the shared default.🤖 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 `@cmd/periscope/archive_write_backend.go` around lines 831 - 836, Add a brief comment at the startupSync initialization in the DuckDB startup path explaining that runPGWatchStartupSync is intentionally the shared default because DuckDB mirrors PG’s SyncAll-based startup behavior. Leave the hook precedence and assignment logic unchanged.cmd/periscope/archive_write_backend_test.go (1)
936-1055: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider deduplicating with
TestLocalPGPushWatchGivesDeferredScopesAPollingOwner.This test (Lines 938-1055) duplicates almost the entire body of the existing PG variant (Lines 821-934), differing only in hook field names/types and a couple of literals. A shared helper parameterized over the push-watch entry point, hook wiring, and result type would cut duplication and reduce drift risk if the scenario's timing/assertions change later.
🤖 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 `@cmd/periscope/archive_write_backend_test.go` around lines 936 - 1055, Deduplicate TestLocalDuckDBPushWatchGivesDeferredScopesAPollingOwner with TestLocalPGPushWatchGivesDeferredScopesAPollingOwner by extracting their shared setup, polling-obligation assertion, file creation, reconciliation check, and shutdown flow into a parameterized helper. Pass the backend-specific push-watch entry point, hook wiring, and result/type differences through helper parameters, while keeping each test as a thin wrapper that supplies its variant-specific values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/periscope/archive_write_backend_test.go`:
- Around line 936-1055: Deduplicate
TestLocalDuckDBPushWatchGivesDeferredScopesAPollingOwner with
TestLocalPGPushWatchGivesDeferredScopesAPollingOwner by extracting their shared
setup, polling-obligation assertion, file creation, reconciliation check, and
shutdown flow into a parameterized helper. Pass the backend-specific push-watch
entry point, hook wiring, and result/type differences through helper parameters,
while keeping each test as a thin wrapper that supplies its variant-specific
values.
In `@cmd/periscope/archive_write_backend.go`:
- Around line 831-836: Add a brief comment at the startupSync initialization in
the DuckDB startup path explaining that runPGWatchStartupSync is intentionally
the shared default because DuckDB mirrors PG’s SyncAll-based startup behavior.
Leave the hook precedence and assignment logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 24125667-9b16-4949-88d9-07b477b2a4d4
📒 Files selected for processing (3)
cmd/periscope/archive_write_backend.gocmd/periscope/archive_write_backend_test.gocmd/periscope/duckdb_watch.go
e6fa88c
into
cursor/agentsview-plus-periscope-f559
1 participant
Bug and impact
Local
periscope duckdb watchnever tombstones deletions under unwatched or deferred watch roots.When a session source lives under a root the file watcher cannot cover (symlinked recursive root, root missing at startup, coverage lost at runtime, or persistent polling dir), deleting the source file leaves the session active in SQLite and in the DuckDB mirror indefinitely. Interval floor pushes only run
SyncAll, which does not prove missed deletions.This is the same class of bug that PG watch fixed with
unwatchedRootPoller+ReconcileWatchRoots; DuckDB watch was missing that owner.Concrete trigger: Run
periscope duckdb watchwith a Codex root configured as a symlink to a real directory → sync a session → delete the JSONL → wait for the interval push → session remains in SQLite and DuckDB.Not a PR #29 overlay regression:
archive_write_backend.gois byte-identical to the purified base on the synthesis branch; the gap predates the merge overlay.Root cause
PGPushWatchwiresnewArchivePushUnwatchedPoller, passes a shared syncEngineto the watcher, and reconciles deferred scopes viaReconcileWatchRootson poll ticks.DuckDBPushWatch(local mode) passednilengine, had no poller, and relied onrunLocalSyncAuthoritative+ interval pushes only.Fix
Mirror PG watch for local DuckDB watch:
Engineshared with watcher and pollerunwatchedRootPollerfor deferred/unwatched rootssyncWatchBatchon watcher eventsduckDBPusherthat runsSyncAllthen pushes the mirror (no per-push engine recreation)Validation
TestLocalDuckDBPushWatchGivesDeferredScopesAPollingOwner(new) — mirrors existing PG testTestLocalPGPushWatchGivesDeferredScopesAPollingOwner— still passesTestPushWatchProductionOwners*— still passesTestLocalArchiveWriteBackendDuckDBPushValidatesRemoteBeforeLocalSync— still passesStacks on #29 (
cursor/agentsview-plus-periscope-f559).Note: Daemon-delegated
duckdb watch(CLI → daemon HTTP push) is unchanged; the daemon's main serve loop already owns unwatched polling for its watch path.Summary by CodeRabbit
New Features
Bug Fixes