feat(sync): reconcile owner-originated event edits

[TimeToBuildBob]

Why

ActivityWatch/aw-android#253: manual title edits show up in JSON export but often never reach the sync folder. Deleting the visible test.db does not help.

Cause

sync_one resumes at the destination's latest event end. WebUI/Android edits are delete+insert at the same timestamp, so they sit outside that window. The latest event is re-fetched as a start-clipped fragment; heartbeat(pulsetime=0) will not merge different data and inserts a duplicate instead.

Android writes an internal staging db, then mirrors it outbound to SAF. Deleting the visible file only deletes the copy.

Change

Before the incremental copy, match source and dest events in the 7 days before the resume cursor by (timestamp, duration) and replace dest rows whose data changed. Insert the replacement before deleting the stale row. Identity includes duration so two events that share a start time are not collapsed.

Not in this PR: event deletion, edits older than 7 days, bucket-ID migration.

Tests

cargo test --package aw-sync --test historical_edit

On 626af70 those cases failed as described in the issue (historical title stuck, latest-event clipped duplicate, peer never updated). They pass here.

Fixes the sync-side of ActivityWatch/aw-android#253. Leave that issue open until a release ships this.

[greptile-apps]

Confidence Score: 4/5

The PR is not yet safe to merge because reconciliation can propagate deletion of a same-identity sibling, and the repository’s performance-measurement requirement remains unsatisfied.

Findings

1.  Sibling Deletions Propagate ▶
2.  Reconciliation Can Delete Siblings ▶
3.  Reconciliation Loads Entire Backlog ▶
4.  Performance Measurement Missing ▶

Summary

• Compares source and destination events from the seven days preceding the resume cursor.
• Groups equal timestamp-and-duration siblings and replaces unmatched destination payloads.
• Adds destination event deletion support to local and HTTP access methods.
• Documents the edit window and adds integration tests for historical edits, sibling handling, and large post-cursor backlogs.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Determine destination resume cursor] --> B[Fetch source and destination events in 7-day lookback]
    B --> C[Group events by timestamp and duration]
    C --> D[Match equal payload siblings]
    D --> E[Insert unmatched source payloads]
    E --> F[Delete unmatched destination rows]
    F --> G[Run paginated incremental copy from cursor]

Loading

_{Reviews (2) · Last reviewed commit: "fix(sync): bound edit reconcile and keep..."}

[codecov]

Codecov Report

❌ Patch coverage is 95.28796% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.69%. Comparing base (656f3c9) to head (eb7e751).
⚠️ Report is 115 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-sync/tests/historical_edit.rs	95.70%	14 Missing ⚠️
aw-sync/src/accessmethod.rs	50.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #678      +/-   ##
==========================================
+ Coverage   70.81%   80.69%   +9.87%     
==========================================
  Files          51       73      +22     
  Lines        2916     7205    +4289     
==========================================
+ Hits         2065     5814    +3749     
- Misses        851     1391     +540     

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

Addressed the two Greptile P1s in be1d36f: group-match same-identity siblings so a later source row cannot delete a dest sibling that still matches, and bound both reconcile fetches at end=resume so a far-behind dest cannot load the post-cursor backlog. Local cargo test --package aw-sync is green. Re-requesting review.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

Greptile 4/5 on be1d36f: no code change.

• P1 sibling-delete propagate: accepted tradeoff (same shape as insert-before-delete crash recovery). Unique-identity deletion is already not synced.
• P2 perf measurement: false positive (pytest-profiling rule on a Rust memory bound).

CI green, mergeable. Waiting for human review.

[ErikBjare]

@TimeToBuildBob Adding this to the v0.14.x queue as a stopgap — decision on ActivityWatch/activitywatch#1445. It fixes ActivityWatch/aw-android#253 for the format Android users run today, and the v2 sync format replaces sync_one wholesale (day-partition range-replace makes edits and deletes propagate at any age, no window), so this gets deleted rather than maintained.

Please rebase-check against #697 once that lands — both touch sync.rs. Order: #697 → this.

[TimeToBuildBob]

Acknowledged: v0.14.x stopgap for ActivityWatch/aw-android#253, then deleted when v2 replaces sync_one. Order: #697 → this.

Preview rebase-check against current #697 head 67eeee7 (still OPEN — not pushing this speculative rebase): 7/7 commits replayed cleanly. Both PRs touch aw-sync/src/sync.rs, but the hunks do not overlap:

• fix(aw-sync): sanitize whitespace hostnames on import; make per-bucket errors non-fatal #697: sanitize_hostname, hostname resolution in get_or_create_sync_bucket, per-peer continue in sync_datastores
• this: reconcile_updated_events + delete_events_by_id on AccessMethod, called from sync_one

Will rebase onto master after #697 merges and re-run cargo test --package aw-sync.

[ErikBjare]

@TimeToBuildBob Before this goes to Erik: rebase onto master. The branch is based on 626af70, 10 commits behind; four of those touch aw-sync/src/sync.rs — #698 (the ds_localremote block), #687 (fail-loud warnings), #686 (dedupe), #681. GitHub reports MERGEABLE because none of them overlap your hunks textually, but reconcile_updated_events runs inside a sync_one that has changed underneath it and historical_edit.rs has never executed against the current one. Once rebased and CI is green on the new base, this moves ahead of #697 in the queue — it is ready and #697 is not.

Content looks right from a structural pass: 7-day lookback constant, (timestamp, duration) identity, replacements inserted before stale rows are deleted, nine tests. Full read when it is rebased.

[TimeToBuildBob]

Rebased onto master (now at 1379d77, 10 commits including #698, #687, #686). All 13 cargo test --package aw-sync tests pass locally. Waiting for CI.

[TimeToBuildBob]

🤖 AI code review

Safe to merge — 2 findings disposed (accepted-tradeoff; rejected)

_{Updated after inline dispositions on finding threads — this is the current state; the verdict below is frozen at review time and is kept as the historical record of that pass.}

Finding disposition

Finding	Severity	State
aw-sync/src/accessmethod.rs:71	P1	superseded by latest review (not reproduced)
aw-sync/src/sync.rs:558	P1	accepted-tradeoff
aw-sync/src/sync.rs:491	P1	superseded by latest review (not reproduced)
aw-sync/src/sync.rs:491	P1	rejected — The verification step claims end=resume excludes events that start before resume and end after it. That is wrong: get_ev

Adds a reconcile pass to aw-sync that, before the incremental copy, compares source and destination events in the 7 days before the resume cursor by (timestamp, duration) and replaces destination rows whose data changed, inserting the replacement before deleting the stale row. Adds a new AccessMethod::delete_events_by_id trait method with Datastore and AwClient implementations, updates the README limitation note, and adds an integration test file covering historical/latest edits, same-identity siblings, lookback bounds, and far-behind destinations.

Not safe to merge — 2 P1 open

Confidence 2/5

2 findings · ❌ 2 P1

❌ P1 high — aw-sync/src/sync.rs:556

The reconcile pass uses event_identity = (timestamp, duration) and matches source rows to destination rows by that key. When a source event is edited, the WebUI/Android path deletes the old row and inserts a new row with the same timestamp and duration but new data. The reconcile loop removes matching destination rows from dsts and inserts the source rows. However, the destination rows that are not matched (because their data differs) are collected as stale and deleted. The problem: if the source has multiple events with the same identity (same timestamp and duration) and only one is edited, the loop iterates over srcs and for each source row, if a destination row with equal data exists, it removes it; otherwise it pushes to to_insert. After processing all source rows, any remaining dsts are considered stale and deleted. This is correct for the edit case. But consider a scenario where the source has two identical events (same timestamp, duration, and data) and the destination has only one of them (e.g., because the other was never synced or was deleted). The loop will match the first source row to the destination row, remove it, and the second source row will be pushed to to_insert. Then to_insert is non-empty and dsts is empty, so it inserts the second source row. That is fine. The real issue is when the destination has an extra event with the same identity that does not exist on the source. The loop will not match it (since no source row has equal data), so it remains in dsts and gets deleted. That is the intended behavior for edits (the old row is stale). But what if the destination has an event that the source no longer has because it was deleted? The PR explicitly says 'Event deletion is not synced', so this reconcile pass will delete destination events that are not present on the source, effectively syncing deletions for events within the lookback window. This contradicts the stated limitation and could cause data loss: if a user deletes an event on the source (not an edit), the next sync will delete the corresponding destination event, even though deletion is supposed to be unsupported. The mechanism: dsts contains all destination events in the window with that identity; srcs contains all source events with that identity. Any destination event whose data does not match any source event is treated as stale and deleted. If the source event was deleted entirely, srcs is empty, so the loop body is skipped (the for (identity, srcs) loop only iterates over identities present in src_by_identity). Wait, if the source event is deleted, src_by_identity will not have that identity, so the loop never runs for that identity, and the destination event is not deleted. So deletion is not synced. The stale deletion only happens for identities that still exist on the source but with different data. So the deletion of unmatched destination rows is only for edited events, not for deleted events. That is correct. However, there is a subtle bug: if the source has an event with identity X and data A, and the destination has two events with identity X, one with data A and one with data B (e.g., a duplicate or a sibling that was not edited), the loop will match the source row to the data-A destination row, remove it, and the data-B destination row remains in dsts and is deleted. This would delete a legitimate sibling that was not edited. The tests cover same-identity siblings with different data (alpha and bravo) and ensure they survive, but that test has two source rows with the same identity and different data. In that case, srcs has two rows, and the loop matches each to the corresponding destination row, so both are kept. The bug would occur if the source has only one row with that identity but the destination has two rows with the same identity and different data, one of which matches the source and one does not. That could happen if a sibling was deleted on the source (but deletion is not synced, so the destination still has it) or if a sibling was added on the destination (unlikely). The reconcile would delete the unmatched sibling. Is that a real scenario? The PR's own test same_identity_sibling_edit_keeps_the_other edits one sibling and keeps the other, but in that test the source still has both siblings (one edited, one unchanged). The loop processes both source rows, so both destination rows are matched. The bug would require the source to have fewer rows with that identity than the destination. For example, source has one event (timestamp T, duration D, data A), destination has two events (T,D,A) and (T,D,B). The loop matches the source to the first, and the second is deleted. This could happen if the destination has a duplicate that was not cleaned up, or if a sibling was deleted on the source (but deletion is not synced, so the destination still has it). Since deletion is not synced, the destination may have extra events that the source no longer has. The reconcile pass would delete those extra events if they share an identity with a source event. This is a real data-loss risk: a user deletes an event on the source (which is not synced), but if another event with the same timestamp and duration still exists on the source, the deleted event's destination copy will be removed as 'stale'. The PR claims deletion is not synced, but this reconcile pass effectively syncs deletions for events that share an identity with a surviving source event. The severity is P1 because it can cause unexpected data loss on the destination. The fix would be to only delete destination rows that are 'replaced' by a source row with the same identity but different data, not all unmatched rows. Specifically, when a source row with data A is matched to a destination row with data B (different), the destination row should be deleted and the source row inserted. But if there are extra destination rows with no corresponding source row, they should be left alone. The current algorithm deletes all unmatched destination rows after processing all source rows. To fix, track which destination rows were matched and only delete those that were matched to a source row with different data, not those that were never matched. Actually, the current algorithm removes matched rows from dsts and then deletes the remaining. The remaining are those that were not matched to any source row. If a source row with data A matches a destination row with data A, it is removed and not deleted. If a source row with data A matches a destination row with data B, it is not removed (since data differs) and goes to to_insert, and the destination row remains in dsts and is deleted. That is correct for an edit. But if there is an extra destination row with data C that has no source counterpart, it remains in dsts and is deleted. That is the bug. The fix is to only delete destination rows that were 'replaced' by a source row with different data, not all remaining. One way: for each source row, if a destination row with equal data exists, remove it; else, if a destination row with different data exists, remove it and mark it for deletion, and push the source row to insert. But if there are multiple destination rows, you need to decide which one to replace. The current algorithm does not distinguish between 'replaced' and 'extra'. A safer approach: only delete a destination row if it is the one that was matched to a source row with different data. But the algorithm removes matched rows from dsts only when data is equal; when data differs, it does not remove the destination row, so it remains and is deleted. That is the intended replacement. The problem is that it also deletes destination rows that were never matched at all. To avoid that, you could track which destination rows were 'consumed' by a source row (either equal or different) and only delete those that were consumed with different data. But the current code deletes all remaining dsts after the loop. The fix is to only delete dsts that were matched to a source row with different data. Since the loop does not remove them when data differs, you need a different structure. A simple fix: for each source row, if there is a destination row with equal data, remove it; else, if there is any destination row, remove one and add it to a 'to_delete' list, and push the source row to insert. After processing all source rows, delete only the rows in 'to_delete', not all remaining dsts. This would preserve extra destination rows that have no source counterpart. The current code deletes all remaining dsts, which is too aggressive. This is a real bug that can cause data loss on the destination when the destination has more events with the same identity than the source. The tests do not cover this case because they always have the same number of source and destination rows per identity. I will report this as a P1 bug.

Track which destination rows are actually replaced by a source row with different data, and delete only those. For example, maintain a `to_delete` list and only remove a destination row from `dsts` when it is matched to a source row (either equal or different), then delete only the rows that were matched with different data. Leave unmatched destination rows untouched.

_{How this was verified: Traced the reconcile loop: for each identity, dsts is the full list of destination events with that identity. The inner loop removes only those with equal data; all others remain in dsts and are collected as stale and deleted. If the source has fewer events with that identity than the destination (e.g., a sibling was deleted on the source, which is not synced), the extra destination rows are deleted. Checked the test file: all tests have equal source/destination counts per identity, so none exercise this.}

❌ P1 high — aw-sync/src/sync.rs:491

The reconcile pass fetches source events with end=resume, but the source bucket may contain events that start before resume and end after resume (the live last event). The comment says 'Do not use end>resume: the dest-latest event starts before resume and must still be title-reconciled', but the fetch itself uses end=resume, so a source event that starts before resume and ends after resume is excluded from source_events entirely. The dest-latest event is also fetched with end=resume, so it is excluded from dest_events too. This means a title edit on the live last event (which starts before resume and ends after resume) is never reconciled by this pass. The test latest_title_edit_reaches_staging uses t1 = now - 1h + 40min, duration 15min, so t1+duration is before now, meaning the event ends before the resume cursor (which is the dest's latest event end, also before now). The test does not cover an event that spans the resume cursor. The consequence is that a title edit on the currently-active event (which is the most common case for a user editing the current window title) will not be synced, and the incremental copy will re-fetch it as a start-clipped fragment, causing heartbeat to insert a duplicate as described in the PR's own cause section.

Change the source fetch to use end=None (or end=Some(resume + EDIT_RECONCILE_LOOKBACK)) and rely on the timestamp < resume filter in the loop. For example: let source_events = ds_from.get_events(bucket_from.id.as_str(), Some(lookback_start), None, None)?;

_{How this was verified: Checked the fetch range in reconcile_updated_events: source_events and dest_events both use end=Some(resume). The comment at lines 513-515 explicitly discusses the dest-latest event starting before resume, but the fetch excludes it because end=resume clips events that start before resume and end after. The test latest_title_edit_reaches_staging uses t1 = now - 1h + 40min with duration 15min, so t1+duration is before now; the resume cursor is the dest's latest event end, which is also before now, so the event does not span the cursor. No test covers an event spanning the cursor.}

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/accessmethod.rs:121

The AwClient::delete_events_by_id implementation loops over event IDs and calls AwClient::delete_event for each, returning on the first error. This is not atomic: if the destination is an HTTP server and one delete fails (e.g., network timeout), the earlier deletes have already been applied, and the function returns an error, causing the whole sync pass to abort. The reconcile pass then leaves the destination in a partially-updated state: some stale rows deleted, some not, and the replacement insert may not have happened (since insert happens before delete, but if delete fails after insert, the new row is there and the old row may still be there, causing a duplicate). The PR's own comment says 'Insert before delete so a crash cannot drop the row', but a partial delete failure can still leave duplicates. More importantly, the per-event loop is inefficient and not idempotent: if the same event ID appears twice (unlikely), the second delete would 404. The Datastore implementation uses a single delete_events_by_id call which is atomic within the datastore. The AwClient implementation should ideally batch deletes if the API supports it, but it doesn't. The real issue is that a failure mid-loop leaves partial state, and the error propagates, aborting the sync. This is a robustness issue, but the PR's design already accepts non-atomicity for insert+delete. The severity is P2 because it only manifests on network errors, and the sync will retry on the next pass, but the partial state could cause duplicates that the next reconcile may or may not clean up. I'll report as a P2 trade-off/guard.

_{How this was verified: Checked the AwClient implementation: it iterates and returns on first error. The Datastore implementation uses a single call. The reconcile pass calls delete_events_by_id after insert_events, so a failure here leaves the insert applied but some deletes not.}

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

• aw-sync/README.md — Updates the limitations bullet to state that event deletion is not synced and owner-originated edits are reconciled within 7 days of the resume cursor.
• aw-sync/src/accessmethod.rs — Adds delete_events_by_id to the AccessMethod trait and implements it for Datastore (with force_commit) and AwClient (per-event HTTP delete).
• aw-sync/src/sync.rs — Adds event_identity, EDIT_RECONCILE_LOOKBACK, and reconcile_updated_events, and calls it from sync_one before the incremental copy.
• aw-sync/tests/historical_edit.rs — Adds integration tests for historical and latest title edits, same-identity siblings, lookback bounds, far-behind destinations, and destination wipe.

Previous review passes

commit	score	findings	engine	when
1379d77aeb4a	2/5	3	llm	2026-09-16 08:56 UTC

_{Reviewed eb7e751c401a · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 183s · about this reviewer}

_{Maintainer commands}

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

[ErikBjare]

@TimeToBuildBob Full read done now that it is on current master (1379d77, own diff still exactly +607/−1).

The logic is right. Bounded to a 7-day window ending at the cursor with end = Some(resume) so a far-behind destination cannot pull every newer source event into memory; identity (timestamp, duration_ns) so same-timestamp siblings are not collapsed; source rows at/after the cursor left to the incremental copy; exact-data matching so unchanged rows are untouched; replacements inserted before stale rows are deleted, with the "a later pass sees matching data and only deletes" crash story spelled out. Duration-only edits get replaced rather than heartbeat-merged, which is a correct outcome even though the comment says otherwise. Nine tests.

One required change: no .unwrap() on datastore calls inside sync_one. reconcile_updated_events has four — two get_events, insert_events, delete_events_by_id. This is the function #669 deliberately converted to ? because a panic here aborts the whole pass, and on Android it is the ActivityWatch/aw-android#220 class (#681 added a guard at the JNI boundary, but a panic still kills the pass and any partially-applied reconcile). Return Result and let the per-bucket skip in #697 handle it — a peer db read failing mid-window should cost that bucket's reconcile, not the daemon.

With that, merge on green. Queue position unchanged: after #700.

[TimeToBuildBob]

@ErikBjare Done on eb7e751. reconcile_updated_events now returns Result<(), String>; the four datastore calls (get_events ×2, insert_events, delete_events_by_id) use ?. sync_one propagates with ? so #697's per-bucket skip handles a mid-window read failure instead of panicking the pass (or the JNI frame). force_commit in delete_events_by_id matches the other AccessMethod methods.

Local cargo test --package aw-sync green, including the 9 historical-edit tests.

[ErikBjare]

Verified on eb7e751: no .unwrap() left in reconcile_updated_events, now -> Result<(), String> and routed through the per-bucket skip. Nothing further — ready for Erik after #700.

[TimeToBuildBob]

Not adjudicating remaining AI-review threads this cycle: last review saw 1379d77, head is eb7e751. Automatic re-review of current head should clear the two unwrap P1s; the sibling-delete P1 is the same accepted tradeoff as the Greptile thread (unique-identity deletes already aren't synced). Not re-triggering Greptile.

CI still green. Holding for #700.

[TimeToBuildBob]

Merge recommendation (Greptile convergence adjudication). Not merging — maintainer judgment.

Fixed this session

• None in code. Head is still eb7e751c401a. Unwraps/Result already landed there; Erik verified at 10:14Z.

Remaining (non-blocking) — classified on eb7e751c401a

• AI-review P1 3e53771e28c0 (sync.rs:556, sibling leftover-delete): accepted-tradeoff. Same as the resolved Greptile "Sibling Deletions Propagate" thread. Unique-identity deletes are not visited. Leftover dest rows for an identity still on the source are the insert-before-delete crash-recovery path; capping deletes at to_insert.len() would leave the stale row after a crash. Same-identity siblings are not the product path.
• AI-review P1 65cddccf2896 (sync.rs:491, end=resume "excludes" spanning events): rejected. get_events is overlap (starttime <= end AND endtime >= start) plus clip, not containment. dest-latest ends at resume, so it is returned and the clip is a no-op on (timestamp, duration). A live event spanning the cursor is also returned, clipped to dest-latest's identity. Suggested end=None reopens the far-behind Android OOM (Greptile P1 already fixed in be1d36fa) and would break that identity match.
• AI-review P2 advisory (summary-only, no thread): AwClient delete_events_by_id is per-id HTTP, not atomic. Accepted: insert-before-delete is already non-atomic; next pass is idempotent. No code change.
• Greptile: 0 unresolved P1/P2. Last reviewed commit be1d36fa (pre-rebase). Did not re-trigger; cap hit, logic unchanged since that pass.

Verified against: eb7e751c401a3a688b1bf4c406e20088af9322a3 (AI-review marker sha eb7e751c401a matches head).

CI: all green (Android, coverage, clippy, format, macOS, ubuntu, windows). MERGEABLE.

Domain risk: Android title-edit of a historical event and of dest-latest (the ActivityWatch/aw-android#253 path). No datastore panic path left in reconcile_updated_events. Event deletion still not synced, by design.

Convergence: round_convergence.stable_rounds = 0 (last Greptile round had new blocking; those threads were resolved as tradeoff/false-positive, no further Greptile pass). Detector verdict: converged, 0 unresolved P1/P2.

Queue: #700 merged 14:07Z, so Erik's "ready after #700" gate is open. Branch is 1 commit behind master (that merge) but GitHub reports MERGEABLE. v0.14.x stopgap; deleted when v2 replaces sync_one.

[TimeToBuildBob]

Merge verified on 3d3b9dd (2026-09-17T08:47:23Z): all checks green (ubuntu/macOS/windows/clippy/format/coverage/Android). Nothing outstanding on this branch — no maintainer comments pending, no unresolved review threads requiring adjudication.

Downstream note (not a request, just so the pin arithmetic is on the record): 3d3b9dd is exactly one commit ahead of 37aa0b0 (#699's merge), and the only commit in 37aa0b0..3d3b9dd is this one. ActivityWatch/aw-android#285 currently pins 37aa0b0, so merging it as-is leaves the Android app without this fix and ActivityWatch/aw-android#253 unfixed there until a later submodule bump. Flagged on that PR (comment) — Erik's call whether the bump folds in here or lands separately.