diff --git a/AGENTS.md b/AGENTS.md index a89b96e..7c5782e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,7 +89,11 @@ repo is in PLAN.md's "Backend layout" section. codepoint in the Kotlin that the script did not subset is a glyph that silently isn't there. Rerun the script and commit its output when adding one; it needs network access. `md-cog` and `md-refresh` are deliberately - the same codepoints dev-updater uses and must not drift from it. + the same codepoints dev-updater uses and must not drift from it. The + subset is the **Mono** face, where every glyph is one em square — that is + what makes two icon buttons the same width without either being given + one, and it is why `GLYPH_SIZE` is smaller than it looks like it should + be. - `.dev-updater.ron` — what Dev Updater is asked to do with this checkout: the server (built in `server/`, run as `service: Managed(...)`) and the APK (built in `app/`), built in parallel. The project it serves is the @@ -221,16 +225,19 @@ first if a remote spawn ever mangles an argument. The emulator app reaches it at `https://10.0.2.2:8443`; enroll it with `adb -s "$SERIAL" shell "am start -a android.intent.action.VIEW -d 'aiapp://enroll?host=10.0.2.2&port=8443&token=…'"` (quote so the device shell doesn't eat the `&`s). -- **Name the device on every `adb` call.** This checkout runs its own - emulator — `run-android.sh` derives the AVD name from the directory, so - a second clone gets a second AVD rather than queueing for one shared - machine-wide `tdep`. With more than one attached, a bare `adb shell` or - `adb get-state` fails with `more than one device/emulator` and a bare - `adb shell pm list packages` comes back **empty**, which reads as the app - having been uninstalled rather than as the question being ambiguous. - Get the serial the way `run-android.sh` does — match `adb -s emu - avd name` against the AVD — or export `ANDROID_SERIAL`, which every - `adb` call honours without `-s`. +- **The emulator is `~/repos/emulator-tools`' business, not this repo's.** + `emu up` creates and boots the AVD named after this checkout (`ai-app-2`), + refusing when the machine has no room for one; `emu list` says what is + attached and what it costs; `emu down` stops it. `run-android.sh` is that + plus a build and an install. Run that repo's `install.sh` once if `emu` is + missing. + The `adb` on `PATH` after sourcing `android-env.sh` is that repo's wrapper, + which fills in `-s` from the same rule — so a bare `adb shell` reaches this + checkout's emulator and refuses to reach another one's. That defaulting is + what makes the old advice unnecessary rather than wrong: with two attached + and no `-s`, a bare `adb shell pm list packages` comes back **empty**, + which reads as the app having been uninstalled rather than as the question + being ambiguous. ## Where things run (host vs this VM) @@ -280,15 +287,21 @@ day: - **Stopping the server no longer stops the sessions.** After `pkill ai-server` the `claude` processes are still there, on purpose, and the next start picks them up (`reattaching to the claude-cli it left - running` in the log). To actually end one, delete the session — that is - the only path that stops a process. + running` in the log). To end one, either `POST /sessions/{id}/stop` — + 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. - **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 session is live loses output or replays it. - **`--resume` only ever runs when nothing is running.** That check is the fix for the incident below, and the reason there is one entry point - (`ClaudeDriver::launch`) rather than a spawn and an attach. + (`ClaudeDriver::launch`) rather than a spawn and an attach. The status a + launch reports obeys the same rule: a session recorded as `exited` whose + 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. - 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 2817db8..d2be19c 100644 --- a/PLAN.md +++ b/PLAN.md @@ -298,6 +298,65 @@ Two consequences worth stating: going away and means to come back) and `stop` (the session is being deleted, so the process must not survive). Every driver owes exactly one of them. +### Stopping and starting a session's process (decided 2026-08-30) + +If a session outlives the backend, the person holding the phone needs the +other direction too: **end the process without ending the session, and start +it again on the same conversation.** `POST /sessions/:id/stop` and +`/start`. + +Three decisions worth not undoing: + +- **Stop signals the recorded process and says nothing else.** It does not + go through the driver and it does not announce `Exited`. The record is the + session's rather than any dialect's, so signalling it here works for a + session whose driver is in no state to be asked and adds no trait method a + new driver could implement wrongly — and the driver's own reader already + reports the death correctly, draining the last output and recording the + status. Announcing it from here would be a guess arriving ahead of the + measurement, and wrong for the grace period a process that ignores SIGTERM + keeps running. +- **Start replaces the driver and nothing else.** The transcript, the event + pump and the SSE stream every open phone is reading stay where they were, + so starting a session again is not a reconnect for anybody watching, and + there is still exactly one writer of the transcript — which relaunching + the whole `LiveSession` would not be, since the old pump outlives its + session and an imported session's sync task would go on feeding it. + `LiveSession` and `Commands` therefore share one `Mutex>` + rather than each holding a copy. +- **Start is 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 exactly the two-CLIs-on-one-conversation fault + `session::process` exists to prevent. + +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 — +`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 +running (interrupt — the process stays), a red stop when it is not (end the +process), and a green play when it has exited (start it again). One button +rather than three that come and go, so its presence is never the signal. + ### Importing refuses a session that is already open (decided 2026-08-29) Claude Code keeps a descriptor per live session at @@ -448,7 +507,9 @@ POST /sessions spawn {provider, host, model, cwd, permiss GET /sessions/:id/events?after=N SSE: transcript replay from N, then live POST /sessions/:id/message {text, attachment_ids} POST /sessions/:id/answer {question_id, answer} (questions and permissions) -POST /sessions/:id/interrupt +POST /sessions/:id/interrupt stop the running turn; the process stays +POST /sessions/:id/stop end the process; the session and transcript stay +POST /sessions/:id/start run the process again, continuing the conversation POST /sessions/:id/model {model} POST /sessions/:id/compact (llama sessions) POST /sessions/:id/attachments multipart upload → id (referenced by /message) diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt index b572694..af14e79 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt @@ -558,6 +558,22 @@ fun interruptSession(settings: ServerSettings, sessionId: String) { requestFromServer(settings, "/sessions/$sessionId/interrupt", method = "POST") {} } +/** + * Ends the process behind a session, leaving the session and its transcript. + * + * Not a delete and not an interrupt: the conversation stays exactly where it is and [startSession] + * picks it back up. The server reports what it could not do -- there was nothing running, or the + * machine would not say whether there was -- rather than answering the same way either way. + */ +fun stopSession(settings: ServerSettings, sessionId: String) { + requestFromServer(settings, "/sessions/$sessionId/stop", method = "POST") {} +} + +/** Starts the process again on the conversation it left. See [stopSession]. */ +fun startSession(settings: ServerSettings, sessionId: String) { + requestFromServer(settings, "/sessions/$sessionId/start", method = "POST") {} +} + /** * Removes a Claude Code session from the machine. * diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/NerdIcons.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/NerdIcons.kt index 71f126b..7910a67 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/NerdIcons.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/NerdIcons.kt @@ -31,6 +31,12 @@ import androidx.compose.ui.unit.sp * tofu. Adding one means adding its codepoint in *both* places; a codepoint here that the script * did not subset is a glyph that silently isn't there. * + * The subset is the font's **Mono** face, where every glyph is exactly one em wide and one em tall. + * That is what makes two icons the same size without either of them being given a size: the + * proportional face's advances run from 0.46 em to 0.92 em, so a Send button and a Stop button side + * by side came out visibly different widths, and matching them at the call site would have meant + * one hardcoded measurement per pair. [GLYPH_SIZE] carries the cost. + * * The same arrangement as dev-updater, down to the cog and the refresh arrow being the same two * Material Design codepoints. Those two must not drift: an icon that means "settings" in one app * and something else in the other is the failure this is worth preventing. The script is copied @@ -52,9 +58,27 @@ val REFRESH_GLYPH = glyph(0xF0450) /** `md-send` -- the filled paper plane: submit what is in the composer. */ val SEND_GLYPH = glyph(0xF048A) -/** `md-stop` -- a filled square: interrupt the turn that is running. */ +/** + * `md-stop` -- a filled square: end the process behind this session. + * + * The square is what stop has meant since tape decks, and it is spent here on the thing that + * actually stops rather than on pausing. [PAUSE_GLYPH] is the turn; this is the session. + */ val STOP_GLYPH = glyph(0xF04DB) +/** + * `md-pause` -- two bars: take the running turn away and leave the session there. + * + * The pair with [STOP_GLYPH] and [PLAY_GLYPH] is the point: one button in the composer says what + * pressing it now would do to the process, and the three marks are the three answers. An interrupt + * ends a turn and nothing else -- the CLI is still there and still holds the conversation -- which + * is a pause, not a stop, and drawing it as a square said otherwise. + */ +val PAUSE_GLYPH = glyph(0xF03E4) + +/** `md-play` -- start the process again, on the conversation it left. See [PAUSE_GLYPH]. */ +val PLAY_GLYPH = glyph(0xF040A) + /** * `md-send_clock` -- the same paper plane with a clock on it: this message will wait its turn. * @@ -142,5 +166,13 @@ fun Glyph( Text(glyph, fontFamily = NerdIcons, fontSize = size, color = colour, modifier = modifier) } -/** The size an icon draws at beside a line of text. */ -private val GLYPH_SIZE = 20.sp +/** + * The size an icon draws at beside a line of text. + * + * 17 rather than the 20 it was while the font was the proportional face. A glyph there filled at + * most 0.83 em of its point size and most filled a good deal less, so the number was standing in + * for the headroom above the tallest one; in the Mono face every glyph fills its em exactly, and + * keeping 20 would have made every icon in the app step up by a fifth for no reason anybody asked + * for. This is what the largest of them already drew at. + */ +private val GLYPH_SIZE = 17.sp 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 33b2638..f7bcd3e 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt @@ -1478,30 +1478,44 @@ fun SessionScreen( }, ) } - if (running) { - // The same filled shape as the button beside it, not an outlined one: these - // are two things you can do about the turn that is running, and weighting one - // of them as secondary said they were a primary action and its qualifier. - // What separates them is the colour and the mark, which is what they mean. - Button( - onClick = { act { interruptSession(settings, summary.id) } }, - colors = actionButtonColors(stopColor), - ) { - // A filled square, which is what stop has looked like since tape decks. - Glyph( - STOP_GLYPH, - colour = LocalContentColor.current, - modifier = Modifier.semantics { contentDescription = "Stop" }, - ) + // The same filled shape as the button beside it, not an outlined one: these are + // two things you can do about the session, and weighting one of them as secondary + // said they were a primary action and its qualifier. What separates them is the + // colour and the mark, which is what they mean. + // + // Always here, rather than arriving with the turn as it used to. A control that + // comes and goes makes its own presence the signal, and its absence could not say + // whether there was nothing to do; a button that is always in the same place also + // cannot push Send off the end of the row by turning up. + val process = + when { + running -> ProcessAction.Pause + status == "exited" -> ProcessAction.Start + else -> ProcessAction.Stop } - Spacer(Modifier.width(8.dp)) + Button( + onClick = { act { process.perform(settings, summary.id) } }, + colors = actionButtonColors(process.colour()), + ) { + Glyph( + process.glyph, + colour = LocalContentColor.current, + modifier = Modifier.semantics { contentDescription = process.label }, + ) } + Spacer(Modifier.width(8.dp)) // The paper plane, with a clock on it while a turn is in flight: sending then // queues the message for the next tool boundary rather than starting a turn of // its own, and the two have to be told apart at a glance. The label says the same // thing to a screen reader, which has nothing else to read. + // + // Disabled while there is nothing to send, rather than pressable and silent: + // `send` has always returned early on an empty composer, so the button promised + // something it would not do, and the only feedback was the ripple. Disabled and + // not hidden, for the reason the button beside it is always here. Button( onClick = { send() }, + enabled = input.isNotBlank() || pendingAttachments.isNotEmpty(), colors = actionButtonColors(if (running) queueColor else sendColor), ) { Glyph( @@ -1522,6 +1536,39 @@ fun SessionScreen( /** What pressing Send does right now, said the same way to the eye and to a screen reader. */ private fun sendLabel(running: Boolean) = if (running) "Queue" else "Send" +/** + * What the composer's process button would do if it were pressed now. + * + * One value rather than four parallel conditions over the status, because the mark, the colour, the + * name a screen reader is given and the request that goes out are four halves of one decision. A + * button drawn as a pause that terminates the CLI is the worst bug available here, and separate + * branches over the same condition are how that happens -- these three each have to cover every + * case, and the compiler says so. + */ +private enum class ProcessAction(val glyph: String, val label: String) { + /** A turn is running: take it back, and leave the process holding the conversation. */ + Pause(PAUSE_GLYPH, "Pause"), + /** Nothing is running, but the process behind the session is: end it. */ + Stop(STOP_GLYPH, "Stop"), + /** The process is gone: start it again, on the conversation it left. */ + Start(PLAY_GLYPH, "Start"), +} + +@Composable +private fun ProcessAction.colour() = + when (this) { + ProcessAction.Pause -> pauseColor + ProcessAction.Stop -> stopColor + ProcessAction.Start -> startColor + } + +private fun ProcessAction.perform(settings: ServerSettings, sessionId: String) = + when (this) { + ProcessAction.Pause -> interruptSession(settings, sessionId) + ProcessAction.Stop -> stopSession(settings, sessionId) + ProcessAction.Start -> startSession(settings, sessionId) + } + /** * An inline transcript image, fetched (authenticated, pinned) from the session's files route. The * bitmap is remembered per ref, so scrolling doesn't refetch. diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt index 18146c6..9d084dc 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Theme.kt @@ -213,13 +213,14 @@ val overLimitColor: Color @Composable get() = MaterialTheme.colorScheme.error /** - * The composer's three buttons, coloured by what pressing one does rather than by where it sits. + * The composer's buttons, coloured by what pressing one does rather than by where it sits. * - * Green sends now, blue sends later, red takes the running turn away. The pair of greens and the - * pair of reds elsewhere in this file are deliberate near-collisions worth naming: [runningColor] - * is green because a session is working, and [failedColor] is red because one fell over -- those - * are *states*, and these are *actions*. A reader never has to tell them apart, because nothing - * here is a state and nothing there is pressable. + * Green makes something happen now, blue makes it happen later, orange takes back what is in + * flight, red ends the process. The near-collisions with the states above are deliberate and worth + * naming rather than collapsing: [runningColor] is green because a session is working, + * [failedColor] is red because one fell over, [awaitingColor] is the same orange because a session + * is waiting on somebody -- those are *states*, and these are *actions*. A reader never has to tell + * them apart, because nothing here is a state and nothing there is pressable. */ val sendColor: Color @Composable get() = Mocha.Green @@ -228,10 +229,30 @@ val sendColor: Color val queueColor: Color @Composable get() = Mocha.Blue -/** Interrupting the running turn -- the one button here that takes something away. */ +/** + * Interrupting the running turn: the work stops and the session stays. + * + * Orange rather than red because of how much it takes: only what is in flight. The process is still + * there holding the conversation, and the next message starts a turn as though nothing had + * happened. Red is spent on [stopColor], which is the same button in the same place when what it + * would end is the session's process. + */ +val pauseColor: Color + @Composable get() = Mocha.Peach + +/** Ending the session's process -- the one button here that takes something away. */ val stopColor: Color @Composable get() = Mocha.Red +/** + * Starting the process again, on the conversation it left. + * + * The same green as [sendColor] on purpose: both mean "this happens now", and they are never the + * same button -- the process button only offers to start when there is nothing running to stop. + */ +val startColor: Color + @Composable get() = Mocha.Green + /** * A filled button in one of the action colours above. * diff --git a/app/androidApp/src/main/res/font/nerd_icons.ttf b/app/androidApp/src/main/res/font/nerd_icons.ttf index ef7791a..2001639 100644 Binary files a/app/androidApp/src/main/res/font/nerd_icons.ttf and b/app/androidApp/src/main/res/font/nerd_icons.ttf differ diff --git a/app/build-icon-font.sh b/app/build-icon-font.sh index 873af93..a04c4e9 100755 --- a/app/build-icon-font.sh +++ b/app/build-icon-font.sh @@ -32,6 +32,8 @@ GLYPHS=( U+F0450 # md-refresh U+F048A # md-send U+F04DB # md-stop + U+F03E4 # md-pause + U+F040A # md-play U+F1163 # md-send_clock U+F0156 # md-close U+F004D # md-arrow_left @@ -52,9 +54,20 @@ python3 -m venv "$work/venv" unicodes="$(IFS=,; echo "${GLYPHS[*]}")" mkdir -p "$(dirname "$out")" -# The proportional face rather than the Mono one: these are drawn inline -# beside text, where a fixed advance would pad each icon out to a cell. -"$work/venv/bin/pyftsubset" "$work/SymbolsNerdFont-Regular.ttf" \ +# The Mono face rather than the proportional one, which this used until +# 2026-08-30. Every glyph in it is one em wide and one em tall, so two +# icons drawn at the same size are the same size -- which is what makes two +# icon buttons beside each other match without either of them being told a +# width. In the proportional face the advances run from 0.46 em (play) to +# 0.92 em (line chart), so the composer's Send button came out visibly wider +# than the Stop button next to it, and any fix at the call site would have +# been one measurement hardcoded per pair. +# +# The trade is the one the old comment named: an icon inline beside text is +# padded out to a cell. That is worth it, and it is also why GLYPH_SIZE in +# NerdIcons.kt came down when this changed -- a glyph that fills its em +# draws bigger at the same point size than one that does not. +"$work/venv/bin/pyftsubset" "$work/SymbolsNerdFontMono-Regular.ttf" \ --unicodes="$unicodes" \ --layout-features= \ --drop-tables+=DSIG \ diff --git a/app/run-android.sh b/app/run-android.sh index 0da7949..3b65dae 100755 --- a/app/run-android.sh +++ b/app/run-android.sh @@ -1,7 +1,13 @@ #!/bin/sh -# Builds and runs this app on an emulator, creating/booting the AVD first if -# it isn't already up. Same flow as dev-updater's run-android.sh; see that -# script for the reasoning behind the avd handling. +# Builds this app and runs it on this checkout's emulator. +# +# The emulator half of this -- which AVD this checkout means, creating it, +# booting it headless, and refusing to start one the machine has no room for +# -- lives in ~/repos/emulator-tools and is shared with every other Android +# checkout here. This script kept its own copy of that sequence until +# 2026-08-30, as did dev-updater's and ai-app's, and three copies of "boot an +# emulator" is three places for the memory check that was missing from all of +# them. # # Environment setup (SDK location, PATH, ...) lives in ./android-env.sh, # which can also be sourced directly for one-off commands. @@ -9,105 +15,34 @@ set -eu APP_ID="com.example.aiapp" -DEVICE_PROFILE="${DEVICE_PROFILE:-pixel_10}" -SYSTEM_IMAGE="${SYSTEM_IMAGE:-system-images;android-36;google_apis;x86_64}" - SCRIPT_DIR=$(cd "$(dirname "$0")" && pwd) cd "$SCRIPT_DIR" -# One AVD per checkout, named after it -- so two clones of this repo, or a -# clone and a worktree, each get their own rather than fighting over one. -# This used to default to a machine-wide "tdep", which made the emulator the -# one thing here that could not be worked on in parallel: taking it meant -# asking whoever had it, waiting, and handing it back, and installing onto a -# running one steals the foreground from whatever they were looking at. -# Derived rather than written down, so neither clone names the other's. -# Override with AVD_NAME=... to share one deliberately. -AVD_NAME="${AVD_NAME:-$(basename "$(dirname "$SCRIPT_DIR")")}" - # shellcheck source=./android-env.sh . ./android-env.sh -# Prints the adb serial of a running instance of AVD "$1", or nothing. -avd_serial() { - for s in $(adb devices | awk '$2 == "device" {print $1}'); do - if [ "$(adb -s "$s" emu avd name 2>/dev/null | head -n1 | tr -d '\r')" = "$1" ]; then - echo "$s" - return 0 - fi - done -} - -echo "==> Ensuring emulator system image is installed" -android sdk install emulator "$SYSTEM_IMAGE" || echo " (non-fatal: see above)" - -if [ ! -f "$ANDROID_AVD_HOME/$AVD_NAME.ini" ]; then - echo "==> Creating AVD '$AVD_NAME' ($DEVICE_PROFILE, $SYSTEM_IMAGE)" - echo no | avdmanager create avd \ - -n "$AVD_NAME" \ - -k "$SYSTEM_IMAGE" \ - --device "$DEVICE_PROFILE" \ - --sdcard 512M -else - echo "==> Reusing existing AVD '$AVD_NAME'" +if ! command -v emu >/dev/null 2>&1; then + echo "run-android.sh: no 'emu' command." >&2 + echo " It comes from ~/repos/emulator-tools; run that repo's ./install.sh." >&2 + exit 127 fi -# Host keyboard into the emulator -- this app has text fields. -CONFIG_INI="$ANDROID_AVD_HOME/$AVD_NAME.avd/config.ini" -if [ -f "$CONFIG_INI" ]; then - grep -v '^hw\.keyboard=' "$CONFIG_INI" >"$CONFIG_INI.tmp" - echo "hw.keyboard=yes" >>"$CONFIG_INI.tmp" - mv "$CONFIG_INI.tmp" "$CONFIG_INI" -fi - -SERIAL=$(avd_serial "$AVD_NAME") -if [ -n "$SERIAL" ]; then - echo "==> Emulator '$AVD_NAME' already running ($SERIAL)" -else - # Clean up a stray/crashed process for this AVD, if any. The bracketed - # first character keeps the pattern from matching the shell running - # this script -- unbracketed, this kills that shell mid-run. - pkill -f "[e]mulator.*-avd $AVD_NAME" >/dev/null 2>&1 || true - - EMU_LOG="/tmp/$AVD_NAME-emulator.log" - : >"$EMU_LOG" - if [ -n "${DISPLAY:-}" ] || [ -n "${WAYLAND_DISPLAY:-}" ]; then - echo "==> Starting emulator '$AVD_NAME' with GPU acceleration (-gpu host)" - emulator -avd "$AVD_NAME" -gpu host -no-audio >"$EMU_LOG" 2>&1 & - else - echo "==> No display -- starting emulator '$AVD_NAME' headless (-gpu swiftshader_indirect)" - emulator -avd "$AVD_NAME" -gpu swiftshader_indirect -no-audio -no-window \ - >"$EMU_LOG" 2>&1 & - fi - EMU_PID=$! - - i=0 - booted="" - while [ "$i" -lt 150 ]; do - if ! kill -0 "$EMU_PID" 2>/dev/null; then - echo "Emulator process exited unexpectedly. Log output:" >&2 - cat "$EMU_LOG" >&2 - exit 1 - fi - SERIAL=$(avd_serial "$AVD_NAME") - if [ -n "$SERIAL" ]; then - booted=$(adb -s "$SERIAL" shell getprop sys.boot_completed 2>/dev/null | tr -d '\r') - [ "$booted" = "1" ] && break - fi - i=$((i + 1)) - sleep 2 - done - if [ "$booted" != "1" ]; then - echo "Emulator did not finish booting in time. Log output:" >&2 - cat "$EMU_LOG" >&2 - exit 1 - fi -fi +# Prints the serial, having created and booted the AVD if it had to. Named +# after the checkout, so this cannot land on another session's emulator -- +# and refuses rather than starting one when the machine is short of memory, +# because what an OOM kills is somebody else's work rather than the emulator +# that asked for the memory. +echo "==> Emulator" +SERIAL=$(emu up) +export ANDROID_SERIAL="$SERIAL" echo "==> Building debug APK" ./gradlew :androidApp:assembleDebug APK="androidApp/build/outputs/apk/debug/androidApp-debug.apk" echo "==> Installing and launching $APK" -adb -s "$SERIAL" install -r "$APK" -adb -s "$SERIAL" shell am start -n "$APP_ID/.MainActivity" +# ANDROID_SERIAL above is what aims these; the adb wrapper would work it out +# from the checkout anyway, but a script that says which device it means does +# not depend on being run from the right directory. +adb install -r "$APK" +adb shell am start -n "$APP_ID/.MainActivity" diff --git a/server/src/routes.rs b/server/src/routes.rs index 247a7f6..a369fff 100644 --- a/server/src/routes.rs +++ b/server/src/routes.rs @@ -17,7 +17,9 @@ //! `reset` frame plus the newest window) //! POST /sessions/{id}/message {text, attachmentIds?} //! POST /sessions/{id}/answer {questionId, answers} (questions and permissions) -//! POST /sessions/{id}/interrupt +//! POST /sessions/{id}/interrupt stop the running turn; the process stays +//! POST /sessions/{id}/stop end the process; the session and transcript stay +//! POST /sessions/{id}/start run the process again, continuing the conversation //! POST /sessions/{id}/title {title} //! POST /sessions/{id}/model {model} //! POST /sessions/{id}/command {text} -- /compact, /clear, /rename x, or the dialect's own @@ -87,6 +89,8 @@ pub fn router(manager: Arc) -> Router { .route("/sessions/{id}/message", post(message)) .route("/sessions/{id}/answer", post(answer)) .route("/sessions/{id}/interrupt", post(interrupt)) + .route("/sessions/{id}/stop", post(stop)) + .route("/sessions/{id}/start", post(start)) .route("/sessions/{id}/title", post(rename)) .route("/sessions/{id}/model", post(set_model)) .route("/sessions/{id}/permission-mode", post(set_permission_mode)) @@ -678,6 +682,31 @@ async fn interrupt( Ok(StatusCode::NO_CONTENT) } +/// Ends the session's process. The session stays, and `start` brings it +/// back -- see [`SessionManager::stop_session`]. +/// +/// Not `lookup`ed: a session that failed to relaunch has no live entry and +/// may still have a process running, which is exactly one worth being able +/// to stop. +async fn stop( + State(manager): State>, + UrlPath(id): UrlPath, +) -> Result { + manager.stop_session(&id).map_err(bad_request)?; + Ok(StatusCode::NO_CONTENT) +} + +/// Starts a process for a session that has none, continuing the same +/// conversation -- see [`SessionManager::start_session`], which refuses +/// unless the session is known to have exited. +async fn start( + State(manager): State>, + UrlPath(id): UrlPath, +) -> Result { + manager.start_session(&id).map_err(bad_request)?; + Ok(StatusCode::NO_CONTENT) +} + /// The usage screen needs two things that live in different places: the /// cache, and the current list of machines to ask. Carried together rather /// than the monitor holding the manager, which would point the dependency diff --git a/server/src/session/claude.rs b/server/src/session/claude.rs index 2dace08..0805307 100644 --- a/server/src/session/claude.rs +++ b/server/src/session/claude.rs @@ -100,12 +100,6 @@ pub(super) mod translate; const RESUME_FILE: &str = "claude-session.json"; -/// Grace period between asking a process to stop and killing it. -/// -/// Only [`Driver::stop`] uses it -- a session being deleted. Detaching -/// does not stop anything, so it has no grace period and needs none. -const STOP_GRACE: std::time::Duration = std::time::Duration::from_secs(5); - /// Messages handed to the CLI that it has not visibly acted on yet. /// /// The CLI *does* take a message written mid-turn: it goes into the next @@ -239,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`. @@ -249,6 +247,7 @@ impl ClaudeDriver { provider.name, record.pid ); + started_here = false; record } // Recorded, and the machine will not say whether it is still @@ -262,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. @@ -673,7 +699,7 @@ impl Driver for ClaudeDriver { fn stop(&self) { self.reading.store(false, Ordering::SeqCst); if let Some(record) = process::live(&self.session_dir) { - process::stop(&record, STOP_GRACE); + process::stop(&record, process::STOP_GRACE); } process::clear(&self.session_dir); } diff --git a/server/src/session/llama.rs b/server/src/session/llama.rs index f65b669..9c5f3a0 100644 --- a/server/src/session/llama.rs +++ b/server/src/session/llama.rs @@ -269,9 +269,6 @@ const SERVER_LOG: &str = "llama-server.log"; /// seconds late costs nothing. const WATCH_INTERVAL: std::time::Duration = std::time::Duration::from_secs(2); -/// Grace period between asking llama-server to stop and killing it. -const STOP_GRACE: std::time::Duration = std::time::Duration::from_secs(5); - /// An owner-only log opened for appending, so the two streams pointed at /// it do not overwrite each other and a reattach keeps what came before. fn log_file(path: &Path) -> Result { @@ -447,7 +444,7 @@ impl Driver for LlamaDriver { fn stop(&self) { self.cancel.store(true, Ordering::Relaxed); if let Some(record) = process::live(&self.session_dir) { - process::stop(&record, STOP_GRACE); + process::stop(&record, process::STOP_GRACE); } process::clear(&self.session_dir); } diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index b53fc0d..f29f5eb 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -169,11 +169,21 @@ pub struct SessionInfo { pub created: f64, } +/// What is running a session at this moment. +/// +/// Behind a lock because a session outlives its process: stopping one and +/// starting it again replaces the driver while the transcript, the event +/// pump and the stream every open phone is reading stay exactly where they +/// were. Shared with [`Commands`] rather than copied into it, because two +/// holders of "the driver" are two answers to that question the moment one +/// of them is replaced. +type DriverCell = Arc>>; + /// A running session: its driver plus the shared state the event pump /// keeps current. Cheap to clone-by-`Arc` into request handlers. pub struct LiveSession { meta: SessionConfig, - driver: Arc, + driver: DriverCell, /// Commands asked for and not yet run, oldest first, with the pump /// that will run them. Shared with that pump, which is what notices /// the boundary. @@ -195,12 +205,17 @@ pub struct LiveSession { /// for the turn to end. Drivers therefore never have to think about it, /// and a new provider cannot get it wrong by omission. struct Commands { - driver: Arc, + driver: DriverCell, sink: EventSink, waiting: Mutex>, } impl Commands { + /// Whatever is driving the session now -- see [`DriverCell`]. + fn driver(&self) -> Arc { + self.driver.lock().unwrap().clone() + } + /// Runs `command` now if the session is between turns, holds it until /// it is, and refuses it outright if there will never be one. Whichever /// happened, the phone is told. @@ -236,9 +251,10 @@ impl Commands { }); return; } - if self.driver.between_turns() { + let driver = self.driver(); + if driver.between_turns() { let _ = self.sink.send(Event::CommandSent { id, text }); - command.apply(self.driver.as_ref()); + command.apply(driver.as_ref()); return; } let _ = self.sink.send(Event::CommandQueued { @@ -258,7 +274,8 @@ impl Commands { /// began by itself, which it does: a background task finishing makes it /// pick the conversation back up with nothing written to it. fn take_one(&self) { - if !self.driver.between_turns() { + let driver = self.driver(); + if !driver.between_turns() { return; } let Some((id, command)) = self.waiting.lock().unwrap().pop_front() else { @@ -268,7 +285,7 @@ impl Commands { id, text: command.label(), }); - command.apply(self.driver.as_ref()); + command.apply(driver.as_ref()); } /// Gives up on everything held, because the session cannot run them. @@ -336,6 +353,11 @@ struct Shared { } impl LiveSession { + /// Whatever is driving this session now -- see [`DriverCell`]. + fn driver(&self) -> Arc { + self.driver.lock().unwrap().clone() + } + /// Hands the user's message to the driver, which records it in the /// transcript by reporting that it has taken it -- see `MessageTaken`. /// @@ -348,7 +370,7 @@ impl LiveSession { // drew a person's screenshot as a row floating above the bubble // that sent it, and left the phone inferring from adjacency which // message an image went with -- a thing the sender already knew. - self.driver.send_user_message(text, images); + self.driver().send_user_message(text, images); } pub fn answer_question(&self, question_id: &str, answers: &[String]) { @@ -356,7 +378,7 @@ impl LiveSession { id: question_id.to_string(), answers: answers.to_vec(), }); - self.driver.answer_question(question_id, answers); + self.driver().answer_question(question_id, answers); } /// Asks the session to run a command on itself, now or at the next @@ -367,14 +389,14 @@ impl LiveSession { } pub fn interrupt(&self) { - self.driver.interrupt(); + self.driver().interrupt(); } /// Leaves this session's process running and stops attending to it, /// for a server that is going away and means to come back. See /// [`Driver::detach`]. pub fn detach(&self) { - self.driver.detach(); + self.driver().detach(); } /// Compacts at the next boundary. Through the command queue like @@ -942,7 +964,7 @@ impl SessionManager { // change, and as an error if it cannot. The config above is a // different question -- what to launch this session with next // time -- and it is answered by the request. - session.driver.set_permission_mode(mode); + session.driver().set_permission_mode(mode); } Ok(()) } @@ -1025,11 +1047,144 @@ impl SessionManager { if let Some(session) = inner.live.get(id) { // See `set_session_permission_mode`: the driver reports what // it is set to, this only asks. - session.driver.set_model(model); + session.driver().set_model(model); } Ok(()) } + /// Ends this session's process, leaving the session -- its transcript, + /// its place in the list, everything a phone is watching -- exactly + /// where it is. [`SessionManager::start_session`] is the way back. + /// + /// The signal is all this does. Whether the process actually went, what + /// it said on the way out, and the `Exited` that follows are reported by + /// the path a session that died on its own already takes: the driver's + /// own reader notices within a poll, drains what was still unread, and + /// records it. Announcing it from here would be this side's guess + /// arriving ahead of the measurement, and it would be wrong for the five + /// seconds a process that ignores SIGTERM keeps running. + /// + /// Deliberately not routed through the driver. The record is the + /// session's rather than any dialect's -- `session::process` writes it + /// for every provider that has a process at all -- so asking it here + /// stops a session whose driver is in no state to be asked, and adds no + /// method a new driver could implement wrongly. + pub fn stop_session(&self, id: &str) -> Result<()> { + if !self + .inner + .read() + .unwrap() + .config + .sessions + .iter() + .any(|meta| meta.id == id) + { + bail!("no session {id}"); + } + // Three answers, and they are three different things to tell + // somebody: it is running (stop it), it is not (nothing to do), and + // nobody could find out (nothing was signalled, and saying "nothing + // is running" would be inventing the answer). + let record = match process::recorded(&self.data_dir.join(id)) { + Some((record, process::Liveness::Alive)) => record, + Some((_, process::Liveness::Unknown)) => bail!( + "this machine won't say whether this session's process is still running, so it \ + wasn't signalled" + ), + Some((_, process::Liveness::Dead)) | None => { + bail!("this session has no process running") + } + }; + tracing::info!("stopping session {id} (pid {})", record.pid); + process::stop(&record, process::STOP_GRACE); + 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. + /// + /// 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 + /// reconnect for anybody watching -- and there is still exactly one + /// 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. + /// + /// 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 + .config + .sessions + .iter() + .find(|meta| meta.id == id) + .with_context(|| format!("no session {id}"))? + .clone(); + let existing = inner.live.get(id).cloned(); + let status = match &existing { + Some(session) => *session.shared.status.lock().unwrap(), + None => status_of_unlaunched(&self.data_dir.join(id)), + }; + 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"), + } + // Fresh from the config, like every other launch: a model or a + // permission mode changed while the session was stopped is what it + // starts with. + let (setup, provider) = resolve(&inner.config, &meta)?; + match existing { + Some(session) => { + *session.driver.lock().unwrap() = make_driver( + &meta, + &setup, + &provider, + &self.models_dir, + session.dir(), + session.transcript_path(), + &session.sink, + )?; + } + // Nothing is live for this one -- a session whose launch failed + // when the server started, which has no pump either. That is the + // whole of `launch`, and the same call the server start makes. + None => { + let session = launch( + meta, + &setup, + &provider, + &self.data_dir, + &self.models_dir, + None, + self.notifications.clone(), + )?; + inner.live.insert(id.to_string(), session); + } + } + // 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(()) + } + /// Kills the process, releases everything the spawn created, and /// deletes the transcript and files -- the complete path out. pub fn delete_session(&self, id: &str) -> Result<()> { @@ -1045,7 +1200,7 @@ impl SessionManager { // Stopped, not detached: this is the one exit where the // process must not survive, because the conversation it // belongs to is being removed. See `Driver::stop`. - session.driver.stop(); + session.driver().stop(); } let dir = self.data_dir.join(id); if dir.exists() { @@ -1339,25 +1494,15 @@ fn launch( ); } - let driver: Arc = match provider.kind { - DriverKind::Echo => Arc::new(EchoDriver::new(sink.clone(), dir.clone())), - DriverKind::LlamaCpp => Arc::new(LlamaDriver::launch( - &meta, - provider, - &Transport::for_setup(setup), - models_dir, - &transcript_path, - &dir, - sink.clone(), - )?), - DriverKind::ClaudeCli => Arc::new(ClaudeDriver::launch( - &meta, - provider, - &Transport::for_setup(setup), - &dir, - sink.clone(), - )?), - }; + let driver = Arc::new(Mutex::new(make_driver( + &meta, + setup, + provider, + models_dir, + &dir, + &transcript_path, + &sink, + )?)); let commands = Arc::new(Commands { driver: Arc::clone(&driver), @@ -1386,6 +1531,44 @@ fn launch( })) } +/// Whatever runs this session's provider, pointed at the session's own +/// directory and reporting into `sink`. +/// +/// Split out of [`launch`] because a session outlives its process: it is +/// also what [`SessionManager::start_session`] builds when somebody starts a +/// stopped session again. That path replaces the driver and nothing else, so +/// it has to construct one the same way rather than becoming a second answer +/// to "what runs this". +fn make_driver( + meta: &SessionConfig, + setup: &SetupConfig, + provider: &ProviderConfig, + models_dir: &Path, + dir: &Path, + transcript_path: &Path, + sink: &EventSink, +) -> Result> { + Ok(match provider.kind { + DriverKind::Echo => Arc::new(EchoDriver::new(sink.clone(), dir.to_path_buf())), + DriverKind::LlamaCpp => Arc::new(LlamaDriver::launch( + meta, + provider, + &Transport::for_setup(setup), + models_dir, + transcript_path, + dir, + sink.clone(), + )?), + DriverKind::ClaudeCli => Arc::new(ClaudeDriver::launch( + meta, + provider, + &Transport::for_setup(setup), + dir, + sink.clone(), + )?), + }) +} + /// The one writer of a session's transcript: assigns sequence numbers, /// appends, updates the shared status/activity view, fans out. Ends when /// every sender is dropped -- i.e. when the session is deleted and its @@ -1639,7 +1822,10 @@ mod tests { let dir = tempfile::tempdir().expect("tempdir"); let (sink, mut events) = mpsc::unbounded_channel(); let commands = Commands { - driver: Arc::new(EchoDriver::new(sink.clone(), dir.path().to_path_buf())), + driver: Arc::new(Mutex::new(Arc::new(EchoDriver::new( + sink.clone(), + dir.path().to_path_buf(), + )))), sink, waiting: Mutex::new(VecDeque::new()), }; @@ -2233,6 +2419,80 @@ mod tests { std::fs::write(path, rewritten).expect("write transcript"); } + /// Stopping and starting a session is about its *process*, and the two + /// refusals are the whole of what keeps starting one from becoming a + /// second one on the same conversation. + /// + /// Echo has no process, which makes it the right session to ask the + /// first question of: "there is nothing to stop" is an answer, and + /// reporting success would leave a phone showing a session it believes + /// it stopped. The second question is asked of a session that has been + /// told it exited, since the guard is on the *status* rather than on + /// which driver it is. + #[tokio::test] + async fn a_session_is_started_again_only_once_it_is_known_to_have_exited() { + 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 refused = manager.stop_session(&info.id).expect_err("nothing to stop"); + assert!( + refused.to_string().contains("no process running"), + "said: {refused:#}" + ); + let refused = manager + .start_session(&info.id) + .expect_err("already running"); + assert!( + refused.to_string().contains("already running"), + "said: {refused:#}" + ); + + // What a driver reports when its process goes, without a process + // to go: the guard reads the recorded status, so this is the same + // state a stopped claude session reaches. + let _ = session.sink.send(Event::Status { + state: SessionStatus::Exited, + }); + collect_until(&mut rx, |event| { + matches!( + event, + Event::Status { + state: SessionStatus::Exited + } + ) + }) + .await; + + manager.start_session(&info.id).expect("start again"); + collect_until(&mut rx, is_idle).await; + // 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( + &session, + &manager.session(&info.id).expect("still live") + )); + } + #[tokio::test] async fn a_restart_relaunches_sessions_and_continues_the_numbering() { let dir = tempfile::tempdir().expect("tempdir"); diff --git a/server/src/session/process.rs b/server/src/session/process.rs index d2f2a26..f2cc3cb 100644 --- a/server/src/session/process.rs +++ b/server/src/session/process.rs @@ -204,6 +204,13 @@ pub fn clear(session_dir: &Path) { } } +/// Grace period between asking a session's process to stop and killing it. +/// +/// Here rather than beside each caller: it is a property of stopping one of +/// these, and two drivers plus the manager had written the same five seconds +/// down separately, which is three places for it to drift. +pub const STOP_GRACE: std::time::Duration = std::time::Duration::from_secs(5); + /// Asks it to stop, then makes sure. Used where a leaked process must /// actually end: a deleted session, or one being replaced. ///