From 50f9b956d9dc3a67afcaa9ceaf25582daf56d3cc Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 30 Aug 2026 13:40:54 -0400 Subject: [PATCH 1/4] Check "exited" against the process record before believing it A session adopted at a backend start keeps the transcript's last status, so one whose process had been reported gone and was then found again read as `exited` while its CLI was running. `exited` is the word that draws the phone's Start button and lets `start_session` build a driver, so Start was accepted every time it was pressed -- and since starting replaces the driver without retiring the old one, each press left another reader on the same process. Every line the CLI wrote was then translated once per reader: three presses put three interleaved copies of one reply on screen, which is what it was reported as. So `exited` is now checked against `session::process`, the one authority on whether a process exists, in `launch` and again in `start_session`. A record that is not known to be dead makes it false, and what replaces it is `unknown` -- there is a process, and nothing here has heard from it, which is the answer `status_of_unlaunched` already gave to the same question. The correction goes out through the sink rather than into the manager's view alone, or the list and the session screen would disagree about it in the way this same button did a commit ago. A driver that `start_session` replaces now gets `Driver::detach`, which already existed for the backend going away and is the whole of what a driver whose process has exited is owed. On the phone the process button is disabled while its own request is in flight, so a second press cannot be decided against a status the first has not changed yet. That is a courtesy rather than the fix; the server refuses it either way, because a phone that has lost the stream cannot be relied on to know. Verified against a stand-in CLI, with the state forced by hand: before, three Starts returned 204 and left four readers on one process and the status still `exited`; after, the session reports `unknown` on both surfaces and all three are refused. Then driven on the emulator -- Stop, Start, Stop, Start alternated correctly with one process at a time, and the list, the transcript and the record all agree. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 11 ++ PLAN.md | 30 ++++ .../kotlin/com/example/aiapp/SessionScreen.kt | 21 ++- server/src/session/mod.rs | 158 +++++++++++++++++- 4 files changed, 210 insertions(+), 10 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7c5782e..96eccf9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -302,6 +302,17 @@ day: 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. +- **`exited` is never taken on trust; it is checked against the process + record** (`corrected` in `session/mod.rs`). It is the one status that draws + the phone's Start button and lets `start_session` build a driver, so a + record that is not known to be dead makes it false and the session reports + `unknown` instead. Without that, a session adopted at a backend start kept + the transcript's `exited` while its CLI was running, Start was accepted + every press, and each press left another reader on the same process — + which reads on screen as one reply written several times, interleaved + (`GotGotGot it — it — it —`), not as anything to do with a button. + A driver that `start_session` replaces gets `Driver::detach` for the same + reason: swapping the `Arc` does not end the tasks the old one is running. - 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 d2be19c..d069af7 100644 --- a/PLAN.md +++ b/PLAN.md @@ -351,6 +351,36 @@ better answer until its output says otherwise. Coming from the driver also orders it against the exit `follow` reports, which a status written from the manager could not be. +**`Exited` is a claim about a process, and the record is what settles it.** +Adopting saying nothing left one word standing that a live process +contradicts. A session whose process was reported gone and then found again +at the next backend start kept `Exited` from the transcript — and `Exited` is +the word that draws a Start button. Start was then accepted every time it was +pressed, and since starting replaces the driver, each press attached *another* +reader to the one process: every line the CLI wrote was translated once per +reader, so three presses put three interleaved copies of one reply on screen. +Two rules come out of it, and neither is optional: + +- **`Exited` is checked against `session::process` before it is believed** — + `corrected`, called in `launch` and again in `start_session`. A record that + is not known to be dead makes it false, and what replaces it is `Unknown`: + there is a process, and nothing here has heard from it, which is the answer + `status_of_unlaunched` already gave to the same question. Every other status + is left exactly as it was — those are the pump's, written from what the + process itself said, and none of them authorises starting anything. The + correction goes out through the sink for the reason above: written into the + manager's view alone it would be the list and the screen disagreeing again. +- **A driver that is replaced is detached.** Swapping the `Arc` does not end + the tasks the old one is running. `Driver::detach` is what does — it already + existed for the backend going away — and it is the whole of what a driver + whose process has exited is owed. + +The phone's half is that the process button is disabled while its own request +is in flight, so a second press cannot be decided against a status the first +one has not changed yet. That is a courtesy rather than the fix: the server +refuses the second request either way, because a phone that has lost the +stream cannot be relied on to know. + 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 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 567a0cd..9bb7500 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt @@ -555,6 +555,12 @@ fun SessionScreen( var compactingFor by remember { mutableStateOf(null) } var streamError by remember { mutableStateOf(null) } var actionError by remember { mutableStateOf(null) } + // Whether the composer's process button has a request out. What it does next is decided from + // the session's status, and the status only changes once the server has answered and the + // stream has carried it back -- so two presses in that gap are two requests, both decided + // against the state before either of them. The server refuses the second one, but a control + // that can be pressed while its own last press is still in flight is asking to be. + var processInFlight by remember { mutableStateOf(false) } val context = LocalContext.current // Seeded from what was left in the box last time and written back on every keystroke, so // leaving the screen -- or the system reclaiming the app -- does not throw away a half-typed @@ -997,7 +1003,7 @@ fun SessionScreen( } } - fun act(onFailure: () -> Unit = {}, action: () -> Unit) { + fun act(onFailure: () -> Unit = {}, onDone: () -> Unit = {}, action: () -> Unit) { scope.launch { try { withContext(Dispatchers.IO) { action() } @@ -1005,6 +1011,11 @@ fun SessionScreen( } catch (e: ApiException) { actionError = e.message onFailure() + } finally { + // Whatever happened, including the failure above: a caller that re-enables a + // control here must get it back on the path where the request was refused too, + // or the refusal is what disables the control permanently. + onDone() } } } @@ -1503,7 +1514,13 @@ fun SessionScreen( else -> ProcessAction.Stop } Button( - onClick = { act { process.perform(settings, summary.id) } }, + onClick = { + processInFlight = true + act(onDone = { processInFlight = false }) { + process.perform(settings, summary.id) + } + }, + enabled = !processInFlight, colors = actionButtonColors(process.colour()), ) { Glyph( diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index f29f5eb..f4fe012 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -1132,16 +1132,30 @@ impl SessionManager { .with_context(|| format!("no session {id}"))? .clone(); let existing = inner.live.get(id).cloned(); + let dir = self.data_dir.join(id); let status = match &existing { - Some(session) => *session.shared.status.lock().unwrap(), - None => status_of_unlaunched(&self.data_dir.join(id)), + Some(session) => { + let last = *session.shared.status.lock().unwrap(); + let now = corrected(last, &dir); + if now != last { + // Published, not merely acted on. The phone is drawing a + // Start button on the strength of the word this has just + // disproved, and it learns what a session is doing from + // the stream like everything else -- so a correction + // nobody sends leaves that button there to be pressed + // again, and again. Through the sink, which keeps the + // pump the only writer of the status. + let _ = session.sink.send(Event::Status { state: now }); + } + now + } + None => status_of_unlaunched(&dir), }; match status { SessionStatus::Exited => {} - SessionStatus::Unknown => bail!( - "this machine won't say whether this session's process is still running, so \ - nothing was started" - ), + SessionStatus::Unknown => { + bail!("there is still a process recorded for this session, so nothing was started") + } _ => bail!("this session is already running"), } // Fresh from the config, like every other launch: a model or a @@ -1150,6 +1164,12 @@ impl SessionManager { let (setup, provider) = resolve(&inner.config, &meta)?; match existing { Some(session) => { + // The driver being replaced is still reading this session's + // output, and replacing the value it lives in does not end + // the tasks that do it. Its process has exited -- that is + // how this line was reached -- so there is nothing left to + // preserve and `detach` is the whole of what it is owed. + session.driver().detach(); *session.driver.lock().unwrap() = make_driver( &meta, &setup, @@ -1226,6 +1246,38 @@ impl SessionManager { /// reports `Unknown` too: this server is not driving it, so it genuinely /// does not know what it is doing -- and that is worth a word that means /// "wait", not one that means "act". +/// The last word about a session, with the one status that cannot be taken +/// on trust checked against the only authority on it. +/// +/// `Exited` is not just a description: it is the word that offers a phone a +/// Start button and lets [`SessionManager::start_session`] build a second +/// CLI against a conversation. So before it is believed it is checked +/// against the process record, and a record that is not known to be dead +/// makes it false. What replaces it is `Unknown` -- there is a process, and +/// nothing here has heard from it -- which is the same answer +/// [`status_of_unlaunched`] gives to the same question. +/// +/// Every other status is left exactly as it was. Those are the pump's, +/// written from what the process itself said, and none of them authorises +/// starting anything. +/// +/// This was reachable and did happen: a session adopted at server start +/// keeps the transcript's last word, so one whose process was reported gone +/// and then found again read as `exited` while it was running. Start was +/// accepted every time it was pressed, each press attaching another reader +/// to the one process, and every line it wrote was then translated once per +/// reader -- three presses put three interleaved copies of one reply on +/// screen. +fn corrected(status: SessionStatus, session_dir: &Path) -> SessionStatus { + if status != SessionStatus::Exited { + return status; + } + match process::recorded(session_dir) { + Some((_, process::Liveness::Alive | process::Liveness::Unknown)) => SessionStatus::Unknown, + Some((_, process::Liveness::Dead)) | None => SessionStatus::Exited, + } +} + fn status_of_unlaunched(session_dir: &Path) -> SessionStatus { match process::recorded(session_dir) { // Nothing was ever recorded: an echo session, or one whose @@ -1422,6 +1474,7 @@ fn launch( wg_app_link::private::create_dir(&dir)?; let transcript_path = dir.join("transcript.jsonl"); let mut transcript = Transcript::open(&transcript_path)?; + let last_status = transcript.last_status().unwrap_or(SessionStatus::Idle); // Before the driver starts, so the token is there when it looks and // the history is already in the transcript a phone will read. if let Some(seed) = seed { @@ -1439,8 +1492,11 @@ fn launch( // What it was last known to be doing, not an assumption. A driver // that has something to say corrects this within its first poll; // one adopting a process that has been quiet says nothing, and - // this is then the only true answer available. - status: Mutex::new(transcript.last_status().unwrap_or(SessionStatus::Idle)), + // this is then the only true answer available. Where it is *not* + // the true answer, the correction goes through the sink below + // rather than being written here, so that it reaches the + // transcript too -- see there. + status: Mutex::new(last_status), title: Mutex::new(meta.title.clone()), // What the transcript last recorded, not the clock: this server has // just been told nothing, and `now()` claimed every relaunched @@ -1494,6 +1550,22 @@ fn launch( ); } + // Before the driver, because the driver may start a process and say so, + // and that has to be the later word of the two. + // + // Sent rather than written into `shared`: the session screen replays the + // transcript and the session list reads `shared`, so a correction made + // in only one of them is the two of them describing one session + // differently -- which is how the phone came to show a Start button on a + // running session in the first place. One event, and the pump puts it in + // both. + let corrected_status = corrected(last_status, &dir); + if corrected_status != last_status { + let _ = sink.send(Event::Status { + state: corrected_status, + }); + } + let driver = Arc::new(Mutex::new(make_driver( &meta, setup, @@ -2493,6 +2565,76 @@ mod tests { )); } + /// The status is a claim about a process, and the process record is + /// what settles it. + /// + /// Without this the phone offered Start on a session whose CLI was + /// running, and taking it up attached a second reader to that one + /// process rather than failing -- so the session went on saying + /// `exited`, the button stayed, and each further press added another + /// reader. On screen that was one reply written as many times as the + /// button had been pressed, interleaved word by word. + #[tokio::test] + async fn a_stale_exited_does_not_start_anything_while_a_process_is_recorded() { + 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(); + + // A live process for this session: this test's own, which is the + // one process certain to still be there when the guard looks. + let record = process::Record::of( + std::process::id(), + process::Detail::Stdio { stdout_read: 0 }, + ) + .expect("record this process"); + process::write(&data_dir.join(&info.id), &record); + let _ = session.sink.send(Event::Status { + state: SessionStatus::Exited, + }); + collect_until(&mut rx, |event| { + matches!( + event, + Event::Status { + state: SessionStatus::Exited + } + ) + }) + .await; + + let refused = manager + .start_session(&info.id) + .expect_err("a process is recorded"); + assert!( + refused.to_string().contains("still a process recorded"), + "said: {refused:#}" + ); + // And the word that was wrong is taken back, on the stream and in + // the transcript -- otherwise the button that asked for this is + // still there, still saying Start. + collect_until(&mut rx, |event| { + matches!( + event, + Event::Status { + state: SessionStatus::Unknown + } + ) + }) + .await; + assert_eq!(manager.sessions()[0].status, SessionStatus::Unknown); + assert_eq!( + Transcript::open(&data_dir.join(&info.id).join("transcript.jsonl")) + .expect("reopen transcript") + .last_status(), + Some(SessionStatus::Unknown), + ); + } + #[tokio::test] async fn a_restart_relaunches_sessions_and_continues_the_numbering() { let dir = tempfile::tempdir().expect("tempdir"); From 4a122f7b2565a1e78792f2ec691fd0a43de4f827 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 30 Aug 2026 13:56:28 -0400 Subject: [PATCH 2/4] Start a stopped session's process when a message is sent to it Refusing was work handed back: read the status word, find the other button, press it, type the message again. Sending plainly means "do this now", and `--resume` puts the new process on the same conversation, so nothing about the message changes -- only whether there was anything there to read it. `POST /sessions/{id}/message` now goes through the manager, which starts a process first when the session is known to have exited. Only on `exited`: `unknown` has a process that may well be reading its fifo, and starting a second CLI on that guess is the fault `session::process` exists to prevent, so the message goes to the driver as it always did. The Start button and this ask one function, `start_if_exited`, and want opposite answers from it -- "there is already a process" is a refusal worth showing to somebody who pressed Start, and nothing at all to a message being sent. Deciding it in one place under the one write lock is also what keeps two requests that arrive together from starting two CLIs. Verified over the API and on the emulator: with the session reporting `exited` and the composer showing a play button, typing a message and pressing Send started the process, delivered the message and ran the turn -- transcript order `idle`, `userMessage`, `running`, `idle` -- and the button became a stop. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 6 ++ PLAN.md | 14 +++++ server/src/routes.rs | 10 ++- server/src/session/mod.rs | 124 +++++++++++++++++++++++++++++++++----- 4 files changed, 137 insertions(+), 17 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 96eccf9..8762055 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -291,6 +291,12 @@ day: 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. +- **Sending a message to a stopped session starts it.** `POST + .../message` goes through `SessionManager::send_message`, which starts a + process first when the session is known to have exited and then delivers + the message to the driver that has one behind it. Only on `exited`: + `unknown` has a process that may well be reading its fifo. So the Start + button is for when you want a process and nothing to say to it yet. - **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 diff --git a/PLAN.md b/PLAN.md index d069af7..7b1c30f 100644 --- a/PLAN.md +++ b/PLAN.md @@ -375,6 +375,20 @@ Two rules come out of it, and neither is optional: existed for the backend going away — and it is the whole of what a driver whose process has exited is owed. +**Sending a message starts the process if there isn't one** (decided +2026-08-30). Refusing was work handed back: read the status word, find the +other button, press it, type the message again. Sending plainly means "do +this now", and `--resume` puts the new process on the same conversation, so +nothing about the message changes — only whether there was anything there to +read it. The manager's `send_message` and the Start button ask one function +(`start_if_exited`) and want opposite answers from it: "there is already a +process" is a refusal worth showing to somebody who pressed Start, and +nothing at all to a message. Deciding it in one place under one write lock is +also what stops two requests arriving together from starting two CLIs. Only +`Exited` starts anything, for the reason above — `Unknown` has a process that +may well be reading its fifo, and the message goes to the driver as it always +did. + The phone's half is that the process button is disabled while its own request is in flight, so a second press cannot be decided against a status the first one has not changed yet. That is a courtesy rather than the fix: the server diff --git a/server/src/routes.rs b/server/src/routes.rs index a369fff..7de102f 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -16,6 +16,7 @@ //! (a backlog past CATCH_UP_LIMIT arrives as a //! `reset` frame plus the newest window) //! POST /sessions/{id}/message {text, attachmentIds?} +//! (starts the process first if it has exited) //! POST /sessions/{id}/answer {questionId, answers} (questions and permissions) //! POST /sessions/{id}/interrupt stop the running turn; the process stays //! POST /sessions/{id}/stop end the process; the session and transcript stay @@ -640,11 +641,16 @@ async fn message( UrlPath(id): UrlPath, axum::Json(body): axum::Json, ) -> Result { - let session = lookup(&manager, &id)?; + // For the 404 a session that is not here has always answered with; the + // send itself goes through the manager, which may have to start a + // process before there is anything to send to. + lookup(&manager, &id)?; if body.text.trim().is_empty() && body.attachment_ids.is_empty() { return Err(ApiError::BadRequest("message is empty".to_string())); } - session.send_message(body.text, body.attachment_ids); + manager + .send_message(&id, body.text, body.attachment_ids) + .map_err(bad_request)?; Ok(StatusCode::NO_CONTENT) } diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index f4fe012..d3a31a4 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -1100,9 +1100,14 @@ impl SessionManager { 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. + /// Starts a process for a session whose process has ended, for somebody + /// who asked for exactly that. + /// + /// Anything other than a session known to have exited is a refusal to + /// report, because the person pressing this expects a process to appear + /// and is owed the reason one did not. [`SessionManager::send_message`] + /// asks the same question of [`SessionManager::start_if_exited`] and + /// wants the opposite answer. /// /// 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 @@ -1110,10 +1115,56 @@ impl SessionManager { /// 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<()> { + match self.start_if_exited(id)? { + SessionStatus::Exited => Ok(()), + SessionStatus::Unknown => { + bail!("there is still a process recorded for this session, so nothing was started") + } + _ => bail!("this session is already running"), + } + } + + /// Hands a message to a session, starting its process first if that + /// session has none. + /// + /// Sending is the one instruction that plainly means "do this now", so a + /// session whose CLI has ended starts it rather than answering that it + /// cannot -- which left the person holding the phone to read a status + /// word, find a second button, press it, and type the message again. + /// `--resume` puts the new process on the same conversation, so nothing + /// about the message changes; only whether there was anything there to + /// read it. + /// + /// Started before the message rather than after, because starting + /// replaces the driver and the driver that takes the message has to be + /// the one with a process behind it. + pub fn send_message(&self, id: &str, text: String, images: Vec) -> Result<()> { + // Only `Exited` starts anything -- see `start_if_exited`. A session + // this cannot say has exited keeps the behaviour it always had: the + // message goes to the driver, which answers for it. + self.start_if_exited(id)?; + self.session(id) + .with_context(|| format!("no session {id}"))? + .send_message(text, images); + Ok(()) + } + + /// Starts a process for the session if it is known to have exited, and + /// reports what the session was found to be doing either way. `Exited` + /// is therefore the one returned value that means something was started. + /// + /// One decision with two callers who want opposite things from it: a + /// Start button treats "there is already a process" as a refusal worth + /// showing, and a message being sent treats it as nothing at all. + /// Deciding it here, under the one write lock, is also what stops two + /// requests that arrive together from starting two CLIs on one + /// conversation. + /// + /// Nothing is started on `Unknown`. That means nobody could find out + /// whether the process is alive, and starting one on that is precisely + /// the second-CLI-on-one-conversation fault `session::process` exists to + /// prevent. /// /// What the session then *reports* is the driver's to say, not this /// function's: the phone's list reads the manager's status and the @@ -1122,7 +1173,7 @@ impl SessionManager { /// is what a status set here without an event produced, visible as a /// stop button that turned into a play button a moment after the screen /// opened. - pub fn start_session(&self, id: &str) -> Result<()> { + fn start_if_exited(&self, id: &str) -> Result { let mut inner = self.inner.write().unwrap(); let meta = inner .config @@ -1151,12 +1202,8 @@ impl SessionManager { } None => status_of_unlaunched(&dir), }; - match status { - SessionStatus::Exited => {} - SessionStatus::Unknown => { - bail!("there is still a process recorded for this session, so nothing was started") - } - _ => bail!("this session is already running"), + if status != SessionStatus::Exited { + return Ok(status); } // Fresh from the config, like every other launch: a model or a // permission mode changed while the session was stopped is what it @@ -1202,7 +1249,7 @@ impl SessionManager { // here as well would be a second writer of the same fact, and the // one that cannot see whether the process it is describing is still // there. - Ok(()) + Ok(SessionStatus::Exited) } /// Kills the process, releases everything the spawn created, and @@ -2565,6 +2612,53 @@ mod tests { )); } + /// Sending is an instruction that means "now", so it does not answer + /// that the session's process has gone -- it starts one and delivers + /// the message to it. + /// + /// Refusing was the old behaviour and it was work handed back: read the + /// status word, find the other button, press it, type the message + /// again. `--resume` puts the new process on the same conversation, so + /// the message it reads is the one that was typed. + #[tokio::test] + async fn a_message_starts_the_process_a_stopped_session_has_not_got() { + 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 _ = session.sink.send(Event::Status { + state: SessionStatus::Exited, + }); + collect_until(&mut rx, |event| { + matches!( + event, + Event::Status { + state: SessionStatus::Exited + } + ) + }) + .await; + + manager + .send_message(&info.id, "carry on".to_string(), Vec::new()) + .expect("send to a stopped session"); + collect_until( + &mut rx, + |event| matches!(event, Event::UserMessage { text, .. } if text == "carry on"), + ) + .await; + // And the session is running again, not merely written to: a message + // delivered to a session still reporting `exited` is one the phone + // draws under a Start button. + assert_ne!(manager.sessions()[0].status, SessionStatus::Exited); + } + /// The status is a claim about a process, and the process record is /// what settles it. /// From 1a132b4de316e3a4a8199e1a50cedf74a9c2b774 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 30 Aug 2026 14:06:59 -0400 Subject: [PATCH 3/4] Start a stopped session's process for a command too Same reasoning as the message path a commit ago, and the same objection to leaving it out: a command is something somebody asked the session to do, and answering "its process has exited" hands back the work of starting one. `/compact` on a stopped session is the case that shows it -- what is being asked for is exactly what a stopped session needs before it is useful again. `POST /sessions/{id}/command` and `/compact` now go through `SessionManager::run_command`. A rename is deliberately not one of them: it is persisted and listed whether or not a process ever hears about it, so starting a CLI to tell it a name would be spending a resume on nothing. It stays a forward to a process that happens to be there. A command needs one thing a message did not. `Commands::submit` refuses on `Exited`, and a driver that has just started a process announces `Idle` through the sink rather than writing it -- so a command judged against the session's own status would be refused by the word the start had just replaced, in a window narrow enough that only a test reliably hits it. `start_if_exited` returning `Exited` is what says a process was started, so the status the command is judged against comes from there rather than from a re-read the pump may not have caught up with. The test fails without it. `LiveSession::compact` went with this: `/compact` the route and "/compact" the typed command were two ways to the same command, and now there is one. Verified over the API against a stand-in CLI: with the session reporting `exited`, both `/clear` and `POST /compact` started the process and were delivered -- transcript order `idle`, `commandSent`, `running`, `idle`, with no "this session's process has exited" anywhere. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 15 ++++--- PLAN.md | 25 ++++++++---- server/src/routes.rs | 29 +++++++++----- server/src/session/mod.rs | 82 ++++++++++++++++++++++++++++++++------- 4 files changed, 115 insertions(+), 36 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8762055..bccbd04 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -291,12 +291,15 @@ day: 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. -- **Sending a message to a stopped session starts it.** `POST - .../message` goes through `SessionManager::send_message`, which starts a - process first when the session is known to have exited and then delivers - the message to the driver that has one behind it. Only on `exited`: - `unknown` has a process that may well be reading its fifo. So the Start - button is for when you want a process and nothing to say to it yet. +- **A message or a command sent to a stopped session starts it.** `POST + .../message`, `.../command` and `.../compact` go through + `SessionManager::send_message` and `::run_command`, which start a process + first when the session is known to have exited and then hand the thing to + the driver that has one behind it. Only on `exited`: `unknown` has a + process that may well be reading its fifo. `/rename` is the exception — + the name is persisted and listed either way, so it is forwarded to a + process that happens to be there and never starts one. So the Start button + is for when you want a process and nothing to say to it yet. - **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 diff --git a/PLAN.md b/PLAN.md index 7b1c30f..3faffa7 100644 --- a/PLAN.md +++ b/PLAN.md @@ -375,19 +375,30 @@ Two rules come out of it, and neither is optional: existed for the backend going away — and it is the whole of what a driver whose process has exited is owed. -**Sending a message starts the process if there isn't one** (decided +**A message or a command starts the process if there isn't one** (decided 2026-08-30). Refusing was work handed back: read the status word, find the -other button, press it, type the message again. Sending plainly means "do -this now", and `--resume` puts the new process on the same conversation, so -nothing about the message changes — only whether there was anything there to -read it. The manager's `send_message` and the Start button ask one function +other button, press it, type the thing again. Both plainly mean "do this +now", and `--resume` puts the new process on the same conversation, so +nothing about what was typed changes — only whether there was anything there +to read it. A rename is deliberately not one of them: it is persisted and +listed whether or not a process ever hears about it, so starting a CLI to +tell it a name would be spending a resume on nothing. The manager's +`send_message` and `run_command` and the Start button ask one function (`start_if_exited`) and want opposite answers from it: "there is already a process" is a refusal worth showing to somebody who pressed Start, and nothing at all to a message. Deciding it in one place under one write lock is also what stops two requests arriving together from starting two CLIs. Only `Exited` starts anything, for the reason above — `Unknown` has a process that -may well be reading its fifo, and the message goes to the driver as it always -did. +may well be reading its fifo, and what was typed goes to the driver as it +always did. + +A command needs one thing a message does not. `Commands::submit` refuses on +`Exited`, and a driver that has just started a process announces `Idle` +through the sink rather than writing it — so a command judged against the +session's own status would be refused by the word the start had just +replaced. `start_if_exited` returning `Exited` is what says a process was +started, so `run_command` judges against `Idle` from there rather than +re-reading a status the pump may not have caught up with. The phone's half is that the process button is disabled while its own request is in flight, so a second press cannot be decided against a status the first diff --git a/server/src/routes.rs b/server/src/routes.rs index 7de102f..1c9151a 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -24,6 +24,7 @@ //! POST /sessions/{id}/title {title} //! POST /sessions/{id}/model {model} //! POST /sessions/{id}/command {text} -- /compact, /clear, /rename x, or the dialect's own +//! (starts the process first if it has exited) //! POST /sessions/{id}/compact //! POST /sessions/{id}/attachments multipart image upload -> {id}, referenced by /message //! GET /sessions/{id}/files/{name} images the session produced or was sent @@ -840,15 +841,22 @@ async fn command( Some((name, rest)) => (name, rest.trim()), None => (text, ""), }; - match (name, rest) { - ("/compact", _) => lookup(&manager, &id)?.run_command(SessionCommand::Compact), - ("/clear", _) => lookup(&manager, &id)?.run_command(SessionCommand::Clear), - // Through the manager, not the session: a name is persisted and - // listed as well as forwarded, and that is one operation. + // Everything but a rename goes through the manager, which starts the + // session's process first if it has exited. A rename is persisted and + // listed whether or not a process hears about it, so it neither needs + // one nor is worth starting one for. + let command = match (name, rest) { + ("/compact", _) => SessionCommand::Compact, + ("/clear", _) => SessionCommand::Clear, ("/rename", "") => return Err(bad_request(anyhow::anyhow!("a session needs a name"))), - ("/rename", title) => manager.rename_session(&id, title).map_err(bad_request)?, - _ => lookup(&manager, &id)?.run_command(SessionCommand::Raw(text.to_string())), - } + ("/rename", title) => { + manager.rename_session(&id, title).map_err(bad_request)?; + return Ok(StatusCode::NO_CONTENT); + } + _ => SessionCommand::Raw(text.to_string()), + }; + lookup(&manager, &id)?; + manager.run_command(&id, command).map_err(bad_request)?; Ok(StatusCode::NO_CONTENT) } @@ -856,7 +864,10 @@ async fn compact( State(manager): State>, UrlPath(id): UrlPath, ) -> Result { - lookup(&manager, &id)?.compact(); + lookup(&manager, &id)?; + manager + .run_command(&id, SessionCommand::Compact) + .map_err(bad_request)?; Ok(StatusCode::NO_CONTENT) } diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index d3a31a4..92f68e0 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -399,13 +399,6 @@ impl LiveSession { self.driver().detach(); } - /// Compacts at the next boundary. Through the command queue like - /// every other instruction to the session, so pressing it during a - /// turn holds rather than writing into that turn. - pub fn compact(&self) { - self.run_command(SessionCommand::Compact); - } - pub fn subscribe(&self) -> broadcast::Receiver { self.events.subscribe() } @@ -1150,6 +1143,42 @@ impl SessionManager { Ok(()) } + /// Runs one of the session's own commands, starting its process first + /// if that session has none. + /// + /// The same reasoning as [`SessionManager::send_message`], and for the + /// same reason it is not left to each caller: a command is something + /// somebody asked the session to do, and answering "its process has + /// exited" hands back the work of starting one. `/compact` on a + /// stopped session is the case that shows it -- the thing being asked + /// for is exactly what a stopped session needs before it is useful + /// again. + /// + /// Renaming is deliberately not here. It is persisted and listed + /// whether or not a process ever hears about it, so starting a CLI to + /// tell it a name would be spending a resume on nothing -- see + /// [`SessionManager::rename_session`], which forwards it to a process + /// that happens to be there. + pub fn run_command(&self, id: &str, command: SessionCommand) -> Result<()> { + // Judged against the status *after* the start, not the one that + // caused it. A driver that has just started a process announces + // `idle` through the sink and the pump may not have recorded it + // yet, so reading the session's own status here would refuse the + // command the start was for -- `Commands::submit` refuses on + // `Exited`, which is exactly the word that has just stopped being + // true. `start_if_exited` returning `Exited` is what says a process + // was started; anything else is a status nothing has invalidated. + let status = match self.start_if_exited(id)? { + SessionStatus::Exited => SessionStatus::Idle, + found => found, + }; + self.session(id) + .with_context(|| format!("no session {id}"))? + .commands + .submit(command, status); + Ok(()) + } + /// Starts a process for the session if it is known to have exited, and /// reports what the session was found to be doing either way. `Exited` /// is therefore the one returned value that means something was started. @@ -2612,16 +2641,22 @@ mod tests { )); } - /// Sending is an instruction that means "now", so it does not answer - /// that the session's process has gone -- it starts one and delivers - /// the message to it. + /// A message and a command both mean "now", so neither answers that the + /// session's process has gone -- they start one and go to it. /// /// Refusing was the old behaviour and it was work handed back: read the - /// status word, find the other button, press it, type the message - /// again. `--resume` puts the new process on the same conversation, so - /// the message it reads is the one that was typed. + /// status word, find the other button, press it, type the thing again. + /// `--resume` puts the new process on the same conversation, so what it + /// reads is what was typed. + /// + /// Both halves in one test because they are one rule. A command is the + /// half that can fail on its own: `Commands::submit` refuses on + /// `Exited`, and the start it has just been given announces `Idle` + /// through the sink rather than writing it -- so a command judged + /// against the session's own status would be refused by the word the + /// start replaced, in a window a test is the only thing likely to hit. #[tokio::test] - async fn a_message_starts_the_process_a_stopped_session_has_not_got() { + async fn an_instruction_starts_the_process_a_stopped_session_has_not_got() { let dir = tempfile::tempdir().expect("tempdir"); let config_path = dir.path().join("config.ron"); let data_dir = dir.path().join("sessions"); @@ -2657,6 +2692,25 @@ mod tests { // delivered to a session still reporting `exited` is one the phone // draws under a Start button. assert_ne!(manager.sessions()[0].status, SessionStatus::Exited); + + let _ = session.sink.send(Event::Status { + state: SessionStatus::Exited, + }); + collect_until(&mut rx, |event| { + matches!( + event, + Event::Status { + state: SessionStatus::Exited + } + ) + }) + .await; + + manager + .run_command(&info.id, SessionCommand::Clear) + .expect("clear a stopped session"); + collect_until(&mut rx, |event| matches!(event, Event::Cleared)).await; + assert_ne!(manager.sessions()[0].status, SessionStatus::Exited); } /// The status is a claim about a process, and the process record is From 6119926a4d375f7bbc85dcce9723b67a199140f7 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 30 Aug 2026 14:16:29 -0400 Subject: [PATCH 4/4] Start the process for a rename too -- the CLI keeps its own name The last commit left renaming out on the grounds that the name is persisted and listed whether or not a process hears about it. That was wrong, and Iris said so: Claude Code keeps its own copy of the name, that copy is what its session picker shows and what other agents read when they list sessions, and a session is only ever *given* a name at birth -- every later start is a `--resume`, which passes no `--name`. So a rename that reached no process left the two lists disagreeing permanently, with this app's the only one that had moved. The cost of a resume buys the one thing renaming is for. It stays `rename_session` rather than becoming a command like the rest, because the name is persisted and listed as well as forwarded and that is one operation. The save happens first and the lock is dropped before the telling, so a failure to start reports that the telling failed rather than the rename, which by then has already happened. `LiveSession::run_command` went with it. It read `shared.status` and that read is exactly what a just-started session cannot be judged by, so every caller now goes through the manager -- which is also what the four tests that used it were standing in for. Verified against a stand-in CLI that echoes its stdin: a session reporting `exited` was renamed, the process started with `--resume`, and the CLI received `/rename after the restart` on stdin. The list shows the new name and the session reports idle. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 13 ++-- PLAN.md | 16 +++-- server/src/routes.rs | 12 ++-- server/src/session/mod.rs | 132 ++++++++++++++++++++++++++++---------- 4 files changed, 127 insertions(+), 46 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index bccbd04..c07e0d3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -296,10 +296,15 @@ day: `SessionManager::send_message` and `::run_command`, which start a process first when the session is known to have exited and then hand the thing to the driver that has one behind it. Only on `exited`: `unknown` has a - process that may well be reading its fifo. `/rename` is the exception — - the name is persisted and listed either way, so it is forwarded to a - process that happens to be there and never starts one. So the Start button - is for when you want a process and nothing to say to it yet. + process that may well be reading its fifo. `/rename` starts one too, and + for a sharper reason than the rest: the CLI keeps its own copy of the + name, that copy is what its session picker and other agents' session + lists show, and a session is only ever *given* a name at birth — every + later start is a `--resume` — so a rename that reached no process would + leave the two lists disagreeing for good. Its save happens before the + telling, so a failure there says the telling failed rather than the + rename. So the Start button is for when you want a process and nothing to + say to it yet. - **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 diff --git a/PLAN.md b/PLAN.md index 3faffa7..123e936 100644 --- a/PLAN.md +++ b/PLAN.md @@ -380,10 +380,18 @@ Two rules come out of it, and neither is optional: other button, press it, type the thing again. Both plainly mean "do this now", and `--resume` puts the new process on the same conversation, so nothing about what was typed changes — only whether there was anything there -to read it. A rename is deliberately not one of them: it is persisted and -listed whether or not a process ever hears about it, so starting a CLI to -tell it a name would be spending a resume on nothing. The manager's -`send_message` and `run_command` and the Start button ask one function +to read it. A rename is included, and for a sharper reason than the rest: +Claude Code keeps its own copy of the name, that copy is what its session +picker shows and what other agents read when they list sessions, and a +session is only ever *given* a name at birth, since every later start is a +`--resume`. So a rename that reached no process would leave the two lists +disagreeing permanently, with this app's the only one that had moved — and +the cost of a resume buys the one thing renaming is for. It stays +`rename_session` rather than becoming a command like the others, because the +name is persisted and listed as well as forwarded and that is one operation; +the save happens first, so a failure to start reports that the telling +failed, not the rename. The manager's `send_message` and `run_command` and +the Start button ask one function (`start_if_exited`) and want opposite answers from it: "there is already a process" is a refusal worth showing to somebody who pressed Start, and nothing at all to a message. Deciding it in one place under one write lock is diff --git a/server/src/routes.rs b/server/src/routes.rs index 1c9151a..e949d90 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -841,10 +841,14 @@ async fn command( Some((name, rest)) => (name, rest.trim()), None => (text, ""), }; - // Everything but a rename goes through the manager, which starts the - // session's process first if it has exited. A rename is persisted and - // listed whether or not a process hears about it, so it neither needs - // one nor is worth starting one for. + // All of these start the session's process first if it has exited: a + // command is something somebody asked the session to do, and answering + // that its process is gone hands back the work of starting one. + // + // A rename still goes through `rename_session` rather than being a + // command like the rest, because the name is persisted and listed as + // well as forwarded, and that is one operation. It starts a process + // too, and for a sharper reason than the others -- see there. let command = match (name, rest) { ("/compact", _) => SessionCommand::Compact, ("/clear", _) => SessionCommand::Clear, diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index 92f68e0..5daaf08 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -381,13 +381,6 @@ impl LiveSession { self.driver().answer_question(question_id, answers); } - /// Asks the session to run a command on itself, now or at the next - /// boundary. See [`Commands`] for why it may not be now. - pub fn run_command(&self, command: SessionCommand) { - self.commands - .submit(command, *self.shared.status.lock().unwrap()); - } - pub fn interrupt(&self) { self.driver().interrupt(); } @@ -997,6 +990,15 @@ impl SessionManager { /// unlike the model and the permission mode, this is settled here and /// the driver is *told*, rather than asked and believed: see /// [`Driver::set_title`]. + /// + /// Telling it is not decoration, which is why this starts a stopped + /// session like any other command. Claude Code keeps its own copy of + /// the name, and that copy is what its session picker shows and what + /// other agents see when they list sessions -- and a session is only + /// ever *given* a name at birth, since every later start is a + /// `--resume`. So a rename that reached no process would leave the two + /// lists disagreeing permanently, with the app's the only one that had + /// moved. pub fn rename_session(&self, id: &str, title: &str) -> Result<()> { let title = title.trim(); // An empty name is not a name, and it is what a cleared field @@ -1005,25 +1007,37 @@ impl SessionManager { if title.is_empty() { bail!("a session needs a name"); } - let mut inner = self.inner.write().unwrap(); - if !inner.config.sessions.iter().any(|meta| meta.id == id) { - bail!("no session {id}"); + { + let mut inner = self.inner.write().unwrap(); + if !inner.config.sessions.iter().any(|meta| meta.id == id) { + bail!("no session {id}"); + } + let mut candidate = inner.config.clone(); + for meta in candidate.sessions.iter_mut().filter(|meta| meta.id == id) { + meta.title = title.to_string(); + } + candidate.save(&self.config_path)?; + inner.config = candidate; + // The name is this server's and changes now, whatever happens + // next: the list shows it immediately, and the process is told + // at the next boundary. + if let Some(session) = inner.live.get(id) { + *session.shared.title.lock().unwrap() = title.to_string(); + } } - let mut candidate = inner.config.clone(); - for meta in candidate.sessions.iter_mut().filter(|meta| meta.id == id) { - meta.title = title.to_string(); - } - candidate.save(&self.config_path)?; - inner.config = candidate; - if let Some(session) = inner.live.get(id) { - // The name is this server's and changes now. Telling whatever - // runs the session is a command, and commands wait for the - // turn to end -- so the list shows the new name immediately - // and the CLI is told at the next boundary. - *session.shared.title.lock().unwrap() = title.to_string(); - session.run_command(SessionCommand::SetTitle(title.to_string())); - } - Ok(()) + // Dropped the lock first -- `run_command` takes it again to decide + // whether anything needs starting, and this is not a reentrant one. + // + // The context matters more than it looks: the rename above is saved + // by the time this can fail, so a bare error would report a rename + // that did not happen. What failed is only the telling. + self.run_command(id, SessionCommand::SetTitle(title.to_string())) + .with_context(|| { + format!( + "renamed to \"{title}\" here, but the session's own copy of the name could \ + not be changed" + ) + }) } pub fn set_session_model(&self, id: &str, model: &str) -> Result<()> { @@ -1154,11 +1168,6 @@ impl SessionManager { /// for is exactly what a stopped session needs before it is useful /// again. /// - /// Renaming is deliberately not here. It is persisted and listed - /// whether or not a process ever hears about it, so starting a CLI to - /// tell it a name would be spending a resume on nothing -- see - /// [`SessionManager::rename_session`], which forwards it to a process - /// that happens to be there. pub fn run_command(&self, id: &str, command: SessionCommand) -> Result<()> { // Judged against the status *after* the start, not the one that // caused it. A driver that has just started a process announces @@ -2025,7 +2034,9 @@ mod tests { }) .await; - session.run_command(SessionCommand::Clear); + manager + .run_command(&info.id, SessionCommand::Clear) + .expect("clear"); let held = collect_until(&mut rx, |event| { matches!( event, @@ -2336,7 +2347,9 @@ mod tests { }) .await; - session.run_command(SessionCommand::Raw("/tool held".to_string())); + manager + .run_command(&info.id, SessionCommand::Raw("/tool held".to_string())) + .expect("command"); let seen = collect_until(&mut rx, |event| { matches!(event, Event::CommandQueued { .. }) }) @@ -2380,7 +2393,9 @@ mod tests { let session = manager.session(&info.id).expect("live session"); let mut rx = session.subscribe(); - session.run_command(SessionCommand::Raw("/tool now".to_string())); + manager + .run_command(&info.id, SessionCommand::Raw("/tool now".to_string())) + .expect("command"); let seen = collect_until(&mut rx, |event| matches!(event, Event::ToolStart { .. })).await; // Sent, and never queued: a session between turns has nothing to // wait for, and a phone should not draw a bubble that resolves in @@ -2537,7 +2552,9 @@ mod tests { // A clear leaves it unmeasured: the conversation is gone, and how // much is left is a thing nobody has counted. - session.run_command(SessionCommand::Clear); + manager + .run_command(&info.id, SessionCommand::Clear) + .expect("clear"); collect_until(&mut rx, |event| matches!(event, Event::Cleared)).await; assert_eq!(manager.sessions()[0].context_tokens, None); @@ -2713,6 +2730,53 @@ mod tests { assert_ne!(manager.sessions()[0].status, SessionStatus::Exited); } + /// A rename is not decoration, so it starts a stopped session too. + /// + /// Claude Code keeps its own copy of the name; that copy is what its + /// session picker shows and what other agents read when they list + /// sessions, and a session is only ever *given* a name at birth, since + /// every later start is a `--resume`. So a rename that reached no + /// process would leave the two lists disagreeing permanently, with this + /// app's the only one that had moved. + #[tokio::test] + async fn a_rename_reaches_the_process_even_when_one_has_to_be_started() { + 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 _ = session.sink.send(Event::Status { + state: SessionStatus::Exited, + }); + collect_until(&mut rx, |event| { + matches!( + event, + Event::Status { + state: SessionStatus::Exited + } + ) + }) + .await; + + manager + .rename_session(&info.id, "the new name") + .expect("rename a stopped session"); + collect_until( + &mut rx, + |event| matches!(event, Event::CommandSent { text, .. } if text == "/rename the new name"), + ) + .await; + // Both halves: the name this server lists changed, and it was told + // to a process rather than only written down. + assert_eq!(manager.sessions()[0].title, "the new name"); + assert_ne!(manager.sessions()[0].status, SessionStatus::Exited); + } + /// The status is a claim about a process, and the process record is /// what settles it. ///