perf(rate-limit-guard): spool the statusline snapshot, drain it on a cadence (0.7.0) - #2521
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 3m 49s —— View job Security review —
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aad6de84ce
ℹ️ 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".
|
Last security-reviewed head: |
|
Claude finished @kyle-sexton's task in 4m 49s —— View job Code review — PR #2521
Scope: this repo runs a dedicated Finding posted inline🟡 Nit — What held up well
Not independently verifiedThe Bash tools available to this review lane don't permit executing the test suite or |
|
Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @cursor[bot]'s task in 1m 5s —— View job Security review —
|
|
Claude finished @cursor[bot]'s task in 2m 8s —— View job Code review —
|
e8cd2d0 to
dba094b
Compare
|
Claude finished @cursor[bot]'s task in 1m 37s —— View job Security review —
|
…cadence (0.7.0) Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…iene gate Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
39dd89e to
c2a4809
Compare
|
Claude finished @cursor[bot]'s task in 1m 11s —— View job Security review —
|
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 48s —— View job Code review —
|
… contract suite Address all five PR review findings: - Timer reads no longer fork: now_ms assigns via printf -v instead of a $(command substitution), which on MSYS added roughly a spawn floor to every sample (measured same-window on the repo tee: old instrument median 1434 ms vs new 510 ms). README carries an instrument note: the recorded #2521 absolutes are instrument-inclusive; the delta stands. - Missing EPOCHREALTIME (bash < 5.0) is now a loud refusal with rationale instead of a set -u unbound-variable abort; README reconciles this hard floor with the tee's own graceful 3.2+ degradation. - A failing render aborts the lane (bench-idle exits, bench-load marks and discards the run) instead of being timed as a plausible sample. - The load lane's pad-to-one-second arithmetic is computed properly in pace_sleep_arg: 0 ms spent now sleeps 1.000 s, not 0.1 s. - New co-located bench.test.sh contract smoke suite: unit-tests the lib helpers, runs each lane once with tiny parameters against the repo tee under an isolated HOME, and asserts the failure-abort paths — shape and behaviour only, never timing. scripts/affected-tests.sh --explain now maps every changed path (the P1 finding); exec bits set on all five scripts for the hygiene exec-bit gate. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W
…s (0.7.4) (#2583) ## Summary PR #2521's headline render-path measurement — `render.sh` alone **234.4 ms** vs **1047.1 ms** behind the pre-#2521 tee (Windows/MSYS, n=9, same-window) — was produced by a benchmark harness that lived only in an untracked local scratch directory. The merged perf claim was unreproducible, and nothing could catch a regression that quietly reintroduced the render-path cost. This PR commits the harness. - **`bench/lib-bench.sh`** — shared helpers: the spawn-floor control (median of 11 bare `bash -c exit` spawns, bracketing every timed section — on MSYS the process-creation floor dominates every number, so a run whose floor moved is discarded), the canonical statusline payload fixture, `now_ms`/`median`. - **`bench/bench-idle.sh`** — the lane that produced the headline numbers: N sequential renders, floor before/after, median + mean + raw samples. - **`bench/bench-load.sh`** — the concurrency lane: N virtual sessions rendering once a second for M seconds, the shape that stresses the spool/drain election. - **`bench/trace-probe.sh`** — xtrace of a non-elected render, printing everything executed before passthrough: the check that the render path stays fork-free, which is the property #2521 exists to protect. - **`bench/README.md`** — records what #2521 measured, on what platform, with what discipline, and how to re-run each lane (including isolating a run from the machine's live `~/.claude/rate-limit-guard/` contract file). Plugin `0.7.3 → 0.7.4` with a changelog entry, per the shipped-contents-change convention. ## Adaptation from the scratch originals - `STATUSLINE_ENTRY` no longer defaults to a machine-local `~/.claude/statusline/entrypoint.sh`; it defaults to this repo's `scripts/statusline-tee.sh` in standalone mode, resolved relative to the bench dir — runnable from a clean checkout. The env override is documented for measuring a real machine entrypoint. - `trace-probe.sh` defaults its tee argument to the repo copy the same way (it already isolated itself under a throwaway `HOME`). - `# shellcheck disable=SC2034` on the payload fixture (consumed by the sourcing scripts), matching the hook-utils precedent. - Everything else is byte-faithful to what produced the #2521 numbers. Verified against the tee at head: spool path, `.last-drain` stamp, `RLG_TEE_DRAIN_INTERVAL` (default 30), and the `{"e":…,"p":…}` record shape all still match. Three scratch files were **not** brought over: `gates.sh` (throwaway wrapper around repo CI gates, hardcoded to a dead worktree path), `pr-body.md` (byte-identical to #2521's merged body), `gates.log` (empty). ## CI stance Deliberately no CI wiring: a wall-clock benchmark on shared runners is noise, not a gate. None of these files use the `*.test.sh` suffix that `scripts/run-plugin-tests.sh` and the CI test lanes discover, so nothing new runs or gates in CI beyond the standard shell-lint gates. The tee's behavioural coverage remains `scripts/statusline-tee.test.sh`. ## Verification - Smoke-ran from the clean worktree: `bench-idle.sh 3` under an isolated `HOME`, and `trace-probe.sh` (pre-passthrough trace shows builtin-only work). - `shellcheck -x --rcfile=.shellcheckrc`, `shfmt -d`, `scripts/check-shell-portability.sh --paths` on all four scripts: clean. - `markdownlint-cli2`, `typos`, `editorconfig-checker` on changed files: clean. - `scripts/check-changelog-parity.sh` `--check` / `--check-order` / `--check-bump origin/main` / `--check-preserved origin/main`: pass. - `scripts/validate-plugins.sh`: all manifests + catalog pass. - All 15 suites `scripts/affected-tests.sh origin/main` selected: pass. - rate-limit-guard's own suites (`statusline-tee.test.sh` 96, `statusline-shim.test.sh` 36, `record-rate-limit-stop.test.sh` 19): all pass. ## Related Closes #2582 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Aac8xjCjMxFsXGHCXKHY4W --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
The statusline tee fires on every assistant message and every
refreshIntervaltick, once per open session. Measured same-window here (Windows/MSYS, n=9):render.shalone 234.4 ms,render.shbehind this wrapper 1047.1 ms. The wrapper dominates, and the dominant term inside it is process creation — a cost MSYS has no cheap primitive for.0.6.x made that work cheaper (nine spawns to four). This release takes it off the render path instead.
A refresh now records one line to a per-session spool file using only bash builtins — zero external processes, zero subshells — and one elected refresh per 30 s drains the batch through the same
tee_snapshot, unchanged.Design
Per-session files with a truncating
>, not a shared append spool. POSIX guarantees write atomicity for pipes up toPIPE_BUFand explicitly leaves regular-file concurrent-write behaviour unspecified. Through Cygwin/MSYS the observed no-interleave bound on appends is around a kilobyte, while statusline payloads are multiple kilobytes — and bash's buffered builtin output can split one large record across syscalls regardless. Atomicity therefore comes from file disjointness: no two writers ever share a file. A record torn by a kill mid-write failsfromjsonin the drain and is dropped (covered by a test).The filename is a shard key, never trusted data.
session_idarrives in the harness payload. It must match^[A-Za-z0-9._-]{1,64}$and not begin with a dot, or it shards to the literal namemisc. Traversal attempts, embedded quotes, 200-character values,has space, and JSON nulls are all covered by a test asserting nothing is written outsidespool/.Election is stamp-based and the elected refresh drains in-process. There is no timer to hang this on: Claude Code hooks are strictly event-driven and none fires on a schedule (hooks docs), an OS scheduler would mean three mechanisms across three platforms, and a resident lock-holder would have to be forked off a render — the exact cost being removed — and would be killed with it, since Claude Code cancels in-flight statusline scripts. So the renders are the clock. A herd collapses for one failed
mkdir.Bash 4.2 floor (
%(%s)Tis a 4.2 builtin). Below it — macOS bash 3.2, whereforkis cheap and this problem does not arise — the previous synchronous path runs untouched.RLG_TEE_ASYNC=1keeps its current behaviour on every version.Design deviation from the brief
The election takes its own lock (
spool/.drain.lock) rather than the existing snapshot lock. Two reasons, both load-bearing:tee_snapshotacquires and releases the snapshot lock through one global (TEE_LOCK). A drain holding that lock would maketee_snapshotburn its full retry budget (3×find+ 3×sleep) and thenrmdirthe lock out from under its own caller.tee_snapshotitself is untouched: same atomic temp-then-rename, samemkdir/rmdirconcurrent-writer lock, same windowless-writer preservation check.Verification
PASS=96 FAIL=0— all 75 pre-existing assertions pass unmodified, plus 21 new.RLG_TEE_DRAIN_INTERVAL=0is exported once near the top of the suite so existing cases keep their synchronous-visibility semantics; election is covered in dedicated cases that set the cadence per invocation.captured_at, proven rather than asserted:rlg_body) called by both the live probe and the drain, so the two cannot drift.HOMEwith a fresh stamp, runs the tee underbash -xwithBASH_XTRACEFD, and asserts that nojq|mkdir|mv|rmdir|find|date|uname|sleep|chmod|rminvocation and no command-substitution subshell frame appears before the passthrough.Reader-visible change (inside the existing contract)
The contract file now trails the newest refresh on the machine by up to 30 s instead of being rewritten on every refresh, and
captured_atis the observation time of the chosen record rather than the flush time — which is what lets a windowless refresh flush a window-bearing sibling's record without faking freshness. The reader contract budgets ten minutes of staleness and its operable floor values are unchanged.reference/reader-contract.mddocuments the cadence, thespool/inventory, and the.tee-disabledmarker.Enablement gate
The gate still gates the write, but it cannot be evaluated on the render path — reading settings costs a
jq. A drain that readsrate_limit_guard_enabled: falsewrites an epoch-stamped.tee-disabledmarker and drops the spool; refreshes then stop recording on one builtin test. The marker expires, so re-enabling the plugin recovers without a restart.🤖 Generated with Claude Code
https://claude.ai/code/session_01Lah151d2hvN4fBgMYdyjSH
Related
No linked issue — performance work with no tracked issue.