Skip to content

Commit 0fe1d03

Browse files
Ilanlidoclaude
andauthored
CM-71568 - Write config files atomically and quarantine corrupt ones (#522)
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f112931 commit 0fe1d03

2 files changed

Lines changed: 114 additions & 5 deletions

File tree

‎cycode/cli/utils/yaml_utils.py‎

Lines changed: 32 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import os
2+
import tempfile
23
from collections.abc import Hashable
34
from typing import Any, TextIO
45

@@ -34,22 +35,48 @@ def _yaml_object_safe_load(file: TextIO) -> dict[Hashable, Any]:
3435
return loaded_file
3536

3637

38+
def _quarantine_corrupt_file(filename: str) -> None:
39+
# Renamed rather than deleted: the file may hold the only copy of the user's credentials,
40+
# and keeping it around leaves something to look at in the next bug report.
41+
try:
42+
os.replace(filename, f'{filename}.corrupt')
43+
except OSError as e:
44+
logger.warning('Failed to quarantine corrupt file, %s', {'filename': filename}, exc_info=e)
45+
46+
3747
def read_yaml_file(filename: str) -> dict[Hashable, Any]:
3848
if not os.access(filename, os.R_OK) or not os.path.exists(filename):
3949
logger.debug('Config file is not accessible or does not exist: %s', {'filename': filename})
4050
return {}
4151

42-
with open(filename, encoding='UTF-8') as file:
43-
return _yaml_object_safe_load(file)
52+
try:
53+
with open(filename, encoding='UTF-8') as file:
54+
return _yaml_object_safe_load(file)
55+
except yaml.YAMLError as e:
56+
logger.warning('Config file is corrupt and will be moved aside, %s', {'filename': filename}, exc_info=e)
57+
_quarantine_corrupt_file(filename)
58+
return {}
4459

4560

4661
def write_yaml_file(filename: str, content: dict[Hashable, Any]) -> None:
47-
if not os.access(filename, os.W_OK) and os.path.exists(filename):
62+
directory = os.path.dirname(filename)
63+
if not os.access(directory, os.W_OK) or (os.path.exists(filename) and not os.access(filename, os.W_OK)):
4864
logger.warning('No write permission for file. Cannot save config, %s', {'filename': filename})
4965
return
5066

51-
with open(filename, 'w', encoding='UTF-8') as file:
52-
yaml.safe_dump(content, file)
67+
# Atomic write to avoid race conditions between concurrent CLI processes
68+
file_descriptor, temp_filename = tempfile.mkstemp(dir=directory, prefix=f'.{os.path.basename(filename)}.')
69+
try:
70+
with os.fdopen(file_descriptor, 'w', encoding='UTF-8') as file:
71+
yaml.safe_dump(content, file)
72+
file.flush()
73+
os.fsync(file.fileno())
74+
75+
os.replace(temp_filename, filename)
76+
except Exception:
77+
if os.path.exists(temp_filename):
78+
os.remove(temp_filename)
79+
raise
5380

5481

5582
def update_yaml_file(filename: str, content: dict[Hashable, Any]) -> None:

‎tests/utils/test_yaml_utils.py‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import os
2+
from pathlib import Path
3+
from typing import TYPE_CHECKING
4+
5+
import pytest
6+
from pyfakefs.fake_filesystem import FakeFilesystem
7+
8+
from cycode.cli.utils.yaml_utils import read_yaml_file, update_yaml_file, write_yaml_file
9+
10+
if TYPE_CHECKING:
11+
from pytest_mock import MockerFixture
12+
13+
_CLIENT_ID = 'b3a1f2c4-1111-2222-3333-444455556666'
14+
_CORRUPT_CONTENT = 'cycode_client_id: valid\nycode_client_id\nleftover tail\n'
15+
16+
_DIRECTORY = '/home/user/.cycode'
17+
_FILENAME = f'{_DIRECTORY}/credentials.yaml'
18+
19+
20+
def test_reading_a_corrupt_file_quarantines_it_and_returns_empty(fs: FakeFilesystem) -> None:
21+
fs.create_dir(_DIRECTORY)
22+
23+
for marker in ('first', 'second'):
24+
contents = f'{marker}\n{_CORRUPT_CONTENT}'
25+
with open(_FILENAME, 'w', encoding='UTF-8') as file:
26+
file.write(contents)
27+
28+
assert read_yaml_file(_FILENAME) == {}
29+
assert not os.path.exists(_FILENAME)
30+
# only the most recent corrupt file is kept, so repeated failures cannot pile up
31+
assert [path.name for path in Path(_DIRECTORY).iterdir()] == ['credentials.yaml.corrupt']
32+
assert Path(f'{_FILENAME}.corrupt').read_text(encoding='UTF-8') == contents
33+
34+
35+
def test_updating_a_corrupt_file_recovers_instead_of_raising(fs: FakeFilesystem) -> None:
36+
fs.create_file(_FILENAME, contents=_CORRUPT_CONTENT)
37+
38+
update_yaml_file(_FILENAME, {'cycode_client_id': 'recovered'})
39+
40+
assert read_yaml_file(_FILENAME) == {'cycode_client_id': 'recovered'}
41+
42+
43+
@pytest.mark.parametrize('read_only_path', [_DIRECTORY, _FILENAME])
44+
def test_write_is_skipped_on_a_read_only_filesystem(
45+
fs: FakeFilesystem, mocker: 'MockerFixture', read_only_path: str
46+
) -> None:
47+
fs.create_dir(_DIRECTORY)
48+
write_yaml_file(_FILENAME, {'cycode_client_id': 'original'})
49+
50+
real_access = os.access
51+
mocker.patch(
52+
'os.access',
53+
side_effect=lambda path, mode: (
54+
False if mode == os.W_OK and str(path) == read_only_path else real_access(path, mode)
55+
),
56+
)
57+
58+
write_yaml_file(_FILENAME, {'cycode_client_id': 'updated'})
59+
60+
assert read_yaml_file(_FILENAME) == {'cycode_client_id': 'original'}
61+
62+
63+
def test_write_leaves_no_temporary_files_behind(fs: FakeFilesystem) -> None:
64+
fs.create_dir(_DIRECTORY)
65+
write_yaml_file(_FILENAME, {'cycode_client_id': _CLIENT_ID})
66+
67+
assert [path.name for path in Path(_DIRECTORY).iterdir()] == ['credentials.yaml']
68+
69+
70+
# Stands in for a concurrent writer stopping partway through: the existing file must survive untouched,
71+
# which is what truncating it up front could never guarantee.
72+
def test_a_failed_write_leaves_the_existing_file_untouched(fs: FakeFilesystem, mocker: 'MockerFixture') -> None:
73+
fs.create_dir(_DIRECTORY)
74+
write_yaml_file(_FILENAME, {'cycode_client_id': 'original'})
75+
mocker.patch('yaml.safe_dump', side_effect=OSError('no space left on device'))
76+
77+
with pytest.raises(OSError, match='no space left on device'):
78+
write_yaml_file(_FILENAME, {'cycode_client_id': 'updated'})
79+
80+
mocker.stopall()
81+
assert read_yaml_file(_FILENAME) == {'cycode_client_id': 'original'}
82+
assert [path.name for path in Path(_DIRECTORY).iterdir()] == ['credentials.yaml']

0 commit comments

Comments
 (0)