fix: atomic writes for cache and config files#1243
Conversation
HelifeWasTaken
force-pushed
the
mdalleau/atomic-cache-writes
branch
2 times, most recently
from
544095a to
965bfa6
Compare
mattisdalleau-gg
requested review from
6d7a,
agateau-gg,
benjaminrigaud-gg,
clement-tourriere,
mattisdalleau-gg and
timothedelion
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1243 +/- ##
==========================================
- Coverage 93.56% 93.51% -0.05%
==========================================
Files 181 181
Lines 9433 9474 +41
==========================================
+ Hits 8826 8860 +34
- Misses 607 614 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Cache and config writes (ai_discovery.json, update_check.yaml,
auth_config.yaml, the secret-scan cache, the docker-image ID cache
and the plugin manifest) previously did one of:
- truncate-in-place writes (open("w") / O_TRUNC), or
- tmp + rename with a fixed ".tmp" filename shared by every process
Both patterns are unsafe under concurrent ggshield invocations: the
first leaves concurrent readers seeing a zero-length / partial file,
the second has two processes racing on the same tempfile. This shows
up in practice when several `ggshield secret scan ai-hook` invocations
run in parallel from AI-agent PreToolUse hooks - cache contents get
truncated or partially written, and the failures are silently swallowed
by surrounding `except OSError: pass` / `except Exception` blocks.
Introduce `atomic_write_text` / `atomic_write_bytes` in
`ggshield/utils/files.py` that use `tempfile.mkstemp` (per-process
unique name in the destination directory) followed by `os.replace`,
and switch every affected call site over.
Co-Authored-By: Mattis Dalleau <mattisdalleau@gmail.com>
HelifeWasTaken
force-pushed
the
mdalleau/atomic-cache-writes
branch
from
965bfa6 to
d28520e
Compare
There was a problem hiding this comment.
I'm curious about the context: is this an error you or someone actually encountered? Under which OS?
The PR description looks AI generated and based on a code analysis, but mentions that the phenomenon has been "observed in practice".
The main idea seems sound, and I see how concurrent writes could corrupt a file like ai_discovery.json, so this kind of change is likely welcomed. Still, I'm very surprised this would ever trigger a SIGBUS, which sounds more like a "Python" problem than a "ggshield" problem. I would have expected a simple deserialization error (still undesirable, but which would have been handled gracefully in the code).
Moreover, the most important test, which is supposed to exhibit that the new code prevents the corruption, passes consistently with a naive implementation, so there is no guarantee that we actually fixed an issue here.
| def atomic_write_bytes(path: Path, data: bytes, *, mode: int = 0o644) -> None: | ||
| """Atomically write *data* to *path*. See :func:`atomic_write_text`.""" | ||
| path = Path(path) | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| fd, new_path = _open_new_sibling(path, mode) | ||
| try: | ||
| # See atomic_write_text: path-based chmod for pyfakefs / Windows compat. | ||
| os.chmod(new_path, mode) | ||
| with os.fdopen(fd, "wb") as f: | ||
| f.write(data) | ||
| _replace_with_retry(new_path, path) | ||
| except BaseException: | ||
| try: | ||
| os.unlink(new_path) | ||
| except OSError: | ||
| pass | ||
| raise |
There was a problem hiding this comment.
This is, barring the fdopen flags, an exact duplication of atomic_write_text. We should probably have a single implementation. If we want to keep the two *_text and *_bytes methods (which is fine), have them be thin wrappers around a common implementation.
There was a problem hiding this comment.
Hi the actual problem was verified:
❯ ggshield auth login
[1] 368219 bus error (core dumped) ggshield auth login
SIGBUS is sound due to data corruption opening the file with O_TRUNC and re-reading it conccurently (around 5~6 agents using the ggshield secret scan ai-hook) which is probably a timeframe of <1-2ms but very existant actually
Once the SIGBUS occur and the agents are actively calling it becomes persistent as mmap (probably?) cannot sanitize the inode of the file fast enough when truncating
|
|
||
| assert target.read_bytes() == payload | ||
|
|
||
| def test_concurrent_writers_produce_valid_file(self, tmp_path: Path) -> None: |
There was a problem hiding this comment.
This test does not justify the change, as this naive (and non-atomic) implementation of atomic_write_text() also passes it:
def atomic_write_text(path, data, mode, encoding):
path.parent.mkdir(parents=True, exist_ok=True)
path.write_text(data, encoding=encoding)
path.chmod(mode)To be honest, I tried to make this test exhibit the error with the naive implementation but couldn't, even with larger and more convoluted payloads.
|
Two additional comments:
Maybe we want to have utils for both needs, but the "cache stampede" avoidance is definitely useful: we will soon need to debounce hook calls as some agents call multiple times the same hook with the same payload if it is configured in multiple places. |
| fd = os.open( | ||
| str(self.cache_path), os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600 | ||
| text = json.dumps(self.to_dict()) | ||
| except Exception as e: |
There was a problem hiding this comment.
the except should be after the next try
3 participants
Summary
Several ggshield cache/config writes were unsafe under concurrent invocations:
ai_discovery.json(ggshield/verticals/ai/cache.py),auth_config.yaml/update_check.yaml(ggshield/core/config/utils.py), the secret-scan cache (ggshield/core/cache.py) and the AI fallback user-id (ggshield/verticals/ai/user.py) used truncate-in-place writes (open("w")/O_TRUNC). A second process reading mid-write would see a zero-length or partial file.ggshield/core/scan/id_cache.py) and the plugin manifest (ggshield/core/plugin/downloader.py) already used a tmp+rename pattern, but with a fixed.tmpfilename, so two concurrent writers would race on the same tempfile.Both classes of bug surface when several
ggshield secret scan ai-hookinvocations run in parallel from AI-agentPreToolUsehooks. Failures are silently swallowed by the surroundingexcept OSError: pass/except Exceptionblocks, so when nothing else goes wrong the visible symptom is just degraded caching — but as called out in the note above, a torn write on a file another process has mmap'd surfaces asSIGBUSand kills the reader outright.Changes
atomic_write_text/atomic_write_bytesinggshield/utils/files.py. They usetempfile.mkstemp(per-process unique name in the destination directory) followed byos.replace. POSIX guarantees the rename is atomic — readers see either the previous content or the new content, never a partial write.ggshield/verticals/ai/cache.py(save_discovery_cache)ggshield/verticals/ai/user.py(UUID fallback)ggshield/core/config/utils.py(save_yaml_dict, coversauth_config.yamlandupdate_check.yaml)ggshield/core/cache.py(Cache.save)ggshield/core/scan/id_cache.py(IDCache._save)ggshield/core/plugin/downloader.py(_write_manifest)tests/unit/utils/test_files.py, including aThreadPoolExecutor-based concurrent-writers test that asserts the final file is always one of the inputs (never a torn mix) and no temp files leak.tests/unit/core/plugin/test_downloader.py::test_tmp_file_is_cleaned_up_on_rename_failureto patchos.replace(now used by the helper) instead ofpathlib.Path.replace, and assert no leftover temp files in the directory.Test plan
pytest tests/unit— full unit suite (1919 passed)pytest tests/unit/utils/test_files.py— new atomic-write tests (21 passed, incl. concurrent-writers)pytest tests/unit/core/test_cache.py tests/unit/core/config tests/unit/core/scan tests/unit/core/plugin tests/unit/verticals/ai tests/unit/core/test_check_updates.py— modules with touched call sites