Skip to content

fix: never leave a sync upsert's own write in a tombstoned row - #759

Merged
helmerzNL merged 3 commits into
release/v26-betafrom
claude/arrow-criterion-distributors-hujk9y
Sep 3, 2026
Merged

fix: never leave a sync upsert's own write in a tombstoned row#759
helmerzNL merged 3 commits into
release/v26-betafrom
claude/arrow-criterion-distributors-hujk9y

Conversation

@helmerzNL

@helmerzNL helmerzNL commented Sep 3, 2026

Copy link
Copy Markdown
Owner

The symptom

A film is added, is recognised, appears — and after the next sync it is gone. On the
self-hosted PWA and the coupled iOS app at the same time, because the row is shared.
Nothing errors and nothing is logged: the server answers status: applied, so both
sides believe the add succeeded.

Reported against two Arrow releases (EAN 5027035025124 and 5027035027531).

The cause

apply_movie_upsert cleared deleted_at only when resurrect_tombstone was set, and
that happens on exactly one route: the dedup ladder misses,
find_tombstoned_movie_by_identity finds a tombstone, and the client's edit post-dates
the deletion. Three other routes reach the same write with entity_id already pointing
at a tombstoned row:

  • a clientEntityId mapping stored by an earlier delete-wins response,
  • the barcode-owner lookup in resolve_new_movie_identity,
  • a re-push replaying either of those.

On all three the payload was written into the dead row and deleted_at stayed set — a
successful write into an invisible row. The mapping route is the worst of the three:
once that mapping is stored it skips the ladder, so the record can never come back on
that client no matter how often it is re-added.

The fix

Delete-wins already returns tombstoned_movie_upsert_result before the write, so
reaching the update means the server decided to apply it. Clearing deleted_at there
is therefore unconditional, guarded by AND deleted_at IS NOT NULL so an ordinary
live-row update stays a no-op. That makes "an applied write in a tombstoned row"
structurally impossible instead of caught per route. A resurrection outside the ladder
route is logged rather than silent.

The tombstone lookup also gains the two guards trede 2 already has, and which its
digits-only barcode match had neither of:

  • a media-type veto, so a deleted show cannot answer for an incoming film and
    report it deleted when it never was;
  • a minimum digit length, because synthetic import placeholders
    (IMPORT-<title>-BOX-01) collapse to a digits-only key of "01", and one tombstone
    would otherwise match every unrelated box-set member ever imported. The code already
    warned about this collision a few hundred lines further down; the guard was missing
    here.

The veto is a veto, not a filter: a conflicting candidate blocks only itself, so a
legitimate match behind it is still found.

The CI registration, and why it is in this PR

postgres-smoke lists its test modules by hand, so a new file is invisible to it
until it is named. The workflow says so itself a few steps above the change:

Listed by hand like every module above it. So a silent drop here restores the gap
without anything failing to say so.

This PR demonstrated exactly that. Its first push carried the test file alone —
next_app.py was dropped from the index by a stash round-trip between staging and
committing — and postgres-smoke went green, although the resurrection test fails
against the unfixed code it was pushed with. The check was green because the module was
never collected, not because anything passed.

So the last commit names the module in the job. Verified with the job's own invocation
(python -m unittest tests.test_next_sync_tombstone_resurrection_postgres from
app/backend, DATABASE_URL set), which runs all seven tests rather than skipping them.

What is deliberately not changed

  • Delete-wins on an explicit entityId. A push naming a tombstoned row is an
    update intent and still loses to the deletion. NextTombstoneResponseContractTests
    pins it and still passes.
  • A missing updated_at. The PWA never sends it, so client_ts is None and the
    ladder route can never resurrect. That is a second defect with a real trade-off
    against the H4 replay guard, and it needs the production diagnosis before choosing a
    behaviour. Not folded in here.
  • delete_movie_records. It hard-deletes (DELETE FROM movies), and
    delete_container_records(delete_members=True) cascades to it, so rows can vanish
    leaving no tombstone at all. Separate finding, separate trade-off about what the
    delta feed should report.

Verification

  • New PostgreSQL-backed tests (test_next_sync_tombstone_resurrection_postgres.py).
    DB-backed because the question is what the row looks like afterwards, which a fake
    cursor cannot answer — the SQL issued looked correct throughout this bug.
  • The resurrection test fails against the unfixed code and passes with it, checked
    in both directions.
  • Full backend suite: 3517 tests, no regressions. The only 4 errors are
    test_next_export PDF/XLSX cases failing on reportlab/openpyxl missing from the
    local environment, unrelated to this change.
  • check_undefined_names.py, the forbidden-path guard, and the version guard against
    origin/release/v26-beta all pass.

app/VERSION is untouched — CI bumps it on beta after the merge. .github/workflows/
is a protected path, but protected paths decide whether a bump is due, not who
applies it.

The recording cursor in test_next_sync.py gains rowcount, which a real cursor
always carries.

Deployment-file changes

None. No Compose file, no .env.example, and no size, rate or timeout limit is
changed. Nothing for an operator to do.

Translations

No user-facing strings added or changed; the only new message is a server-side log
line. No i18n work required.


📦 Change Spec — release notes (fill at merge)

version: ""
name: "Vanishing Discs"
headline: "Discs you add stay added"
highlights:
  - "A disc you add now stays in your collection after a sync, on every device."
fixes:
  - "Fixed a disc that was added and recognised disappearing again after the next sync, on both the web app and the iOS app."
  - "Fixed re-adding such a disc having no effect, however many times it was tried."
  - "Fixed a deleted series being able to make a newly added film with the same barcode disappear."
breaking: []
cta: ""
internal_only:
  - "postgres-smoke now runs the tombstone resurrection tests; the job names its modules by hand and a new file is otherwise never collected."
  • Change Spec filled in, or this PR is internal-only (no user-facing release notes).

🤖 Generated with Claude Code

https://claude.ai/code/session_012BzJmAfWwxpHBdrciP6sG2

A movie added on one device could reappear as added, survive the response,
and be gone again after the next sync -- on the server, the PWA and the
coupled iOS app at once, with nothing reporting it.

`apply_movie_upsert` cleared `deleted_at` only when `resurrect_tombstone`
was set, and that happens on exactly one route: the dedup ladder misses,
`find_tombstoned_movie_by_identity` finds a tombstone, and the client's
edit post-dates the deletion. Three other routes reach the same write with
`entity_id` already pointing at a tombstoned row:

- a `clientEntityId` mapping stored by an earlier delete-wins response,
- the barcode-owner lookup in `resolve_new_movie_identity`,
- a re-push replaying either of those.

On all three the payload was written into the dead row, the server
answered `status: applied`, and the row kept its `deleted_at`. The mapping
route is the worst: once stored it skips the ladder, so the record could
never come back on that client no matter how often it was re-added.

Delete-wins already returns `tombstoned_movie_upsert_result` before the
write, so reaching the update means the server decided to apply it.
Clearing `deleted_at` there is therefore unconditional, guarded by
`AND deleted_at IS NOT NULL` so an ordinary live-row update stays a no-op.
A resurrection outside the ladder route is logged rather than silent.

Also gives the tombstone lookup the two guards trede 2 already has, and
which its digits-only barcode match had neither of:

- a media-type veto, so a deleted show cannot answer for an incoming film
  and report it deleted when it never was;
- a minimum digit length, because synthetic import placeholders
  (`IMPORT-<title>-BOX-01`) collapse to a key of "01" and one tombstone
  would otherwise match every unrelated box-set member ever imported.

The veto is a veto, not a filter: a conflicting candidate blocks only
itself, so a legitimate match behind it is still found.

Tests are PostgreSQL-backed because the question is what the row looks
like afterwards, which a fake cursor cannot answer. The resurrection test
fails against the unfixed code. Delete-wins on an explicit `entityId` is
deliberate and unchanged; `NextTombstoneResponseContractTests` still pins
it. The recording cursor in test_next_sync.py gains `rowcount`, which a
real cursor always carries.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012BzJmAfWwxpHBdrciP6sG2
The production half of the previous commit, which landed the tests alone:
a stash round-trip between staging and committing dropped these two files
from the index, and the commit went out unverified.

`apply_movie_upsert` cleared `deleted_at` only when `resurrect_tombstone`
was set -- one route out of four that can reach the write with `entity_id`
already on a tombstoned row. On the other three the payload was written
into the dead row, the server answered `status: applied`, and the record
vanished again on the next delta with nothing reporting it.

Delete-wins returns before the write, so reaching it means the server
decided to apply. The clear is therefore unconditional, guarded by
`AND deleted_at IS NOT NULL` so an ordinary live-row update stays a no-op.

Also gives `find_tombstoned_movie_by_identity` the two guards trede 2
already has: a media-type veto, and a minimum digit length so synthetic
import placeholders collapsing to "01" cannot match every unrelated
box-set member.

`test_next_sync.py`'s recording cursor gains `rowcount`, which a real
cursor always carries.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012BzJmAfWwxpHBdrciP6sG2
The job lists its test modules by hand, so a new file is invisible to it
until it is named. That is not a footgun this change discovered -- the
workflow says so a few steps up: "Listed by hand like every module above
it. So a silent drop here restores the gap without anything failing to
say so."

This PR demonstrated it. The first push carried the test file alone and
postgres-smoke went green, although the resurrection test fails against
the unfixed code it was pushed with. Green was not evidence: the module
was never collected.

Verified with the job's own invocation -- `python -m unittest
tests.test_next_sync_tombstone_resurrection_postgres` from `app/backend`
with DATABASE_URL set -- which runs all seven rather than skipping them.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012BzJmAfWwxpHBdrciP6sG2
@helmerzNL
helmerzNL merged commit 6dce2db into release/v26-beta Sep 3, 2026
2 checks passed
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