refactor(sandbox): let the runner reset itself for reuse instead of the pool - #165
Merged
Merged
Conversation
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
R3b of the refactoring series (
docs/UNOSIM_REFACTORING_OPL.md), audit finding A6. Follows R3a (#164).Finding re-verified
SandboxRunnerPoolreset a released runner through a cast toSandboxRunnerInternaland assigned about 20ExecutionStatefields 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:removeAllListenerson runner, registry and batchers (none are EventEmitters);fileBuilder.reset(the method does not exist).Change
SandboxRunner.resetForReuse()owns the reset:stop()is logged and the reset continues; before, it aborted the remaining reset);runner.resetForReuse()inside its existing reset timeout and stuck-runner replacement.SandboxRunnerInternal,clearRunnerListenersandresetRunnerStateare gone (−167 lines).Tests
tests/server/services/sandbox-runner-reset.test.tsagainst 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.tsdescribed the runner's internal reset from the outside; this was the coupling being removed. Its assertions moved, none were dropped:resetForReuseresetForReuserejects or hangs instead ofstopnpm 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