-
Notifications
You must be signed in to change notification settings - Fork 2.7k
perf(cli): cache GitHub PR list in the daemon route with a 60s TTL #7705
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ import type { Application } from 'express'; | |
| import { | ||
| GITHUB_PR_ERROR_MESSAGE_MAX, | ||
| fetchGitHubPullRequests, | ||
| type FetchGitHubPullRequestsResult, | ||
| } from '@qwen-code/qwen-code-core'; | ||
| import type { SendBridgeError } from '../server/error-response.js'; | ||
| import type { WorkspaceRegistry } from '../workspace-registry.js'; | ||
|
|
@@ -17,6 +18,8 @@ import { | |
| } from '../workspace-route-runtime.js'; | ||
| import { applyReadHeaders } from './workspace-file-read.js'; | ||
|
|
||
| const DEFAULT_CACHE_TTL_MS = 60_000; | ||
|
|
||
| function sanitizeMessage(message: string, workspaceCwd: string): string { | ||
| return message.split(workspaceCwd).join('<workspace>'); | ||
| } | ||
|
|
@@ -26,8 +29,54 @@ export function registerWorkspaceQualifiedGitHubPrsRoutes( | |
| deps: { | ||
| workspaceRegistry: WorkspaceRegistry; | ||
| sendBridgeError: SendBridgeError; | ||
| /** Coalescing/refresh window for the cached PR list. Defaults to 60s. */ | ||
| cacheTtlMs?: number; | ||
| }, | ||
| ): void { | ||
| const ttlMs = deps.cacheTtlMs ?? DEFAULT_CACHE_TTL_MS; | ||
|
|
||
| // Closure-scoped, per-workspace PR cache (one daemon per process). `gh pr | ||
| // list` with CI rollup is the slow part (multi-second GitHub round-trips), | ||
| // and the panel is glanceable, so a short TTL turns repeat opens instant. | ||
| // A pending load is reused regardless of age, so concurrent opens share one | ||
| // `gh` spawn even when it outlives the TTL; the window starts once the load | ||
| // settles. Only `ok` results are cached — `cli_unavailable` / `failed` / | ||
| // `not_a_repo` clear the entry so the next open retries (gh may since have | ||
| // been installed or authed, or the workspace may have become a repo). | ||
| interface PrsCacheEntry { | ||
| promise: Promise<FetchGitHubPullRequestsResult>; | ||
| settledAt: number | null; | ||
| } | ||
| const cache = new Map<string, PrsCacheEntry>(); | ||
|
|
||
| const getPullRequests = ( | ||
| workspaceCwd: string, | ||
| ): Promise<FetchGitHubPullRequestsResult> => { | ||
| const now = Date.now(); | ||
| const existing = cache.get(workspaceCwd); | ||
| const fresh = | ||
| existing !== undefined && | ||
| (existing.settledAt === null || now - existing.settledAt < ttlMs); | ||
| if (!fresh) { | ||
| const entry: PrsCacheEntry = { | ||
| promise: fetchGitHubPullRequests(workspaceCwd), | ||
| settledAt: null, | ||
| }; | ||
| cache.set(workspaceCwd, entry); | ||
| entry.promise.then( | ||
| (result) => { | ||
| if (cache.get(workspaceCwd) !== entry) return; | ||
| if (result.kind === 'ok') entry.settledAt = Date.now(); | ||
| else cache.delete(workspaceCwd); | ||
| }, | ||
| () => { | ||
| if (cache.get(workspaceCwd) === entry) cache.delete(workspaceCwd); | ||
| }, | ||
|
Comment on lines
+72
to
+74
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] The promise rejection handler (this Concrete cost: a regression in the rejection handler (e.g., a refactor that drops the Consider adding a test analogous to "does not cache a failed result" but using — qwen3.7-max via Qwen Code /review |
||
| ); | ||
| } | ||
| return cache.get(workspaceCwd)!.promise; | ||
| }; | ||
|
|
||
| app.get('/workspaces/:workspace/github/prs', async (req, res) => { | ||
| const route = 'GET /workspaces/:workspace/github/prs'; | ||
| const runtime = resolveWorkspaceRuntimeFromParam( | ||
|
|
@@ -40,7 +89,7 @@ export function registerWorkspaceQualifiedGitHubPrsRoutes( | |
|
|
||
| applyReadHeaders(res); | ||
| try { | ||
| const result = await fetchGitHubPullRequests(runtime.workspaceCwd); | ||
| const result = await getPullRequests(runtime.workspaceCwd); | ||
| switch (result.kind) { | ||
| case 'ok': | ||
| res.status(200).json({ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] Per-workspace PR cache has no eviction for removed workspaces — Failure scenario: workspace removal via
removeWorkspace()does not propagate to this closure-scopedMap; orphaned entries (~few KB each) accumulate over months of add/remove cycles until daemon restart.The
WorkspaceRegistryhas no removal event mechanism, so the simplest fix is a lazy sweep: ingetPullRequests, delete entries whosesettledAtis older than a generous max-age (e.g. 1 hour) before the freshness check. Alternatively, accept the leak as negligible and document it.— qwen3.7-max via Qwen Code /review