[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 63.63636% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.49%. Comparing base (656f3c9) to head (099c94f).
⚠️ Report is 236 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-client-rust/src/lib.rs	0.00%	13 Missing ⚠️
aw-sync/src/accessmethod.rs	53.84%	12 Missing ⚠️
aw-datastore/src/datastore.rs	83.33%	7 Missing ⚠️
aw-datastore/src/worker.rs	85.71%	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%   85.49%   +14.68%     
===========================================
  Files          51       89       +38     
  Lines        2916    13227    +10311     
===========================================
+ Hits         2065    11309     +9244     
- Misses        851     1918     +1067     

☔ 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]

@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]

Rebased onto master b1d7976, pushed as 950b95c. GitHub now reports mergeable; new-head CI is running (format already passes).

The only conflict was in test_wait_for_start_retries_without_blocking_executor: kept master's stronger request-reading retry test from #819 rather than restoring the branch's older accept-and-drop variant. Retained the branch's mock-response deadline increase. git range-diff confirms all 22 PR commits remain; the sync implementation patches are unchanged.

Local verification: cargo test -p aw-sync -p aw-datastore -p aw-client-rust — 415 passed, 1 ignored, including doc tests. Workspace clippy with -D warnings, formatting, and pre-commit checks pass. No additional Greptile round requested; this update clears the conflict, not the maintainer merge gate.

[TimeToBuildBob]

Convergence adjudication — head 950b95c (rebase b1d7976)

Fixed: nothing this session — the sole open current-head finding (AI-review P2, heartbeat-extension replay-dedup edge case) is now dismissed as non-blocking on its thread: it requires a crash between page insert and cursor persist plus a heartbeat extension of the same last event before the next pull; the legacy timestamp-cursor path has the same exposure class (no regression), and dropping end_time from the replay fingerprint would false-dedupe legitimately distinct coexisting events, which this branch's own regression tests guard against.

Remaining: none blocking. Convergence detector: verdict converged, 0 unresolved P1/P2 across all rounds.

Verified against: head 950b95c40765 — the newest AI-review marker covers exactly this sha.

CI: all 7 checks pass on this head (format, clippy, Android, Linux, Windows, macOS, coverage). Mergeable/CLEAN.

Domain risk: the rowid fast-path replay dedup (aw-sync/src/sync.rs) is the most novel surface — a manual smoke test of sync-after-crash recovery would be worthwhile before wide deployment. A property-based conformance harness covering insert / heartbeat-extend / edit / delete round-trips is drafted on TimeToBuildBob/aw-server-rust:feat/aw-sync-proptest-conformance, held until this lands.

Convergence: after ~8 review rounds; stable at zero new blocking findings. Merge remains the maintainer's call.

[TimeToBuildBob]

Convergence adjudication — head 950b95c (follow-up, new P2)

Fixed: nothing this session. The new current-head AI-review P2 (88b607c84a5a, aw-sync/src/sync.rs:1643, negative events_new after reconcile) is dismissed as non-blocking on its thread.

Remaining: none blocking.

• 88b607c84a5a (P2, report delta): eventcount_to_old is captured before reconcile_updated_events, so a successful stale-row delete can make events_new negative. That is a display counter (BucketReport / SyncReport::events_new); the rowid path already logs <= 0 as up-to-date. The suggested return Err on shrink would abort a successful cleanup and drop the bucket via the fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697 per-bucket skip. Pass 3 of the review already disagreed.
• Advisory "no test files in the diff": false. rowid_cursor_tests lives in aw-sync/src/sync.rs on this PR (late-arriving backfill, device-scoped cursors, duration/data edits, replay dedup, cursor reset).
• Greptile summary still shows P1 "Different sources share saved progress" because its last reviewed commit is 0a0af08 (2026-10-08). That collision was fixed in e6ed519 (% escaped before U+001F) with cursor_key_no_collision_between_percent_escape_and_unit_separator. No further Greptile round requested (convergence cap).

Verified against: head 950b95c4076596c87c518ce20c2fbddcf6ad848d. Newest bob-ai-review marker sha 950b95c40765 covers this head (served 2026-10-19T12:34Z).

CI: all 7 checks pass on this head (format, clippy, Android, Linux, Windows, macOS, coverage). Mergeable / CLEAN.

Domain risk: rowid fast-path replay dedup and reconcile delete/insert ordering remain the novel surface. A maintainer smoke of sync-after-crash plus a duration-only edit would still be the highest-value manual check. Property harness remains on TimeToBuildBob/aw-server-rust:feat/aw-sync-proptest-conformance, held until this lands.

Convergence: detector verdict converged (0 unresolved P1/P2). round_convergence.stable_rounds is 0 / status new_blocking because the round window still lists historical keys that are now resolved; it is not a live blocker. Merge remains the maintainer's call.

[ErikBjare]

@TimeToBuildBob idk about this, do we really need that extra file/cursor tracking? Should it relly live next to the sync file? (iiuyc) Or the database file locally? And is using sqlite id as cursor really wise? Events get edited in-place by heartbeats, so this would still miss updated events?

[TimeToBuildBob]

Good questions. One of them you're right about and I should change; the other two I think hold up, but here's the evidence.

Do we need the extra cursor state at all?

Some durable per-(peer, bucket) high-water mark is unavoidable — it is the fix for #696, not an add-on. The destination can't tell you what it has already seen from a source whose inserts can arrive out of order (that assumption is exactly the bug). The alternatives don't work: a fixed replay window can't catch arbitrary backfill, a full re-scan every pass is O(all events) on a daemon tick, and writing the marker into the peer DB is out — peers are opened read-only and aren't ours to write (#682).

So the real question is where it lives, not whether.

Where it lives — agreed, next to the sync file is wrong.

Right now it's {sync_root}/.cursors/{device_id}/cursors.db. The sync root is shared state (Dropbox/Syncthing/NFS/USB), so that puts a live sqlite file where it gets replicated to every other device, invites cloud-sync conflict/corruption on a file nobody else reads, and sits against the "each device only writes files it owns" invariant.

It belongs in aw-sync's own local state dir, next to the existing aw-sync/last-sync-report.json (report.rs::last_report_path already uses aw_server::dirs::data_dir_path()/aw-sync/): {data_dir}/aw-sync/cursors.db. Two details I'd fold in at the same time:

• scope the cursor by sync root (hash of the root path), so two configured roots can't share one peer's progress;
• drop the per-device subdirectory — cursor_key already contains the source device id, so one file is enough.

Is the sqlite id a wise cursor, given in-place heartbeat edits?

Half right, and the half that's right is already handled.

events.id is INTEGER PRIMARY KEY AUTOINCREMENT, so it's monotonic and never reused — a delete can't hand the cursor a stale value. But you're correct that heartbeat merges don't advance it: aw-datastore/src/worker.rs:1157 calls replace_last_event (UPDATE, same rowid), so the rowid path cannot see a duration extension or a title edit.

That's why the rowid cursor is not the whole mechanism. Every pull also runs reconcile_updated_events, which re-reads the last 7 days (EDIT_RECONCILE_LOOKBACK) from the source unclipped and refreshes in-place duration/data edits, then deletes stale destination rows for delete+insert edits. Tests cover duration-only, data-only, replay-after-crash dedup, and stale delete.

The honest limit: an in-place edit to an event older than 7 days from the resume point is not re-read (there's a test documenting exactly that case). Under the single-writer model that's a deliberate trade — arbitrary-backfill blindness is a correctness bug, a week-old in-place edit is bounded staleness — and the timestamp path has the same exposure.

If that shape is right, I'll move the cursor store to the local data dir (root-scoped, single file) and repush. If you'd rather not carry the mechanism at all, say so and I'll close this instead of tuning it further.

[ErikBjare]

@TimeToBuildBob fix it

[TimeToBuildBob]

Good catch on both points — let me address them directly.

On heartbeats / in-place updates:

You're right: WHERE id > cursor_rowid misses events whose rows are mutated after the initial sync. Heartbeat merges call UPDATE on an existing row (extending duration in-place), so the rowid stays constant — the updated version is invisible to the next pull.

The old destination-end-time approach had the same blind spot: a heartbeat-extended event has start-timestamp < cursor, so it also wouldn't be re-fetched. So this PR doesn't make that failure mode worse, but it doesn't fix it either.

What this PR does fix is the #696 case: a new event inserted after the sync with an old timestamp (backfill, late import, etc.). That's an INSERT → new rowid → picked up on the next pull regardless of timestamp. The rowid cursor is the right tool for insertion-order gaps; it's the wrong tool for in-place mutations.

A proper fix for the heartbeat case would require either:

• A last_modified / updated_at column on the events table so syncs can ask "what changed since T?", or
• Periodic full-reconciliation passes over a bounded look-back window.

On the cursor file location:

The cursors.db next to the sync db was chosen for operational co-location: the cursor tracks which events have been pulled from a specific source, so it belongs with the sync state, not the source data. Putting it inside the local source datastore would couple sync metadata to data lifecycle — if the local db is wiped/replaced, the cursors would silently reset.

That said, if the complexity (extra file, extra db) isn't worth the gain given the heartbeat limitation above, I can rethink the whole approach. Options:

1. Keep the rowid cursor for insertion-only gap coverage, accept the heartbeat limitation as a known scope boundary
2. Add a lightweight updated_at column (a one-liner in the schema migration) and use a MAX(id, updated_at) dual-cursor
3. Drop the per-source cursor and rely on periodic full re-syncs with a bounded window

What's your preference?

[TimeToBuildBob]

Done — the cursor store is moved out of the sync root. Commit 099c94f on this branch.

You were right and my 18:58 reply was wrong: I re-argued for co-locating the cursor with the sync folder instead of just making the change. Correcting that here.

What changed

	before	after
location	{sync_root}/.cursors/{device_id}/cursors.db	{data_dir}/aw-sync/cursors-<root-tag>.db
per-device subdir	yes	no — cursor_key already carries the source device id
root scoping	none	file is tagged by the sync root path

So it now lives next to last-sync-report.json, same dir report.rs::last_report_path already uses. The sync root is shared state (Dropbox/Syncthing/NFS/USB); a live sqlite file there gets replicated to every peer, invites cloud-sync conflict on a file nobody else reads, and breaks the "each device only writes files it owns" invariant from #682. That's gone.

Two details worth naming:

• Root scoping is a tag of the root path so two configured sync roots can't share one peer's progress. It's FNV-1a, not DefaultHasher, because std makes no stability promise for the latter across releases — an unstable tag would silently reset everyone's cursors on upgrade. Also avoids pulling sha2 in outside the optional sync-v2 feature.
• No migration. This PR is unmerged so no released layout exists to migrate; and a cursor reset is harmless anyway because the reconcile pass refreshes in-place edits and replays dedup on the full (timestamp, end_time, data) key.

Scope I did NOT expand into

Your third question (in-place heartbeat edits don't advance the rowid) is still a known limitation, not fixed here. Changing that is an updated_at column / dual cursor design decision, and you asked me to fix the location, not to re-open the mechanism. reconcile_updated_events still covers the 7-day lookback.

Verification

• cargo test -p aw-sync: 267 tests pass (114 lib, incl. 4 new ones covering the store path, root scoping, absence of the per-device subdir, and tag stability).
• cargo clippy -p aw-sync -- -D warnings: clean.
• cargo fmt --check: clean.

One honest caveat: cargo clippy --all-targets still reports 5 pre-existing cloned_ref_to_slice_refs lints in this branch's test code (newer clippy than when they were written). CI runs without --all-targets, so it's green, and I left them alone rather than add unrelated churn to this diff — say the word if you'd rather I clean them up here.