fix: use atomic writes for persistent JSON state files - #27913
fix: use atomic writes for persistent JSON state files#27913vanthinh6886 wants to merge 1 commit into
Conversation
Replace direct write_text() calls with the existing atomic_json_write() helper (utils.py) which uses temp file + fsync + os.replace to prevent corruption on crash or power loss. Files fixed: - tools/skills_hub.py: SkillLockFile.save() and TapsFile.save() (installed skills registry and taps config) - gateway/sticker_cache.py: _save_cache() (sticker description cache) - tools/environments/base.py: _save_json_store() (generic JSON store used by environment backends)
|
Board James triage pass: this PR's |
|
Thanks for the focused crash-safety fix. I verified the premise still holds on current main for part of the PR, but the salvage needs a little more coverage and one stale hunk handled. Problems
Suggested changes
Automated hermes-sweeper review. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused persistence-safety work. The core premise remains valid on current main, but the patch needs a broader, re-targeted salvage.
Problems
tools/environments/base.py:169-172,tools/skills_hub.py:3404-3406, andtools/skills_hub.py:3479-3481still use direct JSON writes, so those fixes remain useful.- The same Skills Hub state module leaves sibling JSON cache writes direct at
tools/skills_hub.py:1141-1149,tools/skills_hub.py:3351-3368, andtools/skills_hub.py:3845-3850. gateway/sticker_cache.py:39-56is already atomic on current main viamkstemp,fsync, andos.replace; that hunk is a shared-helper cleanup rather than a missing crash-safety fix.
Suggested changes
- Re-target the lock/taps changes to their current locations and include the sibling Skills Hub cache writers, or document why those caches may remain non-atomic.
- Add save-boundary coverage for the affected helpers; existing helper tests (
tests/hermes_cli/test_atomic_json_write.py:13-120) do not verify these call sites are wired to it.
Automated hermes-sweeper review.
| @@ -2594,8 +2595,7 @@ def load(self) -> dict: | |||
| return {"version": 1, "installed": {}} | |||
There was a problem hiding this comment.
Please add coverage for this persistence boundary, not only atomic_json_write itself. Current Skills Hub tests exercise save/load behavior but do not verify that the lock and tap writers use the crash-safe write path.
| json.dumps(cache, indent=2, ensure_ascii=False), | ||
| encoding="utf-8", | ||
| ) | ||
| atomic_json_write(CACHE_PATH, cache) |
There was a problem hiding this comment.
Current main already writes this cache atomically with mkstemp, fsync, and os.replace; this is a useful consolidation onto the shared helper, but it should be treated as cleanup rather than part of the missing atomic-write fix.
Problem
Several files write persistent JSON state (installed skills registry, taps config, sticker cache, environment store) using
write_text()directly. If the process crashes or loses power mid-write, the file is left in a partially-written state — corrupted JSON that causesJSONDecodeErroron next read.Fix
Replace
write_text()with the existingatomic_json_write()helper fromutils.py, which uses:tempfile.mkstemp()— write to a temp filef.flush()+os.fsync()— ensure data hits diskos.replace()— atomically swap temp file into placeThis is the same pattern already used by
gateway/status.py,gateway/pairing.py, andcron/jobs.py.Files Changed
tools/skills_hub.pySkillLockFile.save()— installed skills registrytools/skills_hub.pyTapsFile.save()— skill taps configurationgateway/sticker_cache.py_save_cache()— sticker description cachetools/environments/base.py_save_json_store()— generic env backend storeBefore vs After
JSONDecodeErroron next readTests
Existing tests cover the happy path. Atomic write correctness is already validated by the
atomic_json_write()implementation and its existing test coverage intests/test_utils.py.