fix(orchestrator): stop the probe PR resolver re-walking the whole PR tree - #377
Open
miyaontherelay wants to merge 1 commit into
Open
fix(orchestrator): stop the probe PR resolver re-walking the whole PR tree#377miyaontherelay wants to merge 1 commit into
miyaontherelay wants to merge 1 commit into
Conversation
… tree A sweep went silent for 11m53s and then reported stalled (inFlightMs 727833, missedPasses 12) while the process stayed healthy — liveHeartbeat logging throughout, log ring lossless, consecutiveFailures 0. Nothing was failing. The sweep was slow by construction, in `resolveIssuePrFromMount`, which answers "which PR belongs to this issue" by reading every mounted PR record one at a time. Four defects, all in the same walk. 1. `#resolveIssuePr` could not scope to a repository. `resolveIssuePrFromMount` has always honoured `opts.repo`, but `#resolveIssuePr`'s own opts had no `repo` field, so it forwarded `undefined` and walked EVERY configured repository — 21 in the live workspace — to find a PR that can only live in one. None of its four call sites nor the `probePrResolver` port could scope it. Adds `repo?: string` and threads the routing answer through all of them via `#probeRepoForIssue`, which reuses `dependencyRepoForIssue` — the same helper `#dependencyIsTerminalOrMerged` already uses for its own probe. Ambiguous routing still walks unscoped: narrowing on a guess would miss a PR that is really there. 2. The mount hit never populated the cache it reads. `#resolveIssuePr` reads `#probePrResolvedCache` at the top, then runs the mount walk FIRST — the common hit — and returned without ever writing it. The cache had a reader and no writer on the hot path, so the whole walk repeated per caller, per sweep, forever. Now cached on the same terms as the gh branch: same key, same TTL, same draft exclusion. 3. The walk read most pull requests twice. `githubPullRoots` returns two roots for one repository — the nested `<owner>/<repo>/pulls/` layout and the flat `<owner>__<repo>/pulls/by-id/` alias — and unions them into a Set keyed by PATH STRING, so one PR under two spellings counted twice. Deduped on the identity the path already carries via `githubPullPathParts`, which costs no read. Paths that carry no PR identity (`_index.json`, per-PR `comments/*`) are left in the walk and still read exactly as before. 4. The read loop was invisible. `listTree` is wrapped by `#listRelayfileTree` — named, timed, logged. The `readFile` per candidate ran in a bare try/catch that swallows failures into `undefined`, with no logger, counter or progress line, which is why twelve minutes of real work was indistinguishable from a hung process for three prior investigation layers. Adds progress reporting on the same cadence helper the ready-issue read loop uses, plus a `probePrMountReads` counter. Also, two things found while fixing the above: - The cache invalidation on completion deleted only the BARE issue key, never the `:open` / `:legacy` suffixed variants `#resolveIssuePr` actually writes. Every `openOnly` probe — i.e. the completion path — was never invalidated. Harmless while the mount branch wrote nothing; a live correctness bug the moment it does. Now clears the whole key family. - `#dependencyIsTerminalOrMerged` does not go through `#resolveIssuePr` (it must not fall back to gh), so it saw no cache at all, and `#terminalDependencyIdentities` memoises only the TRUE answer. A dependency that is not merged was re-walked in full for every issue declaring it, on every sweep. Adds a sweep-scoped memo for the negative answer, cleared beside the terminal set so a PR merging between sweeps is still observed. This is the path that produced the reported repro. Relationship to #374, which bounds the whole sweep: complementary, not redundant. #374 stops a wedge burning unbounded wall-clock — the seatbelt. This removes the reason the walk is expensive — the brakes. relayfile-adapters#271 would remove the walk entirely by putting `headRef` in the pull index row. NOT FIXED, deliberately: no early break on a maximal-score match. The sort is `b.score - a.score || b.prNumber - a.prNumber`, so a score-30 hit does not win until every higher-numbered candidate is known to score no better, and `readProbePrCandidate` takes `pr.number` from the payload rather than the path, so path order does not prove PR-number order. Semantics could not be shown preserved, so per the brief the dedupe ships and the early break does not. No index fast path either: `pulls/_index.json` rows carry no `headRef`, and the primary match (score 30) is a branch match, so the index cannot exclude any PR from consideration and a title hit (score 20) must never be returned while an unread branch match could outrank it. Instead the resolver now logs WHY it fell back — index absent, shape unrecognised, or present without `headRef` — so the day adapters#271 lands shows up in the logs rather than passing unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7 Session-Id: 1a0d070b-c811-486f-9a86-9e9743becdb7
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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. Comment |
Contributor
|
@coderabbitai review Requested for exact head |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/orchestrator/factory.ts">
<violation number="1" location="src/orchestrator/factory.ts:3022">
P1: When a Linear issue is routed by `keywordRules` without a matching label or project, this helper scopes every PR probe to `config.repos.default`, although dispatch creates the PR in the keyword-selected repository. Pass the actual triage decision into probe routing, or leave the probe unscoped when the route cannot be determined without it.</violation>
<violation number="2" location="src/orchestrator/factory.ts:3033">
P2: Because `repo` is now a resolution dimension, include the normalized repository in both the resolved-PR cache key and the GitHub backoff key. Otherwise a repository-route change can reuse an old repository's PR or negative backoff and make completion probe or close the wrong PR.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| * to find a PR that is really there, which is worse than a slow walk. | ||
| */ | ||
| #probeRepoForIssue(issue: LinearIssue): string | undefined { | ||
| return dependencyRepoForIssue(issue, undefined, this.#config) |
There was a problem hiding this comment.
P1: When a Linear issue is routed by keywordRules without a matching label or project, this helper scopes every PR probe to config.repos.default, although dispatch creates the PR in the keyword-selected repository. Pass the actual triage decision into probe routing, or leave the probe unscoped when the route cannot be determined without it.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/orchestrator/factory.ts, line 3022:
<comment>When a Linear issue is routed by `keywordRules` without a matching label or project, this helper scopes every PR probe to `config.repos.default`, although dispatch creates the PR in the keyword-selected repository. Pass the actual triage decision into probe routing, or leave the probe unscoped when the route cannot be determined without it.</comment>
<file context>
@@ -2993,9 +2999,29 @@ export class FactoryLoop implements Factory {
+ * to find a PR that is really there, which is worse than a slow walk.
+ */
+ #probeRepoForIssue(issue: LinearIssue): string | undefined {
+ return dependencyRepoForIssue(issue, undefined, this.#config)
+ }
+
</file context>
| openOnly?: boolean | ||
| failOnLookupError?: boolean | ||
| allowLegacyGithubBranch?: boolean | ||
| repo?: string |
There was a problem hiding this comment.
P2: Because repo is now a resolution dimension, include the normalized repository in both the resolved-PR cache key and the GitHub backoff key. Otherwise a repository-route change can reuse an old repository's PR or negative backoff and make completion probe or close the wrong PR.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/orchestrator/factory.ts, line 3033:
<comment>Because `repo` is now a resolution dimension, include the normalized repository in both the resolved-PR cache key and the GitHub backoff key. Otherwise a repository-route change can reuse an old repository's PR or negative backoff and make completion probe or close the wrong PR.</comment>
<file context>
@@ -3004,6 +3030,7 @@ export class FactoryLoop implements Factory {
openOnly?: boolean
failOnLookupError?: boolean
allowLegacyGithubBranch?: boolean
+ repo?: string
} = {},
): Promise<ResolvedIssuePr | undefined> {
</file context>
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
1 participant
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.
A sweep went silent for 11m53s and then reported stalled (
inFlightMs 727833,missedPasses 12, ratio 0.985) while the process stayed healthy — liveHeartbeat logging throughout, log ring lossless (droppedBytes 0),consecutiveFailures 0. Nothing was failing. The sweep was slow by construction, insideresolveIssuePrFromMount, which answers "which PR belongs to this issue" by reading every mounted PR record one at a time.Relationship to #374
Complementary, not redundant.
headRefin the pull index rowRebased onto
00d51ad; no conflicts.sweep-budget.test.ts(all 810 lines of #374's suite) passes alongside this change. No instrumentation collision: #374'sbudget?.assertNotExpired()and the ready-issue read-progress idiom already coexist in the same loop (factory.ts:3652-3658); A4 mirrors the logging half of that loop, so the two mechanisms sit side by side exactly as they already do upstream.The four defects
1.
#resolveIssuePrcould not scope to a repositoryresolveIssuePrFromMounthas always honouredopts.repo, but#resolveIssuePr's own opts had norepofield, so it forwardedundefinedand walked EVERY configured repository — 21 in the live workspace — to find a PR that can only live in one. None of its four call sites nor theprobePrResolverport could scope it.Adds
repo?: stringand threads the routing answer through all of them via a new#probeRepoForIssue, which reusesdependencyRepoForIssue— the same helper#dependencyIsTerminalOrMergedalready uses for its own probe. Ambiguous routing (multi-route decision, unlabelled issue) still walks unscoped: narrowing on a guess would silently miss a PR that is really there.Changed:
#probeRepoForIssue(new, factory.ts:3021), opts field at 3033, call sites at 2991, 3002, 4750, 19033, port at 1204.2. The mount hit never populated the cache it reads
#resolveIssuePrreads#probePrResolvedCacheat the top, then runs the mount walk first — the common hit — and returned without ever writing it. The cache had a reader and no writer on the hot path, so the whole walk repeated per caller, per sweep, forever.Now cached on the same terms as the gh branch below it: same key, same TTL, same draft exclusion (the reason to keep a draft uncached is a property of the PR, not of which resolver observed it).
Changed: factory.ts:3053-3065.
3. The walk read most pull requests twice
githubPullRootsreturns two roots for one repository — the nested<owner>/<repo>/pulls/layout and the flat<owner>__<repo>/pulls/by-id/alias — and unions them into aSetkeyed by path string, so one PR under two spellings counted twice. That is the2877 + 1156 = 4033in the repro for roughly 1156 actual pull requests.Deduped on the identity the path already carries, via
githubPullPathParts— no mount read, so the dedupe is free. Insertion order is preserved and the first spelling wins, which is the candidate the existing stable sort already kept when two spellings of one PR tied, so the winner does not move. Paths carrying no PR identity (_index.json, per-PRcomments/*.json) stay in the walk and are read exactly as before rather than filtered on a guess.Changed: factory.ts:20643-20674.
4. The read loop was invisible
listTreeis wrapped by#listRelayfileTree— named, timed, logged. ThereadFileper candidate ran inside a bare try/catch that swallows failures intoundefined, with no logger, no counter and no progress line. That is why twelve minutes of real work was indistinguishable from a hung process for three prior investigation layers; the observability gap is a first-class defect here, not a nice-to-have.Adds progress reporting on
#logTimedProgress— the same cadence helper the ready-issue read loop uses — plus aprobePrMountReadscounter.Changed:
#probeMountWalkProgress(new, factory.ts:5206), wired at 3050 and 9280.Two more found while fixing the above
The cache invalidation was already broken. On completion it deleted only the bare issue key, never the
:open/:legacysuffixed variants#resolveIssuePractually writes — so everyopenOnlyprobe (#openPrForIssue,#openCompletionPr, i.e. the completion path) was never invalidated at all. Harmless while the mount branch wrote nothing; a live correctness bug the moment it does. Now clears the whole key family (factory.ts:16024-16041).#dependencyIsTerminalOrMergedhad no cache at all — and it is the path that produced the repro. It callsresolveIssuePrFromMountdirectly (it must not fall back to gh), so it never saw#resolveIssuePr's cache, and#terminalDependencyIdentitiesmemoises only the TRUE answer. A dependency that is not merged was re-walked in full for every issue declaring it, on every sweep. Adds a sweep-scoped memo for the negative answer, cleared beside the terminal set so a PR merging between sweeps is still observed (factory.ts:812, 3605, 9273).What is NOT fixed, deliberately
No early break on a maximal-score match. The sort is
b.score - a.score || b.prNumber - a.prNumber, so a score-30 hit does not win until every higher-numbered candidate is known to score no better; andreadProbePrCandidatetakespr.numberfrom the payload rather than the path, so path order does not prove PR-number order. Semantics could not be shown preserved, so per the brief the dedupe ships and the early break does not.No index fast path.
pulls/_index.jsonrows carry noheadRef, and the primary match (score 30) is a branch match — so the index cannot exclude any PR from consideration, and a title hit (score 20) must never be returned as the answer while an unread branch match could outrank it. Note this is a stronger objection than "the index is the wrong shape": it holds even on a well-formed index. Instead the resolver now logs why it fell back (index-absent/index-shape-unrecognised/index-without-head-ref/index-usable), so the day adapters#271 lands shows up in the logs rather than passing unnoticed.No bounded concurrency in the read loop. It would fix wall-clock without changing read count, but it is a behavioural change to the hot path that nobody asked for; recommended as a follow-up.
On
_index.jsonshapes — evidenceVerified directly against the
relayfile-adapterscheckout ate6edb075:index-emitter.ts:112-134(buildRepoIssuesIndexFile/buildRepoPullsIndexFile) writes a bare top-level array at the canonical nested path — the shape Factory's reader accepts. Confirmed bybulk-ingest.test.ts:490, which assertspulls/_index.jsonparses to[{ id, title, updated, number, state, merged, mergedAt }].lazy.ts:161,179(eager backfill) writes{ issues: [...] }/{ pulls: [...] }— object-wrapped — to the same canonical path.bulk-writer.ts:902writes a directory manifest at the flat alias path; it contains no records.Answering the question on
#githubIssuePathsFromIndex(factory.ts:8759): it is conditionally, not universally, falling back. On mounts last written by the incremental index emitter the shape is the bare array it accepts andlabelsis present on issue rows (added for exactly this gate,index-emitter.ts:20-23), so it works. On eager-backfilled mounts it gets the{ issues: [...] }object, failsArray.isArray, and silently falls back to the tree walk. Same canonical path, two writers, so which behaviour you get depends on which writer touched it last. Not fixed here — separate lane.Red-then-green evidence
Every test pinned on read count, not wall clock, using
FakeMountClient.reads. All measured on the rebased tree (378bd95on00d51ad), each fix reverted independently:scopes the probe PR mount walk to the issue repository…#probeRepoForIssuereturnsundefinedexpected [ …(13) ] to have a length of 5 but got 13reads each probe PR record once when the same PR is mounted under both pull rootsexpected [ …(2) ] to have a length of 1 but got 2serves a repeated probe PR resolution for one issue from cache…expected [ …(4) ] to have a length of 2 but got 4The first is the O(N) vs O(N×R) assertion: 4 PRs in each of 3 configured repos, issue routed to one — 5 reads scoped, 13 unscoped.
Test results
Environment caveat, stated plainly: the sandbox has no access to the private npm registry, so
npm cicould not install this branch's dependency tree. Tests ran against an overlay of the nearest locally-available packages. Two consequences, both verified rather than assumed:src/node/factory-persona-card.test.tsfails 7/7 — it needs@relaycast/a2a@^6.2.0and only1.1.7was available locally. Confirmed pre-existing: identical 7 failures with my changes stashed on pristinemain.tsc --noEmitreports 225 errors repo-wide from the same version skew (e.g.@relayfile/sdkmissing exports,Promise.withResolversneeding a newer lib). Zero of them are insrc/orchestrator/factory.ts.Scope
Touches only
src/orchestrator/factory.tsandsrc/orchestrator/factory.test.ts, per the gate.🤖 Generated with Claude Code