Skip to content

refactor(sandbox): let the runner reset itself for reuse instead of the pool - #165

Merged
ttbombadil merged 1 commit into
mainfrom
refactor/runner-reset-ownership
Oct 3, 2026
Merged

ttbombadil merged 1 commit into
mainfrom
refactor/runner-reset-ownership

Conversation

@ttbombadil

Copy link
Copy Markdown
Collaborator

Purpose

R3b of the refactoring series (docs/UNOSIM_REFACTORING_OPL.md), audit finding A6. Follows R3a (#164).

Finding re-verified

SandboxRunnerPool reset a released runner through a cast to SandboxRunnerInternal and assigned about 20 ExecutionState fields by hand. Any new state field had to be remembered in the pool, or it survived into the next user's run. Part of that code did nothing:

  • removeAllListeners on runner, registry and batchers (none are EventEmitters);
  • fileBuilder.reset (the method does not exist).

Change

  • SandboxRunner.resetForReuse() owns the reset:
    • stops a running run (a failing stop() is logged and the reset continues; before, it aborted the remaining reset);
    • clears the process listeners and the same execution-state fields as before;
    • resets the I/O registry (tolerating failure) and clears the timeout manager.
  • The pool only calls runner.resetForReuse() inside its existing reset timeout and stuck-runner replacement. SandboxRunnerInternal, clearRunnerListeners and resetRunnerState are gone (−167 lines).

Tests

  • New tests/server/services/sandbox-runner-reset.test.ts against the real runner: state fields, listener clearing, registry reset incl. failure, stop of a running runner incl. failing stop. It was RED before (method missing).
  • sandbox-runner-pool.test.ts described the runner's internal reset from the outside; this was the coupling being removed. Its assertions moved, none were dropped:
Pool test before Now
field resets on release (2 tests), running runner stopped, registry reset called, registry reset failure tolerated runner tests above; pool test asserts delegation to resetForReuse
reset error tolerated, stuck runner replaced, slot freed while reset hangs unchanged intent; the mock's resetForReuse rejects or hangs instead of stop
  • npm run check, ESLint, unit 2713 passed, runner-reuse race test green, Docker integration 26/26 locally, pre-push incl. Sonar quality gate PASSED.

🤖 Generated with Claude Code

…he pool

The pool reset a released runner through a cast to its private fields, so a
new execution-state field could silently survive into the next user's run.
SandboxRunner.resetForReuse() now owns the reset; the pool only delegates.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ttbombadil
ttbombadil merged commit 206d203 into main Oct 3, 2026
5 checks passed
@ttbombadil
ttbombadil deleted the refactor/runner-reset-ownership branch October 3, 2026 21:51
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