Skip to content

kernel: concurrent batch membership edits are silently lost — the #302 clobber, one collection over #327

Description

@JArmandoAnaya

The defect

_batch_sync_children is the exact shape #302 fixed for progress, one collection over.

Repository.update is session.merge(to_row(entity)) plus a child sync, and that sync
was delete every membership row of this batch, then re-insert the caller's list. A
Batch carries every member, so two callers adding different assets to one draft wrote
the same row set:

A: read [a, b]  ->  write [a, b, x]
B: read [a, b]  ->  write [a, b, y]     # deletes x, re-inserts a and b
final: [a, b, y]

Neither call was refused. BatchService.add_assets and remove_assets both returned a
Batch, and A's return value described a membership that no longer existed a moment later.
Same for two removals, and — the case with no race between two membership writers at all —
for any batch update landing beside a membership edit: approve reads a whole batch, sets
state, and saves it, putting membership back as it stood before whatever landed while it
was deciding.

SQLite's single writer does not prevent it, for #302's reason: serializing writes is not
serializing read-modify-write, and pysqlite defers BEGIN to the first write, so neither
read was inside a transaction.

Why it has not bitten yet, and why that is about to stop

add_assets / remove_assets have no route. The only production caller is
IngestService._materialize, and one ingest run holds the workspace alone. #281 puts
POST and DELETE /batches/{id}/assets in front of both — and the gallery's bulk bar
behind that — so the mitigation is the missing surface, and #281 removes it.

The fix

#302's recipe, applied to the row shape that already exists: batch_asset is keyed
(batch_id, asset_id), which is what a disjoint write wants.

  • The port gains its narrow non-repository writes beside set_asset_progress:
    add_batch_assets and remove_batch_assets.
  • No version column. Row existence is the contended datum — it is its own version
    stamp, and a column would be a second name for the same fact. (This is where it differs
    from set_asset_progress, whose expected guard exists because progress is a value that
    moves between states.)
  • No-op both ways: adding a member the batch holds and removing one it does not are 200,
    not errors. A loser whose target state already holds has nothing left to do.
  • Insert-if-absent is not enough, and that is the trap worth recording: a stale writer
    would then resurrect a member another had just removed. Both directions come from the
    same place — a whole-collection write derived from an out-of-date copy — so the
    whole-collection write is removed, not narrowed. Membership is written once at creation
    and afterwards only by the two narrow writes.

Cost, taken deliberately: Batch.asset_ids stops being writable through Repository.update,
so a stored membership can no longer be reordered by handing back a permuted list. Nothing
offers that, and its only implementation was the defect.

Fixed in #281's PR as Part 0, with a deterministic three-case concurrency test that is red
against the pre-fix code and, for the removal case, against the insert-if-absent half-fix too.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions