fix(ci): recover hosted runner loss once#7530
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a guarded hosted-runner recovery workflow, shared runner-loss evidence modules, stable-snapshot rerun orchestration, PR gate integration, and extensive classifier, controller, workflow, and trigger tests. ChangesHosted-runner recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant SourceWorkflow
participant HostedRunnerRecoveryWorkflow
participant RecoveryController
participant GitHubActionsAPI
SourceWorkflow->>HostedRunnerRecoveryWorkflow: completed workflow_run event
HostedRunnerRecoveryWorkflow->>RecoveryController: run with SOURCE_RUN_ID
RecoveryController->>GitHubActionsAPI: collect and verify runner-loss evidence
RecoveryController->>GitHubActionsAPI: compare repeated snapshots
RecoveryController->>GitHubActionsAPI: request full source rerun
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / low confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
The refreshed branch-specific guard is green: The remaining I am keeping that policy/package repair out of this runner-recovery PR. Once #7531 lands, I will refresh this PR against the repaired base and let the normal CI run validate the combined merge revision. |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Independent security review — PASSReviewed exact head
Residual non-blocking risks are limited to the GitHub API lacking compare-and-set for a trusted manual rerun race after the final snapshot, fail-closed availability if GitHub schemas or canonical markers drift, and floating GitHub-managed Validation: exact-head recovery/security contracts pass 106/106 locally; an independent dependency-light pass completed 150 additional tests. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
tools/e2e/hosted-runner-loss.mts (1)
591-599: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueEvaluate the trusted-marker predicate once per job.
hasTrustedMarkerruns twice for every job (filterthensome), so shutdown-log parsing over the 64 KiB tail plus all annotation/check matching is repeated. Compute once and derive both values.♻️ Proposed refactor
- const runnerLostMarkerCount = options.jobs.filter(hasTrustedMarker).length; - const otherNonPassingEvidencePresent = options.jobs.some((job) => !hasTrustedMarker(job)); + const trustedMarkers = options.jobs.map(hasTrustedMarker); + const runnerLostMarkerCount = trustedMarkers.filter(Boolean).length; + const otherNonPassingEvidencePresent = runnerLostMarkerCount !== options.jobs.length; return { terminalClassificationPresent: otherNonPassingEvidencePresent,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/e2e/hosted-runner-loss.mts` around lines 591 - 599, Update the classification logic around hasTrustedMarker to evaluate the predicate once for each job, cache the resulting booleans, and derive both runnerLostMarkerCount and otherNonPassingEvidencePresent from that cached collection. Preserve the existing terminalClassificationPresent and jobConclusion behavior.test/hosted-runner-recovery.test.ts (2)
731-755: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd coverage for the repository and token trust guards.
recoverHostedRunnerLossrejects any repository other thanNVIDIA/NemoClawand an empty token (hosted-runner-recovery.mtsLines 456-459), but no test pins that behavior. These are the outermost trust-boundary checks for anactions: writecontroller, so a regression there would be silent.💚 Suggested tests
it("refuses to recover outside the trusted repository (`#7140`)", async () => { const requests = setupRoutes(); await expect( recoverHostedRunnerLoss({ ...recoveryRequest(), repository: "attacker/NemoClaw" }), ).rejects.toThrow(/restricted to NVIDIA\/NemoClaw/u); expect(requests).toEqual([]); }); it("refuses to recover without a token (`#7140`)", async () => { const requests = setupRoutes(); await expect(recoverHostedRunnerLoss({ ...recoveryRequest(), token: "" })).rejects.toThrow( /GitHub token is required/u, ); expect(requests).toEqual([]); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/hosted-runner-recovery.test.ts` around lines 731 - 755, Add tests alongside the existing recoverHostedRunnerLoss guard and mutation tests for the repository and token trust checks. Verify a non-NVIDIA/NemoClaw repository rejects with the restricted-repository error and an empty token rejects with the required-token error; assert both cases produce no requests.
253-276: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFixture-integrity
expect()calls inside route handlers can be swallowed.These assertions run inside the code under test's async call stack; production
try/catchblocks (e.g. the shutdown-log path inhosted-runner-loss-github.mts) can absorb them and turn a broken fixture into a passing-but-meaningless run. Throwing a plainErrorhere does not fix the swallow, but at least keeps the failure textually distinguishable from a Vitest assertion. Prefer asserting fixture completeness beforesetupRoutesreturns.As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/hosted-runner-recovery.test.ts` around lines 253 - 276, Move the fixture-completeness assertions for check-run and annotation jobs out of the githubFetchRoute handlers and perform them before setupRoutes returns, so production error handling cannot swallow them. Validate that every referenced job ID exists in lastJobs during route setup, then let handlers perform only lookups and responses without Vitest expect calls.Source: Path instructions
tools/e2e/hosted-runner-loss-github.mts (2)
554-561: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the swallowed error in the warning.
This catch disables recovery silently; without the cause, host-pattern/schema drift is undiagnosable from CI logs.
♻️ Suggested change
- } catch { + } catch (error) { console.warn( - `Could not authenticate hosted-runner shutdown log for job ${job.id}; automatic retry remains disabled`, + `Could not authenticate hosted-runner shutdown log for job ${job.id}; automatic retry remains disabled: ${ + error instanceof Error ? error.message : String(error) + }`, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/e2e/hosted-runner-loss-github.mts` around lines 554 - 561, Update the catch around downloadWorkflowJobLogTail in the hosted-runner job handling flow to bind the thrown error and include its details in the console.warn message, while preserving the existing job ID context and retry-disabled behavior.
531-533: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the non-passing predicate.
The
["success","skipped","neutral"]filter is duplicated at Lines 531-533 and 572; divergence here silently changes what counts as evidence.♻️ Suggested extraction
+const PASSING_CONCLUSIONS = new Set(["success", "skipped", "neutral"]); +const isNonPassing = (job: WorkflowJob): boolean => !PASSING_CONCLUSIONS.has(job.conclusion ?? "");Also applies to: 571-574
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/e2e/hosted-runner-loss-github.mts` around lines 531 - 533, Extract the shared non-passing job predicate from the jobs filtering logic into a reusable local helper or constant, then use it in both the filters around the current nonPassingJobs calculation and the later duplicate block. Preserve the existing success, skipped, and neutral exclusions so both checks remain consistent.tools/e2e/hosted-runner-recovery.mts (2)
336-343: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDeep/stale source runs throw instead of being ignored.
If the source run has scrolled past
MAX_WORKFLOW_RUN_PAGES(1000 runs) or its listing page is short before it appears, the controller exits non-zero rather than reporting a benign "not eligible". That is fail-closed for mutation, but it turns an expected steady-state condition into a red recovery job. Consider distinguishing "source older than the searched window" (ignore) from genuine listing inconsistencies (throw).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/e2e/hosted-runner-recovery.mts` around lines 336 - 343, The workflow run lookup should treat a source run beyond the searched window, including short pages encountered before it appears, as not eligible rather than throwing. Update the recovery logic around the page-limit and incomplete-listing checks to return the existing benign skip/ignore outcome for stale runs, while preserving errors for genuine listing inconsistencies such as exhausting all runs without finding the source.
371-380: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass
source.runAttemptinstead of the literal1.Eligibility already pins
runAttempt === 1(Line 258), so the literal is correct today but silently decouples the two if that rule ever relaxes.♻️ Suggested change
- options.sourceRunId, - 1, + options.sourceRunId, + source.runAttempt,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/e2e/hosted-runner-recovery.mts` around lines 371 - 380, Update the listNonPassingWorkflowJobs call in the recovery flow to pass source.runAttempt instead of the hardcoded 1, preserving the existing argument order and hostedRunnerLossPolicy options.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/e2e/hosted-runner-loss-github.mts`:
- Around line 156-161: Update the annotation validation around the nullable text
fields title and raw_details to accept null values and treat them as empty
strings before checking byte length. Preserve rejection of non-string, non-null
values and ensure listing continues without aborting recovery when either field
is null.
---
Nitpick comments:
In `@test/hosted-runner-recovery.test.ts`:
- Around line 731-755: Add tests alongside the existing recoverHostedRunnerLoss
guard and mutation tests for the repository and token trust checks. Verify a
non-NVIDIA/NemoClaw repository rejects with the restricted-repository error and
an empty token rejects with the required-token error; assert both cases produce
no requests.
- Around line 253-276: Move the fixture-completeness assertions for check-run
and annotation jobs out of the githubFetchRoute handlers and perform them before
setupRoutes returns, so production error handling cannot swallow them. Validate
that every referenced job ID exists in lastJobs during route setup, then let
handlers perform only lookups and responses without Vitest expect calls.
In `@tools/e2e/hosted-runner-loss-github.mts`:
- Around line 554-561: Update the catch around downloadWorkflowJobLogTail in the
hosted-runner job handling flow to bind the thrown error and include its details
in the console.warn message, while preserving the existing job ID context and
retry-disabled behavior.
- Around line 531-533: Extract the shared non-passing job predicate from the
jobs filtering logic into a reusable local helper or constant, then use it in
both the filters around the current nonPassingJobs calculation and the later
duplicate block. Preserve the existing success, skipped, and neutral exclusions
so both checks remain consistent.
In `@tools/e2e/hosted-runner-loss.mts`:
- Around line 591-599: Update the classification logic around hasTrustedMarker
to evaluate the predicate once for each job, cache the resulting booleans, and
derive both runnerLostMarkerCount and otherNonPassingEvidencePresent from that
cached collection. Preserve the existing terminalClassificationPresent and
jobConclusion behavior.
In `@tools/e2e/hosted-runner-recovery.mts`:
- Around line 336-343: The workflow run lookup should treat a source run beyond
the searched window, including short pages encountered before it appears, as not
eligible rather than throwing. Update the recovery logic around the page-limit
and incomplete-listing checks to return the existing benign skip/ignore outcome
for stale runs, while preserving errors for genuine listing inconsistencies such
as exhausting all runs without finding the source.
- Around line 371-380: Update the listNonPassingWorkflowJobs call in the
recovery flow to pass source.runAttempt instead of the hardcoded 1, preserving
the existing argument order and hostedRunnerLossPolicy options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f00a94a3-dcda-4489-8a31-b0fe1b10efe1
📒 Files selected for processing (15)
.github/workflows/hosted-runner-recovery.yamlci/source-shape-test-budget.jsontest/e2e/README.mdtest/helpers/e2e-workflow-contract.tstest/helpers/vitest-watch-triggers.tstest/hosted-runner-loss-internal-error.test.tstest/hosted-runner-recovery-workflow.test.tstest/hosted-runner-recovery.test.tstest/pr-e2e-gate-runner-loss-classifier.test.tstest/pr-e2e-gate-runner-loss-retry.test.tstest/vitest-watch-triggers.test.tstools/e2e/hosted-runner-loss-github.mtstools/e2e/hosted-runner-loss.mtstools/e2e/hosted-runner-recovery.mtstools/e2e/pr-e2e-gate.mts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary <!-- 1-3 plain sentences: what changes and why. Describe before-and-after behavior when it applies. Follow the NemoClaw Writing Guide: https://github.com/NVIDIA/NemoClaw/blob/main/WRITING.md. Do not add unrelated prose cleanup. --> Prevent a transient registry or transport failure while pulling a published base image from silently cascading into the memory- and disk-heavy local Hermes build. Positively classified transient pulls receive at most three attempts; exhaustion stops with a distinct temporary-failure status, while deterministic candidate absence and invalid-image failures retain their existing behavior. ## Related Issue <!-- Fixes #NNN or Closes #NNN. Remove this section if none. --> Part of #7144 and parent epic #7140. ## Changes <!-- List concrete changes. If this adds an abstraction, configuration, fallback, migration, or compatibility path, name its current requirement and consumer, explain why a direct change is insufficient, and identify the test that protects it. --> - Retry an allowlisted transient Docker pull failure at most three times with fixed one- and two-second backoff. - Keep this command-level registry/transport recovery separate from #7146/#7530 whole-workflow hosted-runner-loss recovery. - Refuse retries for manifest or repository absence, authentication and authorization failures, non-429 HTTP 4xx responses, digest or integrity failures, invalid references, platform mismatch, certificate validation failures, and unknown diagnostics. - Drain the full Docker stderr stream while retaining only its final 64 KiB in a private temporary directory; discard an ambiguous leading tail record and fail closed when truncated output cannot be classified. - Sanitize retained diagnostics before writing them to GitHub logs, including URL credentials, arbitrarily long query keys, authentication headers and folded values, cookies, known token forms, CR records, ANSI CSI/OSC sequences, invalid bytes, and log-command injection. - Exit with `EX_TEMPFAIL` 75 after transient exhaustion so the current resolver callers cannot reinterpret the failure as a missing candidate and launch a local Hermes build. - Cover transient recovery and exhaustion, deterministic single-attempt behavior, adversarial diagnostic redaction, bounded capture, and the no-local-build caller contract. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes internal CI base-image resolution and failure classification; it does not change CLI, configuration, API, policy, or supported runtime behavior. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent nine-category security review passed exact head `4edb0c05d` with no findings. It covered classification precedence, status preservation, credential and log-command redaction, bounded memory/disk use, FIFO cleanup/cancellation, fail-closed truncation and collector failures, and the no-local-build exhaustion contract. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review <!-- Required for code and documentation changes after the changes and applicable validation are complete. Keep one review checkbox and one instance of each visible or hidden field. For Evidence, list changed documentation paths. For documentation-only changes, also state that the writing rules and documentation style were reviewed. For other results, explain why no documentation change is needed or why the review is blocked. For Agent, use a consistent product and surface name, such as Codex Desktop, Codex CLI, Claude Code, or Cursor. After committing all review changes, put `git rev-parse --short HEAD` and `git rev-parse --short HEAD:AGENTS.md` in the hidden metadata below. Rerun the review and refresh that metadata after any new commit. This receipt is advisory during the data-collection pilot. --> - [x] Documentation writer subagent reviewed the completed changes - Result: `no-docs-needed` - Evidence: The exact three-commit diff changes only `.github/actions/base-image-resolver.sh` and `test/base-image-resolver-helper.test.ts`. It changes internal GitHub Actions failure handling and does not alter the CLI, configuration, API, sandbox runtime, or a user/operator workflow. - Agent: Codex Desktop <!-- docs-review-head-sha: 4edb0c0 --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence <!-- Required only when scripts/prepare-dgx-station-host.sh changes. Maintainers must review the linked evidence before approving or merging. This is human-reviewed evidence, not authenticated hardware provenance. Exceptional bypasses use existing repository governance and must be documented on the PR. --> - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project integration test/base-image-resolver-helper.test.ts`: 61/61 passed; shfmt, ShellCheck, Biome, Bash syntax, and `git diff --check` passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable; the change is isolated to one shared resolver helper and its caller-contract tests. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved container image pull recovery with automatic retries for temporary registry failures. * Prevented retries for permanent failures and avoided unsafe local fallback builds when diagnostics are incomplete. * Improved error messages by limiting output and removing sensitive credentials and control characters. * Ensured temporary diagnostic files are cleaned up after failed or interrupted pulls. * **Tests** * Added coverage for retry behavior, failure classification, redaction, truncation, cleanup, and fallback prevention. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Summary
Add a trusted-main recovery controller that reruns an eligible workflow once only when fail-closed evidence proves the latest first attempt lost a GitHub-hosted runner. Real test failures, mixed failures, superseded runs, manual reruns, and already-recovered runs remain ineligible.
Related Issue
Part of #7146 and parent epic #7140.
Changes
actions: writecontroller that performs one full workflow rerun, preserving job dependencies and refusing mixed or ordinary failures.Type of Change
Quality Gates
beedbfd922c91f2e360bb7b2423d97a7bb9320a8; trusted source identity, least privilege, double evidence fingerprints, mixed-failure rejection, nullable annotation normalization, token isolation, and the one-rerun ceiling were verified with no findings.Documentation Writer Review
docs-updatedtest/e2e/README.mddocuments the internal recovery-controller contract, trust boundary, operator diagnostics, null-or-empty annotation fields, and 30-day retirement/reset condition; no end-userdocs/page is needed.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run build:cli,npm run typecheck:cli, and the title/project/source-shape/test-size contracts passed. After the current-main sync, six focused recovery and controller files passed 206 tests; merge, commit, and push hooks passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not applicable; this is an isolated trusted-workflow controller covered by its full affected import surface and diff-scoped hooks.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
null/empty runner-loss annotation title/details.