Repository navigation
fix(recovery): quarantine retained standby storage before rebuild - #4685
Conversation
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 35 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (5)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (3)
🔇 Additional comments (7)
📝 WalkthroughWalkthroughThe change adds a completed-join recovery path for a retained wedding standby. The tool validates recovery evidence, pauses reconciliation, removes the specified completed Job and detached claim with identity preconditions, and checks retained storage and replacement health. A workflow gates production execution on manual dispatch conditions, and the README describes the recovery procedure. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change adds a separately approved recovery path for the retained wedding-db-1 standby. The recovery preserves the original storage and replaces only the stalled join Job and its detached claim. Destructive steps are guarded by identity checks and a production-only manual dispatch. No outstanding defects were identified, so the change appears ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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 24 functions across 3 files. (2 skipped: 2 unsupported.)
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 |
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: bc9ec01
- CodeRabbit: freshly verified this repository's included-review refusal at 2026-10-09T17:33:32Z, observed after this head was published; the stated 54-minute window ends at 18:27:32Z. Source: #4669 (comment). The earlier substantive review of aae9e92 is not counted as a review of this head.
- Codex: freshly re-read the account-wide code-review usage refusal published at 2026-10-08T15:23:30Z; it states no reset, and recovery requires an account limit change. Source: #4650 (comment). No additional credits are authorized.
- Cursor Bugbot: freshly re-read this user/team's usage or spend refusal published at 2026-10-07T19:32:36Z; it states no reset, and recovery requires an account limit change. Source: devantler-tech/monorepo#3949 (comment). Direct current-head check reads show no Bugbot result.
Verdict: no P0/P1 findings
Reviewed all five changed files for correctness, security and unnecessary complexity. Direct reads of this PR's review objects, conversation, threads and current-head reviewer check runs show no unresolved findings. CodeRabbit's earlier functional review found no actionable defect; its test-documentation warning is addressed by documenting every new named helper and test. The follow-up changes comments only.
Traced the fixed-target recovery through complete namespace consumer and backup reads, exact resource identities, current reviewed-source checks, the live operator leader and a fresh structured pause acknowledgment. The conditional Job and claim deletions happen only while the repair exclusively owns that pause. A failed read or rejected mutation returns HOLD without retries or reverse cleanup. The original volume and backing storage remain identity-bound, detached and retained; the old claim reservation is never cleared. Actual replacement mounts, not optional labels alone, prove fresh storage. Protected peers and backups are rechecked before every write, and success requires two separated complete healthy observations.
The production workflow accepts only a separately confirmed first main dispatch, runs race tests before credentials, uses the existing protected environment and shared deployment lock, and refuses changed source. It introduces no configurable mutation target or free-form command. The consumed legacy confirmation cannot enter this path.
Local whole-package race tests pass at this exact head. Earlier RED controls established ordinal-name reuse, actual mounted-volume freshness, all active or unknown backup phases, and complete unlabelled-backup coverage; stateful transaction tests cover success and the rejection boundaries. The published implementation also passed an actual OIDC-only production plan, with no writes. Workflow lint, Go validation and safety CI passed at the implementation-identical predecessor. Current-head hosted validation and the merge-group checks remain required; this review does not claim production recovery or waive any required check.
Readiness is bound to Exact-head CI completed successfully. GitHub reports CLEAN with no pending or failed checks. Every readable required check and required workflow passes. The configured managed code-quality rule has no readable per-head result; the normal merge API remains authoritative for that rule, without any bypass. The live plan below was freshly repeated immediately before promotion. The regression tests, race suite, vet, scoped lint, workflow lint and offline security analysis pass. The exact-head hosted recovery and initial-replacement safety jobs pass before any production credentials are restored. The signed head is valid and there are no unresolved threads or unaddressed review findings. The complete local review is posted as a GitHub Review at this head; its classifier reports GREEN. All three external providers' applicable quota refusals were verified before that fallback; no paid inference or Copilot review was requested. The clock-skew correction preserves lease expiry and operator identity, rejects replayed reconciliation IDs using a bounded pre-pause log baseline, and immediately stops on malformed pause acknowledgment. As the operator of this procedure, I repeated the actual default command against production through OIDC-only reads at 19:33 UTC. Its complete observation guard reports The separately approved recovery is executed only by a first, confirmed main dispatch behind the production environment and shared deployment lock. It conditionally removes only the stalled completed join job and old detached claim identity while preserving the retained PV and backing volume; the protected peers, backups and disruption budget stay untouched. Any drift or partial failure stops at HOLD without a retry or cleanup. Ordinary deployment filters and every production health/readback gate remain unchanged. |
The queue entry was removed before any recovery dispatch after the related boot PR's review identified a shared-code clock-skew defect. A regression reproduced the rejection with a 1 ms clock difference. That could stop the one-time recovery after reconciliation was paused. The signed head The clock-boundary and replay regressions failed before the repair. The complete race-enabled recovery suite, vet, scoped lint and diff checks pass. A fresh OIDC-only production plan also passes with the new baseline read: The current-head local review is GREEN after directly verifying all three external providers' applicable limits. Hosted recovery safety, initial replacement safety and security checks have passed, but the full CI run is still pending. This remains a draft until the complete current-head check set passes. Superseded validation runs are confirmed cancelled; no production transaction was interrupted. The existing recovery approval has not been consumed; execution still requires actual merge, another fresh plan, no competing production writer and the protected first main dispatch. No database recovery mutation has been made. The related boot PR carries this same final recovery implementation, so a later integration cannot restore the old clock check. |
@coderabbitai full review |
|
devantler
left a comment
devantler
left a comment
There was a problem hiding this comment.
🤖 Generated by the Agentic Engineer
Self-review (fallback — CodeRabbit, Codex and Cursor Bugbot unavailable)
Reviewed commit: c68fefe1e8572fcd7bac45552e09d7dfe61b4e50
- CodeRabbit: this PR's included-review request was refused at 2026-10-09T18:53:31Z, with a 35-minute retry window ending at 19:28:31Z. Freshly read at 19:00Z: #4685 (comment). This ongoing service limit applies to a new head; no current-head substantive review is present.
- Codex: the account code-review allowance is exhausted, reported at 2026-10-08T15:23:30Z and freshly re-read during this review: #4650 (comment). No reset is stated; recovery requires an account-limit change. No paid credits are authorized.
- Cursor Bugbot: this user/team's usage or spend limit was reported at 2026-10-07T19:32:36Z and freshly re-read during this review: devantler-tech/monorepo#3949 (comment). No reset is stated; recovery requires an account-limit change. Direct current-head check reads contain no Bugbot verdict.
Verdict: no P0/P1 findings
Reviewed all five changed files for correctness, security and unnecessary complexity. Direct reads of the current PR's review objects, conversation, threads and provider check runs show no unresolved finding. The clock-skew finding from the related boot PR was retained and independently reproduced, not discarded because the review provider is limited.
Traced the complete, fixed-target recovery through namespace-wide consumer and backup reads, observed resource identities, unchanged protected peers, current reviewed-main source checks and the bound operator leader. The original volume and physical backing identity remain detached, retained and reserved to the original claim. Job and claim deletions retain UID/resource-version conditions. The replacement's actual mounted storage must be new, and success requires two separated healthy observations.
The new two-second timestamp tolerance does not extend lease expiry, relax operator identity or admit an already observed reconciliation ID. The same bounded log window is read before and after pausing; malformed, truncated or oversized baseline coverage stops before mutation. A malformed acknowledgment after pausing now returns HOLD immediately, without waiting, deleting storage or performing cleanup. The audited operator fetches the Cluster before checking its pause annotation; that acknowledgment still comes from the bound leader and names the exact Cluster context.
The first confirmed main dispatch remains the only execution path. Race tests precede production credentials, the protected environment and shared production lock remain unchanged, and every write rechecks the reviewed source. This adds no free-form target, resume mode, retry after an unknown write, primary mutation, backup mutation or disruption-budget exception.
Independent clock-boundary regressions failed with a 1 ms skew on the old implementation. Stateful replay and unreadable-baseline controls failed before the replay safeguard; the malformed-acknowledgment control failed before immediate-stop handling. The full package passes race-enabled tests, vet and scoped lint at the final implementation. A fresh actual OIDC-only production plan also passes with no writes, including the baseline log read.
Current-head hosted CI and merge-group checks are still required. This review is neither a database-recovery receipt nor authority to bypass those checks. The approved recovery has not been dispatched.
Why
The previous standby repair can leave its replacement stuck on retained storage, blocking safe maintenance.
What
Add a separately confirmed recovery that preserves the original disk, replaces only the stalled job and storage claim, and rebuilds the standby on fresh storage. It stops if protected identities, backups, pause ownership or healthy peers change.
Part of #4673