fix(mothership): pass typed fork refusals through and stop overlapping retries by waleedlatif1 · Pull Request #8564 · simstudioai/sim

waleedlatif1 · 2026-10-02T08:21:34Z

Summary

Problem: forking a chat calls the Mothership worker's /api/chats/fork. copyWorkerConversation turned every non-2xx into a generic Error, so the route answered 500 even when the worker refused for a reason the caller can act on: the chat is gone (404), the selected response hasn't finished (409), or the conversation is too long to fork (413).
Root cause of the overlap: the retry loop's own catch swallowed the first attempt's thrown error, so a 400, a 500, a malformed receipt and a 15 s timeout were all retried. A slow fork that hit the 15 s per-attempt timeout was sent again while the worker was still copying the first one.
Fix:
- Typed refusals: worker 404 → OrchestrationError('not_found'), 409 → conflict, 413 → payload_too_large. The route passes those through as 404/409/413 with a caller-facing message. The existing not_found/forbidden → 404 and validation → 400 mappings are unchanged.
- Retries: only a recognized socket failure (isRetryableNetworkError: a refused, reset, dropped or unreachable connection anywhere in the cause chain) and 502/503/504 get one more attempt. EHOSTUNREACH joins the shared classifier's codes, because it is a connection that never opened. Everything else, including a timed-out attempt, fails at once. A timeout may mean the worker is still copying.
- Timeout: the per-attempt timeout is 45 s. The worker's 30 s budget bounds each statement, not the copy, so a large fork can outlast an attempt. The worker PR rolls a copy back once its caller's connection closes, so a timed-out attempt leaves no unpublished conversation behind.
- Message shown: the fork action now toasts the refusal's own message for 409 and 413 instead of "Failed to fork chat", so the person sees "has not finished" or "too long to fork". Other failures keep the generic message.
- Error body: the status is captured before the unread error body is released, and the release is best effort, so a cancel failure can't retry or hide a refusal.
Worker side: the worker PR that adds the 413 ceiling, the 30 s statement budget and rollback on caller disconnect is simstudioai/mothership#575. This change is safe without it, because a worker that never returns 413 is still mapped correctly.

Behaviour changes

POST /api/mothership/chats/[chatId]/fork can now return 409 and 413, besides the existing 400/401/404/500. The body shape is the same { error } used by its other error responses. The success response and the route contract are unchanged.
A worker 500, a malformed receipt or a timed-out attempt is no longer retried.

Type of Change

Bug fix

Testing

Red on origin/staging (9 tests), each green now:
- fork-worker.test.ts: 404/409/413 classified with one request, 500/400 not repeated, a timed-out attempt not repeated, and a 404 whose body fails to cancel still classified (added in review). These tests drive a fake worker that records the fork requests it receives and assert on those, not on mock calls.
- fork/route.test.ts: worker 404/409/413 passed through as the response status.
Mutations, each reverted on its own:
- Retrying 500 makes the "does not repeat 500" test red.
- Retrying any thrown error makes the timeout test red.
- Dropping the route pass-through makes the route 409/413 tests red.
- Dropping the 404 classification makes the worker and route 404 tests red.
- An unguarded body cancel makes the cancel test red.
- Retrying any TypeError makes the non-socket TypeError test red.
- Dropping EHOSTUNREACH makes its retry test red.
Existing test changed: "refuses an unconfirmed copy" now expects one attempt for a malformed or mismatched receipt and two for an unreachable worker.
Follow-up round: the toast and comment change was verified with type-check, bun run lint, check:api-validation:strict and check:audits (58), all passing. It is a rendered-message change, so it has no unit test.
vitest run lib/mothership/chat app/api/mothership: 44 files, 573 tests pass.
bun run lint, apps/sim type-check, bun run check:audits (57 audits) and bun run check:api-validation:strict all pass.

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing (new tests pass the test-audit authoring gate)
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…g retries

vercel · 2026-10-02T08:21:38Z

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
docs	Ready	Preview	Oct 2, 2026 6:02pm UTC

waleedlatif1 · 2026-10-02T08:21:42Z

@greptile

waleedlatif1 · 2026-10-02T08:21:43Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-02T08:21:48Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

No issues found across 4 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Re-trigger cubic}

greptile-apps · 2026-10-02T08:25:04Z

Confidence Score: 5/5

[Medium risk] Refines fork operation error handling and retry logic.

The PR appears safe to merge; no outstanding finding or new actionable issue remains.

Summary

The PR passes worker fork refusals through as actionable API responses and limits retries to connection and gateway failures.

The follow-up change displays 409/413 refusal messages in the fork UI.
Both previous findings are no longer outstanding.

_{Reviews (5) · Last reviewed commit: "fix(mothership): show a fork refusal's o..."}

cubic-dev-ai

All reported issues were addressed across 4 files

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

…cancel

waleedlatif1 · 2026-10-02T08:31:36Z

@greptile

waleedlatif1 · 2026-10-02T08:31:37Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-02T08:31:44Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

All reported issues were addressed across 4 files

_{Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic}

waleedlatif1 · 2026-10-02T08:37:23Z

@greptile

waleedlatif1 · 2026-10-02T08:37:24Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-02T08:37:28Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

No issues found across 4 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Re-trigger cubic}

waleedlatif1 · 2026-10-02T08:43:36Z

@greptile

waleedlatif1 · 2026-10-02T08:43:37Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-02T08:43:44Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

No issues found across 5 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Re-trigger cubic}

The route passes a worker 409 or 413 through with a message the person can act on, but the fork action replaced every failure with "Failed to fork chat". It now shows the refusal's message for those two statuses. The per-attempt timeout's comment no longer claims it sits above a 30 s worker budget: that budget bounds each statement, not the copy. A timed-out attempt leaves nothing behind because the worker rolls back a copy whose caller has gone (mothership#575).

waleedlatif1 · 2026-10-02T18:00:22Z

@greptile

waleedlatif1 · 2026-10-02T18:00:23Z

@cubic-dev-ai review this PR

cubic-dev-ai · 2026-10-02T18:00:29Z

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

No issues found across 6 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

_{Re-trigger cubic}

waleedlatif1 · 2026-10-02T19:20:19Z

Closing in favour of one consolidated fork PR (paired with the worker change) that carries these fixes together with the remaining fork gaps.

fix(mothership): pass typed fork refusals through and stop overlappin…

7f42068

…g retries

vercel Bot temporarily deployed to Preview October 2, 2026 08:21 Inactive