fix(git): bound the default-branch remote query, and make the bound bound - #3603
Merged
Conversation
…ound `Repository::default_branch()` may fall through to `git ls-remote`, which nothing limited: an unanswered SYN costs ~127 s per address on Linux and git tries each of a host's addresses in turn, so a remote behind a dropped VPN stalled `wt list --full` and `wt switch` for minutes. Adding a 10 s bound alone would have been decorative. `Cmd::timeout` killed only the direct child, and a grandchild inherits its stderr pipe — so a surviving one held the write end open and `read_to_end` blocked until it exited. Measured: a 3 s timeout on an `ls-remote` whose upload-pack sleeps 120 s returned `TimedOut` after 120.03 s. `git-remote-https` sitting in `connect()` is exactly that shape. So a timed child now spawns into its own process group and expiry tears down the group (`taskkill /T /F` on Windows); the same probe returns at 3.20 s. Every existing `.timeout()` caller was latently unbounded the same way. A timed-out detection falls back to local inference *without* persisting it: `worktrunk.default-branch` is what stops later calls from re-detecting, so a guess made during an outage would otherwise become permanent. Only a timeout separates cleanly — `ls-remote` exits 128 whether the network is down or the remote has no HEAD — so other failures stay cached. Isolating the process group costs the tty broadcast: Ctrl-C no longer reaches a timed child, so the user waits out the remaining bound. Seconds, against an orphan holding a pipe for its own full runtime. Also routes the four remaining hand-rolled git test envs through `configure_git_env`, so they carry the `GIT_ALLOW_PROTOCOL` deny from #3596.
worktrunk-bot
approved these changes
Jul 25, 2026
This was referenced Jul 25, 2026
max-sixty
added a commit
that referenced
this pull request
Jul 26, 2026
) Two follow-ups from #3603. ## `[list] task-timeout-ms` didn't bound a command that set its own timeout `Cmd::run` resolved its two timeout sources as a precedence chain — `self.timeout.or_else(thread_local)` — so a command carrying an explicit `.timeout()` escaped a shorter budget its caller had set. `wt list` sets exactly such a budget on every collect worker from `[list] task-timeout-ms`, and the 10s bound #3603 put on remote default-branch detection sits inside one. With `task-timeout-ms = 50`, that command ran for the full 10s. Both values are ceilings — one bounds a command, the other bounds every command on the thread — so the tighter of the two now wins. The key's own documentation already promises that it "kills individual git commands that exceed this duration", so this makes the documented behavior true rather than changing it; no doc update goes with it. I confirmed the defect rather than inferring it: with the old line restored, the new test's command runs the full 10.01s against a 50ms budget, 200× over. The setting is unset by default, so nothing changes for a default config. The one nearby thing this could have clamped is the picker's 2s pager bound, and it can't — `pager.rs` calls `child.wait_timeout` on a raw `Child`, not through `Cmd`. `COMMAND_TIMEOUT`'s doc named the `wt switch` picker as its driver and omitted the config key that actually feeds it, which reads as though the thread-local has no production callers at all. It now names the key and says it's unset by default. ## A Windows CI flake in `test_cmd_run_file_current_dir_is_errored` The test created a `NamedTempFile` purely to get a path that isn't a directory, and on Windows the creation itself failed: ``` PathError { path: "D:\\tmp\\.tmpWKLe5F", err: Os { code: 5, kind: PermissionDenied, message: "Access is denied." } } ``` tempfile retries only a *reported* collision (`AlreadyExists` / `AddrInUse`), so a denial propagates. The error the test actually exercises comes from `check_spawn_preconditions` — `metadata` plus `is_dir`, before anything spawns — so the test needs nothing more than an existing non-directory path. The test binary is one, and using it means the test creates nothing under the shared temp root. ### What I did not do The obvious next step is a prefix-separation sweep: `TempDir` and `NamedTempFile` draw names from one namespace (a `.tmp` prefix plus six random characters), and this suite feeds it ~260 default-prefix `tempdir()` sites plus a temp directory per test repo, so a temp *file* name can land on a temp *directory* name. That would explain a denial rather than a collision report — but only if Windows returns `ACCESS_DENIED` where a file creation hits an existing directory name, instead of `FILE_EXISTS`. I can't test that from macOS, and it's the load-bearing link, so I left the other eleven `NamedTempFile::new()` sites alone rather than sweep production (`src/commands/step/commit.rs`, `src/git/repository/working_tree.rs`) on an unverified premise. Worth doing if someone can confirm the semantic. ## Testing The timeout fix carries a regression test that fails without it (`Ok` after 10s) and passes with it. The Windows fix is verified only by the local suite — the flake it removes is rare and Windows-only, so CI going green here is consistent with it but doesn't demonstrate it. > _This was written by Claude Code on behalf of max_ Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
max-sixty
added a commit
that referenced
this pull request
Jul 27, 2026
…#3608) ## What prompted this Getting #3605 green ran into codecov reporting a `base_commit` three commits older than the real merge-base. This audits whether our config causes that. ## The cause Codecov picks a PR's base by walking back to the newest ancestor that has a coverage report. It used the real merge-base for PRs #3480, #3532 and #3602, and a stale one for #3603 and #3605. The difference is whether the merge-base uploaded a report. **29 of the last 40 main commits did not.** `ci` had one concurrency group for main pushes, and GitHub cancels the *pending* run in a group whenever a newer one joins, even with `cancel-in-progress: false`. So the question is how long a run holds the group, and a run isn't done until its slowest job is: | job | duration on main | |-----|------------------| | `fast-checks` | 2 min | | `code-coverage` | 3-4 min | | `test (windows)` | 11 min | | `collect affected coverage (windows)` | 110-129 min | Each main run held the group for ~2 hours, so nearly every subsequent main push was cancelled while queued, taking the 4-minute coverage job with it. Every cancelled main run's `updated_at` lands within a second of the next push's `created_at`. The 2 hours is real work, not queue: 2-5s from `created_at` to `started_at`, then 108 minutes inside `cargo affected collect` — 4181 tests under `-C instrument-coverage` with a per-test LLVM profile, ~5 GB of profraw. ## The fix: one workflow per cadence The three groups of jobs have incompatible needs, and one group was serving all of them. | workflow | cadence on main | why | |----------|-----------------|-----| | `ci` | every commit, ~11 min | required gate + fast checks | | `coverage` | every commit, keyed per-sha | a skipped upload leaves later PRs on a stale base | | `affected` | sampled, ~2 h | a DB a few commits old still anchors a correct superset | `affected` keeps exactly the grouping it has today, so its sampling is unchanged and deliberate. It just no longer drags the other two along. ### Scope of the impact The posted `codecov/patch` check scopes to the PR's own GitHub diff, so a stale base did **not** score PRs against other people's lines. On #3605 the posted 91.66% is exactly `github.rs`'s 11/12, while the stale-base compare object reported 64/65 across 13 files. What a stale base costs: - `codecov/project` reports "compared to \<stale sha\>" - the patch `auto` target is the stale base's project coverage (0.02pp here) - the compare API object widens to `base..head`, which is what made the investigation look like silence Separately, `test`/`lint`/`fast-checks` also stopped completing on main. Nothing load-bearing rode on that (they already ran on the PR), but it left `tend-ci-fix` with nothing to watch, since it doesn't fire on cancelled runs. ## Two smaller fixes - `ignore: "**/tests/**"` compiles to `.*/tests/.*` (confirmed against codecov's validator), which needs a leading directory and so never matched `tests/` itself. Inert today since `cargo llvm-cov` reports only `src/` (verified against a downloaded `cobertura.xml`), but now correct if that changes. Now `tests/**`. - `fail_ci_if_error` gated on `github.repository_owner`, which is the *base* repo's owner on a fork PR too, so the soft-fail its comment describes never applied. It keys off the head repo now. ## Docs The API behaviour was ours to misuse, not codecov's to explain. Three traps, all confirmed against the live API: - `file_report/<path>/` 404s with `coverage info not found` because the route swallows the trailing slash into the path. Without it the endpoint returns `line_coverage`. - `?pullid=N` always compares the PR's **current** head. `?base=&head=` asks about an earlier commit. - the compare response has no `patch_totals` key, and `.name` is `{base, head}` rather than a string, so a filename lookup silently matches nothing. A working recipe already existed in `running-tend`, but that skill is scoped to CI. `tests/CLAUDE.md` owns coverage investigation, so the queries go there and `running-tend` points at them instead of keeping a second copy. Re-running the corrected query against #3605's failing commit reproduces the miss exactly: `src/git/remote_ref/github.rs:164`, the `gh repo set-default` hint, matching what the session eventually found by hand. ## This PR demonstrates it It changes no Rust at all, only YAML and markdown. Codecov still reported a **10-file, 111-line patch** on its first commit, because it based the comparison on `203603909` rather than the real merge-base `32f380a27`. Every main commit in between has no report: | commit | ci run | report | |--------|--------|--------| | `32f380a27` | queued | no | | `9645e3e13` | cancelled | no | | `bcd1ffdfd` | cancelled | no | | `8865f20ab` | cancelled | no | Every one of those 111 patch lines belongs to somebody else's merged commit. It passed at 100% only because those commits are well covered. > _This was written by Claude Code on behalf of @max-sixty_ --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: worktrunk-bot <254187624+worktrunk-bot@users.noreply.github.com>
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #3596, which walled the test suite off from the network. That PR left the same unbounded call live for real users:
Repository::default_branch()is the one detection helper allowed to fall through togit ls-remote, and nothing limited how long it could take not to answer — an unanswered SYN costs ~127 s per address on Linux (tcp_syn_retries=6) and git tries each of a host's addresses in turn, so a remote behind a dropped VPN or a dead host stalledwt list --fullandwt switchfor minutes.Cmd::timeoutdidn't bound wall-clockAdding a bound alone would have been decorative, which is the substance of this PR.
run_with_timeout_implkilled only the direct child, and a grandchild inherits the child's stderr pipe — so a surviving one held the write end open andread_to_endblocked until it exited. Measured on a 3 s timeout over anls-remotewhose upload-pack sleeps 120 s:git-remote-httpssitting inconnect()is exactly that shape: it doesn't notice git died. So a timed child spawns into its own process group and expiry tears down the group —killpgwith TERM → KILL escalation on Unix,taskkill /T /Fon Windows, matchingwt step tether. Every existing.timeout()caller (the fsmonitor stop/lsof probes,reap.rs, the shell probe) was latently unbounded the same way and is fixed with it.The isolation costs the kernel's tty broadcast: a Ctrl-C no longer reaches a timed child, so the user waits out the remaining bound. That's seconds, against an orphan holding a pipe for as long as its own operation takes, once per spawn. Recorded at
run_with_timeout_impland in CLAUDE.md's Signal Handling section, whose "only the current child does" claim was no longer true.The cache is the other half
A timed-out query takes the local-inference fallback but is not persisted to
worktrunk.default-branch. That cache is what stops later calls from re-detecting, so a guess made while the network was down would otherwise become the repo's permanent answer.detect_from_remotenow returns aRemoteDetectionenum so the cacheability decision is explicit and exhaustive rather than anOptionthat loses the distinction.Only a timeout separates cleanly, via
ErrorKind::TimedOut.ls-remoteexits 128 whether the network is down or the remote simply has no HEAD, so telling those apart would mean reading git's error text — and re-querying on every command is the cost the cache exists to avoid. Those stay cached, as before.Reviewing
src/shell_exec.rs— the process-group teardown; its docstring carries the rationalesrc/git/repository/config.rs—RemoteDetection, the 10 s bound, and the no-persist pathsrc/git/repository/mod.rs—run_commandnow delegates to arun_command_boundedthat takes the boundAlso routes the four remaining hand-rolled git test envs (
src/git/remove.rs,src/git/repository/tests.rs,tests/integration_tests/bare_repository.rs) throughconfigure_git_env, so they carry #3596'sGIT_ALLOW_PROTOCOLdeny rather than re-deriving a subset of it. All four run local-only git commands today, so this is completeness, not a live hole — and a full suite run underGIT_TRACEconfirms zerogit-remote-*transport-helper spawns across 4374 tests.Testing
Three tests, none of which touch the network: the grandchild case via
sh -c 'sleep 30; :'(which stops the shellexecing sleep, so there really is a grandchild), the end-to-end no-persist behavior viaremote.origin.uploadpackpointed at asleep, and the fail-fast unresolvable remote, which had no coverage before. The second waits out the real 10 s bound; both hanging-remote tests are#[cfg(unix)]since the vehicle needs a POSIXsleep, sotest (windows)doesn't cover the no-persist path.