Let the driver say it started, so both screens agree
The session list reads the manager's status; the session screen replays the transcript. Correcting a relaunched session's stale `exited` by writing the manager's view directly left those two saying different things about one session -- which showed up as a stop button that turned into a play button a moment after the screen opened, and a status row that disagreed with the row it was opened from. So the correction goes through the event sink instead, from the driver that started the process: `EchoDriver::new` and `LlamaDriver::attached` already announce the state they start in, and `ClaudeDriver` was the one starting a process silently. It says idle only when it started one -- adopting says nothing, since a process that was already running may be mid-turn. Coming from the driver also orders it against the exit `follow` reports for a process that dies immediately, which a status written from the manager could not be. Verified against a stand-in CLI: stop, restart the backend, and the list and the transcript's last status both say idle, with the relaunch recorded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
a4ec8cfbd8
commit
1d843f20f2
3 files changed
+79
-43
No files matched your search
@@ -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
|
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,
|
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`
|
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
|
while the launch it had just gone through was starting a new process —
|
||||||
status now says `Idle` when a launch leaves a process running — `Exited`
|
`Exited` there is not merely stale, it is the word that refuses every command
|
||||||
there is not merely stale, it is the word that refuses every command and
|
and invites somebody to start a second process against a live conversation.
|
||||||
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
|
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
|
colour say what pressing it would do now: an orange pause while a turn is
|
||||||
|
|||||||
@@ -233,6 +233,10 @@ impl ClaudeDriver {
|
|||||||
// to find. Nothing was ever recorded for one, so this answers "no"
|
// to find. Nothing was ever recorded for one, so this answers "no"
|
||||||
// without needing to know that, which is why the remote case is
|
// without needing to know that, which is why the remote case is
|
||||||
// not a branch here.
|
// 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) {
|
let record = match process::recorded(session_dir) {
|
||||||
// Still running, and ours. Pick it up where it was left --
|
// Still running, and ours. Pick it up where it was left --
|
||||||
// the one path that must not pass `--resume`.
|
// the one path that must not pass `--resume`.
|
||||||
@@ -243,6 +247,7 @@ impl ClaudeDriver {
|
|||||||
provider.name,
|
provider.name,
|
||||||
record.pid
|
record.pid
|
||||||
);
|
);
|
||||||
|
started_here = false;
|
||||||
record
|
record
|
||||||
}
|
}
|
||||||
// Recorded, and the machine will not say whether it is still
|
// Recorded, and the machine will not say whether it is still
|
||||||
@@ -256,12 +261,39 @@ impl ClaudeDriver {
|
|||||||
meta.id,
|
meta.id,
|
||||||
record.pid
|
record.pid
|
||||||
);
|
);
|
||||||
|
started_here = false;
|
||||||
record
|
record
|
||||||
}
|
}
|
||||||
Some((_, process::Liveness::Dead)) | None => {
|
Some((_, process::Liveness::Dead)) | None => {
|
||||||
|
started_here = true;
|
||||||
Self::start(meta, provider, transport, session_dir)?
|
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
|
// Where reading of its output had reached. A process just started
|
||||||
// has said nothing, so its record says zero and this is the same
|
// has said nothing, so its record says zero and this is the same
|
||||||
// question with the same answer.
|
// question with the same answer.
|
||||||
|
|||||||
+29
-39
@@ -1114,6 +1114,14 @@ impl SessionManager {
|
|||||||
/// nobody could find out whether the process is alive, and starting one
|
/// nobody could find out whether the process is alive, and starting one
|
||||||
/// on that is precisely the second-CLI-on-one-conversation fault that
|
/// on that is precisely the second-CLI-on-one-conversation fault that
|
||||||
/// `session::process` exists to prevent.
|
/// `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<()> {
|
pub fn start_session(&self, id: &str) -> Result<()> {
|
||||||
let mut inner = self.inner.write().unwrap();
|
let mut inner = self.inner.write().unwrap();
|
||||||
let meta = inner
|
let meta = inner
|
||||||
@@ -1140,7 +1148,7 @@ impl SessionManager {
|
|||||||
// permission mode changed while the session was stopped is what it
|
// permission mode changed while the session was stopped is what it
|
||||||
// starts with.
|
// starts with.
|
||||||
let (setup, provider) = resolve(&inner.config, &meta)?;
|
let (setup, provider) = resolve(&inner.config, &meta)?;
|
||||||
let session = match existing {
|
match existing {
|
||||||
Some(session) => {
|
Some(session) => {
|
||||||
*session.driver.lock().unwrap() = make_driver(
|
*session.driver.lock().unwrap() = make_driver(
|
||||||
&meta,
|
&meta,
|
||||||
@@ -1151,7 +1159,6 @@ impl SessionManager {
|
|||||||
session.transcript_path(),
|
session.transcript_path(),
|
||||||
&session.sink,
|
&session.sink,
|
||||||
)?;
|
)?;
|
||||||
session
|
|
||||||
}
|
}
|
||||||
// Nothing is live for this one -- a session whose launch failed
|
// Nothing is live for this one -- a session whose launch failed
|
||||||
// when the server started, which has no pump either. That is the
|
// when the server started, which has no pump either. That is the
|
||||||
@@ -1166,19 +1173,15 @@ impl SessionManager {
|
|||||||
None,
|
None,
|
||||||
self.notifications.clone(),
|
self.notifications.clone(),
|
||||||
)?;
|
)?;
|
||||||
inner.live.insert(id.to_string(), Arc::clone(&session));
|
inner.live.insert(id.to_string(), session);
|
||||||
session
|
|
||||||
}
|
}
|
||||||
};
|
}
|
||||||
// The recorded status is `Exited` and this has just made it untrue.
|
// Nothing is announced from here. A driver that starts a process
|
||||||
// Said here because nothing else will say it: a CLI that has been
|
// reports the session idle itself, in order with everything else it
|
||||||
// given no work writes nothing, so the session would sit at
|
// says about that process -- see `ClaudeDriver::launch`. Saying it
|
||||||
// `Exited` -- refusing every command, refusing every message, and
|
// here as well would be a second writer of the same fact, and the
|
||||||
// showing a phone an offer to start a second process against the
|
// one that cannot see whether the process it is describing is still
|
||||||
// conversation this one is already running.
|
// there.
|
||||||
let _ = session.sink.send(Event::Status {
|
|
||||||
state: SessionStatus::Idle,
|
|
||||||
});
|
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -1501,29 +1504,6 @@ fn launch(
|
|||||||
&sink,
|
&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 {
|
let commands = Arc::new(Commands {
|
||||||
driver: Arc::clone(&driver),
|
driver: Arc::clone(&driver),
|
||||||
sink: sink.clone(),
|
sink: sink.clone(),
|
||||||
@@ -2492,9 +2472,19 @@ mod tests {
|
|||||||
|
|
||||||
manager.start_session(&info.id).expect("start again");
|
manager.start_session(&info.id).expect("start again");
|
||||||
collect_until(&mut rx, is_idle).await;
|
collect_until(&mut rx, is_idle).await;
|
||||||
// Idle rather than exited, and said by the start rather than left
|
// Idle rather than exited, and *recorded* -- said by the driver that
|
||||||
// for a driver that has been given no work to say for itself.
|
// 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!(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,
|
// The same live session throughout: only the driver was replaced,
|
||||||
// so nothing a phone is reading was interrupted.
|
// so nothing a phone is reading was interrupted.
|
||||||
assert!(Arc::ptr_eq(
|
assert!(Arc::ptr_eq(
|
||||||
|
|||||||
Reference in new issue
Block a user