feat: harden SWE against filesystem overflow - #2386
Conversation
|
/ok to test d68e7bc |
|
/claude review |
| apptainer_cmd = ( | ||
| f"apptainer exec --writable-tmpfs --cleanenv --pid --no-mount home,tmp,bind-paths " | ||
| f'OVERLAY_DIR="$(mktemp -d "${{TMPDIR:-/tmp}}/{overlay_prefix}XXXXXX")" && ' | ||
| f"trap 'rm -rf \"$OVERLAY_DIR\"' EXIT TERM INT && " |
There was a problem hiding this comment.
RISK — overlay dirs leak on the exact failure mode they're meant to protect against (disk exhaustion).
trap 'rm -rf "$OVERLAY_DIR"' EXIT TERM INT only fires on those three signals. But this project's own teardown for timed-out / OOM episodes is SIGKILL, not SIGTERM: _kill_container_tree sends SIGKILL (app.py:2317, 2322) and _finish_container_command calls process.kill() (SIGKILL) on timeout (app.py:2494), as does _kill_active_command (app.py:2539). SIGKILL cannot be trapped, so the shell dies without running the trap and $OVERLAY_DIR under $TMPDIR is orphaned.
Blast radius: long training runs are precisely the ones that repeatedly hit agent/eval timeouts and the memory watchdog. Each killed episode leaves a full SWE working-tree overlay in $TMPDIR; these accumulate until $TMPDIR fills — the same "filesystem fills mid-run" failure preserve_episode_artifacts was added to prevent, but this path isn't governed by that knob and is never reaped.
Fix: track the overlay path Python-side and rmtree it in _kill_active_command / the timeout branch after the process is reaped, and/or add a startup sweep of stale apptainer_overlay_* dirs under $TMPDIR (the fixed prefix exists for exactly this). The trap alone only covers clean exits, not the kill paths that dominate at scale.
|
SHIP WITH CARE Sandbox-hardening + episode-scratch-lifecycle change to the SWE agent harness. Net improvement: the One reliability finding worth resolving before this rides a long training run (inline on app.py:3362):
NOTE (author's call): |
| apptainer_cmd = ( | ||
| f"apptainer exec --writable-tmpfs --cleanenv --pid --no-mount home,tmp,bind-paths " | ||
| f'OVERLAY_DIR="$(mktemp -d "${{TMPDIR:-/tmp}}/{overlay_prefix}XXXXXX")" && ' | ||
| f"trap 'rm -rf \"$OVERLAY_DIR\"' EXIT TERM INT && " |
There was a problem hiding this comment.
RISK — overlay dirs leak on the exact path they're meant to protect against (disk exhaustion).
trap 'rm -rf "$OVERLAY_DIR"' EXIT TERM INT only fires on those three signals. But this project's own teardown for timed-out / OOM episodes is SIGKILL, not SIGTERM: _kill_container_tree sends SIGKILL (app.py:2317, 2322) and _finish_container_command calls process.kill() (SIGKILL) on timeout (app.py:2494), as does _kill_active_command (app.py:2539). SIGKILL cannot be trapped, so the shell dies without running the trap and $OVERLAY_DIR under $TMPDIR is orphaned.
Blast radius: long training runs are precisely the ones that hit agent/eval timeouts and the memory watchdog repeatedly. Each killed episode leaves a full SWE working-tree overlay in $TMPDIR. These accumulate until $TMPDIR fills — the same "filesystem fills mid-run" failure the preserve_episode_artifacts knob was added to prevent, but this path isn't governed by that knob and never gets reaped.
Fix: add a startup sweep of stale apptainer_overlay_* dirs under $TMPDIR (they carry a fixed prefix for exactly this reason), and/or add KILL handling by tracking the overlay path on the Python side and rmtree-ing it in _kill_active_command / the timeout branch after the process is reaped. The trap alone covers clean exits, not the kill paths that dominate at scale.
There was a problem hiding this comment.
Thanks for the review, you're absolutely right. I've addressed this in my follow-up commit.
|
/ok to test 5eb218e |
|
/claude review |
|
SHIP — LGTM, no reliability concerns. Reviewed the SWE-bench harness sandbox-hardening + episode-cleanup change end to end:
No public API, async, or scoring-correctness surface touched. |
5eb218e to
0ab9cbd
Compare
0ab9cbd to
de191a7
Compare
de191a7 to
80ea7c3
Compare
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
Signed-off-by: Teodor-Dumitru Ene <teodord.ene@gmail.com>
80ea7c3 to
920e2e1
Compare
apptainer's per-container session tmpfs has a size capped by
sessiondir max size. SWE commonly exceeds the default value, with a simplegit reset --hardusually killing the episode.This PR backs the writable layer with a directory overlay that is freshly created under
TMPDIRand removed when the container exits.Episode results currently land next to the server sources and accumulate forever. This PR adds
results_rootto be able to specify a different root directory - and avoid disk overflow - andpreserve_episode_artifactswhich can clean up stale episode results if set to False.