Use atomic YAML replacement for configuration writes - #69
Conversation
|
Thanks for putting this together, @Snuffy2 — the atomic write helper is a nice fix and I like the direction here. One thing I want to flag before merging: in Also, curious about the regression tests referenced in the commit messages — since |
Always route session and global config loads through the durable YAML loader so a valid backup can recover a missing primary file. Remove the session backup when deleting a session so secret-bearing backup data does not survive deletion. Add regression coverage for both public config paths.
Distinguish empty YAML files from valid top-level YAML null values so None can round-trip through the durable store. Also remove stale old-label backups during session rename to prevent missing-primary recovery from resurrecting a renamed session. Add focused regressions for both behaviors.
Move YAML load/write serialization into yaml_store with per-path re-entrant locks so all callers share same-path protection. Remove duplicated config/proxy locks and clean up session backup files on rename and delete to avoid retaining stale secrets. Add regression coverage for concurrent same-path writes plus session backup rename/delete cleanup.
Route session and global config loads through yaml_store even when the primary file is absent so valid backups can be restored. Add integration coverage for missing-primary recovery for session and global config files.
Coerce config and proxy YAML loads back to mappings before callers use mapping operations. Clear stale destination session backups during rename when no source backup exists, and remove duplicate delete-session coverage. Add regression tests for wrong-shaped YAML configs, proxies, and stale rename backups.
Return conflicts for stale session updates, journal label renames with an exact private pending payload, and recover committed renames before discovery or deletion. Validate session labels before filesystem access and keep normal missing optional configs quiet.
Keep directory fsync and orphan cleanup best-effort, use short UUID transaction artifacts, enforce UTF-8-safe session filename limits, and tolerate malformed rename journals without blocking discovery.
|
Good catch! I'll add that try/except in. I had a test suite for this branch but I realized there were no other tests in this repo so I pulled them out into a separate branch. I was going to add a separate pr after this with a full e2e, integration and some unit tests suite. I'm still building and testing everything. I can add just the tests for this branch back into this PR if you'd like. |
926e87b to
5dabe49
Compare
|
Makes sense, thanks for explaining. We don't track `tests/` in git ourselves either — everyone's environment differs enough that a shared suite can be flaky, and sensitive info has snuck into test scripts here before, so it's stayed local/untracked on purpose. Sounds like your plan for a fuller e2e/integration/unit suite is more solid than what either of us would have in a single PR anyway, so I'm fine merging this now and picking up tests when that lands separately. Merged — thanks for the fix on the corrupt-config path, and for the quick turnaround. |
2 participants
Summary
Centralizes YAML persistence behind a small atomic-write helper so interrupted writes cannot truncate configuration files. Existing malformed configuration is surfaced instead of silently replaced with defaults.
What Changed
Why
Directly writing YAML configuration files can leave them empty or partially written if serialization or disk I/O fails. Several callers also treated malformed configuration as missing data, which could overwrite recoverable user configuration.
This change uses one compact write-to-temp-and-replace boundary and explicit load errors. It intentionally avoids backup rotation, journaling, batching, or power-loss-atomic rename machinery.