fix: surface_error failover now throws FailoverError to prevent UI hang

[Ricardo-M-L]

Summary

Fixes #64793

When an LLM request times out and the failover policy decides to surface_error, the handleAssistantFailover function previously fell through to return { action: "continue_normal" }, silently swallowing the error. This caused the WebSocket connection to abort before any final or error event could be broadcast to the UI, leaving the client spinner hanging indefinitely.

• assistant-failover.ts: surface_error decisions now return { action: "throw" } with a properly constructed FailoverError, ensuring the error propagates through the promise chain to broadcastChatError
• chat.ts: Added a terminalEventSent safety net in the .finally() handler — if neither .then() nor .catch() managed to broadcast a terminal event (e.g. due to a connection abort race), a fallback error event is emitted
• Tests: Added 9 regression tests covering timeout, billing, rate-limit, auth, generic failure, idle-timeout retry, and continue_normal preservation scenarios; extended failover-policy.test.ts with 2 timeout surface_error policy assertions

Test plan

• assistant-failover.surface-error-throws.test.ts — 9 tests pass
• failover-policy.test.ts — all assertions pass with new timeout cases
• Manual: deploy with a slow LLM provider (e.g. minimax-m2.5 via NVIDIA API), trigger timeout, verify UI displays error instead of hanging
• Verify existing retry/fallback behavior is preserved (idle timeout retry, profile rotation, model fallback)

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR fixes a silent error-swallowing bug where surface_error failover decisions in handleAssistantFailover fell through to return { action: "continue_normal" }, preventing any terminal event from reaching the UI and leaving the client spinner hanging. The fix makes surface_error return { action: "throw", error: FailoverError } — which the existing caller in run.ts already handles by re-throwing — and adds a terminalEventSent safety net in chat.ts to emit a fallback error broadcast if a connection-abort race prevented either branch from sending a terminal event.

Confidence Score: 5/5

Safe to merge — the fix is well-scoped, the caller already handled the throw action, and 9 regression tests cover the key paths.

Only finding is a P2 test name mismatch with no impact on correctness or runtime behavior. Core logic, error propagation, and the safety net are all sound.

No files require special attention; the P2 comment on failover-policy.test.ts is cosmetic only.

Prompt To Fix All With AI

This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run/failover-policy.test.ts
Line: 133-147

Comment:
**Misleading test name contradicts the fixture**

The test description says "not rotated" but the fixture sets `profileRotated: true`. In `resolveRunFailoverDecision`, `profileRotated: true` means the rotation already happened; passing `false` here would yield `rotate_profile`, not `surface_error`. The comment inside the body ("With profileRotated=true") acknowledges this but the name still misleads readers.

```suggestion
  it("surfaces error for timeout when no fallback is configured and rotation already exhausted", () => {
    // When timedOut=true and not during compaction, shouldRotateAssistant returns true.
    // With profileRotated=true and no fallback, we get surface_error.
```

How can I resolve this? If you propose a fix, please make it concise.

_{Reviews (1): Last reviewed commit: "fix: surface_error failover decision now..." | Re-trigger Greptile}

[greptile-apps]

Misleading test name contradicts the fixture

The test description says "not rotated" but the fixture sets profileRotated: true. In resolveRunFailoverDecision, profileRotated: true means the rotation already happened; passing false here would yield rotate_profile, not surface_error. The comment inside the body ("With profileRotated=true") acknowledges this but the name still misleads readers.

Suggested change

	it("surfaces error for timeout when no fallback is configured and not rotated", () => {
	// When timedOut=true and not during compaction, shouldRotateAssistant returns true.
	// With profileRotated=true and no fallback, we get surface_error.
	const decision = resolveRunFailoverDecision({
	stage: "assistant",
	aborted: false,
	fallbackConfigured: false,
	failoverFailure: false,
	failoverReason: null,
	timedOut: true,
	timedOutDuringCompaction: false,
	profileRotated: true,
	});
	expect(decision.action).toBe("surface_error");
	});
	it("surfaces error for timeout when no fallback is configured and rotation already exhausted", () => {
	// When timedOut=true and not during compaction, shouldRotateAssistant returns true.
	// With profileRotated=true and no fallback, we get surface_error.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run/failover-policy.test.ts
Line: 133-147

Comment:
**Misleading test name contradicts the fixture**

The test description says "not rotated" but the fixture sets `profileRotated: true`. In `resolveRunFailoverDecision`, `profileRotated: true` means the rotation already happened; passing `false` here would yield `rotate_profile`, not `surface_error`. The comment inside the body ("With profileRotated=true") acknowledges this but the name still misleads readers.

```suggestion
  it("surfaces error for timeout when no fallback is configured and rotation already exhausted", () => {
    // When timedOut=true and not during compaction, shouldRotateAssistant returns true.
    // With profileRotated=true and no fallback, we get surface_error.
```

How can I resolve this? If you propose a fix, please make it concise.

[steipete]

Closing this as implemented after Codex review.

Current main already covers the underlying timeout/UI-hang problem and the broader surface_error propagation path, with a more precise implementation than this PR.

What I checked:

• surface_error handling on main: Current main throws FailoverError for concrete surface_error provider failures, but intentionally keeps plain timeout and external-abort cases on the continue_normal path because those are surfaced downstream by the runner. (src/agents/pi-embedded-runner/run/assistant-failover.ts:204, fd65caf4b0ba)
• Explicit timeout payload already exists: The runner now emits an explicit timeout error payload when a timed-out run produced no assistant payloads, which directly addresses the orphaned-turn / hanging-client case this PR describes. (src/agents/pi-embedded-runner/run.ts:1821, fd65caf4b0ba)
• Mainline regression coverage: Current tests cover both sides of the superseding behavior: surfaced provider failures throw, while plain timeouts stay on the runner timeout-payload path. (src/agents/pi-embedded-runner/run/assistant-failover.test.ts:59, fd65caf4b0ba)
• Timeout path stays verified end-to-end: Current integration-style tests assert that timed-out runs with no reply return an explicit error payload containing a timeout message, rather than silently completing. (src/agents/pi-embedded-runner/run.overflow-compaction.loop.test.ts:464, fd65caf4b0ba)

So I’m closing this as already implemented rather than keeping a duplicate issue open.

Review notes: reviewed against fd65caf4b0ba; fix evidence: commit fd65caf4b0ba.