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] 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