fix(aw-sync): replace destination-as-marker cursor with per-source rowid cursor

[TimeToBuildBob]

Problem

sync_one used the newest event end-time in the destination bucket as its resume cursor.
This permanently loses late-arriving events: any event inserted into the source after the initial sync — even one with an old timestamp — is silently skipped on every subsequent pull because its timestamp falls before the stored cursor.

Fixes #696.

Solution

Replace the destination-end-time cursor with a per-source rowid cursor stored in a separate cursors.db Datastore next to the sync database.

Key format: sync.cursor.{src_device_id}.{bucket_id}

SQLite events.id is monotonically increasing in insertion order. WHERE id > last_cursor_rowid catches every inserted event regardless of its timestamp, so late-arriving backfill and out-of-order inserts are picked up on the next pull.

Changes

aw-datastore

• Add get_events_since_rowid(bucket_id, since_rowid, limit) to DatastoreInstance and Datastore public API (Command::GetEventsSinceRowid).

aw-sync/accessmethod

• Add default get_events_since_rowid trait method returning None for AwClient / HTTP peers (rowid queries are not supported over HTTP).
• Datastore override returns Some(…).

aw-sync/sync

• Helpers: open_or_create_cursor_ds, cursor_db_root, cursor_key.
• cursor_ds is threaded through sync_run → pull_from_remotes → sync_datastores_with_cursor → sync_one.
• Rowid fast-path in sync_one: batches events since the stored cursor, persists max(rowid) after each batch. Falls through to the timestamp-based path when the source does not support rowid queries (HTTP peers) or the cursor DB is unavailable.
• sync_datastores (public API) is unchanged; the new sync_datastores_with_cursor carries the cursor parameters.

aw-query (incidental fix)

• Remove duplicate #[allow] attributes introduced in 7ed5bb8 (clippy::block_scrutinee duplicated across lib.rs and parser.rs) that blocked cargo clippy --deny warnings.

Tests

New rowid_cursor_tests module in aw-sync/src/sync.rs:

• late_arriving_backfill_syncs_on_second_pull: inserts 3 events, syncs (establishes cursor at rowid 3), inserts a backfill event at T=5s and a new event at T=40s, syncs again — destination ends up with all 5 events.
• cursors_are_device_scoped: two source devices each contribute 2 events to independent cursors; verifies buckets accumulate the correct counts after two pulls.

Test run

test result: ok. 74 passed; 0 failed; 0 ignored
  (65 lib tests + 9 internal tests + 24 integration tests + 3 roundtrip tests)

[greptile-apps]

Confidence Score: 4/5

[High risk] Replaces sync cursor storage and event-fetching logic.

Fix the collision in cursor_key before merging, so distinct sources cannot overwrite each other’s saved progress.

Findings

1.  Different sources share saved progress ▶

Summary

This PR replaces timestamp-based pull progress with saved row IDs for file-backed sources, so newly inserted activity can sync even when its timestamp is old.

• Adds row-ID reads, saved progress, replay checks, and tests.
• Adds unclipped event reads through the Rust client and server.
• Since the last review, changes cursor-key escaping and restores clipped reconciliation for timestamp-only copies.
• The new escaping can make distinct source IDs share saved progress.
• TimeToBuildBob accepted the destination durability gap and deferred a server-side commit guarantee.
• TimeToBuildBob accepted heartbeat-extension limits and deferred refreshing the trailing event.
• TimeToBuildBob accepted recovery of pre-upgrade backfills as separate work tracked in aw-sync: rowid upgrade bootstrap does not recover backfills lost under the old timestamp cursor #793; bootstrap progress now saves after a successful timestamp pass.
• TimeToBuildBob accepted the exact-same-timestamp edge case as outside normal watcher use and deferred a stronger identity check.
• TimeToBuildBob accepted wide destination reads and deferred bounded reads.
• TimeToBuildBob disputed cursor-file discovery and overlapping-event deletion; the threads withdrew those claims, and the current code also uses unclipped reads for deletion checks.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    Source[Peer database] --> Read[Read rows after saved ID]
    Cursor[(Hidden cursor database)] --> Read
    Read --> Compare[Check destination copies]
    Compare --> Write[Copy events to destination]
    Write --> Save[Save highest scanned ID]
    Save --> Cursor
    Fallback[Timestamp-only copy] --> Destination[Local activity server]
    Write --> Destination

Loading

_{Reviews (10) · Last reviewed commit: "fix(sync): clipped source in reconcile f..." · Reviewed by Greptile}

[codecov]

Codecov Report

❌ Patch coverage is 75.51020% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.48%. Comparing base (656f3c9) to head (472dfa1).
⚠️ Report is 199 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-sync/src/sync.rs	82.63%	29 Missing ⚠️
aw-client-rust/src/lib.rs	0.00%	13 Missing ⚠️
aw-sync/src/accessmethod.rs	50.00%	8 Missing ⚠️
aw-datastore/src/datastore.rs	81.25%	6 Missing ⚠️
aw-datastore/src/worker.rs	84.61%	2 Missing ⚠️
aw-server/src/endpoints/bucket.rs	66.66%	1 Missing ⚠️
aw-sync/src/sync_wrapper.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master     #789       +/-   ##
===========================================
+ Coverage   70.81%   84.48%   +13.66%     
===========================================
  Files          51       82       +31     
  Lines        2916    11763     +8847     
===========================================
+ Hits         2065     9938     +7873     
- Misses        851     1825      +974     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

Review convergence summary — commit 4cc9241

All 7 Greptile findings from the prior review round (on af388b1) have been addressed:

Finding	Status
P1 Pulls duplicate existing activity	Fixed — bootstrap pass detection + per-page cursor save
P1 Continued activity stops growing	Acknowledged — known tradeoff; filed #790 as follow-up
P1 Deleted copies cannot be restored	Fixed — reset cursor to 0 when destination is empty
P1 Requested dates are ignored	Fixed — sync_spec.start filter applied in rowid fast path
P1 Peers pull the wrong database	False positive — cursors stored under .cursors/ dot-directory, skipped by both walkers
P1 Damaged rows block later activity	Fixed — get_events_since_rowid returns max_scanned_rowid to advance past corrupt rows
P2 Sync paths can drift apart	Fixed — sync_datastores delegates to sync_datastores_with_cursor

Greptile re-review on 4cc9241 completed: pass. CI checks queued (pending runner availability).

Domain risk: the heartbeat-extension limitation (#790) is a bounded known tradeoff, not a regression blocking the core #696 correctness fix.

[TimeToBuildBob]

Review convergence summary — head 2be8333 (updated)

Round 2 of Greptile review is settled. No blocking findings remain.

Finding	Classification	Status
P1 Date filter drops crossing events	Real bug	Fixed in 2be8333: the filter is now timestamp + duration > start, the same overlap rule the timestamp path uses
P1 Bootstrap skips uncopied activity	Real, but pre-existing (#696): pre-upgrade backfills were already lost under the old cursor	Not a regression. One-time source-history reconcile tracked in #793; Greptile agreed it doesn't block this PR

From round 1:

• Continued activity stops growing: the known trailing-event heartbeat-extension gap, tracked in aw-sync: rowid cursor misses heartbeat extensions to trailing open event #790 (Greptile acknowledged it)
• Peers pull the wrong database: false positive, retracted (.cursors/ dot-dir is skipped by both walkers)

CI on 2be8333: clippy, format, ubuntu and windows pass. macOS, Android and coverage were still running when this was written.

Ready for maintainer review/merge once CI finishes. This PR fixes the core #696 issue: late-arriving and backfilled events are no longer silently lost. Known follow-ups: #790 and #793.

[ErikBjare]

@greptileai review

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

🤖 AI code review

Replaces the destination-newest-timestamp resume cursor in aw-sync pulls with a per-source SQLite rowid cursor persisted in a dedicated cursors.db, adding rowid-range datastore queries, an unclipped events HTTP parameter, and a large rowid-cursor test module. Introduces sync_run_with_cursor_root and threads a cursor Datastore through the pull path while keeping the public sync_datastores signature unchanged.

Safe to merge — no P0/P1 findings

Confidence 5/5

✅ No thread-worthy findings. Advisory notes follow; they are retained without opening review threads.

1 advisory finding (summary-only, not scored)

These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.

⚠️ P2 medium — aw-sync/src/sync.rs

This is a fix(...) PR but no test files are included in the diff. Erik's feedback: 'where is the repro & fixes they are supposed to catch' (gptme#3441), 'that measurement should come with a regression test' (gptme#3446). Add a test that would have caught this bug. (Advisory: Erik merged all such PRs but consistently requested tests.)

Add a test file that reproduces the bug before the fix and passes after it.

_{How this was verified: static preflight: fix-commit + touched-files scan (rule 7)}

Files changed (8) — the diff as I read it

• aw-client-rust/src/blocking.rs — Adds a blocking proxy_method for get_events_unclipped mirroring the async client method.
• aw-client-rust/src/lib.rs — Adds async get_events_unclipped that appends unclipped=true to the bucket events request.
• aw-datastore/src/datastore.rs — Adds get_events_since_rowid and get_max_event_rowid to DatastoreInstance, querying events by rowid order with corrupt-row handling.
• aw-datastore/src/worker.rs — Adds EventPage response variant and GetEventsSinceRowid/GetMaxEventRowid commands and public Datastore methods.
• aw-server/src/endpoints/bucket.rs — Adds optional unclipped query parameter to the events GET endpoint and dispatches to get_events_unclipped when true.
• aw-sync/src/accessmethod.rs — Adds get_events_unclipped, get_events_since_rowid, and get_max_event_rowid to the AccessMethod trait with default None and Datastore/AwClient overrides.
• aw-sync/src/sync.rs — Adds cursor datastore helpers, threads cursor state through sync_one, and adds a large rowid-cursor test module.
• aw-sync/src/sync_wrapper.rs — Switches pull_db to sync_run_with_cursor_root so cursor storage lives at the sync root instead of the host subfolder.

Previous review passes

commit	score	findings	engine	when
47558010b2d0	3/5	2	llm	2026-10-07 19:59 UTC
dff58323fc70	3/5	1	llm	2026-10-07 21:23 UTC
084af979de66	2/5	2	llm	2026-10-07 21:52 UTC
fbf23ef75d98	5/5	0	llm	2026-10-07 23:43 UTC
113305354470	5/5	0	llm	2026-10-09 04:55 UTC
53dd8b403a4a	2/5	2	llm	2026-10-10 10:04 UTC
913f6b5efce1	5/5	0	llm	2026-10-10 12:32 UTC
6622f9ce9439	2/5	3	subscription	2026-10-10 15:00 UTC
472dfa1ebb00	4/5	3	llm	2026-10-11 03:24 UTC
472dfa1ebb00	4/5	1	llm	2026-10-11 04:04 UTC

_{Reviewed 472dfa1ebb00 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 704s · about this reviewer}

_{Maintainer commands}

_{@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.}

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

CI-green and mergeable (Greptile 5/5) — waiting only on a maintainer click.

This PR is ready to merge, but the bot has pull-only access to this repo and can't self-merge — surfacing it here so it isn't lost. The monitoring loop will stop re-flagging it now that this note is posted.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

Fixed in `1133053`.

The collision: enc replaced \u{1f} → %1F but did not escape literal %, so cursor_key("A%1F", "B") and cursor_key("A\u{1f}", "B") produced the same key.

Fix: encode % → %25 before \u{1f} → %1F, making "A%1F" → "A%251F" while "A\u{1f}" → "A%1F" — distinct. Added cursor_key_no_collision_between_percent_escape_and_unit_separator test to rowid_cursor_tests to lock in the invariant.

[TimeToBuildBob]

Status update — 2026-10-08

All CI checks pass. Two P1 findings addressed since last review:

Finding 1 (timestamp-path reconcile bug, commits 0a0af08): reconcile_updated_events used get_events_unclipped for all callers. For events edited with both a longer duration AND different data, the unclipped identity (timestamp, 60s) differed from the dest identity (timestamp, 5s), so reconcile skipped the replacement and left stale data in the head. Fixed by passing use_unclipped_source = cursor_ds.is_some() — the timestamp path uses clipped source, matching identities and triggering the data-change replacement. Regression test: timestamp_path_replaces_event_with_changed_duration_and_data.

Finding 2 (cursor_key collision, commit 1133053): enc replaced \u{1f} → %1F without escaping literal %, so cursor_key("A%1F", "B") and cursor_key("A\u{1f}", "B") produced the same key. Fixed by encoding % → %25 before \u{1f} → %1F. Regression test: cursor_key_no_collision_between_percent_escape_and_unit_separator.

Greptile trigger cap (8) was reached before the cursor_key fix could be scored. The last score (4/5) predates this fix; expecting 5/5 with fresh review.

[TimeToBuildBob]

Property-based conformance harness for aw-sync, drafted against this branch: TimeToBuildBob/aw-server-rust:feat/aw-sync-proptest-conformance (aw-sync/tests/conformance.rs, proptest over insert / heartbeat-extend / edit / delete / sync, one invariant: dest == source by (timestamp, duration, data) after push+pull). Not opening it as a PR until this lands.

One thing it surfaced that matters for this PR's review: the rowid cursor path is unreachable from aw-sync/tests/. sync_datastores delegates with cursor_ds = None, and sync_datastores_with_cursor is private, so an integration test can only drive the timestamp fallback. The in-crate #[cfg(test)] modules cover the rowid path, but any black-box harness (this one, or a future fuzz target) cannot. Suggest exporting sync_datastores_with_cursor (or a thin pub wrapper taking Option<&Datastore> for the cursor store) in this PR so the follow-up harness can run the same operation model through both paths.

On the timestamp path, same result at c535d77 (master) and 1133053 (this head), 0.2 s per run: seed_793 (backfill behind cursor) fails, a second distinct event at the same timestamp is dropped, and editing a zero-duration event after sync leaves the pre-edit row in dest (proptest-shrunk, 3 ops). Expected for the fallback path; noted so the harness's first green run is read against the rowid path, not this one.

[TimeToBuildBob]

Convergence adjudication deferred (freshness rechecked 2026-10-10): current head is 6622f9ce9439. The newest Bob AI review covers that head, but Greptile’s last reviewed commit remains 0a0af08b, preceding the subsequent fixes and rebase. Under this adjudication’s freshness gate, I cannot classify or dismiss findings until review provenance covers the current head. No findings resolved, no merge recommendation, and no additional Greptile round requested. All seven current-head CI check runs pass. The three current-head AI-review findings remain open for the next review/fix reaction; this deferral does not dismiss them or declare the PR merge-ready.

[TimeToBuildBob]

Rebased onto current master (faeded2), pushed as 6622f9c. GitHub now reports mergeable / CLEAN.

The only conflict was in the datastore response enum: preserved master's FilteredEvents variant alongside this PR's EventPage variant. git range-diff confirms the 18 PR commits are retained; sync logic is unchanged by the rebase.

Local verification: cargo test -p aw-sync -p aw-datastore — 285 passed, 1 ignored; clippy with -D warnings and formatting checks pass. This includes the latest upgrade-bootstrap regression. New-head CI will run separately; no additional Greptile round requested.

[TimeToBuildBob]

Convergence adjudication deferred (freshness recheck, 2026-10-11): current head is 472dfa1ebb00, which postdates the newest Bob AI review (6622f9ce9439, 2026-10-10) and Greptile's last reviewed commit (0a0af08b, reviewed 2026-10-08). Under this adjudication's freshness gate I cannot classify findings against a head no review pass has seen.

State at 472dfa1ebb00:

• 0 unresolved review threads — the three 2026-10-10 findings were adjudicated: two P1 known-tradeoff items replied+resolved, and the P2 empty-batch cursor-save bug fixed in 472dfa1.
• All 7 CI checks pass; PR is MERGEABLE.
• No merge recommendation is posted and no additional Greptile round is requested (Greptile is in backoff at the prior head; the bob-ai-review-sweep.timer re-reviews new heads automatically).

Awaiting the automatic re-review of 472dfa1; a later cycle can adjudicate once review provenance covers this head. Maintainer merge is still required (Bob is pull-only on ActivityWatch repos).