Repository navigation
fix(recovery): reuse the guarded Cluster connection - #4700
Conversation
@coderabbitai review |
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📜 Recent review details
📝 Walkthrough
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue remains in the reviewed change. Normal verification can proceed. Pre-merge checks |
  |
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 55.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 3 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: reusing the guarded Cluster connection during recovery. Description check The description is directly related to the changeset and explains the connection reuse, scope, safety constraints, and verification results. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 55.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 3 files. (1 skipped: 1 unsupported.)
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
Comment @coderabbitai help to get the list of available commands.
The review follow-up is in 948e3fc. A new regression reproduced the unchecked default-transport panic, then passed after the helper was changed to refuse unavailable or incompatible defaults. Fixture serialization, collection assertions and the HTTP/1 hijacker are now checked. Missing function comments were added. The full race suite, vet and lint with The secret-scanner URI is a deliberately invalid fixture under The earlier review is stale after this push. A new exact-head review is required before merge; no production recovery has been retried. |
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: 948e3fc
- CodeRabbit: its edited review summary, verified now, reports zero included reviews remaining after the 2026-10-10T08:33:25Z event; account-plan scope, recovery on the next included allowance or maintainer-authorized credits. That review covered the previous head, not this one.
- Codex: the provider's account usage-limit response from 2026-10-08T15:23:18Z remains present on a fresh read; recovery requires account credits or quota recovery. The current lane sweep also reports a later limit and no subsequent substantive green.
- Cursor Bugbot: its user/team usage-limit response from 2026-10-09T05:33:32Z remains present on a fresh read; recovery requires an administrator to change the usage/spend limit.
Current PR review objects, all conversation comments, complete review threads and current-head checks were read before fallback. No current-head substantive provider review or unresolved thread exists. Prior ancillary findings were fixed or refuted in the resolution record; no scanner or check was disabled.
The complete four-file diff was reviewed for correctness, credential handling, identity/scope preservation, byte and deadline bounds, no automatic mutation replay, refusal behavior, CAS semantics and production wiring. Independent exact-head review found no P0/P1/P2 findings. Full race tests, vet, the flagged lint checks and current-main source-wiring ablations pass. This is offline code review, not live recovery proof.
Verdict: no P0/P1 findings
Ready at 948e3fc. Required CI and all selected recovery safety checks pass, the exact-head local review is GREEN, all review threads are resolved, and the ancillary findings are fixed or refuted. The branch is conflict-free against current main. User evaluation: exercised the actual client against a synthetic mTLS API that offers HTTP/2. The client positively uses HTTP/1, reuses the final GET connection, sends the unchanged conditional PATCH with no intervening discovery/read, and refuses every tested failure without replay or cleanup. The unavailable-default regression reproduced a panic before the fix and passes afterward. The current-main source-wiring ablations also pass. These are runtime client observations, not a claim that production recovery succeeded; that acceptance remains the separately approved first-attempt workflow after merge. |
Summary
Reuse the existing protected certificate connection for the database Cluster's final read and conditional patch. This removes command startup, discovery and an extra target read from the recovery window without changing any safety precondition.
Only the protected completed-join recovery and pause diagnostic use this connection. Ordinary OIDC planning and other resource operations stay unchanged. Unsupported credentials fail closed; writes cannot be retried or redirected. No new dependency, credential or deployment exception is introduced.
Verification
The transport regression fails with the old CLI forwarding behavior and passes with this change. Synthetic TLS tests cover connection reuse, exact patch and dry-run queries, credential restrictions, bounded capture, throttling, redirects and lost responses. The full recovery race suite, Go vet, affected lint and current-main source-wiring regressions pass.
This shortens a demonstrated request window; it does not prove which precondition caused the previous rejection or guarantee recovery. Production remains at HOLD until a separately approved first-attempt recovery passes live verification.
Related: #4673. The boot-image and init-log repair chain still depends on database recovery.