Skip to content

fix(git): back off host probes a wedged runtime never answers - #15293

Open
OrcaWin wants to merge 2 commits into
mainfrom
OrcaWin/wsl-git-probe-breaker
Open

OrcaWin wants to merge 2 commits into
mainfrom
OrcaWin/wsl-git-probe-breaker

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Aug 18, 2026 •

Copy link
Copy Markdown
Collaborator
Files Added Deleted Net
Test 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​654 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​654
Prod 3 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​538 $\color{#cf222e}{\Huge{\mathbf{−}}}$​11 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​527

What

Crash report f2521868 (v1.4.184, Windows, WSL Ubuntu-24.04). In the hour before the user force-killed the app: 431 git remote get-url calls, 414 of them dying on the 30s deadline, peaking at 15 concurrent wsl.exe children with a max span of 120s. It ran for two hours and got worse over time (44 → 68 → 125 → 73 → 59 → 32 slow calls per 10-minute bucket). The renderer stayed healthy; every git-backed panel was permanently stale. The user's own note: "session restored problem infinitly repeat".

Why it never recovered

REMOTE_URL_PROBE_TIMEOUT_MS is 30s, so a wedged host makes the probe settle, not hang. runCoalescedProbe only joins an in-flight probe, so the entry is deleted in its finally and PROBE_COALESCE_STALE_MS never trips. The next poll re-issues immediately. There was no negative cache, no circuit breaker, and nothing bounding concurrency — so the loop fed itself: more spawns → more contention → each 30s deadline stretched toward 120s → more overlap.

The fix

Per-host admission control in a new src/main/git/git-host-probe-breaker.ts, consulted from readRemoteUrl — the single function every forge-identity probe funnels through (remote-ref-probe-cache, github-repository-identity, gitlab-project-ref-resolution, hosted-review), covering native, WSL and SSH/relay in one place. The git call itself is unchanged.

A three-rung ladder on consecutive transient failures. A stable answer — including a repo reporting no such remote — is proof the host is alive and clears the whole budget. "Consecutive" means within one episode: a streak decays after 120s, so two failures either side of a laptop suspend are not counted together.

streak behaviour why that rung
1 nothing changes a blip: cold WSL interop, one dropped relay frame. Zero happy-path cost.
2 ceiling drops 8 → 1, overflow queues behind the single slot the host has burned 60s answering nothing, so stop fanning out — but two timeouts are not yet proof it is wedged, so the overflow waits for the trial rather than being told about an outage that has not been established
3 breaker opens wedged; probing on demand only feeds the loop

Backoff: 30s × 2^(opens−1), capped at 120s. 30s is one probe deadline — retrying sooner spends more wall time inside the host than outside it. 120s is the longest span a single wedged call was observed to hold the host in this report; past that, waiting buys no further idle time and only delays recovery. Neither is a permanent give-up.

Host scoping (never global): native | wsl:<distro> | ssh:<connectionId>:<generation>. The key is derived from the route the runner will actually take, not from the caller's hint: a Windows-drive worktree linked into a WSL repo runs host git and is keyed native; a \\wsl$\… UNC cwd names its distro and wins over the hint, the way resolveCommand reads it; off Windows nothing routes into WSL. Distro names are case-folded because wsl.exe matches them case-insensitively. An SSH reconnect bumps the generation, which is a new key, so a fresh transport never serves the old one's cooldown, and the older generations of that connection are retired so churn cannot grow the state map.

Recovery is unattended and needs no restart: one answer resets streak, open count and deadline. Worst case is one cap interval.

Visibility. The user in this report had no idea why every panel was stale. Transitions leave durable crash breadcrumbs — git_host_probe_breaker_open, git_host_probe_breaker_still_open (coalesced on a 5-minute floor so a two-hour outage is still legible in a 30-entry ring), git_host_probe_recovered. Individual failures log nothing.

Fix evidence

Simulated incident — remote-url-probe-wedged-host-backoff.test.ts

A wedged distro, every spawn burning its full 30s deadline, polled every 10s for a simulated hour:

reported incident with this change
spawns in the hour 431 27
per-10-min buckets 44, 68, 125, 73, 59, 32 7, 4, 4, 4, 4, 4
peak concurrency 15 8 healthy → 1 degraded → 0 open

The escalating shape is gone; it converges to one deadline per 150s and stays there. A single success then restores full speed — five polls afterward issue five spawns.

A 15-wide burst (the report's peak) against an already-failing host issues zero spawns. After recovery, the same 15-wide burst issues all 15 again.

Real wsl.exe on Windows 11, Ubuntu-24.04 (the report's distro)

HEALTHY_SERIAL_MS       [89,76,87,80,81,85,72,72]   breaker state: null (nothing retained)
BURST15_UNGUARDED_MS 268  GUARDED_MS 248
BURST15_UNGUARDED_MS 235  GUARDED_MS 276
BURST15_UNGUARDED_MS 253  GUARDED_MS 266
COLD_START_PROBE_MS     1331   (after `wsl --terminate Ubuntu-24.04`)   state: null

A 15-wide burst — wider than the ceiling of 8 — is within noise of unguarded, three rounds each way: wsl.exe spawn cost already serializes these harder than the ceiling does. A genuinely cold distro answers in 1.3s, 22× inside the deadline, and leaves no failure state — which is precisely why rung 1 must be free.

A nonexistent distro fails in ~35ms with a non-transient error, so the breaker correctly stays out of it (state: null): a fast, cheap, deterministic failure is not a storm, and backing it off would only delay a distro that is about to come online.

Gates

Re-run on macOS after the adversarial-review fixes:

$ pnpm vitest run --config config/vitest.config.ts \
    src/main/git/git-host-probe-breaker.test.ts \
    src/main/git/remote-url-probe.test.ts \
    src/main/git/remote-url-probe-wedged-host-backoff.test.ts
 Test Files  3 passed (3)
      Tests  29 passed (29)

$ pnpm vitest run --config config/vitest.config.ts src/main/git src/main/github \
    src/main/gitlab src/main/source-control src/main/azure-devops src/main/gitea src/main/bitbucket
 Test Files  185 passed | 2 skipped (187)
      Tests  2043 passed | 4 skipped (2047)

$ pnpm run typecheck        # clean, no output
$ pnpm exec oxlint          # clean, no output
$ pnpm run check:max-lines-ratchet
max-lines ratchet OK — 188 grandfathered suppression(s), no new bypasses.
$ pnpm run audit:code-quality:native    # clean
$ pnpm run audit:code-quality:type-aware # clean
$ pnpm run check:reliability-gates
Reliability gate manifest check passed for 85 gate(s).
$ pnpm dlx knip@5.88.1 --config config/knip.json   # no new unused/duplicate exports

The earlier Windows run of this branch reported 8 failed | 151 passed in src/main/git; those 32 failures were real-git and symlink-privilege environment failures on that machine (Command failed: git init, EPERM: symlink) and did not reproduce on macOS or in CI. All 60 GitHub Actions checks on this PR are green.

ELI5

Orca keeps asking each of your repos "who hosts you?" by running a tiny git command. On Windows that means starting wsl.exe. If WSL gets stuck, that command doesn't fail — it just sits there for 30 seconds and then gets killed.

The old code treated that like any other finished answer: it threw it away and asked again. And again. Fifteen questions at a time, none of them ever answered, for two hours — each one making the next one slower, until the user killed the app.

Now Orca notices. One bad answer is forgiven; WSL is genuinely slow when it wakes up. Two, and it stops asking fifteen questions at once and asks one. Three, and it stops asking for half a minute, then a minute, then settles at asking once every two minutes — forever, but gently. The moment WSL replies even once, everything snaps back to normal instantly. Nobody has to restart anything, and if it ever happens again the crash report says so in plain words.

Trade-offs

  • A burst wider than 8 queues. Measured as within noise on real WSL (above), because process spawn already dominates. On a host where probes are much slower than spawn setup, a fan-out over many distinct repos would take two waves instead of one. Callers queue rather than fail, so nothing is lost — only ordered. This is the one thing that could make a healthy host marginally slower, and it's the reason the ceiling is 8 rather than something tight.
  • A degraded host serializes concurrent probes. After two consecutive failures the ceiling is one slot and the overflow waits rather than failing: at that rung the breaker has concluded nothing yet, and the running trial may be about to disprove the failure. The wait is bounded by one deadline, after which the host has either answered (everyone proceeds) or the breaker has opened (everyone is refused immediately, with a reason). Only once the breaker is open does a probe get refused without running, and only a host that has already failed has its queue depth capped at 64.
  • Worst-case staleness on a wedged host is one cap interval (120s) after the host recovers, if no other probe happens to land. Bounded, unattended, no restart. Tightening the cap would speed recovery at the cost of more spawns during the outage.
  • Blocked probes surface as transient/unavailable, exactly like the deadline kill they stand in for, so no caller can cache them as an answer. isTransientGitProbeError recognises them, which is what keeps hosted-review on its failure path and the ref caches from publishing a false negative.
  • Not covered: git calls outside readRemoteUrl. A wedged host still costs each of those one deadline. Gating them would mean shedding user-initiated git, which is a different and much riskier decision. readRemoteUrl is where the report's storm actually lived.
  • A never-settling probe would have pinned its host forever, so a slot is reclaimed 180s after admission — comfortably past the longest a single guarded region can legitimately run (a 5s routing probe, a 30s deadline, and a 30s direct-WSL fallback retry). The abandoned probe's own result still counts when it eventually settles; only its slot is returned early.

Limitations

Written to be read as a list of things this does not do.

Scope. The breaker guards readRemoteUrl only. Every other git call against the same wedged host — status polling, upstream checks, history — still pays a full deadline each, unbounded and uncoalesced. The incident's total wsl.exe pressure is reduced, not eliminated, and a user on a wedged distro can still see a slowdown after this fix.

Recovery depends on callers. Nothing inside the module schedules a retry. Recovery works only because every consumer treats a transient/blocked probe as uncacheable (remote-ref-probe-cache returns null without publishing, github-repository-identity caches only a stable missing remote, hosted-review throws to stay on its failure path), so the next poll re-enters and becomes the half-open trial. If a future caller ever starts caching a transient result, recovery stalls silently: there is no watchdog and no timer in the breaker itself.

A half-wedged host is not defended. A host that alternates failure and success never accumulates a streak of three, so a 50%-wedged host keeps storming at full width. That is the deliberate design — a stable answer is proof the host is alive — but it is a real gap. TRANSIENT_PROBE_PATTERNS is also narrow: a WSL failure whose text is not literally timed out / ETIMEDOUT / ECONNRESET / EPIPE / connection … counts as an answer and clears the entire budget.

The native bucket covers every local repo. One repo on a dead network mount or an unmounted external drive produces timed out errors filed under native and, at three consecutive occurrences, backs off the user's healthy local repos too. In practice interleaved successes keep the streak from accumulating, and the streak now decays after 120s, but nothing in the design prevents it.

One guarded probe can hold a degraded host longer than one deadline. gitExecFileAsync may spend up to 5s in prepareWslLinkedWorktreeGitRouting and then retry the direct-WSL fallback with a second full timeout, so ~65s inside a single guarded region is possible. Everything else on that host is serialized behind it for that span. The 180s slot-reclamation bound is sized above this, which also means a genuinely hung probe is tolerated for up to 180s before its slot is returned.

Queue fairness is unspecified. release wakes all waiters at once and tryAdmit admits fresh arrivals without consulting queued, so a queued caller can lose repeatedly to newcomers under sustained load, and there is no per-caller deadline on the wait — a queued caller inherits the wall time of the probes ahead of it. No deadlock is possible (waiters are only enqueued while a slot is occupied, and every in-flight probe releases and wakes them), but a wave of many slow-but-succeeding probes on one key could push a queued caller past PROBE_COALESCE_STALE_MS (60s), at which point the coalescer above abandons and re-issues while the abandoned probe still holds a slot. I could not construct a realistic workload that reaches this; it is unverified residual risk, not a demonstrated defect.

Wake-up cost is O(waiters) per release. With the healthy queue now unbounded, a very wide burst re-wakes every waiter on each release. At a few hundred waiters this is microseconds of closure calls; it is not free, and it is the price of never converting a healthy host's overflow into a failure.

Clock handling is a clamp, not a monotonic source. Cooldowns still use Date.now(). A backwards step is now repaired on read (a deadline further out than the 120s cap is pulled back to now), but a forward step simply expires a cooldown early, and the module has no performance.now() baseline.

Process-global state. gitProbeHostStates is module-level. Only the probe suites call _resetGitHostProbeBreaker, so a future test elsewhere that produces three consecutive transient-shaped failures could gate later tests in the same file with no obvious signal.

Not re-verified by this review. The Windows wsl.exe measurements in the evidence section above (HEALTHY_SERIAL_MS, the BURST15 rounds, the 1331ms cold start, the ~35ms nonexistent-distro failure) and the claim that the 32 failures in the original Windows run were pre-existing were taken on the author's machine and could not be reproduced during review, which ran on macOS. Everything under "Gates" was re-run.

Eviction still has a floor. If all 64 tracked hosts are simultaneously mid-outage, the third eviction pass drops one that is still cooling down — bounded memory has to win somewhere. That host is then re-probed at full width once. In the reported configuration this needs 64 concurrently-wedged distinct hosts.

Adversarial review

Three independent reviewers audited this branch — none of them the author. Lenses: field-report fidelity (does the fix actually change the reported failure), self-inflicted denial of service (can the guard strand a host that is fine), state retention and eviction, non-monotonic time, concurrency and lost wake-ups, and evidence honesty (does the diagnostic text match what happened). They returned ten blocking findings (three of which are the same queue-overflow defect, found independently by all three). I rejected none: every one describes a real defect, and all ten are fixed in 27fdeba, each with a regression test that I verified fails when the corresponding guard is disabled. One sub-claim inside one finding is wrong and is called out below; the finding it sits in is still real.

The notable ones:

  • A backwards clock step stranded the host permanently. blockedUntilMs is an absolute Date.now() deadline with no upper clamp. An NTP step correction, VM snapshot restore or dual-boot RTC fix left every later admission on the blocked branch for the length of the jump — hours or days, not the documented 120s — and because recovery is entirely success-driven, no probe could run to produce the success that clears it. Exactly the outcome the PR exists to prevent. Fixed by repairing a deadline further out than the cap on read.
  • The host key did not match the route, which silently disabled the fix. The key came from wslDistro / the UNC path, but gitExecFileAsync routes a Windows-drive worktree linked into a WSL repo to host git. Those instant successes were filed under wsl:<distro> and reset the wedged distro's streak on every poll, so on a mixed layout the breaker never opened at all. Reproduced against the real readRemoteUrl before the fix: 20 interleaved poll cycles, 20 spawns on the wedged host, breaker state still null. The PR body previously claimed this case was only a mislabelled breadcrumb; that was wrong and the text is corrected above. The key is now derived from the same routing decision the runner makes. The reviewer also claimed the inverse case (a \\wsl$ UNC path with no wslDistro) runs native git — that part is incorrect: resolveCommand routes on the cwd regardless of the hint, so a UNC cwd does execute in the distro. The only real defect in that direction was the missing platform gate, which is fixed.
  • Queue overflow was shed as a host failure on hosts with no failures. MAX_QUEUED_PER_HOST = 64 on top of a ceiling of 8 meant caller 73+ received a GitHostProbeBlockedError that isTransientGitProbeError reports as unavailable, even with consecutiveUnavailable === 0. Reachable: hosted-review issues an uncoalesced assertRemoteUrlReadable per branch, the per-card refresh runs on mount and on every visibilitychange, and every local repo shares the single native key. Now only a host that has actually failed has its backlog bounded.
  • The serialize rung failed probes on a host that had merely timed out twice, and a shed reached neither recordAnswered nor recordUnavailable, so the rung sustained itself while one slow probe held the single slot. Downstream, hosted-review-branch-cache turns that into a 60s → 15min lookup backoff. The overflow queues now.
  • Eviction deleted the open breaker first. A healthy host is deleted on release and re-inserted at the end of the Map, so a host with retained failure state is always the oldest entry — and evictIdleHostStates walked insertion order. A reviewer reproduced the wedged distro being evicted at flap 63 of SSH reconnect churn, after which the still-wedged host was probed again immediately. Eviction now drops the least informative entry first, and an SSH connection's retired generations are pruned at the source.
  • No bound on an in-flight slot. A probe that never settled would pin the degraded ceiling of one for the life of the process — the same defect coalesced-probe.ts was written to fix, reintroduced. Neither reviewer could construct a live hang (both call paths carry a deadline), so this is defence in depth rather than a demonstrated bug, but the guard is what keeps any future hang from turning into "this host is dead forever".
  • consecutiveUnavailable never decayed, so two failures a laptop-suspend apart still counted as consecutive and pinned the host at the degraded ceiling. It now decays after 120s.
  • The shed message asserted failures that never happened — verbatim, "Git host native did not answer 0 consecutive probes". It now names the reason.

Also fixed while in there, from the reviewers' non-blocking notes: parseWslUncPath had no platform guard (a literal //wsl$/x directory off Windows was keyed as a distro), and host keys were not case-folded, so one distro spelled two ways split into two half-blind budgets.

Retention moved into a new git-host-probe-state.ts — it is where this fails silently, and the eviction ordering only makes sense next to the forgetting rules. No max-lines suppression was added or bumped.

Relation to the #15039–#15257 stack

Complementary, not overlapping. Based on main, not stacked — #15039 is already merged, and #15060 / #15257 touch runner.ts, wsl-git-read-environment.ts and wsl-direct-git-read-commands.ts. This PR touches remote-url-probe.ts and adds two new files. No textual conflict, and neither branch depends on the other.

That stack fixes what a git call returns (argv mangling through --exec, the login-shell banner landing on stdout, and #15257 removing the shell from reads entirely, which should cut per-call cost materially). This PR fixes how often Orca is willing to make a call that never returns. A perfectly correct, shell-free probe against a wedged or absent distro still burns its full deadline, and before this change Orca would retry that forever at unbounded concurrency.

Incidental confirmation from the real-WSL run above: on stock Ubuntu 24.04 the probe came back as

To run a command as administrator (user "root"), use "sudo <command>".
See "man sudo_root" for details.

git@github.com:acme/orca.git

— a live reproduction of exactly the banner contamination #15060/#15257 fix, observed on main while validating this change. Both fixes are needed: that one makes the answer correct, this one stops the retry storm when there is no answer at all.

Crash f2521868: a wedged WSL distro cost 431 `git remote get-url` calls in
one hour, 414 of them dying on the 30s deadline, peaking at 15 concurrent
`wsl.exe` children — and it escalated rather than settling, because the probe
*settles* on its deadline, so the in-flight coalescer drops its entry and the
next poll re-issues immediately. There was no negative cache, no circuit
breaker, and no concurrency bound, so "problem infinitly repeat" was literal.

Add per-host admission control at `readRemoteUrl`, the one function every forge
identity probe funnels through. One unanswered probe changes nothing; two drop
the host to a single slot and shed the overflow; three open a breaker that
backs off 30s, 60s, then 120s. Any answer — including a repo reporting no such
remote — clears the whole budget, so recovery is unattended and bounded by one
cap interval. State is keyed by the runtime that executes git (native, WSL
distro, SSH connection generation), so a dead distro never gates a host that is
answering. Transitions leave durable crash breadcrumbs; failures do not.
@coderabbitai

coderabbitai Bot commented Aug 18, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Added a host-scoped Git probe breaker with WSL and SSH-generation keys. The breaker limits concurrency, queues or blocks probes, tracks unavailable results, applies capped cooldowns, and records outage breadcrumbs. Shared state now supports stale-slot recovery, SSH-generation cleanup, and bounded retention. Local and SSH remote-URL probes use the breaker, and blocked probes are transient errors. Vitest coverage verifies breaker, recovery, isolation, wedged-host backoff, and breadcrumb behavior.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 44.12% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description explains the problem, design, trade-offs, limitations, and test evidence, but omits required template sections such as Linked Issue, Visual Proof, checklist, and author details. Add the missing template sections, including a real Linked Issue, Visual Proof or N/A, completed Testing and Checklist items, Review, Notes, and Author details.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly identifies the Git probe backoff fix for wedged runtimes, which matches the main change.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/main/git/git-host-probe-breaker.ts (1)

151-163: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Clamp a cooldown that a backward wall-clock step pushed into the future.

tryAdmit compares state.blockedUntilMs against Date.now(). If the system clock steps backward, for example after an NTP correction or a VM suspend, blockedUntilMs can sit far beyond GIT_HOST_PROBE_MAX_COOLDOWN_MS in the future. The host then stays suppressed for the length of the step, which removes the unattended recovery this module exists to keep. Clamp the deadline to the ceiling so the trial probe still runs.

🛡️ Proposed guard
 function tryAdmit(state: HostProbeState): Admission {
   const now = Date.now()
+  // A backward clock step must not stretch a cooldown past its own ceiling.
+  if (state.blockedUntilMs > now + GIT_HOST_PROBE_MAX_COOLDOWN_MS) {
+    state.blockedUntilMs = now
+  }
   if (state.blockedUntilMs > now) {
     return 'blocked'
   }

The assignment keeps blockedUntilMs > 0, so the half-open trial path still applies.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 35864bb4-5402-4fec-a18b-f05361dd0729

📥 Commits

Reviewing files that changed from the base of the PR and between 3a9f40e and 85439df.

📒 Files selected for processing (5)
  • src/main/git/git-host-probe-breaker.test.ts
  • src/main/git/git-host-probe-breaker.ts
  • src/main/git/remote-url-probe-wedged-host-backoff.test.ts
  • src/main/git/remote-url-probe.test.ts
  • src/main/git/remote-url-probe.ts

Included review availability: Your plan includes up to 10 reviews per rolling hour; 6 remain after this review.

Comment thread src/main/git/remote-url-probe.ts Resolved
Adversarial review found the breaker could do the two things it exists to
prevent: block a host forever, and fail probes on a host that is fine.

- The cooldown is an absolute wall-clock deadline. A backwards clock step (NTP
  correction, VM snapshot restore, RTC fix) left every later admission blocked
  for the size of the jump, and recovery is success-driven, so no probe could
  run to earn the success that clears it. Clamp a deadline further out than the
  cap back to now.
- The host key came from the caller's `wslDistro` hint, not from the route the
  runner takes. A Windows-drive worktree linked into a WSL repo runs *host* git,
  so its instant successes were filed under the wedged distro and reset the
  streak every poll — the breaker never opened. Resolve the linked-worktree
  route first, take the cwd's distro before the hint, and gate on win32 the way
  `resolveCommand` does. Keys are case-folded because wsl.exe matches distro
  names case-insensitively.
- Queue overflow was shed as a `blocked` error, which callers classify as a host
  failure. On a host with a clean budget that invented an outage: >72 concurrent
  probes (one hosted-review lookup per visible worktree card, all sharing the
  `native` key) hard-failed the tail. Only a host that has actually failed gets
  its backlog bounded.
- The serialize rung shed instead of queueing, so two timeouts were enough to
  fail every concurrent probe — and a shed reaches neither recorder, so the rung
  sustained itself. It queues now; the wait is one deadline, after which the
  host has answered or the breaker has opened.
- Nothing bounded an in-flight slot. A probe that never settled pinned the
  degraded ceiling of one for the life of the process — the defect
  `coalesced-probe.ts` already had to fix once. Slots carry an admission stamp
  and are reclaimed past 180s.
- `consecutiveUnavailable` never decayed, so two failures a suspend apart still
  counted as consecutive and pinned the host at the degraded ceiling.
- Eviction walked insertion order, and an open breaker is always the *oldest*
  entry (a healthy host is deleted on release and re-inserted at the end), so
  ~64 keys of SSH reconnect churn silently erased the wedged host's cooldown.
  Evict the least informative entry first, and retire an SSH connection's older
  generations at the source so churn stops growing the map at all.
- The shed message interpolated a streak of 0 into "did not answer 0 consecutive
  probes". It now names the reason it was shed.

Retention moves to `git-host-probe-state.ts`: it is where this fails silently,
and the eviction ordering only makes sense next to the forgetting rules.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (2)
src/main/git/git-host-probe-breaker.test.ts (2)

165-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Derive the WSL host key instead of hardcoding 'wsl:Ubuntu'.

gitProbeHostKey lowercases the distro, so production emits wsl:ubuntu. The suite drives the breaker with 'wsl:Ubuntu', a key the derivation never produces. The behavior under test is key-agnostic, so the assertions still pass, but the blocked-message assertion at Line 318 then pins text that no real breadcrumb contains.

Build the key once from gitProbeHostKey and reuse it.

♻️ Proposed refactor
+const WSL_HOST = gitProbeHostKey({ wslDistro: 'Ubuntu' })

Then replace the literals, for example:

-    const outstanding = probeOnce('wsl:Ubuntu', () => held.promise)
+    const outstanding = probeOnce(WSL_HOST, () => held.promise)
-    expect(await blockedMessage('wsl:Ubuntu')).toBe(
-      'Git host wsl:Ubuntu did not answer 3 consecutive probes; suppressed for ~30s.'
+    expect(await blockedMessage(WSL_HOST)).toBe(
+      `Git host ${WSL_HOST} did not answer 3 consecutive probes; suppressed for ~30s.`
     )

Also applies to: 311-320


262-277: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Assert the concurrent outcome from the awaited promise. Replace the mutable .then capture with expect(await concurrent).toBe('answered'); this keeps the assertion direct and preserves the ordering check.


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3c69d1b3-ba3a-44a4-9735-b4e96ea0bcf8

📥 Commits

Reviewing files that changed from the base of the PR and between 85439df and 27fdeba.

📒 Files selected for processing (5)
  • src/main/git/git-host-probe-breaker.test.ts
  • src/main/git/git-host-probe-breaker.ts
  • src/main/git/git-host-probe-state.ts
  • src/main/git/remote-url-probe-wedged-host-backoff.test.ts
  • src/main/git/remote-url-probe.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/main/git/remote-url-probe.ts
  • src/main/git/remote-url-probe-wedged-host-backoff.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.

Comment on lines +59 to +76
/**
* Every guarded probe carries its own deadline, but nothing here can prove it,
* and at the degraded ceiling of one slot a probe that never settled would
* block its host for the life of the process — the exact defect
* `coalesced-probe.ts` already had to fix once. Reclaim a slot past the longest
* a single guarded region can legitimately run: a 5s routing probe, a 30s
* deadline, and a 30s direct-WSL fallback retry, with room to spare.
*/
export const GIT_HOST_PROBE_SLOT_STALE_MS = 180_000

/**
* "Consecutive" has to mean "within one episode". Two deadline kills either
* side of a laptop suspend say nothing about the host now, and with no decay
* they would pin it at the degraded ceiling until some probe ran alone. Sized
* like the cooldown cap: past one full backoff interval a stale blip has had
* every chance to repeat.
*/
export const GIT_HOST_PROBE_STREAK_DECAY_MS = 120_000

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Shorten the production comments.

Keep each comment to the non-obvious fact that the code needs.

  • src/main/git/git-host-probe-breaker.ts#L59-L76: reduce the stale-slot and streak-decay rationale to brief comments.
  • src/main/git/git-host-probe-state.ts#L1-L9: reduce the module retention rationale to a brief summary.
  • src/main/git/git-host-probe-state.ts#L39-L44: reduce the SSH-generation cleanup rationale to a brief summary.
  • src/main/git/git-host-probe-breaker.ts#L150-L166: reduce the admission and queue rationale to brief comments.

As per coding guidelines, “Comments must be concise, non-obvious, and brief—prefer one line; do not explain obvious behavior or walk through code.”

📍 Affects 2 files
  • src/main/git/git-host-probe-breaker.ts#L59-L76 (this comment)
  • src/main/git/git-host-probe-state.ts#L1-L9
  • src/main/git/git-host-probe-state.ts#L39-L44
  • src/main/git/git-host-probe-breaker.ts#L150-L166

Source: Coding guidelines

Comment on lines +233 to +239
function reclaimStaleSlots(state: GitProbeHostState, now: number): void {
for (const slot of state.inFlight) {
if (now - slot.admittedAtMs > GIT_HOST_PROBE_SLOT_STALE_MS) {
state.inFlight.delete(slot)
}
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift

Schedule stale-slot reclamation.

reclaimStaleSlots runs only when a later caller reaches tryAdmit. If all admitted probes never settle and no later probe arrives, queued callers remain blocked and the host state never becomes idle or evictable after GIT_HOST_PROBE_SLOT_STALE_MS.

Schedule reclamation for the earliest slot expiry. Wake waiters and remove a now-forgettable state when that timer fires. Add a test with no subsequent admission.

Comment on lines +259 to +261
function recordUnavailable(hostKey: string, state: GitProbeHostState, halfOpen: boolean): void {
state.consecutiveUnavailable += 1
state.lastUnavailableAtMs = Date.now()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Decay the streak before recording an unavailable result.

Concurrent probes can settle more than GIT_HOST_PROBE_STREAK_DECAY_MS apart. Since decay only occurs in tryAdmit, those old failures still accumulate and can serialize or open the breaker after the defined episode window.

Proposed fix
 function recordUnavailable(hostKey: string, state: GitProbeHostState, halfOpen: boolean): void {
+  const now = Date.now()
+  if (
+    state.consecutiveUnavailable > 0 &&
+    now - state.lastUnavailableAtMs > GIT_HOST_PROBE_STREAK_DECAY_MS
+  ) {
+    state.consecutiveUnavailable = 0
+  }
   state.consecutiveUnavailable += 1
-  state.lastUnavailableAtMs = Date.now()
+  state.lastUnavailableAtMs = now
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function recordUnavailable(hostKey: string, state: GitProbeHostState, halfOpen: boolean): void {
state.consecutiveUnavailable += 1
state.lastUnavailableAtMs = Date.now()
function recordUnavailable(hostKey: string, state: GitProbeHostState, halfOpen: boolean): void {
const now = Date.now()
if (
state.consecutiveUnavailable > 0 &&
now - state.lastUnavailableAtMs > GIT_HOST_PROBE_STREAK_DECAY_MS
) {
state.consecutiveUnavailable = 0
}
state.consecutiveUnavailable += 1
state.lastUnavailableAtMs = now

This branch has not been deployed

No deployments
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