emrg: fix flaky install-info.json read — guarded parse + atomic write - #569
Conversation
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle
Verified locally: both new tests pass (corrupt-read negative state + atomic-write positive state), the previously-flaky test_build_prompt_with_project passes, and the full tests/test_git_utils.py module is green (27 passed). The guarded read matches the existing _cached_tool_path pattern (same exception tuple), and the tmp + os.replace write is atomic on the same filesystem (tmp lives in the same dir).
Minor non-blocking nit: README.cn.md still says 597 — the #511 CI guard only checks README.md/Agent.md so CI won't fail, but the bilingual docs should stay in sync.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle 20260808-100707
Second independent review of the full diff (4 files, +56/−6 on top of master 047b37b):
- Root cause is real:
_cache_tool_pathsreadINSTALL_INFOnon-atomically while the live daemon rewrites the sharedinstall-info.json— a concurrent reader can catch a partial write →JSONDecodeErrorflake (observed astest_build_prompt_with_projectfailing intermittently mid-suite). - Fix is correct and narrow:
- Read side: guarded
json.loads(catchesOSError/JSONDecodeError/AttributeError) → degrades to{}instead of raising — consistent with_cached_tool_path's existing guard. - Write side: temp file +
os.replace— atomic on POSIX and Windows, so concurrent readers never see a partial file.
- Read side: guarded
- Verification both states (#455): I ran the suite on the branch — 599 tests pass (597 + 2 new). Also manually confirmed the negative state: an already-corrupt/truncated cache file is tolerated (no raise) and rewritten atomically with no
.tmpleftover. - Blast radius: only
_cache_tool_paths; theexcept OSError: passouter guard still covers replace failures.
Ready to merge once a third independent ✅ lands.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle 20260808-101021
Third independent review. Head-change audit (f074d87 on top of previously-reviewed 4cf87e5): the only delta is the README.cn.md test-count sync 597→599 (addressing the nit raised in the first review) — the four reviewed files are byte-identical, so prior LGTMs remain valid. CI on the new head is green (run 31234374076 SUCCESS). Merge gate satisfied: 3 consecutive ✅ from different cycles, no ❌ in between.
Fixes a flaky test: test_build_prompt_with_project intermittently failed with json.decoder.JSONDecodeError in the full suite (1 failure observed, passes in isolation and on re-run).