fix: the race fix's temp name assumed one bootstrap per process (4295)
CI and images / lint (push) Successful in 3s
CI and images / extension-version (push) Successful in 3s
CI and images / frontend-build (push) Successful in 22s
CI and images / backend-lint-and-test (push) Successful in 30s
CI and images / integration (push) Successful in 2m9s
CI and images / sign-extension (push) Successful in 3s
CI and images / build-agent (push) Successful in 6s
CI and images / build-web (push) Successful in 1m48s
CI and images / smoke-web (push) Successful in 55s
CI and images / promote (push) Skipped
CI and images / lint (push) Successful in 3s
CI and images / extension-version (push) Successful in 3s
CI and images / frontend-build (push) Successful in 22s
CI and images / backend-lint-and-test (push) Successful in 30s
CI and images / integration (push) Successful in 2m9s
CI and images / sign-extension (push) Successful in 3s
CI and images / build-agent (push) Successful in 6s
CI and images / build-web (push) Successful in 1m48s
CI and images / smoke-web (push) Successful in 55s
CI and images / promote (push) Skipped
Run 7370: six of the eight threads in the new test died with
FileNotFoundError(2, 'No such file or directory')
The first cut named the temp file `.credential_key.b64.<pid>.tmp`, which
assumes one bootstrap per PROCESS. Eight threads share one pid, so all eight
raced the same filename and six found it already unlinked by another.
The assumption held for hypercorn's workers, which are processes, and would
have held in production. It was still an assumption the code did not need to
make, and the test found it in one run — which is the test doing its job
rather than the test being wrong, so `tempfile.mkstemp` replaces the guess.
Verified against the SHIPPED file, loaded by path so the package `__init__`
(which wants quart) stays out of it: 25 consecutive attempts, eight threads
through a barrier each time, one key, no leftover temp files. My first check
of this reimplemented the logic in a scratch script, which measures a copy —
rule 10 names that exact failure, and it is not evidence about the code that
ships.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LVjrnpQjRgHdvq95rASoiR
This commit is contained in:
@@ -24,6 +24,7 @@ rows undecryptable (recovery = delete the rows and re-upload).
|
||||
|
||||
import logging
|
||||
import os
|
||||
import tempfile
|
||||
from pathlib import Path
|
||||
|
||||
from cryptography.fernet import Fernet, InvalidToken
|
||||
@@ -101,10 +102,21 @@ class CredentialCrypto:
|
||||
# `os.replace`, which would succeed — so two processes that both
|
||||
# generated a key would each think they had won, and the loser's key
|
||||
# would overwrite the one the winner had already handed to Fernet.
|
||||
# `mkstemp`, not a pid-derived name. The first cut spelled the temp
|
||||
# file `.credential_key.b64.<pid>.tmp`, which assumes one bootstrap per
|
||||
# process — and the test that exercises this with eight THREADS shares
|
||||
# one pid, so all eight raced the same filename and six died with
|
||||
# FileNotFoundError when another had already unlinked it. The
|
||||
# assumption held for hypercorn's workers and would have held in
|
||||
# production; it was still an assumption the code did not need to make.
|
||||
key = Fernet.generate_key()
|
||||
tmp = parent / f".{self._key_path.name}.{os.getpid()}.tmp"
|
||||
fd, tmp_name = tempfile.mkstemp(
|
||||
dir=parent, prefix=f".{self._key_path.name}.", suffix=".tmp",
|
||||
)
|
||||
tmp = Path(tmp_name)
|
||||
try:
|
||||
tmp.write_bytes(key)
|
||||
with os.fdopen(fd, "wb") as fh:
|
||||
fh.write(key)
|
||||
os.chmod(tmp, 0o600)
|
||||
try:
|
||||
os.link(tmp, self._key_path)
|
||||
|
||||
Reference in New Issue
Block a user