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 <noreply@anthropic.com>
This commit is contained in:
1 parent
4a122f7b25
commit
1a132b4de3
4 files changed
+115
-36
No files matched your search
@@ -291,12 +291,15 @@ day:
|
|||||||
which keeps the session and its transcript, and `POST .../start` brings
|
which keeps the session and its transcript, and `POST .../start` brings
|
||||||
the process back on the same conversation — or delete the session, which
|
the process back on the same conversation — or delete the session, which
|
||||||
ends the conversation too.
|
ends the conversation too.
|
||||||
- **Sending a message to a stopped session starts it.** `POST
|
- **A message or a command sent to a stopped session starts it.** `POST
|
||||||
.../message` goes through `SessionManager::send_message`, which starts a
|
.../message`, `.../command` and `.../compact` go through
|
||||||
process first when the session is known to have exited and then delivers
|
`SessionManager::send_message` and `::run_command`, which start a process
|
||||||
the message to the driver that has one behind it. Only on `exited`:
|
first when the session is known to have exited and then hand the thing to
|
||||||
`unknown` has a process that may well be reading its fifo. So the Start
|
the driver that has one behind it. Only on `exited`: `unknown` has a
|
||||||
button is for when you want a process and nothing to say to it yet.
|
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`,
|
- **Each session directory now holds `process.json`, `stdin.fifo`,
|
||||||
`stdout.log` and `stderr.log`.** `stdout.log` is the driver's input, read
|
`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
|
from the byte offset in `process.json`; removing either by hand while the
|
||||||
|
|||||||
@@ -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
|
existed for the backend going away — and it is the whole of what a driver
|
||||||
whose process has exited is owed.
|
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
|
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
|
other button, press it, type the thing again. Both plainly mean "do this
|
||||||
this now", and `--resume` puts the new process on the same conversation, so
|
now", and `--resume` puts the new process on the same conversation, so
|
||||||
nothing about the message changes — only whether there was anything there to
|
nothing about what was typed changes — only whether there was anything there
|
||||||
read it. The manager's `send_message` and the Start button ask one function
|
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
|
(`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
|
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
|
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
|
also what stops two requests arriving together from starting two CLIs. Only
|
||||||
`Exited` starts anything, for the reason above — `Unknown` has a process that
|
`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
|
may well be reading its fifo, and what was typed goes to the driver as it
|
||||||
did.
|
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
|
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
|
is in flight, so a second press cannot be decided against a status the first
|
||||||
|
|||||||
+20
-9
@@ -24,6 +24,7 @@
|
|||||||
//! POST /sessions/{id}/title {title}
|
//! POST /sessions/{id}/title {title}
|
||||||
//! POST /sessions/{id}/model {model}
|
//! POST /sessions/{id}/model {model}
|
||||||
//! POST /sessions/{id}/command {text} -- /compact, /clear, /rename x, or the dialect's own
|
//! 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}/compact
|
||||||
//! POST /sessions/{id}/attachments multipart image upload -> {id}, referenced by /message
|
//! POST /sessions/{id}/attachments multipart image upload -> {id}, referenced by /message
|
||||||
//! GET /sessions/{id}/files/{name} images the session produced or was sent
|
//! 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()),
|
Some((name, rest)) => (name, rest.trim()),
|
||||||
None => (text, ""),
|
None => (text, ""),
|
||||||
};
|
};
|
||||||
match (name, rest) {
|
// Everything but a rename goes through the manager, which starts the
|
||||||
("/compact", _) => lookup(&manager, &id)?.run_command(SessionCommand::Compact),
|
// session's process first if it has exited. A rename is persisted and
|
||||||
("/clear", _) => lookup(&manager, &id)?.run_command(SessionCommand::Clear),
|
// listed whether or not a process hears about it, so it neither needs
|
||||||
// Through the manager, not the session: a name is persisted and
|
// one nor is worth starting one for.
|
||||||
// listed as well as forwarded, and that is one operation.
|
let command = match (name, rest) {
|
||||||
|
("/compact", _) => SessionCommand::Compact,
|
||||||
|
("/clear", _) => SessionCommand::Clear,
|
||||||
("/rename", "") => return Err(bad_request(anyhow::anyhow!("a session needs a name"))),
|
("/rename", "") => return Err(bad_request(anyhow::anyhow!("a session needs a name"))),
|
||||||
("/rename", title) => manager.rename_session(&id, title).map_err(bad_request)?,
|
("/rename", title) => {
|
||||||
_ => lookup(&manager, &id)?.run_command(SessionCommand::Raw(text.to_string())),
|
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)
|
Ok(StatusCode::NO_CONTENT)
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -856,7 +864,10 @@ async fn compact(
|
|||||||
State(manager): State<Arc<SessionManager>>,
|
State(manager): State<Arc<SessionManager>>,
|
||||||
UrlPath(id): UrlPath<String>,
|
UrlPath(id): UrlPath<String>,
|
||||||
) -> Result<StatusCode, ApiError> {
|
) -> Result<StatusCode, ApiError> {
|
||||||
lookup(&manager, &id)?.compact();
|
lookup(&manager, &id)?;
|
||||||
|
manager
|
||||||
|
.run_command(&id, SessionCommand::Compact)
|
||||||
|
.map_err(bad_request)?;
|
||||||
Ok(StatusCode::NO_CONTENT)
|
Ok(StatusCode::NO_CONTENT)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+68
-14
@@ -399,13 +399,6 @@ impl LiveSession {
|
|||||||
self.driver().detach();
|
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<SeqEvent> {
|
pub fn subscribe(&self) -> broadcast::Receiver<SeqEvent> {
|
||||||
self.events.subscribe()
|
self.events.subscribe()
|
||||||
}
|
}
|
||||||
@@ -1150,6 +1143,42 @@ impl SessionManager {
|
|||||||
Ok(())
|
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
|
/// 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`
|
/// reports what the session was found to be doing either way. `Exited`
|
||||||
/// is therefore the one returned value that means something was started.
|
/// 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
|
/// A message and a command both mean "now", so neither answers that the
|
||||||
/// that the session's process has gone -- it starts one and delivers
|
/// session's process has gone -- they start one and go to it.
|
||||||
/// the message to it.
|
|
||||||
///
|
///
|
||||||
/// Refusing was the old behaviour and it was work handed back: read the
|
/// Refusing was the old behaviour and it was work handed back: read the
|
||||||
/// status word, find the other button, press it, type the message
|
/// status word, find the other button, press it, type the thing again.
|
||||||
/// again. `--resume` puts the new process on the same conversation, so
|
/// `--resume` puts the new process on the same conversation, so what it
|
||||||
/// the message it reads is the one that was typed.
|
/// 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]
|
#[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 dir = tempfile::tempdir().expect("tempdir");
|
||||||
let config_path = dir.path().join("config.ron");
|
let config_path = dir.path().join("config.ron");
|
||||||
let data_dir = dir.path().join("sessions");
|
let data_dir = dir.path().join("sessions");
|
||||||
@@ -2657,6 +2692,25 @@ mod tests {
|
|||||||
// delivered to a session still reporting `exited` is one the phone
|
// delivered to a session still reporting `exited` is one the phone
|
||||||
// draws under a Start button.
|
// draws under a Start button.
|
||||||
assert_ne!(manager.sessions()[0].status, SessionStatus::Exited);
|
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
|
/// The status is a claim about a process, and the process record is
|
||||||
|
|||||||
Reference in new issue
Block a user