feat(e2e): classify hosted-runner resource pressure and infrastructure loss - #7168
Conversation
Host-memory snapshots that only read raw MemFree make healthy Linux page cache look like exhaustion, and a hosted VM can disappear before logs identify the cause, encouraging broad retries that hide deterministic regressions. Add the trusted evidence contract that issue NVIDIA#7146 owns: - tools/e2e/runner-pressure-core.mts: pure parsers for /proc/meminfo, loadavg, cgroup v2 scalars and memory.events, PSI, ps (comm and RSS only — never argv), docker stats, and docker system df; a bounded secret-safe E2E_RESOURCE_SNAPSHOT line built by an explicit field-by-field allowlist serializer that drops lists before scalars when over budget; a machine-readable terminal classification (assertion, timeout, process-oom, container-oom, disk-pressure, unknown) driven by positive evidence only, so low raw MemFree alone is never OOM; a hosted-runner-loss signature that requires the attempt to have produced no terminal classification; and a retry policy allowing at most one retry, only for confirmed runner loss. - tools/e2e/runner-pressure.mts: fail-closed CLI (snapshot, classify, decide-retry) with validated env inputs and best-effort collectors, so the snapshot line still emits on hosts without cgroup v2 or Docker. - test/e2e/support/runner-pressure.test.ts: covers each parser, the allowlist and length bound under adversarial input, every terminal classification including the acceptance criteria (assertion gets zero retries; runner loss retries exactly once and links attempts; ambiguous stays unknown), and the CLI through real subprocesses including the unsupported-subcommand guard. The snapshot and classification lines are single prefixed records a phase-heartbeat stream (NVIDIA#7101) can carry verbatim; this change defines no second progress framework and no workflow wiring yet. Refs NVIDIA#7146 Signed-off-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds hosted-runner resource telemetry, bounded evidence artifacts, terminal failure classification, runner-loss retry decisions, trusted live-test outcomes, workflow validation, and PR gate retry lineage reporting. ChangesRunner pressure handling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant E2EWorkflow
participant RebuildHermesProgress
participant RunnerPressureCLI
participant RunnerPressureCore
participant PRGate
E2EWorkflow->>RunnerPressureCLI: capture snapshot and baseline
RebuildHermesProgress->>RunnerPressureCLI: record phase baseline
E2EWorkflow->>RunnerPressureCLI: classify failed test
RunnerPressureCLI->>RunnerPressureCore: classify evidence and decide retry
RunnerPressureCore-->>PRGate: retry decision
PRGate->>PRGate: link prior runner-loss attempts
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 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 / high 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: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
|
@cv can i get the ci now |
|
@apurvvkumaria hey can u take a look over the fix |
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head 7f1d8fff04943f1beffa07b340e33b01af9b33e8. The focused suite passes 36/36, but the current change does not yet satisfy #7146's functional and secret-safety boundaries:
-
The implementation is inert. No canonical workflow, heartbeat, reporter, or gate invokes
runner-pressure.mts, consumes either evidence prefix, rejects missing/malformed terminal evidence, or links a single runner-loss retry. #7146 explicitly requires representative heavy lanes to emit this evidence and the retry to preserve/link both attempts. Please wire the smallest canonical path and add a workflow-boundary test. -
The claimed secret-safe serializer emits workload-controlled
topProcesses[].commandcontainers[].nameverbatim. Both can contain credential material.snapshot.atis also copied without shape/length validation, sorenderSnapshotLine()can leak arbitrary input and exceed its stated bound. Remove workload-controlled identifiers from logged evidence (numeric PID/container rank or another non-secret identity is safer), validate the timestamp as canonical bounded data, and add adversarial token-shaped tests. -
decideRetry({ runnerLoss: true, classification: "assertion", attempt: 1 })currently returnsretry: true, contradicting the invariant that classified assertions/OOM/disk/timeouts are never retried. Fail closed whenever a terminal classification is present, even if the runner-loss flag is also true, and cover contradictory inputs. -
OOM counters and kernel-log matches are treated as absolute booleans/counters rather than deltas from a pre-phase baseline, so an earlier OOM can misclassify an unrelated later failure. Also,
parseDockerStatsstops at the first five rows instead of selecting the largest consumers. Capture/compare baselines and sort before limiting so the attribution is actually phase-specific and reports the largest consumers required by #7146.
Please keep the integration on the existing #7101 heartbeat/control-plane path rather than creating a parallel retry loop.
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/support/runner-pressure.test.ts`:
- Around line 599-609: Update the runHelper invocation in the strict pre-phase
baseline test to explicitly unset or override DOCKER_OOM_CONTAINER in the
spawned environment, while preserving the existing E2E_PHASE value and
deterministic containerOomKilled: false assertion.
🪄 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: a1e0e6da-cb98-47ac-9bb9-88ccf2566129
📒 Files selected for processing (13)
.github/workflows/e2e.yamltest/e2e/live/rebuild-hermes-progress.tstest/e2e/support/rebuild-hermes-progress.test.tstest/e2e/support/runner-pressure-workflow-boundary.test.tstest/e2e/support/runner-pressure.test.tstest/helpers/vitest-watch-triggers.tstest/pr-e2e-gate-lifecycle.test.tstest/vitest-watch-triggers.test.tstools/e2e/pr-e2e-gate.mtstools/e2e/runner-pressure-core.mtstools/e2e/runner-pressure-workflow-boundary.mtstools/e2e/runner-pressure.mtstools/e2e/workflow-boundary.mts
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e/support/live-test-outcome-invocation.test.ts`:
- Around line 46-84: Add a per-call timeout below the 30-second Vitest limit to
both the `spawnSync` invocation running Vitest and the one running `CLASSIFIER`,
preserving their existing options and ensuring hung child processes fail fast.
🪄 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: c47195da-3635-4727-973e-7010b4a4bad0
📒 Files selected for processing (10)
.github/workflows/e2e.yamltest/e2e-risk-signal-reporter.test.tstest/e2e/risk-signal-reporter.tstest/e2e/support/fixtures/live-test-outcome.fixture.test.tstest/e2e/support/live-test-outcome-invocation.test.tstest/e2e/support/runner-pressure-workflow-boundary.test.tstest/e2e/support/runner-pressure.test.tstools/e2e/live-test-outcome.mtstools/e2e/runner-pressure-workflow-boundary.mtstools/e2e/runner-pressure.mts
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/e2e.yaml
- test/e2e/support/runner-pressure.test.ts
- tools/e2e/runner-pressure-workflow-boundary.mts
- tools/e2e/runner-pressure.mts
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
|
Addressed exact-head advisor blocker PRA-1 in |
Create and consume runner evidence through no-follow, single-link descriptors. Reject symlink and hardlink redirection during PR-controlled live tests. Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Superseded by exact head a5209a3. The current revision wires the canonical heartbeat/gate path, removes workload-controlled identifiers, validates timestamps, fails closed on contradictory retry evidence, uses phase-local OOM deltas and sorted consumers, and now protects evidence files against link substitution. Focused tests and CLI type-checking pass.
Select link creators from a fixed map so the security fixture adds no test conditional. Co-authored-by: Souvik Ghosh <138186578+souvikDevloper@users.noreply.github.com> Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Reviewed exact head 04e7e87 against current base 78e0e4e. The prior integration, secret-safety, fail-closed retry, phase-baseline, and evidence-file blockers are addressed: snapshots expose only bounded numeric/rank data, terminal classifications override runner-loss claims, OOM attribution uses monotonic phase deltas, runner loss requires a trusted hosted-runner marker and receives at most one linked retry, and evidence files reject symlink/hardlink substitution. Current-head CI, security scans, advisors, protected E2E, and all review threads are green. I also re-audited the workflow wiring and controller retry history at this revision; no blocking issue found.
wscurran
added
area: ci
4 participants
Summary
Maintainers need to distinguish deterministic E2E failures from process/container OOM kills, disk exhaustion, and hosted-runner loss without exposing workload-controlled data. This PR implements the #7146 runner-pressure evidence and retry contract on the existing #7101 Hermes phase-heartbeat path.
Fixes #7146
Changes
rebuild-hermesandrebuild-hermes-stale-baseto produce, consume, validate, and upload terminal evidence.Verification
Verifiedby GitHub.No user-facing command or configuration behavior changes; contributor documentation is not affected.
Signed-off-by: Souvik Ghosh 138186578+souvikDevloper@users.noreply.github.com
Summary by CodeRabbit
New Features
runner-pressureCLI and workflow boundary validation with fail-closed behavior.Tests