steid
Sign in

@jamesgill /

steid

feat: every read-side git process is bounded, and a timeout is not a fault

Blame and grep are about to land, and either can run for a long time on a
large repository. Nothing bounded a read before, so a slow one held a worker
for as long as the client waited. Twenty seconds is far above any read a page
should make and far below hung; the child is kill_on_drop so a cancelled
request takes git with it. GitQueryError gains a kind so a page can render
'took too long' instead of a 500. Done once here rather than five times in
five parallel branches.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WGBYVHV86DDvJCBKsDFHT6
JamesPatrickGill authored 16 hours agoparent7aae36cBrowse filesc974d8045e54df5cc303a66af5c56008af0746b3

4 files changed+112 −11

Cargo.toml+1 −1View file
@@ -17,7 +17,7 @@ serde = { version = "1.0.229", features = ["derive"] }
1717 sha2 = "0.10"
1818 sqlx = { version = "0.9.0", default-features = false, features = ["runtime-tokio", "sqlite", "macros", "migrate"] }
1919 subtle = "2.6.1"
20−tokio = { version = "1.53.1", features = ["rt-multi-thread", "macros", "process", "fs", "io-util"] }
20+tokio = { version = "1.53.1", features = ["rt-multi-thread", "macros", "process", "fs", "io-util", "time"] }
2121 tokio-util = { version = "0.7", features = ["io"] }
2222 topcoat = { version = "0.5.0", features = ["icon-iconify", "tailwind", "ui"] }
2323 uuid = { version = "1.24.0", features = ["v4"] }
plans/progress.md+10 −0View file
@@ -4,6 +4,16 @@
44
55 467 tests. Active milestone in [current.md](current.md).
66
7+### Section 1 wave — read-only browsing · in progress
8+
9+**Every read-side `git` process is now bounded at 20 s** (`GIT_TIMEOUT` in
10+`git_query.rs`), and the child is `kill_on_drop` so a cancelled request takes git with
11+it. Added before blame and grep arrived, because those are the first reads that can
12+run long on a big repository, and five agents each adding their own bound would have
13+produced five. `GitQueryError::is_timeout()` is how a page tells "asked too much"
14+(render a state, offer less) from "git failed" (a 500). Tokio's `time` feature was
15+enabled for it.
16+
717 ### Milestone 0 — Skeleton · done
818
919 Topcoat 0.5 app serving pages, `AppConfig` from `STEID_*` env, SQLite pool in app
src/application/port.rs+44 −4View file
@@ -392,22 +392,62 @@ pub trait GitQuery: Send + Sync {
392392
393393 /// A repository could not be read.
394394 #[derive(Debug)]
395−pub struct GitQueryError(Box<dyn std::error::Error + Send + Sync>);
395+pub struct GitQueryError {
396+ kind: GitQueryErrorKind,
397+ source: Box<dyn std::error::Error + Send + Sync>,
398+}
399+
400+/// Why a read failed — the one distinction a page needs to make.
401+///
402+/// A timeout is the caller asking too much of one request (blame on a huge file, a grep
403+/// across a large tree), and the page should say so and offer less. Anything else is a
404+/// fault in the repository or in git, and is a 500.
405+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
406+pub enum GitQueryErrorKind {
407+ Failed,
408+ TimedOut,
409+}
396410
397411 impl GitQueryError {
398412 pub fn new(error: impl Into<Box<dyn std::error::Error + Send + Sync>>) -> Self {
399− Self(error.into())
413+ Self {
414+ kind: GitQueryErrorKind::Failed,
415+ source: error.into(),
416+ }
417+ }
418+
419+ /// The git process was killed because it exceeded `limit`.
420+ pub fn timed_out(limit: std::time::Duration) -> Self {
421+ Self {
422+ kind: GitQueryErrorKind::TimedOut,
423+ source: format!("git did not finish within {limit:?}").into(),
424+ }
425+ }
426+
427+ pub fn kind(&self) -> GitQueryErrorKind {
428+ self.kind
429+ }
430+
431+ pub fn is_timeout(&self) -> bool {
432+ self.kind == GitQueryErrorKind::TimedOut
400433 }
401434 }
402435
403436 impl std::fmt::Display for GitQueryError {
404437 fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
405− write!(f, "could not read the repository: {}", self.0)
438+ match self.kind {
439+ GitQueryErrorKind::Failed => {
440+ write!(f, "could not read the repository: {}", self.source)
441+ }
442+ GitQueryErrorKind::TimedOut => {
443+ write!(f, "reading the repository took too long: {}", self.source)
444+ }
445+ }
406446 }
407447 }
408448
409449 impl std::error::Error for GitQueryError {
410450 fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
411− Some(&*self.0)
451+ Some(&*self.source)
412452 }
413453 }
src/infrastructure/git_query.rs+57 −6View file
@@ -677,18 +677,45 @@ fn parse_latest_tag(stdout: &[u8]) -> Option<TagSummary> {
677677 /// Only ever used for commands whose subject has already been confirmed to exist, so a
678678 /// failure really is a failure. Built from [`git_command`] so the host isolation 0006
679679 /// insists on cannot drift out of this module.
680+/// How long one read-side `git` process may run before it is killed.
681+///
682+/// Every read here is a subprocess on a request path, and nothing bounded it before
683+/// blame and grep arrived — either can run for a long time on a large repository, and
684+/// a request that never finishes holds a worker for as long as the client waits.
685+/// Twenty seconds is far above any read a page should make and far below "hung".
686+const GIT_TIMEOUT: Duration = Duration::from_secs(20);
687+
680688 async fn run<I, S>(repo: &Path, args: I) -> Result<Output, GitQueryError>
681689 where
682690 I: IntoIterator<Item = S>,
683691 S: AsRef<OsStr>,
684692 {
685− let mut command = git_command();
686− command.arg("-C").arg(repo).args(args).stdin(Stdio::null());
693+ run_within(repo, args, GIT_TIMEOUT).await
694+}
687695
688− let output = command
689− .output()
690− .await
691− .map_err(|error| GitQueryError::new(format!("could not run git: {error}")))?;
696+/// [`run`] with an explicit limit, so the timeout path can be tested without waiting
697+/// twenty seconds for it.
698+async fn run_within<I, S>(repo: &Path, args: I, limit: Duration) -> Result<Output, GitQueryError>
699+where
700+ I: IntoIterator<Item = S>,
701+ S: AsRef<OsStr>,
702+{
703+ let mut command = git_command();
704+ command
705+ .arg("-C")
706+ .arg(repo)
707+ .args(args)
708+ .stdin(Stdio::null())
709+ // Dropping the future on timeout must take the process with it, or a killed
710+ // request leaves git running to completion for nobody.
711+ .kill_on_drop(true);
712+
713+ let output = match tokio::time::timeout(limit, command.output()).await {
714+ Ok(result) => {
715+ result.map_err(|error| GitQueryError::new(format!("could not run git: {error}")))?
716+ }
717+ Err(_elapsed) => return Err(GitQueryError::timed_out(limit)),
718+ };
692719
693720 if !output.status.success() {
694721 return Err(GitQueryError::new(format!(
@@ -1777,4 +1804,28 @@ mod tests {
17771804 assert!(unix_time(-1) < UNIX_EPOCH);
17781805 assert_eq!(unix_time(0), UNIX_EPOCH);
17791806 }
1807+
1808+ #[tokio::test]
1809+ async fn a_read_that_exceeds_its_limit_is_a_timeout_not_a_fault() {
1810+ let (_dir, repo) = fixture_repo_for_timeout().await;
1811+ let error = run_within(&repo, ["rev-parse", "HEAD"], Duration::ZERO)
1812+ .await
1813+ .expect_err("a zero limit cannot be met");
1814+ assert!(error.is_timeout(), "{error}");
1815+ }
1816+
1817+ /// A bare repository with nothing in it: `rev-parse` failing is not the point, the
1818+ /// process being cut off before it can answer is.
1819+ async fn fixture_repo_for_timeout() -> (TempDir, std::path::PathBuf) {
1820+ let dir = TempDir::new().unwrap();
1821+ let repo = dir.path().join("t.git");
1822+ let status = git_command()
1823+ .args(["init", "--bare", "-q"])
1824+ .arg(&repo)
1825+ .status()
1826+ .await
1827+ .unwrap();
1828+ assert!(status.success());
1829+ (dir, repo)
1830+ }
17801831 }