handleStart's post-openTerminal continuation has no liveness guard — unmount mid-provision can register a phantom session holder and silently break another block's Cancel/Skip

[moxious]

Context

PR #1768 ("Wire requirements/objectives/skippable through ChallengeBlock") carries a session-holder refcounting fix in ChallengeBlock (module-level activeSessionHolders / activeConnectingInstances maps, claimSession/releaseSession, hasOtherSessionHolders/hasOtherConnectingInstances) that closes a worse, already-verified bug: without it, one ChallengeBlock's Cancel/Skip could disconnect a Coda sandbox session that a different, still-live ChallengeBlock was legitimately using — killing another learner's active session outright.

That fix is a real improvement and should not block on this issue. But round-14 review of #1768 (finding F17) identified a narrower defect it introduces, filed here separately per that review's recommendation.

Root cause

In src/components/interactive-tutorial/challenge-block.tsx, handleStart's only guard on the continuation after await terminalCtx.openTerminal(vmOpts) is cancelRequestedRef.current, which is set only by handleCancel. There is no mounted/liveness guard.

const nextSessionId = await terminalCtx.openTerminal(vmOpts);
if (cancelRequestedRef.current) {
  ...
  return;
}
if (nextSessionId) {
  claimSession(nextSessionId);   // <-- registers instanceId into activeSessionHolders
  ...
  runSetup(nextSessionId);       // <-- re-claims + runs real exec calls against the sandbox
}

If the ChallengeBlock instance unmounts while openTerminal() is still pending (e.g. the learner navigates to a different guide/step before a slow VM provision resolves, without clicking Cancel first), the unmount cleanup effect only runs releaseSession():

useEffect(() => {
  return () => {
    releaseSession();
  };
}, [releaseSession]);

At the moment of unmount, heldSessionIdRef.current is still null (nothing has been claimed yet — claimSession hasn't run), so releaseSession() has nothing to undo for this in-flight request. It does not stop the pending handleStart continuation — React does not cancel in-flight promises on unmount.

When openTerminal() eventually resolves (the VM finishes provisioning), the dead closure's continuation still runs: cancelRequestedRef.current is false (Cancel was never clicked), so it unconditionally calls claimSession(nextSessionId) — which inserts the unmounted instance's instanceId into the module-level activeSessionHolders map for that session — and then runSetup(nextSessionId), which actually executes setup commands against the sandbox. Nothing will ever call unregisterSessionHolder for that (sessionId, instanceId) pair again, because the component's single unmount cleanup already fired before this registration happened.

hasOtherSessionHolders(sessionId, instanceId) / hasOtherConnectingInstances(instanceId) — the functions this round's fix added specifically so handleCancel can tell whether another live block still needs the shared session — then see this phantom holder as a legitimate second owner. A later, actually live ChallengeBlock sharing that same Coda session can have its own legitimate Cancel/Skip silently skip the terminalCtx.disconnect() call it should make (isOwner && !hasOther becomes false), leaving that sandbox VM running rather than being torn down when the learner intended to end it.

No test in challenge-block.test.tsx exercises unmount while openTerminal() is still pending — the existing unmount() calls in the suite (e.g. the counter-reset test) unmount only after the component has already settled into a stable state, never mid-await.

Reproduction scenario

1. Learner A opens a guide, reaches a ChallengeBlock in coda mode, clicks Start challenge. openTerminal() is dispatched and is slow (VM provisioning takes, say, 10-20s).
2. Before it resolves, Learner A navigates away from that step/guide (without clicking Cancel) — the ChallengeBlock unmounts.
3. openTerminal() eventually resolves with a session id. The dead component's handleStart continuation runs anyway, calls claimSession(nextSessionId) (registering a phantom, never-released holder) and runSetup(nextSessionId).
4. Learner A (or a different learner sharing the same underlying terminal session, per the shared-session model this fix targets) later opens a different ChallengeBlock that reuses the same live Coda session, then clicks Cancel or Skip intending to tear it down.
5. handleCancel calls hasOtherSessionHolders(liveSessionId, instanceId), which returns true because of the phantom holder from step 3 — so terminalCtx.disconnect() is skipped. The Cancel/Skip action silently no-ops on the backend disconnect, leaving the Coda sandbox VM running under a control the UI told the learner had ended.

Required fix

Per the round-14 review recommendation:

• Track liveness — a mounted ref (e.g. isMountedRef), or an AbortController wired through openTerminal — and check it in handleStart's post-await continuation before calling claimSession/runSetup. If not live, do not register a holder and do not run setup.
• On unmount, if this instance was still awaiting openTerminal() (i.e. handleStart was in flight and had not yet claimed a session), run the same hasOtherSessionHolders/hasOtherConnectingInstances-gated disconnect logic that handleCancel performs today — not just the local bookkeeping releaseSession() currently does — so an instance that dies mid-provision still relinquishes (and, if it turns out to be the sole owner, disconnects) the session it was about to claim.

Required test coverage

Add to src/components/interactive-tutorial/challenge-block.test.tsx:

• Unmount a ChallengeBlock while its openTerminal() promise is still pending, then resolve it — assert activeSessionHolders/activeConnectingInstances end up with no holder registered for the dead instance (exported test helper resetSessionHoldersForTest already exists for setup/teardown; a similar accessor may be needed to assert on the maps, or assert indirectly via a second block's Cancel behaving correctly afterward).
• A second, live ChallengeBlock sharing the same session must still successfully disconnect() on Cancel/Skip after a sibling block unmounted mid-provision — i.e. the phantom-holder regression this issue describes must not reproduce.

Severity / priority

P2 — bounded resource waste and a real cross-block correctness bug, not an emergency.

Verified against the Coda backend (grafana/grafana-coda, server/src/vms.js + pool-manager.js): every VM is granted a hard expires_at (default 30 min, env-configurable via VM_LIFETIME_MINUTES/MAX_VM_LIFETIME_MINUTES), swept by a cron reaper (destroyExpiredVMs) that runs every ~1 minute regardless of frontend behavior, with an independent guest-side self-shutdown backstop (~40 min from boot) in case the reaper itself is down. So this bug cannot leak a sandbox VM indefinitely — the worst case is roughly 30-40 minutes of wasted/billed compute per occurrence, plus a silently-broken Cancel/Skip for whoever legitimately owns that shared session during that same bounded window.

This should be fixed in a follow-up soon after #1768 merges (it's a real, easily-triggered gap — "navigate away before Start finishes" is an ordinary learner action, not an edge case), but it should not block #1768, which fixes a strictly worse bug (killing a different learner's live session outright, with no bound at all on user-visible impact).

Reference

Introduced by the session-holder refcounting fix in #1768. Relevant code: src/components/interactive-tutorial/challenge-block.tsx (handleStart, handleCancel, claimSession/releaseSession, module-level activeSessionHolders/activeConnectingInstances).

[moxious]

🧭 Implementation readiness

Stakes: If a learner starts a Coda challenge and leaves before the VM finishes, the abandoned block can register a fake session holder. A second learner sharing that session can then click Cancel and have it silently fail to disconnect the sandbox.

Sanity check: The session-holder code this issue depends on exists only in the open, approved pull request #1768, not yet on main. This issue's fix can land as part of that PR or as an immediate follow-up once it merges.

Severity: 🟠 Medium — a narrow but real correctness bug. It has a bounded workaround: Coda's VM reaper still cleans up the sandbox.

Code basis:

• src/components/interactive-tutorial/challenge-block.tsx:654-716 @cfd6db6f — handleStart's post-openTerminal continuation checks only cancelRequestedRef, then unconditionally calls claimSession and runSetup.
• src/components/interactive-tutorial/challenge-block.tsx:366-370 @cfd6db6f — Unmount cleanup calls only releaseSession. No mounted flag exists for the pending continuation to check.
• src/components/interactive-tutorial/challenge-block.tsx:342-364 @cfd6db6f — releaseSession only clears a held session. At unmount time none is held yet, so it does nothing for the pending claim.
• src/components/interactive-tutorial/challenge-block.tsx:108-156 @cfd6db6f — Module-level activeSessionHolders and activeConnectingInstances maps persist across instances, letting a dead closure register a lasting phantom holder.
• src/components/interactive-tutorial/challenge-block.tsx:769-784 @cfd6db6f — handleCancel already has the isOwner/hasOther disconnect pattern the unmount cleanup should copy.
• src/integrations/coda/TerminalContext.tsx:72,233 @cfd6db6f — openTerminal takes no AbortSignal. AbortController is not a same-scope swap for a local mounted ref.
• src/components/interactive-tutorial/challenge-block.tsx:969-972 @cfd6db6f — resetChallengeCounter wipes the holder maps on every guide swap, bounding but not preventing the phantom-holder gap.

Test coverage: src/components/interactive-tutorial/challenge-block.test.tsx:1987-2032 already sets up a two-block shared-session scenario the new test can extend.

Labels: Applied agent/ready, bug, coda, robustness, severity:MEDIUM

Open judgment calls:
None. The implementation approach is unambiguous.

Traps for the implementer:

• The issue's quoted snippets are simplified. Read the real cancelled branch in handleStart before patching it.
• resetChallengeCounter already wipes the holder maps on every guide swap. Do not mistake this for the fix.
• Reuse handleCancel's existing disconnect pattern via terminalCtxRef.current inside the unmount cleanup.
• Run the liveness check before releaseSession() runs. releaseSession itself clears the connecting-instance flag the check needs.
• This fix targets code that exists only on PR Wire requirements/objectives/skippable through ChallengeBlock #1768's head, not on main. Land it inside Wire requirements/objectives/skippable through ChallengeBlock #1768 or as an immediate follow-up once Wire requirements/objectives/skippable through ChallengeBlock #1768 merges.

Verdict: ✅ Ready for an agent to implement. (tracking: agent/ready)

Posted by the prep-issues tool, not written by a person.