fix(scanner): move updated_at when the upsert takes its conflict branch - #54
Merged
Conversation
`Track.updated_at` carries `onupdate=func.now()`, and SQLAlchemy applies that to ORM flushes and Core `update()` — but not to an `on_conflict_do_update` `set_` clause, which is emitted verbatim. So `_create_track`'s conflict branch rewrote 22 columns and left the timestamp reading whenever the row was last touched by some other path. **Narrower than it first looked, and the tests are the more useful half.** ADR-0011 asserts this makes the cursor unreliable for a rescan that changes a track's tags. It does not: the ordinary rescan goes through `_update_track`, which is plain ORM attribute assignment, so `onupdate` applies and the timestamp moves. Measured against a real database rather than read off the code — the first two attempts at that measurement both reported a false positive, because `expire_on_commit=False` means the in-memory attribute can be older than the row. The tests here read back on their own engine for that reason. The conflict branch is reached only when two scans race on the same `file_path`, which is what the comment above it always said it was for. Still worth fixing: the timestamp is the cursor an incremental client would page from, and a row whose tags changed but whose `updated_at` did not is invisible to a delta — invisible in the silent direction. Three tests pin the behaviour a delta would rest on, none of which existed: a retag moves it, an unchanged rescan does *not* (or every delta carries the whole library), and the conflict branch moves it. They guard a design that does not exist yet, which is exactly when this is cheap to break by "optimising" the rescan into a bulk statement. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014P9p2fvFnyiywBxGkv4gfW
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a real but much narrower gap than ADR-0011 claimed, and corrects the claim in #53.
What ADR-0011 said, and why it was wrong
The draft asserted that a rescan changing a track's tags leaves
updated_atuntouched, because thescanner's upsert omits it from an
on_conflict_do_updateset_clause that SQLAlchemy emitsverbatim. Both halves of that mechanism are true. The conclusion was not.
That upsert lives in
_create_track— the new-track path. Its conflict branch is reached onlywhen two scans race on the same
file_path, which is what the comment above it always said it wasfor. The ordinary rescan goes through
_update_track, which is plain ORM attribute assignment, soonupdate=func.now()applies and the timestamp moves.Caught only by measuring it. The first two attempts reported a false positive: with
expire_on_commit=False, the in-memory attribute can be older than the row it came from, so"before" was a stale cached value rather than what the database held. Reading the row back on a
separate connection showed the timestamp advancing exactly when the rescan ran, past a deliberate
two-second gap.
What this changes
One line —
updated_atadded to theset_clause — so the race path behaves like every other writeto that column. Worth doing because the timestamp is the cursor an incremental client would page
from, and a row whose tags changed but whose
updated_atdid not is invisible to a delta, in thesilent direction.
The tests are the more useful half
Three, none of which existed, pinning what a delta would rest on:
updated_atThey read back on their own engine, for the reason above. They guard a design that does not exist
yet, which is exactly when it is cheap to break by "optimising" the rescan into a bulk statement.
23 scanner tests pass, plus
test_api_tracksandtest_api_library(68 total). #53 is updated tomatch — two backend prerequisites now, not three.
🤖 Generated with Claude Code
https://claude.ai/code/session_014P9p2fvFnyiywBxGkv4gfW