Skip to content

Checkpoint the output variables a deployment actually captured

Placeholder ppxd requested to merge fix/checkpoint-output-variables into main

Summary

  • Resuming a deployment silently lost every output variable it had produced. PersistCheckpointAsync built the checkpoint by filtering the variable list with Name.StartsWith("Squid.Action."), but output variables are minted by SpecialVariables.Output as Squid.Action[{step}].Output.{name} — a bracket after Action, not a dot — so the predicate matched none of them. It did still match unrelated action-scoped config variables such as Squid.Action.Kubernetes.Namespace, so the column looked populated and nothing surfaced the loss. Any step after a resume that referenced an earlier step's output resolved it as empty.
  • This lands on the pause/resume paths specifically — transient-failure pause (#434), the resumable deploy timeout (#428) and guided-failure — i.e. the flagship recoverability behaviour.
  • Persist the set the executor actually captured rather than re-deriving it by name. The capture site already knows exactly which variables are outputs, and it mints three forms per capture (step-qualified, machine-qualified, bare alias); the bare alias is indistinguishable from an ordinary variable by name, so no predicate can select them correctly. DeploymentTaskContext.CapturedOutputVariables accumulates them and is re-seeded from the restored set on resume, so a deployment that pauses twice still checkpoints the first run's outputs.
  • Selection + sensitive-value encryption move into CheckpointOutputVariableSerializer, next to the existing OutputVariableMerger, so the behaviour is directly unit-testable against production code.

Non-breaking

  • The column keeps its List<VariableDto> shape; the restore path is untouched.
  • Checkpoints written by earlier servers still resume — including ones holding the old config-variable content (regression-locked by Resume_LegacyCheckpointWrittenByTheOldPredicate_StillRestores).
  • Sensitive values keep the same encrypt-on-write / decrypt-on-read contract and KDF scope; already-encrypted values are still not double-wrapped.
  • Empty capture still leaves the column null rather than "[]".
  • No interface, enum, wire or schema change. Full solution builds clean.

Why the existing suite missed it

The unit tests serialized through a test-local mirror of the production method whose doc-comment claimed "Drift detector below ensures the production helper retains the same contract" — no such detector existed. The mirror reproduced the buggy predicate, and the tests fed it invented names ("Squid.Action.Deploy.ApiKey") which satisfy that predicate but which production never emits. Both sides agreed and both were wrong. The 4 E2E tests that could have caught it are [Fact(Skip)], and even un-skipped they use the same invented names.

The mirror is deleted; every name in the new tests comes from SpecialVariables.Output, so they cannot drift from production again.

Test plan

  • Unit CheckpointOutputVariableSerializerTests (11): all three captured forms persist; sensitive encrypted and never plaintext; non-sensitive stays readable; already-encrypted not double-wrapped; live variable never mutated; empty/null → null; round-trip preserves the sensitivity flag. Includes an explicit guard asserting the bracketed production name does not match the old dot predicate.
  • Unit CheckpointOutputVariableAccumulationTests (3): restored outputs are re-seeded into the captured set (the multi-pause case), remain resolvable by later steps, and a fresh deployment seeds nothing.
  • Unit CheckpointSensitiveVarEncryptionTests rewritten to drive production on both halves; adds legacy-checkpoint compatibility.
  • Integration (real Postgres + DI-resolved AES-GCM) IntegrationCheckpointOutputVariableRoundTrip: production serializer → real jsonb column → production resume phase; asserts the secret is not in the column and all three name forms restore.
  • Red-green proven: re-planting the old predicate fails 7/11 serializer tests; removing the resume re-seed fails exactly the multi-pause test.
  • Full suites green: UnitTests 6067/6067, IntegrationTests 397/397.

Follow-up (not in this PR)

ApplyBatchResults' accumulation line has no automated coverage — the honest home for it is the resume E2E, whose 4 tests are skipped pending an IDeploymentCheckpointService spy decorator (already tracked in their skip reasons). Worth doing next, since that gap is what let this ship.

Merge request reports