Move provider retry policy into the provider-retry plugin - #1623
Conversation
![bb-slop-cop[bot]](https://avatars.githubusercontent.com/in/4577281?s=60&v=4){kind=small}
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: Claude can hit a usage limit while a background agent still sends final events. Those events could hide the failed user request and cancel its scheduled retry. This change keeps recovery tied to the latest accepted user turn. It also suppresses most false turns from late Claude events.
I found two issues:
-
P2 — Active and idle events inspect every thread. The retry plugin now calls the recovery SDK on every normal transition. This adds two loopback calls and several synchronous SQLite reads to most turns. The active and idle handlers should inspect only threads that already have a retry entry.
-
P2 — Bridge errors bypass the new suppression rule. The rule covers SDK messages, but the bridge-error envelope uses another path. A failed result followed by a stream error can still create a false, unaccepted turn and duplicate error output. The common Claude error path should honor the same suppression state.
I found no security issue. The retry checks remain scoped to one thread and require the exact accepted request before reuse.
I also checked architecture and duplication. The database helper has no existing equivalent. Its location in @bb/db is correct. I found no stale names or duplicate recovery policy.
Validation passed for the changed paths:
- Claude adapter: 152 tests.
- Server recovery: 15 tests.
- Provider retry plugin: 21 tests.
- Host daemon contract: 50 tests.
- Type checks: all four affected packages.
- GitHub CI: all required checks pass.
I did not run a browser test. This path needs a real Claude subscription limit and has no browser-only route.
afe4a63 to
210c4e9
Compare
ymichael
changed the title
|
Redesign update: provider-specific retry recovery has been removed from the server, public SDK, and core CLI. The plugin now owns classification, scheduling, timing, cancellation, continuation text, and its manual retry command. Core exposes only the provider-neutral in-process This also eliminates the review's loopback-call concern. The Claude bridge-error/background-drain suppression remains in place and is covered by the latest tests. The branch has been rebased onto current |
5bb74e3 to
10990e5
Compare
|
Correction to my earlier redesign note: the
The branch is rebased onto current |
10990e5 to
5266130
Compare
|
Improved the generic The API now supports an exclusive There is no recovery-specific API, database migration, or new index. The published plugin SDK declarations were regenerated and the SDK version is now 0.4.9. Validation on rebased commit |
5266130 to
a3ca11d
Compare
| threadId: string, | ||
| candidate: RecoveryCandidate, | ||
| ): Promise<void> { | ||
| await this.bb.sdk.threads.send({ |
There was a problem hiding this comment.
🚨 slopcop/review — P1: Keep the retry check and append atomic
inspectProviderRetry() checks failed request A. This normal send has no expected request or event sequence.
A user can finish request B between these calls. The thread becomes idle, so this code starts a stale Please continue. turn.
The normal send also records a user action. It can ask for permission, change prompt history, and replace a newer sticky model.
The removed server path repeated the candidate check inside one immediate transaction. Add a generic conditional system send with the same atomic check.
The server must keep the system initiator and must not change the user's current settings.
| // 0.x releases and is intentionally vacuous for them until a future 1.0. | ||
| // Rebuildable artifacts still rebuild on the exact sdkVersion-differs trigger. | ||
| export const PLUGIN_SDK_VERSION = "0.4.8"; | ||
| export const PLUGIN_SDK_VERSION = "0.4.9"; |
There was a problem hiding this comment.
🚨 slopcop/review — P1: Do not remove public methods in a compatible SDK release
This patch removes rateLimitRecovery and continueAfterRateLimit from the public plugin SDK. It changes version 0.4.8 to 0.4.9.
The contract above says all 0.x minor and patch releases are compatible. An installed 0.4.8 plugin still loads, then gets a runtime TypeError.
Keep deprecated compatibility methods until a compatibility-major release. Otherwise, make a real major transition with a migration path.
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain English summary: This change moves provider retry decisions from the main server into the Provider retry plugin.
The plugin now reads normal thread events and sends a hidden continuation turn after a provider limit. Claude also ignores late events after failure.
I found four P1 issues:
- The retry check and send are not atomic. A newer user turn can finish between them, then the stale retry starts unwanted work.
- The patch removes two public plugin SDK methods in a compatible 0.4.9 release. Existing 0.4.8 plugins can load and fail at runtime.
- SQLite selects the full thread sequence index for the new filtered reverse query. A sparse 300,000-row check took 861 ms.
- A failure without rate-limit history reads the complete matching history. The classifier then does quadratic work across prior requests.
The first issue also changes authority. The retry now looks like user work, can request permission, and can replace a newer sticky model.
I found no separate data exposure issue. I also found no duplicate recovery policy or stale recovery name.
The policy belongs in the plugin. The server still needs a generic atomic system-send operation, not provider-specific retry policy.
Validation:
- Provider retry passed 23 tests.
- The Claude provider passed 262 tests.
- The affected type checks passed.
- All required GitHub checks pass.
- The development server and browser smoke test passed.
- The plugin detail page loaded without browser errors.
- The scheduler ran, and
bb provider-retry status --jsonreturned an empty list. - The filtered descending event route and exclusive
beforeSeqcursor worked on a real smoke thread. - A real subscription reset was not available, so I did not test that external event.
- One local full database run had an unrelated migration test timeout while other tests used the host. GitHub and isolated database checks passed.
I left this as a comment review. I did not approve the pull request or request changes through GitHub.
ba1ef7b to
d4e457c
Compare
d4e457c to
2fbf9fd
Compare
Main commit 1222451 moved provider retry policy into the provider-retry plugin (those parts merged cleanly) and taught the claude translator not to manufacture an unaccepted provider-only turn from late SDK drain output after a terminal failure. That translator fix lived in the deleted event-translation.ts/task-translation callback; re-implemented in the delta translator's turn mirror: - ClaudeThreadDialectState.suppressUnacceptedTurnStart arms on a failed result boundary and clears when a turn actually opens (mirrorOpenTurn) or a real accepted input arrives (acceptInput), matching main's onTurnStart and emitCanonicalTurnInputAccepted clears. - While suppressed (flag set, no open turn, no pending accepted input): conversation_reset, compacting status, assistant messages, stream events, and draining bridge errors translate to nothing; an api-retry notice stays a thread-scoped provider error; a hard rate-limit rejection surfaces its snapshot without opening a turn or arming the deferred rejection; and a task_started may not materialize a new task (TranslateClaudeTaskMessageArgs.turnStartSuppressed — the delta analogue of main's ensureTurnStarted returning undefined). Both regression tests from the old event-translation suite are ported to the delta harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Main commit 1222451 moved provider retry policy into the provider-retry plugin (those parts merged cleanly) and taught the claude translator not to manufacture an unaccepted provider-only turn from late SDK drain output after a terminal failure. That translator fix lived in the deleted event-translation.ts/task-translation callback; re-implemented in the delta translator's turn mirror: - ClaudeThreadDialectState.suppressUnacceptedTurnStart arms on a failed result boundary and clears when a turn actually opens (mirrorOpenTurn) or a real accepted input arrives (acceptInput), matching main's onTurnStart and emitCanonicalTurnInputAccepted clears. - While suppressed (flag set, no open turn, no pending accepted input): conversation_reset, compacting status, assistant messages, stream events, and draining bridge errors translate to nothing; an api-retry notice stays a thread-scoped provider error; a hard rate-limit rejection surfaces its snapshot without opening a turn or arming the deferred rejection; and a task_started may not materialize a new task (TranslateClaudeTaskMessageArgs.turnStartSuppressed — the delta analogue of main's ensureTurnStarted returning undefined). Both regression tests from the old event-translation suite are ported to the delta harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Main commit 1222451 moved provider retry policy into the provider-retry plugin (those parts merged cleanly) and taught the claude translator not to manufacture an unaccepted provider-only turn from late SDK drain output after a terminal failure. That translator fix lived in the deleted event-translation.ts/task-translation callback; re-implemented in the delta translator's turn mirror: - ClaudeThreadDialectState.suppressUnacceptedTurnStart arms on a failed result boundary and clears when a turn actually opens (mirrorOpenTurn) or a real accepted input arrives (acceptInput), matching main's onTurnStart and emitCanonicalTurnInputAccepted clears. - While suppressed (flag set, no open turn, no pending accepted input): conversation_reset, compacting status, assistant messages, stream events, and draining bridge errors translate to nothing; an api-retry notice stays a thread-scoped provider error; a hard rate-limit rejection surfaces its snapshot without opening a turn or arming the deferred rejection; and a task_started may not materialize a new task (TranslateClaudeTaskMessageArgs.turnStartSuppressed — the delta analogue of main's ensureTurnStarted returning undefined). Both regression tests from the old event-translation suite are ported to the delta harness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
provider-retrypluginbb thread retrycommandthreads.events.listquery with typed event filters, newest-first ordering, and an exclusive backward cursor(thread_id, type, sequence)index with bounded per-type reads and a global ordered mergeOwnership boundary
The server has no provider-rate-limit recovery policy and exposes no special failed-turn continuation API. It continues to own the ordinary thread/event/send contracts that every SDK consumer can use.
The
provider-retryplugin reconstructs the latest accepted failed request from the public event stream, classifies normalized provider events, preserves the original execution settings, schedules automatic retries, and exposesbb provider-retry retry. Before release it re-inspects the event history and then uses the existingthreads.send({ mode: "start" })API with an agent-onlyPlease continue.turn.The plugin now fetches the latest request directly, searches only rate-limit observations for the current provider state, and pages forward through the six relevant lifecycle/provider types beginning at that request. Ordinary failures therefore do not read prior request history. Failure classification uses one-pass request/acceptance/completion indexes instead of rescanning the event array for every request.
The generic event reader handles multi-type filters as one bounded index-backed read per type, then merges those already-sorted pages to preserve global
asc/desc, cursor, and limit semantics.The Claude provider plugin owns the provider translation fix. After a terminal failure it suppresses provider-only turn starts while late background or bridge events drain, and clears that suppression when a new accepted request begins.
Contract and data changes
threads.events.listSDK/server contract withbeforeSeq,order, and typedtypesafterSeqpagination@get-bb/plugin-sdkat 0.4.8 and regenerates its bundled declarations/templatesRoot cause
The retry plugin correctly scheduled the original accepted rate-limited failure. While Claude background output drained, the Claude translator opened a synthetic turn with no client request or accepted input. That transition cancelled the scheduled retry and made recovery observe the wrong turn.
Validation
origin/main(bf3d67ae5); PR head is2fbf9fdf3@bb/domain,@bb/server-contract,@bb/sdk,@bb/db,@get-bb/plugin-sdk,@bb/templates,@bb/server, andbb-plugin-provider-retry@get-bb/plugin-sdkat 0.4.8 because that version has not been publishedgit diff --checkpassed