ci(e2e): stop waiting for managed image completion - #9556
Conversation
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit f73419f in the TypeScript / code-coverage/cliThe overall coverage in commit f73419f in the Show a code coverage summary of the most impacted files.
Updated |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe publication gate now selects matching workflow runs without waiting for overall completion. It validates required publisher jobs from the selected run attempt, polls until they are ready, and ignores unrelated downstream job completion. ChangesBase publication gate
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The change shortens the base-image publication wait while retaining SHA, attempt, publisher, and artifact validation. No actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant E2EGate
participant WorkflowRun
participant PublisherJobs
E2EGate->>WorkflowRun: select matching publication run
E2EGate->>PublisherJobs: retrieve jobs for selected attempt
PublisherJobs-->>E2EGate: return required publisher statuses
E2EGate->>WorkflowRun: verify bound run after publishers are ready
E2EGate-->>E2EGate: release E2E fanout
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
jyaunches
left a comment
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
The implementation loads every job from every run attempt and then creates a second attempt-selection authority inside validatePublisherJobs. At tools/e2e/base-image-publication.mts:450, it stores arrays of occurrences per publisher, carries each occurrence’s attempt, detects duplicates per attempt, and later searches those arrays for the selected attempt. This machinery exists only because waitForBaseImagePublication requests /actions/runs/{id}/jobs?filter=all at line 640.
GitHub already provides an attempt-scoped jobs endpoint, and this repository already uses it in tools/e2e/native-runtime-qualification-collector.mts and tools/e2e/hosted-runner-loss-github.mts. Asking for the selected attempt directly preserves the required trust boundary without maintaining a parallel in-memory history registry. It also prevents irrelevant prior-attempt job records from participating in this gate.
Refactor direction
Request /actions/runs/${run.id}/attempts/${run.attempt}/jobs?per_page=100, following the existing qualification pattern.
Then simplify validatePublisherJobs to retain at most one current-attempt record per required publisher name. Require each retained job’s run_id, run_attempt, and head_sha to match the selected run, reject duplicate required names, and classify only those three jobs as pending, successful, or failed. Keep the final bound-run recheck and exact artifact validation unchanged.
Expected result
The #9549 readiness behavior and fail-closed attempt binding remain intact, while the all-attempt registry, occurrence arrays, attempt searches, and associated fixtures disappear. The verifier has one attempt authority—the selected GitHub run attempt—and the production change becomes smaller.
senthilr-nv
added
chore
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Pushed signed follow-up The verifier now requests GitHub's attempt-scoped jobs endpoint for the selected run attempt. The follow-up removes the all-attempt occurrence registry (20 additions / 45 deletions). Validation:
Fresh CI/advisor checks are now running on the exact new head. |
Resolved by 1b9748b: the verifier now uses the attempt-scoped jobs endpoint and removes the all-attempt occurrence registry.
jyaunches
left a comment
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Resolved
The attempt-selection blocker is resolved at 1b9748bfcbbe3494d687107b4b552aa17e27f458.
The verifier now requests the selected run's attempt-scoped jobs endpoint. validatePublisherJobs retains only one current-attempt record per required publisher and preserves the exact run ID, run attempt, head SHA, duplicate-name, pending, and failure checks. The all-attempt occurrence registry and its search machinery are gone.
The follow-up adds 20 lines and removes 45, for a net reduction of 25 lines. The final bound-run recheck and exact artifact validation remain unchanged, and I found no replacement LOC or codebase-simplicity blocker.
This resolves only the prior LOC reduction and codebase-simplicity finding. It is not an approval or a judgment on other review dimensions.
|
Outside contributor here, not a maintainer — no decision implied. The only human review on this thread was scoped to LOC reduction and explicitly did not judge other dimensions, so I spent an hour on the one property that seemed worth an independent pair of eyes: whether dropping whole-run completion as evidence leaves the gate fail-closed. As far as I can tell it does. Details below in case they save someone else the trace. Fail-closed matrixI transcribed
Every accept path requires all three required jobs The The question the refactor raises, and why I think it is fine
Tracing it through, that race only ever costs a poll interval, never correctness:
So the worst case is one extra 30s poll before the accurate error, not an acceptance. Worth stating somewhere, because "why is it safe to judge terminality from an earlier response?" is the first thing a later reader will ask. Small noteThree of the throw sites this change introduces have no assertion behind them: What I did not checkI did not run the test suite, type-check, build, or trigger any workflow, and I have no way to exercise the real GitHub endpoints. The matrix above comes from a hand-transcribed copy of the function executed under plain Nice result on the 26-minute saving — the attempt-scoped endpoint reads much more clearly than the all-attempts registry it replaces. |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
7 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Approved exact head f73419f9ef08ea8f10d1660702ecf15c201f320a. The gate now selects the matching base-image publication run without waiting for unrelated downstream completion, while validating required publisher jobs against the selected run attempt. The attempt-scoped jobs endpoint and current-attempt registry close the prior-attempt ambiguity. All commits are GitHub Verified, all review threads are resolved, CodeRabbit and both advisor lanes are clean, and the complete current check suite is green after the unrelated installer test rerun passed.
5 participants
Summary
The base image publication gate previously waited for overall workflow completion even though it consumed only base publisher evidence. It now verifies the three current-attempt publishers as soon as they finish, removing a measured median 26m 35s downstream wait while preserving exact SHA, attempt, and artifact checks.
Related Issue
Fixes #9549
Changes
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passednpx vitest run --project e2e-support test/e2e/support/base-image-publication.test.ts test/e2e/support/base-image-publication-workflow-boundary.test.tspassed 87 tests.systemctlor GNUfindand could not pass on macOS.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Rebecca Sliter 571084+rsliter@users.noreply.github.com
Summary by CodeRabbit