Skip to content

fix(host-service): dedupe PR refresh calls with repo-keyed cache#3884

Merged
Kitenite merged 3 commits into
mainfrom
gh-cli-throttle-debug
Apr 30, 2026
Merged

fix(host-service): dedupe PR refresh calls with repo-keyed cache#3884
Kitenite merged 3 commits into
mainfrom
gh-cli-throttle-debug

Conversation

@Kitenite
Copy link
Copy Markdown
Collaborator

@Kitenite Kitenite commented Apr 29, 2026

Multiple projects targeting the same GitHub repo each fired their own GraphQL query every 10s, and force=true paths bypassed the per-project debounce entirely. Replace with a single repo-keyed response cache so N projects on the same repo collapse to 1 call, branch-sync/tRPC bursts share the in-flight promise, and the polling cadence drops to 20s.

Description

Related Issues

Type of Change

  • Bug fix
  • New feature
  • Documentation
  • Refactor
  • Other (please describe):

Testing

Screenshots (if applicable)

Additional Notes


Summary by cubic

Deduplicates GitHub PR refresh calls in host-service with a repo-keyed cache and caches PR detail fetches to cut duplicate API/CLI calls across projects and views.

  • Bug Fixes
    • Added repo-keyed PR list cache (10s TTL) and gh pr view detail cache (30s TTL); both share in-flight promises and evict on failure.
    • Explicit refreshes (branch sync and refreshByWorkspaces) bypass the repo cache to avoid stale reads; bypassed fetches still populate the cache while poll ticks keep deduping.
    • Increased project refresh interval to 20s; removed per-project debounce/force and rely on in-flight dedupe via a unified refresh path.

Written for commit d1c299c. Summary will update on new commits. Review in cubic

Summary by CodeRabbit

  • Refactor
    • Reduced redundant GitHub requests to lower network traffic
    • Added repository-level deduplication with a 10s TTL to speed repeated PR listings
    • Cached pull request content fetches to avoid repeated external CLI calls
    • Unified refresh behavior across startup and workspace/branch triggers
    • Increased project refresh interval from 10s to 20s
    • Failed cache entries are evicted so subsequent attempts retry promptly

Multiple projects targeting the same GitHub repo each fired their own
GraphQL query every 10s, and force=true paths bypassed the per-project
debounce entirely. Replace with a single repo-keyed response cache so N
projects on the same repo collapse to 1 call, branch-sync/tRPC bursts
share the in-flight promise, and the polling cadence drops to 20s.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 29, 2026

📝 Walkthrough

Walkthrough

Repository-level promise dedupe cache for GitHub PR fetches (10s TTL, evict on failure) added; per-project refresh throttling (nextProjectRefreshAt) removed and PROJECT_REFRESH_INTERVAL_MS increased to 20s; refreshEligibleProjects no longer accepts force; refreshProject now accepts { bypassCache?: boolean } and bypasses repo cache when requested. Also adds a TTL-backed in-flight/fulfilled promise cache for gh pr view in the workspace-creation procedure.

Changes

Cohort / File(s) Summary
Pull-request polling
packages/host-service/src/runtime/pull-requests/pull-requests.ts
Increased PROJECT_REFRESH_INTERVAL_MS to 20s; removed per-project nextProjectRefreshAt throttling and force propagation; refreshEligibleProjects no longer takes force; refreshProject signature changed to accept { bypassCache?: boolean }; added repoPullRequestCache that deduplicates in-flight GraphQL/PR-node fetches by owner/name with a 10s TTL and evicts entries on fetch failure.
GitHub PR content caching
packages/host-service/src/trpc/router/workspace-creation/procedures/get-github-pull-request-content.ts
Introduced a normalized key TTL-based Map cache that stores in-flight/completed promises for gh pr view calls; reuses cached promise within TTL; evicts entry on rejection; parses and normalizes results (lowercased state, defaulted body, nullable logins -> null).

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant HostService
  participant RepoCache
  participant GitHubAPI

  Caller->>HostService: request PR data (project-trigger or explicit)
  HostService->>RepoCache: lookup cache by owner/name
  alt cache hit (valid TTL)
    RepoCache-->>HostService: return cached promise/result
  else cache miss or expired
    HostService->>RepoCache: store in-flight promise
    HostService->>GitHubAPI: fetch PR nodes (GraphQL / gh)
    GitHubAPI-->>HostService: PR data or error
    alt success
      RepoCache-->>HostService: resolved result (store with fetchedAt)
      HostService-->>Caller: return PR data
    else failure
      HostService->>RepoCache: evict entry
      HostService-->>Caller: return error
    end
  end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 I hop through code with whiskers bright,
Bundling promises, keeping fetches light.
Ten seconds of shelter, errors set free,
Refreshes simpler—now dance with me! 🥕✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding a repo-keyed cache to deduplicate PR refresh calls. It's concise, clear, and directly reflects the primary objective of the changeset.
Description check ✅ Passed The description includes a helpful summary and cubic-generated details about the changes, but the PR template sections (Related Issues, Type of Change, Testing, Screenshots, Additional Notes) are empty checkboxes and placeholder comments.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch gh-cli-throttle-debug

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
Review rate limit: 2/8 reviews remaining, refill in 41 minutes and 2 seconds.

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

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
packages/host-service/src/runtime/pull-requests/pull-requests.ts (1)

372-395: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Restore a cache-bypass path for explicit refreshes.

Now that refreshProject no longer accepts a force/bypass option, both refreshPullRequestsByWorkspaces() and the branch-sync follow-up are forced through the settled repo cache too. That means a PR opened/closed or checks update right after the last poll can still come back stale here for up to 10 seconds, which regresses the old “refresh now” behavior.

Suggested direction
- private async refreshProject(projectId: string): Promise<void> {
+ private async refreshProject(
+   projectId: string,
+   options: { bypassRepoCache?: boolean } = {},
+ ): Promise<void> {
- await Promise.all(
-   projectIds.map((projectId) => this.refreshProject(projectId)),
- );
+ await Promise.all(
+   projectIds.map((projectId) =>
+     this.refreshProject(projectId, { bypassRepoCache: true }),
+   ),
+ );

Thread that option down to getCachedRepoPullRequests() so only poll-driven refreshes reuse settled cache entries.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts` around
lines 372 - 395, refreshProject currently forces all callers through the settled
repo cache because it no longer accepts a force/bypass flag; restore a bypass
path by adding a boolean parameter (e.g., forceRefresh) to refreshProject and
thread it into performProjectRefresh so callers like
refreshPullRequestsByWorkspaces and the branch-sync follow-up can pass true to
bypass settled cache logic; ensure performProjectRefresh forwards the flag down
to getCachedRepoPullRequests (and any intermediate helpers) so only poll-driven
refreshes reuse settled cache entries while explicit refreshes skip it; preserve
inFlightProjects behavior by keying by projectId only and storing the promise
returned when forceRefresh is false, but for forceRefresh === true always call
performProjectRefresh directly (or store a separate in-flight entry) so explicit
bypasses are not blocked by settled cached promises.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 372-395: refreshProject currently forces all callers through the
settled repo cache because it no longer accepts a force/bypass flag; restore a
bypass path by adding a boolean parameter (e.g., forceRefresh) to refreshProject
and thread it into performProjectRefresh so callers like
refreshPullRequestsByWorkspaces and the branch-sync follow-up can pass true to
bypass settled cache logic; ensure performProjectRefresh forwards the flag down
to getCachedRepoPullRequests (and any intermediate helpers) so only poll-driven
refreshes reuse settled cache entries while explicit refreshes skip it; preserve
inFlightProjects behavior by keying by projectId only and storing the promise
returned when forceRefresh is false, but for forceRefresh === true always call
performProjectRefresh directly (or store a separate in-flight entry) so explicit
bypasses are not blocked by settled cached promises.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1bf7432f-4888-49ad-94eb-d6416ada85b8

📥 Commits

Reviewing files that changed from the base of the PR and between f5cf3f1 and fdcef1b.

📒 Files selected for processing (1)
  • packages/host-service/src/runtime/pull-requests/pull-requests.ts

Comment thread packages/host-service/src/runtime/pull-requests/pull-requests.ts
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 29, 2026

🧹 Preview Cleanup Complete

The following preview resources have been cleaned up:

  • ✅ Neon database branch

Thank you for your contribution! 🎉

The PR detail panel re-invokes `gh pr view` on every mount, so re-renders,
tab switches, and click-back patterns each shell out fresh. Wrap the
procedure in a 30s TTL cache keyed on `owner/name#prNumber`, sharing the
in-flight promise across concurrent callers and evicting on failure so
transient errors don't poison subsequent reads.
The branch-sync follow-up and the `refreshByWorkspaces` tRPC mutation
both fire when external state has just changed (local SHA moved, or a PR
was just merged) — exactly the case where the 10s settled cache returns
stale data. Thread a bypassCache option from those call sites down to
the cache lookup so explicit refreshes always fetch fresh while the
polling tick keeps deduping. Bypass paths still write to the cache so
subsequent polls benefit.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
packages/host-service/src/runtime/pull-requests/pull-requests.ts (1)

508-538: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep in-flight repo fetches reusable when bypassing settled cache.

bypassCache currently skips the lookup entirely, and fetchedAt is stamped before the GitHub call resolves. That lets two explicit refresh bursts for the same repo—or any slow fetch that runs longer than the 10s TTL—fan back out into duplicate GraphQL requests, which is the main failure mode this cache is meant to prevent.

Suggested fix
 private readonly repoPullRequestCache = new Map<
   string,
-  { promise: Promise<GraphQLPullRequestNode[]>; fetchedAt: number }
+  {
+    promise: Promise<GraphQLPullRequestNode[]>;
+    fetchedAt: number | null;
+  }
 >();

 private async getCachedRepoPullRequests(
   repo: NormalizedRepoIdentity,
   options: { bypassCache?: boolean } = {},
 ): Promise<GraphQLPullRequestNode[]> {
   const cacheKey = `${repo.owner.toLowerCase()}/${repo.name.toLowerCase()}`;
-  if (!options.bypassCache) {
-    const cached = this.repoPullRequestCache.get(cacheKey);
-    if (
-      cached &&
-      Date.now() - cached.fetchedAt < REPO_PULL_REQUEST_CACHE_TTL_MS
-    ) {
-      return cached.promise;
-    }
+  const cached = this.repoPullRequestCache.get(cacheKey);
+  if (
+    cached &&
+    (cached.fetchedAt === null ||
+      (!options.bypassCache &&
+        Date.now() - cached.fetchedAt < REPO_PULL_REQUEST_CACHE_TTL_MS))
+  ) {
+    return cached.promise;
   }

-  const fetchedAt = Date.now();
   const promise = (async () => {
     const octokit = await this.github();
-    return fetchRepositoryPullRequests(octokit, {
+    const nodes = await fetchRepositoryPullRequests(octokit, {
       owner: repo.owner,
       name: repo.name,
     });
+    const entry = this.repoPullRequestCache.get(cacheKey);
+    if (entry?.promise === promise) {
+      entry.fetchedAt = Date.now();
+    }
+    return nodes;
   })();
   // Evict on failure so the next caller retries instead of serving a
   // poisoned cache entry for the rest of the TTL.
   promise.catch(() => {
     if (this.repoPullRequestCache.get(cacheKey)?.promise === promise) {
       this.repoPullRequestCache.delete(cacheKey);
     }
   });
-  this.repoPullRequestCache.set(cacheKey, { promise, fetchedAt });
+  this.repoPullRequestCache.set(cacheKey, { promise, fetchedAt: null });
   return promise;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts` around
lines 508 - 538, The cache currently skips lookup when bypassCache is true and
stamps fetchedAt before the GitHub call settles, causing duplicate GraphQL
requests; update getCachedRepoPullRequests to always check repoPullRequestCache
for an existing entry and, if an entry exists whose promise is still pending,
return that promise even when options.bypassCache is true, but only bypass a
settled (non-pending) cached result when bypassCache is true; also avoid setting
fetchedAt before the fetch resolves—set fetchedAt when the promise fulfills
(e.g., in promise.then) so TTL is based on the completion time, while keeping
the existing promise.catch eviction logic that deletes the cacheKey if the
stored promise fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@packages/host-service/src/runtime/pull-requests/pull-requests.ts`:
- Around line 508-538: The cache currently skips lookup when bypassCache is true
and stamps fetchedAt before the GitHub call settles, causing duplicate GraphQL
requests; update getCachedRepoPullRequests to always check repoPullRequestCache
for an existing entry and, if an entry exists whose promise is still pending,
return that promise even when options.bypassCache is true, but only bypass a
settled (non-pending) cached result when bypassCache is true; also avoid setting
fetchedAt before the fetch resolves—set fetchedAt when the promise fulfills
(e.g., in promise.then) so TTL is based on the completion time, while keeping
the existing promise.catch eviction logic that deletes the cacheKey if the
stored promise fails.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6cceb4ff-2495-4bfa-b017-1c414fde3617

📥 Commits

Reviewing files that changed from the base of the PR and between 4975b41 and d1c299c.

📒 Files selected for processing (1)
  • packages/host-service/src/runtime/pull-requests/pull-requests.ts

@Kitenite Kitenite merged commit 5d285ec into main Apr 30, 2026
14 checks passed
@Kitenite Kitenite deleted the gh-cli-throttle-debug branch April 30, 2026 00:36
Kitenite added a commit that referenced this pull request Apr 30, 2026
…tion

Three changes that complete the test-suite plumbing:

1. Renamed \`test/integration/*.test.ts\` → \`*.integration.test.ts\` to match
   the repo convention used by \`v2-diff-surfaces.integration.test.ts\`,
   \`git-helpers.integration.test.ts\`, and the desktop's
   \`composer.integration.test.ts\`. Bun discovers \`*.test.ts\` recursively
   so the suffix is purely a readability signal — communicates "this
   test has external deps / is slower" to the next reader.

2. Added \`test\` and \`test:integration\` scripts to host-service/package.json.
   Without a \`test\` script, \`turbo test\` was silently skipping host-service —
   so neither this PR's new tests nor the pre-existing
   \`*.integration.test.ts\` files were running in CI.
   \`bun test --pass-with-no-tests\` matches the convention used by chat,
   workspace-fs, etc.

3. Fixed the pre-existing pull-requests.test.ts assertion that was checking
   the old \`refreshProject(projectId, true)\` signature; the procedure was
   updated in #3884 to \`{ bypassCache: true }\` but the unit test wasn't.
   This was the "1 fail" baseline I'd been carrying as known-broken.

\`turbo test --filter=@superset/host-service\` now runs all 361 tests
across 42 files in ~57s with 0 failures.
Kitenite added a commit that referenced this pull request Apr 30, 2026
* test(host-service): add integration test harness and 48 cases across 10 routers

Boots `createApp` against a `bun:sqlite`-backed drizzle db with fake providers
and drives a tRPC client over `app.fetch` (no port, no network). Covers
health, host, workspace, workspace-creation, workspace-cleanup, project,
filesystem, cloud, notifications, github, ports — including 401 paths, CORS
allowlist, and real-git fixtures for branch + status flows.

`CreateAppOptions` now accepts optional `db` and `api` overrides so the
harness can inject the bun:sqlite db (better-sqlite3 isn't loadable under
Bun; bundled-Node still uses better-sqlite3 in prod) and a fake cloud api.
`CreateAppResult.dispose()` lets tests cleanly stop the PR runtime, event
bus, and owned db.

* test(host-service): add integration tests for git, pull-requests, workspace-creation misc

Adds 18 more tests covering:
- git router: listBranches, getStatus (clean/dirty), getBaseBranch + setBaseBranch
  round-trip, renameBranch
- pullRequests router: getByWorkspaces hydration with linked PR rows,
  refreshByWorkspaces no-op
- workspace-creation misc: getContext, getProgress (in-memory store),
  generateBranchName empty/unknown-project guards

* test(host-service): add terminal, project-setup, git-history integration tests

+17 tests across three more files:
- terminal: listSessions empty, killSession NOT_FOUND for unknown
  workspace + terminal, auth required
- project.setup: clone with no/unparseable repoCloneUrl, repoint without
  allowRelocate, project.create empty/template NOT_IMPLEMENTED, idempotent
  remove for missing project
- git history: listCommits empty/with-commits, getCommitFiles, getDiff
  staged content, getBranchSyncStatus no-remote + detached HEAD

* test(host-service): add workspace create/delete + ws-auth integration tests

+9 tests across two more files:
- workspace.create + workspace.delete: success path persists worktree+row,
  cloud failure rolls back the on-disk worktree, main-by-path rejection,
  delete removes worktree + row + calls cloud, auth required
- ws auth: /events 401 without token, 401 with wrong token, accepted with
  valid token query param, /terminal/* 401 without auth

* test(host-service): add workspaceCreation.adopt integration tests

+5 tests covering: PROJECT_NOT_SETUP when project missing, no-managed-worktree
guard, NOT_FOUND for unregistered explicit worktreePath, successful adopt of
an explicit worktree path with cloud + local row creation, and recordBaseBranch
persisting `branch.<name>.base` in git config.

* test(host-service): add Octokit-mocked github + workspaceCreation tests

Adds optional \`github\` factory override to \`CreateAppOptions\` so tests can
inject a fake Octokit-shaped object instead of hitting api.github.com.

+9 tests across two files:
- github router (mocked): getPRStatus head filter, getPR pull_number,
  listPRs pagination forwarding, getRepo
- workspaceCreation github procedures: searchGitHubIssues direct \`#N\`
  lookup, free-text search fallback, repoMismatch for cross-repo URLs;
  searchPullRequests direct \`#N\` lookup + PR filter on search results

* test(host-service): expand github mocks + workspace-cleanup branch + project cloud fallback

+6 tests:
- github (mocked Octokit): listDeployments + listDeploymentStatuses param
  forwarding, getUser, mergePR with mergeMethod
- workspaceCleanup: deleteBranch=true removes the branch after worktree
  teardown
- project: findByPath cloud-fallback path when no local project + parseable
  GitHub remote

* test(host-service): add chat + auth router integration tests

Adds optional \`chatRuntime\` (ChatRuntimeManager) and \`chatService\`
(ChatService) overrides to \`CreateAppOptions\` so tests can stub the mastra-
backed runtimes that previously blocked coverage of these routers.

+16 tests:
- chat router: getDisplayState / listMessages / getSnapshot / sendMessage
  (including fire-and-forget cloud lastActiveAt update) / endSession /
  respondToApproval (zod enum guard) / getSlashCommands /
  resolveSlashCommand / auth required
- auth router: getAnthropicStatus, startAnthropicOAuth, completeOAuth,
  setAnthropicApiKey (zod min(1) guard + happy path), OpenAI status +
  setOpenAIApiKey, both disconnect endpoints, auth required

* test(host-service): add workspaceCreation create + checkout validation tests

+6 tests on the validation surface of the two heaviest workspaceCreation
procedures (full happy paths still need extensive cloud + git mocks):
- create: empty branchName / workspaceName rejected, PROJECT_NOT_SETUP for
  unknown project
- checkout: exactly-one-of branch|pr refine guard, negative PR number
  rejected at zod, PROJECT_NOT_SETUP for unknown project

* test(host-service): add bug-hunt suite — found .git/config.lock race

Adds 4 deliberately-aggressive bug-hunt files (38 tests, 1 todo) that probe
defenses rather than pin behavior:

- bug-hunt.test.ts: filesystem sandbox (writeFile/readFile/deletePath/
  movePath/createDirectory '..' traversal), git-flag injection through
  setBaseBranch/renameBranch, idempotency, auth-header parsing edge cases,
  SQL/identifier injection
- bug-hunt-2.test.ts: symlink-based sandbox escape (rejected),
  copyPath/createDirectory traversal, partial-failure consistency,
  manual worktree deletion graceful handling, detached HEAD edges
- bug-hunt-3.test.ts: branch-name path traversal (incidentally blocked
  by git's refname rules), workspace.delete legacy dirty-skip behavior,
  setBaseBranch race todo
- bug-hunt-4.test.ts: cross-project worktree adoption confusion,
  double-call cloud propagation

Found one real bug (marked test.todo so the suite stays green and the
fix removes the .todo):

  BUG: parallel git.setBaseBranch calls race on .git/config.lock and
  one returns a 500 with "error: could not lock config file .git/config:
  File exists". A renderer double-click during a slow request hits this.
  Fix: catch lock-contention errors and retry, or serialize per-workspace
  config writes.

Defenses verified to hold:
- workspace-fs realpath checks block all traversal + symlink escapes
- git's refname rules incidentally block `../` in branch names (used
  by workspace.create which doesn't validate before path.join)
- drizzle parameterizes against SQL injection
- timing-safe PSK comparison, multi-token query rejected
- cross-project worktree adoption properly rejected by git scope
- workspaceCleanup.destroy idempotent on missing rows

* test(host-service): v2 bug-hunt — found 2 progress-store leaks in workspaceCreation

Two new v2 bugs (test.todo) on top of the .git/config.lock race already
documented:

1. workspaceCreation.create leaks an 'active' progress entry when the
   project isn't set up locally. setProgress(pendingId, 'ensuring_repo')
   runs BEFORE requireLocalProject throws PROJECT_NOT_SETUP, and the
   throw path doesn't call clearProgress. Renderer shows a stuck spinner
   for up to 5 minutes (until sweepStaleProgress catches it).

2. Same pattern with whitespace-only branchName: setProgress reaches
   'creating_worktree' before the .trim() check throws BAD_REQUEST.
   clearProgress is only called inside the worktree-add try/catch
   downstream; everything in between leaks.

Same leak shape exists in workspaceCreation.checkout for any throw
between setProgress(ensuring_repo) at line 21 and requireLocalProject
at line 23 — covered by the same fix surface.

Suggested fix: wrap the procedure body in `try { ... } finally
{ clearProgress(pendingId) }` and remove the scattered clearProgress
calls. Or make setProgress/clearProgress a tRPC middleware bound to
the procedure lifecycle.

* fix(host-service): plug progress-store leak in workspaceCreation create + checkout

Wraps both procedures in a try/finally so every throw path clears the
pending-id progress entry. Previously, errors thrown between
\`setProgress(...)\` and the scattered \`clearProgress(...)\` call sites
(project-not-setup, whitespace branch name, listBranchNames /
safeResolveWorktreePath / mkdirSync failures, etc.) left a stale "active"
step that the renderer kept polling for up to 5 minutes — a stuck spinner
on the Pending Workspace page until \`sweepStaleProgress\` caught it.

The scattered \`clearProgress\` calls inside the body are now redundant
but harmless (the call is idempotent); leaving them for diff minimalism.

Flips two .todo regressions in bug-hunt-v2.test.ts to real \`test()\`
asserting \`getProgress\` returns null after each early-throw path.

* fix(host-service): retry git config writes on .git/config.lock contention

Concurrent \`git config\` writes to the same repo race on .git/config.lock
and one fails with "error: could not lock config file .git/config: File
exists". Reproduced via two parallel \`git.setBaseBranch\` calls (renderer
double-click on the base-branch picker during a slow request).

Adds \`gitConfigWrite\` helper that retries on lock contention with
exponential backoff (30/60/120/240ms, 4 attempts max). Routes the three
in-tree config writers through it:
  - git.setBaseBranch
  - workspaceCreation.create base-branch record
  - workspaceCreation.adopt recordBaseBranch

Flips the previously-todo bug-hunt-3 regression test to a real \`test()\`
asserting two parallel setBaseBranch calls converge without a 500.

* refactor(host-service tests): extract scenarios + seed + cloud-fake helpers

Pulled the four-step beforeEach (host + repo + projects.insert + workspaces.insert)
that was duplicated across 12 test files into three composable helpers:

- test/helpers/seed.ts — \`seedProject\`, \`seedWorkspace\`, \`seedTerminalSession\`,
  \`seedPullRequest\`. Tests express *what* they need rather than the drizzle
  insert shape, so future schema changes only touch this file.

- test/helpers/cloud-fakes.ts — pre-canned cloud-API factories
  (\`cloudOk.hostEnsure\`, \`cloudOk.workspaceCreate\`, \`cloudOk.workspaceDelete\`,
  \`cloudOk.workspaceGetFromHost\`, etc.) plus bundled \`cloudFlows.workspaceCreateOk\`
  / \`workspaceDeleteOk\` for whole-flow setups. Replaces 5+ inline copies of
  \`(input: unknown) => ({ id: randomUUID(), ...echo })\`.

- test/helpers/scenarios.ts — high-level lifecycle bundles:
  \`createBasicScenario\` (host + repo + project + main workspace),
  \`createFeatureWorktreeScenario\` (basic + real \`git worktree add\` + feature
  workspace row), \`createProjectScenario\` (project only). Each owns its
  \`dispose()\` so afterEach is one-line.

Migrated 13 test files. Bug-hunt suites kept as-is — single-purpose probes
read better with explicit setup. Net diff: -205 lines across migrated tests
(helpers add +412 net for the suite, paid back as the suite grows).

182 integration tests still pass. Typecheck + lint clean.

* fix(host-service): address PR review — dispose isolation, retry off-by-one, post-worktree rollback, weak test assertions

PR #3915 review feedback from coderabbitai + greptile. All actionable items
addressed.

src/:
- app.ts dispose: each cleanup step now isolated in its own try/catch so a
  throw in pullRequestRuntime.stop() can't leak the SQLite handle. Logs
  warnings instead of dropping silently.
- config-write.ts: retry counter is the number of *additional* attempts
  after the first, fixing an off-by-one (default 4 was actually 3 waits).
  Clamp to 0; initialize lastErr to a real Error so retries=0 can never
  throw undefined.
- workspace-creation/create.ts: enablePushAutoSetupRemote and the base-
  branch config write now run inside a try block that calls
  rollbackWorktree() on failure. Previously a throw in either left an
  orphaned `<repoPath>/.worktrees/<branch>` and dangling local branch
  on disk.

test/:
- createTestHost: fetchApp now builds a Request first and passes it alone
  to app.fetch — Hono's second arg is the Cloudflare-style env, NOT a
  RequestInit, so init was being silently ignored when input was already
  a Request. dispose now runs sqlite/temp-dir cleanup in a finally so
  thrown app dispose can't leak handles or leave temp dirs behind.
- smoke.test.ts + cloud.test.ts: missing `await` on `.rejects` could let
  tests pass before the assertion ran (false-positive green).
- bug-hunt.test.ts: tightened the concurrent-create assertion to
  `featureRows.length <= 1` (the actual collision we're guarding against);
  replaced `.catch(() => null)` with an explicit second NOT_FOUND
  assertion in the SQL-injection smoke.
- bug-hunt-2.test.ts: rolled back the tautological
  `errorThrown || !worktreeStillThere` check to two concrete assertions
  (throws + worktree retained), and renamed the test to "v1 does NOT
  roll back" to reflect what it actually pins. Removed the misleading
  "persistence after restart" describe block — the harness creates a
  fresh tmp dbPath per host so two hosts can never share a db, the
  test couldn't ever exercise restart.
- ws-auth.test.ts: replaced `not.toBe(401)` with
  `expect([404,426]).toContain(...)` so a future 5xx regression on
  /events fails this test instead of silently passing.

Skipped: greptile's `args` guard suggestion on gitConfigWrite (rename
suggestion, low value); the `$client` undocumented-internal note on
app.ts (already best-effort with try/catch around it after this fix).

* fix(host-service tests): address 2 new PR review findings

- scenarios.ts: removed misleading \`stableId\` export — each function
  returned a fresh \`randomUUID()\` so callers expecting determinism would
  be silently wrong. No call sites; safe to delete.

- workspace-creation-misc.test.ts: \`getProgress reflects state\` test
  wrote to the module-level progress-store map without clearing it. Now
  wraps the assertion body in try/finally with \`clearProgress(pendingId)\`
  so the entry doesn't leak across suites (the only other cleanup path
  is \`sweepStaleProgress\` on a 5 min timer).

* chore(host-service): wire tests into turbo + match repo naming convention

Three changes that complete the test-suite plumbing:

1. Renamed \`test/integration/*.test.ts\` → \`*.integration.test.ts\` to match
   the repo convention used by \`v2-diff-surfaces.integration.test.ts\`,
   \`git-helpers.integration.test.ts\`, and the desktop's
   \`composer.integration.test.ts\`. Bun discovers \`*.test.ts\` recursively
   so the suffix is purely a readability signal — communicates "this
   test has external deps / is slower" to the next reader.

2. Added \`test\` and \`test:integration\` scripts to host-service/package.json.
   Without a \`test\` script, \`turbo test\` was silently skipping host-service —
   so neither this PR's new tests nor the pre-existing
   \`*.integration.test.ts\` files were running in CI.
   \`bun test --pass-with-no-tests\` matches the convention used by chat,
   workspace-fs, etc.

3. Fixed the pre-existing pull-requests.test.ts assertion that was checking
   the old \`refreshProject(projectId, true)\` signature; the procedure was
   updated in #3884 to \`{ bypassCache: true }\` but the unit test wasn't.
   This was the "1 fail" baseline I'd been carrying as known-broken.

\`turbo test --filter=@superset/host-service\` now runs all 361 tests
across 42 files in ~57s with 0 failures.

* fix(host-service tests): drop unused biome-ignore suppressions

CI Lint flagged the noDelete suppressions as having no effect — Biome 2.4.2 doesn't fire the rule on these `delete process.env.X` calls in test code, so the comments were dead annotations.

* fix(host-service tests): address PR review — teardown guards, tighter assertions

7 valid findings from coderabbit on the latest commit (greptile re-flagged
4 already-fixed issues; ignoring those).

- 8 test files: \`afterEach\` now disposes via \`scenario?.dispose()\` (or
  \`host?.dispose()\`) so a thrown \`beforeEach\` doesn't mask the original
  error with a teardown crash. Optional chain only — types stay
  non-optional so test bodies don't need \`!\` everywhere.
- bug-hunt-v2: renamed test from "TEARDOWN_FAILED" to "destroy rejects a
  main workspace BEFORE running teardown or cloud-delete" — the body
  only verifies the phase-0 main-workspace guard, not real
  TEARDOWN_FAILED mapping (which needs a PTY-enabled harness).
- smoke CORS: \`not.toBe("http://evil.example")\` would also pass for a
  misconfigured wildcard \`*\`. Replaced with \`toBeNull()\` — Hono's CORS
  middleware omits the header entirely for non-allowlisted origins.
- workspaceCreation.adopt PROJECT_NOT_SETUP test was passing on "any
  throw"; now asserts \`{ data: { code: "PRECONDITION_FAILED" } }\`
  (the cause shape that \`projectNotSetupError\` produces).
- workspace.get / gitStatus NOT_FOUND tests now assert
  \`{ data: { code: "NOT_FOUND" } }\` instead of just \`TRPCClientError\`
  instance.

Skipped (greptile re-flags of already-fixed issues):
- app.ts:212 dispose isolation (fixed in 5812eaf)
- app.ts:209 \`$client\` undocumented (acknowledged tradeoff)
- config-write.ts:22 args guard (rename suggestion, low value)
- createTestHost.ts:102 fetchApp env (fixed in 5812eaf)
- createTestHost.ts:173 cleanup finally (fixed in 5812eaf)
- workspace-creation-misc.ts:78 progress cleanup (fixed in 705f924)

* chore(host-service tests): trim verbose comments + dead helpers

Pass over my own additions following the deslop guide:

- app.ts: collapsed 5 individual JSDoc blocks for the test-harness
  override fields into one umbrella comment + concise per-field types.
  Same for createTestHost.ts.
- scenarios.ts: removed unused \`withBasicScenario\` helper (no callers);
  trimmed restate-the-obvious field doc on \`hostOptions\`.
- cloud-fakes.ts: removed unused \`organization\`, \`userMe\`,
  \`chatUpdateSession\` factories; tightened the top JSDoc + workspaceCreate
  comment.
- fakes.ts: removed unused \`ProcedureKind\` type export.
- bug-hunt-v2: dropped a "Today: progress is non-null …" comment that
  described pre-fix behavior the test no longer demonstrates; trimmed
  meta-explanations of test design choices to focus on the
  non-obvious assertion.
- bug-hunt: pruned setBaseBranch / renameBranch test bodies to one
  intent comment each (down from 3 multi-line prose blocks).
- workspace-cleanup: tightened a "switching hosts" inline.

No behavior change. 398 tests still pass; typecheck + lint clean.
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.

1 participant