You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
api: carry restore decisions in one RestoreIntent value - #1660
User restores and the node drain's evacuation restores share the Kubernetes restore executor, which told them apart by testing evacuation_placement.is_some(), or passing evacuation: bool, at each decision: which claim to take, whether to join an in-flight or a completed restore, the charge, the wake coalescer, the snapshot and the pod placement, the runtime upgrade, the started-recovery redrive, and who commits restoring -> started. One Option<Option<EvacuationPlacement>> meant both "this is the drain" and "which cohort", and another kind of restore would have had to add a condition to every one of those branches.
Merging this builds a RestoreIntent once: RestoreIntent::user on the user restore and wake paths, and RestoreIntent::drain inside restore_evacuated_execution, whose signature is unchanged. The executor, ensure-pod, the suspend restore plan and the restore-source selector read its named fields (target, claim, snapshot, pod_placement, allows_runtime_upgrade, charge, coalesces, owner_commits_started, redrives_started_recovery) instead. The charge moves from RestoreClaimHandling into the intent, and the wake drops it for a preclaimed caller, which paid for its claim itself, as preclaimed_with_cleanup did.
This changes no behaviour. Each constructor sets every field to what the old test gave its caller. Branches keyed on the target state or on the claim variant keep that key, and branches that never fire for the drain keep the drain's old value.
Validation
At this head:
Flipping each field in its constructor fails an existing executor test for most fields. the_drains_re_placement_of_a_backed_off_stopped_evacuation_still_places_a_pod now passes a rollout cohort and checks the restore pod's cohort toleration and required node affinity, which no test covered before; routing the cohort to the no-cohort arms in ensure_pod_exists fails it. The drain's coalesces, owner_commits_started and redrives_started_recovery can't fire today: the drain is always preclaimed, its claim has no lease token, and its row is never started.
Future sizes at opt-level 0, base to head: no future on the path is larger than before, except the detached restore drive, 16 bytes larger, which is now boxed.
Pull CI: Rust format, workspace clippy with the repository structure check, and the full Rust test inventory (both groups) pass.
Locally, pplx-space-api and pplx-space-cluster-controller-server tests at CI's test profile (opt-level 0), with Postgres and Redis running: 3802 of 3803 pass. routes::e2b::tests::detail_reports_authoritative_federated_ownership_not_metadata fails on a JSON snapshot's key order when only these packages are built; it passes in Pull CI's full run.
Kind, at this head: local.suspend_restore_cross_node_from_object_storage, local.concurrent_restore_converges_to_one_execution and native_contract.command_wakes_idle_suspended_sandbox (the user constructor: lease claim, join and coalescing, remote snapshot, the owner committing started), and local.forced_drain_evacuates_sandbox and native.forced_drain_re_places_stopped_sandbox (the drain constructor: the marker claim, the newest remote snapshot, no source pin, the stopped materialization). All pass.
Severity and risk
Severity: does not apply; a refactor with no behaviour change.
Risk: a field set to the wrong value in a constructor would change that decision for every restore of that kind. The existing executor suites catch a flip of most fields, and the drain test above checks that the cohort reaches the restore pod; no test catches a user restore placed as an evacuation (see Not covered). Roll back by reverting this commit.
Not covered
On Kind, no scenario runs a drain with a rollout cohort, a started-recovery redrive, or an enforced admission charge. An executor test checks the cohort's toleration and affinity on the drain's restore pod, and executor tests cover the redrive and the charge.
No test drives a user restore through the executor and checks its source-node placement; only an ensure_pod_exists test without an intent checks it (expired_restore_redrive_with_vanished_source_uses_remote_snapshot_without_hard_pin).
No actionable comments were generated in the recent review.
Walkthrough
Restore behavior is now represented by RestoreIntent, with separate policies for user restores and drain restores. Restore planning accepts explicit snapshot choices instead of an evacuation boolean. Pod creation derives evacuation tolerations and affinity from the intent’s placement. Callers and tests now pass the intent or snapshot choice explicitly.
perplexityai/SPACE#1068: Both changes affect charging and attribution for preclaimed restores.
Merge Risk: Moderate · up to 25b41
This refactor changes how restore decisions are carried and is described as behavior-preserving. No concrete defect was found. The charge, redrive and source-placement paths have no scenario coverage, so check them before merging or accept that gap explicitly.
Security architecture risk: Low · up to 25b41
The inspected changes preserve the separation between user restores and privileged drain operations. No introduced security issue was identified. Residual risk is concentrated in preserving lifecycle and recovery behavior across this policy refactor.
Retained concerns
No architecture-level concerns identified.
Security review details
Security Blast Radius
inferred — The sensitive surface is shared sandbox lifecycle orchestration: claim ownership, snapshot-source selection and Kubernetes pod placement. The inspected operations remain keyed to a sandbox identity; no introduced path to select another sandbox's snapshot or acquire drain authority was identified.
Trust Boundaries and Controls
observed — User lifecycle calls reach fixed user-restore policy rather than accepting an intent from request input. The inspected production caller of restore_evacuated_execution is the node-drain controller, which derives placement and target from drain reconciliation. The executor separately selects the evacuation-marker claim and rejects a stopped target under user claim mode.
Resilience and Maintainability Implications
observed — Claim winners retain deferred charging and claim release on charge rejection. A preclaimed wake makes its cloned intent charge-exempt before detached execution, preserving the previous protection against charging the same activation again. Started-recovery supersession continues through the existing bounded redrive paths for user intent.
Pre-merge checks: 6
Check name
Status
Explanation
Title check
The title meets the format requirements and describes the main restore intent refactor.
Description Follows The Pr Template
The description contains all four required items. It describes the change, reports validation results, states severity and risk, and identifies the AI tool and its contributions.
Bug Fixes Name A Regression Test
This PR refactors restore decisions without fixing a stated application defect. The check does not apply.
Weakened Safeguards Are Justified
The changed drain test uses a cohort instead of no cohort. The description explains that this adds coverage for cohort placement. The existing assertions remain, and the diff adds cohort assertions. N…
Agent Tooling Runs No New Commands
The diff changes only Kubernetes Rust source and tests. It does not change agent manifests, skills, guideline files, or agent command files.
Public-Safe Content
The diff, title, and description contain no identified restricted content. The cohort domain in the test uses a reserved example domain. The reported measurements are from validation, not production.
Full details: Weakened Safeguards Are Justified
Explanation
The changed drain test uses a cohort instead of no cohort. The description explains that this adds coverage for cohort placement. The existing assertions remain, and the diff adds cohort assertions. No other safeguard weakening was found.
Recent review info
Automatic approval policy
Commit: 25b4176ccc15.
Setting
Effective value
Source
Automatic approvals
Enabled
Global configuration
Merge risk
Moderate
Global configuration
Security risk
Not set
Global configuration
No overrides matched. The global limits apply.
Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 1 remain after this review.
User restores and the node drain's evacuation restores share the
Kubernetes restore executor, which told them apart by testing
`evacuation_placement.is_some()`, or passing `evacuation: bool`, at each
decision: which claim to take, whether to join an in-flight or a
completed restore, the charge, the wake coalescer, the snapshot and the
pod placement, the runtime upgrade, the started-recovery redrive, and
who commits `restoring -> started`. One
`Option<Option<EvacuationPlacement>>` meant both "this is the drain" and
"which cohort", and another kind of restore would have had to add a
condition to every one of those branches.
Build a `RestoreIntent` once: `RestoreIntent::user` on the user restore
and wake paths, and `RestoreIntent::drain` inside
`restore_evacuated_execution`. The executor, ensure-pod, the suspend
restore plan and the restore-source selector read its named fields
(`target`, `claim`, `snapshot`, `pod_placement`,
`allows_runtime_upgrade`, `charge`, `coalesces`,
`owner_commits_started`, `redrives_started_recovery`) instead. The
charge moves from `RestoreClaimHandling` into the intent, and the wake
drops it for a preclaimed caller, which paid for its claim itself, as
`preclaimed_with_cleanup` did.
This changes no behaviour. Each constructor sets every field to what the
old test gave its caller. Branches keyed on the target state or on the
claim variant keep that key, and branches that never fire for the drain
keep the drain's old value. `restore_evacuated_execution` keeps its
signature. A drain restore test now checks that a rollout cohort reaches
the restore pod. At opt-level 0 no future on the path is larger than
before, except the detached restore drive, 16 bytes larger, which is now
boxed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
User restores and the node drain's evacuation restores share the Kubernetes restore executor, which told them apart by testing
evacuation_placement.is_some(), or passingevacuation: bool, at each decision: which claim to take, whether to join an in-flight or a completed restore, the charge, the wake coalescer, the snapshot and the pod placement, the runtime upgrade, the started-recovery redrive, and who commitsrestoring -> started. OneOption<Option<EvacuationPlacement>>meant both "this is the drain" and "which cohort", and another kind of restore would have had to add a condition to every one of those branches.Merging this builds a
RestoreIntentonce:RestoreIntent::useron the user restore and wake paths, andRestoreIntent::draininsiderestore_evacuated_execution, whose signature is unchanged. The executor, ensure-pod, the suspend restore plan and the restore-source selector read its named fields (target,claim,snapshot,pod_placement,allows_runtime_upgrade,charge,coalesces,owner_commits_started,redrives_started_recovery) instead. The charge moves fromRestoreClaimHandlinginto the intent, and the wake drops it for a preclaimed caller, which paid for its claim itself, aspreclaimed_with_cleanupdid.This changes no behaviour. Each constructor sets every field to what the old test gave its caller. Branches keyed on the target state or on the claim variant keep that key, and branches that never fire for the drain keep the drain's old value.
Validation
At this head:
the_drains_re_placement_of_a_backed_off_stopped_evacuation_still_places_a_podnow passes a rollout cohort and checks the restore pod's cohort toleration and required node affinity, which no test covered before; routing the cohort to the no-cohort arms inensure_pod_existsfails it. The drain'scoalesces,owner_commits_startedandredrives_started_recoverycan't fire today: the drain is always preclaimed, its claim has no lease token, and its row is neverstarted.pplx-space-apiandpplx-space-cluster-controller-servertests at CI's test profile (opt-level 0), with Postgres and Redis running: 3802 of 3803 pass.routes::e2b::tests::detail_reports_authoritative_federated_ownership_not_metadatafails on a JSON snapshot's key order when only these packages are built; it passes in Pull CI's full run.local.suspend_restore_cross_node_from_object_storage,local.concurrent_restore_converges_to_one_executionandnative_contract.command_wakes_idle_suspended_sandbox(the user constructor: lease claim, join and coalescing, remote snapshot, the owner committingstarted), andlocal.forced_drain_evacuates_sandboxandnative.forced_drain_re_places_stopped_sandbox(the drain constructor: the marker claim, the newest remote snapshot, no source pin, the stopped materialization). All pass.Severity and risk
Severity: does not apply; a refactor with no behaviour change.
Risk: a field set to the wrong value in a constructor would change that decision for every restore of that kind. The existing executor suites catch a flip of most fields, and the drain test above checks that the cohort reaches the restore pod; no test catches a user restore placed as an evacuation (see Not covered). Roll back by reverting this commit.
Not covered
ensure_pod_existstest without an intent checks it (expired_restore_redrive_with_vanished_source_uses_remote_snapshot_without_hard_pin).after_charge) instead of replacing it with an exempt charge.AI assistance: Claude Code made the change, ran the validation and drafted this description. A human reviews before merge.