fix(server): a draft can retry its first send after a failed bootstrap - #8226
fix(server): a draft can retry its first send after a failed bootstrap#8226shivamhwp wants to merge 2 commits into
Conversation
A new thread's first send creates the thread and starts the turn in one bootstrap. When the bootstrap fails partway (worktree prep, setup script, a dropped connection), the server rolls back with thread.delete. That is a soft delete, the draft keeps its client-minted thread id, and the retry's thread.create hit requireThreadAbsent, which treated the tombstone as a live thread: "Thread already exists and cannot be created twice", on every retry, until the draft was abandoned. requireThreadAbsent now only blocks on a live row. Each per-thread projector drops its rows for the old incarnation when it applies thread.created, so a re-created id starts with an empty timeline and per-projector replay stays deterministic. A replayed thread.deleted that a later thread.created supersedes no longer removes attachment files, since those already belong to the new incarnation. Both thread.create paths in ws.ts wait for the deletion reactor to drain first, so the old incarnation's session stop and terminal close always finish before the new thread can own those resources. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…criber The drain that gates thread.create only waited for the reactor's queue to empty, but thread.deleted reaches that queue through an asynchronous subscriber. A retry arriving between publish and enqueue could re-create the id and then have the old cleanup stop the new session. The reactor now tracks the highest event sequence its subscriber has handed on, and drain first waits for that to reach the engine's latestSequence before draining the worker. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1c06a88. Configure here.
| Stream.filter((seen) => seen >= target), | ||
| Stream.runHead, | ||
| ); | ||
| yield* worker.drain; |
There was a problem hiding this comment.
Drain misses post-snapshot deletes
Medium Severity
drain reads latestSequence once, then waits only until seenSequence reaches that snapshot. A thread.deleted that commits after that read can still be unpublished to the subscriber when drain finishes, so ws.ts can dispatch thread.create while the old incarnation’s stopSession / terminal close remains queued and later runs against the new id.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 1c06a88. Configure here.
ApprovabilityVerdict: Not approved Macroscope's review found this PR not approvable — This is a cross-cutting thread-lifecycle fix that changes projection replay, attachment cleanup, websocket ordering, and asynchronous resource cleanup. The drain implementation still has a plausible race where a deletion committed after its sequence snapshot is not awaited before the replacement thread is created. You can add or adjust custom eligibility rules. Learn more. |


Sending the first message on a new thread bootstraps
thread.createandthread.turn.starttogether. When the bootstrap fails partway (worktree prep on a repo with no commits, a setup script, a dropped connection), the server rolls back withthread.delete. That is a soft delete, and the draft keeps its client-minted thread id, so the retry'sthread.createhitrequireThreadAbsentand was refused with "Thread already exists and cannot be created twice", on every retry, until the draft was abandoned. #7664 rotates the id on the client when the server reports the deletion, which covers the common path but not an interrupted send or a failed cleanup.requireThreadAbsentnow only blocks on a live row. Each per-thread projector drops its rows for the old incarnation when it appliesthread.created, so a re-created id starts with an empty timeline and per-projector replay stays deterministic. A replayedthread.deletedthat a laterthread.createdsupersedes no longer removes attachment files, since those already belong to the new incarnation. Boththread.createpaths inws.tswait for the deletion reactor to drain first, so the old incarnation's session stop and terminal close always finish before the new thread can own those resources.Closes #4647. Closes #5721.
Verification: server suites 452/452, typecheck, lint. Each new test fails with its fix reverted. Live app with client id rotation disabled: nine same-id retries against a repo with no commits, zero invariant errors, clean projection. Full report with screenshots: https://y5pnrxd87iej.postplan.dev
Out of scope, noted for follow-up: the provider resume cursor and checkpoint git refs are not cleaned on delete (pre-existing, unreachable from this path since the failed incarnation never starts a turn). The same stale-child-rows hole exists in orchestrator V2's
ON CONFLICT DO UPDATEonthread.created.Claude Fable 5 via Claude Code.
Note
Allow draft retry after failed bootstrap by resetting projections on
thread.createdrequireThreadAbsentin commandInvariants.ts now acceptsthread.createwhen a soft-deleted thread with the same id exists, so a draft can retry its first send.thread.created, andthread.deletedonly removes attachment files when no laterthread.createdsupersedes it (checked via neweventStore.hasEventAfter).ThreadDeletionReactor.drainin ThreadDeletionReactor.ts now blocks until all publishedthread.deletedevents are consumed and the worker queue is empty.ThreadDeletionReactor.drainbefore dispatchingthread.create(including bootstrap), preventing cleanup of a prior incarnation from racing with the new one.thread.createon a previously soft-deleted thread id now silently clears all existing per-thread projection rows; callers reusing a thread id after deletion will see empty projections for the new incarnation.Macroscope summarized 1c06a88.
Note
Medium Risk
Changes core thread lifecycle (invariants, projection replay, deletion cleanup ordering, and WS dispatch) with broad test coverage but non-trivial race and stale-state edge cases.
Overview
Fixes failed first-send bootstrap rollbacks blocking retries: drafts keep their client-minted thread id after a soft
thread.delete, butthread.createused to fail on “already exists.”requireThreadAbsentnow only rejects creation when the thread row is still live (deletedAtis null). Onthread.created, per-thread projectors clear their tables for that id (messages, turns, sessions, activities, plans, pending approvals) so a re-created thread does not inherit stale projection rows. During replay,thread.deletedskips attachment cleanup whenhasEventAfterfinds a laterthread.createdfor the same id, so on-disk files for the retried thread are not wiped.ThreadDeletionReactor.drainwaits until the subscriber has seen published deletions up to the current head, then drains the worker queue.ws.tsawaits that drain before directthread.createand before bootstrapthread.create, so session stop / terminal close for the old incarnation cannot race the new thread.Extensive tests cover invariants, projection replay, engine re-create, reactor drain, and WS ordering.
Reviewed by Cursor Bugbot for commit 1c06a88. Bugbot is set up for automated code reviews on this repo. Configure here.