diff --git a/PLAN.md b/PLAN.md index 11abbc3..d2be19c 100644 --- a/PLAN.md +++ b/PLAN.md @@ -332,10 +332,24 @@ Three decisions worth not undoing: That last rule found a real bug in the launch path, which is where the phone would have hit it: a relaunched session took its status from the transcript, so one whose process had died before a backend restart reported `Exited` -while the launch it had just gone through was starting a new process. The -status now says `Idle` when a launch leaves a process running — `Exited` -there is not merely stale, it is the word that refuses every command and -invites somebody to start a second process against a live conversation. +while the launch it had just gone through was starting a new process — +`Exited` there is not merely stale, it is the word that refuses every command +and invites somebody to start a second process against a live conversation. + +**Who says so matters as much as what is said.** The first fix wrote `Idle` +straight into the manager's view, and that produced a second bug on the +phone: the session list reads the manager's status and the session screen +replays the transcript, so a status written in one and not the other is two +screens disagreeing about one session — visible as a stop button that turned +into a play button a moment after the screen opened. So the rule is that +**a driver announces the state it starts in, through the event sink**, which +is what `EchoDriver::new` and `LlamaDriver::attached` already did; +`ClaudeDriver` was the one that started a process silently. It says `Idle` +only when it *started* one — adopting says nothing, because a process that +was already running may be mid-turn and the transcript's last word is the +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. 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 diff --git a/server/src/session/claude.rs b/server/src/session/claude.rs index af43fab..0805307 100644 --- a/server/src/session/claude.rs +++ b/server/src/session/claude.rs @@ -233,6 +233,10 @@ impl ClaudeDriver { // to find. Nothing was ever recorded for one, so this answers "no" // without needing to know that, which is why the remote case is // not a branch here. + // Whether this launch *started* a process or picked up one that was + // already there. The two owe the session different things -- see the + // `Status` below. + let started_here; let record = match process::recorded(session_dir) { // Still running, and ours. Pick it up where it was left -- // the one path that must not pass `--resume`. @@ -243,6 +247,7 @@ impl ClaudeDriver { provider.name, record.pid ); + started_here = false; record } // Recorded, and the machine will not say whether it is still @@ -256,12 +261,39 @@ impl ClaudeDriver { meta.id, record.pid ); + started_here = false; record } Some((_, process::Liveness::Dead)) | None => { + started_here = true; Self::start(meta, provider, transport, session_dir)? } }; + + // A process this driver has just started has been asked for nothing, + // which is what idle means. Said here because nothing else will say + // it: the CLI writes not one line until it is given work, so a + // session whose transcript last recorded `Exited` -- one whose + // process died while this server was down, or one somebody stopped + // from the phone -- would keep that word. `Exited` refuses every + // command sent to the session, and it offers a phone the chance to + // start a second CLI against a conversation that already has one. + // + // From the driver rather than from the manager, and before `follow` + // is spawned, so it cannot overtake the exit `follow` reports for a + // process that dies immediately: both come from here, in this order. + // Adopting says nothing, because a process that was already running + // may be mid-turn, and the transcript's last word is the better + // answer until its output says otherwise. + // + // The llama driver has always done this (see `LlamaDriver::attached`, + // which reports `Running` while the model loads and `Idle` when it + // answers); this side was the one silent about it. + if started_here { + let _ = sink.send(Event::Status { + state: SessionStatus::Idle, + }); + } // Where reading of its output had reached. A process just started // has said nothing, so its record says zero and this is the same // question with the same answer. diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index 07f1ae4..f29f5eb 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -1114,6 +1114,14 @@ impl SessionManager { /// 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. + /// + /// 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 + /// session screen replays the transcript, so a status written in one and + /// not the other is two screens disagreeing about one session -- which + /// 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<()> { let mut inner = self.inner.write().unwrap(); let meta = inner @@ -1140,7 +1148,7 @@ impl SessionManager { // permission mode changed while the session was stopped is what it // starts with. let (setup, provider) = resolve(&inner.config, &meta)?; - let session = match existing { + match existing { Some(session) => { *session.driver.lock().unwrap() = make_driver( &meta, @@ -1151,7 +1159,6 @@ impl SessionManager { session.transcript_path(), &session.sink, )?; - session } // Nothing is live for this one -- a session whose launch failed // when the server started, which has no pump either. That is the @@ -1166,19 +1173,15 @@ impl SessionManager { None, self.notifications.clone(), )?; - inner.live.insert(id.to_string(), Arc::clone(&session)); - session + inner.live.insert(id.to_string(), session); } - }; - // The recorded status is `Exited` and this has just made it untrue. - // Said here because nothing else will say it: a CLI that has been - // given no work writes nothing, so the session would sit at - // `Exited` -- refusing every command, refusing every message, and - // showing a phone an offer to start a second process against the - // conversation this one is already running. - let _ = session.sink.send(Event::Status { - state: SessionStatus::Idle, - }); + } + // Nothing is announced from here. A driver that starts a process + // reports the session idle itself, in order with everything else it + // says about that process -- see `ClaudeDriver::launch`. Saying it + // 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(()) } @@ -1501,29 +1504,6 @@ fn launch( &sink, )?)); - // The transcript's last word is what this session was doing when - // something was last watching it, and building the driver above may have - // just made it untrue: a session recorded as `Exited` with a process - // running again is one this launch has started. Carrying `Exited` - // forward is not merely stale -- it is the word that refuses every - // command sent to the session, and the word that invites somebody to - // start a *second* process against a conversation that already has one, - // which is the fault `session::process` exists to prevent. It reached a - // phone as a play button on a session that was already running. - // - // Idle is what is true: there is a process, and nothing has asked it for - // anything. Written rather than announced, because nobody watched a - // transition -- this is the state the session is being restored in, and - // an event would put a status change in the transcript that never - // happened. A record nobody could check stays as it was and is corrected - // by the driver's first poll, which reports `Unknown` for it. - { - let mut status = shared.status.lock().unwrap(); - if *status == SessionStatus::Exited && process::live(&dir).is_some() { - *status = SessionStatus::Idle; - } - } - let commands = Arc::new(Commands { driver: Arc::clone(&driver), sink: sink.clone(), @@ -2492,9 +2472,19 @@ mod tests { manager.start_session(&info.id).expect("start again"); collect_until(&mut rx, is_idle).await; - // Idle rather than exited, and said by the start rather than left - // for a driver that has been given no work to say for itself. + // Idle rather than exited, and *recorded* -- said by the driver that + // was just built, like every driver says what state it is starting + // in. The manager writing it directly is what made the phone's list + // and its session screen disagree: one reads this status and the + // other replays the transcript, so a status in only one of them is + // two screens describing one session differently. assert_eq!(manager.sessions()[0].status, SessionStatus::Idle); + assert_eq!( + Transcript::open(&data_dir.join(&info.id).join("transcript.jsonl")) + .expect("reopen transcript") + .last_status(), + Some(SessionStatus::Idle), + ); // The same live session throughout: only the driver was replaced, // so nothing a phone is reading was interrupted. assert!(Arc::ptr_eq(