Repository navigation
[db] record a stay-stopped runtime upgrade decision on its Kubernetes execution - #1304
Conversation
Review — 1 findingLow
Checked and found accurate: the Verified against |
5fed4be to
2adf499
Compare
ac-pplx
left a comment
There was a problem hiding this comment.
The problem. A runtime upgrade of a stopped sandbox, which the stacked PR adds, can fail partway: the Wake times out, or the backup enqueue or the pod delete fails after a good boot. The retry then has to resend exactly the upgrade that was sent, because sandbox-manager refuses a repeat of an operation whose target or authorization differs. Deciding again doesn't reliably name that operation: a policy change between attempts changes its inputs. This PR adds the three columns that keep the decision with the claimed execution.
Example.
attempt 1 claim execution G -> record (op X, E1, runtime-environment-blocked:E0) on G -> Wake X ... times out
(policy changes: the default moves to E2)
attempt 2 finds G -> reads X, E1 and the authorization from G -> resends X (deciding again would have named E0 -> E2)
Verdict. Approved; 1 thread to resolve before merge.
The schema checks out:
- The columns are nullable with no default, so no row is rewritten.
- The
NOT VALIDchecks pass for NULLs, and the all-or-none expression is never NULL. - The authorization-id pattern accepts only ids that sandbox-manager's trim-then-1..256-byte check leaves unchanged.
- The target pattern matches the catalog's.
- The table has no triggers.
- No
SELECT *orRETURNING *readssandbox_k8s_executionsat the merge base, so replicas running the previous image don't hit a cached-plan result change.
The fixes suggested on the stacked PR keep using these three columns.
Before merge (inline threads)
- The down migration should refuse while an active execution records a decision
Nits
- The up comment says "A later migration can VALIDATE them". Nothing plans to, and leaving the checks
NOT VALIDis harmless, since new writes are checked. Drop the sentence or keep it; either is fine. - The description mentions "the pull request stacked on this one" twice. A link to #1322 would help a reader.
2adf499 to
2ece836
Compare
2ece836 to
d230330
Compare
Summary
Adds three nullable columns to
sandbox_k8s_executionsthat record the runtime upgrade a stay-stopped activation decided for an execution: the upgrade's operation id, its target environment reference and its authorization id. The PR changes only the schema. Nothing writes or reads the columns yet, and every existing row reads NULL.Terms
sandbox_k8s_executionsrow, the Kubernetes pod an activation claims to run a sandbox on a node.UpgradeRuntimecan already end a sandbox stopped (docs/execution-environments.md, "Upgrading a sandbox's runtime"), but an upgrade Wake today must start it. PR [api] trigger a stopped sandbox's runtime upgrade from a test hook, and smoke it on Kind #1322, stacked on this one, has the control plane run this upgrade for a stopped sandbox.operation_idincrates/libs/api/src/controllers/substrate/runtime_upgrade.rs), so a retry with the same inputs names the same upgrade. Sandbox-manager keyssandbox_runtime_upgradesby it.runtime-environment-<cause>:<environment>, where the cause is why the source environment must move when the upgrade is decided: it'sblockedordeprecated, orunlistedwhen the policy gives it no state (authorizationin the same file).sha256:orblake3:followed by 64 lowercase hex digits, the formatruntime_environments.environment_refchecks.The problem
A failed stay-stopped upgrade attempt can leave its execution behind: the dispatched Wake timed out, or the backup enqueue or the pod delete failed after a successful boot. The operation id, the target and the authorization live only in the failed attempt's memory, so a retry that finds the leftover execution can't tell which upgrade it was running. It can't re-send that upgrade's Wake, and it can't tell from the target whether the upgraded disk still needs its backup. Deciding again doesn't recover it. The operation id hashes the decision's cause, source environment and cluster default with the sandbox, activation and subject (
operation_idincrates/libs/api/src/controllers/substrate/runtime_upgrade.rs), so a retry whose inputs differ from the first attempt's, as after a policy change, can derive a different operation, or decide on none, while the first attempt's upgrade may already have run. Re-sending the recorded operation id takes the recorded authorization id, which names the same cause and source: while sandbox-manager keeps its record of an operation, it refuses a repeat whose sandbox, target, target state or authorization id differs from the recorded ones (upgradeincrates/libs/sbx/mgr/server/src/service/runtime_upgrade.rs).The fix is to persist the decision with the claimed execution. A retry then re-sends that exact upgrade Wake, which sandbox-manager settles as the same operation, and the recorded target decides whether the disk needs its backup. Public SPACE's migration guard requires a Postgres migration to ship in a pull request that changes no runtime code, so the schema comes first here, and the store code and retry path follow in PR #1322, which is stacked on this one.
How this fixes it
runtime_upgrade_operation_id UUID,runtime_upgrade_target_environment_ref TEXTandruntime_upgrade_authorization_id TEXT. All three are nullable with no default, so adding them changes the catalog only and rewrites no row. The operation id names no control-plane row, so it has no foreign key.sandbox_k8s_executions_runtime_upgrade_target_checkholds the reference to the patternruntime_environmentsandruntime_upgrade_campaignsuse,^(sha256|blake3):[0-9a-f]{64}$. The UUID type checks the operation id.sandbox_k8s_executions_runtime_upgrade_authorization_id_checkholds it to 1 to 256 bytes of printable ASCII with no leading or trailing space, whichruntime-environment-<cause>:<environment>always is. Sandbox-manager trims the id and then requires 1 to 256 bytes. An id of that shape is unchanged by the trim, and the pattern needs no locale to classify it, so every id the check accepts, sandbox-manager accepts unchanged.sandbox_k8s_executions_runtime_upgrade_decision_checkrejects a row that records part of a decision.NOT VALID. Adding them doesn't scan the table. Every existing row is NULL and satisfies them, and new writes are checked either way.sandbox_k8s_executionsand refuses while any active execution records a decision, naming how many and one sandbox. Such an execution is a leftover a retry still has to finish, or one a start took over. Its comment says how each gets finished or retired while the image that writes the columns is deployed. Once only inactive executions record decisions, down drops the three columns.Validation
mainwith a migrator built frommain, inserted an execution row, then ran this branch's migrator up, down and up:pg_dump --schema-onlymatches the dump of the database migrated tomain, apart from pg_dump's per-dump\restricttoken.scripts/check-migration-versions.sh,scripts/check-postgres-migrations-isolated.shandscripts/check-postgres-migrations-squawk.shpass (Squawk 2.69.0: 0 issues).ci/scripts/run-rust-checks.sh tests-unprivilegedandtests-privileged) passes, except for six privileged tests that host can't run: the fs-verity tests innode-runtime-core, twonode-runtime-supervisorcontained_launchertests andspaced-guest-worker::embedding. CI'scommon / Rust testsjob runs both lanes, including those six.Not validated: a Kind or deployed run. Nothing reads or writes the columns yet.
Dependencies
None. This merges on its own. PR #1322, which adds the store code and the retry path, is stacked on this one.
Severity and risk
Severity: medium. Without the recorded decision, a retry of a failed stay-stopped upgrade can't tell which upgrade it was running or whether the disk needs its backup.
Risk: low. The migration adds three nullable columns and three
NOT VALIDchecks to one table under a 1-second lock timeout, and nothing uses them yet. Roll back by running the down migration after rolling back any image that writes the columns; it refuses while an active execution records a decision.AI assistance: Claude Code drafted the migration, its comments, the validation scripts and this description.