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. ///