Add retry action for stopped unaccepted requests - #3030
Conversation
|
🚨 SLOP COP 🚨 · I’m reviewing this pull request now under the SlopCop guidelines. I’m checking security, code quality and architecture (including duplication/refactoring opportunities), performance, and the stopped-request retry flow end to end before posting the consolidated review. |
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
| ): Promise<RetryTurnResponse> { | ||
| const { request, thread } = args; | ||
| const failed = requireFailedTurn(deps, { | ||
| const failed = requireRetryableTurn(deps, { |
There was a problem hiding this comment.
🚨 slopcop/review — P1: A stopped retry can become permanently stuck after a transient wait clears.
This newly admitted stopped-request path can reach attemptDispatch and create a retry row waiting on host-offline or provisioning. When recovery clears that wait to null, isOrdinaryTurnEndQueuedMessage classifies the retry as an ordinary row, so the still-active manual-stop pause prevents the automatic claim. That pause only clears after a later root turn starts, but this retry is the turn that must start it.
I reproduced this through the app: Retry created an immutable attempt-2 card on an offline host; after the host wait was cleared, the row remained queued with waitingOn: null and the original Retry action was gone. Please exempt retry payloads from the manual-stop pause (or atomically record explicit restart intent), and add a retry → transient wait → wait-clears regression.
| if ( | ||
| row.kind === "conversation" && | ||
| row.role === "user" && | ||
| row.initiator === "user" && |
There was a problem hiding this comment.
🚨 slopcop/review — P1: The UI can attach Retry to the wrong message—or omit it—because it ignores the server’s request ID.
The server permits the latest stopped, unaccepted request regardless of initiator, but this helper selects only the last pending initiator === "user" row and never compares retryableStoppedTurnRequestId. If an agent-originated request is the stalled request, an older pending user bubble can receive the button even though clicking it retries the newer agent request; with no older user bubble, no action appears.
Please carry request identity/sequence onto timeline rows and match the DTO’s exact request ID, or deliberately restrict server eligibility to user-initiated requests and document/test that contract.
| ) { | ||
| return null; | ||
| } | ||
| const pending = loadFailedTurn(db, thread.id); |
There was a problem hiding this comment.
🚨 slopcop/review — P2: This adds an unbounded event-history scan to every ordinary idle-thread detail response.
toThreadResponseFromThread now calls this helper for every idle thread. loadFailedTurn uses the legacy-compatible “latest request” query; SQLite’s plan chooses events_thread_sequence_idx (thread_id=?) and scans backward from the thread tail until it finds a request. A long completed turn therefore pays O(events since request) just to conclude there is no stopped retry, on the server event loop.
Please make the cheap, type-indexed manual-stop check happen before loading the request, or consolidate this into one index-friendly request-aware query/persisted eligibility marker.
| onForkMessage: props.onForkMessage, | ||
| onEditMessage: props.onEditMessage, | ||
| onRetryMessage: props.onRetryMessage, | ||
| retryMessagePending: props.retryMessagePending ?? false, |
There was a problem hiding this comment.
🚨 slopcop/review — P3: Toggling one retry button rerenders every loaded conversation row.
retryMessagePending lives in the shared static renderer context consumed by each ConversationRowContent. The false → true → false mutation cycle therefore invalidates all loaded user and assistant rows twice, including Markdown-heavy history when timeline windowing is off.
Please localize this pending state to the retryable row/action (for example, a narrow context or small retry-action component) so unrelated timeline rows keep their memoization.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: This PR gives people a recovery button when they manually stop a request that the AI provider never acknowledged. Clicking “Retry request” reuses the existing retry/queue machinery so the original prompt can run again and later queued messages can continue in order.
The direction is solid and the focused happy-path coverage is good, but I found two correctness gaps and two performance issues:
- P1 — retries can get permanently stuck after a transient wait. If the retry first waits on an offline host or provisioning, clearing that wait turns it into an “ordinary” queue row that is still blocked by the manual-stop pause. The pause cannot clear until a new root turn starts, while this retry is the row that needs to start it. The UI has already removed the Retry action, leaving only manual Send now as recovery.
- P1 — the UI can label the wrong message as retryable. The server returns the exact retryable request ID and allows agent-initiated requests, but the timeline ignores that ID and heuristically picks the last pending user-initiated bubble. An agent-originated stalled request can therefore produce no button or put the button under an older user message while retrying the newer request.
- P2 — idle-thread reads gain an unbounded event scan. Every idle thread detail response now loads the last request before checking for a manual stop. Because the legacy-compatible query’s SQLite plan uses the thread/sequence index, cost grows with all events after the last request even for normal completed threads.
- P3 — retry pending state invalidates every loaded conversation row. The shared row-renderer context changes twice per retry, rerendering unrelated user and Markdown-heavy assistant rows when large timelines are loaded.
Security review found no actionable vulnerability. Retry still requires a writable thread, validates the exact eligible request, re-enters attachment validation and dispatch-policy hooks, and preserves the original execution tuple.
Architecturally, I’d consolidate stopped-request eligibility and the existing manual-stop queue-pause policy into one request-aware database classifier; both currently interpret the same event history independently, which helped create the first inconsistency. I found no stale retryFailedTurn names or other significant duplication.
Verification on 567a01fdc01187085ecf925f3993883db62822f5: 20 server tests, 71 app tests, the deterministic recovery integration test, the 12-task cross-package typecheck, and git diff --check all passed. In browser QA with doobie, Retry produced the attempt-2 queue card; after simulating the offline-host wait clearing, the row remained queued with waitingOn: null and the Retry button remained absent, confirming finding 1 end to end.
The PR advanced during review to d4aaf9207; that delta only normalizes demo/mobile fixtures and bumps the Plugin SDK version, so these findings still apply.
1 participant
Human comments
What was wrong
The broad busy-turn and pending-start message-loss paths from #2370 are already covered by #2379, #2816, #2884, and #2886. One recovery gap remained: if a provider never acknowledged or started the latest dispatched request, a manual Stop could release the orphaned runtime, but the visible original request had no supported way to run again. The existing retry endpoint only admitted failed threads, so recovery required retyping the request and could strand messages parked behind it. See the investigation report.
What changed
ThreadResponseand add a destructiveRetry requestaction below the original user message. The action reuses the existing retry path, including retry IDs/attempts and duplicate-delivery safeguards.There is no database migration or new lifecycle event. The public plugin backend contract change bumps
@get-bb/plugin-sdkfrom 0.4.43 to 0.4.44.HOST_DAEMON_PROTOCOL_VERSIONremains 179 because this changes the server HTTP response and UI/SDK behavior, not any server/daemon payload, result, default, or meaning.How you verified
pnpm exec turbo run test --filter=@bb/server -- --run test/threads/turn-stopped-unaccepted-retry.test.ts test/threads/turn-failed-retry.test.ts— 20 tests passed.pnpm exec turbo run test --filter=@bb/app -- --run src/components/thread/timeline/MessageActionBar.test.tsx src/components/thread/timeline/ThreadTimelineRows.actions.test.tsx— 71 tests passed.pnpm exec turbo run test --filter=@bb/integration-tests -- --run fake/recovery/stopped-unaccepted-request-recovery.test.ts— 1 test passed with in-memory SQLite.pnpm exec turbo run typecheck --filter=@bb/app --filter=@bb/server --filter=@bb/server-contract --filter=@bb/sdk --filter=@bb/integration-tests --filter=bb-plugin-scripted-echo-provider --filter=@bb/cli— 12 tasks passed.pnpm exec turbo run build --filter=@bb/app --filter=@bb/server --filter=@bb/cli— 7 tasks passed.node .github/workflows/check-plugin-sdk-version.mjsand the npm version guard passed; 0.4.44 is unpublished and ready for the publish job.pnpm exec turbo run test --filter=@get-bb/plugin-sdk -- --run src/__tests__/version.test.ts— 1 test passed.pnpm exec turbo run test typecheck --filter=@bb/demo-server --force— demo contract fixture typecheck and 9 tests passed.pnpm exec turbo run typecheck --force— all 86 workspace packages passed after auditing the remaining contract consumers.sdk-public-api.json; combined SDK/Guide/app/docs tests and typechecks passed (including 3,852 app tests and the API sync test), andbb plugin build plugins/plugin-api-docssucceeded.oxfmt --checkon every changed file andgit diff --check origin/main...HEADpassed.Retry requestdelivered the original and parked follow-up once, in order, without falsely marking the thread failed.Fixes #2370
@slopcop — tagged for visibility and review.