diff --git a/CHANGELOG.md b/CHANGELOG.md index 9f290884..d71fdb70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -93,6 +93,17 @@ experience may change before `1.0.0`. `[all]`, and the nightly install job and `scripts/smoke-uv-tool-install.sh` import the runtime in that venv. Existing installs pick it up with `./scripts/install.sh --tools-only`. +- Returning to the Study view after `studyloop web` restarted no longer offers + the session the old server was running and then reports *This session + reports transport "pty", which this view cannot render*. A browser-terminal + or ACP session runs inside the server that started it; when that server + stops without ending it, `/api/session/state` now reports the session ended + (the verdict `/session/start` already reached) instead of live with nothing + to connect to, and deletes nothing, because an unended session's parking lot + holds unflushed notes. The console's fallback also stops blaming the + renderer when only the connection is missing: a known transport with no + connection URL now reads *The server did not return a connection for this + session*. ## [0.5.0] - 2026-09-21 diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 7c6f9863..e55e0b65 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -148,6 +148,13 @@ If the transport is already `pty` and the panel still reports no terminal, the server did not return a connection for the session. Ending and restarting the session clears this; the message says so rather than leaving a blank pane. +A session whose server stopped while it ran (`studyloop web` was restarted, +crashed, or its terminal was closed) is not offered for reattach. A browser +terminal or ACP session runs inside the server that started it, so once that +process is gone `/api/session/state` reports the session ended and the Study +view shows the picker. Start a new session; the old one's parking-lot file is +left in place. + ## The Terminal Is Empty After A Page Refresh The panel should reattach on its own. What holds today: diff --git a/packages/studyloop/src/studyloop/session_state.py b/packages/studyloop/src/studyloop/session_state.py index 0107402a..13215b89 100644 --- a/packages/studyloop/src/studyloop/session_state.py +++ b/packages/studyloop/src/studyloop/session_state.py @@ -446,6 +446,29 @@ def _reservation_pid_is_live_and_foreign(state: dict) -> bool: return _pid_is_alive(pid) +def web_claim_is_orphaned(state: dict) -> bool: + """Whether ``state`` names a web session (PTY/ACP) whose server has gone. + + A web-owned session lives inside the server process that started it, and + ``pid`` records that process. When the pid is neither this process nor + alive, nothing can be serving the session, whatever ``mode`` says: the + crash-then-restart cell :func:`claim_blocks_web_start` already treats as + stale on the start side. The read side needs the same verdict, or + ``/api/session/state`` offers a session for reattach that nothing serves. + + A CLI session is never judged here -- its ``pid`` is the CLI process, + which can exit while its multiplexer session lives on. This process's own + pid is never an orphan (a start in flight writes its reservation before + the slot is acquired), and a claim with no ``pid`` cannot be proven dead. + """ + if not _claim_exists(state) or state.get("transport") not in ("pty", "acp"): + return False + pid = state.get("pid") + if not isinstance(pid, int) or pid == os.getpid(): + return False + return not _pid_is_alive(pid) + + def claim_blocks_web_start(state: dict) -> bool: """Whether a file claim should block a new web PTY/ACP session start. diff --git a/packages/studyloop/src/studyloop/web/routes/session/_ipc.py b/packages/studyloop/src/studyloop/web/routes/session/_ipc.py index acf6e937..2d379022 100644 --- a/packages/studyloop/src/studyloop/web/routes/session/_ipc.py +++ b/packages/studyloop/src/studyloop/web/routes/session/_ipc.py @@ -4,7 +4,7 @@ import logging -from studyloop.session_state import PARKING_FILE, STATE_FILE, TOPICS_FILE +from studyloop.session_state import PARKING_FILE, STATE_FILE, TOPICS_FILE, web_claim_is_orphaned logger = logging.getLogger(__name__) @@ -43,6 +43,13 @@ def _get_full_state() -> dict: f.unlink(missing_ok=True) return {"topics": [], "parking": []} + if web_claim_is_orphaned(state): + # A PTY/ACP session lives inside the server that started it, and that + # process is gone, so the session is too: report it ended, the verdict + # /session/start already reaches. Delete nothing -- a session that + # never ended never flushed its parking lot, and that is learner data. + state = {**state, "mode": "ended"} + topics = session_pkg.parse_topics_file() parking = session_pkg.parse_parking_file() return { diff --git a/packages/studyloop/src/studyloop/web/static/js/components/live-agent-console.js b/packages/studyloop/src/studyloop/web/static/js/components/live-agent-console.js index 6016fe23..cc10c453 100644 --- a/packages/studyloop/src/studyloop/web/static/js/components/live-agent-console.js +++ b/packages/studyloop/src/studyloop/web/static/js/components/live-agent-console.js @@ -812,16 +812,20 @@ export function liveAgentConsole(origin = 'study') { The ttyd iframe was retired once the pty path survived a page reload, and ttyd retirement stage 3 removed the server-side transport axis entirely — the server now rejects transport=ttyd/STUDYLOOP_TRANSPORT=ttyd outright. - This method is the generic "the server reported a transport this view - cannot render" fallback for any future unrecognised value, not a - ttyd-specific branch. */ + Two different failures land here, and the message must name the right + one: a KNOWN transport with no connection URL (the server gave nothing + to connect to), or a transport this view genuinely cannot render. + Telling a learner "cannot render pty" sent them looking for a renderer + problem that was not there. */ _mountUnavailable(detail) { this.terminalMode = 'unavailable'; this.connected = false; this.statusDot = 'error'; this.status = 'No terminal available'; - this.statusMessage = detail && detail.transport - ? `This session reports transport "${detail.transport}", which this view ` + const transport = detail && detail.transport; + const renderable = transport === 'pty' || transport === 'acp'; + this.statusMessage = transport && !renderable + ? `This session reports transport "${transport}", which this view ` + 'cannot render. End the session and start it again with the browser ' + 'terminal or ACP.' : 'The server did not return a connection for this session. Ending and ' diff --git a/packages/studyloop/tests/js/live-agent-console-unavailable.test.js b/packages/studyloop/tests/js/live-agent-console-unavailable.test.js new file mode 100644 index 00000000..659233c8 --- /dev/null +++ b/packages/studyloop/tests/js/live-agent-console-unavailable.test.js @@ -0,0 +1,41 @@ +/** + * The live console's "No terminal available" fallback must say what actually + * went wrong. A KNOWN transport ('pty', 'acp') arriving without a connection + * URL is a missing connection, not a transport this view cannot render; only + * an unrecognised transport gets the "cannot render" sentence. + * + * Found live: after `studyloop web` was restarted, a session whose server had + * stopped was adopted with transport "pty" and no URL, and the learner read + * 'This session reports transport "pty", which this view cannot render' — + * about the one transport the view renders with xterm. The "did not return a + * connection" sentence the troubleshooting guide describes for exactly this + * case was unreachable, because every caller passes a transport. + */ +// Run with: node --test 'packages/studyloop/tests/js/**/*.test.js' + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; + +import { liveAgentConsole } from + '../../src/studyloop/web/static/js/components/live-agent-console.js'; + +for (const transport of ['pty', 'acp']) { + test(`a known transport (${transport}) with no URL is reported as a missing connection`, () => { + const view = liveAgentConsole('study'); + + view.start({ transport, wsUrl: null, topic: 'Decorators', studySessionId: 's-1' }); + + assert.equal(view.terminalMode, 'unavailable'); + assert.doesNotMatch(view.statusMessage, /cannot render/); + assert.match(view.statusMessage, /did not return a connection/); + }); +} + +test('an unrecognised transport still names the transport it cannot render', () => { + const view = liveAgentConsole('study'); + + view.start({ transport: 'carrier-pigeon', wsUrl: '/api/session/ws?study_session_id=s-1' }); + + assert.equal(view.terminalMode, 'unavailable'); + assert.match(view.statusMessage, /transport "carrier-pigeon", which this view cannot render/); +}); diff --git a/packages/studyloop/tests/test_session_slot_reconcile.py b/packages/studyloop/tests/test_session_slot_reconcile.py index 06dee615..2acbd745 100644 --- a/packages/studyloop/tests/test_session_slot_reconcile.py +++ b/packages/studyloop/tests/test_session_slot_reconcile.py @@ -28,6 +28,7 @@ from __future__ import annotations import asyncio +import os import sys from pathlib import Path @@ -559,3 +560,81 @@ async def test_an_existing_session_state_is_still_marked_ended(self) -> None: _signal_dashboard_ended() assert session_state.read_session_state()["mode"] == "ended" + + +# --------------------------------------------------------------------------- +# Defect 4 — a web session whose server stopped is offered for reattach +# --------------------------------------------------------------------------- + +_DEAD_PID = 999_999_999 # beyond any pid the kernel hands out + + +class TestAWebSessionWhoseServerStoppedIsNotLive: + """A PTY/ACP session lives inside the server process that started it. + + When that server stops without ending the session, the state file still + says ``mode=focus`` and names the dead server's ``pid``. ``/session/start`` + already treats that claim as stale (``claim_blocks_web_start``), but this + endpoint reported it as live with no ``reattach_url``. After a restart of + ``studyloop web`` the console adopted it and told the learner it "cannot + render" transport pty, the one transport it renders with xterm. + """ + + def _write_orphan(self, **overrides: object) -> None: + session_state.write_session_state( + { + "study_session_id": "orphan-1", + "topic": "Decorators", + "mode": "focus", + "transport": "pty", + "origin": "study", + "pid": _DEAD_PID, + **overrides, + } + ) + + def test_state_reports_it_ended(self, client: TestClient) -> None: + self._write_orphan() + + state = client.get("/api/session/state").json() + + assert state.get("mode") == "ended", "a session whose server is gone was offered as live" + assert "reattach_url" not in state + + def test_an_acp_orphan_is_reported_ended_too(self, client: TestClient) -> None: + self._write_orphan(transport="acp") + + assert client.get("/api/session/state").json().get("mode") == "ended" + + def test_nothing_is_deleted(self, client: TestClient) -> None: + """A session that never ended never flushed its parking lot; those are + the learner's notes, so reporting it ended must not destroy them.""" + self._write_orphan() + session_state.PARKING_FILE.write_text("- a parked question\n") + + client.get("/api/session/state") + + assert session_state.STATE_FILE.exists() + assert session_state.PARKING_FILE.read_text() == "- a parked question\n" + + def test_a_live_foreign_server_s_session_is_left_alone(self, client: TestClient) -> None: + # pid 1 always exists and is foreign to the test process. + self._write_orphan(pid=1) + + assert client.get("/api/session/state").json().get("mode") == "focus" + + def test_this_process_s_own_claim_is_left_alone(self, client: TestClient) -> None: + """A start in flight writes its reservation with this process's pid + before the slot is acquired; that is not an orphan.""" + self._write_orphan(pid=os.getpid(), mode="starting") + + assert client.get("/api/session/state").json().get("mode") == "starting" + + def test_a_cli_session_is_left_alone(self, client: TestClient) -> None: + """A CLI session's pid is the CLI process, which can exit while its + multiplexer session lives on; the zombie rule judges CLI sessions.""" + session_state.write_session_state( + {"study_session_id": "cli-1", "topic": "tmux", "mode": "focus", "pid": _DEAD_PID} + ) + + assert client.get("/api/session/state").json().get("mode") == "focus"