fix(plugins): keep the salt working where hardlinks are not supported
Self-review of the atomic-publish fix. Some network mounts and container volumes reject os.link, and the outer handler swallowed that into "no salt", which meant repo_hash and session_hash were dropped on every run for that whole cohort. The race being closed is narrow; losing the hashes for an entire filesystem is not a fair trade. Falls back to claiming the name with O_CREAT|O_EXCL and writing, which is what this did before. The empty-file window reopens there, but it is benign now: a reader landing in it gets "" and omits the hash for that process rather than caching a guessable path digest, which was the actual defect. 266 passed, 8 skipped. Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
This commit is contained in:
@@ -147,6 +147,19 @@ def _install_salt() -> str:
|
||||
os.link(temporary, path)
|
||||
except FileExistsError:
|
||||
pass
|
||||
except OSError:
|
||||
# No hardlinks here (some network mounts, some container volumes).
|
||||
# Claim the name directly instead. That reopens the empty-file
|
||||
# window, but the window is now benign: a reader that lands in it
|
||||
# gets "" and omits the hash for that process rather than caching a
|
||||
# guessable one. Losing the hashes on every run of an entire
|
||||
# filesystem is the worse failure.
|
||||
try:
|
||||
fallback = os.open(path, os.O_CREAT | os.O_EXCL | os.O_WRONLY, 0o600)
|
||||
with os.fdopen(fallback, "w", encoding="utf-8") as stream:
|
||||
stream.write(temporary.read_text(encoding="utf-8"))
|
||||
except OSError:
|
||||
pass
|
||||
except OSError:
|
||||
pass
|
||||
finally:
|
||||
|
||||
@@ -352,6 +352,26 @@ def test_a_half_written_salt_is_never_visible_to_another_process(isolated_env, m
|
||||
assert salt_path.read_text(encoding="utf-8").strip() == salt
|
||||
|
||||
|
||||
def test_a_filesystem_without_hardlinks_still_gets_a_salt(isolated_env, monkeypatch):
|
||||
"""Publishing by link must not become a silent loss of the hashes.
|
||||
|
||||
Some network mounts and container volumes reject os.link. Returning ""
|
||||
there would drop repo_hash and session_hash on every run for that whole
|
||||
cohort, which is a bigger loss than the narrow race the link closes.
|
||||
"""
|
||||
telemetry._salt_cache = ""
|
||||
monkeypatch.setattr(
|
||||
telemetry.os, "link", lambda src, dst: (_ for _ in ()).throw(OSError(38, "not implemented"))
|
||||
)
|
||||
|
||||
salt = telemetry._install_salt()
|
||||
|
||||
assert len(salt) == 32, "no salt on a filesystem without hardlinks"
|
||||
assert telemetry._salt_path().read_text(encoding="utf-8").strip() == salt
|
||||
assert telemetry._scoped_digest("git@github.com:acme/x.git") != ""
|
||||
assert not list(telemetry._salt_path().parent.glob("telemetry-salt.*.tmp"))
|
||||
|
||||
|
||||
def test_a_concurrent_writer_does_not_clobber_the_published_salt(isolated_env):
|
||||
"""Second process to finish must adopt the first one's salt, not replace it.
|
||||
|
||||
|
||||
Reference in New Issue
Block a user