Skip to content

hack: repo-publish: refuse to commit over a racing publish - #77

Open
martinpitt wants to merge 3 commits into
amutable-systems:mainfrom
martinpitt:publish-merge-race
Open

martinpitt wants to merge 3 commits into
amutable-systems:mainfrom
martinpitt:publish-merge-race

Conversation

@martinpitt

Copy link
Copy Markdown
Member

With --merge, repo-publish.sh reads the current targets from timestamp.json, merges them into the new targets, and only then runs repoctl snapshot. That starts the transaction and takes the timestamp ETag that TxnCommit compares against.

A publish which commits between the merge and the snapshot is not detected: the snapshot commits successfully (based on the newer timestamp), but with targets merged from the older one, and the other publisher's targets are gone. --no-merge has the same problem whenever the given targets were derived from the repository's current state.


I noticed that while evaluating what our infrastructure needs to do for supporting multiple architecture builds. That discovered this race condition. It will not actually affect our infra (at least not with the current plan), but this still seems worth addressing for other users.

`TxnCommit` only detects a racing writer that committed after `TxnStart`.
Callers which compute new role data from an earlier read of the
repository, such as merging the previous targets, have no way to tell
that the repository moved on in between, and silently drop the other
writer's changes.

Add a `TxnOp` which fails with `ErrClobberedTransaction` unless the
transaction starts from a given timestamp version. Timestamp versions
identify a repository state, as `TxnCommit` never overwrites a versioned
timestamp file.

Signed-off-by: Martin Pitt <martin@amutable.com>
Expose `RequireTimestampVersion` so that scripts which compute targets
before calling `repoctl snapshot` can refuse to commit over a repository
state they did not compute them from.

Signed-off-by: Martin Pitt <martin@amutable.com>
With `--merge`, repo-publish.sh reads the current targets from
`timestamp.json`, merges them into the new targets, and only then runs
`repoctl snapshot`. That starts the transaction and takes the timestamp
ETag that `TxnCommit` compares against.

A publish which commits between the merge and the snapshot is not
detected: the snapshot commits successfully (based on the newer
timestamp), but with targets merged from the older one, and the other
publisher's targets are gone. `--no-merge` has the same problem whenever
the given targets were derived from the repository's current state.

Read `timestamp.json` once, merge from that copy, and pass its version
to `repoctl snapshot --if-timestamp-version`, so that the snapshot fails
instead.

Signed-off-by: Martin Pitt <martin@amutable.com>
@cyphar

cyphar commented Oct 6, 2026

Copy link
Copy Markdown
Member

I'll take a proper look at this once I have some time later but tufrepo.Tx explicitly supports avoiding clobbering (modelled after the S3-friendly If-Matches mechanism) so I'm a little confused this would happen at all.

@cyphar

cyphar commented Oct 6, 2026

Copy link
Copy Markdown
Member

Ah, it's because of repo-publish.sh -- the goal is to not use these scripts for much longer. If the publishing was implemented using tufrepo this problem wouldn't exist AFAICS.

@martinpitt

Copy link
Copy Markdown
Member Author

@cyphar ah, I see! Do I get this right? tufrepo is an internal module, driven by hardhat repoctl. But it currently only supports a local filesystem repo, and the S3 backend does not exist.

Our infra still uses repo-publish.sh, but if you declare that obsolete, then please feel free to just close this PR (I don't have any emotional attachment to it nor block on it, I just didn't want to ignore it after learning about it).

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