Skip to content

[db] record a stay-stopped runtime upgrade decision on its Kubernetes execution - #1304

Merged
the-gigi-pplx merged 1 commit into
mainfrom
gigi/pla-7298-stopped-upgrade-decision-migration
Nov 2, 2026
Merged

the-gigi-pplx merged 1 commit into
mainfrom
gigi/pla-7298-stopped-upgrade-decision-migration

Conversation

@the-gigi-pplx

@the-gigi-pplx the-gigi-pplx commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Adds three nullable columns to sandbox_k8s_executions that 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

  • Execution: one sandbox_k8s_executions row, the Kubernetes pod an activation claims to run a sandbox on a node.
  • Stay-stopped upgrade: a runtime upgrade that cold-boots a stopped sandbox's disk under the target environment and leaves the sandbox stopped. Sandbox-manager's UpgradeRuntime can 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 id: the UUID that names one upgrade. The control plane derives it as a v5 UUID from the sandbox, the activation, the move (source, target and cause) and the subject (operation_id in crates/libs/api/src/controllers/substrate/runtime_upgrade.rs), so a retry with the same inputs names the same upgrade. Sandbox-manager keys sandbox_runtime_upgrades by it.
  • Authorization id: the id of the authorization an upgrade Wake runs under, runtime-environment-<cause>:<environment>, where the cause is why the source environment must move when the upgrade is decided: it's blocked or deprecated, or unlisted when the policy gives it no state (authorization in the same file).
  • Environment reference: sha256: or blake3: followed by 64 lowercase hex digits, the format runtime_environments.environment_ref checks.

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_id in crates/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 (upgrade in crates/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

  • Add runtime_upgrade_operation_id UUID, runtime_upgrade_target_environment_ref TEXT and runtime_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.
  • Check the target's format. sandbox_k8s_executions_runtime_upgrade_target_check holds the reference to the pattern runtime_environments and runtime_upgrade_campaigns use, ^(sha256|blake3):[0-9a-f]{64}$. The UUID type checks the operation id.
  • Bound the authorization id. sandbox_k8s_executions_runtime_upgrade_authorization_id_check holds it to 1 to 256 bytes of printable ASCII with no leading or trailing space, which runtime-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.
  • Set all three or none. sandbox_k8s_executions_runtime_upgrade_decision_check rejects a row that records part of a decision.
  • Add the checks NOT VALID. Adding them doesn't scan the table. Every existing row is NULL and satisfies them, and new writes are checked either way.
  • Refuse to drop a decision that's still needed. The down migration locks sandbox_k8s_executions and 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

  • Round trip on a disposable Postgres 16 container. I migrated it to main with a migrator built from main, inserted an execution row, then ran this branch's migrator up, down and up:
    • After up, the existing row reads NULL in all three columns. They're nullable with no default, and the three checks are present and not yet validated.
    • Decisions with sha256 and blake3 targets, and with 1-byte, 256-byte and inner-space authorization ids, are accepted, and so is clearing a whole decision. These are rejected: each partial decision, uppercase hex, an unknown algorithm, a 65-digit digest, a non-UUID operation id, an empty authorization id, a whitespace-only one, ones with a leading space or tab, a trailing newline or a trailing no-break space, a lone no-break space, a non-ASCII letter, a 257-byte one, a 258-byte one of 129 characters, and updates that clear one column of a recorded decision.
    • Down refuses, and changes nothing, while four active executions record decisions, and again after one of them is retired. Once every execution that records a decision is inactive, down succeeds and keeps every row, and pg_dump --schema-only matches the dump of the database migrated to main, apart from pg_dump's per-dump \restrict token.
    • The second up succeeds, and every row reads NULL.
  • scripts/check-migration-versions.sh, scripts/check-postgres-migrations-isolated.sh and scripts/check-postgres-migrations-squawk.sh pass (Squawk 2.69.0: 0 issues).
  • The full workspace suite through CI's entry points on a Linux build host (ci/scripts/run-rust-checks.sh tests-unprivileged and tests-privileged) passes, except for six privileged tests that host can't run: the fs-verity tests in node-runtime-core, two node-runtime-supervisor contained_launcher tests and spaced-guest-worker::embedding. CI's common / Rust tests job 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 VALID checks 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.

@the-gigi-pplx
the-gigi-pplx marked this pull request as ready for review November 2, 2026 17:03
@the-gigi-pplx

Copy link
Copy Markdown
Collaborator Author

Review — 1 finding

Low

  1. db/postgres/20261102162511_execution_runtime_upgrade_decision.up.sql:20 — the stated reason re-deciding can't recover names a refusal that re-deciding can't hit.
    The comment, and "The problem" in the description, say sandbox-manager refuses a repeated operation id whose authorization id differs, and that the policy can change between attempts. But EnvironmentMove::operation_id (crates/libs/api/src/controllers/substrate/runtime_upgrade.rs:361) hashes cause, from and to into the v5 UUID, which are the same inputs authorization() builds the id from (line 345). So when the policy is unchanged, a re-decision rebuilds the same operation id and the same authorization id, and the invalid_argument branch at crates/libs/sbx/mgr/server/src/service/runtime_upgrade.rs:338 can't fire. When the policy has changed, it produces a new operation id. That id then isn't refused as a mismatch; it gets fenced as unavailable by the first operation's sandbox_runtime_upgrades row (refuse_runtime_upgrade_fence_in, line 58) until that one settles. That's still a reason to persist the decision, but the loss is "a retry can't name, so can't resume or settle, the upgrade in flight", not a mismatch refusal.
    Reword the sentence in both places before merge. Once the migration has been applied, editing its text changes its checksum, so the comment can't be corrected later.

Checked and found accurate: the ^(sha256|blake3):[0-9a-f]{64}$ pattern matches runtime_environments / runtime_upgrade_campaigns. Sandbox-manager's required_text trims and then bounds at AUTHORIZATION_ID_MAX_BYTES = 256 (runtime_upgrade.rs:794, :801). The runtime-environment-<cause>:<environment> producer fits the ASCII pattern. "Down runs once, without the lock retry up has" matches crates/libs/db/migrator/src/lib.rs:328. The all-or-none check is correct as a pair of NULL-equality conjuncts. NOT VALID plus nullable/no-default columns rewrites nothing. There are no newer migrations on main since the branch point, so the version order holds.

Verified against 5fed4be1312ac81b80b0d4ed9105150a9ced77c0. I read the code at the cited lines and checked CI (all green, including Migration guards). I didn't re-run the up/down round trip or the regex boundary cases myself; the description's Postgres 16 run covers those.

@ac-pplx ac-pplx left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 VALID checks 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 * or RETURNING * reads sandbox_k8s_executions at 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)

  1. 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 VALID is 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants