make the publish lock keep the guarantee it claims - #84
Merged
Conversation
Four findings from the same review, plus the raw-owner half of a fifth. The default fell back to `tempfile.gettempdir()` when /tmp was unusable, and that reads TMPDIR. The fixed path exists because TMPDIR split two writers of one output directory onto two lock files; falling back to it in a read-only-/tmp container reintroduced exactly that, in the one place nobody would look. It refuses now and names the flag that settles it. A directory the operator names was taken on trust. O_NOFOLLOW stops a symlink planted at the path; it does not stop another uid unlinking the pathname between two writers' opens and creating its own file there, after which each holds a different inode and flock excludes neither. On the default it is /tmp's sticky bit that prevents that, so a lock directory writable by other users has to carry one. A private directory is the ordinary case and is left alone. The lock file is named after the output path, which is container-local: one volume mounted at /data/out in one container and /mnt/out in another is two paths and so two locks, even with --lock-dir pointed at the volume. --lock-key lets whatever orchestrates the run name the destination instead. An empty key is refused, since it would collapse every output directory sharing a lock directory onto one file. And the exclusion had only ever been measured between two threads of one interpreter with the default directory monkeypatched, which cannot see a TMPDIR difference at all. There is a case now that spawns real processes with different TMPDIR values and a shared --lock-dir; it passed as written, so it documents a guarantee that held rather than one that did not. The ownership check also compares the raw mission id. The label is 64 bits of a digest and the raw id is already annotated beside it, so a collision could decide a delete on the digest alone. A caller holding no raw ids still reconciles by fingerprint, rather than matching nothing. Mutations: falling back to TMPDIR, trusting the lock directory, ignoring the key, accepting an empty one, dropping the raw-owner comparison, and making an absent owner set match nothing instead of everything. Each fails the test that names it. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Four lock findings from the review of #81, and the half of a fifth that #83 left open.
The default fell back to
tempfile.gettempdir(). That reads TMPDIR, which is the split namespace the fixed path was added to close. In a container with a read-only/tmp, two writers with different TMPDIR values took two lock files of the same name in two directories and both published — the original bug, reintroduced in the one place nobody would look for it.A custom
--lock-dirwas taken on trust.O_NOFOLLOWstops a symlink planted at the path. It does not stop another uid unlinking the pathname between two writers' opens and creating its own file there; each then holds a different inode andflockexcludes neither. On the default it is/tmp's sticky bit that prevents this.The lock is named after the output path, which is container-local. One volume mounted at
/data/outin one container and/mnt/outin another is two paths, so two locks, even with--lock-dirpointed at the volume.The exclusion had only been measured between threads. Two threads of one interpreter with
_default_lock_dirmonkeypatched cannot see a TMPDIR difference at all, while the claim is about separate processes.What
The default refuses instead of falling back, and says which flag settles it. A lock directory writable by other users has to carry the sticky bit; a private one is the ordinary case and needs nothing.
--lock-keynames the destination when the path cannot, and an empty key is refused because it would collapse every output directory sharing a lock directory onto one file.There is a process-level exclusion case now: real subprocesses, different TMPDIR values, one shared
--lock-dir, and the second gives up. It passed as written, so it documents a guarantee that already held rather than one that did not — which is the point of writing it down.The ownership check also compares the raw mission id. The label is 64 bits of a digest and the raw id is already annotated beside it, so a collision could otherwise decide a delete on the digest alone. A caller holding no raw ids still reconciles by fingerprint rather than matching nothing.
Verification
Written as failing tests first, then each guarantee checked against a mutation: falling back to TMPDIR, trusting the lock directory, ignoring the key, accepting an empty one, dropping the raw-owner comparison, and making an absent owner set match nothing instead of everything. Each fails the test that names it.
The sticky-bit case is skipped when running as root, where the mode bits do not stop the write and the shape under test does not exist — skipped rather than asserted, which would pass for the wrong reason in a root container.
1011 pytest passed, 1 skipped.
ruffclean,mypyclean, 15 goldens pass.Depends on #83
Fourth in the stack: #81, #82, #83, then this.