Skip to content

Keep persistent sandbox sessions responsive over HTTP - #528

Open
abhishekraok wants to merge 15 commits into
mainfrom
codex/oct31-483-sessions
Open

abhishekraok wants to merge 15 commits into
mainfrom
codex/oct31-483-sessions

Conversation

@abhishekraok

@abhishekraok abhishekraok commented Oct 31, 2026 •

Copy link
Copy Markdown
Collaborator

Codex · codex://threads/01a1326f-ed81-75e3-80e4-661392dbe4b5; updated by Claude Code session 27c852a2-737f-40d7-a147-784d52aa3a9b

SWE-ReX persistent shell calls block its HTTP loop while waiting for pexpect output. Run shell lifecycle/actions in serialized workers so health checks and file requests remain responsive while shell state persists. Cancellation, explicit interrupts, and closure stop the active reader before another operation reads the terminal; ignored interrupts escalate against that terminal's foreground process group. Serialize registry mutations to preserve domain errors during concurrent create/close operations.

Refs remaining sandbox HTTP paths (#483). Its parent, sandbox-http-responsive (#527), merged on 2026-11-12; this now targets main. No task specs, scores, dependencies, default images, or sandbox restart policy change. An interrupt of an active reader acknowledges its stop; the interrupted command retains its own captured output.

Validation:

  • uv run --no-sync ruff check src/ tests/, uv run --no-sync ruff format --check src/ tests/, and uv run --no-sync ty check src/ pass.
  • 21 real-pexpect regressions pass: HTTP responsiveness, persistent cwd/environment, serialization, timeout/cancellation recovery, ignored SIGINT with no deadline, active interruption, queued closure, and concurrent registry lifecycle operations.
  • Earlier full local verification, before the review correction: uv run --no-sync pytest tests/ --ignore=tests/integration -v --basetemp=/tmp/oe483sfinal: 5,085 passed, 41 skipped with bundled Git 2.53. macOS emits pexpect/forkpty threading warnings; a real Linux container also passed.
  • CPU-only Linux Python 3.10.19 with the pinned SWE-ReX source: actual server bootstrap, persistent state, responsive HTTP health/files during execution, session deadline/interrupt recovery, ignored-SIGINT unbounded-action recovery, and runtime close all passed. All owned containers were removed. No live Modal or GPU trial was run.

The production executor supplies finite action timeouts. Active interruption sends SIGINT, allows 0.5 seconds of grace, then targets the terminal's foreground process group if needed. The verified scope is the executor's finite-timeout paths plus the specific ignored-SIGINT canary above.

Remaining robustness follow-ups under (#483): exotic unbounded actions with background processes holding the PTY, cleanup failures that complicate reader ownership, repeated shutdown cancellation, and active-interrupt/completion races. Keep the issue open for these cases.

Final independent review found no blocking defects. Earlier CI passed before the review correction; refreshed CI is running on the new head. No merge is requested. The updated head runs explicit CI, since automatic PR CI targets main.

Review correction (a4be0e17): reader timeout during interrupt grace, both completion-boundary windows, overlapping cancellation, natural completion, and failed compound-action prompt drains now retain exclusive cleanup ownership. A reader timeout is handled before the grace timeout exception; prompt draining avoids another SIGINT to an idle shell. The latest correction passes 35 scoped session/command tests plus lint, formatting, and type checks; refreshed CI above verifies the new head.

@abhishekraok
abhishekraok marked this pull request as ready for review October 31, 2026 19:25
@abhishekraok
abhishekraok requested a review from a team as a code owner October 31, 2026 19:25
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 31, 2026 •

Copy link
Copy Markdown
Codex Review Summary: ✅ Completed

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-11-12T20:21:01.751113Z cb5ab0a New commits
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 361fc566d5

ℹ️ 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".

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@abhishekraok

Copy link
Copy Markdown
Collaborator Author

Written by Claude Code session eval-metrics-handoff

Cross-family review (Claude, of this Codex-written PR): ready. It has one cleanup bug, now fixed in 9db5e95.

  • Fixed: if interrupt cleanup failed during a cancellation (for example shell.close raising), the error escaped _run_worker before its state reset. The caller then got RuntimeError instead of CancelledError, and _active_worker and _interrupt_task were left stale. The next run and close() re-raised the stale error, so the session stayed registered. Now the session is marked closing, the state resets and the cancellation propagates. test_failed_cancellation_cleanup_still_cancels_and_closes fails on the previous head.
  • Non-blocking: an interrupt while a command runs always returns exit_code=0, even when escalation closed the terminal, and _closing isn't set on that path. The executor only logs interrupt results, so scores are unaffected.
  • Test gap: no test checks that server_command installs the session patch. I checked by hand that the bootstrap uses ResponsiveSessionRuntime and ResponsiveBashSession.

test_http_session.py and test_http_command.py: 36 passed locally. ruff and ty are clean.

@abhishekraok

Copy link
Copy Markdown
Collaborator Author

Written by Claude Code session 27c852a2-737f-40d7-a147-784d52aa3a9b

Replies to the cross-family review's non-blocking notes:

  • Interrupt returns exit_code=0 and leaves _closing unset when escalation closed the terminal: left as is. The executor only logs interrupt results, so no score depends on them.
  • No test that server_command installs the session patch: left as is; the review checked it by hand.

Also merged the updated base (codex/oct31-483, now including main) into this branch; ruff, ty and tests/core/harness (313 passed) are clean.

Base automatically changed from codex/oct31-483 to main November 12, 2026 20:17
# Conflicts:
#	README.md
#	src/olmo_eval/harness/sandbox/server_command.py

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant