Skip to content

Stores: update(annotation) — replace by id in one transaction (#66) - #67

Merged
thorwhalen merged 2 commits into
mainfrom
feat/store-atomic-update-66
Oct 13, 2026
Merged

thorwhalen merged 2 commits into
mainfrom
feat/store-atomic-update-66

Conversation

@thorwhalen

@thorwhalen thorwhalen commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Fixes #66.

store.update(annotation) replaces the annotation with that id atomically on every backend (added to the IntervalAnnotationStore protocol). Raises KeyError for an unknown id (add stays the way to create); the replacement may change interval or tier. Additive: nothing existing changes signature.

  • SqliteStore: one BEGIN IMMEDIATE transaction updates the row and its R*Tree entry. On a connection shared between threads a reader would see the open transaction's state, so annotation reads (all, by_tier, __getitem__, _candidates, id lookup) now take the store's lock.
  • MemoryStore: a same-interval replace is a single slot write; a move inserts the new copy before removing the old, so the id is never absent even to a lock-free reader. Writers and the snapshot readers share an RLock.
  • MappingStore: overwrites the one mapping key (never absent in the backing mapping), then the in-memory copy, under the lock.
  • PostgresStore: one UPDATE ... WHERE id AND owner/project statement (atomic in any isolation mode); TierOverlapError as in add.

Tests: tests/test_store_update.py (memory, mapping, sqlite): replace, move, unknown id, and the acceptance test, a reader thread against a replacing thread, same-interval and moving. Guard checked by mutation: swapping update for remove+add fails all six concurrency cases; removing the sqlite read lock fails both sqlite ones. Postgres tests are in tests/test_store_postgres.py (including a reader on a second connection). No Postgres server on the machine that built this, so those tests were not run here (they skip); the Postgres code follows the file's existing patterns.

Local: full suite with --doctest-modules: 902 passed, 5 skipped.

Adversarial review (fresh Opus subagent): no correctness defect in the update paths; Postgres checked by reading (placeholders, Range/Jsonb adaptation, KeyError via rowcount, ExclusionViolation). Two confirmed findings fixed in the second commit: (1) the reader thread could die and the test still pass, so it now records the reader's exceptions and asserts it ran; (2) the new RLock broke copy.deepcopy/pickle of MemoryStore/MappingStore, so __getstate__/__setstate__ rebuild the lock (test added). Also: MappingStore add/remove now take the lock, and a Postgres test for TierOverlapError from update. Left for follow-up (filed as an issue): MappingStore __setitem__/__delitem__ do not take the lock; iteration over the store while an update moves an annotation can still raise, as with add before.

🤖 Generated with Claude Code

thorwhalen and others added 2 commits October 13, 2026 16:04
remove(id) then add(...) is two commits, so a reader between them sees the id
missing. update exists on MemoryStore, MappingStore, SqliteStore and
PostgresStore; it raises KeyError for an unknown id.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
… MappingStore add/remove (#66)

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@thorwhalen
thorwhalen merged commit aa58553 into main Oct 13, 2026
12 checks passed
@thorwhalen
thorwhalen deleted the feat/store-atomic-update-66 branch October 13, 2026 16:21
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.

Store-level atomic update (replace by id in one transaction)

1 participant