[TomNaessens]

Review map: dodona-edu/dodona#9402 (step 2 of 7: what each pull request does, the review order and live status)

This pull request lets several Papyros instances share one runtime: new Papyros({ runtime }). It is step 2 of 5 that turn Papyros into a component library offering both the sandbox and a small code playground for Dodona's reading activities (asked for on dodona-edu/dodona#9211). This step is state only: no UI.

Stacked on #1169. It also contains the change from #1159 (approved, closed so the stack reads as one series): runner.start(mode, { files }) (next to main's editorTab option) lets a run start from a workspace holding only the given files, written during the run's Loading phase and skipped when Stop was pressed meanwhile. The shared runtime relies on it: every playground run declares its own files.

Step	PR	What it is	Where to spend review time
1	#1169	Sandbox refactor: shared error/output rendering and helpers	Light: mechanical, covered by tests
2	#1168 (this)	Shared runtime, with run id routing	Here: the rules for sharing one worker
3	#1170	<p-code-playground>, ported from Dodona	The port diff (see #1170)
4	#1171	Data files as read-only tabs in the playground	Light
5	#1185	Demo page with playground scenarios	Try it instead of reading it

The Dodona side: dodona-edu/dodona#9304 (thin wrapper) and #9305 (files).

Why

A reading activity can hold many playgrounds. Each needs its own editor, output and files, so each gets its own Papyros instance, and every existing component (which takes .papyros) works per playground as is. But each instance used to start its own Pyodide worker. A shared runtime keeps one worker per language for all of them.

How to review

Moved without changes (from Runner and Papyros into PapyrosRuntime): the worker clients per language, the worker launch and its retry handling, the input channel and service worker registration. Runner/Papyros keep delegating accessors, so the public API is unchanged.

New, worth reading closely (PapyrosRuntime.ts, then the Runner.ts diff):

• One instance runs at a time: start() claims the runtime before it touches any state, and a refused start() leaves the instance untouched. A claim is a Run { id, owner }, and Backend puts that id on every event the worker emits while the run's code runs, so both workers send it without worker-specific code.
• PapyrosRuntime.dispatch routes by that id alone: an event of the latest run goes to its owner, also when it arrives after the run settled; an event of an earlier run, or from a replaced worker, is dropped. Loading events go to every instance (every editor relints on them), but only the running instance tracks the install. Other events without an id (output from an asyncio task or a setTimeout that outlived its run, provideFiles' Files snapshot) are dropped on a shared runtime and still delivered on a private one. Runner and Debugger don't route themselves anymore: they read the run from the runtime. (Proposed by @bmesuere in review; closes Route worker events by run id on a shared runtime #1187.)
• Booting the runtime shows as Loading: state reads Loading while launching is true (the scratchpad shows "Loading" while Python downloads, and Run stays available and queues the run), instead of reporting Ready before the download even started. A Pyodide boot failure is remembered on the runtime, so the other instances get the same UnsupportedBrowserError instead of each booting a new worker that fails the same way.
• Installing packages is a property, not a run state: phase is the run itself, and state reads Loading while loadingPackages isn't empty. A Loading event can therefore no longer leave an instance in a wrong state, and on a shared runtime only the running instance tracks installs. stop() and provideInput() check again after awaiting the backend, so they never act on another instance's run.
• After a package install, an editor only lints again when it shows an import error (E0401): every editor on a shared runtime receives the install's Loading events, and re-linting all of them queued one pylint run per editor on the one worker.
• Guards only where sharing would break things: stop() and provideInput() from another instance do nothing; a stuck stop replaces the worker and frees it for the others; recovery runs once per runtime and doesn't replay files (each shared run writes its own through start(mode, { files })); disposing one instance leaves the worker to the others.
• runner.ensureLaunched(): launches once and rejects on failure, without the alert/confirm that papyros.launch() shows per instance.

Decisions worth questioning: one Papyros per playground instead of one shared instance with swapped state (that was Dodona's scheduler, and it breaks as soon as components bind to papyros.io); no counters or flags for unlikely races.

Behaviour changes without a shared runtime: new Papyros() gets a private runtime, so the sandbox works as before (MultiInstance.test.ts passes unchanged), except: a second start() while running is now ignored instead of wiping the first run's output, provideInput() outside a run does nothing, and a stuck stop now really replaces the worker (before, its relaunch was a no-op on a live worker).

Manual testing instructions

1. yarn vitest run --exclude '**/.claude/**'
2. Read test/__tests__/state/SharedRuntime.test.ts: one test per rule above.

Checklist

• Tests: SharedRuntime.test.ts, including @bmesuere's repro cases and a seeded model-based test that runs random sequences of starts, stops, input, lint events, restarts and disposals over two instances and checks invariants after every step; full suite green locally (398), with Files.test.ts and RuntimeRecovery.test.ts cases from Let a run start from a workspace with only the given files #1159
• Accessibility: n/a (no UI)
• Docs: README "Multiple instances"
• AI: Claude Code wrote the change

🤖 Generated with Claude Code

[bmesuere]

🤖 Reviewed this PR against its stack base. No additional actionable findings. Validation of the complete stack at 530ba26: 334 existing tests passed; TypeScript checking, ESLint, and the app build passed.

[bmesuere]

🤖 The shared runtime makes the run state in Runner harder to keep right, and I think a small design change would make that much easier, before #1170 builds on it.

The problem. "Loading packages" is stored as a run state with a one-slot undo: onLoad saves the current state in previousState and restores it afterwards. But setState overwrites previousState on every change, so whatever gets restored is whatever happened to be there last. The shared runtime adds more ways to hit this, because Loading events now reach every instance and the isBusyFor gate in the Loading subscription is checked separately for the "loading" and the "loaded" of a pair. Two cases fail against the fakeClient from SharedRuntime.test.ts (tests below):

1. A's editor lint starts installing numpy while nothing runs, then B clicks Run before the install finishes. A skips the "loaded" event because B is running, so A stays in Loading with loadingPackages = ["numpy"]. Every playground lints right after boot, so this window is easy to hit on a page with several blocks.
2. An idle instance that has run before (so previousState is Running) gets a "loaded" without having seen its "loading". onLoad restores previousState, and the idle instance now reports running. provideFiles on a shared runtime does this without any race, because the worker's per-file "loaded" events go to every instance.

A related case: stop() checks isBusyFor before await this.availableBackend(), then interrupts whatever is running on the shared client. If another instance starts a run during that await, stop() interrupts the other instance's run.

Proposal. Make loading a separate property instead of a state, and let only the running instance track it:

// written only by the run lifecycle: start, stop, launch, onStart, onEnd
@stateProperty phase: RunState = RunState.Ready;

@stateProperty get state(): RunState {
    return this.loadingPackages.length > 0 ? RunState.Loading : this.phase;
}

onLoad then only adds and removes package names and never touches the run state, and previousState goes away. A stray or unmatched event can at worst leave a name in the list, and never puts an instance in the wrong state. On a shared runtime, only the instance that holds the runtime processes Loading events; the editors still get them through the event bus and relint as before. Nothing outside Runner compares against RunState.Loading, so the public API doesn't change.

Two things would make this hold up as the playground grows:

• Model-based tests on top of fakeClient. Run random sequences (A or B starts, stops, lint loading/loaded/failed, end, restart, dispose) and after each step check invariants: at most one instance is running; an instance that isn't running is never Running or AwaitingInput; every start() promise settles. A seeded random loop is enough, with no new dependency. This catches the interleavings that are hard to think of as single tests, which is where the risk in this PR is.
• Later, give each run an object with an id. tryAcquire would return a Run { id, owner } and the worker would put that id on every event of the run. Run events would then be routed by id. Events without an id come from lint and change no run state. Events from runs or workers that no longer exist would be dropped. stop() would interrupt only its own Run. This replaces the owner, late-output and isBusyFor rules with one exact rule, but it needs a change in both workers, so a follow-up issue makes more sense than this PR.

I'd do the first part (separate loading property, running instance only) in this PR or right after it. The tests would be good to have before #1170, and the run id can wait.

Repro tests (drop into SharedRuntime.test.ts)

const loading = (status: string): BackendEvent => ({
    type: BackendEventType.Loading,
    data: { modules: ["numpy"], status },
    contentType: "application/json",
});

it("does not leave an idle instance loading when a run starts mid-install", async () => {
    const client = fakeClient();
    const { runtime, a, b } = await sharing(() => client);
    client.emit(loading("loading"));
    const running = b.runner.start();
    await vi.waitFor(() => expect(client.workerProxy.runCode).toHaveBeenCalled());
    client.emit(loading("loaded"));
    client.emit(end);
    client.finishRun();
    await running;
    expect(a.runner.state).toBe(RunState.Ready);
    runtime.dispose();
});

it("ignores a loaded event an idle instance never saw loading for", async () => {
    const client = fakeClient();
    const { runtime, a, b } = await sharing(() => client);
    let running = b.runner.start();
    await vi.waitFor(() => expect(client.workerProxy.runCode).toHaveBeenCalled());
    client.emit(start);
    client.emit(end);
    client.finishRun();
    await running;
    running = a.runner.start();
    await vi.waitFor(() => expect(client.workerProxy.runCode).toHaveBeenCalledTimes(2));
    client.emit(loading("loading"));
    client.emit(end);
    client.finishRun();
    await running;
    client.emit(loading("loaded"));
    expect(b.runner.state).toBe(RunState.Ready);
    runtime.dispose();
});

[TomNaessens]

🤖 @bmesuere your design review is in:

• Loading as a property (da5a2a0): phase is written only by the run lifecycle, state reads Loading while loadingPackages isn't empty, and previousState is gone. On a shared runtime only the running instance tracks installs and forgets them when its run ends. On a private runtime (the app, Dodona's scratchpad), installs triggered by linting still show as Loading, as before.
• The stop() race: stop() and provideInput() check again after awaiting the backend and never act on another instance's run.
• Tests: your two repro tests, a test for the stop() race, and the model-based test you suggested: seeded random sequences of starts, stops, input, lint events, restarts and disposals over two instances, with invariants after every step. The old Runner fails 40 of 50 seeds. It also found two more races, both fixed: an instance disposed while its run waited for the backend still ran its code on the shared worker, and provideInput() could write into another instance's run.
• Run id routing: built right away instead of as a follow-up, in Route worker events by run id on a shared runtime #1188 (closes Route worker events by run id on a shared runtime #1187), between this PR and Share rendering and helpers between the sandbox components #1169. Backend puts the run's id on every event, so both workers send it without worker-specific code, and the runtime routes on that id alone.

[TomNaessens]

🤖 Rebase notes for #1194 (reviewed at 910fb15). From this PR on, git merge-tree also conflicts in ButtonLint.ts, Runner.ts and Library.ts. The notes on #1169 cover the files below this PR.

Runner.ts: the second argument of start()

• This PR adds start(mode?, files?). Move the run buttons into the tab bar, add a host tab and compact the input #1194 adds start(mode?, options: RunOptions = {}), with RunOptions { editorTab?: EditorTabId } exported as a type from Library.ts, and calls this.papyros.io.reset(options.editorTab ?? CODE_TAB).
• Agreed on Move the run buttons into the tab bar, add a host tab and compact the input #1194's review: fold both into one options object, start(mode, { editorTab, files }), and Tom is fine doing that here in the stack.
• Callers to update: CodePlayground.ts (Add a code playground component #1170: start(RunMode.Run, []), and from Show a code playground's data files as read-only tabs #1171 start(RunMode.Run, this.dataFiles.loadForRun())), plus the tests that pass files (Runner, RunLifecycle, SharedRuntime). Show a code playground's data files as read-only tabs #1171 widens files to a promise, so RunOptions.files takes readonly FileEntry[] | Promise<readonly FileEntry[]>.
• In run(), io.reset(editorTab) has to stay before the first await: Move the run buttons into the tab bar, add a host tab and compact the input #1194's README promises that start() selects the tab synchronously, because the promise only settles when the run ends. A refused start() (when tryAcquire fails) should still leave the tab alone.

ButtonLint.ts

• This PR adds runInProgress: Loading while the backend boots, or while it installs packages for the editor, is not a run in progress unless runtime.isRunning(papyros). buttonSet and buttons use it.
• Move the run buttons into the tab bar, add a host tab and compact the input #1194 rewrote buttons into buttonSpecs (an array of { filled, icon, label, disabled, onClick }), rendered by renderButton() twice: as a labelled button (.wide) and as a 32px icon button with aria-label and title (.narrow). A @container papyros-code-runner (width < 30rem) query picks which one shows.
• buttonSpecs still branches on state === Ready || Error. Port: branch on !this.runInProgress, and compute disabled from runner.state === RunState.Error, as this PR does. Keep this PR's buttonSet.
• Move the run buttons into the tab bar, add a host tab and compact the input #1194's refocus picks whichever of firstButtonRef / firstIconButtonRef is connected and has client rects. The slotted buttons follow a divider and are hidden while debugger.active, driven by a hasSlotted state set on slotchange.

Library.ts: #1194 exports CODE_TAB, HOST_TAB and the type RunOptions; this PR exports PapyrosRuntime and the types PapyrosOptions and Run. Keep all of them.

Debugger.ts merges cleanly; keep #1194's HOST_TAB exception in validateActiveTab.

[TomNaessens]

🤖 Rebased onto main as a5d61c1; the #1194 note above is applied: start(mode?, options: RunOptions = {}) with files added to main's RunOptions, io.reset(editorTab) synchronous right after the claim, and a refused start leaves the tab alone (test extended with editorTab: HOST_TAB). ButtonLint.buttonSpecs branches on !this.runInProgress; exports and Debugger's HOST_TAB exception kept.

[TomNaessens]

🤖 Simplify pass after the rebase (now 9966107):

• The boot is a launching property, not a phase: state reads Loading while it is true. launch() no longer writes phase, which removed two compensating checks in run(). ButtonLint.runInProgress is runtime.isRunning(papyros).
• After a package install, an editor only lints again when it shows an import error (E0401); before, every editor on a shared runtime queued a pylint run on the one worker. Pylint's message id is now on WorkerDiagnostic.code.
• runtime.release(run) returns whether it ended the run; one writeFiles helper (still sequential); one _reset_workspace() in papyros.py.

[TomNaessens]

🤖 note: Fixed a CI-only hang the simplify pass introduced in this PR (eba4dda).

Since the boot became the launching property, the relaunch after an interrupt no longer moved phase out of Stopping. When that relaunch took longer than 5 s (CI boots Pyodide in four test files at once), stop() took it for a deadlock and replaced the worker a second time, mid-launch; the launch call on the terminated worker never settled and Jspi.test.ts "still replaces the worker to interrupt a busy loop" timed out after 180 s. An interrupted or terminated run now ends before the relaunch instead of after it. Regression test in RunLifecycle.test.ts ("lets a stop that replaced the worker return while the replacement launches"), which fails without the fix.

[TomNaessens]

🤖 Automated review on Tom's behalf; summary below.

---

Generated by Claude Code

[TomNaessens]

🤖 Blocking finding. Reviewed with the code-review skill (escalated because the diff is large and this PR changes how student code is run on a shared worker), with a checkout of the PR head. Covers the full PR against its base #1169 (step 2 of the stack in dodona-edu/dodona#9402).

• Blocking: disposing an instance whose run waits for the first launch restarts the worker mid-launch, and that launch never settles, so every other instance on the runtime hangs in Loading.
• Non-blocking: under JSPI, events from other worker calls made during a suspended run carry that run's id.
• Question: the boot-time Loading state also disables the sandbox's language and example pickers.

The skill raised 10 candidates; I verified each against the code and dropped 7:

• stale sys.modules: already discussed and decided on this PR;
• dropped id-less Files events, and canStart ignoring other instances' runs: documented decisions;
• unused ensureLaunched: the playground uses it;
• relint only on E0401, a relaunch loop in availableBackend, and restarting the worker on dispose: speculative or deliberate.

---

Generated by Claude Code

[TomNaessens]

🤖 note: Changed since the approval (10a890e): fixes for three review-routine findings.

• A launch settles when its worker is terminated or replaced, and waiting instances continue on the replacement: removing a playground while Python boots no longer hangs the others.
• Only events of runCode's own call carry its run id: a block waiting at input() no longer shows another block's package install as its own (JSPI).
• The language and example pickers stay enabled while Python boots, as on main (disabled during a run or while debugging).

The seeded model test gained a provideFiles action; seeds 108 and 306 catch all seven guards.