diff --git a/AGENTS.md b/AGENTS.md index cd241c2..acec458 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -312,6 +312,20 @@ 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. +- **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` 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 @@ -323,6 +337,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..123e936 100644 --- a/PLAN.md +++ b/PLAN.md @@ -351,6 +351,69 @@ 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. + +**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 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 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 +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 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 +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 3fe9ec6..8dac1be 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt @@ -568,6 +568,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 @@ -1030,7 +1036,7 @@ fun SessionScreen( } } - fun act(onFailure: () -> Unit = {}, action: () -> Unit) { + fun act(onFailure: () -> Unit = {}, onDone: () -> Unit = {}, action: () -> Unit) { scope.launch { try { withContext(Dispatchers.IO) { action() } @@ -1038,6 +1044,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() } } } @@ -1538,7 +1549,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/routes.rs b/server/src/routes.rs index a369fff..e949d90 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 @@ -23,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 @@ -640,11 +642,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) } @@ -834,15 +841,26 @@ 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. + // 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, ("/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) } @@ -850,7 +868,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 f29f5eb..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(); } @@ -399,13 +392,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() } @@ -1004,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 @@ -1012,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<()> { @@ -1100,9 +1107,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 +1122,87 @@ 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(()) + } + + /// 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. + /// + 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. + /// + /// 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 +1211,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 @@ -1132,17 +1221,27 @@ 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" - ), - _ => 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 @@ -1150,6 +1249,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, @@ -1182,7 +1287,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 @@ -1226,6 +1331,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 +1559,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 +1577,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 +1635,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, @@ -1877,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, @@ -2188,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 { .. }) }) @@ -2232,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 @@ -2389,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); @@ -2493,6 +2658,195 @@ mod tests { )); } + /// 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 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 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"); + 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); + + 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); + } + + /// 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. + /// + /// 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");