fix(dispatch): read the terminal-dispatch-failure attempt row through issueStateKey (#438) - #452
fix(dispatch): read the terminal-dispatch-failure attempt row through issueStateKey (#438)#452khaliqgant wants to merge 1 commit into
Conversation
… issueStateKey (#438) factory.ts:6122-6123 wrote the failed-dispatch attempt row via #recordDispatchFailure(decision.issue), which keys under issueStateKey(issue) — the composed Relayfile sense path for a GitHub-native ref. The very next line read it back under the raw decision.issue.key (the bare GitHub issue number), so the read always missed and terminalFailure was permanently false for GitHub issues. The two keys only coincide on the Linear surface, which is why the Linear-surface test already passed. The visible effect: once maxAttempts is reached, a GitHub-native dispatch lifecycle was saved 'retryable' and handed to #scheduleDispatchLifecycleRetry instead of being closed out 'abandoned', even though the attempt row itself (written and read consistently elsewhere via issueStateKey) was already terminal and refusing every further dispatch with "dispatch already terminal" — an unresolvable retry loop against a lifecycle that can never dispatch again. Fix: read through issueStateKey(decision.issue) so the writer and reader agree, matching every other call site in this file. grep for other write/read pairs pairing issueStateKey(...) with a bare `.issue.key` (or the reverse) across getDispatchAttempts / recordDispatchAttempt / releaseInFlight / getCanonicalState / recordCanonicalState found none — the fixed line was the only mismatch of this shape in factory.ts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0145bFsaCqLTFhDNj89CuA5U Session-Id: a044ee0a-7d0b-4861-a3ec-8fae6e56bfaa
|
@coderabbitai review Requested for exact head |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe dispatch failure path now reads attempt state with ChangesGitHub dispatch lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The dispatch-attempt lookup now uses the canonical issue state key, allowing terminal GitHub dispatch failures to transition to abandoned. Regression coverage verifies the lifecycle does not remain retryable after the terminal attempt latch is reached. Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0 files. (2 skipped: 2 too large.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
|
@codex review Requesting an explicit review — CI is green, and cubic reported "no issues found," but that's on 2 files with no per-line threads, which isn't the same as a reviewer having actually engaged with the change. Please specifically challenge:
Also grepped every Not merging — |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Candidate files ==='
fd -a -t f 'factory(\.test)?\.ts$' . | sort
printf '%s\n' '=== Changed files and diff ==='
git diff --stat HEAD^ HEAD
git diff --unified=80 HEAD^ HEAD -- \
src/orchestrator/factory.ts \
src/orchestrator/factory.test.ts \
|| true
printf '%s\n' '=== factory.ts outline: relevant symbols ==='
ast-grep outline src/orchestrator/factory.ts \
--items all \
--match 'issueStateKey|recordDispatchFailure|dispatchBlockReason|scheduleDispatchLifecycleRetry|getDispatchAttempts|recordDispatchAttempt|releaseInFlight|getCanonicalState|recordCanonicalState|getDispatchLifecycle|saveDispatchLifecycle' \
--view expanded \
|| true
printf '%s\n' '=== attempt/canonical/lifecycle call sites ==='
rg -n -C 5 \
'issueStateKey\s*\(|`#recordDispatchFailure`\s*\(|`#dispatchBlockReason`\s*\(|`#scheduleDispatchLifecycleRetry`\s*\(|getDispatchAttempts\s*\(|recordDispatchAttempt\s*\(|releaseInFlight\s*\(|getCanonicalState\s*\(|recordCanonicalState\s*\(|getDispatchLifecycle\s*\(|saveDispatchLifecycle\s*\(' \
src/orchestrator/factory.ts \
|| true
printf '%s\n' '=== target test and nearby tests ==='
rg -n -C 18 \
"closes a GitHub-native lifecycle abandoned, not retryable, once the attempt latch refuses it|backs off dispatch errors and enforces a retry gap before the bounded terminal attempt|leaves an abandoned row alone when the attempt latch already refuses the issue" \
src/orchestrator/factory.test.ts \
|| trueLength of output: 102737 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Key derivation ==='
sed -n '20845,20895p' src/orchestrator/factory.ts
printf '%s\n' '=== Failure recording and dispatch gate ==='
sed -n '10085,10170p' src/orchestrator/factory.ts
printf '%s\n' '=== Terminal failure transition and retry scheduling ==='
sed -n '6108,6215p' src/orchestrator/factory.ts
printf '%s\n' '=== Durable lifecycle retry driver ==='
sed -n '8200,8405p' src/orchestrator/factory.ts
printf '%s\n' '=== Test helper declarations and implementations ==='
rg -n -C 12 \
'class DurableSpawnFailingFleetClient\b|function githubIssuePath\b|const githubIssuePath\b|function githubIssueFile\b|const githubIssueFile\b|function issueKey\b|const issueKey\b|async runOnce\b' \
src/orchestrator/factory.test.ts \
src/orchestrator/factory.ts \
|| true
printf '%s\n' '=== Full target test ==='
sed -n '19970,20020p' src/orchestrator/factory.test.ts
printf '%s\n' '=== Named state API calls with direct bare issue-key arguments ==='
rg -n -P -C 4 \
'(getDispatchAttempts|recordDispatchAttempt|releaseInFlight|getCanonicalState|recordCanonicalState)\s*\([^,\n]+,\s*[^,\n]*\.issue\.key\b|(getDispatchAttempts|recordDispatchAttempt|releaseInFlight|getCanonicalState|recordCanonicalState)\s*\([^,\n]+,\s*issue\.key\b' \
src/orchestrator/factory.ts \
|| trueLength of output: 40950 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== GitHub issue fixture ==='
sed -n '400,470p' src/orchestrator/factory.test.ts
printf '%s\n' '=== Failing fleet fixture hierarchy ==='
rg -n -C 30 \
'class SpawnFailingFleetClient\b|class DurableSpawnFailingFleetClient\b|spawnPty failed|async spawn\b|spawn\s*\(' \
src/orchestrator/factory.test.ts \
|| true
printf '%s\n' '=== runOnce dispatch-error handling ==='
sed -n '3230,3460p' src/orchestrator/factory.ts
rg -n -C 18 'await this\.dispatch\s*\(|dispatch\(decision|dispatch failed|dispatchErrors' src/orchestrator/factory.ts
printf '%s\n' '=== Retryable lifecycle recovery branch ==='
sed -n '8390,8710p' src/orchestrator/factory.ts
printf '%s\n' '=== All named attempt/canonical API call arguments ==='
rg -n -P \
'(getDispatchAttempts|recordDispatchAttempt|releaseInFlight|getCanonicalState|recordCanonicalState)\s*\(' \
src/orchestrator/factory.tsLength of output: 134175
The behavior change is correct. For a GitHub-native issue, A terminal attempt latch must not use the ordinary dispatch retry path. The test pins the defect. It uses a GitHub-native path and
Before the fix, the attempt assertion still passes, but the lifecycle remains I also checked the named I did not execute the test suite in this review environment. ✅ Action performedFull review finished. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Fixes #438:
factory.ts's terminal-dispatch-failure branch wrote the failed-dispatch attempt row via#recordDispatchFailure(decision.issue), which keys underissueStateKey(issue)— the composed Relayfile sense path for a GitHub-native ref. The very next line read it back under the rawdecision.issue.key(the bare GitHub issue number) instead. The two keys only coincide on the Linear surface (issueStateKeyfalls back toissue.keythere), which is why that surface was unaffected.Effect on origin/main: once
maxAttemptswas reached for a GitHub-native issue, the durable dispatch lifecycle was saved'retryable'and handed to#scheduleDispatchLifecycleRetryinstead of being closed out'abandoned'— even though the attempt row itself (written and read consistently everywhere else viaissueStateKey) was alreadyterminal: trueand refusing every further dispatch attempt with"dispatch already terminal". The result is an unresolvable retry loop against a lifecycle that can never dispatch again.Fix: read through
issueStateKey(decision.issue)atfactory.ts:6123so the writer and reader agree — matching every other call site that touches the attempt-state store in this file.Grep for other write/read mismatches of this shape
Per the lane brief's DoD, I grepped every call site touching the attempt/canonical state store (
getDispatchAttempts,recordDispatchAttempt,releaseInFlight,getCanonicalState,recordCanonicalState) insrc/orchestrator/factory.tsfor a write keyed byissueStateKey(...)paired with a read keyed by a bare.issue.key(or the reverse). No other occurrences found — the line fixed here (factory.ts:6123, formerlydecision.issue.key) was the only mismatch of this shape. Not fixing any others in this PR since none were found; this is a report, not a scope expansion.(
getDispatchLifecycle/saveDispatchLifecyclecall sites were also inspected — all usedispatchLifecycleKey(...)or a variable derived from it consistently, so they're outside this defect's shape.)Test plan
factory.test.ts:'closes a GitHub-native lifecycle abandoned, not retryable, once the attempt latch refuses it'.1.0.'backs off dispatch errors and enforces a retry gap before the bounded terminal attempt') and the related'leaves an abandoned row alone when the attempt latch already refuses the issue'still pass after the fix.gh run list --branch lane/438-attempt-state-keyonce checks land (not--commit).Not merging —
mergePolicyisneverper the lane brief.🤖 Generated with Claude Code
https://claude.ai/code/session_0145bFsaCqLTFhDNj89CuA5U
Summary by cubic
Fixes a bug where the terminal-dispatch-failure branch read the attempt row under a different key than it wrote, causing GitHub-native issues to be left
'retryable'forever instead of being closed out'abandoned'.issueStateKey; the read used the rawissue.key, which only matches on the Linear surface.issueStateKeyso writer and reader agree, matching every other attempt-state call site.Written for commit 6c3fba7. Summary will update on new commits.