You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
CreateApplication decides whether an application is new by reading it back first:
existing, err:=s.store.GetByTaskID(req.TaskID)
Two concurrent injects of the same TaskID can both get gorm.ErrRecordNotFound, both generate a reference ID, and both call CreateOrUpdate. task_id is the primary key and Save upserts, so only one row survives — but two counter values are consumed and either ID can be the one that persists.
Tolerable today, and not introduced by the reference ID work — the CreateConsignment call in the same function already has the race. NSW doesn't send concurrent duplicate injects for one task, and refid counters are explicitly not gapless, so a wasted value is by design rather than a defect. Raised by review on #307 and deliberately deferred there.
Worth revisiting if inject volume grows or the service runs multiple replicas, since the window is a full read-then-write round trip.
Proposed Solution
Make first-injection handling atomic per TaskID. Two options:
A conditional insert plus retry — INSERT ... ON CONFLICT (task_id) DO NOTHING, then treat "no row inserted" as "already existed" and re-read instead of generating. Portable across SQLite and PostgreSQL, and needs no new locking primitive.
A PostgreSQL advisory lock keyed on the task ID around the read-then-write. Simpler to reason about, but Postgres-only, so the SQLite dev/test path needs its own answer.
Either way, add a concurrency regression test asserting one Generate call and one persisted reference ID for N parallel injects of the same task.
Generate inside CreateOrUpdate's transaction — narrows the window but doesn't close it, since the existence check would still happen outside, and it couples the store to reference ID generation.
Problem
CreateApplicationdecides whether an application is new by reading it back first:Two concurrent injects of the same
TaskIDcan both getgorm.ErrRecordNotFound, both generate a reference ID, and both callCreateOrUpdate.task_idis the primary key andSaveupserts, so only one row survives — but two counter values are consumed and either ID can be the one that persists.Tolerable today, and not introduced by the reference ID work — the
CreateConsignmentcall in the same function already has the race. NSW doesn't send concurrent duplicate injects for one task, andrefidcounters are explicitly not gapless, so a wasted value is by design rather than a defect. Raised by review on #307 and deliberately deferred there.Worth revisiting if inject volume grows or the service runs multiple replicas, since the window is a full read-then-write round trip.
Proposed Solution
Make first-injection handling atomic per
TaskID. Two options:INSERT ... ON CONFLICT (task_id) DO NOTHING, then treat "no row inserted" as "already existed" and re-read instead of generating. Portable across SQLite and PostgreSQL, and needs no new locking primitive.Either way, add a concurrency regression test asserting one
Generatecall and one persisted reference ID for N parallel injects of the same task.Alternatives
CreateOrUpdate's transaction — narrows the window but doesn't close it, since the existence check would still happen outside, and it couples the store to reference ID generation.