diff --git a/docs/UNOSIM_REFACTORING_OPL.md b/docs/UNOSIM_REFACTORING_OPL.md index a6bce2581..627b72471 100644 --- a/docs/UNOSIM_REFACTORING_OPL.md +++ b/docs/UNOSIM_REFACTORING_OPL.md @@ -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 | – | @@ -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 | – | – | @@ -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 | – | – | diff --git a/server/services/sandbox-runner-pool.ts b/server/services/sandbox-runner-pool.ts index 44322c456..6204eea5c 100644 --- a/server/services/sandbox-runner-pool.ts +++ b/server/services/sandbox-runner-pool.ts @@ -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 { @@ -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; @@ -241,7 +195,7 @@ export class SandboxRunnerPool { let resetTimer: ReturnType | undefined; try { await Promise.race([ - this.resetRunnerState(runner), + runner.resetForReuse(), new Promise((_, reject) => { resetTimer = setTimeout( () => reject(new Error("Runner reset timed out")), @@ -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 { - 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 & { diff --git a/server/services/sandbox-runner.ts b/server/services/sandbox-runner.ts index 524d1aab6..ca8e7b760 100644 --- a/server/services/sandbox-runner.ts +++ b/server/services/sandbox-runner.ts @@ -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 { + 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, diff --git a/tests/server/services/sandbox-runner-pool.test.ts b/tests/server/services/sandbox-runner-pool.test.ts index 8421e2e18..c77c9a283 100644 --- a/tests/server/services/sandbox-runner-pool.test.ts +++ b/tests/server/services/sandbox-runner-pool.test.ts @@ -16,6 +16,7 @@ vi.mock("../../../server/services/sandbox-runner", () => { isRunning = false; initialize = runnerInitializeMock; stop = vi.fn().mockResolvedValue(undefined); + resetForReuse = vi.fn().mockResolvedValue(undefined); // The real SandboxRunner uses a getter/setter that delegates to executionState.state _state = "stopped"; get state() { @@ -308,52 +309,9 @@ describe("SandboxRunnerPool", () => { expect(runners[0].stop).toHaveBeenCalled(); }); - it("resets runner state on release", async () => { - const pool = getSandboxRunnerPool(); - await pool.initialize(); - - const runner = await pool.acquireRunner(); - - // Simulate runner had been used — set executionState fields - runner.executionState.outputBuffer = "some output"; - runner.executionState.totalOutputBytes = 1000; - runner.executionState.processKilled = true; - runner.executionState.pendingCleanup = true; - - await pool.releaseRunner(runner); - - // After release, executionState fields should be cleaned - expect(runner.state).toBe("stopped"); - expect(runner.executionState.outputBuffer).toBe(""); - expect(runner.executionState.totalOutputBytes).toBe(0); - expect(runner.executionState.processKilled).toBe(false); - expect(runner.executionState.pendingCleanup).toBe(false); - }); - - it("resets executionState.processKilled on release (regression: pool used ad-hoc property)", async () => { - const pool = getSandboxRunnerPool(); - await pool.initialize(); - - const runner = await pool.acquireRunner(); - - // Simulate a simulation that was stopped (processKilled = true) - runner.executionState.processKilled = true; - runner.executionState.pendingCleanup = true; - runner.executionState.isSendingOutput = true; - runner.executionState.totalOutputBytes = 5000; - runner.executionState.messageQueue = [{ type: "stale" }]; - - await pool.releaseRunner(runner); - - // Critical: processKilled must be reset on executionState, not as ad-hoc property - expect(runner.executionState.processKilled).toBe(false); - expect(runner.executionState.pendingCleanup).toBe(false); - expect(runner.executionState.isSendingOutput).toBe(false); - expect(runner.executionState.totalOutputBytes).toBe(0); - expect(runner.executionState.messageQueue).toEqual([]); - }); - - it("handles runner with running state during release", async () => { + // The field-level reset assertions that used to live here moved to + // sandbox-runner-reset.test.ts: the runner owns its reset (resetForReuse). + it("delegates the reset of a released runner to the runner itself", async () => { const pool = getSandboxRunnerPool(); await pool.initialize(); @@ -361,7 +319,8 @@ describe("SandboxRunnerPool", () => { runner.isRunning = true; await pool.releaseRunner(runner); - expect(runner.stop).toHaveBeenCalled(); + expect(runner.resetForReuse).toHaveBeenCalledOnce(); + expect(runner.stop).not.toHaveBeenCalled(); }); it("rejects requests beyond the configured queue limit", async () => { @@ -431,50 +390,14 @@ describe("SandboxRunnerPool", () => { await pool.initialize(); const runner = await pool.acquireRunner(); - // Make stop throw - runner.stop = vi.fn().mockRejectedValue(new Error("stop failed")); - runner.isRunning = true; + // Make the runner's reset throw + runner.resetForReuse = vi.fn().mockRejectedValue(new Error("reset failed")); // Should not throw await expect(pool.releaseRunner(runner)).resolves.toBeUndefined(); }); - it("calls registryManager.reset() during runner release", async () => { - const pool = getSandboxRunnerPool(); - await pool.initialize(); - - const runner = await pool.acquireRunner(); - const mockRegistryManager = { - destroy: vi.fn(), - reset: vi.fn(), - removeAllListeners: vi.fn(), - }; - runner.registryManager = mockRegistryManager; - - await pool.releaseRunner(runner); - - expect(mockRegistryManager.reset).toHaveBeenCalledOnce(); - }); - - it("handles registryManager.reset() failure gracefully during release", async () => { - const pool = getSandboxRunnerPool(); - await pool.initialize(); - - const runner = await pool.acquireRunner(); - const mockRegistryManager = { - destroy: vi.fn(), - reset: vi.fn().mockImplementation(() => { - throw new Error("reset failed"); - }), - removeAllListeners: vi.fn(), - }; - runner.registryManager = mockRegistryManager; - - // Should not throw even when reset() fails - await expect(pool.releaseRunner(runner)).resolves.toBeUndefined(); - }); - - it("replaces stuck runner when stop() hangs beyond reset timeout", async () => { + it("replaces stuck runner when its reset hangs beyond reset timeout", async () => { const pool = new SandboxRunnerPool({ minRunners: 5, maxRunners: 5, @@ -483,9 +406,8 @@ describe("SandboxRunnerPool", () => { await pool.initialize(); const runner = await pool.acquireRunner(); - runner.isRunning = true; - // Make stop() hang forever - runner.stop = vi.fn().mockReturnValue(new Promise(() => {})); + // Make the runner's reset hang forever + runner.resetForReuse = vi.fn().mockReturnValue(new Promise(() => {})); const statsBefore = pool.getStats(); expect(statsBefore.inUseRunners).toBe(1); @@ -513,9 +435,8 @@ describe("SandboxRunnerPool", () => { runners.push(await pool.acquireRunner()); } - // Make runner[0].stop() hang - runners[0].isRunning = true; - runners[0].stop = vi.fn().mockReturnValue(new Promise(() => {})); + // Make runner[0]'s reset hang + runners[0].resetForReuse = vi.fn().mockReturnValue(new Promise(() => {})); // Queue a new request const pendingAcquire = pool.acquireRunner(); diff --git a/tests/server/services/sandbox-runner-reset.test.ts b/tests/server/services/sandbox-runner-reset.test.ts new file mode 100644 index 000000000..ce2c2a75c --- /dev/null +++ b/tests/server/services/sandbox-runner-reset.test.ts @@ -0,0 +1,89 @@ +/** + * SandboxRunner.resetForReuse(): the runner owns the reset of its execution + * state before the pool hands it to the next user (previously done by the pool + * through private fields). + */ +import { describe, expect, it, vi } from "vitest"; +import { SandboxRunner } from "../../../server/services/sandbox-runner"; +import type { IProcessController } from "../../../server/services/process-controller"; + +function fakeProcessController(): IProcessController { + return { + spawn: vi.fn().mockResolvedValue(null), + onStdout: vi.fn(), + onStderr: vi.fn(), + onStderrLine: vi.fn(), + supportsStderrLineStreaming: vi.fn(() => true), + onClose: vi.fn(), + onError: vi.fn(), + writeStdin: vi.fn(() => true), + kill: vi.fn(), + destroySockets: vi.fn(), + hasProcess: vi.fn(() => false), + clearListeners: vi.fn(), + getPid: vi.fn(() => null), + }; +} + +type Internals = { + executionState: Record; + registryManager: { reset: () => void }; +}; + +function internals(runner: SandboxRunner): Internals { + return runner as unknown as Internals; +} + +describe("SandboxRunner.resetForReuse", () => { + it("clears the previous run's state and process listeners", async () => { + const processController = fakeProcessController(); + const runner = new SandboxRunner({ processController }); + const state = internals(runner).executionState; + Object.assign(state, { + outputBuffer: "some output", + totalOutputBytes: 5000, + processKilled: true, + pendingCleanup: true, + isSendingOutput: true, + messageQueue: [{ type: "stale" }], + backpressurePaused: true, + onOutputCallback: vi.fn(), + }); + + await runner.resetForReuse(); + + expect(runner.simulationState).toBe("stopped"); + expect(state).toMatchObject({ + outputBuffer: "", + totalOutputBytes: 0, + processKilled: false, + pendingCleanup: false, + isSendingOutput: false, + messageQueue: [], + backpressurePaused: false, + onOutputCallback: null, + }); + expect(processController.clearListeners).toHaveBeenCalledOnce(); + }); + + it("resets the I/O registry and tolerates a failing registry reset", async () => { + const runner = new SandboxRunner({ processController: fakeProcessController() }); + const reset = vi.spyOn(internals(runner).registryManager, "reset").mockImplementation(() => { + throw new Error("reset failed"); + }); + + await expect(runner.resetForReuse()).resolves.toBeUndefined(); + expect(reset).toHaveBeenCalled(); + }); + + it("stops a running runner and still resets when stop fails", async () => { + const runner = new SandboxRunner({ processController: fakeProcessController() }); + internals(runner).executionState.state = "running"; + internals(runner).executionState.outputBuffer = "pending"; + const stop = vi.spyOn(runner, "stop").mockRejectedValue(new Error("stop failed")); + + await expect(runner.resetForReuse()).resolves.toBeUndefined(); + expect(stop).toHaveBeenCalledOnce(); + expect(internals(runner).executionState.outputBuffer).toBe(""); + }); +});