fix(sandbox): retry cold glibc probes - #8389
Conversation
senthilr-nv
added
area: packaging
|
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:
📝 WalkthroughWalkthroughThe glibc compatibility probe now retries empty Docker output with a longer timeout, uses unique retained-container names, and cleans up failed probes. Tests, Docker mocks, and CI jobs validate the retry lifecycle. ChangesGlibc compatibility probing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CI
participant getImageGlibcVersion
participant Docker
CI->>getImageGlibcVersion: Run lifecycle regression test
getImageGlibcVersion->>Docker: Start named glibc probe
Docker-->>getImageGlibcVersion: Empty output or timeout
getImageGlibcVersion->>Docker: Force-remove failed probe container
getImageGlibcVersion->>Docker: Retry with a new container name
Docker-->>getImageGlibcVersion: Parseable glibc output
getImageGlibcVersion-->>CI: Report compatibility result
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/lib/sandbox-base-image/image-compatibility.test.ts`:
- Around line 84-87: Update the cleanup assertion in the sandbox image
compatibility test to verify the complete forced-removal command, including
["rm", "-f", containerNames[0]], rather than only the container name. Also
assert the cleanup options that preserve the retained-container requirement,
ensuring failed probe containers are force-removed.
In `@src/lib/sandbox-base-image/image-compatibility.ts`:
- Line 37: Update the containerName generation in the glibc probe to remove
reliance on process.pid and use a collision-resistant identifier unique across
processes and lifetimes, such as a UUID or cryptographically random value, while
preserving the existing probe prefix and sequence context as appropriate.
🪄 Autofix
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: 9bb2440d-d45e-4800-85ae-55a453d17d39
📒 Files selected for processing (4)
src/lib/agent/base-image-hermes-resolution.test.tssrc/lib/sandbox-base-image/image-compatibility.test.tssrc/lib/sandbox-base-image/image-compatibility.tstest/helpers/onboard-script-mocks.cjs
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
1 terminology difference 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 against this exact revision. Recommended E2E: None 1 warning · 0 suggestionsWarningsWarnings do not block.
|
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
Security review — exact head 8619d8d7dd9ed9dba7e3de6d027fbda2eaedb8bd against base c6713c9dd17a9682654713ccb6829b8714f1b307: PASS with no findings, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. The patch changes no secret source, credential value, environment allowlist, or logging path.
-
Input Validation and Data Sanitization — PASS. Docker execution remains argv-based. The image reference stays a single argument, and the generated container name uses
randomUUID()under a fixed prefix rather than caller-controlled shell text. -
Authentication and Authorization — PASS. No identity, permission, ownership, or authorization boundary changes.
-
Dependencies and Third-Party Libraries — PASS. No package, image pin, registry, or dependency version changes.
-
Error Handling and Logging — PASS. An empty first probe triggers one bounded retry only after forced cleanup of the retained named container. Non-empty incompatible output is rejected immediately, and two empty probes still fail closed.
-
Cryptography and Data Protection — PASS.
randomUUID()is used only for collision-resistant temporary container identity. No cryptographic primitive, key handling, or protected-data flow changes. -
Configuration and Security Headers — PASS. No configuration schema, network policy, port, capability, privilege, or security-header change.
-
Security Testing — PASS. Exact-head focused suites pass 16/16 and 32/32. Tests bind the full
docker rm -f <name>cleanup command and timeout, distinct UUID-form probe names, the 20-second/120-second bounds, immediate incompatible-output rejection, and Docker mock argument handling. -
System Security — PASS. The glibc minimum remains authoritative. Cleanup reduces retained host container state, retry count is bounded, and the implementation does not broaden Docker privileges or runtime access.
Files Reviewed
src/lib/agent/base-image-hermes-resolution.test.tssrc/lib/sandbox-base-image/image-compatibility.test.tssrc/lib/sandbox-base-image/image-compatibility.tstest/helpers/onboard-script-mocks.cjs
Provenance and Required Gates
Reproduced bug #8375 establishes the remediation scope without adding a supported product surface. All three commits are GitHub Verified. The effective patch has raw SHA-256 121cf353fd0ec61e9e0835a25db2b16aa4f541d3b076550ba7b74a5483ea88b1 and stable patch ID 554ed6d6fb0fee0e544564f20e156a9024fd6bc9. Existing exact-head documentation writer review records no docs needed; docs/reference/commands.mdx already owns the preserved compatibility contract.
Fresh required CI, protected E2E, unresolved-review resolution, and independent exact-head approval remain mandatory. This review waives none of those gates.
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
Security revalidation — exact head 4c19d40326f6155ea856e1d07eee8ae726c122c3 against current base 1608281462923ff8282151c7be1a109ee262b4e9: PASS with no findings, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. The effective PR patch is unchanged from the prior exact review. The base refresh adds no overlapping secret, credential, environment, or sensitive-logging path.
-
Input Validation and Data Sanitization — PASS. The reviewed validation and command/path construction are unchanged. The new base delta does not alter an effective PR file.
-
Authentication and Authorization — PASS. No identity, ownership, permission, or authorization boundary changed in the PR patch or its integration with the refreshed base.
-
Dependencies and Third-Party Libraries — PASS. The PR's reviewed dependency and artifact identities are unchanged. Base PR #8387 adds verified llama.cpp GGUF acquisition in eight disjoint files and does not change this PR's dependency surface.
-
Error Handling and Logging — PASS. The reviewed fail-closed behavior and diagnostics are unchanged; the base refresh introduces no overlapping error or logging path.
-
Cryptography and Data Protection — PASS. The PR's reviewed cryptographic and protected-data behavior is unchanged. No new cross-patch data flow exists.
-
Configuration and Security Headers — PASS. The effective PR patch preserves its reviewed configuration, network, privilege, capability, port, and header boundaries.
-
Security Testing — PASS. Raw patch SHA-256 remains
121cf353fd0ec61e9e0835a25db2b16aa4f541d3b076550ba7b74a5483ea88b1and stable patch ID remains554ed6d6fb0fee0e544564f20e156a9024fd6bc9. Documentation writer revalidation passed at this exact head with no docs change needed. Prior targeted test evidence remains applicable; fresh exact-head GitHub Actions is authoritative. -
System Security — PASS. The signed refresh is conflict-free, preserves the reviewed patch byte-for-byte, and introduces no new runtime trust-boundary interaction with the disjoint base delta.
Provenance and Required Gates
This exact-head revalidation supplements the full prior nine-category review at #8389 (review). It applies the current rubric from .agents tree 7320c9e331ddbcd9ff83e0fce9387a004e3bd1aa.
Fresh required CI, protected E2E, the exact documentation receipt, unresolved-review resolution, GitHub commit verification, and an independent exact-head approval remain mandatory. This review waives none of those gates.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Verdict
Security review — exact head 488f2720a56c71f0db9ef1a4b1dfc76ad28abf2f against current base b148bc13ee53b7a1f612d07e42275ec732e71877: PASS with no findings, subject to every repository merge gate.
Findings Table
No findings.
Detailed Analysis
-
Secrets and Credentials — PASS. The effective PR patch is unchanged and adds no secret, credential source, environment exposure, or sensitive logging. The refreshed base introduces no overlapping credential path.
-
Input Validation and Data Sanitization — PASS. Reviewed validation, command construction, and path handling remain unchanged. The portable-profile base delta has no exact file overlap with this PR.
-
Authentication and Authorization — PASS. No identity, ownership, permission, or authorization boundary is widened by the PR patch or its integration with the refreshed base.
-
Dependencies and Third-Party Libraries — PASS. The PR's reviewed dependency and artifact identities are unchanged. Base PRs #8333 and #8376 add disjoint E2E artifact restoration and portable experimental onboarding behavior.
-
Error Handling and Logging — PASS. Reviewed fail-closed behavior and diagnostics remain unchanged; the signed integrations add no overlapping error or logging path.
-
Cryptography and Data Protection — PASS. Reviewed cryptographic and protected-data behavior remains unchanged, with no new cross-patch data flow.
-
Configuration and Security Headers — PASS. The effective PR patch preserves its reviewed configuration, network, privilege, capability, port, and header boundaries. The hidden portable profile does not bypass this PR's authority checks.
-
Security Testing — PASS. Raw patch SHA-256 remains
121cf353fd0ec61e9e0835a25db2b16aa4f541d3b076550ba7b74a5483ea88b1and stable patch ID remains554ed6d6fb0fee0e544564f20e156a9024fd6bc9. The prior exact image-compatibility and sandbox-base-resolution evidence remains applicable. Exact-head documentation writer revalidation passed; fresh GitHub Actions is authoritative. -
System Security — PASS. Both signed refreshes are conflict-free, preserve the effective patch byte-for-byte, and introduce no new runtime trust-boundary interaction. For #7853 specifically, the portable profile supplies onboarding defaults but neither stages messaging credentials nor bypasses registry-driven credential-drift validation.
Provenance and Required Gates
The exact head preserves contributor history through signed merge commits and applies the current rubric from .agents tree 7320c9e331ddbcd9ff83e0fce9387a004e3bd1aa. This supplements the full prior nine-category review at #8389 (review).
Fresh required CI, protected E2E, the exact documentation receipt, unresolved-review resolution, GitHub commit verification, and an independent exact-head approval remain mandatory. This review waives none of those gates.
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/image-compatibility-docker-lifecycle.test.ts (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the established POSIX PATH separator.
Line 68 uses
path.delimiter. This test runs on Linux CI, so use:to match the repository test convention.Proposed change
- process.env.PATH = `${fixtureDir}${path.delimiter}${originalPath}`; + process.env.PATH = `${fixtureDir}:${originalPath}`;Based on learnings: use the established POSIX
:separator in these Linux-only tests and do not replace it withpath.delimiter.🤖 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/image-compatibility-docker-lifecycle.test.ts` at line 68, Update the PATH construction in the test setup around the environment assignment to use the established POSIX “:” separator instead of path.delimiter, preserving the existing fixtureDir and originalPath ordering.Source: Learnings
🤖 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.
Nitpick comments:
In `@test/image-compatibility-docker-lifecycle.test.ts`:
- Line 68: Update the PATH construction in the test setup around the environment
assignment to use the established POSIX “:” separator instead of path.delimiter,
preserving the existing fixtureDir and originalPath ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c925717e-d92d-4289-9b0c-f32780a0fb99
📒 Files selected for processing (3)
.github/workflows/pr-self-hosted.yaml.github/workflows/sandbox-images-and-e2e.yamltest/image-compatibility-docker-lifecycle.test.ts
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/image-compatibility-docker-lifecycle.test.ts (1)
68-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the POSIX
:separator for the Bash shim.The shim runs through Bash, and this test runs on POSIX CI.
path.delimitermakes the injectedPATHdepend on the host OS. Use:so the fixture directory remains a valid POSIXPATHentry.Based on learnings, this repository uses
:forPATHconstruction in Linux-only tests.Suggested change
- process.env.PATH = `${fixtureDir}${path.delimiter}${originalPath}`; + process.env.PATH = `${fixtureDir}:${originalPath}`;🤖 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/image-compatibility-docker-lifecycle.test.ts` at line 68, Update the PATH assignment in the test setup to join fixtureDir and originalPath with the POSIX “:” separator instead of path.delimiter, keeping the Bash shim’s PATH valid on POSIX CI.Source: Learnings
🤖 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/image-compatibility-docker-lifecycle.test.ts`:
- Around line 90-100: Extract the cleanup logic from the finally block into a
named helper, and have it remove every probe container name recorded by the
Docker shim, including both the initial and retry probe names. Invoke this
helper from the finally path so cleanup still occurs when the test fails before
assertions complete, while preserving fixture-directory removal and PATH
restoration.
- Line 32: Update the environment setup around originalPath to also record
whether process.env.PATH was initially present, then restore that presence state
in the finally block: reinstate the saved value when present and delete
process.env.PATH when it was originally absent. Keep the existing test cleanup
behavior unchanged for other state.
- Around line 20-23: Increase the timeout configured through testTimeoutOptions
for the “removes a retained first probe before accepting the retry (`#8375`)” test
so it exceeds the 120-second imageMeetsMinimumGlibc retry duration and includes
setup and cleanup margin.
---
Nitpick comments:
In `@test/image-compatibility-docker-lifecycle.test.ts`:
- Line 68: Update the PATH assignment in the test setup to join fixtureDir and
originalPath with the POSIX “:” separator instead of path.delimiter, keeping the
Bash shim’s PATH valid on POSIX CI.
🪄 Autofix
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: c8f93d6f-39ce-48e8-afa8-a9b78ee4dde1
📒 Files selected for processing (7)
.github/workflows/pr-self-hosted.yaml.github/workflows/sandbox-images-and-e2e.yamlsrc/lib/agent/base-image-hermes-resolution.test.tssrc/lib/sandbox-base-image/image-compatibility.test.tssrc/lib/sandbox-base-image/image-compatibility.tstest/helpers/onboard-script-mocks.cjstest/image-compatibility-docker-lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/agent/base-image-hermes-resolution.test.ts
- src/lib/sandbox-base-image/image-compatibility.ts
- .github/workflows/pr-self-hosted.yaml
- test/helpers/onboard-script-mocks.cjs
- src/lib/sandbox-base-image/image-compatibility.test.ts
- .github/workflows/sandbox-images-and-e2e.yaml
…to codex/fix-cold-glibc-probe
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
3 participants
Summary
A cold Docker run could return no glibc probe output and leave its probe container behind.
The probe now removes that container and makes one bounded second attempt.
Non-empty incompatible output still fails without another attempt.
Related Issue
Fixes #8375
Changes
docker rm -fafter an empty result, then make one attempt bound at 120 seconds.Type of Change
Quality Gates
docs/reference/commands.mdx. No command, flag, configuration, output, error, default, workflow, or supported surface changes.aac928729219ead63ca62010fdba8f1f454dc4c1and base SHAbf429e90e5b217632356e2d918bfc333e5b8e959. The later commitfd0b2ad6b97068cf22b20694c169f21b9a674081changes only regression coverage and its Docker-capable CI wiring. Required checks and automated review remain separate requirements.Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shdoes not change.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHub — all 19 commits appear asVerified.pre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable — normal hooks passed forfd0b2ad6b.833fc102dare pending; no waiver is recorded.npm run docsbuilds without warnings (doc changes only) — not applicable; no documentation file changed.Sensitive-Path Security Review
Independent review found no security finding for production commit
aac928729219ead63ca62010fdba8f1f454dc4c1against base SHAbf429e90e5b217632356e2d918bfc333e5b8e959.node:cryptois built in; no package, image pin, registry, or dependency metadata changed.Required checks and independent maintainer approval remain separate merge requirements.
GitHub Actions
GitHub Actions is authoritative for PR commit
833fc102df5caa2c511cf15b3b781d40c293c08dagainst base SHAdf56c02b28ec4882c4c98006cdb5dd0809319291.Required checks are pending.
No check waiver is recorded.
Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests