mirror of
https://github.com/ChuckBuilds/LEDMatrix.git
synced 2026-10-04 22:35:08 +00:00
* fix(config): stop same-second backups overwriting each other
A backup's version is its identity. save_config_atomic() hands the path
back, rollback_config(backup_version=...) looks that version up, and the
paired secrets backup is found by reusing the same string.
The version was stamped at second granularity, so two saves inside the
same second produced the same filename and the second shutil.copy2()
silently overwrote the first backup. The path a caller was still holding
then pointed at different content, and rolling back to it restored the
wrong config. A user saving twice in quick succession lost a restore
point with no error.
list_backups() made it worse. It parsed the version off Path.stem, which
drops only the last dot-component, so for config.json.backup.20240101_120000
parts was ['config', 'json', 'backup'] and parts[-2] was 'json' -- never
'backup'. The filename branch was unreachable: every backup fell through
to the mtime fallback and reported a second-granularity restamp of its
mtime rather than the name on disk, so a unique filename alone would not
have been enough for rollback to find the right version.
Stamp microseconds, and never overwrite an existing backup -- on a
collision bump a -N suffix rather than lose a restore point. Parse the
version off the exact glob prefix so it round-trips with the filename,
still reading the legacy second-granularity format so restore points that
predate this keep working.
Two tests had encoded the bug:
- test_multiple_config_changes asserted a rollback produced plugin1=45
with plugin2=15, a state no single backup ever held -- 45 was only in
the second backup, 15 only in the first. It passed because the two
saves collided onto one file, so the first version resolved to the
second's content. Corrected to the state that backup actually holds.
- test_backup_rotation asserted against a hardcoded max of 3 while
setUp configured 5, and still passed: every save in its loop collapsed
onto a single filename, so there was only ever one backup to count and
rotation was never exercised. It now asks the manager for its limit
and overshoots it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(config): fold collision suffix into ordering, close backup-path race
_parse_backup_version() stripped any trailing "-segment" unconditionally,
so a collision-suffixed backup parsed to the exact same timestamp as its
sibling and list_backups() had no deterministic way to order them. Only
strip the suffix when it's numeric, and fold it back in as extra
microseconds so same-tick collisions sort newest-first reliably.
_create_backup() also checked backup_path.exists() before shutil.copy2(),
which two concurrent callers can both pass for the same path -- the second
copy2() then silently destroys the first call's restore point. Reserve
each path (config and, when configured, secrets) with exclusive file
creation instead of a check-then-copy, retrying on a real conflict.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
176 lines
7.6 KiB
Python
176 lines
7.6 KiB
Python
"""
|
|
Integration tests for configuration save/rollback flows.
|
|
"""
|
|
|
|
import unittest
|
|
import tempfile
|
|
import shutil
|
|
import json
|
|
from pathlib import Path
|
|
|
|
from src.config_manager_atomic import AtomicConfigManager, SaveResultStatus
|
|
from src.config_manager import ConfigManager
|
|
|
|
|
|
class TestConfigFlowsIntegration(unittest.TestCase):
|
|
"""Integration tests for configuration flows."""
|
|
|
|
def setUp(self):
|
|
"""Set up test fixtures."""
|
|
self.temp_dir = Path(tempfile.mkdtemp())
|
|
self.config_path = self.temp_dir / "config.json"
|
|
self.secrets_path = self.temp_dir / "secrets.json"
|
|
self.backup_dir = self.temp_dir / "backups"
|
|
|
|
# Create initial config
|
|
initial_config = {
|
|
"plugin1": {"enabled": True, "display_duration": 30},
|
|
"plugin2": {"enabled": False, "display_duration": 15}
|
|
}
|
|
|
|
with open(self.config_path, 'w') as f:
|
|
json.dump(initial_config, f)
|
|
|
|
# Initialize atomic config manager
|
|
self.atomic_manager = AtomicConfigManager(
|
|
config_path=str(self.config_path),
|
|
secrets_path=str(self.secrets_path),
|
|
backup_dir=str(self.backup_dir),
|
|
max_backups=5
|
|
)
|
|
|
|
# Initialize regular config manager
|
|
self.config_manager = ConfigManager()
|
|
# Override paths for testing
|
|
self.config_manager.config_path = self.config_path
|
|
self.config_manager.secrets_path = self.secrets_path
|
|
|
|
def tearDown(self):
|
|
"""Clean up test fixtures."""
|
|
shutil.rmtree(self.temp_dir)
|
|
|
|
def test_save_and_rollback_flow(self):
|
|
"""Test saving config and rolling back."""
|
|
# Load initial config
|
|
initial_config = self.config_manager.load_config()
|
|
self.assertIn("plugin1", initial_config)
|
|
|
|
# Make changes
|
|
new_config = initial_config.copy()
|
|
new_config["plugin1"]["display_duration"] = 60
|
|
new_config["plugin3"] = {"enabled": True, "display_duration": 20}
|
|
|
|
# Save with atomic manager
|
|
result = self.atomic_manager.save_config_atomic(new_config, create_backup=True)
|
|
self.assertEqual(result.status, SaveResultStatus.SUCCESS)
|
|
self.assertIsNotNone(result.backup_path)
|
|
|
|
# Verify config was saved
|
|
saved_config = self.config_manager.load_config()
|
|
self.assertEqual(saved_config["plugin1"]["display_duration"], 60)
|
|
self.assertIn("plugin3", saved_config)
|
|
|
|
# Rollback - extract version from backup path or use most recent
|
|
# The backup_path is a full path, but rollback_config expects a version string
|
|
# So we'll use None to get the most recent backup
|
|
rollback_success = self.atomic_manager.rollback_config(backup_version=None)
|
|
self.assertTrue(rollback_success)
|
|
|
|
# Verify config was rolled back
|
|
rolled_back_config = self.config_manager.load_config()
|
|
self.assertEqual(rolled_back_config["plugin1"]["display_duration"], 30)
|
|
self.assertNotIn("plugin3", rolled_back_config)
|
|
|
|
def test_backup_rotation(self):
|
|
"""Test that backup rotation works correctly."""
|
|
# setUp built the manager with max_backups=5; ask it rather than
|
|
# restating the number, which is how this drifted in the first place.
|
|
max_backups = self.atomic_manager.max_backups
|
|
|
|
# Overshoot the limit so rotation actually has something to remove.
|
|
for i in range(max_backups + 3):
|
|
config = {"test": f"value_{i}"}
|
|
result = self.atomic_manager.save_config_atomic(config, create_backup=True)
|
|
self.assertEqual(result.status, SaveResultStatus.SUCCESS)
|
|
|
|
# Rotation should have trimmed the excess, and kept exactly the limit.
|
|
#
|
|
# This used to assert against a hardcoded 3 while setUp configured 5,
|
|
# and still passed -- because every save in the loop landed in the same
|
|
# second and collapsed onto a single backup filename, so there was only
|
|
# ever one backup to count and rotation was never exercised at all.
|
|
backups = self.atomic_manager.list_backups()
|
|
self.assertEqual(max_backups, len(backups))
|
|
|
|
def test_validation_failure_triggers_rollback(self):
|
|
"""Test that validation failure triggers automatic rollback."""
|
|
# Create invalid config (this would fail validation in real scenario)
|
|
# For this test, we'll simulate by making save fail after write
|
|
|
|
initial_config = self.config_manager.load_config()
|
|
|
|
# Try to save (in real scenario, validation would fail)
|
|
# Here we'll just verify the atomic save mechanism works
|
|
new_config = initial_config.copy()
|
|
new_config["plugin1"]["display_duration"] = 60
|
|
|
|
result = self.atomic_manager.save_config_atomic(new_config, create_backup=True)
|
|
|
|
# If validation fails, the atomic save should rollback automatically
|
|
# (This would be handled by the validation step in the atomic save process)
|
|
self.assertEqual(result.status, SaveResultStatus.SUCCESS)
|
|
|
|
def test_multiple_config_changes(self):
|
|
"""Test multiple sequential config changes."""
|
|
config = self.config_manager.load_config()
|
|
|
|
# Make first change
|
|
config["plugin1"]["display_duration"] = 45
|
|
result1 = self.atomic_manager.save_config_atomic(config, create_backup=True)
|
|
self.assertEqual(result1.status, SaveResultStatus.SUCCESS)
|
|
|
|
# Make second change
|
|
config = self.config_manager.load_config()
|
|
config["plugin2"]["display_duration"] = 20
|
|
result2 = self.atomic_manager.save_config_atomic(config, create_backup=True)
|
|
self.assertEqual(result2.status, SaveResultStatus.SUCCESS)
|
|
|
|
# Verify both changes persisted
|
|
final_config = self.config_manager.load_config()
|
|
self.assertEqual(final_config["plugin1"]["display_duration"], 45)
|
|
self.assertEqual(final_config["plugin2"]["display_duration"], 20)
|
|
|
|
# Rollback to first change - get the backup version from the backup path
|
|
# Extract version from backup path (format: config.json.backup.YYYYMMDD_HHMMSS)
|
|
import os
|
|
backup_filename = os.path.basename(result1.backup_path)
|
|
# Extract timestamp part
|
|
if '.backup.' in backup_filename:
|
|
version = backup_filename.split('.backup.')[-1]
|
|
rollback_success = self.atomic_manager.rollback_config(backup_version=version)
|
|
else:
|
|
# Fallback: use most recent backup
|
|
rollback_success = self.atomic_manager.rollback_config(backup_version=None)
|
|
self.assertTrue(rollback_success)
|
|
|
|
# Verify rollback.
|
|
#
|
|
# result1's backup was taken *before* the first save, so it holds the
|
|
# config setUp wrote -- plugin1=30, plugin2=15 -- and neither change.
|
|
#
|
|
# This used to assert plugin1 == 45, a state no single backup ever held:
|
|
# 45 was only ever in result2's backup, and 15 only in result1's. It
|
|
# passed because both saves landed in the same second, so both backups
|
|
# were written to one filename and "result1's version" resolved to
|
|
# result2's content. Once the backup id became unique the rollback
|
|
# started returning the version actually asked for, and the assertion
|
|
# -- not the rollback -- was what had to change.
|
|
rolled_back_config = self.config_manager.load_config()
|
|
self.assertEqual(rolled_back_config["plugin1"]["display_duration"], 30)
|
|
self.assertEqual(rolled_back_config["plugin2"]["display_duration"], 15)
|
|
|
|
|
|
if __name__ == '__main__':
|
|
unittest.main()
|
|
|