Skip to content

ci(images): isolate Perl harness race#7519

Open
apurvvkumaria wants to merge 4 commits into
mainfrom
codex/base-perl-parallel-tests
Open

ci(images): isolate Perl harness race#7519
apurvvkumaria wants to merge 4 commits into
mainfrom
codex/base-perl-parallel-tests

Conversation

@apurvvkumaria

@apurvvkumaria apurvvkumaria commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

Keeps the parallel Perl test-harness speedup from #7518 while isolating the one upstream test that is unsafe when it overlaps other test processes. ExtUtils::Constant/t/Constant.t now runs alone first, and the remaining complete suite runs in parallel without running that file twice.

Related Issue

Related to #7140. Follow-up to #7518 and #7338.

Changes

  • Run ExtUtils::Constant/t/Constant.t serially before the parallel suite.
  • Exclude exactly that already-passed MANIFEST path from the parallel harness.
  • Preserve ci(images): parallelize Perl regression tests #7518's TEST_JOBS, PERL_TEST_HARNESS_ASAP, and parallel make behavior for the other 2,933 test files.
  • Strengthen the source-shape contract to require both passes, their order, the exact exclusion, and the guard against restoring serial make test.

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: This only changes build-time scheduling of the existing upstream Perl suite; published image contents and supported behavior are unchanged.
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent exact-diff review found no correctness or security blocker; both harness passes fail closed before install and packaging.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: The change is internal base-image test scheduling and does not alter image contents, CLI behavior, user workflows, security guarantees, or contributor commands.
  • Agent: Codex Desktop

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed, or npm run check:diff passed when hooks were skipped or unavailable
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — npx vitest run --project integration test/perl-critical-cve-remediation.test.ts passed 4 tests; the exact native-arm64 perl-builder target passed the isolated 302-test file, the remaining 2,933-file suite, install, package creation, and image build.
  • Applicable broad gate passed — not applicable; this is an isolated Dockerfile test-scheduling change validated by the full upstream Perl suite and its focused repository contract.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Apurv Kumaria akumaria@nvidia.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved Perl build reliability in parallel or emulated environments by executing a sensitive test as a dedicated serial step before running the remaining harness.
    • Added stricter validation to ensure the combined serial and parallel test selections match the full upstream set and include the sensitive test exactly once.
    • Hardened the build flow so packaging only proceeds after the full two-phase test process completes successfully.
  • Tests

    • Expanded the upstream test sequencing checks to verify ordering, correct test filtering, and proper inclusion/exclusion behavior across phases.

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 852a805e-3f1c-425c-9ec6-6427618d2c29

📥 Commits

Reviewing files that changed from the base of the PR and between c6161a8 and d98aa4f.

📒 Files selected for processing (2)
  • Dockerfile.base
  • test/perl-critical-cve-remediation.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • Dockerfile.base
  • test/perl-critical-cve-remediation.test.ts

📝 Walkthrough

Walkthrough

The Perl build now runs Constant.t serially before a filtered parallel upstream test pass. The remediation test verifies selection equivalence, execution ordering, installation, and packaging.

Changes

Perl test harness workflow

Layer / File(s) Summary
Two-phase Perl test execution
Dockerfile.base
Documents the QEMU workaround, validates split selections against the full suite, and runs Constant.t serially before the remaining tests in parallel.
Execution ordering validation
test/perl-critical-cve-remediation.test.ts
Checks enumeration, selection comparison, harness ordering, installation, packaging, split-removal guidance, and base-image build assertions.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant DockerBuild
  participant PerlHarness
  participant TestSelection
  DockerBuild->>PerlHarness: Enumerate full test suite
  DockerBuild->>TestSelection: Compare serial and filtered parallel selections
  DockerBuild->>PerlHarness: Run Constant.t serially
  DockerBuild->>PerlHarness: Run remaining tests in parallel
Loading

Possibly related PRs

  • NVIDIA/NemoClaw#7518: Updates the same Dockerfile and remediation test with the parallel Perl harness workflow refined here.

Suggested labels: area: sandbox, platform: container

Suggested reviewers: laitingsheng, senthilr-nv

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed Summarizes the main change: isolating the Perl harness race by splitting Constant.t out of the parallel test run.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/base-perl-parallel-tests

Comment @coderabbitai help to get the list of available commands.

@apurvvkumaria apurvvkumaria added the area: packaging Packages, images, registries, installers, or distribution label Jul 25, 2026
@apurvvkumaria apurvvkumaria added area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression security v0.0.96 Release target labels Jul 25, 2026
@github-code-quality

github-code-quality Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 90e0b3e in the codex/base-perl-para... branch remains at 96%, unchanged from commit dbac677 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 90e0b3e in the codex/base-perl-para... branch remains at 80%, unchanged from commit dbac677 in the main branch.

Show a code coverage summary of the most impacted files.
File main dbac677 codex/base-perl-para... 90e0b3e +/-
src/lib/state/m...-acquisition.ts 89% 84% -5%
src/lib/sandbox...rce-identity.ts 87% 87% 0%
src/lib/state/m...ock-identity.ts 95% 95% 0%
src/lib/state/m...lock-storage.ts 97% 97% 0%
src/lib/tunnel/services.ts 73% 73% 0%

Updated July 25, 2026 18:58 UTC

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — Informational

Advisor assessment: Informational / high confidence
Next action: Review the warnings below.
Findings: 0 blockers · 2 warnings · 0 suggestions
Status: Canonical ledger: 0 blocker(s), 2 warning(s), 0 suggestion(s).

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 2 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings differ; normalized E2E selections differ; Nemotron reported the same number of blockers, 2 fewer warnings, the same number of suggestions.

Nemotron output stays in workflow artifacts and does not change the assessment above.

Since last review: 0 prior items resolved · 0 still apply · 2 new items found

E2E guidance

Advisory only. E2E / PR Gate selects and runs jobs independently.

Recommended E2E: cloud-onboard

2 warnings · 0 suggestions

Warnings

Warnings do not block.

PRA-1 Warning — Document and reproduce the ExtUtils::Constant harness workaround

  • Location: Dockerfile.base:65
  • Category: correctness
  • Problem: The build isolates `ExtUtils::Constant` because it is asserted to produce an incomplete TAP plan during overlap, but the patch does not identify the upstream failure mechanism or provide a regression that demonstrates the split prevents that failure.
  • Impact: The serial exception can remain after its cause changes or disappears, while the selection check proves coverage only and cannot detect whether the claimed concurrency failure still exists.
  • Recommendation: Link the observed upstream harness failure or a minimal reproducible case, state why it cannot be fixed upstream in this PR, and add checked-in evidence that exercises the relevant concurrent path or records the upstream regression reference.
  • Verification: Inspect the Dockerfile comment and checked-in tests for an upstream issue, reproducer, or test that demonstrates the concurrent `Constant.t` failure rather than only the selected-test list.
  • Test coverage: Add a regression that runs the affected `ExtUtils::Constant` path with the relevant concurrent harness conditions and verifies a complete TAP plan, or reference a stable upstream regression test that proves the defect and its resolution.
  • Evidence: Dockerfile.base isolates `../cpan/ExtUtils-Constant/t/Constant.t` in a serial `make test_harness` pass and excludes it from the parallel pass. The added comparison checks that the full selection equals the serial-plus-parallel selection, but does not exercise or identify the asserted concurrent incomplete-TAP failure. The Dockerfile provides a removal condition but no source reference explaining why the creating upstream state cannot be fixed in this PR.

PRA-2 Warning — Execute the split Perl harness in build-level coverage

  • Location: Dockerfile.base:76
  • Category: tests
  • Problem: The changed Vitest test checks Dockerfile text and command ordering but does not build the Perl stage or observe the serial and parallel harness passes under a supported architecture.
  • Impact: A changed `harness --nre` interpretation, ignored harness argument, or architecture-specific build failure can leave the image without a verified split-harness execution path.
  • Recommendation: Add focused build-level coverage that builds the Perl stage using the split harness, verifies installation succeeds, and checks the resulting runtime's Perl version and existing CVE probes.
  • Verification: Inspect checked-in tests for a test that builds the changed Perl stage and validates the produced runtime, rather than only reading `Dockerfile.base` as text.
  • Test coverage: Build the Perl stage on a supported architecture with the split serial and parallel harness commands, then assert package installation, Perl 5.44.0, and all existing Perl CVE probes in the produced runtime.
  • Evidence: test/perl-critical-cve-remediation.test.ts reads Dockerfile.base with `fs.readFileSync` and asserts source substrings and ordering. Dockerfile.base changes the executed test path to a serial `Constant.t` harness pass followed by an excluded parallel pass. The risk plan identifies the platform-install invariant that a clean supported host installs pinned dependencies and reaches a usable agent.

Workflow run details

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>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

Addressed PRA-1 in signed commit c6161a8. The Perl builder now uses upstream t/harness -dumptests during the image build to compare the sorted full suite against the union of the serial and filtered-parallel selections, and asserts cpan/ExtUtils-Constant/t/Constant.t appears exactly once. The existing commands then run those same selections. I also documented the removal condition: remove the split only after the unsplit parallel harness passes in two consecutive amd64 and arm64 base-image builds. Local evidence: npx vitest run --project integration test/perl-critical-cve-remediation.test.ts (4/4 passed), plus all changed-file prek hooks passed, including hadolint. Fresh exact-head CI is now authoritative.

@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

Documentation writer review completed for exact head c6161a8db: no-docs-needed.

The net change only hardens internal build-stage Perl test scheduling and proves selection equivalence. It does not change the packaged runtime, CVE disposition, sandbox behavior, user commands, configuration, setup, migration, or support claims. The temporary-removal criterion belongs beside the Dockerfile workaround and is enforced by the contract test; existing release/security documentation remains accurate.

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Agent: Codex Desktop

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

Maintainer follow-up: I synced this branch to current main with signed merge commit d98aa4ffb (preserving the original author history). The targeted Perl parallel-lane regression suite passed locally (18 tests), and push hooks passed. Fresh CI is now running on this exact head; no reviewer question is outstanding yet.

Comment thread Dockerfile.base
Comment thread Dockerfile.base
Comment thread test/perl-critical-cve-remediation.test.ts
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria

Copy link
Copy Markdown
Collaborator Author

Exact-head maintainer follow-up for 90e0b3ea4: the two advisor warnings do not require another code change. The required amd64 image job and native arm64 image job each executed the split harness: Constant.t passed alone (1 file, 302 tests), the remaining 2,933-file suite passed with about 1.39M tests, and install/package creation completed. The Dockerfile already records the exact observed overlap signature and a two-architecture removal condition; this PR is the durable local regression reference.

The red E2E checks are unrelated to the Perl change: coordination failed in generate-matrix at Authenticate controller dispatch before any selected E2E job ran. That is the known controller defect fixed by #7529, so a same-head rerun would not be actionable. The PR remains gated on #7529 landing, fresh exact-head E2E, and independent approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: packaging Packages, images, registries, installers, or distribution area: security Security controls, permissions, secrets, or hardening bug-fix PR fixes a bug or regression security v0.0.96 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants