fix(runner): avoid data race by retaining rate limiter pointer (#2653)

[TassioSales]

Proposed changes

Closes #2653

Fixes a data race during runner initialization where runner.New was dereferencing and copying the ratelimit.Limiter returned by its constructor into a value field on Runner. Because the limiter's constructor starts a background goroutine (run()) that concurrently writes to atomic token counters, copying the struct by value races with that goroutine.

Changes

• Changed Runner.ratelimiter field from ratelimit.Limiter to *ratelimit.Limiter.
• Retained constructor pointers directly in runner.New() across all rate limit branches (RateLimitMinute, RateLimit, NewUnlimited) without dereferencing.
• Added nil-safe guard in Close() before calling r.ratelimiter.Stop().
• Added unit tests in TestRunner_RateLimiterInitialization covering all rate limit modes.

Proof

• Ran unit test suite:

  go test -v -run TestRunner_RateLimiterInitialization ./runner
  go test -v -run TestRunner_duplicate ./runner

  All tests passed cleanly.

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)

Summary by CodeRabbit

• Bug Fixes
  • Rate limiting now initializes reliably for unlimited, per-second, and per-minute configurations.
  • Shutdown now safely handles cases where a rate limiter is not available, helping prevent errors when closing the runner.

[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: bded42b0-ddd5-40b3-834d-bc212112e491

📥 Commits

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

📒 Files selected for processing (2)

• runner/runner.go
• runner/runner_test.go

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

---

Walkthrough

The runner now stores the rate-limiter pointer returned by its constructor. Shutdown stops the limiter only when the pointer is non-nil. Tests cover default, per-second, and per-minute configurations.

Changes

Runner rate-limiter handling

Layer / File(s)	Summary
Rate-limiter initialization and shutdown runner/runner.go, runner/runner_test.go	New stores the limiter pointer for each configuration. Close checks for a non-nil pointer before calling Stop. Tests check limiter initialization and call Take for default, per-second, and per-minute configurations.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: mzack9999

Merge Risk: ⚪ Minimal · up to d5395

The runner retains its initialized rate limiter and stops it during shutdown. No merge-blocking behavior is established by the supplied review context.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to d5395

The change preserves the rate limiter’s identity instead of copying its mutable state. Existing rate settings, request throttling, public interfaces, and endpoint exposure remain unchanged. No material security risk introduced or worsened by this PR was identified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The evidenced change is confined to each Runner’s limiter lifecycle and existing throttled request paths. It does not add a caller, endpoint, credential capability, or cross-service dependency.

Trust Boundaries and Controls

• observed — The existing flow from rate options through limiter construction to Take before network operations is preserved. The limiter pointer is not exposed through the public Runner API or the existing concurrency endpoint by this change.

Resilience and Maintainability Implications

• observed — Both base and head can return nil and an error after constructing the limiter if classifier or authentication-provider initialization fails, without an explicit limiter cleanup on those paths. This pre-existing ownership gap is not introduced or worsened by pointer retention.
• observed — In the locally cached limiter implementation, Stop cancels an internal context rather than synchronously joining its goroutine; cancellation can wait behind a ticker receive. These dependency semantics and Runner’s cleanup ordering are unchanged, so the pointer fix does not establish a new prompt-shutdown or whole-Runner idempotency guarantee.

Hardening Proposals

• proposed — As separate lifecycle hardening, consider explicit constructor rollback for resources already started when later initialization fails. This addresses a pre-existing cleanup gap, not a regression in this PR.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 66.67% 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 rate limiter pointer to avoid a data race in runner initialization.
Linked Issues check	✅ Passed	The changes satisfy #2653. Runner.ratelimiter now stores a pointer. New assigns the pointer returned by ratelimit.New and ratelimit.NewUnlimited without copying the limiter value. The change c…
Out of Scope Changes check	✅ Passed	The reported changes are limited to runner/runner.go and runner/runner_test.go. They implement the pointer-retention fix for #2653 and add supporting initialization tests. The changes do not alter…

• Fix all pre-merge checks with AI

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.

---

_{A rabbit checks the limiter’s pace,
Then keeps its pointer in its place.
Three settings pass the Take test,
A guarded stop completes the rest.
The rabbit hops away to rest.}

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