Skip to content

fix(review-swarm): give the lens retry budget a delay that can span a 60s backoff - #259

Merged
kjgbot merged 2 commits into
mainfrom
fix/swarm-retry-delay
Sep 10, 2026
Merged

kjgbot merged 2 commits into
mainfrom
fix/swarm-retry-delay

Conversation

@kjgbot

@kjgbot kjgbot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor

The retries were never the problem. The delay was.

workflows/review-swarm.yaml sets no errorHandling, which looks like "no retry policy configured". It is not. applyReliabilityDefaults() in the runner injects one whenever the key is absent:

applyReliabilityDefaults(config) {
  const existing = config.errorHandling;
  if (existing?.strategy === 'fail-fast' || existing?.strategy === 'continue') {
    return config;                      // early return — no defaults
  }
  ...
  return { ...config, errorHandling: {
    ...existing,
    strategy: 'retry',
    maxRetries,                                                   // DEFAULT_WORKFLOW_MAX_RETRIES = 2
    retryDelayMs: existing?.retryDelayMs ?? DEFAULT_WORKFLOW_RETRY_DELAY_MS,   // = 1000
  }};
}

So every lens has always had three attempts, one second apart — and the retry loop applies it literally (await this.delay(retryDelay)).

Why that fails against this platform

flows#256's swarm died with:

[lens-maintainability] mcp-args --register failed (exit 1):
Error: register failed: registration for 'maintainability' was rate-limited;
retry after 60s: Workspace write capacity is busy; retry with backoff
(code: workspace_busy; attempts: 1)

The server asked for 60 seconds. The step spent its entire budget in two, then reported failed after 2 retries — which reads like patience, and was not. flows#245 hit the sibling shape (database_overloaded, "Retry after the interval in the Retry-After header") with the same 1-second budget.

The change

errorHandling:
  strategy: retry
  retryDelayMs: 60000

Only the delay. maxRetries is deliberately left to the injected default of 2, so this changes how long the swarm waits, never how many times it tries.

Two things worth flagging for the reviewer:

  • strategy: retry is load-bearing. applyReliabilityDefaults early-returns on fail-fast and continue, which would strip the injected defaults and drop every lens to zero retries. Writing the seemingly-conservative value here would make the gate strictly worse.
  • Budget: worst case adds 2 × 60s per lens, against a 30m lens timeout and a 60m swarm timeout. The swarm 60m < GHA poll 65m < GHA job 75m ordering invariant is untouched, and I asserted swarm.timeoutMs is unchanged.

What this does not fix

Honestly: this addresses at most 2 of the 9 failure modes currently blocking this repo (#255 has the full table). workspace_busy and database_overloaded are retryable with a real backoff. The launch-queue deadline (#238) and sandbox-provisioning (#244) failures happen before or outside the step retry loop and are untouched by this. The recurring mount cursor_expired fault is filed separately as AgentWorkforce/relayfile#492.

I am not claiming this unblocks the gate. It removes one specific way the gate lies about having tried.

Incidental

applyReliabilityDefaults contains (existing ? DEFAULT_WORKFLOW_MAX_RETRIES : DEFAULT_WORKFLOW_MAX_RETRIES) — both branches identical. Harmless, but it reads like an intended distinction that got lost. Cloud-side, not changed here.

🤖 Generated with Claude Code

https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR


Note

Low Risk
Workflow reliability tuning only; no auth, data, or application logic changes.

Overview
Configures explicit retry backoff for the review swarm so lens steps can honor platform rate limits that ask for ~60s waits.

Adds an errorHandling block to workflows/review-swarm.yaml with strategy: retry and retryDelayMs: 60000, overriding the runner’s implicit 1s inter-attempt delay while leaving maxRetries on the runner default. Inline comments document why omitting errorHandling still meant three quick retries, why strategy: retry is required (other strategies disable injected retries), and that worst-case extra wait (~120s per lens) stays within existing swarm/GHA timeouts.

Reviewed by Cursor Bugbot for commit 39dd213. Bugbot is set up for automated code reviews on this repo. Configure here.

… 60s backoff

Setting no errorHandling does not mean "no retries". The runner's
applyReliabilityDefaults() injects strategy:retry / maxRetries:2 /
retryDelayMs:1000 whenever errorHandling is absent, so every lens has always
had three attempts one second apart.

That budget cannot survive a server asking for a longer wait. flows#256:

  [lens-maintainability] mcp-args --register failed: registration for
  'maintainability' was rate-limited; retry after 60s: Workspace write
  capacity is busy (code: workspace_busy; attempts: 1)

Three attempts across two seconds against an advertised 60s. The retries are
gone before the requested backoff begins, and the step reports "failed after 2
retries" as though it had waited.

Sets only retryDelayMs. maxRetries stays at the injected default of 2, so this
changes how long the swarm waits, never how many times it tries.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
@coderabbitai

coderabbitai Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 0a44c291-d3e6-465a-b477-bf2e2b6a8bb4


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.

@kjgbot

kjgbot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor Author

Review swarm: maintainability

No fresh transcript was produced for run b955fc6b-9895-4fad-904d-196b8136ac95 (MISSING).

@kjgbot

kjgbot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor Author

Review swarm: history

No fresh transcript was produced for run b955fc6b-9895-4fad-904d-196b8136ac95 (MISSING).

@kjgbot

kjgbot commented Sep 10, 2026 •

Copy link
Copy Markdown
Contributor Author

Review swarm: structure

No fresh transcript was produced for run b955fc6b-9895-4fad-904d-196b8136ac95 (MISSING).

@kjgbot

kjgbot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

🎯 review-swarm: FAILED (M:pass H:pass S:missing)

Lens transcripts posted as sibling comments above.

@github-actions

github-actions Bot commented Sep 10, 2026 •

Copy link
Copy Markdown

Review swarm: FAILED

  • maintainability: MISSING
  • history: MISSING
  • structure: MISSING

Cloud run: b955fc6b-9895-4fad-904d-196b8136ac95

kjgbot pushed a commit that referenced this pull request Sep 10, 2026
…on claim

#258 and #259 failed back-to-back on the same relayfile mount fault, third
occurrence. My "no mode repeats consecutively, therefore no single fix" reading
was true for the sampled window and is false now -- fixing relayfile#492
plausibly would unblock the gate. Named the deadlock: both gate-repair PRs are
blocked by the gate they repair.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
kjgbot pushed a commit that referenced this pull request Sep 10, 2026
…inutes

No review run had occurred since the promote, so the chain was unverified.
Re-ran #259's check. Still running -- and correcting my own comparison: this
branch's prior failure took ~19 min, not the 1-5 min I had generalised from a
different failure mode, so it has not yet outlived its own window.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
@kjgbot

kjgbot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

This PR cannot be validated by its own gate, by design

Worth stating explicitly for whoever reviews it, because the failing review check on this PR is not evidence about this PR.

The gate checks the swarm definition out of main, not out of the PR head:

- name: Check out immutable gate from main
  uses: actions/checkout@v4
  with:
    ref: main
    path: gate-files
    sparse-checkout: |
      workflows/review-swarm.yaml
      ...

and launches that copy:

agent-relay cloud run ../gate-files/workflows/review-swarm.yaml --sync-code --json

workflows/review-swarm.yaml is the only file this PR touches. So its own review run executes the old definition, with the 1-second retry delay, every time. The change cannot take effect for its own validation no matter how often it is re-run.

That immutability is correct and I am not proposing to change it. It exists so a PR cannot weaken the gate that judges it, which is exactly the property you want. The consequence is narrower: a change to the gate definition itself is unfalsifiable by the gate, and can only be exercised by merging.

So the decision here is a human one on the diff's merits. The check will stay red until merge regardless of whether the change is right or wrong, and re-running it produces no new information.

For what it is worth on the merits: the last review run (34440534265, 08:16Z) failed with

Step "lens-structure" failed after 2 retries:
register transport error: takeover failed:
API error (workspace_busy): Workspace write capacity is busy; retry with backoff

which is the mode this PR addresses — the injected default gives each lens three attempts one second apart against a server explicitly asking for backoff. The companion cloud-side fix is AgentWorkforce/cloud#3516, which classifies that exact string; neither is sufficient alone.

The same reasoning applies to #258, which touches .github/workflows/review-swarm.yml — though that file is taken from the PR head, so #258 is self-exercising. Only this one has the problem.

kjgbot pushed a commit that referenced this pull request Sep 10, 2026
The gate sparse-checks workflows/review-swarm.yaml out of main, which is the
only file #259 touches -- so its own run always uses the old definition and the
red check carries no information about the PR. #258 differs: its file comes
from the PR head, so it is self-exercising.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
kjgbot pushed a commit that referenced this pull request Sep 10, 2026
…step

New snapshot (sdk 11.11.0) promoted 09:13. Re-ran #259's check: zero
workspace_busy, zero cursor_expired, and the swarm ran a lens for the first
time tonight -- which exited 1. Not claiming the SDK fixed contention; one
observation is not a trend.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
…not set

The maintainability lens reviewing this PR flagged that "maxRetries stays at
the injected default of 2" is a claim about code in another repository. If the
runner default changes, or someone adds maxRetries to this block, the comment
becomes quietly wrong while the YAML stays correct.

Reworded to state the shape rather than the figure: maxRetries is deliberately
not set, the count lives in the runner, and what this change guarantees is that
it alters how long the swarm waits, never how many attempts it makes. The
current default is still mentioned, but as an illustration of the worst-case
cost rather than as a guarantee.

No behaviour change: errorHandling is still {strategy: retry, retryDelayMs:
60000} and swarm.timeoutMs is unchanged, both asserted after editing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
@kjgbot

kjgbot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Two of three lenses reviewed this PR and both passed

The gate reports red, but that is the structure lens never producing a verdict, not a verdict against the change. I recovered the run's artifacts with agent-relay cloud sync 42374345-25ba-4d31-8cc8-43c4e71f860e, which works on a terminal run and returns what the agents actually wrote:

ops/reviews/20260910-1003-pr259-maintainability.md   140 lines
ops/reviews/20260910-1005-pr259-history.md           533 lines
ops/reviews/20260910-1009-pr259-history.md           761 lines

All three end in REVIEW_PASSED. There is no pr259-structure transcript, consistent with lens-structure exiting 1 before producing one.

So the swarm is largely working now: two lenses reviewed this change in substance and passed it. That is not a full gate pass and I am not presenting it as one — the aggregate legitimately requires all three.

Acting on the maintainability review

It raised a fair point I have fixed in 39dd213:

The YAML has no maxRetries field. The claim "stays at 2" is a statement about code elsewhere. If someone later adds maxRetries: 5 to this block, the comment becomes misleading.

That is exactly right, and it is the failure mode this repo already names — a comment asserting something the code does not enforce. My wording implied the file guaranteed a count it never sets. Reworded to state the shape rather than the figure: maxRetries is deliberately unset, the count lives in the runner, and what this change guarantees is that it alters how long the swarm waits, never how many attempts it makes. The default is still mentioned, as an illustration of worst-case cost rather than as a guarantee.

Asserted after editing that errorHandling is still {strategy: retry, retryDelayMs: 60000} and swarm.timeoutMs is unchanged, so this is comment-only.

The lens also independently derived the budget arithmetic — 3 attempts x 60s = 180s per lens against a 1800s lens timeout — and reached the same conclusion I did about the ordering invariant holding. Worth noting, since it checked the number rather than taking the comment's word for it.

The other maintainability points (the output_contains verification being a liveness check only) are about pre-existing structure in the file, not this diff, and it explicitly classified them as non-blocking.

kjgbot pushed a commit that referenced this pull request Sep 10, 2026
cloud sync works on terminal runs and returned three REVIEW_PASSED transcripts
the GHA log never showed. Acted on the maintainability lens's fair catch about
my own comment (39dd213). Also filed relay#1734: sync strips +x from every file
it applies, which disarmed drain.sh and the gate scripts.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
@kjgbot

kjgbot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

One caveat on the two REVIEW_PASSED verdicts I reported above, which I found while reading the transcripts more carefully: neither lens actually had the PR diff.

The history lens says so outright — "The /tmp diff was absent... Static history review only" — and the maintainability lens gives it away through its citations, which are all line numbers in workflows/review-swarm.yaml itself (line 17, line 121, lines 129-131) rather than diff hunks. Both reviewed the working tree instead of the change.

For this particular PR that happens to be nearly equivalent, since the whole change is a comment block in that one file and reading the file shows it. But I do not want the earlier comment standing unqualified: two lenses passed a change they were not looking at through the input they were told to use.

Filed as #260, with the mechanism — fetch copies the diff to /tmp, the lenses read /tmp, and the two steps do not share a filesystem, while .review-target/pr.diff is git-added and does travel.

It does not change my read of the verdicts' substance — the maintainability review engaged closely with the actual comment text and caught a real flaw in it — but the signal is weaker than "two independent lenses reviewed the diff and passed", and I said that first without checking.

kjgbot pushed a commit that referenced this pull request Sep 10, 2026
The lens tasks read /tmp/pr-<n>.diff, which only the fetch step writes, and
steps do not share a filesystem. Both passing lenses reviewed the working tree
instead -- and passed anyway, which is the real defect. Qualified my earlier
claim on #259 accordingly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FtQSAcGDta5VH9xiZFT4sR
@kjgbot

kjgbot commented Sep 10, 2026

Copy link
Copy Markdown
Contributor Author

Correcting my previous comment on this PR, which was based on the same mistake I have just corrected on #260.

I said neither lens had the diff. The history lens did — I truncated its sentence. It reads "The /tmp diff was absent, so the supplied .review-target/pr.diff was used", and it verified that file against the repo with git diff HEAD^ HEAD -- workflows/review-swarm.yaml | cmp - .review-target/pr.diff. So it reviewed the actual change.

For the maintainability lens I still cannot tell, and this time I will say that rather than infer it: the transcript does not name its source.

So my original report stands closer to as written than my correction did: at least one lens reviewed this diff in substance and passed it, and the maintainability lens engaged closely enough with the change to catch a real flaw in my comment. I over-corrected in the direction of doubt, which is its own kind of inaccuracy.

@kjgbot
kjgbot merged commit 4dd9277 into main Sep 10, 2026
3 of 4 checks passed
@kjgbot
kjgbot deleted the fix/swarm-retry-delay branch September 10, 2026 12:42
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