Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ttbombadil merged 1 commit into from
Oct 3, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions docs/UNOSIM_REFACTORING_OPL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet.
| R6 | Einheitlicher Compile-Hash inkl. Header (Worker-Identität); kein Binary in REST-Payload/LRU | Korrektheit/Performance/Security | A4, A5, S1-INCBIN [Code] | mittel | keine veralteten Cache-Treffer; Worker und direkter Pfad teilen Cache-Einträge; Antwort 59.653 → 562 Byte (Blink-Sketch, Worker-Pfad) | klein | – | fix/compile-cache-key-and-payload | DONE | RED→GREEN: `arduino-compiler-cache-key.test.ts` (Header-Änderung kompiliert neu; gleiche Identität wie der Worker), `compiler-binary-payload.test.ts` (frisch, gecacht, LRU ohne `binary`) | PR-Merge siehe Verlauf |
| R7 | Globaler API-Limiter im Gateway-Modus nach authentifiziertem `subject` statt IP | Skalierung | P1 [Code] | mittel (topologieabhängig) | keine kursweiten 429 hinter Campus-NAT | klein | – | fix/api-rate-limit-identity | DONE | RED→GREEN: `api-rate-limit-key.test.ts` (zwei Subjects hinter einer IP mit getrenntem Budget; ungültiges Gateway-Secret und Local-Modus bleiben pro IP) | PR-Merge siehe Verlauf |
| R3a | Lauf-Generation + Abbruch im Runner-Lifecycle; `ProcessController` leitet nur Events des aktuellen Kindprozesses weiter | Isolation/Lifecycle | S4, S4-CHILD [Code, deterministisch reproduziert] | hoch | keine fremde Ausgabe, kein Start mit fremdem/aufgeräumtem Verzeichnis, kein Eingriff in den Container des Nachfolgers | mittel | – | fix/runner-run-generation | DONE | RED→GREEN: `runner-reuse-race.test.ts` (echter Pool/Runner/ExecutionManager/Semaphore; vorher startete A mit eigenem, bereits gelöschtem Verzeichnis für B), `docker-compile-semaphore-abort.test.ts`, `process-controller-stale-child.test.ts`; Unit 2714, Docker-Integration 26/26 | PR-Merge siehe Verlauf |
| R3b | Reset-Ownership in `runner.resetForReuse()` | Kapselung | A6 [Code] | mittel | Reset an einer Stelle | mittel | R3a | refactor/runner-reset-ownership | OPEN | Pool-/Isolationstests | – |
| R3b | Reset-Ownership in `SandboxRunner.resetForReuse()`; Pool greift nicht mehr in private Runner-Felder | Kapselung | A6 [Code] | mittel | Reset an einer Stelle; neue Felder können nicht mehr am Pool vorbei vergessen werden | mittel | R3a | refactor/runner-reset-ownership | DONE | `sandbox-runner-reset.test.ts` (echter Runner: Felder, Listener, Registry-Reset, Stop-Fehler); Pool-Tests prüfen nur noch die Delegation (Feldaussagen verschoben, keine entfernt) | PR-Merge siehe Verlauf |
| R4a | Orphan-Sweep für Sandbox-Container | Lifecycle | A7 [Code] | mittel | Ressourcen nach Crash frei | klein–mittel | R3a | fix/sandbox-orphan-sweep | OPEN | Sweep-Test (Fake-Executor), Docker-Gate | – |
| R4b | WS-Heartbeat, Serialisierung pro Verbindung, Nachrichtenlimit | Lifecycle | A7 [Code] | mittel | halb offene Verbindungen und Floods begrenzt | mittel | – | fix/ws-connection-lifecycle | OPEN | Lifecycle-Tests mit Fake-Timern | – |
| R5a | Gatekeeper: Queue-Fortsetzung nach TTL, tote Cache-Lock-API | Concurrency | A2 [Code] | mittel | keine hängende Compile-Queue | klein | – | fix/gatekeeper-ttl-handoff | OPEN | echter TTL-Test | – |
Expand All @@ -46,7 +46,7 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet.
| A3 | Fallback umgeht Lastgrenze, keine Worker-Recovery, unbegrenzte Queue | Concurrency | [Code] | mittel | – | – | – | R5b | OPEN | – | – |
| A4 | Compile-Hash ohne Header im direkten Compiler | Korrektheit | [Code] bestätigt | mittel | – | – | – | R6 | DONE | Header-only-Test | `libraries` bleibt außerhalb des Hashes: arduino-cli erhält sie nicht |
| A5 | HEX-Binary in REST-JSON und LRU | Performance | [Code] bestätigt, [gemessen] 59.653 statt 562 Byte | gering | – | – | – | R6 | DONE | Payload-Test | Simulation nutzt das REST-Binary nicht |
| A6 | Pool setzt private Runner-Felder zurück | Kapselung | [Code] | mittel | – | – | – | R3b | OPEN | – | – |
| A6 | Pool setzt private Runner-Felder zurück | Kapselung | [Code] bestätigt | mittel | – | – | – | R3b | DONE | – | Pools `removeAllListeners`-Aufrufe waren wirkungslos (keine EventEmitter), `fileBuilder.reset` existierte nicht |
| A7 | Kein Orphan-Sweep, kein Heartbeat, keine Serialisierung, kein Message-Limit | Lifecycle | [Code] | mittel | – | – | – | R4a/R4b | OPEN | – | – |
| P1 | Globaler API-Limiter pro IP (Campus-NAT) | Skalierung | [Code] bestätigt | mittel | – | – | – | R7 | DONE | Route-Test | Local-Modus bewusst pro IP: dort kann ein Client jederzeit eine neue Session erhalten |
| T1 | Tutor-Adapter mit drei Snapshot-Quellen | Tutor | [Code] | gering | – | – | – | R9 | OPEN | – | – |
Expand Down Expand Up @@ -83,4 +83,5 @@ Abweichungen werden unter „Reihenfolge-Änderungen“ begründet.
| #161 | R2: Per-Identity-Isolation von Code-Fallback und Sketch-CRUD | `18c7d89b` | PR-CI 5/5 grün; Post-Merge-CI von #160 grün |
| #162 | R6: Einheitlicher Compile-Hash, kein Binary in der REST-Antwort | `9716910d` | PR-CI 5/5 grün; Post-Merge-CI von #161 grün |
| #163 | R7: Globaler API-Limiter nach Gateway-Subject | `7a39d554` | PR-CI 5/5 grün; Post-Merge-CI von #162 grün |
| R3a | Lauf-Generation, abbrechbares Start-Slot-Warten, Kindprozess-Guard | – | – |
| #164 | R3a: Lauf-Generation, abbrechbares Start-Slot-Warten, Kindprozess-Guard | `0042bc08` | PR-CI 5/5 grün; Post-Merge-CI von #163 grün |
| R3b | Reset-Ownership im Runner | – | – |
167 changes: 1 addition & 166 deletions server/services/sandbox-runner-pool.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,5 @@
import { SandboxRunner } from "./sandbox-runner";
import { Logger } from "@shared/logger";
import type { IOPinRecord } from "@shared/schema";
import type {
ExecutionState,
TelemetryMetrics,
} from "./sandbox/execution-manager";
import { config } from "../config";

interface PooledRunner {
Expand All @@ -21,47 +16,6 @@ interface QueueEntry {
timeout: NodeJS.Timeout;
}

// Useful internal type for accessing private runner fields safely
type SandboxRunnerInternal = {
state: string;
processKilled: boolean;
executionState: ExecutionState;
processController?: {
proc?: {
stdout?: unknown;
stderr?: unknown;
};
stdoutListeners?: unknown[];
stderrListeners?: unknown[];
closeListeners?: unknown[];
errorListeners?: unknown[];
removeAllListeners?: () => void;
} | null;
pinStateBatcher?: { pause: () => void; resume: () => void } | null;
serialOutputBatcher?: { pause: () => void; resume: () => void } | null;
registryManager?: { destroy: () => void; reset: () => void } | null;
flushMessageQueue?: () => void;
onOutputCallback?: ((line: string, isComplete?: boolean) => void) | null;
outputCallback?: ((line: string, isComplete?: boolean) => void) | null;
errorCallback?: ((line: string) => void) | null;
telemetryCallback?: ((metrics: TelemetryMetrics) => void) | null;
pinStateCallback?:
| ((pin: number, type: string, value: number) => void)
| null;
ioRegistryCallback?:
| ((
registry: IOPinRecord[],
baudrate: number | undefined,
reason?: string,
) => void)
| null;
timeoutManager?: { clear: () => void };
fileBuilder?: { reset: () => void };
flushTimer?: NodeJS.Timeout | null;
// keep object extensible as we access other internal fields in reset logic
[key: string]: unknown;
};

export interface SandboxRunnerPoolOptions {
minRunners?: number;
maxRunners?: number;
Expand Down Expand Up @@ -241,7 +195,7 @@ export class SandboxRunnerPool {
let resetTimer: ReturnType<typeof setTimeout> | undefined;
try {
await Promise.race([
this.resetRunnerState(runner),
runner.resetForReuse(),
new Promise<void>((_, reject) => {
resetTimer = setTimeout(
() => reject(new Error("Runner reset timed out")),
Expand Down Expand Up @@ -332,125 +286,6 @@ export class SandboxRunnerPool {
}, this.idleTimeoutMs);
}

private clearRunnerListeners(runner: SandboxRunnerInternal): void {
const safeRemoveAll = (target: unknown, label: string) => {
if (!target || typeof target !== "object" || target === null) {
return;
}

const maybe = target as { removeAllListeners?: unknown };
if (typeof maybe.removeAllListeners !== "function") {
return;
}

try {
(maybe.removeAllListeners as () => void)();
} catch (error) {
this.logger.debug(
`[SandboxRunnerPool] Failed removeAllListeners on ${label}: ${error}`,
);
}
};

safeRemoveAll(runner, "runner");

const processController = runner.processController;
safeRemoveAll(processController, "processController");
safeRemoveAll(processController?.proc, "processController.proc");
safeRemoveAll(
processController?.proc?.stdout,
"processController.proc.stdout",
);
safeRemoveAll(
processController?.proc?.stderr,
"processController.proc.stderr",
);

safeRemoveAll(runner.registryManager, "registryManager");
safeRemoveAll(runner.serialOutputBatcher, "serialOutputBatcher");
safeRemoveAll(runner.pinStateBatcher, "pinStateBatcher");

if (processController) {
processController.stdoutListeners = [];
processController.stderrListeners = [];
processController.closeListeners = [];
processController.errorListeners = [];
}
}

private async resetRunnerState(runner: SandboxRunner): Promise<void> {
try {
if (runner.isRunning) {
await runner.stop();
}

const r = runner as unknown as SandboxRunnerInternal;

this.clearRunnerListeners(r);

// Use the state setter (delegates to executionState.state)
r.state = "stopped";

// Reset executionState fields directly to avoid creating ad-hoc properties
// on the runner instance that shadow the real executionState fields.
const es = r.executionState;
es.processKilled = false;
es.pauseStartTime = null;
es.totalPausedTime = 0;
es.pinStateBatcher = null;
es.serialOutputBatcher = null;
es.onOutputCallback = null;
es.errorCallback = null;
es.telemetryCallback = null;
es.pinStateCallback = null;
es.ioRegistryCallback = undefined;
es.outputBuffer = "";
es.outputBufferIndex = 0;
es.totalOutputBytes = 0;
es.isSendingOutput = false;
es.pendingCleanup = false;
es.messageQueue = [];
es.stderrFallbackBuffer = "";
es.backpressurePaused = false;

if (es.flushTimer) {
clearTimeout(es.flushTimer);
es.flushTimer = null;
}

if (r.fileBuilder && typeof r.fileBuilder.reset === "function") {
r.fileBuilder.reset();
}

// Reset the existing RegistryManager rather than destroying and recreating it.
// Destroying makes the object permanently unusable (destroyed=true), which breaks
// the ExecutionManager that holds a reference to the same instance.
// The original onUpdate callback uses executionState.ioRegistryCallback dynamically,
// so it picks up the correct callback for each new run automatically.
if (r.registryManager) {
try {
r.registryManager.reset();
} catch (error) {
this.logger.debug(
`[SandboxRunnerPool] RegistryManager reset failed: ${error}`,
);
}
}

if (r.timeoutManager) {
r.timeoutManager.clear();
}

this.logger.debug(
"[SandboxRunnerPool] Runner state reset complete (isolation verified)",
);
} catch (error) {
this.logger.error(
`[SandboxRunnerPool] Error during runner reset: ${error}`,
);
}
}

getStats() {
const sandboxStatuses = this.runners.map((entry) =>
(entry.runner as SandboxRunner & {
Expand Down
50 changes: 50 additions & 0 deletions server/services/sandbox-runner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,56 @@ export class SandboxRunner {
await this.cleanupDockerContainer(containerName);
}

/**
* Returns the runner to a clean state before the pool hands it to the next
* user: stops a run still in progress and clears everything the previous run
* left in the execution state, the process listeners and the I/O registry.
*/
async resetForReuse(): Promise<void> {
if (this.isRunning) {
try {
await this.stop();
} catch (error) {
this.logger.warn(`stop() failed during reset: ${error instanceof Error ? error.message : String(error)}`);
}
}

this.processController.clearListeners();
const s = this.executionState;
s.state = SimulationState.STOPPED;
s.processKilled = false;
s.pauseStartTime = null;
s.totalPausedTime = 0;
s.pinStateBatcher = null;
s.serialOutputBatcher = null;
s.onOutputCallback = null;
s.errorCallback = null;
s.telemetryCallback = null;
s.pinStateCallback = null;
s.ioRegistryCallback = undefined;
s.outputBuffer = "";
s.outputBufferIndex = 0;
s.totalOutputBytes = 0;
s.isSendingOutput = false;
s.pendingCleanup = false;
s.messageQueue = [];
s.stderrFallbackBuffer = "";
s.backpressurePaused = false;
if (s.flushTimer) {
clearTimeout(s.flushTimer);
s.flushTimer = null;
}

// Reset rather than destroy: the ExecutionManager keeps this instance and its
// onUpdate callback reads the next run's ioRegistryCallback from the state.
try {
this.registryManager.reset();
} catch (error) {
this.logger.debug(`RegistryManager reset failed: ${error instanceof Error ? error.message : String(error)}`);
}
this.timeoutManager.clear();
}

getSandboxStatus(): { dockerAvailable: boolean; dockerImageBuilt: boolean } {
return {
dockerAvailable: this.dockerAvailable,
Expand Down
Loading
Loading