Skip to content

Add wall-clock timeout and SSH hardening to authorship note fetch#1184

Closed
jwiegley wants to merge 3 commits into
johnw/review-attribution-test-guidelinesfrom
johnw/review-fix-tests
Closed

Add wall-clock timeout and SSH hardening to authorship note fetch#1184
jwiegley wants to merge 3 commits into
johnw/review-attribution-test-guidelinesfrom
johnw/review-fix-tests

Conversation

@jwiegley
Copy link
Copy Markdown
Contributor

Add wall-clock timeout and SSH hardening to authorship note fetch

fetch_authorship_notes previously shelled out to git fetch via
exec_git, which uses Command::output() and blocks the calling
thread indefinitely. In practice this meant a stalled SSH handshake
to the remote -- credential prompt with no TTY, dead TCP connection,
or BatchMode-less keepalive loss -- would freeze the caller forever
with no way to recover. We observed a ~5 hour hang in a test run
where ssh git@github.com never completed.

Introduce exec_git_with_env_and_timeout in repository.rs: it spawns
the child with piped stdout/stderr drained by background threads
(to avoid pipe-buffer deadlock), polls try_wait with a short
interval, and on timeout kills the child and returns a GitCliError
marked with code: None and a descriptive stderr.

Route fetch_authorship_notes through it with a 60s deadline --
authorship notes are tiny, anything longer is a hung remote -- plus
two env vars that prevent hangs at the SSH layer:

  • GIT_TERMINAL_PROMPT=0 disables interactive credential prompts.
  • GIT_SSH_COMMAND forces BatchMode=yes, ConnectTimeout=10, and
    short keepalives so a dead connection fails fast. Only applied
    when the user has not set GIT_SSH_COMMAND themselves.

Either guard alone would have killed the observed hang within
seconds; together they provide defense in depth.

Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com

Disable env-dependent authorship traversal smoke test

test_load_ai_touched_files_for_specific_commits performs a real
git fetch origin refs/notes/ai against the developer's CWD remote
and requires the remote to have at least 3 authorship notes. It is
a smoke test, not a unit test: in CI or on a machine without SSH
credentials to the configured origin it will either fail or (before
the preceding fetch-timeout hardening) hang indefinitely.

Mark it #[ignore] with a comment spelling out the preconditions
and what a proper hermetic rewrite would look like (a TestRepo with
a local file:// remote seeded with synthetic notes). Re-enable once
ported. The other four tests in the module remain environment-
dependent on CWD but do not touch the network, so are left alone
here.

Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com

Apply pending cargo fmt to hooks and test_repo

Pure rustfmt output -- indentation and macro-call wrapping only.
No behavior change. These were produced by task fmt during an
unrelated session and are bundled here separately to keep them
out of the hang-fix commits.

Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com

Copy link
Copy Markdown
Contributor Author

jwiegley commented Apr 23, 2026

@jwiegley jwiegley marked this pull request as ready for review April 23, 2026 05:40
@jwiegley jwiegley requested a review from svarlamov April 23, 2026 05:40
Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

@svarlamov
Copy link
Copy Markdown
Member

This PR has a lot of changes... I think the patch for the one affected test (from what I can tell) is a ~1 liner to just use new_with_remote_with_mode. Is there any reason that wouldn't work?

jwiegley and others added 3 commits April 28, 2026 14:03
`fetch_authorship_notes` previously shelled out to `git fetch` via
`exec_git`, which uses `Command::output()` and blocks the calling
thread indefinitely. In practice this meant a stalled SSH handshake
to the remote -- credential prompt with no TTY, dead TCP connection,
or BatchMode-less keepalive loss -- would freeze the caller forever
with no way to recover. We observed a ~5 hour hang in a test run
where `ssh git@github.com` never completed.

Introduce `exec_git_with_env_and_timeout` in repository.rs: it spawns
the child with piped stdout/stderr drained by background threads
(to avoid pipe-buffer deadlock), polls `try_wait` with a short
interval, and on timeout kills the child and returns a `GitCliError`
marked with `code: None` and a descriptive stderr.

Route `fetch_authorship_notes` through it with a 60s deadline --
authorship notes are tiny, anything longer is a hung remote -- plus
two env vars that prevent hangs at the SSH layer:

- `GIT_TERMINAL_PROMPT=0` disables interactive credential prompts.
- `GIT_SSH_COMMAND` forces `BatchMode=yes`, `ConnectTimeout=10`, and
  short keepalives so a dead connection fails fast. Only applied
  when the user has not set `GIT_SSH_COMMAND` themselves.

Either guard alone would have killed the observed hang within
seconds; together they provide defense in depth.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
`test_load_ai_touched_files_for_specific_commits` performs a real
`git fetch origin refs/notes/ai` against the developer's CWD remote
and requires the remote to have at least 3 authorship notes. It is
a smoke test, not a unit test: in CI or on a machine without SSH
credentials to the configured origin it will either fail or (before
the preceding fetch-timeout hardening) hang indefinitely.

Mark it `#[ignore]` with a comment spelling out the preconditions
and what a proper hermetic rewrite would look like (a TestRepo with
a local file:// remote seeded with synthetic notes). Re-enable once
ported. The other four tests in the module remain environment-
dependent on CWD but do not touch the network, so are left alone
here.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Pure rustfmt output -- indentation and macro-call wrapping only.
No behavior change. These were produced by `task fmt` during an
unrelated session and are bundled here separately to keep them
out of the hang-fix commits.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jwiegley jwiegley force-pushed the johnw/review-fix-tests branch from 8b08c91 to fd5f46f Compare April 28, 2026 14:03
@jwiegley jwiegley force-pushed the johnw/review-attribution-test-guidelines branch from fd7b5db to 9822853 Compare April 28, 2026 14:03
@svarlamov
Copy link
Copy Markdown
Member

Closing re #1184 (comment)

@svarlamov svarlamov closed this Apr 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants