diff --git a/AGENTS.md b/AGENTS.md index a89b96e..87dc4af 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,7 +89,11 @@ repo is in PLAN.md's "Backend layout" section. codepoint in the Kotlin that the script did not subset is a glyph that silently isn't there. Rerun the script and commit its output when adding one; it needs network access. `md-cog` and `md-refresh` are deliberately - the same codepoints dev-updater uses and must not drift from it. + the same codepoints dev-updater uses and must not drift from it. The + subset is the **Mono** face, where every glyph is one em square — that is + what makes two icon buttons the same width without either being given + one, and it is why `GLYPH_SIZE` is smaller than it looks like it should + be. - `.dev-updater.ron` — what Dev Updater is asked to do with this checkout: the server (built in `server/`, run as `service: Managed(...)`) and the APK (built in `app/`), built in parallel. The project it serves is the @@ -280,15 +284,21 @@ day: - **Stopping the server no longer stops the sessions.** After `pkill ai-server` the `claude` processes are still there, on purpose, and the next start picks them up (`reattaching to the claude-cli it left - running` in the log). To actually end one, delete the session — that is - the only path that stops a process. + running` in the log). To end one, either `POST /sessions/{id}/stop` — + which keeps the session and its transcript, and `POST .../start` brings + the process back on the same conversation — or delete the session, which + ends the conversation too. - **Each session directory now holds `process.json`, `stdin.fifo`, `stdout.log` and `stderr.log`.** `stdout.log` is the driver's input, read from the byte offset in `process.json`; removing either by hand while the session is live loses output or replays it. - **`--resume` only ever runs when nothing is running.** That check is the fix for the incident below, and the reason there is one entry point - (`ClaudeDriver::launch`) rather than a spawn and an attach. + (`ClaudeDriver::launch`) rather than a spawn and an attach. The status a + launch reports obeys the same rule: a session recorded as `exited` whose + launch has just started a process reports `idle`, because `exited` is the + word that refuses every command and offers a phone the chance to start a + second CLI on a live conversation. - Remote sessions are adopted too. The pid recorded for one is the **`ssh` client's**, on this machine — that is the process the backend owns, and it lives as long as the remote command does. (This said "local only" until diff --git a/PLAN.md b/PLAN.md index b4fbf1e..b57abc2 100644 --- a/PLAN.md +++ b/PLAN.md @@ -298,6 +298,51 @@ Two consequences worth stating: going away and means to come back) and `stop` (the session is being deleted, so the process must not survive). Every driver owes exactly one of them. +### Stopping and starting a session's process (decided 2026-08-30) + +If a session outlives the backend, the person holding the phone needs the +other direction too: **end the process without ending the session, and start +it again on the same conversation.** `POST /sessions/:id/stop` and +`/start`. + +Three decisions worth not undoing: + +- **Stop signals the recorded process and says nothing else.** It does not + go through the driver and it does not announce `Exited`. The record is the + session's rather than any dialect's, so signalling it here works for a + session whose driver is in no state to be asked and adds no trait method a + new driver could implement wrongly — and the driver's own reader already + reports the death correctly, draining the last output and recording the + status. Announcing it from here would be a guess arriving ahead of the + measurement, and wrong for the grace period a process that ignores SIGTERM + keeps running. +- **Start replaces the driver and nothing else.** The transcript, the event + pump and the SSE stream every open phone is reading stay where they were, + so starting a session again is not a reconnect for anybody watching, and + there is still exactly one writer of the transcript — which relaunching + the whole `LiveSession` would not be, since the old pump outlives its + session and an imported session's sync task would go on feeding it. + `LiveSession` and `Commands` therefore share one `Mutex>` + rather than each holding a copy. +- **Start is refused unless the session is *known* to have exited.** + `Unknown` means nobody could find out whether the process is alive, and + starting one on that is exactly the two-CLIs-on-one-conversation fault + `session::process` exists to prevent. + +That last rule found a real bug in the launch path, which is where the phone +would have hit it: a relaunched session took its status from the transcript, +so one whose process had died before a backend restart reported `Exited` +while the launch it had just gone through was starting a new process. The +status now says `Idle` when a launch leaves a process running — `Exited` +there is not merely stale, it is the word that refuses every command and +invites somebody to start a second process against a live conversation. + +On the phone this is one button in the composer, left of Send, whose mark and +colour say what pressing it would do now: an orange pause while a turn is +running (interrupt — the process stays), a red stop when it is not (end the +process), and a green play when it has exited (start it again). One button +rather than three that come and go, so its presence is never the signal. + ### Importing refuses a session that is already open (decided 2026-08-29) Claude Code keeps a descriptor per live session at @@ -448,7 +493,9 @@ POST /sessions spawn {provider, host, model, cwd, permiss GET /sessions/:id/events?after=N SSE: transcript replay from N, then live POST /sessions/:id/message {text, attachment_ids} POST /sessions/:id/answer {question_id, answer} (questions and permissions) -POST /sessions/:id/interrupt +POST /sessions/:id/interrupt stop the running turn; the process stays +POST /sessions/:id/stop end the process; the session and transcript stay +POST /sessions/:id/start run the process again, continuing the conversation POST /sessions/:id/model {model} POST /sessions/:id/compact (llama sessions) POST /sessions/:id/attachments multipart upload → id (referenced by /message) diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt index b572694..af14e79 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt @@ -558,6 +558,22 @@ fun interruptSession(settings: ServerSettings, sessionId: String) { requestFromServer(settings, "/sessions/$sessionId/interrupt", method = "POST") {} } +/** + * Ends the process behind a session, leaving the session and its transcript. + * + * Not a delete and not an interrupt: the conversation stays exactly where it is and [startSession] + * picks it back up. The server reports what it could not do -- there was nothing running, or the + * machine would not say whether there was -- rather than answering the same way either way. + */ +fun stopSession(settings: ServerSettings, sessionId: String) { + requestFromServer(settings, "/sessions/$sessionId/stop", method = "POST") {} +} + +/** Starts the process again on the conversation it left. See [stopSession]. */ +fun startSession(settings: ServerSettings, sessionId: String) { + requestFromServer(settings, "/sessions/$sessionId/start", method = "POST") {} +} + /** * Removes a Claude Code session from the machine. * diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/NerdIcons.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/NerdIcons.kt index 71f126b..7910a67 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/NerdIcons.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/NerdIcons.kt @@ -31,6 +31,12 @@ import androidx.compose.ui.unit.sp * tofu. Adding one means adding its codepoint in *both* places; a codepoint here that the script * did not subset is a glyph that silently isn't there. * + * The subset is the font's **Mono** face, where every glyph is exactly one em wide and one em tall. + * That is what makes two icons the same size without either of them being given a size: the + * proportional face's advances run from 0.46 em to 0.92 em, so a Send button and a Stop button side + * by side came out visibly different widths, and matching them at the call site would have meant + * one hardcoded measurement per pair. [GLYPH_SIZE] carries the cost. + * * The same arrangement as dev-updater, down to the cog and the refresh arrow being the same two * Material Design codepoints. Those two must not drift: an icon that means "settings" in one app * and something else in the other is the failure this is worth preventing. The script is copied @@ -52,9 +58,27 @@ val REFRESH_GLYPH = glyph(0xF0450) /** `md-send` -- the filled paper plane: submit what is in the composer. */ val SEND_GLYPH = glyph(0xF048A) -/** `md-stop` -- a filled square: interrupt the turn that is running. */ +/** + * `md-stop` -- a filled square: end the process behind this session. + * + * The square is what stop has meant since tape decks, and it is spent here on the thing that + * actually stops rather than on pausing. [PAUSE_GLYPH] is the turn; this is the session. + */ val STOP_GLYPH = glyph(0xF04DB) +/** + * `md-pause` -- two bars: take the running turn away and leave the session there. + * + * The pair with [STOP_GLYPH] and [PLAY_GLYPH] is the point: one button in the composer says what + * pressing it now would do to the process, and the three marks are the three answers. An interrupt + * ends a turn and nothing else -- the CLI is still there and still holds the conversation -- which + * is a pause, not a stop, and drawing it as a square said otherwise. + */ +val PAUSE_GLYPH = glyph(0xF03E4) + +/** `md-play` -- start the process again, on the conversation it left. See [PAUSE_GLYPH]. */ +val PLAY_GLYPH = glyph(0xF040A) + /** * `md-send_clock` -- the same paper plane with a clock on it: this message will wait its turn. * @@ -142,5 +166,13 @@ fun Glyph( Text(glyph, fontFamily = NerdIcons, fontSize = size, color = colour, modifier = modifier) } -/** The size an icon draws at beside a line of text. */ -private val GLYPH_SIZE = 20.sp +/** + * The size an icon draws at beside a line of text. + * + * 17 rather than the 20 it was while the font was the proportional face. A glyph there filled at + * most 0.83 em of its point size and most filled a good deal less, so the number was standing in + * for the headroom above the tallest one; in the Mono face every glyph fills its em exactly, and + * keeping 20 would have made every icon in the app step up by a fifth for no reason anybody asked + * for. This is what the largest of them already drew at. + */ +private val GLYPH_SIZE = 17.sp diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt index 5a1c4c0..07edf36 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt @@ -1358,30 +1358,44 @@ fun SessionScreen( }, ) } - if (running) { - // The same filled shape as the button beside it, not an outlined one: these - // are two things you can do about the turn that is running, and weighting one - // of them as secondary said they were a primary action and its qualifier. - // What separates them is the colour and the mark, which is what they mean. - Button( - onClick = { act { interruptSession(settings, summary.id) } }, - colors = actionButtonColors(stopColor), - ) { - // A filled square, which is what stop has looked like since tape decks. - Glyph( - STOP_GLYPH, - colour = LocalContentColor.current, - modifier = Modifier.semantics { contentDescription = "Stop" }, - ) + // The same filled shape as the button beside it, not an outlined one: these are + // two things you can do about the session, and weighting one of them as secondary + // said they were a primary action and its qualifier. What separates them is the + // colour and the mark, which is what they mean. + // + // Always here, rather than arriving with the turn as it used to. A control that + // comes and goes makes its own presence the signal, and its absence could not say + // whether there was nothing to do; a button that is always in the same place also + // cannot push Send off the end of the row by turning up. + val process = + when { + running -> ProcessAction.Pause + status == "exited" -> ProcessAction.Start + else -> ProcessAction.Stop } - Spacer(Modifier.width(8.dp)) + Button( + onClick = { act { process.perform(settings, summary.id) } }, + colors = actionButtonColors(process.colour()), + ) { + Glyph( + process.glyph, + colour = LocalContentColor.current, + modifier = Modifier.semantics { contentDescription = process.label }, + ) } + Spacer(Modifier.width(8.dp)) // The paper plane, with a clock on it while a turn is in flight: sending then // queues the message for the next tool boundary rather than starting a turn of // its own, and the two have to be told apart at a glance. The label says the same // thing to a screen reader, which has nothing else to read. + // + // Disabled while there is nothing to send, rather than pressable and silent: + // `send` has always returned early on an empty composer, so the button promised + // something it would not do, and the only feedback was the ripple. Disabled and + // not hidden, for the reason the button beside it is always here. Button( onClick = { send() }, + enabled = input.isNotBlank() || pendingAttachments.isNotEmpty(), colors = actionButtonColors(if (running) queueColor else sendColor), ) { Glyph( @@ -1402,6 +1416,39 @@ fun SessionScreen( /** What pressing Send does right now, said the same way to the eye and to a screen reader. */ private fun sendLabel(running: Boolean) = if (running) "Queue" else "Send" +/** + * What the composer's process button would do if it were pressed now. + * + * One value rather than four parallel conditions over the status, because the mark, the colour, the + * name a screen reader is given and the request that goes out are four halves of one decision. A + * button drawn as a pause that terminates the CLI is the worst bug available here, and separate + * branches over the same condition are how that happens -- these three each have to cover every + * case, and the compiler says so. + */ +private enum class ProcessAction(val glyph: String, val label: String) { + /** A turn is running: take it back, and leave the process holding the conversation. */ + Pause(PAUSE_GLYPH, "Pause"), + /** Nothing is running, but the process behind the session is: end it. */ + Stop(STOP_GLYPH, "Stop"), + /** The process is gone: start it again, on the conversation it left. */ + Start(PLAY_GLYPH, "Start"), +} + +@Composable +private fun ProcessAction.colour() = + when (this) { + ProcessAction.Pause -> pauseColor + ProcessAction.Stop -> stopColor + ProcessAction.Start -> startColor + } + +private fun ProcessAction.perform(settings: ServerSettings, sessionId: String) = + when (this) { + ProcessAction.Pause -> interruptSession(settings, sessionId) + ProcessAction.Stop -> stopSession(settings, sessionId) + ProcessAction.Start -> startSession(settings, sessionId) + } + /** * An inline transcript image, fetched (authenticated, pinned) from the session's files route. The * bitmap is remembered per ref, so scrolling doesn't refetch. diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt index 18146c6..9d084dc 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt @@ -213,13 +213,14 @@ val overLimitColor: Color @Composable get() = MaterialTheme.colorScheme.error /** - * The composer's three buttons, coloured by what pressing one does rather than by where it sits. + * The composer's buttons, coloured by what pressing one does rather than by where it sits. * - * Green sends now, blue sends later, red takes the running turn away. The pair of greens and the - * pair of reds elsewhere in this file are deliberate near-collisions worth naming: [runningColor] - * is green because a session is working, and [failedColor] is red because one fell over -- those - * are *states*, and these are *actions*. A reader never has to tell them apart, because nothing - * here is a state and nothing there is pressable. + * Green makes something happen now, blue makes it happen later, orange takes back what is in + * flight, red ends the process. The near-collisions with the states above are deliberate and worth + * naming rather than collapsing: [runningColor] is green because a session is working, + * [failedColor] is red because one fell over, [awaitingColor] is the same orange because a session + * is waiting on somebody -- those are *states*, and these are *actions*. A reader never has to tell + * them apart, because nothing here is a state and nothing there is pressable. */ val sendColor: Color @Composable get() = Mocha.Green @@ -228,10 +229,30 @@ val sendColor: Color val queueColor: Color @Composable get() = Mocha.Blue -/** Interrupting the running turn -- the one button here that takes something away. */ +/** + * Interrupting the running turn: the work stops and the session stays. + * + * Orange rather than red because of how much it takes: only what is in flight. The process is still + * there holding the conversation, and the next message starts a turn as though nothing had + * happened. Red is spent on [stopColor], which is the same button in the same place when what it + * would end is the session's process. + */ +val pauseColor: Color + @Composable get() = Mocha.Peach + +/** Ending the session's process -- the one button here that takes something away. */ val stopColor: Color @Composable get() = Mocha.Red +/** + * Starting the process again, on the conversation it left. + * + * The same green as [sendColor] on purpose: both mean "this happens now", and they are never the + * same button -- the process button only offers to start when there is nothing running to stop. + */ +val startColor: Color + @Composable get() = Mocha.Green + /** * A filled button in one of the action colours above. * diff --git a/app/androidApp/src/main/res/font/nerd_icons.ttf b/app/androidApp/src/main/res/font/nerd_icons.ttf index ef7791a..2001639 100644 Binary files a/app/androidApp/src/main/res/font/nerd_icons.ttf and b/app/androidApp/src/main/res/font/nerd_icons.ttf differ diff --git a/app/build-icon-font.sh b/app/build-icon-font.sh index 873af93..a04c4e9 100755 --- a/app/build-icon-font.sh +++ b/app/build-icon-font.sh @@ -32,6 +32,8 @@ GLYPHS=( U+F0450 # md-refresh U+F048A # md-send U+F04DB # md-stop + U+F03E4 # md-pause + U+F040A # md-play U+F1163 # md-send_clock U+F0156 # md-close U+F004D # md-arrow_left @@ -52,9 +54,20 @@ python3 -m venv "$work/venv" unicodes="$(IFS=,; echo "${GLYPHS[*]}")" mkdir -p "$(dirname "$out")" -# The proportional face rather than the Mono one: these are drawn inline -# beside text, where a fixed advance would pad each icon out to a cell. -"$work/venv/bin/pyftsubset" "$work/SymbolsNerdFont-Regular.ttf" \ +# The Mono face rather than the proportional one, which this used until +# 2026-08-30. Every glyph in it is one em wide and one em tall, so two +# icons drawn at the same size are the same size -- which is what makes two +# icon buttons beside each other match without either of them being told a +# width. In the proportional face the advances run from 0.46 em (play) to +# 0.92 em (line chart), so the composer's Send button came out visibly wider +# than the Stop button next to it, and any fix at the call site would have +# been one measurement hardcoded per pair. +# +# The trade is the one the old comment named: an icon inline beside text is +# padded out to a cell. That is worth it, and it is also why GLYPH_SIZE in +# NerdIcons.kt came down when this changed -- a glyph that fills its em +# draws bigger at the same point size than one that does not. +"$work/venv/bin/pyftsubset" "$work/SymbolsNerdFontMono-Regular.ttf" \ --unicodes="$unicodes" \ --layout-features= \ --drop-tables+=DSIG \ diff --git a/server/src/routes.rs b/server/src/routes.rs index 247a7f6..a369fff 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -17,7 +17,9 @@ //! `reset` frame plus the newest window) //! POST /sessions/{id}/message {text, attachmentIds?} //! POST /sessions/{id}/answer {questionId, answers} (questions and permissions) -//! POST /sessions/{id}/interrupt +//! POST /sessions/{id}/interrupt stop the running turn; the process stays +//! POST /sessions/{id}/stop end the process; the session and transcript stay +//! POST /sessions/{id}/start run the process again, continuing the conversation //! POST /sessions/{id}/title {title} //! POST /sessions/{id}/model {model} //! POST /sessions/{id}/command {text} -- /compact, /clear, /rename x, or the dialect's own @@ -87,6 +89,8 @@ pub fn router(manager: Arc) -> Router { .route("/sessions/{id}/message", post(message)) .route("/sessions/{id}/answer", post(answer)) .route("/sessions/{id}/interrupt", post(interrupt)) + .route("/sessions/{id}/stop", post(stop)) + .route("/sessions/{id}/start", post(start)) .route("/sessions/{id}/title", post(rename)) .route("/sessions/{id}/model", post(set_model)) .route("/sessions/{id}/permission-mode", post(set_permission_mode)) @@ -678,6 +682,31 @@ async fn interrupt( Ok(StatusCode::NO_CONTENT) } +/// Ends the session's process. The session stays, and `start` brings it +/// back -- see [`SessionManager::stop_session`]. +/// +/// Not `lookup`ed: a session that failed to relaunch has no live entry and +/// may still have a process running, which is exactly one worth being able +/// to stop. +async fn stop( + State(manager): State>, + UrlPath(id): UrlPath, +) -> Result { + manager.stop_session(&id).map_err(bad_request)?; + Ok(StatusCode::NO_CONTENT) +} + +/// Starts a process for a session that has none, continuing the same +/// conversation -- see [`SessionManager::start_session`], which refuses +/// unless the session is known to have exited. +async fn start( + State(manager): State>, + UrlPath(id): UrlPath, +) -> Result { + manager.start_session(&id).map_err(bad_request)?; + Ok(StatusCode::NO_CONTENT) +} + /// The usage screen needs two things that live in different places: the /// cache, and the current list of machines to ask. Carried together rather /// than the monitor holding the manager, which would point the dependency diff --git a/server/src/session/claude.rs b/server/src/session/claude.rs index 2dace08..af43fab 100644 --- a/server/src/session/claude.rs +++ b/server/src/session/claude.rs @@ -100,12 +100,6 @@ pub(super) mod translate; const RESUME_FILE: &str = "claude-session.json"; -/// Grace period between asking a process to stop and killing it. -/// -/// Only [`Driver::stop`] uses it -- a session being deleted. Detaching -/// does not stop anything, so it has no grace period and needs none. -const STOP_GRACE: std::time::Duration = std::time::Duration::from_secs(5); - /// Messages handed to the CLI that it has not visibly acted on yet. /// /// The CLI *does* take a message written mid-turn: it goes into the next @@ -673,7 +667,7 @@ impl Driver for ClaudeDriver { fn stop(&self) { self.reading.store(false, Ordering::SeqCst); if let Some(record) = process::live(&self.session_dir) { - process::stop(&record, STOP_GRACE); + process::stop(&record, process::STOP_GRACE); } process::clear(&self.session_dir); } diff --git a/server/src/session/llama.rs b/server/src/session/llama.rs index f65b669..9c5f3a0 100644 --- a/server/src/session/llama.rs +++ b/server/src/session/llama.rs @@ -269,9 +269,6 @@ const SERVER_LOG: &str = "llama-server.log"; /// seconds late costs nothing. const WATCH_INTERVAL: std::time::Duration = std::time::Duration::from_secs(2); -/// Grace period between asking llama-server to stop and killing it. -const STOP_GRACE: std::time::Duration = std::time::Duration::from_secs(5); - /// An owner-only log opened for appending, so the two streams pointed at /// it do not overwrite each other and a reattach keeps what came before. fn log_file(path: &Path) -> Result { @@ -447,7 +444,7 @@ impl Driver for LlamaDriver { fn stop(&self) { self.cancel.store(true, Ordering::Relaxed); if let Some(record) = process::live(&self.session_dir) { - process::stop(&record, STOP_GRACE); + process::stop(&record, process::STOP_GRACE); } process::clear(&self.session_dir); } diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index b53fc0d..07f1ae4 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -169,11 +169,21 @@ pub struct SessionInfo { pub created: f64, } +/// What is running a session at this moment. +/// +/// Behind a lock because a session outlives its process: stopping one and +/// starting it again replaces the driver while the transcript, the event +/// pump and the stream every open phone is reading stay exactly where they +/// were. Shared with [`Commands`] rather than copied into it, because two +/// holders of "the driver" are two answers to that question the moment one +/// of them is replaced. +type DriverCell = Arc>>; + /// A running session: its driver plus the shared state the event pump /// keeps current. Cheap to clone-by-`Arc` into request handlers. pub struct LiveSession { meta: SessionConfig, - driver: Arc, + driver: DriverCell, /// Commands asked for and not yet run, oldest first, with the pump /// that will run them. Shared with that pump, which is what notices /// the boundary. @@ -195,12 +205,17 @@ pub struct LiveSession { /// for the turn to end. Drivers therefore never have to think about it, /// and a new provider cannot get it wrong by omission. struct Commands { - driver: Arc, + driver: DriverCell, sink: EventSink, waiting: Mutex>, } impl Commands { + /// Whatever is driving the session now -- see [`DriverCell`]. + fn driver(&self) -> Arc { + self.driver.lock().unwrap().clone() + } + /// Runs `command` now if the session is between turns, holds it until /// it is, and refuses it outright if there will never be one. Whichever /// happened, the phone is told. @@ -236,9 +251,10 @@ impl Commands { }); return; } - if self.driver.between_turns() { + let driver = self.driver(); + if driver.between_turns() { let _ = self.sink.send(Event::CommandSent { id, text }); - command.apply(self.driver.as_ref()); + command.apply(driver.as_ref()); return; } let _ = self.sink.send(Event::CommandQueued { @@ -258,7 +274,8 @@ impl Commands { /// began by itself, which it does: a background task finishing makes it /// pick the conversation back up with nothing written to it. fn take_one(&self) { - if !self.driver.between_turns() { + let driver = self.driver(); + if !driver.between_turns() { return; } let Some((id, command)) = self.waiting.lock().unwrap().pop_front() else { @@ -268,7 +285,7 @@ impl Commands { id, text: command.label(), }); - command.apply(self.driver.as_ref()); + command.apply(driver.as_ref()); } /// Gives up on everything held, because the session cannot run them. @@ -336,6 +353,11 @@ struct Shared { } impl LiveSession { + /// Whatever is driving this session now -- see [`DriverCell`]. + fn driver(&self) -> Arc { + self.driver.lock().unwrap().clone() + } + /// Hands the user's message to the driver, which records it in the /// transcript by reporting that it has taken it -- see `MessageTaken`. /// @@ -348,7 +370,7 @@ impl LiveSession { // drew a person's screenshot as a row floating above the bubble // that sent it, and left the phone inferring from adjacency which // message an image went with -- a thing the sender already knew. - self.driver.send_user_message(text, images); + self.driver().send_user_message(text, images); } pub fn answer_question(&self, question_id: &str, answers: &[String]) { @@ -356,7 +378,7 @@ impl LiveSession { id: question_id.to_string(), answers: answers.to_vec(), }); - self.driver.answer_question(question_id, answers); + self.driver().answer_question(question_id, answers); } /// Asks the session to run a command on itself, now or at the next @@ -367,14 +389,14 @@ impl LiveSession { } pub fn interrupt(&self) { - self.driver.interrupt(); + self.driver().interrupt(); } /// Leaves this session's process running and stops attending to it, /// for a server that is going away and means to come back. See /// [`Driver::detach`]. pub fn detach(&self) { - self.driver.detach(); + self.driver().detach(); } /// Compacts at the next boundary. Through the command queue like @@ -942,7 +964,7 @@ impl SessionManager { // change, and as an error if it cannot. The config above is a // different question -- what to launch this session with next // time -- and it is answered by the request. - session.driver.set_permission_mode(mode); + session.driver().set_permission_mode(mode); } Ok(()) } @@ -1025,11 +1047,141 @@ impl SessionManager { if let Some(session) = inner.live.get(id) { // See `set_session_permission_mode`: the driver reports what // it is set to, this only asks. - session.driver.set_model(model); + session.driver().set_model(model); } Ok(()) } + /// Ends this session's process, leaving the session -- its transcript, + /// its place in the list, everything a phone is watching -- exactly + /// where it is. [`SessionManager::start_session`] is the way back. + /// + /// The signal is all this does. Whether the process actually went, what + /// it said on the way out, and the `Exited` that follows are reported by + /// the path a session that died on its own already takes: the driver's + /// own reader notices within a poll, drains what was still unread, and + /// records it. Announcing it from here would be this side's guess + /// arriving ahead of the measurement, and it would be wrong for the five + /// seconds a process that ignores SIGTERM keeps running. + /// + /// Deliberately not routed through the driver. The record is the + /// session's rather than any dialect's -- `session::process` writes it + /// for every provider that has a process at all -- so asking it here + /// stops a session whose driver is in no state to be asked, and adds no + /// method a new driver could implement wrongly. + pub fn stop_session(&self, id: &str) -> Result<()> { + if !self + .inner + .read() + .unwrap() + .config + .sessions + .iter() + .any(|meta| meta.id == id) + { + bail!("no session {id}"); + } + // Three answers, and they are three different things to tell + // somebody: it is running (stop it), it is not (nothing to do), and + // nobody could find out (nothing was signalled, and saying "nothing + // is running" would be inventing the answer). + let record = match process::recorded(&self.data_dir.join(id)) { + Some((record, process::Liveness::Alive)) => record, + Some((_, process::Liveness::Unknown)) => bail!( + "this machine won't say whether this session's process is still running, so it \ + wasn't signalled" + ), + Some((_, process::Liveness::Dead)) | None => { + bail!("this session has no process running") + } + }; + tracing::info!("stopping session {id} (pid {})", record.pid); + process::stop(&record, process::STOP_GRACE); + Ok(()) + } + + /// Starts a process for a session whose process has ended, continuing + /// the same conversation -- for Claude Code, the `--resume` that crash + /// recovery already uses. + /// + /// Only the driver is new. The transcript, the event pump and the stream + /// every open phone is reading stay as they were, so this is not a + /// reconnect for anybody watching -- and there is still exactly one + /// writer of the transcript, which relaunching the whole session would + /// not be. + /// + /// Refused unless the session is *known* to have exited. `Unknown` means + /// nobody could find out whether the process is alive, and starting one + /// on that is precisely the second-CLI-on-one-conversation fault that + /// `session::process` exists to prevent. + pub fn start_session(&self, id: &str) -> Result<()> { + let mut inner = self.inner.write().unwrap(); + let meta = inner + .config + .sessions + .iter() + .find(|meta| meta.id == id) + .with_context(|| format!("no session {id}"))? + .clone(); + let existing = inner.live.get(id).cloned(); + let status = match &existing { + Some(session) => *session.shared.status.lock().unwrap(), + None => status_of_unlaunched(&self.data_dir.join(id)), + }; + match status { + SessionStatus::Exited => {} + SessionStatus::Unknown => bail!( + "this machine won't say whether this session's process is still running, so \ + nothing was started" + ), + _ => bail!("this session is already running"), + } + // Fresh from the config, like every other launch: a model or a + // permission mode changed while the session was stopped is what it + // starts with. + let (setup, provider) = resolve(&inner.config, &meta)?; + let session = match existing { + Some(session) => { + *session.driver.lock().unwrap() = make_driver( + &meta, + &setup, + &provider, + &self.models_dir, + session.dir(), + session.transcript_path(), + &session.sink, + )?; + session + } + // Nothing is live for this one -- a session whose launch failed + // when the server started, which has no pump either. That is the + // whole of `launch`, and the same call the server start makes. + None => { + let session = launch( + meta, + &setup, + &provider, + &self.data_dir, + &self.models_dir, + None, + self.notifications.clone(), + )?; + inner.live.insert(id.to_string(), Arc::clone(&session)); + session + } + }; + // The recorded status is `Exited` and this has just made it untrue. + // Said here because nothing else will say it: a CLI that has been + // given no work writes nothing, so the session would sit at + // `Exited` -- refusing every command, refusing every message, and + // showing a phone an offer to start a second process against the + // conversation this one is already running. + let _ = session.sink.send(Event::Status { + state: SessionStatus::Idle, + }); + Ok(()) + } + /// Kills the process, releases everything the spawn created, and /// deletes the transcript and files -- the complete path out. pub fn delete_session(&self, id: &str) -> Result<()> { @@ -1045,7 +1197,7 @@ impl SessionManager { // Stopped, not detached: this is the one exit where the // process must not survive, because the conversation it // belongs to is being removed. See `Driver::stop`. - session.driver.stop(); + session.driver().stop(); } let dir = self.data_dir.join(id); if dir.exists() { @@ -1339,25 +1491,38 @@ fn launch( ); } - let driver: Arc = match provider.kind { - DriverKind::Echo => Arc::new(EchoDriver::new(sink.clone(), dir.clone())), - DriverKind::LlamaCpp => Arc::new(LlamaDriver::launch( - &meta, - provider, - &Transport::for_setup(setup), - models_dir, - &transcript_path, - &dir, - sink.clone(), - )?), - DriverKind::ClaudeCli => Arc::new(ClaudeDriver::launch( - &meta, - provider, - &Transport::for_setup(setup), - &dir, - sink.clone(), - )?), - }; + let driver = Arc::new(Mutex::new(make_driver( + &meta, + setup, + provider, + models_dir, + &dir, + &transcript_path, + &sink, + )?)); + + // The transcript's last word is what this session was doing when + // something was last watching it, and building the driver above may have + // just made it untrue: a session recorded as `Exited` with a process + // running again is one this launch has started. Carrying `Exited` + // forward is not merely stale -- it is the word that refuses every + // command sent to the session, and the word that invites somebody to + // start a *second* process against a conversation that already has one, + // which is the fault `session::process` exists to prevent. It reached a + // phone as a play button on a session that was already running. + // + // Idle is what is true: there is a process, and nothing has asked it for + // anything. Written rather than announced, because nobody watched a + // transition -- this is the state the session is being restored in, and + // an event would put a status change in the transcript that never + // happened. A record nobody could check stays as it was and is corrected + // by the driver's first poll, which reports `Unknown` for it. + { + let mut status = shared.status.lock().unwrap(); + if *status == SessionStatus::Exited && process::live(&dir).is_some() { + *status = SessionStatus::Idle; + } + } let commands = Arc::new(Commands { driver: Arc::clone(&driver), @@ -1386,6 +1551,44 @@ fn launch( })) } +/// Whatever runs this session's provider, pointed at the session's own +/// directory and reporting into `sink`. +/// +/// Split out of [`launch`] because a session outlives its process: it is +/// also what [`SessionManager::start_session`] builds when somebody starts a +/// stopped session again. That path replaces the driver and nothing else, so +/// it has to construct one the same way rather than becoming a second answer +/// to "what runs this". +fn make_driver( + meta: &SessionConfig, + setup: &SetupConfig, + provider: &ProviderConfig, + models_dir: &Path, + dir: &Path, + transcript_path: &Path, + sink: &EventSink, +) -> Result> { + Ok(match provider.kind { + DriverKind::Echo => Arc::new(EchoDriver::new(sink.clone(), dir.to_path_buf())), + DriverKind::LlamaCpp => Arc::new(LlamaDriver::launch( + meta, + provider, + &Transport::for_setup(setup), + models_dir, + transcript_path, + dir, + sink.clone(), + )?), + DriverKind::ClaudeCli => Arc::new(ClaudeDriver::launch( + meta, + provider, + &Transport::for_setup(setup), + dir, + sink.clone(), + )?), + }) +} + /// The one writer of a session's transcript: assigns sequence numbers, /// appends, updates the shared status/activity view, fans out. Ends when /// every sender is dropped -- i.e. when the session is deleted and its @@ -1639,7 +1842,10 @@ mod tests { let dir = tempfile::tempdir().expect("tempdir"); let (sink, mut events) = mpsc::unbounded_channel(); let commands = Commands { - driver: Arc::new(EchoDriver::new(sink.clone(), dir.path().to_path_buf())), + driver: Arc::new(Mutex::new(Arc::new(EchoDriver::new( + sink.clone(), + dir.path().to_path_buf(), + )))), sink, waiting: Mutex::new(VecDeque::new()), }; @@ -2233,6 +2439,70 @@ mod tests { std::fs::write(path, rewritten).expect("write transcript"); } + /// Stopping and starting a session is about its *process*, and the two + /// refusals are the whole of what keeps starting one from becoming a + /// second one on the same conversation. + /// + /// Echo has no process, which makes it the right session to ask the + /// first question of: "there is nothing to stop" is an answer, and + /// reporting success would leave a phone showing a session it believes + /// it stopped. The second question is asked of a session that has been + /// told it exited, since the guard is on the *status* rather than on + /// which driver it is. + #[tokio::test] + async fn a_session_is_started_again_only_once_it_is_known_to_have_exited() { + let dir = tempfile::tempdir().expect("tempdir"); + let config_path = dir.path().join("config.ron"); + let data_dir = dir.path().join("sessions"); + seed_echo_only(&config_path); + let manager = SessionManager::new(config_path, data_dir.clone(), data_dir.join("models")) + .expect("manager"); + let info = manager.spawn_session(echo_spec()).expect("spawn"); + let session = manager.session(&info.id).expect("live session"); + let mut rx = session.subscribe(); + + let refused = manager.stop_session(&info.id).expect_err("nothing to stop"); + assert!( + refused.to_string().contains("no process running"), + "said: {refused:#}" + ); + let refused = manager + .start_session(&info.id) + .expect_err("already running"); + assert!( + refused.to_string().contains("already running"), + "said: {refused:#}" + ); + + // What a driver reports when its process goes, without a process + // to go: the guard reads the recorded status, so this is the same + // state a stopped claude session reaches. + let _ = session.sink.send(Event::Status { + state: SessionStatus::Exited, + }); + collect_until(&mut rx, |event| { + matches!( + event, + Event::Status { + state: SessionStatus::Exited + } + ) + }) + .await; + + manager.start_session(&info.id).expect("start again"); + collect_until(&mut rx, is_idle).await; + // Idle rather than exited, and said by the start rather than left + // for a driver that has been given no work to say for itself. + assert_eq!(manager.sessions()[0].status, SessionStatus::Idle); + // The same live session throughout: only the driver was replaced, + // so nothing a phone is reading was interrupted. + assert!(Arc::ptr_eq( + &session, + &manager.session(&info.id).expect("still live") + )); + } + #[tokio::test] async fn a_restart_relaunches_sessions_and_continues_the_numbering() { let dir = tempfile::tempdir().expect("tempdir"); diff --git a/server/src/session/process.rs b/server/src/session/process.rs index d2f2a26..f2cc3cb 100644 --- a/server/src/session/process.rs +++ b/server/src/session/process.rs @@ -204,6 +204,13 @@ pub fn clear(session_dir: &Path) { } } +/// Grace period between asking a session's process to stop and killing it. +/// +/// Here rather than beside each caller: it is a property of stopping one of +/// these, and two drivers plus the manager had written the same five seconds +/// down separately, which is three places for it to drift. +pub const STOP_GRACE: std::time::Duration = std::time::Duration::from_secs(5); + /// Asks it to stop, then makes sure. Used where a leaked process must /// actually end: a deleted session, or one being replaced. ///