From e4fc9eb74b3cb60d7af86f0e52052267eedbc499 Mon Sep 17 00:00:00 2001 From: Ilan Lidovski Date: Tue, 25 Aug 2026 11:35:04 +0300 Subject: [PATCH 1/3] CM-71568 - Write config files atomically and quarantine corrupt ones 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. Co-Authored-By: Claude Opus 5 (1M context) --- cycode/cli/utils/yaml_utils.py | 41 ++++++++++-- tests/utils/test_yaml_utils.py | 116 +++++++++++++++++++++++++++++++++ 2 files changed, 152 insertions(+), 5 deletions(-) create mode 100644 tests/utils/test_yaml_utils.py diff --git a/cycode/cli/utils/yaml_utils.py b/cycode/cli/utils/yaml_utils.py index c92acdc8..ed964c26 100644 --- a/cycode/cli/utils/yaml_utils.py +++ b/cycode/cli/utils/yaml_utils.py @@ -1,4 +1,5 @@ import os +import tempfile from collections.abc import Hashable from typing import Any, TextIO @@ -34,22 +35,52 @@ def _yaml_object_safe_load(file: TextIO) -> dict[Hashable, Any]: return loaded_file +def _quarantine_corrupt_file(filename: str) -> None: + # Renamed rather than deleted: the file may hold the only copy of the user's credentials, + # and keeping it around leaves something to look at in the next bug report. + index = 0 + while os.path.exists(f'{filename}.corrupt-{index}'): + index += 1 + + try: + os.replace(filename, f'{filename}.corrupt-{index}') + except OSError as e: + logger.warning('Failed to quarantine corrupt file, %s', {'filename': filename}, exc_info=e) + + def read_yaml_file(filename: str) -> dict[Hashable, Any]: if not os.access(filename, os.R_OK) or not os.path.exists(filename): logger.debug('Config file is not accessible or does not exist: %s', {'filename': filename}) return {} - with open(filename, encoding='UTF-8') as file: - return _yaml_object_safe_load(file) + try: + with open(filename, encoding='UTF-8') as file: + return _yaml_object_safe_load(file) + except yaml.YAMLError as e: + logger.warning('Config file is corrupt and will be moved aside, %s', {'filename': filename}, exc_info=e) + _quarantine_corrupt_file(filename) + return {} def write_yaml_file(filename: str, content: dict[Hashable, Any]) -> None: - if not os.access(filename, os.W_OK) and os.path.exists(filename): + directory = os.path.dirname(filename) + if not os.access(directory, os.W_OK) or (os.path.exists(filename) and not os.access(filename, os.W_OK)): logger.warning('No write permission for file. Cannot save config, %s', {'filename': filename}) return - with open(filename, 'w', encoding='UTF-8') as file: - yaml.safe_dump(content, file) + # Atomic write to avoid race conditions between concurrent CLI processes + file_descriptor, temp_filename = tempfile.mkstemp(dir=directory, prefix=f'.{os.path.basename(filename)}.') + try: + with os.fdopen(file_descriptor, 'w', encoding='UTF-8') as file: + yaml.safe_dump(content, file) + file.flush() + os.fsync(file.fileno()) + + os.replace(temp_filename, filename) + except Exception: + if os.path.exists(temp_filename): + os.remove(temp_filename) + raise def update_yaml_file(filename: str, content: dict[Hashable, Any]) -> None: diff --git a/tests/utils/test_yaml_utils.py b/tests/utils/test_yaml_utils.py new file mode 100644 index 00000000..2fba27b1 --- /dev/null +++ b/tests/utils/test_yaml_utils.py @@ -0,0 +1,116 @@ +import multiprocessing +import os +from pathlib import Path + +import pytest +import yaml +from pyfakefs.fake_filesystem import FakeFilesystem + +from cycode.cli.utils.yaml_utils import read_yaml_file, update_yaml_file, write_yaml_file + +# refresh_access_token() persists a ~900 char JWT, invalidate_access_token() persists nulls. +# That size gap is what let a short write leave a longer write's tail behind it. +_LONG_TOKEN = 'eyJhbGciOiJIUzI1NiJ9.' + ('A' * 800) + '.signature' +_ITERATIONS = 200 + +_CLIENT_ID = 'b3a1f2c4-1111-2222-3333-444455556666' +_CORRUPT_CONTENT = 'cycode_client_id: valid\nycode_client_id\nleftover tail\n' + +_DIRECTORY = '/home/user/.cycode' +_FILENAME = f'{_DIRECTORY}/credentials.yaml' + + +def _write_long_token(filename: str) -> None: + for _ in range(_ITERATIONS): + update_yaml_file( + filename, + { + 'cycode_access_token': _LONG_TOKEN, + 'cycode_access_token_creator': 'h' * 64, + 'cycode_access_token_expires_in': 1755000000.123456, + }, + ) + + +def _write_null_token(filename: str) -> None: + for _ in range(_ITERATIONS): + update_yaml_file( + filename, + { + 'cycode_access_token': None, + 'cycode_access_token_creator': None, + 'cycode_access_token_expires_in': None, + }, + ) + + +# Real processes cannot see a pyfakefs filesystem, so this one case has to touch the disk. +def test_concurrent_updates_never_corrupt_the_file(tmp_path: Path) -> None: + filename = str(tmp_path / 'credentials.yaml') + write_yaml_file(filename, {'cycode_client_id': _CLIENT_ID, 'cycode_client_secret': 's' * 40}) + + context = multiprocessing.get_context('spawn') + processes = [context.Process(target=target, args=(filename,)) for target in (_write_long_token, _write_null_token)] + for process in processes: + process.start() + for process in processes: + process.join(timeout=120) + assert process.exitcode == 0, f'writer process failed with exit code {process.exitcode}' + + with open(filename, encoding='UTF-8') as file: + content = yaml.safe_load(file) + + assert isinstance(content, dict) + assert content['cycode_client_id'] == _CLIENT_ID + + +def test_reading_a_corrupt_file_quarantines_it_and_returns_empty(fs: FakeFilesystem) -> None: + fs.create_file(_FILENAME, contents=_CORRUPT_CONTENT) + + assert read_yaml_file(_FILENAME) == {} + assert not os.path.exists(_FILENAME) + + quarantined = list(Path(_DIRECTORY).glob('credentials.yaml.corrupt-*')) + assert len(quarantined) == 1 + assert quarantined[0].read_text(encoding='UTF-8') == _CORRUPT_CONTENT + + +def test_updating_a_corrupt_file_recovers_instead_of_raising(fs: FakeFilesystem) -> None: + fs.create_file(_FILENAME, contents=_CORRUPT_CONTENT) + + update_yaml_file(_FILENAME, {'cycode_client_id': 'recovered'}) + + assert read_yaml_file(_FILENAME) == {'cycode_client_id': 'recovered'} + + +def test_quarantine_does_not_overwrite_an_earlier_quarantined_file(fs: FakeFilesystem) -> None: + fs.create_dir(_DIRECTORY) + for marker in ('first', 'second'): + with open(_FILENAME, 'w', encoding='UTF-8') as file: + file.write(f'{marker}\nnot: [valid\n') + read_yaml_file(_FILENAME) + + quarantined = [path.read_text(encoding='UTF-8') for path in Path(_DIRECTORY).glob('credentials.yaml.corrupt-*')] + + assert len(quarantined) == 2 + assert any('first' in content for content in quarantined) + assert any('second' in content for content in quarantined) + + +@pytest.mark.parametrize('read_only_path', [_DIRECTORY, _FILENAME]) +def test_write_is_skipped_on_a_read_only_filesystem(fs: FakeFilesystem, read_only_path: str) -> None: + fs.create_dir(_DIRECTORY) + write_yaml_file(_FILENAME, {'cycode_client_id': 'original'}) + fs.chmod(read_only_path, 0o500) + + write_yaml_file(_FILENAME, {'cycode_client_id': 'updated'}) + + fs.chmod(read_only_path, 0o700) + assert read_yaml_file(_FILENAME) == {'cycode_client_id': 'original'} + + +def test_write_leaves_no_temporary_files_behind(fs: FakeFilesystem) -> None: + fs.create_dir(_DIRECTORY) + write_yaml_file(_FILENAME, {'cycode_client_id': _CLIENT_ID}) + + assert [path.name for path in Path(_DIRECTORY).iterdir()] == ['credentials.yaml'] From 904c3f113c32be76fe326b337e87bdbc43150f82 Mon Sep 17 00:00:00 2001 From: Ilan Lidovski Date: Tue, 25 Aug 2026 12:55:07 +0300 Subject: [PATCH 2/3] CM-71568 - Replace environment-coupled yaml_utils tests 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) --- tests/utils/test_yaml_utils.py | 82 ++++++++++++---------------------- 1 file changed, 29 insertions(+), 53 deletions(-) diff --git a/tests/utils/test_yaml_utils.py b/tests/utils/test_yaml_utils.py index 2fba27b1..02c9e47c 100644 --- a/tests/utils/test_yaml_utils.py +++ b/tests/utils/test_yaml_utils.py @@ -1,17 +1,14 @@ -import multiprocessing import os from pathlib import Path +from typing import TYPE_CHECKING import pytest -import yaml from pyfakefs.fake_filesystem import FakeFilesystem from cycode.cli.utils.yaml_utils import read_yaml_file, update_yaml_file, write_yaml_file -# refresh_access_token() persists a ~900 char JWT, invalidate_access_token() persists nulls. -# That size gap is what let a short write leave a longer write's tail behind it. -_LONG_TOKEN = 'eyJhbGciOiJIUzI1NiJ9.' + ('A' * 800) + '.signature' -_ITERATIONS = 200 +if TYPE_CHECKING: + from pytest_mock import MockerFixture _CLIENT_ID = 'b3a1f2c4-1111-2222-3333-444455556666' _CORRUPT_CONTENT = 'cycode_client_id: valid\nycode_client_id\nleftover tail\n' @@ -20,50 +17,6 @@ _FILENAME = f'{_DIRECTORY}/credentials.yaml' -def _write_long_token(filename: str) -> None: - for _ in range(_ITERATIONS): - update_yaml_file( - filename, - { - 'cycode_access_token': _LONG_TOKEN, - 'cycode_access_token_creator': 'h' * 64, - 'cycode_access_token_expires_in': 1755000000.123456, - }, - ) - - -def _write_null_token(filename: str) -> None: - for _ in range(_ITERATIONS): - update_yaml_file( - filename, - { - 'cycode_access_token': None, - 'cycode_access_token_creator': None, - 'cycode_access_token_expires_in': None, - }, - ) - - -# Real processes cannot see a pyfakefs filesystem, so this one case has to touch the disk. -def test_concurrent_updates_never_corrupt_the_file(tmp_path: Path) -> None: - filename = str(tmp_path / 'credentials.yaml') - write_yaml_file(filename, {'cycode_client_id': _CLIENT_ID, 'cycode_client_secret': 's' * 40}) - - context = multiprocessing.get_context('spawn') - processes = [context.Process(target=target, args=(filename,)) for target in (_write_long_token, _write_null_token)] - for process in processes: - process.start() - for process in processes: - process.join(timeout=120) - assert process.exitcode == 0, f'writer process failed with exit code {process.exitcode}' - - with open(filename, encoding='UTF-8') as file: - content = yaml.safe_load(file) - - assert isinstance(content, dict) - assert content['cycode_client_id'] == _CLIENT_ID - - def test_reading_a_corrupt_file_quarantines_it_and_returns_empty(fs: FakeFilesystem) -> None: fs.create_file(_FILENAME, contents=_CORRUPT_CONTENT) @@ -98,14 +51,22 @@ def test_quarantine_does_not_overwrite_an_earlier_quarantined_file(fs: FakeFiles @pytest.mark.parametrize('read_only_path', [_DIRECTORY, _FILENAME]) -def test_write_is_skipped_on_a_read_only_filesystem(fs: FakeFilesystem, read_only_path: str) -> None: +def test_write_is_skipped_on_a_read_only_filesystem( + fs: FakeFilesystem, mocker: 'MockerFixture', read_only_path: str +) -> None: fs.create_dir(_DIRECTORY) write_yaml_file(_FILENAME, {'cycode_client_id': 'original'}) - fs.chmod(read_only_path, 0o500) + + real_access = os.access + mocker.patch( + 'os.access', + side_effect=lambda path, mode: ( + False if mode == os.W_OK and str(path) == read_only_path else real_access(path, mode) + ), + ) write_yaml_file(_FILENAME, {'cycode_client_id': 'updated'}) - fs.chmod(read_only_path, 0o700) assert read_yaml_file(_FILENAME) == {'cycode_client_id': 'original'} @@ -114,3 +75,18 @@ def test_write_leaves_no_temporary_files_behind(fs: FakeFilesystem) -> None: write_yaml_file(_FILENAME, {'cycode_client_id': _CLIENT_ID}) assert [path.name for path in Path(_DIRECTORY).iterdir()] == ['credentials.yaml'] + + +# Stands in for a concurrent writer stopping partway through: the existing file must survive untouched, +# which is what truncating it up front could never guarantee. +def test_a_failed_write_leaves_the_existing_file_untouched(fs: FakeFilesystem, mocker: 'MockerFixture') -> None: + fs.create_dir(_DIRECTORY) + write_yaml_file(_FILENAME, {'cycode_client_id': 'original'}) + mocker.patch('yaml.safe_dump', side_effect=OSError('no space left on device')) + + with pytest.raises(OSError, match='no space left on device'): + write_yaml_file(_FILENAME, {'cycode_client_id': 'updated'}) + + mocker.stopall() + assert read_yaml_file(_FILENAME) == {'cycode_client_id': 'original'} + assert [path.name for path in Path(_DIRECTORY).iterdir()] == ['credentials.yaml'] From 4cbc072911a1b497ed32b086091ddc63bb2c91e7 Mon Sep 17 00:00:00 2001 From: Ilan Lidovski Date: Tue, 25 Aug 2026 13:28:53 +0300 Subject: [PATCH 3/3] CM-71568 - Keep only the most recent quarantined config file Quarantining to .corrupt- 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) --- cycode/cli/utils/yaml_utils.py | 6 +----- tests/utils/test_yaml_utils.py | 30 ++++++++++-------------------- 2 files changed, 11 insertions(+), 25 deletions(-) diff --git a/cycode/cli/utils/yaml_utils.py b/cycode/cli/utils/yaml_utils.py index ed964c26..dba4719e 100644 --- a/cycode/cli/utils/yaml_utils.py +++ b/cycode/cli/utils/yaml_utils.py @@ -38,12 +38,8 @@ def _yaml_object_safe_load(file: TextIO) -> dict[Hashable, Any]: def _quarantine_corrupt_file(filename: str) -> None: # Renamed rather than deleted: the file may hold the only copy of the user's credentials, # and keeping it around leaves something to look at in the next bug report. - index = 0 - while os.path.exists(f'{filename}.corrupt-{index}'): - index += 1 - try: - os.replace(filename, f'{filename}.corrupt-{index}') + os.replace(filename, f'{filename}.corrupt') except OSError as e: logger.warning('Failed to quarantine corrupt file, %s', {'filename': filename}, exc_info=e) diff --git a/tests/utils/test_yaml_utils.py b/tests/utils/test_yaml_utils.py index 02c9e47c..b6541c21 100644 --- a/tests/utils/test_yaml_utils.py +++ b/tests/utils/test_yaml_utils.py @@ -18,14 +18,18 @@ def test_reading_a_corrupt_file_quarantines_it_and_returns_empty(fs: FakeFilesystem) -> None: - fs.create_file(_FILENAME, contents=_CORRUPT_CONTENT) + fs.create_dir(_DIRECTORY) - assert read_yaml_file(_FILENAME) == {} - assert not os.path.exists(_FILENAME) + for marker in ('first', 'second'): + contents = f'{marker}\n{_CORRUPT_CONTENT}' + with open(_FILENAME, 'w', encoding='UTF-8') as file: + file.write(contents) - quarantined = list(Path(_DIRECTORY).glob('credentials.yaml.corrupt-*')) - assert len(quarantined) == 1 - assert quarantined[0].read_text(encoding='UTF-8') == _CORRUPT_CONTENT + assert read_yaml_file(_FILENAME) == {} + assert not os.path.exists(_FILENAME) + # only the most recent corrupt file is kept, so repeated failures cannot pile up + assert [path.name for path in Path(_DIRECTORY).iterdir()] == ['credentials.yaml.corrupt'] + assert Path(f'{_FILENAME}.corrupt').read_text(encoding='UTF-8') == contents def test_updating_a_corrupt_file_recovers_instead_of_raising(fs: FakeFilesystem) -> None: @@ -36,20 +40,6 @@ def test_updating_a_corrupt_file_recovers_instead_of_raising(fs: FakeFilesystem) assert read_yaml_file(_FILENAME) == {'cycode_client_id': 'recovered'} -def test_quarantine_does_not_overwrite_an_earlier_quarantined_file(fs: FakeFilesystem) -> None: - fs.create_dir(_DIRECTORY) - for marker in ('first', 'second'): - with open(_FILENAME, 'w', encoding='UTF-8') as file: - file.write(f'{marker}\nnot: [valid\n') - read_yaml_file(_FILENAME) - - quarantined = [path.read_text(encoding='UTF-8') for path in Path(_DIRECTORY).glob('credentials.yaml.corrupt-*')] - - assert len(quarantined) == 2 - assert any('first' in content for content in quarantined) - assert any('second' in content for content in quarantined) - - @pytest.mark.parametrize('read_only_path', [_DIRECTORY, _FILENAME]) def test_write_is_skipped_on_a_read_only_filesystem( fs: FakeFilesystem, mocker: 'MockerFixture', read_only_path: str