Orchestrator hang: fresh-task retry + richer error diagnostics

[stevegaossou]

What

Adds a fresh-task retry to the orchestrator so a server-side stuck task gets a second shot instead of degrading straight to post-all.

Why

On the benchmark runs, #240 (calico#12960) hung the orchestrator at its poll cap twice — yet its input was the smallest of the three (25 comments vs #241's 42, which completed in ~2.5 min). So it's not payload size — it's a task getting stuck server-side. A healthy orchestrator dedups in ~2–3 min, so a long poll means stuck, not slow.

How

• reviewWithAgent gains a maxPollMs param (defaults to MAX_POLL_MS; personas unchanged).
• orchestrateDedup makes up to ORCH_ATTEMPTS (2) attempts, each a fresh task (distinct msgId) bounded by ORCH_POLL_MS (7 min) — a fresh submission usually doesn't reproduce a hang. Two 7-min attempts fit comfortably inside the 30-min job timeout alongside the persona phase + posting.
• Still lossless: if all attempts produce nothing, post everything unfiltered (graceful degradation preserved).

Validated

Mock-tested: no-retry-needed, attempt-1-fails→retry-succeeds→dedup-applied, and both-fail→graceful-post-all — all pass.

Note: if a task is systematically stuck on a given input (vs. transiently), the retry won't fix it — that's a kagent-side investigation. This just recovers the common transient-hang case automatically.

🤖 Generated with Claude Code

[github-actions]

🔎 Council of Claudes — Correctness

Note

Correctness lens · bugs · completeness · concurrency · edge cases

No correctness issues found

The retry logic and enhanced error diagnostics look consistent with the stated intent. The changes preserve existing behavior for persona reviews (default poll window), add bounded retries for the orchestrator with distinct msgIds per attempt and per GitHub rerun, and improve observability of HTTP and fetch/undici errors. Poll-loop metrics and last-state reporting will aid diagnosing timeouts without altering control flow. Error bodies are safely truncated; error logging uses safe(errDetail(e)) in catch paths. Overall, the behavior remains lossless on failure paths (fall back to posting all comments).

If anything to double-check outside the diff: ensure MAX_POLL_MS and POLL_INTERVAL_MS remain defined as before, and that ORCH_POLL_MS × ORCH_ATTEMPTS plus persona processing fits your CI job timeout budget in practice.

No line-specific findings.

_{🤖 Council of Claudes · 0 inline comment(s)}

[github-actions]

🧪 Council of Claudes — Maintainability & Tests

Tip

Maintainability & Tests lens · simplicity · tests · docs · idioms

Well-documented incremental change with better diagnostics and a sensible retry strategy, but it introduces new logic without automated tests and has a couple of small maintainability nits (magic numbers, minor duplication, testability seams).

• Tests: This adds non-trivial orchestration logic (fresh-task retry, bounded polling, richer error surfacing). Per repo conventions, please add unit tests that exercise the new paths: first-attempt fails then retry succeeds; both attempts fail (graceful post-all); unparseable response triggers retry; poll timeout path with lastState/poll counters; submit retry backoff. The easiest route is to add lightweight Node unit tests (node:test or jest) and create seams for dependency injection (see line-specific notes).
• Configurability: Consider allowing env-var overrides for ORCH_ATTEMPTS and ORCH_POLL_MS so you can tune behavior in CI without code changes.
• Minor duplication: run key/msgId composition logic is duplicated for orchestrator and personas; a tiny helper would reduce drift risk.
• Comments are strong and focused; keep the “here be dragons” rationale where you’ve added bounded polling and retry comments — that context will save time later.

_{🤖 Council of Claudes · 8 inline comment(s)}

[github-actions]

🛡️ Council of Claudes — Security

Caution

Security lens · validation · secrets · authz · isolation

2 potential security issues. General notes:

• Ensure secrets are always masked in logs. When loading ORCHESTRATOR_AGENT_TOKEN (and any persona tokens), call core.setSecret(token) so any accidental echo is redacted in GitHub logs. GitHub auto-masking only applies to “secrets” values verbatim; explicit masking is a defense-in-depth safeguard.
• Transmit tokens only over TLS. The orchestrator and persona endpoints should be HTTPS-only; reject or warn loudly on plaintext http to avoid bearer token exposure over the network.

_{🤖 Council of Claudes · 2 inline comment(s)}

[github-actions]

Council of Claudes — Nell

Important

Nell lens · simulated reviewer · simplicity · naming · error handling · keep useful comments

1 design-level concern about error propagation; a few small nits and observability tweaks

• Overall this is a clear step up in robustness and diagnosability — the fresh-task retry plus errDetail and better rpc errors should help us triage the “stuck task” case. Thanks.
• I think we’re still logging-and-swallowing a fair number of errors by returning null from reviewWithAgent. Would it be better to propagate an error (or a structured result with a reason) so the caller can differentiate timeout vs submit failure vs task failure? That would let us choose different behaviours per case (e.g. retry only on timeout/hang).
• nit: Should ORCH_ATTEMPTS / ORCH_POLL_MS be env-tunable for emergency tuning on CI timeouts?
• OK to include a 200-char body snippet from the gateway in logs? Just checking we don’t risk leaking anything sensitive in provider error pages. Limiting to 200 chars helps, so happy to leave as-is if you’ve validated the responses.
• nit: We sleep before the first poll; do we want to poll once immediately after submit and then back off?

_{🤖 Council of Claudes · 5 inline comment(s)}

[Copilot]

Pull request overview

This PR improves the Council of Claudes GitHub Actions “orchestrator” step by allowing a bounded poll window per orchestrator attempt and retrying once with a fresh task ID when the orchestrator task appears to hang server-side. This preserves the existing “lossless” degradation path (post everything unfiltered) while reducing the chance that a transiently stuck orchestrator task disables deduplication for the whole run.

Changes:

• Add a maxPollMs parameter to reviewWithAgent to bound polling time per request (defaulting to the existing MAX_POLL_MS).
• Introduce orchestrator retry behavior: up to ORCH_ATTEMPTS (2) fresh submissions, each polled up to ORCH_POLL_MS (7 minutes), before falling back to posting all comments unfiltered.

[github-actions]

Council of Claudes — Casey

Warning

Casey lens · simulated reviewer · testing discipline · API design · simplicity · robustness

1 potential correctness issue; a few robustness/logging nits and one test/ops question

• I think the overall approach makes sense — fresh-task retry keyed off RUN_ATTEMPT and a bounded poll window feels like the right trade-off, and the richer error context via errDetail() will really help when we’re staring at CI logs. Good call on including statusText and a short body snippet, and on threading RUN_ATTEMPT into msgIds for both personas and the orchestrator.
• I am a little bit skeptical of the unconditional per-poll error logging — if the network is flapping, this could get pretty noisy. We typically try to aggregate and log once at the end (which you’re already doing) and keep the loop itself at debug or rate-limited. WDYT?
• Do we know for certain which Node version executes this script in our CI? AbortSignal.timeout() needs Node 18+. If this runs under actions/github-script v7 (Node 20) we’re fine — just want to sanity check we don’t have any older pin somewhere.
• I wonder if we want to include the failure reason from the task (if present) when the server marks it failed/canceled/rejected — that’d save a round-trip digging later.
• Maybe minor, but I think we should consider deriving the per-attempt budget from the remaining job time (or MAX_POLL_MS) rather than a hard-coded 7m so we don’t accidentally shove a long persona phase + two 7m orchestrator attempts past the 30m wall. Not blocking, though.
• One more thought: if the tasks API supports a cancel endpoint, should we cancel the stuck first attempt before re-submitting the second? Not strictly required, but it could reduce server load if “stuck” means “will never finish.” If that’s not available or is painful to plumb, fine to leave as-is.

_{🤖 Council of Claudes · 7 inline comment(s)}

[github-actions]

🧪 Maintainability & Tests — The new maxPollMs parameter is a good addition. To improve readability and IDE hints, consider adding a brief JSDoc block documenting the parameter types and behavior, and link it to the orchestrator usage. Also, within this function, the submit-retry loop uses hard-coded values (3 attempts and a 5s incremental backoff) across multiple lines — extracting these into named constants will reduce drift and make logs self-consistent.

Example:

// Tunables for submit retry behavior.
const SUBMIT_ATTEMPTS = 3;
const SUBMIT_BACKOFF_MS = 5000;

// ...
for (let attempt = 1; attempt <= SUBMIT_ATTEMPTS; attempt++) { /* ... */ }
if (attempt < SUBMIT_ATTEMPTS) await sleep(attempt * SUBMIT_BACKOFF_MS);

And reference SUBMIT_ATTEMPTS in the “could not submit after … attempts” log.

[github-actions]

🧪 Maintainability & Tests — The log string hard-codes “3 attempts”, which can drift if the loop is ever changed. Tie this to a constant (e.g., SUBMIT_ATTEMPTS) and reuse it in both the loop guard and the message to keep them in sync.

[github-actions]

🧪 Maintainability & Tests — The incremental backoff uses a hard-coded 5000ms step. Extract to a named constant (e.g., SUBMIT_BACKOFF_MS) with a short comment about why that value is chosen, to make future tuning clearer and avoid hidden magic numbers.

[github-actions]

🧪 Maintainability & Tests — ORCH_ATTEMPTS and ORCH_POLL_MS are good abstractions. For maintainability and operational tuning without code changes, consider allowing environment overrides with sane defaults, e.g.:

const ORCH_ATTEMPTS = Number(process.env.ORCHESTRATOR_ATTEMPTS || 2);
const ORCH_POLL_MS = Number(process.env.ORCHESTRATOR_POLL_MS || 7 * 60 * 1000);

Document these env vars in a header comment.

[github-actions]

🧪 Maintainability & Tests — The fresh-task retry loop in orchestrateDedup is the main new behavior and should be unit tested. To make that straightforward, add a small dependency-injection seam so tests can stub the agent call without touching network/RPC. For example, accept a reviewFn parameter (defaulting to reviewWithAgent), or export reviewWithAgent and orchestrateDedup from a module where tests can inject a stub. That enables tests for: no response then success; unparseable then success; both attempts fail; poll timeout path.

[github-actions]

🧪 Maintainability & Tests — Run key/msgId construction includes RUN_ATTEMPT here, and a similar runKey is built inside orchestrateDedup. Factor this into a tiny helper (e.g., getRunKey() or buildMsgId(prefix, parts...)) to avoid duplication and keep the shape consistent across personas and the orchestrator.

[github-actions]

🧪 Maintainability & Tests — errDetail is useful for surfacing undici’s cause. For future debugging, consider optionally including e.stack when an env var (e.g., DEBUG_ERRORS=1) is set; this keeps default logs terse while making it easy to toggle deeper diagnostics when investigating intermittent failures:

function errDetail(e) {
  const c = e?.cause;
  const cause = c ? ` | cause: ${c.code || c.message || String(c)}` : '';
  const stack = process.env.DEBUG_ERRORS ? ` | stack: ${e?.stack || ''}` : '';
  return `${e?.message || e}${cause}${stack}`;
}

[github-actions]

🧪 Maintainability & Tests — The timeout warning now includes last state and poll counters, which is great. Consider also logging the taskId once at submission success (redacting if needed) so that timeouts and failures can be correlated with server-side logs. A single core.info at submission with taskId would help later investigations without cluttering every poll.

[github-actions]

🛡️ Security — The fetch call sends an Authorization: Bearer token to whatever URL is configured. If ORCHESTRATOR_AGENT_URL (or persona URLs) are accidentally set to http://, the bearer token will traverse the network in plaintext, risking credential exposure or interception (especially in shared CI runners or through proxies).
Mitigation:

• Enforce HTTPS before issuing requests. For example:
  • Validate at the call site (or in rpc): if (!/^https:///i.test(url)) { core.setFailed('Refusing to send token to non-HTTPS URL'); return; }
  • Optionally allow http only when an explicit “I know what I’m doing” flag is set in a trusted CI environment without tokens.

[github-actions]

🛡️ Security — Including a snippet of the HTTP response body in error messages improves diagnostics but can leak sensitive server-side information (stack traces, internal hostnames/paths, or echoed request fragments) into public CI logs. On public PRs, this can reveal internal details of the orchestrator or upstream provider.
Mitigation:

• Keep the truncation (already 200 chars) but consider gating body-snippet logging behind a DEBUG flag or redact obvious sensitive patterns (e.g., Authorization:, Bearer , AWS keys).
• Avoid including body snippets for 401/403/407 responses, and optionally for 5xx in public repos, or hash the snippet for correlation without content exposure.

[github-actions]

Nell — Returning null here means we lose the reason and just treat all failures the same. Return error here? Even a minimal change like returning a tagged outcome (e.g. {text, err, reason: 'timeout'|'submit'|'task-failed'|'parse'}) would let callers decide when to retry vs degrade.

[github-actions]

Nell — OK to ignore read errors on the body snippet? I think that’s fine, but do we want to log if reading fails (for diagnosability), or is silence preferable here?

[github-actions]

Nell — Would it help triage to include the JSON-RPC method in the error? That makes it clear whether this was tasks/new vs tasks/get.

Suggested change

	throw new Error(`http ${r.status} ${r.statusText}${snippet ? ` — ${snippet}` : ''}`);
	throw new Error(`http ${r.status} ${r.statusText}${snippet ? ` — ${snippet}` : ''} (method: ${body?.method || '?'})`);

[github-actions]

Nell — nit: We sleep before the first poll. Would it be neater to poll immediately once (t=0) and then sleep between subsequent polls?

[github-actions]

Nell — nit: Make ORCH_ATTEMPTS / ORCH_POLL_MS overridable via env (with sensible defaults)? Might be handy to tune in CI if we hit edge cases, but definitely not a blocking concern.

[github-actions]

Casey — I think we should surface any server-provided reason on terminal failure. Right now we only log “task failed/canceled/rejected”.
Suggestion:

Suggested change

	if (state === 'failed' || state === 'canceled' || state === 'rejected') {
	if (state === 'failed' || state === 'canceled' || state === 'rejected') {
	const reason = task?.status?.reason || task?.status?.error || task?.status?.message;
	core.warning(`${title}: task ${state}${reason ? ` — ${reason}` : ''}`);
	return null;
	}

That’ll make failures more diagnosable without another repro.

[github-actions]

Casey — I am a little bit skeptical of logging every poll error at info — this could spam the log under intermittent network issues. Maybe bump to debug or rate-limit (e.g., only every Nth occurrence) and rely on the final “(X polls ok, Y poll errors)” line for summary?

Suggested change

	core.info(` ${title}: poll error (${safe(errDetail(e))}), will retry`);
	core.debug(` ${title}: poll error (${safe(errDetail(e))}), will retry`);

No big deal though if we want to keep it at info.

[github-actions]

Casey — nit: It might be helpful to include the taskId in the timeout warning so we can correlate with server-side logs, especially when we have multiple concurrent jobs.

Suggested change

	core.warning(`${title}: task did not complete within ${maxPollMs / 1000}s ` +
	core.warning(`${title}: task ${taskId} did not complete within ${maxPollMs / 1000}s (last state: ${lastState}, ${polls} polls ok, ${pollErrs} poll errors)`);

[github-actions]

Casey — I wonder if we should compute ORCH_POLL_MS based on the remaining job budget (or tie it to MAX_POLL_MS) rather than a fixed 7m. If personas run long, two fixed 7m windows could still bump into the 30m job limit. Not a blocker — just raising it — but we could do something like “min(remaining_time - safety_margin, 7m)” per attempt.

[github-actions]

Casey — Do we want to consider canceling the previous attempt’s task before resubmitting a fresh one? Right now we create a new task after the prior attempt times out and the old one keeps running (and may never finish). If the API exposes a cancel, we could plumb it through reviewWithAgent (e.g., return the taskId so a caller can cancel on retry), but I could be swayed either way if that’s not available or adds too much complexity.

[github-actions]

Casey — nit: Since we now build msgIds in a couple places (personas and orchestrator), would it be worth a small helper like buildMsgId(prefix, keyParts...) to keep the RUN_ID/RUN_ATTEMPT pattern consistent and avoid drift? Fine to leave as-is though.

[github-actions]

Casey — Minor thought: r.text() reads the entire body before slicing to 200 chars. Most error bodies are small, so probably fine, but if you’re worried about a huge gateway payload this could allocate more than necessary. No action required — just calling it out; the whitespace collapse + slice already helps.