Skip to content

fix(mothership): decide once whether a queued send may already be on the server - #8727

Merged
waleedlatif1 merged 5 commits into
stagingfrom
fix/mothership-handoff-send-edit
Oct 7, 2026
Merged

waleedlatif1 merged 5 commits into
stagingfrom
fix/mothership-handoff-send-edit

Conversation

@waleedlatif1

@waleedlatif1 waleedlatif1 commented Oct 7, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #8723. This closes the remaining ways a queued message the server may already hold could be edited into a second turn, and decides that question in one place instead of several.

  • One source of truth for "this id may already be on the server". Before this PR the answer was inferred separately by the store guard, a stopRequired exemption, the stored-handoff restore, and the notAdmitted/neverSent results. The exemption was wrong: a Send-now reuses a re-queued message's earlier id, which may already have reached the server.
    • startSendMessage now decides it where it chooses the id. A reused id carries its queue entry's admissionUnknown, and is unknown when the entry doesn't say. A fresh id is unsent until its POST goes out. Only the server refusing the id clears it.
    • That one value rides the withdrawal result, the stored queued-send handoff, and the queue entry. The notAdmitted/neverSent pair and the stopRequired exemption are gone.
    • The store guard reads only the flag. It fills the flag in only for writers with no say (a session saved before it existed, a send handed over from another surface).
  • A Send-now restored from its stored handoff can't be edited. After a reload the handoff comes back with the flag it was stored with, including a resumed message whose Stop hadn't settled yet.
  • A send superseded at admission is answered as a duplicate. Admission's "superseded" conflict means another attempt with the same id re-took the claim after the 60s in-progress TTL ran out, which can happen because branch, attachment and context preparation all run before admission. That attempt may admit the turn. It used to come back as a bare 409, read as a busy refusal. Admission now throws ChatSendSupersededError, and the route answers it as a 409 naming the send's own id, so the client keeps the message under that id.
  • Doc fixes: claimChatSend fails closed with a 500 (its storage is forced to Postgres), not open. The admission flag's comment notes the 1-hour claim TTL.

Type of Change

  • Bug fix

Testing

  • DOM:
    • keeps a resumed Send-now uneditable when the page reloads before its Stop settles: the reviewer's probe, end to end. It fails on this PR's previous head.
    • guards a Send-now restored from its stored handoff, for both values of the stored flag. Fails on staging.
    • keeps a send answered as a duplicate with no stream uneditable, then retries it.
  • Store: reads a reused handoff id as possibly sent unless the entry says otherwise. Fails on staging.
  • Route: answers a send superseded at admission as a duplicate naming its id. Fails against the staging route.
  • Admission: refuses to admit a send whose claim another attempt took. Fails on staging.
  • The full gate and integration tests ran on the CI runner.

Checklist

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

@vercel

vercel Bot commented Oct 7, 2026 •

Copy link
Copy Markdown

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

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Oct 7, 2026 8:19am UTC

Request Review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@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 cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 5 files

Confidence score: 5/5

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

Turn on auto-fix | Re-trigger cubic

@greptile-apps

greptile-apps Bot commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

[Medium risk] Refines deduplication logic for queued chat messages.

The PR appears safe to merge; no new blocking issues were found.

What we checked:

  • Restored sends stay protected: The test watches the queue before releasing history. Restoration inserts the entry before dispatch, and editQueuedMessage checks its saved flag.

Summary

The PR carries admissionUnknown through sends, withdrawals, stored handoffs, and queue entries. Sends superseded at admission return a duplicate response naming their own ID.

  • Since the previous review, only two DOM regression tests were added: reload during an unanswered POST and a duplicate response naming the stopped turn.
  • The previous unnumbered finding remains addressed. The route names the superseded send’s ID instead of returning an unnamed refusal.
  • No new actionable issues were found.
Diagram
%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Choose message ID] --> B{Reuse an ID?}
  B -->|Yes| C[Carry saved admissionUnknown]
  B -->|No| D[Start with admissionUnknown false]
  C --> E[Before POST: save admissionUnknown true]
  D --> E
  E --> F{Server response}
  F -->|Refuses this ID| G[Restore editable queue entry]
  F -->|Duplicate or uncertain result| H[Keep ID and block edits]
  H --> I[Check history or reattach before retry]
Loading

Reviews (5) · Last reviewed commit: "test(mothership): cover the pre-POST han..." · Reviewed by Greptile

Comment thread apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts Outdated
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@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 cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 8 files

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

Fix all with cubic | Turn on auto-fix | Re-trigger cubic

Comment thread apps/sim/lib/mothership/chat/post.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@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 cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 8 files

Confidence score: 5/5

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

Turn on auto-fix | Re-trigger cubic

@waleedlatif1 waleedlatif1 changed the title fix(mothership): guard a Send-now restored from its stored handoff against edits fix(mothership): decide once whether a queued send may already be on the server Oct 7, 2026
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@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 cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 10 files

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

Fix all with cubic | Turn on auto-fix | Re-trigger cubic

Comment thread apps/sim/stores/mothership-queue/store.ts Outdated
Comment thread apps/sim/app/workspace/[workspaceId]/home/hooks/use-chat.ts
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@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 cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 10 files

Confidence score: 5/5

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

Turn on auto-fix | Re-trigger cubic

…ainst edits

A Send-now whose Stop settled has its POST out under the id its stored
handoff carries. Restored after a reload it had no resumeUserMessageId, so
the queue guard missed it, and an edit could run as a second turn. The guard
now treats a settled handoff's id as an earlier attempt. A direct send can't
reach the never-sent branch (a pending Stop queues it), so that restore now
treats anything but a refusal as unknown. Docs: the 1-hour claim TTL, the
claim failing closed, and the superseded admission conflict classed as busy.
Admission's superseded conflict (another attempt with this id re-took the
claim) went out as a bare 409, which the client reads as a busy refusal and
marks the message editable. The claim's 60s in-progress TTL can run out
before admission, since branch, attachment and context preparation precede
it, so that attempt may admit the turn. The route now answers it as a
duplicate naming the send's id, and the client keeps the message under it.
…e server

Whether a queued message may already be a turn on the server was inferred in
several places (the store guard, a stopRequired exemption, the handoff
restore, notAdmitted/neverSent), and the stopRequired exemption was wrong: a
Send-now reuses a re-queued message's earlier id, which may have been sent.
startSendMessage now decides it where it chooses the id (a reused id carries
its entry's flag, a fresh one is unsent until its POST, a refusal clears it)
and carries that one fact on the withdrawal result, the stored handoff and
the queue entry. The store guard reads only the flag, filling it in only for
writers with no say.
…after rebase

- one helper names the id a queued entry reuses (its Stop handoff's, else the
  withdrawn send's), in the order startSendMessage sends it
- a conflict naming the send's own id is never read as a refusal
- the stored handoff records the flag as it stands, rewritten as the POST
  goes out, instead of inferring it from stopRequired
- the Stop-settlement test's handoff carries the flag the hook writes; a
  flagless legacy handoff is checked against history before it is resent
@waleedlatif1
waleedlatif1 force-pushed the fix/mothership-handoff-send-edit branch from c677e91 to 97f5c94 Compare October 7, 2026 07:43
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@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 cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 10 files

Confidence score: 5/5

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

Turn on auto-fix | Re-trigger cubic

…flict

- a fresh Send-now reloaded with its POST unanswered restores uneditable
  (red without the handoff rewrite as the POST goes out)
- a conflict naming the resent id itself is a duplicate, never a refusal
  (red without the conflictStreamId !== userMessageId check)
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@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 cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 10 files

Confidence score: 5/5

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

Turn on auto-fix | Re-trigger cubic

@waleedlatif1
waleedlatif1 merged commit c3475fe into staging Oct 7, 2026
33 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/mothership-handoff-send-edit branch October 7, 2026 16:32

This branch was previously deployed

1 inactive deployment
Preview — ef654802 Deployed Oct 7, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant