fix(runner): retain the active rate limiter pointer

[root-Manas]

Proposed changes

runner.New copies a rate limiter after its constructor has started the token worker. The worker can update its atomic count concurrently with that copy, causing race detector failures and leaving the runner's counters disconnected from the worker.

Store *ratelimit.Limiter and retain the constructor result for unlimited, per-second, and per-minute modes. Existing Take and Stop calls use the same live instance. CLI rate-limit behavior is unchanged.

Fixes #2653.

Proof

• Untouched upstream dev reproduced the initialization race with GOMAXPROCS=2 go test -p 2 -race ./runner -run TestRunner_duplicate -count=10.
• New regression coverage consumes the sole token in per-second and per-minute modes and checks that the runner observes the worker's depleted count. Both cases fail against the original value field without needing the race detector. Unlimited initialization and cleanup are exercised repeatedly under the race detector.
• Passed on Linux/Go 1.26.0: GOMAXPROCS=2 go test -p 2 -race ./runner -run 'TestRunner.*RateLimiter|TestRunner_duplicate' -count=10 (128.307 seconds).
• Passed on Windows: go test -p 2 ./....
• Passed on Windows: go vet -p 2 ./... and go build -p 2 ./cmd/httpx.
• git diff --check passed. The repository's separate integration harness and hosted CI have not been run locally.

Checklist

• Pull request is created against the dev branch
• All checks passed (lint, unit/integration/regression tests etc.) with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate; no user-facing options changed)

Summary by CodeRabbit

• Bug Fixes
  • Rate-limited runners now consistently reflect token usage, keeping per-second and per-minute limits accurate.
  • Unlimited-rate runners now maintain reliable token availability across repeated creation and shutdown.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: be6058bb-b6f4-4883-b7e8-75961ce16af6

📥 Commits

Reviewing files that changed from the base of the PR and between d3b9d3d and 0bd721f.

📒 Files selected for processing (2)

• runner/ratelimit_test.go
• runner/runner.go

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

---

Walkthrough

Runner now stores the limiter pointer returned by its constructor. New tests check token availability for per-second, per-minute, and unlimited rate limiters.

Changes

Runner rate limiter

Layer / File(s)	Summary
Retain and test the active limiter runner/runner.go, runner/ratelimit_test.go	Runner.ratelimiter changes to a pointer, and New stores the limiter pointers returned by its constructors. Tests check token availability for limited and unlimited runners.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mzack9999

Merge Risk: ⚪ Minimal · up to 0bd72

No actionable merge-blocking issue is identified; the change is ready for normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0bd72

The runner now uses the same limiter instance for request pacing and shutdown. The change does not expose a new entrypoint or change rate-limit options. A construction-failure cleanup gap remains, but the evidence does not show that this change introduced or worsened it.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The examined effect is confined to limiter accounting and lifecycle inside the existing runner request paths; no new external entrypoint is evidenced.

Trust Boundaries and Controls

• observed — Request pacing remains inside Runner: its private limiter is consumed before the primary request and optional probes.

Resilience and Maintainability Implications

• observed — Limited-mode tests verify that the runner observes token depletion; the unlimited-mode test repeatedly constructs, consumes, and closes a runner. Neither verifies cleanup after constructor failure.

Hardening Proposals

• proposed — Consider stopping the limiter if later constructor steps fail, and covering that cleanup path with a focused test; this is not an observed regression from the pointer change.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: retaining the active rate limiter pointer in the runner.
Linked Issues check	✅ Passed	The PR meets the coding requirements in issue #2653. Runner.ratelimiter now stores a pointer. New retains the pointers returned by ratelimit.New and ratelimit.NewUnlimited, so the runner share…
Out of Scope Changes check	✅ Passed	The changes stay within issue #2653. The production change is limited to limiter ownership in runner/runner.go. The added tests verify the initialization race fix and rate-limit state. No unrelated …

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

---

_{A rabbit checks the tokens bright
One hops away, then waits a while
The minute limit joins the race
Unlimited tokens keep their pace
The runner holds the live pointer tight}

_{Comment @coderabbitai help to get the list of available commands.}

[Copilot]

Copilot review overview

🔵 Needs a closer look

The regression test does not deterministically prove the worker and runner share the same limiter instance.

Review effort: Lite
Findings: None

What changed in this PR

This PR fixes runner rate-limiter initialization by retaining the live limiter pointer instead of copying its state.

Changes:

• Stores *ratelimit.Limiter in Runner.
• Adds regression coverage for finite and unlimited rate-limiting modes.

File	Summary
runner/​runner.go	Retains constructor-returned limiter pointers.
runner/​ratelimit_test.go	Adds rate-limiter state and cleanup tests.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[root-Manas]

On the regression-test concern: the finite-mode tests check the counter after Take(). With the old value copy, the worker updates its own counter while the runner still sees the initial count. Both per-second and per-minute tests fail on that code and pass with the pointer fix.

The fixed tests passed ten repeated runs, including the earlier Linux race run. The unlimited-mode test is a race-detector exercise; it doesn't independently prove pointer identity. The 500 ms polling deadline is still sensitive to a heavily stalled test runner.