Bound container image builds server-side, and the deploy step in CI - #12746
Bound container image builds server-side, and the deploy step in CI#12746nellshamrell wants to merge 8 commits into
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Unit Tests 2 files ± 0 459 suites ±0 8m 23s ⏱️ -21s Results for commit f8263fe. ± Comparison against base commit 921e55c. This pull request removes 11 and adds 95 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12746 +/- ##
==========================================
+ Coverage 59.91% 60.00% +0.09%
==========================================
Files 775 776 +1
Lines 46305 46444 +139
==========================================
+ Hits 27745 27871 +126
- Misses 18560 18573 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
48b5344 to
59a5904
Compare
There was a problem hiding this comment.
Pull request overview
Adds a validated, environment-overridable step timeout to the deploy phase in the extension’s Azure/AWS workflow templates to prevent pathological rad deploy runs from consuming the full GitHub Actions job budget, and introduces a structural test to keep the wiring correct.
Changes:
- Add a
Resolve deploy timeoutvalidation step and applytimeout-minutesto theRun rad commandsstep in both Azure and AWS workflow templates. - Document the new
RADIUS_DEPLOY_TIMEOUT_MINUTESvariable and rationale in.github/extension/README.md. - Add
deploy-timeout_test.shplusmake test-deploy-timeout, and include it in the aggregatetesttarget.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| build/test.mk | Adds test-deploy-timeout to the main test target and defines the new test target. |
| .github/extension/run-rad-commands-azure.yml | Validates RADIUS_DEPLOY_TIMEOUT_MINUTES and applies a step-level timeout-minutes to Run rad commands. |
| .github/extension/run-rad-commands-aws.yml | Same timeout validation + step-level timeout wiring for the AWS template. |
| .github/extension/README.md | Documents the new deploy step timeout variable and the step-vs-job rationale/limitations. |
| .github/extension/deploy-timeout_test.sh | Adds structural tests to ensure correct timeout placement, validation, and step ordering. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The run-rad-commands composite action seeded its outcome accumulator to `succeeded` and wrote the result file from a `trap ... EXIT`. Every other assignment was a failure value, so success was never earned - only seeded. A step timeout (the shape PR #12746 makes routine) or a cancelled job kills bash mid-command, so no failure branch runs, but the EXIT trap still fires and publishes the stale `succeeded` seed. publish-deploy-status then maps that to RUN_STATE=succeeded, so a killed deploy is reported as a successful one. Invert the default: seed `interrupted` with a non-zero exit code and assign `succeeded` only after the command loop completes. Failure paths exit before the promotion, so an abnormal termination now reports the seed. publish-deploy-status already maps unrecognized outcomes to `failed`, so the consumer needs no change. Adds command-outcome_test.sh, which extracts the accumulator prologue from action.yml and asserts a killed run publishes a non-success outcome, plus a publish-deploy-status case covering the `interrupted` -> failed mapping. Fixes #12756 Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Approver review
Orientation
What the code changesFive files, +277/-2:
The diff is coherent and contains no unrelated runtime changes. Data and control flowThe pathological QEMU container build occurs inside Key implementation decisionsShould this be a step or job timeout?Code's answer: Step timeout at Azure Why it matters: A job timeout produces cancellation, skipping the Assessment: Sound. Empirical testing corrected an earlier false rationale: Why validate the override separately?Code's answer: Reject malformed, zero, negative, and greater-than-330 values before calling Why it matters: GitHub's runner applies a timeout only when evaluation produces a positive value. Evaluation errors do not fail the step; they silently leave it unbounded. Assessment: Necessary. Base-10 normalization also prevents How does this reach existing repositories?It does not until their workflows are regenerated. The workflow templates are copied into downstream repositories ( Failure handling and operations
The important known defect is that a timed-out deployment is currently published as succeeded. The timeout fires the EXIT trap in This is pre-existing, but the PR makes the path routine. It is disclosed at Security and compatibilitySecurity: No material concerns. The variable enters bash through Compatibility changes:
All three are documented. Test evidence and gapsAutomated evidence:
Manual evidence:
Gaps:
Risk register
FindingsBlocking findings: None. Non-blocking concerns:
Strengths:
Approval recommendationApprove with follow-up. The timeout mitigation is correct, bounded, reversible, and substantially improves the reported behavior. Fix #12756 soon because publishing a timed-out deployment as succeeded is materially misleading, but it does not invalidate the runner-level failure or cleanup behavior delivered here. Final merge should wait for the three remaining CI checks. Coverage and confidence
|
The run-rad-commands composite action seeded its outcome accumulator to `succeeded` and wrote the result file from a `trap ... EXIT`. Every other assignment was a failure value, so success was never earned - only seeded. A step timeout (the shape PR #12746 makes routine) or a cancelled job kills bash mid-command, so no failure branch runs, but the EXIT trap still fires and publishes the stale `succeeded` seed. publish-deploy-status then maps that to RUN_STATE=succeeded, so a killed deploy is reported as a successful one. Invert the default: seed `interrupted` with a non-zero exit code and assign `succeeded` only after the command loop completes. Failure paths exit before the promotion, so an abnormal termination now reports the seed. publish-deploy-status already maps unrecognized outcomes to `failed`, so the consumer needs no change. Adds command-outcome_test.sh, which extracts the accumulator prologue from action.yml and asserts a killed run publishes a non-success outcome, plus a publish-deploy-status case covering the `interrupted` -> failed mapping. Fixes #12756 Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The run-rad-commands composite action seeded its outcome accumulator to `succeeded` and wrote the result file from a `trap ... EXIT`. Every other assignment was a failure value, so success was never earned - only seeded. A step timeout (the shape PR #12746 makes routine) or a cancelled job kills bash mid-command, so no failure branch runs, but the EXIT trap still fires and publishes the stale `succeeded` seed. publish-deploy-status then maps that to RUN_STATE=succeeded, so a killed deploy is reported as a successful one. Invert the default: seed `interrupted` with a non-zero exit code and assign `succeeded` only after the command loop completes. Failure paths exit before the promotion, so an abnormal termination now reports the seed. publish-deploy-status already maps unrecognized outcomes to `failed`, so the consumer needs no change. Adds command-outcome_test.sh, which extracts the accumulator prologue from action.yml and asserts a killed run publishes a non-success outcome, plus a publish-deploy-status case covering the `interrupted` -> failed mapping. Fixes #12756 Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The run-rad-commands composite action seeded its outcome accumulator to `succeeded` and wrote the result file from a `trap ... EXIT`. Every other assignment was a failure value, so success was never earned - only seeded. A step timeout (the shape PR #12746 makes routine) or a cancelled job kills bash mid-command, so no failure branch runs, but the EXIT trap still fires and publishes the stale `succeeded` seed. publish-deploy-status then maps that to RUN_STATE=succeeded, so a killed deploy is reported as a successful one. Invert the default: seed `interrupted` with a non-zero exit code and assign `succeeded` only after the command loop completes. Failure paths exit before the promotion, so an abnormal termination now reports the seed. publish-deploy-status already maps unrecognized outcomes to `failed`, so the consumer needs no change. Adds command-outcome_test.sh, which extracts the accumulator prologue from action.yml and asserts a killed run publishes a non-success outcome, plus a publish-deploy-status case covering the `interrupted` -> failed mapping. Fixes #12756 Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Deploy workflow runs could consume up to GitHub's 360-minute job default before failing. There was no `timeout-minutes` anywhere in .github/extension/, so a pathologically slow build - typically an emulated cross-architecture container build - burned hours of runner time with no bound. Add a `timeout-minutes` to the `Run rad commands` step in both provider workflow templates, defaulting to 30 minutes and overridable per environment with `RADIUS_DEPLOY_TIMEOUT_MINUTES`. The bound is on the step rather than the job deliberately. A job-level timeout ends the job outright, so the `if: always()` teardown would not run and `rad shutdown` would never persist state, and the deployed-graph publish step would be skipped exactly when a deploy hung. A failed step leaves both to run on the job's remaining budget. A preceding `Resolve deploy timeout` step validates the override. The runner applies a step timeout only when it evaluates to more than zero and treats an expression it cannot evaluate as no timeout at all, so an unchecked 0, negative, or malformed value would silently restore the unbounded behavior. Values are constrained to 1-330 minutes, which also keeps the step inside the job budget with room for teardown. This is a mitigation, not a cure: it converts a multi-hour hang into a bounded failure. It does not make builds faster and does not cancel server-side work. Fixes #12595 Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com>
`mapfile` requires bash 4, but macOS ships bash 3.2 and the contributing docs list macOS as a supported contributor OS. CI runs ubuntu so this would have passed there while breaking `make test` locally for macOS contributors, since `test` now depends on `test-deploy-timeout`. Replace the array-based step lookup with a stream-based position lookup that uses no bash 4 constructs. Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com>
Two assertions in the deploy timeout test passed for the wrong reason. The 'minutes=' check matched the local arithmetic assignment, so deleting the write to GITHUB_OUTPUT still passed even though the timeout expression would then evaluate to empty and leave the step unbounded. The variable check matched the name where it appears in the error messages, so dropping the env mapping still passed while silently ignoring the override. Both now match the exact wiring. Also correct an overclaim about the 330-minute ceiling. The setup steps before the deploy draw on the same 360-minute job budget, so the ceiling does not guarantee room for teardown. Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Running the behavior on a real runner disproved the stated justification. A job-level timeout does not skip the always() teardown: a 240-second teardown ran to completion under a job-level timeout, so rad shutdown would still persist state. The real difference is narrower. A job-level timeout cancels the job, which skips the Publish deployed graph and status step because it is guarded by if: !cancelled(), so the run loses its graph and status. A step timeout fails only that step and leaves publish and teardown to run normally. A job-level bound would also fold the setup steps into the same budget. Step-level placement still stands; the reason it was given for it did not. Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…ions The bound lives in the copied workflow file, so an existing repository keeps its previous unbounded behavior until its workflows are regenerated. The shared composite actions are the part that updates itself through the pinned ref, but GitHub does not support timeout-minutes on composite steps, so they cannot carry the bound instead. Say so, and name the shell-level imeout alternative and what it costs. Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Measured on a runner: the shared action writes its result file from an EXIT trap that does fire when the step is killed, and the outcome accumulator is seeded optimistically and never re-affirmed on success. So the timeout this section documents currently produces a run that reports success. Point at the tracking issue and tell readers to trust the run conclusion instead. Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
de96aab to
7b1dd4d
Compare
Match the complete shell defaulting expression instead of relying on an opaque substring. This keeps DEFAULT_TIMEOUT as the source of truth while making the assertion's intent obvious to reviewers and maintainers. Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
The run-rad-commands composite action seeded its outcome accumulator to `succeeded` and wrote the result file from a `trap ... EXIT`. Every other assignment was a failure value, so success was never earned - only seeded. A step timeout (the shape PR #12746 makes routine) or a cancelled job kills bash mid-command, so no failure branch runs, but the EXIT trap still fires and publishes the stale `succeeded` seed. publish-deploy-status then maps that to RUN_STATE=succeeded, so a killed deploy is reported as a successful one. Invert the default: seed `interrupted` with a non-zero exit code and assign `succeeded` only after the command loop completes. Failure paths exit before the promotion, so an abnormal termination now reports the seed. publish-deploy-status already maps unrecognized outcomes to `failed`, so the consumer needs no change. Adds command-outcome_test.sh, which extracts the accumulator prologue from action.yml and asserts a killed run publishes a non-success outcome, plus a publish-deploy-status case covering the `interrupted` -> failed mapping. Fixes #12756 Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
sk593
left a comment
sk593
left a comment
There was a problem hiding this comment.
We should also add timeouts for Radius deployments in the DE/UCP. The deploy workflows waits for the rad deploy command to finish before returning to the user so it's not the workflow that's taking too long to finish, it's the Radius deployment itself.
We can keep timeouts in the workflow as an extra failsafe but I don't think it should replace timeouts within the Radius deployment itself.
The run-rad-commands composite action seeded its outcome accumulator to `succeeded` and wrote the result file from a `trap ... EXIT`. Every other assignment was a failure value, so success was never earned - only seeded. A step timeout (the shape PR #12746 makes routine) or a cancelled job kills bash mid-command, so no failure branch runs, but the EXIT trap still fires and publishes the stale `succeeded` seed. publish-deploy-status then maps that to RUN_STATE=succeeded, so a killed deploy is reported as a successful one. Invert the default: seed `interrupted` with a non-zero exit code and assign `succeeded` only after the command loop completes. Failure paths exit before the promotion, so an abnormal termination now reports the seed. publish-deploy-status already maps unrecognized outcomes to `failed`, so the consumer needs no change. Adds command-outcome_test.sh, which extracts the accumulator prologue from action.yml and asserts a killed run publishes a non-success outcome, plus a publish-deploy-status case covering the `interrupted` -> failed mapping. Fixes #12756 Signed-off-by: Nell Shamrell-Harrington <nells@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@sk593 Thank you — this led me to investigating deeper. Below is what I found, including a correction to part of my own framing. Where the deploy is actually bounded today
The layers are inverted. dynamic-rp's 24h sits outside the DE's 4h, so Radius's own per-resource cap can never fire — the DE always gets there first. That is why "Radius has a deployment timeout" is effectively untrue today, and I think it is the concrete form of the gap you are describing. I could not find a path where UCP bounds a user deployment. Proposed target orderingNote that in CI the workflow bound will still usually fire first. That is intended — it is a runner-cost cap. The server-side bounds are what protect a developer running Proposed follow-up work
To reduce the blast radius of this change, I'd like to pursue a containerImages-specific build timeout first.
|
|
Converting this to a draft while I explore the design |
The workflow timeout added earlier only bounds CI. A containerImages build run by dynamic-rp had no timeout of its own, so it was capped only by the deployment engine's four-hour extensible-resource job timeout. An emulated cross-architecture build could therefore occupy a deployment for hours. Give the build hook its own three-hour budget, applied at the top of executeImageBuild so it also covers retrieving registry credentials. The budget uses WithTimeoutCause with a private sentinel because the async worker cancels operations rather than letting a deadline expire, so DeadlineExceeded alone cannot tell a build we stopped from one the caller cancelled. The resulting message reports the elapsed time and the limit. A deadline alone would have failed open. runScript drains stdout and stderr before calling Wait, and those drains end only at EOF. Killing the process group cannot reach a descendant that left it via setsid while holding the inherited pipes, so the drains, and the build, would block forever. Close the read ends after a grace period once the context is done. cmd.WaitDelay does not help here because it only applies inside Wait. The new regression test hangs for the script's full duration without the forced close, and completes in under a second with it. Signed-off-by: Nell Shamrell-Harrington <nellshamrell@microsoft.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
nellshamrell
changed the title
|
Pushed the server-side timeout — f8263fe. @sk593 you were right, and the layering turned out to be worse than "no timeout". Innermost to outermost, what exists today:
The layers are inverted — dynamic-rp's 24h sits outside the DE's 4h, so no Radius-owned timeout can ever fire. Every hang is bounded by the deployment engine, in a different repo, at 4 hours. (Both DE values are get-only expression-bodied C# properties, so neither is configurable.) So this adds the innermost bound rather than another outer one: a 3-hour budget on the containerImages build, derived at the top of Two things worth your attention. The default is 3h, not something aggressive. The observed pathological build ran 1-2 hours, so a 30-minute server-side default would fail it outright. The workflow's 30m already handles the CI-cost question; this one exists to stop a build occupying a deployment indefinitely, and sits under the DE's 4h so Radius reports the failure. Happy to change the number if you'd rather it be tighter. A deadline alone would have failed open. This is the same fail-open class this PR already hit once with Scope, precisely. This bounds the containerImages post-deployment build hook. It does not cover recipe OCI retrieval ( It's hardcoded rather than configurable because The PR description is updated and my earlier "scope was deliberately kept to the workflow" note is superseded. Left in draft until you've had a look at the direction. |
Radius functional test overviewClick here to see the test run details
Test Status⌛ Building Radius and pushing container images for functional tests... |
|
I'm working on a more comprehensive design for this (as well as an example implementation for it) |
…ject#12759) Fixes radius-project#12756 ## Problem The `run-rad-commands` composite action seeded its outcome accumulator to `succeeded` and wrote the result file from a `trap write_result EXIT`. Every *other* assignment to `OVERALL_OUTCOME` was a failure value (`command_failed`, `disallowed_command`), each followed by an explicit `exit`. So success was never earned — only seeded. That works for a normal command failure: `record()` returns non-zero, a failure branch assigns `command_failed`, and the trap writes the truth. It does **not** work for a *termination*. A step timeout kills bash mid-command, so no failure branch ever runs — but the EXIT trap still fires and writes the stale `succeeded` seed. `publish-deploy-status` then maps that to `RUN_STATE=succeeded`, and a deploy that was killed is published as a successful one. This is worse than the missing-file case the mapping was designed for: `unknown` → `in_progress` is the intended "no verdict" sentinel, and the trap defeats it by supplying a confident wrong verdict instead of no verdict. radius-project#12746 adds a `timeout-minutes` bound to this step, which makes the affected path the normal shape of a slow-deploy failure rather than an edge case. ## Fix Invert the default so the accumulator is pessimistic: - Seed `OVERALL_OUTCOME="interrupted"` / `OVERALL_EXIT=1`. - Assign `succeeded` / `0` exactly once, after the command loop completes. Every failure path exits before reaching the promotion, and an abnormal termination never reaches it at all, so a killed run now reports the seed. `publish-deploy-status` already maps unrecognized outcomes to `failed` via its `*)` arm, so the consumer needs no change. The fix lives in the composite action rather than the workflow templates on purpose: templates are copied into user repositories and only update on regeneration, whereas composite actions resolve at the pinned `{{RADIUS_REF}}` and reach existing repositories automatically. **On cancellation:** the issue asked whether a cancelled job reaches the same path. It does — cancellation kills the step identically — but both deploy workflow templates gate `Publish deployed graph and status` on `if: ${{ !cancelled() }}`, so a cancelled job publishes no status at all. There the seed only corrects the uploaded `rad-commands-result` artifact. The comments and README say so explicitly rather than overclaiming. ## Tests New `command-outcome_test.sh` extracts the accumulator prologue *verbatim* from `action.yml` (scoped to the `Run rad commands` step so an unrelated step can't silently redirect the extraction), then models a step timeout faithfully: it forks a subshell with the same options GitHub gives a `shell: bash` step, waits for the trap to be installed, asserts the result file is not written eagerly, and sends a real `SIGTERM`. It then asserts the published outcome is neither `succeeded`/`success` nor `unknown` (which would map to the neutral `in_progress`), plus static checks that `succeeded` is assigned exactly once and after the last failure assignment. Mutation-tested against three regressions — restoring the optimistic seed, removing the promotion, and renaming `command_failed` — each fails with a specific diagnostic. Also adds an `interrupted` → `failed` case to `publish-deploy-status_test.sh`, and wires the new suite into `make test` as `test-command-outcome`. Validation: both suites pass; `shellcheck` 0.11.0 with the repo rcfile is clean; both `action.yml` files parse. ## Docs `.github/extension/README.md` now documents the pessimistic-accumulator contract next to the `rad-commands-result` output description, including the cancellation caveat. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Fixes #12595
Problem
Deploy workflow runs could consume up to GitHub's 360-minute job default before failing. There was no
timeout-minutesanywhere in.github/extension/, so a pathologically slow build burned hours of runner time with no bound. Observed runs hit 80–120 minutes; a healthy 9-image deploy takes ~5.The underlying cause is established and lives outside this repo — see @sylvainsf's RCA and @nicolejms's
arm build.mdon the issue. TheRadius.Compute/containerImagesrecipe defaults to dual-arch, so thelinux/arm64half builds under QEMU emulation on an amd64 runner, on a BuildKit sidecar pinned tomax-parallelism = 1.#12640 (architecture-aware builds) is the cause-side fix and is already merged. It is opt-in — an empty
MODEor an unsubstituted placeholder disables it — and it cannot help a genuinely mixed-arch cluster. It also bounds nothing. This PR is the backstop for every path it doesn't cover.Change
Two timeouts, at two layers.
In the Radius deployment (
pkg/recipes/driver/bicep): a 3-hour budget on aRadius.Compute/containerImagesbuild. This is the layer @sk593 asked for — see the discussion below.In CI (
.github/extension/):timeout-minuteson theRun rad commandsstep in both provider workflow templates, defaulting to 30 and overridable per environment withRADIUS_DEPLOY_TIMEOUT_MINUTES. This stays as the outer failsafe.Workflow:
timeout-minuteson the deploy stepWhy the step and not the job
A job-level timeout cancels the job. Cancellation skips steps guarded by
if: ${{ !cancelled() }}, which meansPublish deployed graph and statusis lost — removing the Deployed tab exactly when a deploy hung and you most need to see which resource broke. A job-level bound would also fold the setup steps into the same budget, so the time actually available to the deploy would vary with how long setup took.A step-level timeout produces a failed step instead, so publish and teardown both run normally on the job's remaining budget.
Why the override is validated
A
Resolve deploy timeoutstep validatesRADIUS_DEPLOY_TIMEOUT_MINUTESbefore it reachestimeout-minutes. This is not defensive boilerplate — perStepsRunner.cs:An unchecked
0, negative, or malformed value would therefore silently restore the unbounded behavior this PR exists to prevent. Values are constrained to 1–330 and normalized to base 10 (so030isn't read as octal and then rejected byfromJSON). The ceiling is exactly that — a ceiling, not a guarantee: setup steps draw on the same 360-minute budget, so a value near 330 can still leave the job short for teardown. That specific number is a judgment call, happy to adjust.Server-side: a timeout on the containerImages build
@sk593 asked that the Radius deployment itself be bounded, not just the workflow. That was right, and the layering turned out to be worse than "no timeout".
Timeouts that exist today, innermost to outermost:
pkg/dynamicrp/frontend/routes.go:68)rad deployclient waitThe layers are inverted: dynamic-rp's 24h sits outside the deployment engine's 4h, so no Radius-owned timeout can ever fire. Every hang is bounded by the deployment engine, in a different repo, at 4 hours. (Both DE values are get-only expression-bodied C# properties, so neither is configurable.)
This adds the innermost bound: a 3-hour budget derived at the top of
executeImageBuild, so it covers retrieving registry credentials from the target cluster as well as the build script itself. 3h rather than something aggressive because the observed pathological build ran 1–2 hours — a 30-minute server-side default would fail it outright. The workflow's 30m already handles the CI-cost question; this one exists to stop a build occupying a deployment indefinitely, and sits under the DE's 4h so the failure is reported by Radius.It is hardcoded, not configurable.
deploy/Chart/templates/dynamic-rp/configmaps.yaml:64-66hardcodes thebicep:block, so a Go-onlyBicepOptionsfield would be a knob no Helm user could actually set.A deadline alone would have failed open
runScriptdrains stdout and stderr before callingcmd.Wait(), and those drains end only at EOF. EOF needs every write end of the pipe closed. Killing the process group cannot reach a descendant that left it viasetsidwhile holding the inherited pipes — so the drains never return, and the deadline never gets to do anything.cmd.WaitDelaydoes not help, because it only takes effect insideWait.This is the same fail-open class this PR already hit once: a
timeout-minutesthat evaluates to empty is silently ignored by the runner. So it was measured rather than assumed. With the forced close disabled, the new regression test blocks for the script's full 60 seconds; with it, 0.7s.The fix closes the pipe read ends after a grace period once the context is done. Closing an
*os.Filewhile another goroutine is blocked reading it is safe (poll.FD refcounting) — the pending read returnsfs.ErrClosed, which is filtered per-stream so a genuine failure on the other stream is still reported.Error attribution
The async worker cancels operations via
opCancel()rather than letting a deadline expire, so it surfaces ascontext.Canceled, notDeadlineExceeded— meaningDeadlineExceededalone cannot distinguish "we stopped this build" from "the caller went away". The budget therefore usescontext.WithTimeoutCausewith a private sentinel, and the message reports elapsed time and the limit. Note the sentinel is internal:recipes.NewRecipeErrorflattens toerr.Error()and definesIsas type identity, so what reaches the user is the message, not a typed error. Deliberately not changing that shared type here.Scope of the server-side timeout
Bounds the containerImages post-deployment build hook only. Not covered: recipe OCI retrieval (
bicep.go:99-115) and the nested Bicep deployment poll (bicep.go:170-190). On Windowscontainerimages_windows.gois a no-op stub, so a timeout there kills only the shell, not the process tree — pre-existing, not addressed here.Tests
go test -raceon Linux, run 3× for flakiness. New cases: forced close unblocks a drain held by an escaped descendant (verified to hang without the fix); the build timeout fires; an inherited cancellation is not misreported as the build timeout; a build inside the budget still succeeds. The regression test waits for the detached descendant to actually be holding the pipes before cancelling, so it cannot pass vacuously, and reaps that PID on cleanup. POSIX-only paths skip on Windows and wheresetsidis unavailable.Scope and limits (workflow layer)
RADIUS_DEPLOY_TIMEOUT_MINUTESis the escape hatch. If you reach for it, first check whether the time is going into emulated cross-arch builds — raising the timeout hides that cost rather than removing it.rad-commands-resultartifact is not guaranteed on timeout, since the composite action's ownif: always()upload shares the deadline. Teardown still collects and uploads Radius logs outside the timed step.succeeded. Measured on a real runner: the composite'strap write_result EXITdoes fire when the step is killed, and writes the optimisticOVERALL_OUTCOME="succeeded"seed that is never re-affirmed on success. This is pre-existing, but this PR makes the path routine. The fix belongs in the shared composite action (where it also reaches already-generated repos via the pinned ref), so it is tracked separately in Deploy workflow publishessucceededwhenrad deployis killed by a step timeout #12756 rather than widening this PR.delete-azure.yml/delete-aws.ymlhave their own long-running steps and remain unbounded — deliberately out of scope.Testing (workflow layer)
New
deploy-timeout_test.sh, wired in asmake test-deploy-timeoutand included in the aggregatetesttarget that CI runs.The assertions are structural rather than whole-file greps, because
timeout-minutesnested underwith:is valid YAML that GitHub silently treats as an undeclared action input — a grep for the key would pass on exactly the mistake worth catching. Indentation is derived from the file, and step ordering plus the teardown'sif: always()are pinned.Verified by mutation testing — 12 ways of breaking the wiring, each caught:
timeout-minutesnested underwith:vars)0if: always()id:removed$GITHUB_OUTPUTwrite deletedAlso: validation logic exercised across
"",0,-5,030,abc,45m,330,331,9999; both templates parsed structurally to confirm placement;shellcheckandmarkdownlintclean.Verified on a real runner
The static analysis above proves the wiring is present; it does not prove the runner honors it. So the behavior was measured on
ubuntu-latestwithRADIUS_DEPLOY_TIMEOUT_MINUTESset as a real repository variable, using theResolve deploy timeoutstep copied verbatim from this PR.1, deploy sleeps 300s0(validation active)timeout-minutes: 0(validation bypassed)fromJSON('')success✅ silent!cancelled()publish skipped,always()teardown ran fully (240s)Two things this changed in the PR. The fail-open behavior is real and silent — a deleted output write yields a green build with an unbounded step, which is why validation exists and why the test now asserts the exact
GITHUB_OUTPUTwrite. And the last row corrected the rationale above: thealways()teardown survives a job-level timeout, contrary to what this description originally claimed.Follow-ups not included here
An earlier version of this description said scope was kept to the workflow. That is now superseded — the server-side build timeout above is in this PR. Still outstanding, verified in code and not addressed by either timeout:
pkg/cli/deployment/deploy.go:198callsPollUntilDonewith no deadline, andrad deployhas no--timeoutflag. A future flag would need registering on bothrad deployandrad run.pkg/dynamicrp/frontend/routes.go:68hardcodesAsyncOperationTimeout: 24h, versus 20m in corerp and a 120s armrpc default. Lowering it is what would fix the inversion in the table above for every dynamic-rp operation, not just container builds — worth doing, but a broader blast radius than this PR.Happy to open issues for these if useful.