Skip to content

fix(server): show when an answer's delivery to the provider fails - #14496

Open
saphid wants to merge 4 commits into
pingdotgg:mainfrom
saphid:fix/v2-claude-lost-runtime-request
Open

saphid wants to merge 4 commits into
pingdotgg:mainfrom
saphid:fix/v2-claude-lost-runtime-request

Conversation

@saphid

@saphid saphid commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

What Changed

When a user answers a question or approval on a V2 thread, dispatch marks the request resolved before the answer is delivered to the provider. If every delivery attempt failed, nothing reported it. The question disappeared, the provider could still be waiting for an answer it never got, and the run spun with nothing to answer and no error.

Now the last failed runtime-request.respond attempt dispatches a new server-only command, runtime-request.delivery.fail. It adds a failed Answer delivery not confirmed error item to the timeline, on the request's own node: "T3 Code could not confirm that your answer reached the agent. If the run stays stuck, stop it and try again." The answer stays recorded and the run is not marked failed. The item is recorded even when the provider session was detached before the last attempt.

The request is not reopened. A failed delivery does not show whether the provider is still waiting. For example, OpenCode can accept a reply and drop its callback before the HTTP response returns, so a reopened question could be one nobody can answer. Reporting that delivery could not be confirmed is true in every case.

This follows the same pattern as checkpoint.rollback.fail: the last failed attempt tells clients instead of leaving them waiting.

Why

Seen on a Claude thread: the agent asked a question with AskUserQuestion, the user answered, and the delivery effect failed all 5 attempts within about 1.5 s with No pending Claude runtime request <id>. The Claude CLI then blocked on the tool permission callback for 22 minutes while the thread showed "Working" with no question. Only a steer, which aborted the pending tool, got it moving again.

The delivery failed because two servers were running against one data directory, so the effect ran on the server whose Claude session did not hold the pending request. That root cause is #14115, with a fix proposed in #8442. This PR does not address it. It makes a failed delivery visible, whatever the cause.

Verification

  • New RuntimeRequestDelivery.integration.test.ts drives the real orchestrator, outbox and effect worker with a fake provider that asks a live question and refuses every delivery. While retries remain there is no error item. After the fifth failed attempt there is exactly one failed transport_error item on the request's node. It carries the request node's provider thread and turn, the request is still resolved with its answer, and the run is still running. Replaying the failure command adds nothing.
  • A second case detaches the provider session before the last attempt and still gets exactly one item.
  • The first case fails when the worker's dispatch is removed. The detached case fails when the item requires a bound session.
  • Before the rebase, vp test run on RuntimeRequestDelivery.integration, RuntimeRequestService, EffectWorker, runtimeLayer, Orchestrator.control-reads, SteeringCompletion.integration, ThreadMessageIntake and ProviderEventIngestor: 8 files, 108 tests passed.
  • Rebased onto main 7812230572. vp test run on RuntimeRequestDelivery.integration, EffectWorker, RuntimeRequestService and the contracts orchestrationV2 test: 4 files, 51 tests passed. The bound and detached cases are now one it.effect.each, for main's no-test-in-loop lint rule; both pass.
  • apps/server and packages/contracts typecheck clean; lint and format clean on the changed files.
  • Not exercised in a real client. The item uses the existing error turn item type that web and mobile already render.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes (no UI code changed)
  • I included a video for animation/interaction changes (n/a)

Diagnosed, implemented and tested by Claude Opus 5.5 in Claude Code, running inside T3 Code; reviewed by GPT-6 Astra over two rounds (its first round replaced an earlier version that reopened the question).

The 2026-10-05 rebase: Claude Opus 5.5 in T3 Code (Claude Code harness). Independent review: GPT-6.1 Sol (high reasoning) in T3 Code, no actionable findings.

🤖 Generated with Claude Code

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Oct 1, 2026
Comment thread apps/server/src/orchestration-v2/Orchestrator.ts
@macroscopeapp

macroscopeapp Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 0ce95ad

Macroscope's review found this PR approvable — This is a focused server-side bug fix that reports a final runtime-answer delivery failure using the existing error timeline model while preserving the answer and normal retry behavior. The new internal command is additive, server-only, and covered by integration tests for both attached and detached sessions.

You can add or adjust custom eligibility rules. Learn more.

Comment thread apps/server/src/orchestration-v2/EffectWorker.ts Outdated
@juliusmarminge juliusmarminge added the macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews label Oct 1, 2026 — with ChatGPT Codex Connector
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 1, 2026
@juliusmarminge
juliusmarminge deleted the branch pingdotgg:main October 2, 2026 19:23
@juliusmarminge juliusmarminge added the triage:keep-open Keeps this PR open despite not necessarily passing the contribution guide fully label Oct 2, 2026
@juliusmarminge juliusmarminge reopened this Oct 2, 2026
@juliusmarminge
juliusmarminge changed the base branch from t3code/codex-turn-mapping to main October 2, 2026 20:35
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 2, 2026 20:35

Dismissing prior approval to re-evaluate 0ebe6f3

Comment thread apps/web/src/index.css
@juliusmarminge
juliusmarminge force-pushed the fix/v2-claude-lost-runtime-request branch from 0ebe6f3 to 6f5a55b Compare October 2, 2026 20:55
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 21 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: cc099333-1f69-4fa3-a379-c02983fefd2f
📥 Commits

Reviewing files that changed from the base of the PR and between 0ce95ad and 9d7fc3e.

📒 Files selected for processing (4)
  • apps/server/src/orchestration-v2/EffectWorker.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/orchestration-v2/RuntimeRequestDelivery.integration.test.ts
  • packages/contracts/src/orchestrationV2.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 331ce7a5-4554-4e67-a291-9b5dfabad13e
📥 Commits

Reviewing files that changed from the base of the PR and between cd63e55 and 0ce95ad.

📒 Files selected for processing (4)
  • apps/server/src/orchestration-v2/EffectWorker.ts
  • apps/server/src/orchestration-v2/Orchestrator.ts
  • apps/server/src/orchestration-v2/RuntimeRequestDelivery.integration.test.ts
  • packages/contracts/src/orchestrationV2.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.


📝 Walkthrough

Walkthrough

The change adds a server-only command to record failed runtime-request answer delivery. After response attempts are exhausted, the worker can dispatch the command, and the orchestrator can append a failed transport_error turn item.

Changes

Runtime request delivery failure

Layer / File(s) Summary
Define and record delivery failures
packages/contracts/src/orchestrationV2.ts, apps/server/src/orchestration-v2/Orchestrator.ts
The internal command includes command, thread, and request IDs. The orchestrator routes the command and appends a failed error turn item when the request and its node exist. It records the item for detached sessions and does nothing if either is missing.
Dispatch after exhausted response attempts
apps/server/src/orchestration-v2/EffectWorker.ts, apps/server/src/orchestration-v2/RuntimeRequestDelivery.integration.test.ts
The worker dispatches the command after a final non-interrupt failure when no retry is pending. It uses a stable command ID and logs a warning if failure recording fails. Integration tests cover bound and detached sessions, retry counts, replay, and preservation of the resolved answer.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant ProviderAdapter
  participant EffectWorker
  participant Orchestrator
  ProviderAdapter->>EffectWorker: Return response failure
  EffectWorker->>Orchestrator: Dispatch delivery-failure command after final attempt
  Orchestrator->>Orchestrator: Append failed error turn item when request and node exist
Loading

Suggested reviewers: t3dotgg, juliusmarminge

Merge Risk: ⚪ Minimal · up to 0ce95

Failed answer delivery is reported after retries are exhausted, with no identified issue requiring a fix before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 0ce95

The change adds a failure notice without granting clients new permissions or automatically resending an answer. Recovery during server or storage failures remains only partially demonstrated.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The new mutation is bounded to timeline state for the originating thread's request node. The inspected path introduces no arbitrary target-node selection, credential access or additional provider authority; wider deployment exposure is not established by this evidence.

Trust Boundaries and Controls

  • observed — The inspected external dispatch route retains the client-command boundary. Internally, the worker supplies identifiers from its persisted effect, and projection queries bind request, node and optional session context to the same thread rather than trusting a command-supplied node.

Resilience and Maintainability Implications

  • observed — Failure visibility is best effort, not an atomic delivery-and-reporting guarantee. Recording errors are logged while the exhausted response effect can become failed. Missing request/node context emits no timeline event and is rejected by the outer dispatch path. Existing process-loss reconciliation also cancels process-bound runtime responses without invoking this notice. These limitations do not establish a worsened approval boundary.

Hardening Proposals

  • proposed — If durable failure visibility is required, give reporting independent recovery ownership and test persistence failure and process interruption. Recovery should replay only the notice, never automatically redeliver an uncertain approval or reopen the request.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely states the main change: showing when answer delivery to the provider fails.
Description check ✅ Passed The description explains the problem, change, rationale, and verification results, including test coverage and limitations. It does not include the template’s explicit “Scope and approval” section or …
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 2, 2026
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 5, 2026
@saphid

saphid commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Review requested

Date (UTC) Reviewer Where
2026-10-03 Julius Discord DM

Logged so this PR shows when a maintainer was asked to review it.

@saphid
saphid force-pushed the fix/v2-claude-lost-runtime-request branch from cd63e55 to 0ce95ad Compare October 6, 2026 11:40
@macroscopeapp
macroscopeapp Bot dismissed their stale review October 6, 2026 11:40

Dismissing prior approval to re-evaluate 0ce95ad

saphid and others added 4 commits October 7, 2026 03:33
Dispatch marks a runtime request resolved before the answer is delivered.
When every delivery attempt failed, nothing reported it: the question
disappeared while the provider could still be waiting, and the run spun
with no question to answer and no error.

The last failed delivery attempt now dispatches an internal
runtime-request.delivery.fail command, which adds a failed
"Answer delivery not confirmed" error item on the request's node. The
answer stays recorded, because a failed delivery does not show whether
the provider already accepted it, and the run is not marked failed. The
user can see why the run is stuck and stop it.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
The failure item took its driver from the provider session binding, so a
session detached before the last attempt recorded nothing. The item id no
longer depends on the session, and the session only adds provenance when
it is still bound. The warning for a failed recording no longer logs the
raw cause.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Upstream now forbids declaring tests inside for loops (pingdotgg#14921).

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@saphid
saphid force-pushed the fix/v2-claude-lost-runtime-request branch from 0ce95ad to 9d7fc3e Compare October 6, 2026 16:33
@saphid

saphid commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

Review requested

Date (UTC) Reviewer Where
2026-10-06 Julius Discord DM

Logged so this PR shows when a maintainer was asked to review it.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

macroscope-review Opt PRs made by unvouched contributors in for Macroscope review. Vouched contributors auto-reviews size:L 100-499 changed lines (additions + deletions). triage:keep-open Keeps this PR open despite not necessarily passing the contribution guide fully vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants