anvilsign in

collin/anvil · c2ed76da

Give the agent a workspace it can add files to, and stop leaking tmux clients

Collin Richards · 2026-08-18 16:18 UTC · c2ed76da013cfa3b4aea4ddb6589fe4f6366a92d · parent 9711016c · browse files

modifiedcrates/anvil-agent/src/container.rs+147 −4
⋯ 122 unchanged lines
123123
124124 /// Build a tar of `(path, contents, executable)` entries, rooted at `prefix`
125125 /// (no leading slash) and owned by the session user.
126+///
127+/// Emits an explicit entry for every intermediate **directory**, which matters
128+/// more than it looks: a tar of files alone makes Docker create the parent
129+/// directories itself, owned by root. The session runs as `agent`, so `src/`
130+/// would come out root-owned and the agent could edit existing files but never
131+/// add one — which is most of what a coding agent does.
126132 pub fn build_tar(prefix: &str, files: &[(String, Vec<u8>, bool)]) -> Vec<u8> {
127133 let mut builder = tar::Builder::new(Vec::new());
128- for (path, content, executable) in files {
134+ let mtime = anvil_core::agent::now_secs().max(0) as u64;
135+
136+ let header_for = |size: u64, mode: u32, entry_type: tar::EntryType| {
129137 let mut header = tar::Header::new_gnu();
130- header.set_size(content.len() as u64);
131- header.set_mode(if *executable { 0o755 } else { 0o644 });
138+ header.set_size(size);
139+ header.set_mode(mode);
140+ header.set_entry_type(entry_type);
132141 // Ownership matters: the container runs as `agent`, and a checkout it
133142 // cannot write is not a workspace.
134143 header.set_uid(RUN_AS_UID);
135144 header.set_gid(RUN_AS_UID);
136- header.set_cksum();
145+ // Without this every file lands in 1970, which upsets anything that
146+ // compares timestamps (make, and incremental builds generally).
147+ header.set_mtime(mtime);
148+ header
149+ };
150+
151+ // Every ancestor directory, deduplicated and shortest-first so parents are
152+ // created before their children.
153+ let mut dirs: Vec<String> = Vec::new();
154+ for (path, _, _) in files {
155+ let mut parts: Vec<&str> = path.split('/').collect();
156+ parts.pop(); // the file itself
157+ let mut acc = String::new();
158+ for part in parts {
159+ if !acc.is_empty() {
160+ acc.push('/');
161+ }
162+ acc.push_str(part);
163+ if !dirs.contains(&acc) {
164+ dirs.push(acc.clone());
165+ }
166+ }
167+ }
168+ dirs.sort_by_key(|d| d.matches('/').count());
169+
170+ // The root itself, so `/workspace` is agent-owned even when empty.
171+ let mut header = header_for(0, 0o755, tar::EntryType::Directory);
172+ let _ = builder.append_data(&mut header, format!("{prefix}/"), std::io::empty());
173+ for dir in &dirs {
174+ let mut header = header_for(0, 0o755, tar::EntryType::Directory);
175+ let _ = builder.append_data(&mut header, format!("{prefix}/{dir}/"), std::io::empty());
176+ }
177+
178+ for (path, content, executable) in files {
179+ let mode = if *executable { 0o755 } else { 0o644 };
180+ let mut header = header_for(content.len() as u64, mode, tar::EntryType::Regular);
137181 let full = format!("{prefix}/{path}");
138182 if builder
139183 .append_data(&mut header, &full, content.as_slice())
⋯ 278 unchanged lines
418462 })
419463 .collect())
420464 }
465+
466+#[cfg(test)]
467+mod tests {
468+ use super::*;
469+
470+ fn entries(tar: &[u8]) -> Vec<(String, tar::EntryType, u64, u32)> {
471+ let mut archive = tar::Archive::new(tar);
472+ archive
473+ .entries()
474+ .unwrap()
475+ .map(|e| {
476+ let e = e.unwrap();
477+ let header = e.header();
478+ (
479+ e.path().unwrap().to_string_lossy().into_owned(),
480+ header.entry_type(),
481+ header.uid().unwrap(),
482+ header.mode().unwrap(),
483+ )
484+ })
485+ .collect()
486+ }
487+
488+ /// The bug this guards: a tar of files alone leaves Docker to create the
489+ /// parent directories, owned by root. The session runs unprivileged, so
490+ /// `src/` came out root-owned and the agent could edit `src/main.rs` but
491+ /// never add `src/lib.rs` — most of what a coding agent does.
492+ #[test]
493+ fn intermediate_directories_are_present_and_agent_owned() {
494+ let files = vec![
495+ ("README.md".to_string(), b"hi".to_vec(), false),
496+ ("src/main.rs".to_string(), b"fn main() {}".to_vec(), false),
497+ ("a/b/c/deep.txt".to_string(), b"deep".to_vec(), false),
498+ ];
499+ let entries = entries(&build_tar("workspace", &files));
500+
501+ let dirs: Vec<_> = entries
502+ .iter()
503+ .filter(|(_, t, _, _)| *t == tar::EntryType::Directory)
504+ .map(|(p, _, _, _)| p.trim_end_matches('/').to_string())
505+ .collect();
506+ for expected in [
507+ "workspace",
508+ "workspace/src",
509+ "workspace/a",
510+ "workspace/a/b",
511+ "workspace/a/b/c",
512+ ] {
513+ assert!(
514+ dirs.contains(&expected.to_string()),
515+ "missing dir {expected}: {dirs:?}"
516+ );
517+ }
518+
519+ // Everything, files and directories alike, must belong to the session
520+ // user or the workspace is read-only in practice.
521+ for (path, _, uid, _) in &entries {
522+ assert_eq!(*uid, RUN_AS_UID, "{path} is not owned by the session user");
523+ }
524+ }
525+
526+ /// Parents must precede their children, or the ownership set on a
527+ /// directory entry is applied to one Docker already made as root.
528+ #[test]
529+ fn parents_are_written_before_their_children() {
530+ let files = vec![("a/b/c/deep.txt".to_string(), b"x".to_vec(), false)];
531+ let entries = entries(&build_tar("workspace", &files));
532+ let order: Vec<_> = entries.iter().map(|(p, _, _, _)| p.as_str()).collect();
533+
534+ let index = |needle: &str| order.iter().position(|p| p.trim_end_matches('/') == needle);
535+ let root = index("workspace").expect("root dir");
536+ let a = index("workspace/a").expect("a");
537+ let b = index("workspace/a/b").expect("b");
538+ let c = index("workspace/a/b/c").expect("c");
539+ let file = index("workspace/a/b/c/deep.txt").expect("file");
540+ assert!(
541+ root < a && a < b && b < c && c < file,
542+ "wrong order: {order:?}"
543+ );
544+ }
545+
546+ #[test]
547+ fn executables_keep_their_bit() {
548+ let files = vec![
549+ ("run.sh".to_string(), b"#!/bin/sh\n".to_vec(), true),
550+ ("plain.txt".to_string(), b"x".to_vec(), false),
551+ ];
552+ let entries = entries(&build_tar("workspace", &files));
553+ let mode = |name: &str| {
554+ entries
555+ .iter()
556+ .find(|(p, _, _, _)| p == name)
557+ .map(|(_, _, _, m)| *m)
558+ .unwrap()
559+ };
560+ assert_eq!(mode("workspace/run.sh") & 0o111, 0o111);
561+ assert_eq!(mode("workspace/plain.txt") & 0o111, 0);
562+ }
563+}
modifiedcrates/anvil-web/src/agent.rs+48 −6
⋯ 280 unchanged lines
281281
282282 match anvil_agent::start(&app, repo.id, user.id, session_kind, &base_ref, prompt).await {
283283 Ok(id) => Ok(Redirect::to(&format!("/{owner}/{name}/-/agent/{id}")).into_response()),
284- Err(StartError::Disabled) => Err(forbidden()),
284+ // Being at capacity, or switched off, is a normal answer rather than a
285+ // fault — "something went wrong" would send someone hunting a bug that
286+ // isn't there.
287+ Err(e @ (StartError::Disabled | StartError::AtCapacity(_))) => Err(unavailable(&e)),
285288 Err(e) => Err(server_error(e)),
286289 }
287290 }
288291
292+/// A session could not be started for a reason the caller can act on.
293+fn unavailable(reason: &StartError) -> Response {
294+ (
295+ axum::http::StatusCode::SERVICE_UNAVAILABLE,
296+ layout(
297+ "Cannot start a session",
298+ None,
299+ html! {
300+ h1 { "Cannot start a session" }
301+ p.muted { (reason.to_string()) }
302+ @if matches!(reason, StartError::AtCapacity(_)) {
303+ p { "Stop a running session, or raise " code { "agent.max_concurrent" } "." }
304+ }
305+ },
306+ ),
307+ )
308+ .into_response()
309+}
310+
289311 #[derive(serde::Deserialize)]
290312 struct CreateForm {
291313 #[serde(default)]
⋯ 208 unchanged lines
500522 } = terminal;
501523
502524 // Container -> browser.
503- let to_browser = tokio::spawn(async move {
525+ let mut to_browser = tokio::spawn(async move {
504526 while let Some(chunk) = output.next().await {
505527 let Ok(log) = chunk else { break };
506528 let bytes = log.into_bytes();
⋯ 14 unchanged lines
521543 // Browser -> container, plus the control channel.
522544 let control_docker = docker.clone();
523545 let control_exec = exec_id.clone();
524- let to_container = tokio::spawn(async move {
546+ let mut to_container = tokio::spawn(async move {
525547 while let Some(message) = stream.next().await {
526548 match message {
527549 Ok(Message::Binary(data)) => {
⋯ 12 unchanged lines
540562 _ => {}
541563 }
542564 }
565+ // Detach this tmux client explicitly.
566+ //
567+ // Docker has no "kill exec" call, and dropping bollard's streams does
568+ // not reliably tear the exec down — without this, every page view left
569+ // a tmux client attached forever, counting against the container's pids
570+ // limit and taking part in tmux's window sizing.
571+ //
572+ // Writing the prefix (C-b) followed by `d` into the exec's own stdin
573+ // detaches precisely the client on the other end of it, with no need to
574+ // work out which of several clients is ours. `tmux.conf` keeps the
575+ // default prefix, so this stays in step with it.
576+ let _ = input.write_all(b"\x02d").await;
577+ let _ = input.flush().await;
543578 });
544579
545- // Either direction ending means this viewer is gone.
580+ // Either direction ending means this viewer is gone. Abort BOTH halves
581+ // rather than letting the survivor linger: each holds one end of bollard's
582+ // hijacked connection, and while either is alive the `docker exec` — and so
583+ // the tmux client behind it — stays up. Leaving them accumulated a stale
584+ // client per page view, which counts against the container's pids limit and
585+ // takes part in tmux's window sizing.
546586 tokio::select! {
547- _ = to_browser => {}
548- _ = to_container => {}
587+ _ = &mut to_browser => {}
588+ _ = &mut to_container => {}
549589 }
590+ to_browser.abort();
591+ to_container.abort();
550592 anvil_agent::detached(&app, session_id);
551593 }
552594
⋯ 15 unchanged lines