Fix/publish races by neolynx · Pull Request #1572 · aptly-dev/aptly

neolynx · 2026-05-23T11:56:35Z

Description of the Change

I spent some times and tokens to analyze the publish workflows with regards to race conditions and resource locking.

apiPublishRepoOrSnapshot appended published.Key() to resources inside the task closure, after maybeRunTaskInBackground had already been called. The task's locked-resource set is fixed at submission time, so that append had no effect — the published repo key was never registered as a resource. Two concurrent POST /api/publish/{prefix} requests for the same prefix/distribution therefore did not conflict in the task queue: both ran in parallel, each loaded an empty PublishedRepoCollection from the DB, both passed CheckDuplicate, and the second Add silently overwrote the first. Fix: compute the published repo key ("U{storagePrefix}>>{distribution}") from the already-known storage/prefix/distribution values and append it to resources before calling maybeRunTaskInBackground, so concurrent creates for the same destination are serialised by the task queue. The now-dead append inside the closure is removed.

…oints Affected endpoints: apiPublishAddSource, apiPublishSetSources, apiPublishUpdateSource, apiPublishRemoveSource, apiPublishDropChanges. All five handlers shared the same flawed pattern: they loaded the published repo from the DB and mutated it (ObtainRevision / DropRevision) outside the task closure, before the task lock was acquired. Each task closure then just wrote back the already-mutated, pre-lock object. Because the task queue serialises tasks that share a resource key, two concurrent requests appear safe — but each task closure holds a stale copy of the object captured before the lock was taken: Request A loads published: revision = {} Request B loads published: revision = {} <- same DB state A mutates: revision = {main: snap1} B mutates: revision = {contrib: snap2} Task A runs: saves {main: snap1} OK Task B runs: saves {contrib: snap2} <- clobbers A's change Fix: perform only a shallow ByStoragePrefixDistribution outside the task (for the early 404 response, resource key, and task name). Inside the task closure a dedicated taskCollectionFactory is created, the published repo is re-read fresh from the DB (after the lock is acquired), and LoadComplete + all mutations + Update are executed against that authoritative copy.

Affected endpoints: apiPublishUpdateSwitch (PUT), apiPublishUpdate (POST). Both handlers loaded the published repo and mutated scalar fields (Label, Origin, SkipContents, SkipBz2, AcquireByHash, SignedBy, MultiDist, Version) outside the task closure, before the lock was acquired. Inside the task, LoadComplete only refreshed sourceItems — it did not reload scalar fields or the Revision. Two concurrent requests therefore each operated on a stale base: Request A loads published (Label="old"), sets Label="A" Request B loads published (Label="old"), sets Label="B" Task A runs: Update() + Publish() + collection.Update() -> saves Label="A" Task B runs: Update() on B's stale copy -> saves Label="B", silently discarding A's Label change and potentially reconciling a Revision built against the pre-A state. Fix: remove all field mutations and the LoadComplete call from the HTTP handler. Inside the task, a fresh taskCollectionFactory is created, the published repo is re-read via ByStoragePrefixDistribution + LoadComplete (obtaining the current DB state after the lock is held), and then all field mutations are applied before Update / Publish / collection.Update.

codecov · 2026-05-23T12:20:38Z

Codecov Report

❌ Patch coverage is 72.00000% with 56 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.22%. Comparing base (8dc61cf) to head (68814ff).

Files with missing lines	Patch %	Lines
api/publish.go	72.00%	33 Missing and 23 partials ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1572   +/-   ##
=======================================
  Coverage   77.22%   77.22%           
=======================================
  Files         161      161           
  Lines       15085    15143   +58     
=======================================
+ Hits        11649    11694   +45     
- Misses       2292     2297    +5     
- Partials     1144     1152    +8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Affected endpoints: apiPublishRepoOrSnapshot (POST /api/publish/{prefix}), apiPublishDrop (DELETE /api/publish/{prefix}/{distribution}). Both handlers used the outer-scoped collectionFactory and collection variables inside the task closure. These were captured before the task lock was acquired, so under concurrent load each task operated on a stale DB view: apiPublishRepoOrSnapshot: snapshot/localRepo LoadComplete, NewPublishedRepo, CheckDuplicate, Publish, and collection.Add all used the pre-lock collectionFactory/collection. Two concurrent POST to same prefix could both pass CheckDuplicate (neither sees the other in the stale DB view) and race on disk writes. apiPublishDrop: collection.Remove used pre-lock collection, potentially racing with concurrent updates/other drops. Fix: inside the task closure create a fresh taskCollectionFactory and taskCollection. All DB reads (LoadComplete) and writes (CheckDuplicate, Add, Remove, Publish) now run against the authoritative DB state after the lock is held.

…be pre-registered When b.Distribution is empty, the pre-registered resource key U<storage>:<prefix>>><distribution> cannot be constructed, so concurrent POST requests to the same prefix are not serialized by the task queue. Add a log warning so operators are aware of the gap.

Affected endpoint: apiPublishUpdateSwitch (PUT /api/publish/{prefix}/{distribution}). The handler registered only the published repo key as a task resource. The underlying source repos (for local) or snapshots (for snapshot-based published repos) were not locked. Concurrent updates to a source repo or snapshot while a publish-update/switch task was running could produce inconsistent published indexes: Task A: apiPublishUpdateSwitch loads published, reads source repo/snapshot Request B: modifies same source repo or snapshot (add/remove packages, etc) Task A: Update() + Publish() reads stale/modified source -> inconsistent published index, or partial write if source deleted mid-task. Fix: for SourceLocalRepo, iterate published.Sources (component -> source UUID), look up each local repo via localRepoCollection.ByUUID and append string(repo.Key()) to resources. For SourceSnapshot, iterate b.Snapshots, look up each snapshot via snapshotCollection.ByName and append string(snapshot.ResourceKey()) to resources. Task queue now serialises against both the published repo and all its sources.

neolynx added 3 commits May 23, 2026 13:54