Add Apptainer runtime support - #2473
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 058fc580ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This PR adds a substantial Apptainer runtime with image handling, filesystem mounts, resource limits, live processes, and teardown, while also changing shared Docker/Podman process behavior. Unresolved high-severity concerns about workdir masking and cancellation leaks require human review of the runtime lifecycle. No code changes detected at You can add or adjust custom eligibility rules. Learn more. |
058fc58 to
dd9a4be
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd9a4bec66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
dd9a4be to
3f3b66e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f3b66e4e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3f3b66e to
f64cba8
Compare
f64cba8 to
58a1b38
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 58a1b38fd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
58a1b38 to
458e842
Compare
458e842 to
4e1603e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e1603ed91
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4e1603e to
01fa6a9
Compare
01fa6a9 to
f3ed4e5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f3ed4e5182
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3f4c420 to
cd830df
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cd830df4a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e56a4e0 to
e92c21d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e92c21dd31
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
34a9ab7 to
d522b43
Compare
d522b43 to
c45157b
Compare
c45157b to
3cefad5
Compare
3cefad5 to
7c1520c
Compare
| process = await asyncio.create_subprocess_exec( | ||
| "apptainer", | ||
| *args, | ||
| env=env, | ||
| stdin=asyncio.subprocess.PIPE if input is not None else None, | ||
| stdout=asyncio.subprocess.PIPE, | ||
| stderr=asyncio.subprocess.PIPE, | ||
| start_new_session=True, | ||
| ) | ||
| if client_started is not None: | ||
| client_started.set() |
There was a problem hiding this comment.
🟠 High runtimes/apptainer.py:106
Cancelling start() while asyncio.create_subprocess_exec is spawning apptainer instance start leaves client_started unset even if the instance has already been created, so cleanup skips instance stop and leaks the live sandbox and its processes. Set the event before awaiting subprocess creation so cancellation triggers the named-instance cleanup (a stop attempt is harmless if spawning fails).
| process = await asyncio.create_subprocess_exec( | |
| "apptainer", | |
| *args, | |
| env=env, | |
| stdin=asyncio.subprocess.PIPE if input is not None else None, | |
| stdout=asyncio.subprocess.PIPE, | |
| stderr=asyncio.subprocess.PIPE, | |
| start_new_session=True, | |
| ) | |
| if client_started is not None: | |
| client_started.set() | |
| if client_started is not None: | |
| client_started.set() | |
| process = await asyncio.create_subprocess_exec( | |
| "apptainer", | |
| *args, | |
| env=env, | |
| stdin=asyncio.subprocess.PIPE if input is not None else None, | |
| stdout=asyncio.subprocess.PIPE, | |
| stderr=asyncio.subprocess.PIPE, | |
| start_new_session=True, | |
| ) |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/runtimes/apptainer.py around lines 106-116:
Cancelling `start()` while `asyncio.create_subprocess_exec` is spawning `apptainer instance start` leaves `client_started` unset even if the instance has already been created, so cleanup skips `instance stop` and leaks the live sandbox and its processes. Set the event before awaiting subprocess creation so cancellation triggers the named-instance cleanup (a stop attempt is harmless if spawning fails).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c1520c92a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| capture_streams = [self.stdout, self.stderr] | ||
| captures = [asyncio.create_task(stream.capture()) for stream in capture_streams] | ||
| try: | ||
| exit_code = await self._process.wait() |
There was a problem hiding this comment.
When the attached target exits after spawning a descendant that inherits stdout or stderr (for example, sh -c 'sleep 60 &'), asyncio can record the local client's return code while Process.wait() remains blocked until those pipes reach EOF. The _FINISH command intended to kill that process group is only issued after this await, so it can never run; run_attached_process() hangs indefinitely, while ACP teardown burns through its timeouts and may leave the descendant running in a borrowed runtime. Detect the leader/client exit independently of pipe closure and run _FINISH before waiting for the captures to reach EOF.
Useful? React with 👍 / 👎.
7c1520c to
e3211f1
Compare
e3211f1 to
9d8c4a3
Compare
| 'vf-process "$@"' | ||
| ) | ||
| _SIGNAL = 'kill -"$1" "-$2" 2>/dev/null || kill -"$1" "$2"' | ||
| _FINISH = 'kill -KILL "-$1" 2>/dev/null || true; rm -f "$2"' |
There was a problem hiding this comment.
🟠 High runtimes/attached.py:25
AttachedProcess.wait() can hang indefinitely after the leader exits when a detached descendant keeps stdout or stderr open. _FINISH masks a failed process-group kill with || true, so successful rm -f "$2" makes cleanup look reaped and wait() awaits capture tasks that never reach EOF. Preserve the kill status while still removing the pidfile.
-_FINISH = 'kill -KILL "-$1" 2>/dev/null || true; rm -f "$2"'
+_FINISH = 'kill -KILL "-$1" 2>/dev/null; status=$?; rm -f "$2"; exit "$status"'🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/runtimes/attached.py around line 25:
`AttachedProcess.wait()` can hang indefinitely after the leader exits when a detached descendant keeps stdout or stderr open. `_FINISH` masks a failed process-group `kill` with `|| true`, so successful `rm -f "$2"` makes cleanup look reaped and `wait()` awaits capture tasks that never reach EOF. Preserve the `kill` status while still removing the pidfile.
| if self._process.returncode is not None: | ||
| return |
There was a problem hiding this comment.
🟡 Medium runtimes/attached.py:147
After the local setsid -w client exits, terminate() and kill() return without signaling the recorded process group, so descendants such as a background sleep continue running. The _signal guard checks only self._process.returncode; remove that early return so the process group is signaled even after the leader exits.
- if self._process.returncode is not None:
- return🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/runtimes/attached.py around lines 147-148:
After the local `setsid -w` client exits, `terminate()` and `kill()` return without signaling the recorded process group, so descendants such as a background `sleep` continue running. The `_signal` guard checks only `self._process.returncode`; remove that early return so the process group is signaled even after the leader exits.
| image = image_reference(self.config.image) | ||
| transport, separator, _ = image.partition(":") | ||
| if "://" in image or (separator and transport in _SINGLE_COLON_TRANSPORTS): | ||
| local_image = self._tempdir / "image.sif" |
There was a problem hiding this comment.
Images always land in /tmp
Medium Severity
The runtime root and pulled SIF are created with dir="/tmp", so image conversion ignores TMPDIR and other scratch locations. On hosts where /tmp is a small tmpfs, typical of Apptainer HPC nodes, pull or build can fail or exhaust local disk even when the client cache is configured elsewhere.
Reviewed by Cursor Bugbot for commit 96b3ce0. Configure here.
| result = await self._runtime_exec( | ||
| ["sh", "-c", _SIGNAL, "vf-signal", signal, str(self._pid)] | ||
| ) |
There was a problem hiding this comment.
🟠 High runtimes/attached.py:149
When the runtime control CLI stalls, AttachedProcess.terminate() never completes, so ACP shutdown cannot reach its timed wait()/kill() stages and the live target remains running indefinitely. _signal awaits runtime_exec directly even though _CONTROL_TIMEOUT bounds the other control operations; wrap this call in that timeout and return on TimeoutError so shutdown can continue.
- result = await self._runtime_exec(
- ["sh", "-c", _SIGNAL, "vf-signal", signal, str(self._pid)]
- )
+ try:
+ result = await asyncio.wait_for(
+ self._runtime_exec(
+ ["sh", "-c", _SIGNAL, "vf-signal", signal, str(self._pid)]
+ ),
+ _CONTROL_TIMEOUT,
+ )
+ except TimeoutError:
+ return🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @verifiers/v1/runtimes/attached.py around lines 149-151:
When the runtime control CLI stalls, `AttachedProcess.terminate()` never completes, so ACP shutdown cannot reach its timed `wait()`/`kill()` stages and the live target remains running indefinitely. `_signal` awaits `runtime_exec` directly even though `_CONTROL_TIMEOUT` bounds the other control operations; wrap this call in that timeout and return on `TimeoutError` so shutdown can continue.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3c5cc32. Configure here.
| await self._cleanup_async() | ||
| raise SandboxError( | ||
| f"apptainer instance start failed: {started.stderr.strip()}" | ||
| ) |
There was a problem hiding this comment.
Failed start cleanup hides original error
Medium Severity
A non-zero instance start marks the instance as started and then awaits cleanup without suppressing errors. If instance stop fails while the name still appears in the instance list, that teardown RuntimeError replaces the SandboxError that contains the actual start failure. The cancellation path already suppresses cleanup errors so the original exception can propagate.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 3c5cc32. Configure here.
1 participant
Superseded by #2528.
Overview
Adds first-class Apptainer 1.5 instance support as the third layer in the local-runtime stack. The runtime preserves Verifiers' process, filesystem, and resource contract while keeping Apptainer's host-network boundary explicit.
Details
type="apptainer"configuration, runtime dispatch, runtime info, and public exports.nvidia:allGPU mode, and deterministically rejects unsupported disk and GPU settings.Note
Medium Risk
New sandbox surface (Apptainer CLI, global instance names, host networking) plus Docker live-process behavior change via shared attach (
setsid -wmandatory); removed Docker-internal process types could break out-of-tree imports.Overview
Adds
type="apptainer"as a first-class local runtime:ApptainerConfig/ApptainerRuntimeprovision Apptainer ≥1.5 instances (image pull/build, contained instance start, exec, background jobs, read/write), with CPU/memory limits andnvidia:all/rocm:allGPU modes. Config validation rejects restricted networking, per-instance disk limits, and images with startscripts; Docker-style image strings are mapped todocker://or local SIF paths.Introduces
verifiers/v1/runtimes/attached.pyso Docker/Podman and Apptainer share live-process attach: PID via a container pidfile, requiressetsid -w, in-container signalling, and post-exit reaping. Docker’sopen_processnow delegates here; the old in-moduleContainerProcess/ startup-abort helpers are removed.ACP shutdown now
close()s the packet reader’s async stdout source during_stop, reducing dangling streams after harness teardown.Reviewed by Cursor Bugbot for commit 3c5cc32. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Add
ApptainerRuntimeand refactor attached process handling across runtimesApptainerRuntimeselectable viatype="apptainer"in config, supporting image pull/build, instance lifecycle, CPU/memory/GPU limits, process exec/attach, background commands, and file I/O inside the containerAttachedProcess,open_attached_process, andrun_attached_processto share process management logic between Docker and Apptainer runtimes; the Docker runtime'sopen_processnow delegates to this moduleApptainerConfigvalidates workdir as a non-root absolute path without..,:, or,; rejectsnetwork_restricted; requires Apptainer >= 1.5_PacketReaderandACPHarnessSession._stopto close the underlying async source on shutdown, avoiding dangling generators/streamssetsid -win the target environment and will fail fast if unavailable; DockerContainerProcessand several docker helpers are removed — any out-of-tree consumers of those symbols will breakMacroscope summarized 3c5cc32.