fix(rate-limit-guard): reclaim leaked tee temp files and stop windowless clobber - #1822
Conversation
…ess clobber Claude Code cancels an in-flight statusline script when a new update arrives while the previous one is still running, and a cancellation between the write and the rename left the atomic-write temp file behind permanently. No failed rm is needed to explain it: the process never reaches the reclaim line, and the only reclaim paths were write-failure and retry-exhaustion. 61 orphans were found clustered in one busy 27-hour window, which is the shape the correlation predicts — the rename retry loop holds the file open longest exactly when the target is contended, which is also when the session is busy enough to trigger a cancelling update. Two mechanisms, because neither is sufficient alone. A trap reclaims on exit and on a catch-able signal; an age-filtered sweep of leftover siblings on the next refresh recovers what a SIGKILL, a crash, or power loss leaves, which no trap can. Reproduced with an mv shim that parks so the kill lands inside the window: before, SIGTERM and SIGKILL each leak one file; after, SIGTERM leaks none and a SIGKILL orphan is reclaimed by the next refresh. The sweep costs nothing on a clean directory — a shell glob decides whether to spawn anything at all, so a normal refresh runs no extra process on a path that already sits at two to four times the 300 ms debounce interval. Its one-minute age floor cannot race a concurrent session's live temp, whose write-to-rename window is sub-second and bounded by the 300 ms retry loop. Separately, a session with no rate-limit windows overwrote a snapshot that had them. On a mixed-auth machine an API-key or enterprise session landed a snapshot with rate_limits absent and a FRESH captured_at, so consumers never saw "stale" — they saw a current snapshot with no data and dropped to whole-guard reactive-only, on a machine where a window-bearing session had good data available. Each landing could destroy up to the reader contract's full ten-minute staleness budget of usable proactive data. The tee now skips the write when this session has no rate_limits and the target already has them; both tests are substring checks, one on buffered stdin and one on the target read with $(<...), so the guard adds no process to the hot path. A windowless session still writes when the target has no windows either, so a machine with no window-bearing session keeps an honest staleness signal. The reader contract's directory inventory named stop-events.jsonl.lock and told tooling sweeping the directory to expect it while omitting the only litter actually found there; it now documents the temp shape, why it can outlive its writer, and that a cleanup tool should leave it alone. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
|
Claude finished @kyle-sexton's task in 4m 43s —— View job PR Review
SummaryBoth defects are real, the fixes are correct, and the tests are thorough. A few details worth discussing, none blocking. Defect 1 — Trap + Sweep (statusline-tee.sh)Trap design is correct. Using
Defect 2 — Windowless clobber guard (statusline-tee.sh)Guard logic (lines 148–154): if [[ "$payload" != *'"rate_limits"'* && -f "$target" ]]; then
local existing=""
existing=$(<"$target") 2>/dev/null || existing=""
if [[ "$existing" == *'"rate_limits"'* ]]; then
return 0
fi
fiThe substring check for
Sweep skipped on early guard return: A windowless session that detects a window-bearing target (line 153: Test coverageCase 14 (trap/cancellation) — the Case 15 (sweep / aged orphan) — Cases 16/17 (clobber guard / windowless write) — comprehensively cover: (a) windowless does not clobber windows, (b) The SIGKILL case is correctly not tested with a trap (can't trap it) and instead covered indirectly through Case 15's sweep. The PR's verification table makes this gap and the reclaim path explicit, which is the right way to document it. Documentation
CHANGELOG is detailed and correct. The reproduction table from the PR body would be a good addition here, but the prose description is clear enough for a changelog entry. Version bump 0.3.6 → 0.4.0 is appropriate: the clobber guard changes observable write behavior (a windowless session no longer always writes), which is a minor-level behavioral change even though the intent is purely defensive. No issues found in
|
|
Claude finished @kyle-sexton's task in 4m 32s —— View job Security Review — PR #1822
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8a341ad73e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…p before skip Three review findings on the windowless-clobber guard. Window-bearing is now decided with jq has(), not a substring a forwarded value like "session_name":"rate_limits" defeats. The preservation check and the rename are serialized through a mkdir writer lock with a stale-holder age floor, closing the check-then-write race between a windowless and a window-bearing writer. The orphan sweep runs before the preservation early-return so a machine with only windowless sessions still reclaims a killed session's temp. Regression tests for all three. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01STASxNiqEm7doy2Z65un4H
…ee-temp-leak # Conflicts: # plugins/rate-limit-guard/.claude-plugin/plugin.json # plugins/rate-limit-guard/CHANGELOG.md
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-run the job to retry the review. A new push does not re-trigger this lane. |
|
Claude finished @kyle-sexton's task in 4m 31s —— View job Security Review — PR #1822
Reviewed FindingsSUGGESTION | Confidence: Low (narrow, timing-dependent, Windows-specific) — Lock's "proceed unlocked on acquisition failure" path can still let a windowless write clobber a concurrent window-bearing write claude-code-plugins/plugins/rate-limit-guard/scripts/statusline-tee.sh Lines 138 to 151 in e0dc9b6 claude-code-plugins/plugins/rate-limit-guard/scripts/statusline-tee.sh Lines 195 to 239 in e0dc9b6 The new lock correctly serializes the check-then-rename between two lock-abiding writers. But Those two windows are the same order of magnitude, so this interleaving is reachable:
The net effect reproduces the exact defect this PR fixes (a windowless session clobbers good window data with a fresh timestamp, hiding staleness from consumers) — just narrowed to the specific interleaving where a Windows rename-retry and a concurrent lock-acquisition timeout overlap, rather than the previous unconditional case. The design comment on L195-197 ("last-writer-wins between window-bearing snapshots is the pre-existing contract") only reasons about two window-bearing writers racing; it doesn't cover a window-bearing writer racing a windowless writer's delayed completion of an already-passed check. A structural fix would re-check What checked out
No CRITICAL or IMPORTANT findings with high confidence. The one SUGGESTION above is a narrow, timing-dependent residual of the same defect class this PR fixes — real but unlikely to trigger outside the Windows rename-retry path under tight concurrent contention. |

Summary
Two independent defects in the statusline tee, both verified by reproduction. The temp-file leak is the reported symptom; the windowless clobber is the sharper one, because it destroys usable data rather than littering.
Fix
Defect 1 — no crash-safe reclaim of the atomic-write temp file. Claude Code cancels an in-flight statusline script when a new update arrives while the previous one is still running, and a cancellation between the write and the rename left the temp behind permanently. No failed
rmis needed to explain it: the process never reaches the reclaim line, and the only reclaim paths were write-failure and retry-exhaustion.Two mechanisms, because neither is sufficient alone — the report is right that a trap must not ship as the whole fix:
The sweep is gated on a shell glob rather than on the proposed debounce, which gets the cost property the report wanted without the cadence change: on a clean directory — every refresh in normal operation — it spawns nothing, and it only reaches
findwhen a candidate already exists. Its one-minute age floor cannot race a concurrent session's live temp, whose write-to-rename window is sub-second and bounded by the 300 ms retry loop.Defect 2 — a windowless session overwrote a snapshot that had windows. On a mixed-auth machine an API-key or enterprise session landed a snapshot with
rate_limitsabsent and a freshcaptured_at, so consumers never saw "stale" — they saw a current snapshot with no data and dropped to whole-guard reactive-only, on a machine where a window-bearing session had good data available. The tee now skips the write when this session has norate_limitsand the target already has them. Both tests are substring checks — one on buffered stdin, one on the target read with$(<…)— so no process is added to the hot path. A windowless session still writes when the target has no windows either, so a machine with no window-bearing session keeps an honest staleness signal.Verification
Reproduced under a throwaway
HOMEwith anmvshim that parks, so the kill lands inside the write-to-rename window deterministically.origin/main)Sweep, planting one aged orphan and one live sibling then running a normal refresh:
Windowless clobber:
rate_limitsafter a windowless writecaptured_atafter that writeGates:
bash plugins/rate-limit-guard/scripts/statusline-tee.test.sh— PASS=41, FAIL=0. Seven new assertions: a cancel-mid-window case, the sweep reclaiming an aged orphan while sparing a live sibling and not disturbing the write, and three windowless-write cases. Case 7's existing "no temp-file residue" assertion — which passed while the invariant was broken, because its shim drives onlymvfailure — now has the cancellation stand-in it lacked.shellcheckon the tee — clean;check-shell-portability.sh— clean (the test plants an aged file with POSIXtouch -t, not GNUtouch -d);markdownlint-cli2andcheck-changelog-parity.sh --check-order— clean.What this PR deliberately does not do
Suggestion 3, the mtime debounce, is not taken here. It is the highest-leverage item for latency, and the report's margin analysis (10x against the 600 s staleness rule) is sound — but it is the only suggestion that changes a contract-visible cadence: the reader contract requires consumers to arm a Monitor and re-evaluate on every write, because a write is the only signal the windows changed under them. It is a performance change with a contract consequence rather than a defect fix, and the reason it was coupled to the sweep — spawn cost — no longer applies now that the sweep is glob-gated. Bundling a cadence decision into a data-loss fix seemed the wrong trade; it is left for a maintainer, with the issue open.
Suggestion 6, stale-sibling counting in
setup check, is likewise left open — though the fix that most reduces its importance is here: the leak is now self-reclaiming, so the condition the freshness probe cannot see is bounded to about a minute instead of being permanent.Related
atomic-write-staging-remnanthint (*.tmp.*) matching.tmpas an infix, so.rate-limits.json.tmp.<pid>.<random>hints where it previously matched nothing.TODO(#1218)— the single-account gap the debounce decision touches.Fixes #1807
🤖 Generated with Claude Code
https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C