From b282fa2f52ffcf66854f69ee2e6edc8789c3fac3 Mon Sep 17 00:00:00 2001 From: iris <2+iris@noreply.localhost> Date: Mon, 31 Aug 2026 20:11:53 -0400 Subject: [PATCH] Stop a delete re-reading every transcript, and let a busy list scroll Three faults, all of them mine from the batch work or older than it. Deleting one Claude Code session called `list` first -- a full read of every transcript on the machine, about 3.7 seconds against the gigabyte in this VM -- purely to turn an id into a path. A batch of ten spent most of a minute re-reading the same files. `context_of` already resolved an id with a targeted glob; `delete` now does the same and takes 78ms, measured against the same corpus. That glob is why ids are now checked. Both places interpolate the id into `$HOME/.claude/projects/*/"$1".jsonl`, which is an argument rather than script text, so no shell can be talked into running anything -- but a `/` or a `..` still walks the glob out of the directory, and `delete` removes what it lands on. Hex and dashes only, refused rather than escaped, in both places rather than the dangerous one alone. Listing was the only expensive call in the app still on the 5 second default read timeout, against work that takes about four seconds before the tunnel adds anything -- so it timed out against a server that was answering perfectly well. A timeout is for a server that has stopped, so it is now set clear of the work rather than just above it. And `BusyItem` made a row inert by consuming pointer events, which took the drag with the tap: a list could not be scrolled while anything in it was busy. Deciding what a gesture is above the components that already decide it is the wrong place to stand, so the card disables its own click instead and the scroll is left alone. --- .../src/main/kotlin/com/example/aiapp/Api.kt | 12 ++- .../main/kotlin/com/example/aiapp/BusyItem.kt | 26 ++---- .../kotlin/com/example/aiapp/ImportScreen.kt | 5 + .../com/example/aiapp/SessionListScreen.kt | 10 +- server/src/session/import.rs | 93 +++++++++++++++++-- 5 files changed, 116 insertions(+), 30 deletions(-) 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 49828e1..a496e19 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/Api.kt @@ -285,8 +285,18 @@ data class Importable( val inUse: String, ) +/** + * What a machine has that could be continued. + * + * The slowest call this app makes, and it was the only expensive one left on the 5 second default — + * which is how it came to time out against a server that was answering perfectly well. Listing + * means reading every transcript Claude Code has ever written: about four seconds against a + * gigabyte of them before the tunnel adds anything, and that figure grows with every session + * anybody has. A timeout is for a server that has stopped answering, so it is set well clear of how + * long the work takes rather than just above it. + */ fun fetchImportable(settings: ServerSettings, setup: String): List = - requestFromServer(settings, "/setups/$setup/importable") { + requestFromServer(settings, "/setups/$setup/importable", readTimeoutMs = 60000) { it.jsonObjects { session -> Importable( id = session.getString("id"), diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/BusyItem.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/BusyItem.kt index 01f9d36..67786cb 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/BusyItem.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/BusyItem.kt @@ -19,8 +19,6 @@ import androidx.compose.ui.graphics.ColorMatrix import androidx.compose.ui.graphics.Paint import androidx.compose.ui.graphics.drawscope.drawIntoCanvas import androidx.compose.ui.graphics.graphicsLayer -import androidx.compose.ui.input.pointer.PointerEventPass -import androidx.compose.ui.input.pointer.pointerInput import androidx.compose.ui.unit.dp /** @@ -36,29 +34,19 @@ import androidx.compose.ui.unit.dp * a *word* because a spinner alone cannot say which operation this is — deleting and importing are * different in kind, and losing a session to the wrong one is not recoverable by waiting. * - * Inert by consuming pointer events above the content rather than by asking every caller to disable - * its own click handler: the row is covered, so there is nothing left to remember. + * It does **not** make the row inert; the caller disables its own click handling while it passes a + * label. That was the other way round at first — an overlay consuming pointer events, so no caller + * had to remember — and it swallowed the drag along with the tap, which meant a list could not be + * scrolled while anything in it was busy. Consuming taps but not drags means re-deciding what a + * gesture is above the components that already decide it; disabling the click is the platform's own + * answer and leaves the scroll where it belongs. */ @Composable fun BusyItem(label: String?, content: @Composable () -> Unit) { Box { Box(Modifier.busy(label != null)) { content() } if (label != null) { - Box( - Modifier.matchParentSize().pointerInput(Unit) { - // Consumed on the initial pass, so nothing underneath sees the gesture at - // all -- a press that produced a ripple on a row that cannot be pressed - // would say the opposite of everything else here. - awaitPointerEventScope { - while (true) { - awaitPointerEvent(PointerEventPass.Initial).changes.forEach { - it.consume() - } - } - } - }, - contentAlignment = Alignment.Center, - ) { + Box(Modifier.matchParentSize(), contentAlignment = Alignment.Center) { Row(verticalAlignment = Alignment.CenterVertically) { CircularProgressIndicator( modifier = Modifier.width(16.dp).height(16.dp), diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/ImportScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/ImportScreen.kt index eb789ee..a2101fd 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/ImportScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/ImportScreen.kt @@ -455,6 +455,11 @@ private fun ImportableList( Modifier.fillMaxWidth() .padding(vertical = 4.dp) .combinedClickable( + // Off while something is happening to this row -- + // see [BusyItem], which draws that but deliberately + // leaves the gestures alone so the list still + // scrolls. + enabled = running[session.id] == null, onClick = { if (settling(session.id)) return@combinedClickable // In selection mode a tap is a selection, so the diff --git a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt index 775e1ad..d0476c8 100644 --- a/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt +++ b/app/androidApp/src/main/kotlin/com/example/aiapp/SessionListScreen.kt @@ -279,7 +279,15 @@ private fun SessionCard( ) { BusyItem(label = if (deleting) "deleting" else null) { Card( - Modifier.fillMaxWidth().combinedClickable(onClick = onOpen, onLongClick = onLongPress) + // Off while the delete is in flight: a card that still opens a session it is + // deleting is a race the reader can start by tapping. On the card rather than in + // [BusyItem], which leaves gestures alone so the list still scrolls. + Modifier.fillMaxWidth() + .combinedClickable( + enabled = !deleting, + onClick = onOpen, + onLongClick = onLongPress, + ) ) { Column(Modifier.padding(16.dp)) { Row( diff --git a/server/src/session/import.rs b/server/src/session/import.rs index fb69684..7513db8 100644 --- a/server/src/session/import.rs +++ b/server/src/session/import.rs @@ -17,7 +17,7 @@ //! reason: an enrolled token must not be able to turn into "read me this //! arbitrary path". -use anyhow::{Context, Result}; +use anyhow::{Context, Result, bail, ensure}; use serde::Serialize; use serde_json::Value; @@ -618,19 +618,63 @@ fn push_assistant(events: &mut Vec, content: &Value) { /// chance of resuming that conversation, including from an ai-app session /// that was already importing it. pub async fn delete(transport: &Transport, id: &str) -> Result<()> { - let found = list(transport).await?; - let chosen = found - .into_iter() - .find(|candidate| candidate.id == id) - .with_context(|| format!("no Claude Code session {id} on that machine"))?; - let launch = Launch::new("rm", vec!["-f".to_string(), chosen.path.clone()], None); - transport + // The file name *is* the id, so the machine can find it by name. This + // used to call `list` and search its output, which is correct and costs + // a full read of every transcript on the machine -- around four seconds + // against a gigabyte of them, per delete, so a batch of ten took the + // best part of a minute doing nothing but re-reading the same files. + // `context_of` below already resolved an id the cheap way; this is the + // same lookup, and the two now agree. + ensure!(is_session_id(id), "not a Claude Code session id: {id}"); + let script = r#" +for f in "$HOME"/.claude/projects/*/"$1".jsonl; do + [ -f "$f" ] || continue + rm -f "$f" || exit 1 + printf '%s\n' "$f" + exit 0 +done +"#; + let launch = Launch::new( + "sh", + vec![ + "-c".to_string(), + script.to_string(), + "sh".to_string(), + id.to_string(), + ], + None, + ); + // Nothing on stdout means the loop found no such file. Said here rather + // than by exiting non-zero, because a non-zero exit is reported as the + // machine being unreachable -- which is a different thing from the + // session not being there, and only one of them is worth retrying. + let removed = transport .capture(&launch) .await - .with_context(|| format!("deleting {}", chosen.path))?; + .with_context(|| format!("deleting Claude Code session {id}"))?; + if removed.trim().is_empty() { + bail!("no Claude Code session {id} on that machine"); + } Ok(()) } +/// Whether an id is one of ours to put in a shell glob. +/// +/// Both places that resolve an id to a file interpolate it into +/// `$HOME/.claude/projects/*/"$1".jsonl`. That is an argument rather than +/// script text, so a shell cannot be talked into running something -- but a +/// `/` or a `..` inside it still walks the glob out of the directory the id +/// is supposed to name. [`delete`] is where that would be fatal, because it +/// removes whatever it lands on, and it is exactly the reason `delete` used +/// to resolve ids by searching a listing instead. +/// +/// Claude Code names each transcript with a uuid, so hex and dashes is the +/// whole alphabet. Refused rather than escaped: an id that is not one of +/// these did not come from the list this app showed. +fn is_session_id(id: &str) -> bool { + !id.is_empty() && id.len() <= 64 && id.bytes().all(|b| b.is_ascii_hexdigit() || b == b'-') +} + /// How often an imported session checks whether its source file grew. /// /// A poll rather than a watch, because the file may be on another machine @@ -709,6 +753,12 @@ pub async fn line_count(transport: &Transport, path: &str) -> Result { /// Not knowing is a state the status row draws, so there is nothing to be /// gained by inventing a number here. pub async fn context_of(transport: &Transport, session_id: &str) -> Option { + // The same guard `delete` explains, applied to the other member of the + // set: this one only reads, but a glob that can leave the directory is + // worth closing in both places rather than in the dangerous one only. + if !is_session_id(session_id) { + return None; + } // The id crosses as an argument rather than as script text: it comes // from the CLI, but it reaches a shell on a machine that may not be // this one, and the rule there is that data never becomes syntax. @@ -756,6 +806,31 @@ pub async fn replay_after( mod tests { use super::*; + /// The guard on the only thing this module ever puts in a glob. + /// + /// Worth a test of its own because what it protects is a `rm`: `delete` + /// resolves an id straight to `$HOME/.claude/projects/*/"$1".jsonl`, so + /// an id that can contain a slash or a `..` is an id that can name a + /// file outside the directory and have it removed. + #[test] + fn a_session_id_cannot_walk_out_of_the_projects_directory() { + assert!(is_session_id("5ecf21da-d53f-4a11-9c0d-000000000100")); + assert!(is_session_id("deadbeef")); + + assert!(!is_session_id("../../../etc/passwd")); + assert!(!is_session_id("a/b")); + assert!(!is_session_id("..")); + assert!(!is_session_id("a.b")); + assert!(!is_session_id("a*")); + assert!(!is_session_id("a b")); + // Empty would glob to the directory itself, and a long one is not a + // uuid whatever else it is. + assert!(!is_session_id("")); + assert!(!is_session_id(&"a".repeat(65))); + } + + use super::*; + /// A 1x1 PNG, base64 -- the smallest thing with a real header. const PNG: &str = "iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR42mNk\ YPhfDwAChwGA60e6kgAAAABJRU5ErkJggg==";