From 6154cb1949ccdd03ae9fb590d45ac67e3a5f408d Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 30 Aug 2026 12:41:36 -0400 Subject: [PATCH 1/3] Stop and start a session's process from the composer The composer's second button now says what pressing it would do to the process behind the session, in one place that is always there: an orange pause while a turn is running (interrupt, the process stays), a red stop when it is not (end the process), a green play when it has exited (start it again on the same conversation). Send is disabled while there is nothing to send, rather than pressable and silent. Behind it, two routes. `stop` signals the recorded process and says nothing else -- the driver's own reader already reports a death correctly, and announcing it here would be a guess ahead of the measurement. `start` replaces the driver and nothing else, so the transcript, the pump and every open phone's stream stay where they were and there is still one writer of the transcript; it is refused unless the session is known to have exited, since starting on `Unknown` is the two-CLIs-on-one-conversation fault. That last rule found a bug in the launch path: a relaunched session took its status from the transcript, so one whose process had died before a backend restart reported `exited` while the launch had just started a new process -- which refuses every command and offers a phone the chance to start a second CLI on a live conversation. A launch that leaves a process running now says idle. The icon font moves to the Mono face, where every glyph is one em square, so two icon buttons are the same width without either being told one; the proportional advances ran 0.46 to 0.92 em and Send came out visibly wider than Stop. GLYPH_SIZE comes down to match, since a glyph that fills its em draws bigger at the same point size. Verified against a stand-in CLI on the emulator: idle -> stop -> exited -> start -> idle, a turn interrupted from the pause button, and both buttons measured at 171x105 device pixels. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 18 +- PLAN.md | 49 ++- .../src/main/kotlin/com/example/aiapp/Api.kt | 16 + .../kotlin/com/example/aiapp/NerdIcons.kt | 38 +- .../kotlin/com/example/aiapp/SessionScreen.kt | 79 +++- .../main/kotlin/com/example/aiapp/Theme.kt | 35 +- .../src/main/res/font/nerd_icons.ttf | Bin 1888 -> 2060 bytes app/build-icon-font.sh | 19 +- server/src/routes.rs | 31 +- server/src/session/claude.rs | 8 +- server/src/session/llama.rs | 5 +- server/src/session/mod.rs | 336 ++++++++++++++++-- server/src/session/process.rs | 7 + 13 files changed, 562 insertions(+), 79 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a89b96e..87dc4af 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 @@ -280,15 +284,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 b4fbf1e..b57abc2 100644 --- a/PLAN.md +++ b/PLAN.md @@ -298,6 +298,51 @@ 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. 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. + +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 +493,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 5a1c4c0..07edf36 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionScreen.kt @@ -1358,30 +1358,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( @@ -1402,6 +1416,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 ef7791ae5a2da24c589a137354278427dce9db1e..2001639f09f7749096298f3bd1cc0d109511be03 100644 GIT binary patch literal 2060 zcmb7FU2Icj7=FK=pSByc>-smi80}b3Q71oL*AgIM8$X$Dz!VsQKiG9WY~5dB(?D31 z;D$OhPDn7~-~x%^#$*_ru#mvT;EFILM2JprgvDfIF%y%4>#5K8b%vREw*_gtkB)fP5qVcBhZlD-I&H3$nZ? z){)9&U`ZAkvSm-p0e!N5a^?E^ch?+;17B z3CJ$U1ugCIm~2vZ0zb^<*4X|OeV_Y2qgkT4PrO&Dwo-R%@DMyLVabFQ4@3iEqxcGv@)`-)zLC!`XbM@l@}S zLgcVA$9!~Tnv{!As*Gu=O=?Tm4G*J=0@U*x>X}Ec5CxDQvh#U8d1jSG!a<&CP(sdj zlq7{UjD+fzRC7>6agyrwEMlt5%M0~7QD`*a@jAu$M9Q3zsN#XWGlH6BI?;s1N}7_% zCX=ox_iU1sGa|e4U74NRw8DHN?kv={?aV+SkK{;_?cP(2!%dkTby}g5oysrN>Z>W^ zcFdyE$!4~sIhkxRer!oHd2P+wbGCfDo5gJwvo*27$L2c=3Y^9tz6}Yh*-7ems?6WTm^OkxYN;K$@+GdWKuCE#V?$@Ey{X;|j z{g;oVQk9jJ>)QABytlWlvb?;aJyp!Ks!=tP-Z6YFy@M_Nx%Z98&6_u40`%>LKsZ@FSaii;6*D&j^B1)C5U;Le<^7};%K^C#t^TC6+qTkmi__VtB#Gd)Joa?m^vqD6QX2U^eXvHFOA zx$rF#JAA3Sp>t@aCv(JdP(%<{vl}DXLadKZs#2B=nfIjX9$PIP4n#qrPJqYy2}q(@-yqnCk+!M_ic$LR%}I)zn&wt}}(J^Zv# z2ktjh0^S;M+6W!TaT|VWhnEDRyai+onEzwxqf*Rq8E6I7i3lCYn0xghOP=Ra!I#tO i|Hy#n_0;~~aW^B+O`@aNh%$^YyJA*^JZIxGV)`48v#x*u literal 1888 zcmah~Z){Ul6hEhZf7p1=D#{+N(>lXK6% z-#z!-bMAeQn20nwL3V0;VaJBp&Yf?+L?jhq-IpCnkJ9gd+*Rdy1#bs*FON-3tM6c2G0kMkD$*$uN@j0 z&&MihEA&aB4;ZGTCtptnKyjimFV^o{FR=sI+Le|lsf@K!@3_Fp98=SL5WjhhnX zuz$1gla0u%S2OYjs-(3G@J<)jVqx{XPD22`a?xN8R~DB>+eTbhJmmF2O^=W=5;wGyU2k|tAeaLIlyx20dX+P;PaAc4+mul1 z#0sjArlol*!-}ZWTH)j*OVfFy)~L<%ZEND&P^B%HEa|bs+H+p+lPhQqB1EE*a7eOw zDx}63CBurUghNpnJ;_8e(HIN(103{9XVPyTot!gn%+2wpxkVn{G?Sb-Id%Hb!FG&; zhfcq?d(Spw(bgwDx2GuqoN)$N))&sE_X$7y1+s3fg~6scbxtKYM>0{BlfC7v(%(?- z(>Xit$x@+EAhMZ;S)m-0=TVz(EJ7}0qxSXe=Ew$);{?Hws#K|p8jUEb zUGdkj;tQxNI8eEopJAxJfaq{C(S%b4{E1{`A`-Q4J~}gVZ)V0uk~CXxcNC-=Z!wk9 zwKA_U?a?$nl`48_wD@a zwQ3L7X_}+Axq89l!tg9qH}^U;&A92Q)+U!B#?2=nW$pz%E+AY7h}GqsL{d(?nl(Xh zP@)HfD=eY=u$UfH?<#I@gw_$Q;&S$~A4iPURy3g;C`gs)oZp9%C{{b;n>pZ>lTk0K zOSa*rk1u>V#h$629q*pHIG#zjwzRZl(!Zs%$>z};I#b&<`Ksu`Qh^=U^G5*v^x~<> z?cIguWEQXt;H~M*-5aAPj6z}f1pBOvovtf%5*ea*Ahv96wBE9ase@jj<8+O_;}>}^ zU$XqT3x_%ymiD?g{Yhnb1Q^Bax5otkx}yUD#RgV z=OZ-1fDV*KcdnzKul>QMS=vL%@i}~c63jmYQbHWE|Ct7Rtvj` zSejQnjHba$G-}PF2EddEcsF%fBvN(rE{nwda7*15iMVd*J&SNi@Us@7G~hp4B+BLX zW-Su&t?xE$yG5pwIR6$gY*XcX?Xr0uTvxPZvr*YE-Ts?vJa 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/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..af43fab 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 @@ -673,7 +667,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..07f1ae4 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,141 @@ 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. + 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)?; + let session = match existing { + Some(session) => { + *session.driver.lock().unwrap() = make_driver( + &meta, + &setup, + &provider, + &self.models_dir, + session.dir(), + 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 + // 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(), Arc::clone(&session)); + 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, + }); + 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 +1197,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 +1491,38 @@ 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, + )?)); + + // 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), @@ -1386,6 +1551,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 +1842,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 +2439,70 @@ 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 said by the start rather than left + // for a driver that has been given no work to say for itself. + assert_eq!(manager.sessions()[0].status, 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. /// From 1d843f20f28abf262ef8d07c2885adee3ef61136 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 30 Aug 2026 12:57:21 -0400 Subject: [PATCH 2/3] 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 --- PLAN.md | 22 +++++++++--- server/src/session/claude.rs | 32 +++++++++++++++++ server/src/session/mod.rs | 68 +++++++++++++++--------------------- 3 files changed, 79 insertions(+), 43 deletions(-) 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( From 558095520d470cb0c77d3584eb46f2b5d4678302 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Sun, 30 Aug 2026 13:09:36 -0400 Subject: [PATCH 3/3] Take the emulator half out of run-android.sh Which AVD this checkout means, creating it, booting it headless and refusing to start one the machine has no room for is the same sequence in ai-app, ai-app-2 and dev-updater. It now lives once, in ~/repos/emulator-tools, and this script is what is actually specific to this project: a build, an install and a launch. Three copies of "boot an emulator" was three places for the memory check none of them had -- starting one at 2.8 GB available invoked the OOM killer, and what it took first was another session's emulator. Co-Authored-By: Claude Opus 5 --- AGENTS.md | 23 +++++---- app/run-android.sh | 117 ++++++++++----------------------------------- 2 files changed, 39 insertions(+), 101 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 87dc4af..7c5782e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -225,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) 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"