fix(http-bridge): retire denied anchors without redispatch - #1904
Closed
JustYannicc wants to merge 10 commits into
Closed
fix(http-bridge): retire denied anchors without redispatch#1904JustYannicc wants to merge 10 commits into
JustYannicc wants to merge 10 commits into
Conversation
An upstream `previous_response_not_found` against a proxy-injected `previous_response_id` is a verdict about the anchor, but nothing acts on it. Anchor poisoning only scores reader failures whose detail is `stream_incomplete` or `stream_idle_timeout`, and a denial arrives as a terminal upstream event, so it contributes nothing at any poison threshold. The dead id therefore survives in the durable row and in the session, the fresh-reattach path injects it into the next turn, and the store-context trim strips the resent history against it. Upstream then receives a suffix of the conversation behind an id it has already refused, never emits `response.created`, and the attempt presents as an eventless failure. Two of those open the retry circuit and the client gets a 503. Retire the anchor on the first denial instead, clearing the durable continuity record and the in-memory anchor together, and skip it when a sibling request has already advanced the anchor past the denied id. Client-supplied anchors are left alone. Also carry `proxy_injected_previous_response_id` onto the anchored recovery retry state. Without it a denial of the replayed anchor is not attributable to the proxy, so the retirement above cannot fire on the path that needs it most. The same gap reports `previous_response_source=client_supplied` for ids no client sent and keeps `_http_bridge_request_state_wedged_reattach` from recognising the reattach shape it exists to catch. No new dispatch is added: the following turn is the client's own, with the history the client sends, so no forked child response can be created against a parent this proxy cannot observe. The downstream contract is unchanged, so clients keep their anchor and are not driven into a full-history resend. Refs Soju06#1852 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review follow-up on three real gaps. Restrict retirement to anchors injected onto a full-resend-shaped payload. A delta-only request has no other way to convey prior context once its anchor is gone, which is the rule the expired-anchor path already applies before it clears durable continuity. Carry the companion `proxy_injected_anchor_had_full_resend_payload` flag onto the anchored recovery retry state alongside the provenance flag, so a replayed anchor keeps the shape that decides whether it may be retired. Retire the anchor from the grouped fan-out branch too. When one denial settles several requests sharing an anchor, that branch returns before the single-request path, so the shared anchor survived exactly the fan-out failure. Make retirement best-effort. It is bookkeeping, and a failure must not change how the denial reaches the client. Also record in the spec what the implementation actually guarantees: the durable clear is attempted and the in-memory clear is unconditional, because dropping one carrier strictly reduces the ways a denied id can come back. An unconfirmed durable clear is not reported as a retirement, and the surviving durable record re-injects the id on a later turn, which is denied again and re-enters this path. Refs Soju06#1852 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThe HTTP bridge now retires proxy-injected response anchors after upstream denial. It records bounded process-local denial fences, clears matching durable state, prevents denied-anchor dispatch, and preserves provenance across anchored recovery retries. ChangesDenied Bridge Anchor Retirement
Estimated code review effort: 5 (Critical) | ~90 minutes Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Author
|
Duplicate filing of #1902 from the same exact reviewed head. Closing this copy so review stays on one canonical PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This is the focused replacement for stale #1879. It extracts the denied
proxy-managed
previous_response_idlifecycle concern from the much larger#1867 repair stack, so it can be reviewed and merged as one coherent fix.
Problem
When upstream rejects a
previous_response_idthat codex-lb injected, the HTTPbridge can keep the dead anchor addressable and re-inject it into later turns.
That can trim away the client's usable full-resend context, produce repeated
Invalid previous_response_idfailures, and feed retry-circuit 503 responses.Concurrent completion, reconnect, absent-session capture, owner handoff, and
stream cancellation make the retirement boundary race-prone. The prior #1879
head is stale; this branch is rebuilt on the current upstream
mainand keepsthis concern separate from the remaining #1867 recovery work.
What this fixes
previous_response_not_founddenial.rejected before another upstream dispatch.
prepared requests still pin them.
durable cleanup under the original owner fence with a bounded retry budget.
The downstream denial contract remains unchanged: the denied turn is still a
502 stream_incomplete; the proxy does not speculate or redispatch it.What is now possible
the next turn can proceed without carrying that rejected anchor forward.
previous_response_id.predecessor cleanup retires only stale unpinned state.
the associated bridge request.
Type of change
fix:— bug fix (no behavior change beyond the bug)feat:— new user-facing feature or capabilityrefactor:— internal refactorLinked issue: Refs #1852. This is a focused partial fix and does not claim to
close #1852 or absorb all of #1867.
OpenSpec
request/response behavior
Change directory:
openspec/changes/invalidate-denied-bridge-anchor/Changes
Simplicity
.env.example, dashboard navigation, schema, or migrationsurface.
Test plan
Focused lifecycle and dispatch regressions on exact head
24dd93c83:The one broad-suite failure is the pre-existing
test_stream_via_http_bridge_fails_closed_before_file_affinity_when_previous_response_owner_misses;it fails identically on the predecessor because the fixture database lacks the
file_account_pinstable. The cancellation regression that previously failedon this candidate now passes.
Candidate
b311aea760aa639fd96f63bd118f775e9b4a89f924dd93c83b7f80fb5888f61e6bd711d90c9b95a0100a4ef9a9c01c200d51ad22a37deccab14751f8JustYannicc/codex-lb:fix/http-bridge-denied-anchor-retirementChecklist
change directory and requirements are included for hosted validation.
Summary by CodeRabbit
Bug Fixes
502 stream_incompleteerror.Tests