fix(core): stop atomic writes from colliding on a constant PID suffix#5721
Open
AmirF194 wants to merge 1 commit into
Open
fix(core): stop atomic writes from colliding on a constant PID suffix#5721AmirF194 wants to merge 1 commit into
AmirF194 wants to merge 1 commit into
Conversation
atomic_write_json/atomic_write_text build their temp filename as
"{path}.tmp.{os.getpid()}". os.getpid() is constant for the life of a
process, so it only ever distinguishes concurrent writers that live in
different OS processes. Odysseus runs as a single long-lived process
per container, so two concurrent writers to the same path (e.g. two
request handlers racing a settings save) always compute the identical
temp path. Whichever finishes os.replace() first removes the shared
tmp file out from under the other, which then raises FileNotFoundError
on its own os.replace() instead of landing its write.
Fix: derive the temp suffix from uuid4() instead of the PID, so every
call gets a distinct temp path regardless of process/thread identity.
routes/prefs_routes.py's _save() had an independent, hand-rolled copy
of the exact same PID-suffix logic (not the shared core.atomic_io
helper other routes already use, e.g. routes/auth_routes.py) with the
same bug. Replaced it with a call to atomic_write_json.
Fixes odysseus-dev#5596
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.
Summary
atomic_write_json/atomic_write_textincore/atomic_io.pybuild the temp filename as{path}.tmp.{os.getpid()}, meant to keep two concurrent writers from colliding on the same rename target.os.getpid()is constant for the life of a process, so it only distinguishes writers running in different processes. Odysseus runs as a single long-lived process per container, soos.getpid()never changes, and two concurrent writers to the same path (for example, two request handlers racing a settings save) always compute the identical temp path. Whichever finishesos.replace()first removes the shared temp file out from under the other, which then raisesFileNotFoundErroron its ownos.replace()instead of landing its write. This is issue #5596's exact symptom (intermittent 500s, reverted settings).The fix swaps the PID suffix for
uuid4(), which is unique per call regardless of process or thread identity.While tracing every writer of this pattern,
routes/prefs_routes.py's_save()turned out to be an independent, hand-rolled copy of the same temp-file logic (not the sharedcore.atomic_iohelper other routes already use, e.g.routes/auth_routes.py), with the identical PID-suffix bug. It's replaced with a call toatomic_write_json.Target branch
dev, notmain.Linked Issue
Fixes #5596
Type of Change
Checklist
devuvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.How to Test
python -m pytest tests/test_atomic_io.py tests/test_prefs_atomic_write.py -v, which includes a new regression test,test_atomic_write_json_concurrent_writers_do_not_collide, that spins up two threads writing the same path concurrently (a slowjson.dumpwidens the race window deterministically). It fails withFileNotFoundErroragainstdevHEAD and passes against this branch.python -m uvicorn app:app,AUTH_ENABLED=false) in a clean Docker container and exercisedPUT /api/prefs/{key}for a dozen keys, thenGET /api/prefs: every key landed, no errors, no leftover.tmp.*files indata/.python -m pytest -qgives the same 39 failed / 21 errors as unmodifieddevHEAD, all insrc/agent_loop.py-dependent tests, unrelated to this change (src/agent_loop.pyusesdict[str, Any]without importingAny, which also currently blocks the app from booting viauvicorn app:appondevHEAD; I patched that locally, outside this diff, only to run the end-to-end check in item 2). Everything undertests/test_atomic_io.py,tests/test_prefs_atomic_write.py,tests/test_preset_atomic_save.py, andtests/test_api_key_manager_atomic_save.pypasses.Not verified: production-scale concurrency (many simultaneous real users). The repro isolates the exact mechanism (shared, constant PID as the sole temp-file discriminator) rather than reproducing load-level frequency.