Skip to content

Fix rollback leaving stale tombstones that re-delete restored annotations#1751

Merged
BryonLewis merged 1 commit into
mainfrom
fix/rollback-clear-stale-tombstones
Jul 16, 2026
Merged

Fix rollback leaving stale tombstones that re-delete restored annotations#1751
BryonLewis merged 1 commit into
mainfrom
fix/rollback-clear-stale-tombstones

Conversation

@mattdawkins

Copy link
Copy Markdown
Member

Problem

Annotations are lazily deleted: a track removed at revision R keeps its document and is tombstoned with rev_deleted = R. Restoring it means clearing that tombstone.

rollback() built one query keyed on REVISION_CREATED and used it for both the removal and the un-delete:

listQuery = {DATASET: dsId, REVISION_CREATED: {'$gt': revision}}
updateQuery = {'$unset': {REVISION_DELETED: ""}}
TrackItem().removeWithQuery(listQuery)
TrackItem().update(listQuery, updateQuery)   # <-- wrong selector

As an un-delete selector that query is wrong: it only ever matches documents removeWithQuery just dropped, so tombstones are never cleared. The function's own comment states the intent it doesn't implement — "erase deletions for anything deleted after revision".

Why this is worse than it looks

The bug is invisible immediately after the rollback. A track tombstoned rev_deleted = R is still visible while the head sits at R-1, because BaseItem.list() treats rev_deleted > head as live. The rollback appears to work.

But save_annotations() computes new_revision = latest + 1, so the next save of any kind re-issues revision R. The stale tombstone reactivates and the restored track disappears — permanently, since every later head is >= R.

A user who rolls back a bad delete and then makes one unrelated edit silently loses the restored data.

Fix

Select records to restore by REVISION_DELETED; removal still keys off REVISION_CREATED.

Tests

server/tests/test_annotation_rollback.py covers both the query shape and the end-to-end property that a restored track survives a subsequent save. Note the latter matters: a test that asserts only immediately after the rollback passes even against the buggy code.

Both tests fail on main and pass with this change.

🤖 Generated with Claude Code

…ions

Annotations are lazily deleted: a track removed at revision R keeps its
document and is tombstoned with rev_deleted=R.  Restoring it therefore means
clearing that tombstone.

rollback() built a single query keyed on REVISION_CREATED and used it for both
the removal and the un-delete.  As an un-delete selector that query is wrong --
it only ever matches documents that removeWithQuery had just dropped -- so
tombstones were never cleared.

The failure is invisible at first: a track tombstoned rev_deleted=R is still
visible while the head sits at R-1, because reads treat rev_deleted > head as
live.  But save_annotations() computes new_revision as latest+1, so the next
save re-issues revision R, the stale tombstone becomes active, and the restored
track silently disappears -- permanently, since every later head is >= R.

Select the records to restore by REVISION_DELETED instead.  Removal still keys
off REVISION_CREATED.

Tests cover both the query shape and the end-to-end property that a restored
track survives a subsequent save.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@BryonLewis
BryonLewis self-requested a review July 16, 2026 12:47

@BryonLewis BryonLewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Correct fix for the stale-tombstone case, and the post-save assertion is the important one. Note this only affects POST dive_annotation/rollback (scripts/tests), not the readonly revision inspector. I.E there is no UI that hits this endpoint outside some integration tests.

@BryonLewis
BryonLewis merged commit 4ab89d7 into main Jul 16, 2026
3 checks passed
@BryonLewis
BryonLewis deleted the fix/rollback-clear-stale-tombstones branch July 16, 2026 12:52
mattdawkins added a commit that referenced this pull request Jul 16, 2026
Brings in 8 commits, including suppression regions (#1760), the
multicam camera order fix (#1759), signal-killed pipeline handling
(#1752, #1761), the annotation rollback tombstone fix (#1751), CSV note
columns (#1753), and the vitest/ws dependabot bumps.

Viewer.vue merged cleanly: f557b21 restored the SealTK wiring that
merge 8b0ab93 had dropped, so the file no longer conflicts with main.

Two conflicts, both from suppression regions landing on main while
viame/master already carried its own copy:

* LayerManager.vue -- keep viame/master's ../utils import. main's copy
  of this file diverged and no longer needs those symbols, but the
  merged body still calls geojsonToBound, isRotationValue,
  ROTATION_ATTRIBUTE_NAME and featureHasSegmentationPolygon.
* TypeSettingsPanel.vue -- take main's suppression row (adds mt-5).

package.json resolves to main's vitest 4.1.10 bump (viame/master never
changed it off the 3.2.4 base) while keeping onnxruntime-web.

Verified: client tests 611/611 passing on Node 22, eslint clean, desktop
app builds and runs with no console errors, and on a multicam stereo
dataset ImportAnnotations still completes setup with no Annotation Set
combobox and a live openUpload.

Note: vitest 4 requires Node ^20 || ^22 || >=24, so `npm test` now fails
to start on Node 18. client/README.md still says "Requires Node 18+".

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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