Add HTTP retry/backoff to ControlplaneApiDirect and remove @@api_token class state (#383)

[justin808]

Closes #383.

Hardens ControlplaneApiDirect — the HTTP client behind every Control Plane API call:

1. Bounded retry with backoff (Retrier): GET retries transient network errors (Net::OpenTimeout/ReadTimeout, ECONNRESET/ECONNREFUSED/EPIPE/ETIMEDOUT, EOFError, OpenSSL::SSL::SSLError, SocketError), 429, and 5xx. Equal-jitter exponential backoff (0.5·2^n capped at 10s); Retry-After delta-seconds honored, capped at 10s; 3 attempts max; 120s deadline (no new attempt begins after it; an in-flight attempt may run past it). POST/PATCH/PUT/DELETE retry only connect-phase failures — http.max_retries = 0 disables Net::HTTP's hidden internal resend of PUT/DELETE (verified on a real socket: a PUT RST mid-response was silently re-sent before this change, exactly once after it fails cleanly). 4xx ≠ 429 fails immediately. open_timeout 10s so connect-timeout retries are actually reachable within the deadline.
2. @@api_token class variable removed: replaced with ApiToken — a mutex-guarded, process-shared-by-default, per-instance-injectable token provider. Preserves CPLN_TOKEN priority, profile fallback, JWT 5-min-expiry refresh, the token-format guard, the {token:, comes_from_profile:} hash shape used by lib/command/run.rb, and the reset_api_token seam used by lib/core/controlplane.rb on profile switch (issue's claim that it was test-only was incorrect — it's a production call site).
3. Token fetch/refresh now checks Shell.cmd[:success] and raises TokenRefreshError with an actionable message instead of silently proceeding with garbage output.
4. Specs: 44 offline examples (0.09s, zero real sleeps via injected sleeper) — retry/backoff windows (non-overlapping jitter ranges pin exponential growth), Retry-After honored/capped/HTTP-date-fallback, deadline gate (stubbed monotonic clock), per-method retry policy incl. PUT/POST after-send no-retry, token provider lifecycle, class_variables.empty?, no manual state reset anywhere. spec/core/controlplane_api_direct_spec.rb added to the documented offline suite.

Codex Decision Log

• Non-blocking: Replace Shell.abort/exit in library code with typed exceptions handled at the CLI boundary #380 (typed error hierarchy) hasn't landed; the issue marks it as a dependency.
  • Decision: Proceed with file-local TokenRefreshError < StandardError; retry classification keys off exception classes/HTTP status directly.
  • Why: Maintainer explicitly targeted Add HTTP retry/backoff to ControlplaneApiDirect and remove @@api_token class-level state #383 in this batch; the classification is clean without the hierarchy.
  • Review later: Fold TokenRefreshError into the Replace Shell.abort/exit in library code with typed exceptions handled at the CLI boundary #380 hierarchy when it lands.
• Non-blocking: trace class attr left as-is (written by config.rb, read by controlplane.rb) — folding into instance config is not a non-breaking single-file change.
• Non-blocking: Authorization header computed once per call, not per attempt. Worst-case call ≈190s vs the 300s JWT refresh margin — no expiry window.
• Non-blocking (accepted nits): deadline check precedes the sleep, so an attempt can begin ≤10s past the 120s deadline; read-timeout retries cap at 2 attempts (60s default read timeout × deadline arithmetic); validate! takes a positional hash; mutex held across the cpln profile token subprocess (fine for a CLI).

Confidence

• Unit specs: 44 examples, 0 failures, ~0.09s; combined with the verified-double consumer spec (controlplane_api_spec.rb): 71 examples, 0 failures.
• Full documented offline suite + batch lanes: 239 examples, 0 failures at seeds 24680, 13579 (worker) and 77613 (reviewer).
• Independent skeptical review including a real-socket integration harness (8/8 scenarios, 0 leaked sockets): initial major (Net::HTTP hidden resend) found, fixed, and re-verified on the wire; all five fix areas delta-verified.
• Grep proofs: no @@api_token anywhere; reset_api_token only at its definition and the preserved controlplane.rb:23 call site; rubocop clean with three old disables removed and zero added.
• Risk: medium (runtime HTTP path) — mitigated by wire-level verification and unchanged response-handling semantics (200/202/404/403/5xx-raise message shapes byte-compatible, verified against the spec_helper retry matcher from Scope spec retries to transient failures, enable random ordering, tier timeouts (#382) #415).

Batch: CPFLOW B 07-16 15:43 (cpf-b-20260716-1543), lane issue383, epic #387.

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 24 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a8e30471-3f91-4d23-811b-a298e03118ef

📥 Commits

Reviewing files that changed from the base of the PR and between bb3881c and 8062a55.

📒 Files selected for processing (3)

• lib/core/controlplane_api_direct.rb
• spec/README.md
• spec/core/controlplane_api_direct_spec.rb

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch justin808/issue-383

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[greptile-apps]

Greptile Summary

This PR hardens the direct Control Plane HTTP client. The main changes are:

• Adds bounded retries and jittered backoff for transient GET failures.
• Prevents automatic replay of mutating HTTP requests.
• Replaces class token state with a synchronized shared provider.
• Adds token-fetch failure handling and expanded offline tests.

Confidence Score: 5/5

This looks safe to merge after a small retry-classification cleanup.

• Token caching, refresh, and reset behavior remain compatible with existing callers.
• Retry limits and mutating-request replay protection are covered.
• A reset before the first socket write can be classified as an already-sent request, causing a safe retry to be skipped.

lib/core/controlplane_api_direct.rb

Important Files Changed

Filename	Overview
lib/core/controlplane_api_direct.rb	Adds retry handling and synchronized token caching; the request-dispatch flag can suppress a safe retry before bytes are written.
spec/core/controlplane_api_direct_spec.rb	Adds coverage for token lifecycle, retry limits, backoff, response handling, and replay prevention.
spec/README.md	Adds the direct-client specs to the documented offline suite.

_{Reviews (1): Last reviewed commit: "Add HTTP retry/backoff to ControlplaneAp..." | Re-trigger Greptile}

[claude]

Review Summary

This is a solid, well-engineered hardening of ControlplaneApiDirect. Overview and assessment below.

What it does

• Replaces the @@api_token class variable with a mutex-guarded ApiToken provider (process-shared by default, injectable for tests), preserving CPLN_TOKEN priority, profile fallback, JWT-expiry refresh, and the reset_api_token seam used by Controlplane#profile_switch.
• Adds a bounded Retrier (3 attempts, 120s deadline, equal-jitter exponential backoff capped at 10s, Retry-After honored/capped) for GET requests on transient network errors, 429, and 5xx.
• Restricts POST/PATCH/PUT/DELETE retries to connect-phase failures only, and disables Net::HTTP's own hidden internal resend (max_retries = 0) — a real bug this PR fixes (a PUT could previously be silently resent once by Net::HTTP itself on a mid-response RST).
• Token fetch now checks Shell.cmd's :success and raises an actionable TokenRefreshError instead of silently proceeding with garbage/empty output.

Strengths

• Careful separation of "connect happened" vs "response received" (request_sent) correctly gates non-idempotent retries to the connect phase only — verified against real socket behavior per the PR description.
• Retry-After parsing explicitly forces base-10 (Integer(value, 10)), avoiding the classic octal-misparse bug for values like "010".
• Extraction into Retrier/ApiToken/small private methods keeps everything short enough that no rubocop:disable is needed (three old disables removed, zero added).
• Extensive offline test coverage (44 new examples) for backoff windows, deadline gating, per-method retry policy, and token lifecycle — all deterministic (stubbed sleeper/clock, no real sleeps).
• Both production call sites of the old API (lib/core/controlplane.rb#profile_switch, lib/command/run.rb) are preserved correctly.

Suggestions (non-blocking, left as inline comments)

1. 429 vs 5xx retry gating for non-idempotent methods (retry_response?, line ~161): a POST/PUT/DELETE that receives a 429 currently fails immediately with no retry. Since 429 means the server explicitly rejected the request before processing it, retrying is safe regardless of method — unlike 5xx/connection failures, where the mutation's effect is genuinely ambiguous. Worth splitting the 429 case out from the idempotent-only 5xx case.
2. DELETE excluded from IDEMPOTENT_METHODS (line 59): reasonable given uncertainty about whether the deletion already landed server-side before an error response, but since DELETE is normally considered HTTP-idempotent, a short comment explaining the exclusion would help future readers.

Other notes (already acknowledged in the PR's decision log, no action needed)

• Authorization header is computed once per call, not per attempt — accepted given the 190s worst-case vs. 300s JWT refresh margin.
• Read-timeout retries effectively cap at 2 attempts given the 60s default read timeout vs. the 120s deadline — accepted.

Overall: no correctness, security, or performance blockers found. Good test discipline and careful attention to the subtle failure modes (Net::HTTP's hidden resend, octal parsing, mutex-guarded shared token state).