Skip to content

perf(list): resolve the GitLab project ID once per command - #4216

Merged
max-sixty merged 2 commits into
mainfrom
nightly/clean-35570157989
Sep 21, 2026
Merged

max-sixty merged 2 commits into
mainfrom
nightly/clean-35570157989

Conversation

@worktrunk-bot

Copy link
Copy Markdown
Collaborator

wt list --full on a GitLab remote spends one glab repo view API call per row resolving a value that is the same for every row. gitlab_project_id is called from detect_gitlab, which runs once per branch, and the project ID it fetches is a repo-level constant used only to filter MRs by source project. This memoizes it for the process.

wt list renders progressively but cannot finish until every row's task returns, so each of those round trips extends the command the user is waiting on — the case CLAUDE.md's Network Access section calls out for run-to-completion commands. It also spends N times the GitLab API quota for one answer.

The cache is a LazyLock<DashMap<PathBuf, Option<u64>>> keyed by the worktree root, matching the path-keyed process-wide caches in git::repository rather than a bare static — tests run many commands in one process, and a bare static would let one repo's answer leak into another's. The entry API holds the shard lock across the fetch, so concurrent rows sharing the key wait on the first call instead of each spawning their own; a failed lookup caches as None, since retrying it per row is exactly the cost being removed.

The regression test asserts the spawn count through mock_calls rather than the rendering, because the table looks identical either way. On this branch's parent it fails with 5 glab repo view calls against an expected 1.

A drive-by in the same doc comment: it described gitlab_project_id as "the GitLab equivalent of get_origin_owner for GitHub", and get_origin_owner no longer exists — the GitHub side now resolves its owner through branch_owner_repo, from local git config with no network call at all.

Verified locally: cargo test --test integration ci_status (63 passed), cargo test --bins ci_status (42 passed), cargo clippy --all-targets --all-features, cargo fmt --check.

`gitlab_project_id` shells out to `glab repo view`, a GitLab API call, and
`detect_gitlab` runs once per branch — so `wt list --full` over N worktrees
spent N round trips resolving one repo-level constant. Memoize it per
worktree root for the process, keyed by path like the process-wide caches
in `git::repository`, so in-process test runs can't read another repo's
answer.

The doc comment also pointed at `get_origin_owner`, which no longer
exists; the GitHub side resolves its owner through `branch_owner_repo`
from local git config.
@worktrunk-bot worktrunk-bot added the nightly-cleanup Issues found by nightly code quality sweep label Sep 21, 2026

@worktrunk-bot worktrunk-bot left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Self-review; two notes, neither blocking.

The None branch of the memo is the one part of GitLab CI detection that never retries. CachedCiStatus expires every 30–60 s, so a long-lived picker re-enters detect_gitlab — including on an explicit alt-r, which re-runs the collect pipeline in-process (PickerCollector::invoke, the if cmd.trim() == "refresh" branch) — and from now on gets the process-cached None back. The blast radius is narrow: with no project ID a branch with exactly one MR still resolves through the unambiguous-single-MR fallback, so only a branch carrying several same-named MRs goes dark. Leaving it as-is is defensible, but it's the maintainer's call: caching only Some would restore the retry at the cost of one extra failing glab repo view per row in a repo glab can't answer for.

Related, and the reason it's worth stating: the docstring's justification — "retrying it once per row is the cost this exists to remove" — holds for a successful lookup but overstates the failing one. When the project ID can't be resolved, detect_gitlab still issues its own per-row glab mr list, so caching the failure halves that repo's calls rather than removing the duplication.

The keying rationale describes a test shape this suite doesn't have. Nothing reaches gitlab_project_id in-process: integration tests spawn wt as a subprocess (the mocks are that same binary under another argv[0]), and this module's in-process tests cover only the parsers. Path-keying is still right, for the reason the # Caching section in src/git/repository/mod.rs gives for its own statics — "so two Repository instances pointed at the same path see the same answer" — which doesn't claim coverage that isn't there. Suggestion inline; I'll push it.

Comment thread src/commands/list/ci_status/gitlab.rs Outdated
The keying rationale cited tests running many commands in one process, but
nothing reaches gitlab_project_id in-process: integration tests spawn wt as a
subprocess and this module's in-process tests cover only the parsers. Give the
reason src/git/repository/mod.rs gives for its own path-keyed statics, and say
what caching a failed lookup costs.
@max-sixty
max-sixty merged commit 325153b into main Sep 21, 2026
37 checks passed
@max-sixty
max-sixty deleted the nightly/clean-35570157989 branch September 21, 2026 07:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

nightly-cleanup Issues found by nightly code quality sweep

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants