CM-71568 - Write config files atomically and quarantine corrupt ones - #522
Merged
Conversation
update_yaml_file did a read-modify-write with open(filename, 'w'), which truncates in place. Two concurrent cycode processes both truncated to zero and each kept its own file offset, so whichever wrote fewer bytes left the longer writer's tail behind past its end. The boundary is a raw byte offset, so it landed mid-token and produced an unparseable credentials.yaml. The size gap is built in: refresh_access_token persists a ~900 char JWT while invalidate_access_token persists nulls. Parallel AI guardrails hook invocations put both on the same file at the same time. Recovery was impossible because update_yaml_file reads before it writes, so cycode auth raised the ScannerError before it could write the repair. The hook then respawned cycode auth, forever. Write to a sibling temp file and os.replace it into place, so a reader never observes a partial file and a short write cannot leave a long tail. On a YAMLError, move the file aside as <name>.corrupt-<n> and carry on with an empty config; this sits at the single choke point every command goes through, so cycode auth can now repair the machine on its own. Concurrent updates can still lose each other's keys, which is accepted: the losing writer's credentials are still valid ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Both new tests were coupled to the runner environment rather than to the behaviour under test, and failed across the tests_full matrix. fs.chmod cannot express a read-only filesystem on CI, which runs as root: root passes every permission check, so the write went through and the test read back 'updated' instead of 'original'. Patch os.access for the path under test instead, which is what the code actually branches on. The concurrency test spawned real writer processes, which meant writing a script to disk to keep multiprocessing spawn from re-importing __main__ (the pytest entry point) and to work on windows-latest. Dropped in favour of injecting a failure mid-write: the existing file has to survive untouched, which truncating it up front could never guarantee. Same class of bug, no subprocesses, and it still fails against the pre-fix code. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Quarantining to .corrupt-<n> meant a machine hitting this repeatedly would accumulate copies of its credentials file, none of which get cleaned up. Use a fixed .corrupt name instead: os.replace overwrites atomically, so the free-index search goes away and only the latest failure is kept. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
omer-roth
approved these changes
Aug 25, 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.
update_yaml_file did a read-modify-write with open(filename, 'w'), which truncates in place. Two concurrent cycode processes both truncated to zero and each kept its own file offset, so whichever wrote fewer bytes left the longer writer's tail behind past its end. The boundary is a raw byte offset, so it landed mid-token and produced an unparseable credentials.yaml.
The size gap is built in: refresh_access_token persists a ~900 char JWT while invalidate_access_token persists nulls. Parallel AI guardrails hook invocations put both on the same file at the same time.
Recovery was impossible because update_yaml_file reads before it writes, so cycode auth raised the ScannerError before it could write the repair. The hook then respawned cycode auth, forever.
Write to a sibling temp file and os.replace it into place, so a reader never observes a partial file and a short write cannot leave a long tail. On a YAMLError, move the file aside as .corrupt- and carry on with an empty config; this sits at the single choke point every command goes through, so cycode auth can now repair the machine on its own.
Concurrent updates can still lose each other's keys, which is accepted: the losing writer's credentials are still valid ones.