Skip to content

fix(cascade): commit watcher upserts for a path in delivery order - #462

Merged
0xKT merged 2 commits into
mainfrom
fix/watcher-upsert-order
Sep 24, 2026
Merged

0xKT merged 2 commits into
mainfrom
fix/watcher-upsert-order

Conversation

@gloryfromca

Copy link
Copy Markdown
Member

Summary

Two watcher events for the same file could reach md_change_state in either order: each event schedules its own upsert coroutine and each upsert awaits the database, so the last committer won. Windows synthesises a created for every file under a freshly created parent directory, which hands the handler the same file four or five times. On the Windows soak box (integration branch of the open PRs, 630de2d) one of those stale duplicates committed after an atomic save's added and put the first write's mtime and lsn back on the row: test_atomic_replace_over_existing_target_keeps_the_row_alive (from #454) failed 1 run in 4 there and nowhere else.

Fix: one asyncio.Lock per handler around the upsert. Tasks are created in delivery order and the lock is FIFO, so the row ends with the last event. Pre-existing on main; Windows only makes it visible. The scanner's sweep still writes on its own path (disk is the truth there).

Area

  • Architecture / core
  • Benchmark
  • Use case
  • Docs
  • DX
  • CI / build / release

Verification

  • tests/unit/test_memory/test_cascade/test_watcher_upsert_order.py: a repo double whose first upsert commits 50 ms late, two created events with distinct mtimes. Against the previous watcher.py it fails with committed == [m2, m1]; with the lock it passes.
  • uv run pytest tests/unit/test_memory/test_cascade -k watcher: 6 passed; lint-imports: 4 kept.
  • Diagnosis on the box (not in the repo): NTFS mtimes differ for any write gap ≥ 2 ms (420 trials), watchdog delivered deleted+moved within 3–13 ms in 30/30 replaces, and the product path updated the row in 22–112 ms in 15/15 probes outside pytest — the failure only reproduced with the synthetic duplicate created events of a fresh directory tree, which is what the ordering fix addresses. The Windows re-run of the flaky test on the fixed code follows with the next soak launch (the box's checkout is the running soak's code).

Checklist

  • make lint clean for the touched files (ruff + import-linter)
  • Tests added and shown to fail without the fix
  • No CJK / no dead code introduced

Notes for Reviewers

Why a lock and not a monotonic WHERE excluded.mtime >= mtime on the upsert: a deleted event carries mtime 0.0 (the file is gone) and must still win over an earlier added; order, not mtime, is the invariant.

🤖 Generated with Claude Code

Each watcher event schedules its own upsert coroutine on the loop, and each
upsert awaits the database, so two events for the same path could commit in
either order and the last committer won. Windows synthesises a 'created' for
every file under a freshly created parent directory, which hands the handler
the same file four or five times; on the Windows soak box one of those stale
duplicates committed after an atomic save's 'added' and put the first write's
mtime and lsn back on the row (test_atomic_replace_over_existing_target_keeps
_the_row_alive failed 1 run in 4, only there).

Serialise the upserts behind one asyncio.Lock per handler; tasks are created
in delivery order and the lock is FIFO, so the row ends with the last event.
The scanner's sweep still writes on its own path.

Verification: the new test fails against the previous watcher with
committed == [m2, m1]; passes with the lock. The six existing watcher tests
pass; lint-imports 4/4.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@gloryfromca

Copy link
Copy Markdown
Member Author

Windows evidence, same box (ThinkPad, Windows 11, soak load running throughout):

code test_atomic_replace_over_existing_target_keeps_the_row_alive note
integration branch without this fix (630de2d) 3 failed / 10 runs (1 in the full suite, 1/3, 1/6) row identical to first: stale duplicate committed last
integration branch with this fix (a6cb8d5) 0 failed / 7 runs (1 in the touched-area suite, 6/6 single runs)

The same race showed its other face on the GitHub Windows runner today, on #454 which does not carry this fix: test_unlink_enqueues_deleted — the deleted upsert landed and a late added overwrote it, so the row read added 15 s after the file was gone (job 107549179603). That is the variant that would wipe LanceDB rows for an existing file in the mirror case, and why order, not mtime, is the invariant here.

Throughput check on macOS (real SQLite, N created events back to back, time to all rows committed): 981–992 events/s with the lock vs 886–1103 without — same within noise. Side effect worth knowing: without the lock MAX(lsn)+1 handed duplicate sequence numbers (192 distinct of 500, 800 of 2000); with it every row has its own lsn, which is what ORDER BY lsn in the worker assumes.

@arelchan
arelchan self-requested a review September 24, 2026 08:33
@0xKT
0xKT merged commit 732e0f4 into main Sep 24, 2026
10 checks passed
@0xKT
0xKT deleted the fix/watcher-upsert-order branch September 24, 2026 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants