fix(session): retry an empty completion regardless of finish reason

[Yedigaryan]

Issue for this PR

Closes #37735
Refs #45315

Type of change

• Bug fix

What does this PR do?

Builds on this branch (#40531) by widening its guard so it also covers finish: "stop", not just "unknown", and bounds the empty-response retry so it can't loop forever.

This branch already tracks whether a stream generated anything (text-delta/reasoning-delta with non-zero length, tool-input-start/tool-call) and retries via SessionRetry.EmptyResponseError when finish === "unknown" && !generated. Per @bekandev's repro and production data on #37735 (4 models, all finishing stop), that guard never fires for the more common case: a provider that opens the assistant role and closes on stop without ever emitting a content delta. OpenCode currently records that as a normal successful completion — no error, no retry, the turn is just silently empty.

This PR:

• widens the condition to (finish === "unknown" || finish === "stop") && !generated
• adds !ctx.needsCompaction, since compaction deliberately interrupts the stream mid-turn and would otherwise false-positive as an empty response
• parameterizes the error message with the actual finish reason, for diagnosis
• bounds the retry: policy() had no cap on EmptyResponseError at all — it retried unconditionally, backoff climbing to the 30s ceiling and cycling forever, each attempt a real billable request. That was reachable but narrow when scoped to unknown only; widening to stop makes it reachable by ordinary, deterministic behavior (some small local models and some providers on refusal always return empty-stop). Added EMPTY_RESPONSE_MAX_RETRIES = 5 and capped the schedule to it, so an empty completion that never recovers now surfaces a terminal error instead of looping — same exhaustion mechanism retryable() already uses via Cause.done(meta.attempt).

Nothing else changes — generated tracking and the EmptyResponseError/SessionRetry.policy hookup are this branch's existing code.

Note on #37372, which a bot comment on #37735 flagged as a possible duplicate: it isn't. That issue is filed against the separate v2 branch's session runner (packages/core/src/session/runner/llm.ts), a different implementation from this repo's dev/packages/opencode/src/session/processor.ts, which is what this PR touches. This change doesn't address or close it.

How did you verify your code works?

• bun test test/session/ + test/cli/run/run-process.test.ts — 387 pass, 0 fail
• bun run typecheck — clean
• oxlint / prettier --check on touched files — 0 new warnings vs. this branch unpatched (verified via git stash A/B)
• 4 new tests: an empty stop completion retries and recovers; a stop completion with real reasoning content is correctly left alone; repeated empty stop completions exhaust EMPTY_RESPONSE_MAX_RETRIES and surface a terminal error (llm.calls capped at EMPTY_RESPONSE_MAX_RETRIES + 1); an empty stop completion whose usage alone crosses the context limit sets needsCompaction before the guard runs, and does not also count as an empty-response attempt
• Fixed a latent issue in the existing "publish retry status updates" test: it used llm.text("") as inert filler after a simulated 503, which now retries too under the widened guard and masks the 503 being measured — changed to real text (this is the only llm.text("") in the suite, per @bekandev's note on Empty stop completions are recorded as successful responses #37735)

Checklist

• I have tested my changes locally
• I have not included unrelated changes in this PR

[github-actions]

Thanks for your contribution!

This PR doesn't have a linked issue. All PRs must reference an existing issue.

Please:

1. Open an issue describing the bug/feature (if one doesn't exist)
2. Add Fixes #<number> or Closes #<number> to this PR description

See CONTRIBUTING.md for details.

[github-actions]

The following comment was made by an LLM, it may be inaccurate:

Based on my search results, I found potentially related PRs:

• PR fix(opencode): retry empty unknown responses #40531 - fix(opencode): retry empty unknown responses — This is the branch that PR fix(session): retry an empty completion regardless of finish reason #49061 builds on, mentioned in the description
• PR fix(session): retry empty unknown-finish streams instead of stopping #43881 - fix(session): retry empty unknown-finish streams instead of stopping — Directly related to empty stream retry logic
• PR fix(session): retry empty stream truncations and discard partial parts #26167 - fix(session): retry empty stream truncations and discard partial parts — Related to empty stream handling

However, none of these appear to be duplicates of PR #49061. PR #49061 is a specific extension of the retry logic to handle the stop finish reason in addition to unknown, while the other PRs address different aspects of session retry behavior or earlier iterations of the feature.

No duplicate PRs found

[holny]

Ran the processor suite locally at a6274a2 (on top of the retry-empty-response base): 18/18 pass including the two new cases, typecheck clean. The reasoning-only guard is a nice subtle test — that one would have bitten.

One thing I'd push back on before this merges, though:

The empty-response retry is unbounded. In Effect.retry(SessionRetry.policy(...)) (processor.ts:681), policy treats EmptyResponseError as always-retryable (retry.ts:189) and nothing caps the attempts — backoff climbs to the 30s ceiling and cycles forever, and every attempt is a real billable llm.stream call. With finish === "unknown" that was mostly pathological. Widening to finish === "stop" makes it reachable by much more mundane behavior: a model that deterministically replies empty-with-stop (small local models do this, some providers also emit stop on refusals) would loop indefinitely while the session sits in retry status. Suggest bounding just the empty case — e.g. intersecting the schedule with a small recurs(n) for EmptyResponseError, or tracking attempts in ctx and surfacing a terminal error after N — plus a test that pushes empty-stop responses repeatedly and asserts the calls stop at N.

Two smaller notes:

• The new !ctx.needsCompaction guard is the right call (don't burn a retry against a context that's about to compact), but no test exercises empty+stop with compaction pending — same harness could cover it.
• Scope note for reviewers: the base stack (#40531) predates the current v2 layout — packages/opencode/src/session/processor.ts and retry.ts don't exist on v2 anymore — so this lands only as part of the stack, and the gate will need re-deriving when the stack rebases.

The change to the 503 status test (empty → "after") with the comment explaining why is much appreciated, by the way.

[Yedigaryan]

The linked issue is #37735, and the PR body says Closes #37735. This targets retry-empty-response (#40531's branch) rather than dev, because that's where the guard this PR extends already lives — stacking on it means building on #40531's existing, tested code instead of reimplementing it from scratch on dev.

GitHub doesn't populate closingIssuesReferences for a PR whose base isn't the default branch, so needs:issue fires here despite the explicit reference — the same false-positive that hit #37379 on #37372, for the same reason.

Happy to retarget straight to dev if that's preferred, or leave it stacked on #40531 so the two review together. Maintainers, let me know which you'd rather have.

[Yedigaryan]

Good catch, and correct — verified against the actual code before fixing anything. policy() had no bound on EmptyResponseError at all: the ternary set retry = { message } unconditionally, never falling through to Cause.done(meta.attempt). Confirmed there's no Schedule.recurs/attempt cap composed at the Effect.retry(SessionRetry.policy({...})) call site in processor.ts either — so yes, unbounded, and widening unknown to also cover stop does make it reachable by ordinary deterministic behavior rather than just a rare gateway hiccup, exactly as you describe.

Fixed in 467fd03:

• Added EMPTY_RESPONSE_MAX_RETRIES = 5 and capped the schedule to it (your first suggested approach — tracking the count and returning Cause.done past the bound, same exhaustion path retryable() already uses).
• New test pushes EMPTY_RESPONSE_MAX_RETRIES + 1 consecutive empty-stop completions and asserts llm.calls stops there, value === "stop", and handle.message.error is surfaced.
• New test for your first smaller note: an empty stop completion whose usage alone crosses a tiny test context limit, asserting needsCompaction wins (value === "compact", llm.calls === 1, no error) and doesn't also burn an empty-response attempt.

On your scope note: agreed, and worth being explicit for reviewers — this only makes sense stacked on #40531 as-is, since processor.ts/retry.ts don't exist on v2. Whoever rebases this stack will need to re-derive the guard against whatever the v2 equivalent looks like; not something this PR can pre-solve.

PR description updated to match.

[holny]

Read the cap — that's the shape I was after. Routing the exhausted case through the same Cause.done path retryable() uses keeps one exhaustion story instead of two, and pinning llm.calls at EMPTY_RESPONSE_MAX_RETRIES + 1 in a test is exactly the assertion that was missing. The extra case for compaction winning without burning an attempt covers the second note too.

Agreed on the stack framing being the honest one — re-deriving against whatever the v2 equivalent looks like is a rebase-time problem, not something this PR can pre-solve. Nothing else from me.