Skip to content

Read/fork handlers run blocking git subprocesses on the tokio runtime (no spawn_blocking, no timeout) #204

Description

@beardthelion

Surfaced during review of #196 (pre-existing; git/store.rs and the fork clone are untouched by that PR's diff, so splitting here rather than widening #196's scope).

fork_repo — crates/gitlawb-node/src/api/repos.rs:1567 runs std::process::Command::new("git").args(["clone","--mirror",…]).output() inside the async handler. output() blocks the worker thread; a git clone --mirror of a non-trivial repo can take minutes and there is no timeout, unlike other git walks in this file that use tokio::task::spawn_blocking (repos.rs:67,110,661,1143) or tokio::process::Command.

Read handlers — list_commits/get_tree/get_blob call store::{resolve_head,log,ls_tree,read_file} (api/repos.rs:360-361,397,443,481), which are synchronous std::process::Command::output() calls (git/store.rs:92-231). A slow disk or large repo blocks a worker for seconds; with a finite worker pool a handful of concurrent /commits, /tree, /blob, or one /fork can starve the runtime, including /health and /ready.

Impact: availability/reliability regression; git_service_timeout_secs is bypassed on these paths.

Fix direction: wrap the blocking store calls (and the fork clone) in spawn_blocking bounded by tokio::time::timeout(git_service_timeout_secs, …), returning AppError::Timeout. Verified real by execution during the #196 review.

Activity

  1. added
    crate:nodegitlawb-node — the serving node and REST API
    kind:securityVulnerability fix or hardening
    sev:highMajor break or real security/trust risk, no easy workaround
    subsystem:apiNode REST API request/response surface
    subsystem:storageBlob/object store, Arweave, IPFS, archives
    on Jul 15, 2026
  2. beardthelion commented on Aug 15, 2026

    @beardthelion
    CollaboratorAuthor

    A third site, on the background path rather than a request handler, so it escapes the
    git_service_timeout_secs framing above in a way worth noting separately.

    encrypt_and_pin is pub async fn (crates/gitlawb-node/src/encrypted_pin.rs:109) and calls the
    synchronous read bare inside its loop (:155):

    let data = match crate::git::store::read_object(repo_path, oid) {

    No spawn_blocking and no block_in_place anywhere in the file. read_object
    (crates/gitlawb-node/src/git/store.rs:681-688) composes object_type and read_object_content,
    both plain std::process::Command::output() with no deadline and no process-group reaping
    (store.rs:277-281, store.rs:309-313). The bounded twin's own docstring states the rule this
    breaks (store.rs:643-645): "This is SYNCHRONOUS blocking work ... Async callers must run it under
    tokio::task::spawn_blocking ... calling it directly from a runtime task blocks a worker."

    What makes it worth adding here rather than leaving to a later sweep is that it is residue from a fix
    that already named it. Commit 03dde22e (#174) touched encrypted_pin.rs by exactly one line,
    bounding pin_git_object only, and its message says: "Not bounded: the git read, since
    store::read_object shells two bare Command::output() calls with no timeout or process-group reaping
    and this loop does not run it under spawn_blocking." Both sibling pin loops got the bounded read
    afterwards (ipfs_pin.rs:299-306, pinata.rs:166-173, each spawn_blocking(|| read_object_bounded(..., read_deadline))).
    This loop did not.

    Reachability differs from the sites above. Post-receive only: run_encrypt_pin_task into
    pin_and_encrypt_objects (repos.rs:1181) into encrypt_and_pin (repos.rs:1224), gated on the repo
    carrying a path-scoped rule with withheld blobs. So it needs push rights plus rule setup, though on an
    open node any registered user can arrange both.

    The impact shape is also different, and is why the request-path framing does not cover it: the push
    response has already returned, so there is no slow request to observe. Per withheld blob, two blocking
    git cat-file children hold a runtime worker for their full duration with no ceiling. EncryptInflight
    caps one task per repo, so concurrency across repos is bounded by nothing, and N concurrent repos hold
    N workers, which starves the shared runtime including /health and /ready exactly as described
    above.

    Fix is the same shape as the one already proposed here, with one wrinkle: encrypt_and_pin currently
    takes neither a git_bin nor a deadline, so both have to be threaded in from EncryptTaskCtx before
    it can call read_object_bounded.

    Not verified by execution; the regression test needs a live Postgres and a full node build. The static
    path is unambiguous, since both constituents are visibly untimed Command::output() and no wrapper
    exists in the file.

  3. added
    sev:mediumDegraded but workaround exists
    and removed
    sev:highMajor break or real security/trust risk, no easy workaround
    on Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    crate:nodegitlawb-node — the serving node and REST APIkind:securityVulnerability fix or hardeningsev:mediumDegraded but workaround existssubsystem:apiNode REST API request/response surfacesubsystem:storageBlob/object store, Arweave, IPFS, archives

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions