Skip to content

fix(oauth): bound refresh consumers and preserve retryable capacity - #7117

Closed
luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:codex/upstream-oauth-refresh-consumers-20261015
Closed

luvs01 wants to merge 1 commit into
lidge-jun:devfrom
luvs01:codex/upstream-oauth-refresh-consumers-20261015

Conversation

@luvs01

@luvs01 luvs01 commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Cancelled requests release their active-turn slots while shared OAuth refresh work can remain pending. This change bounds those unresolved consumers independently, preserves stale-owner cleanup, and reports temporary saturation as retryable capacity instead of an authentication failure.

  • Limit unresolved OAuth refresh consumers to 256 per process, including joins to shared refreshes. Keep the separate 32-flight ceiling. Fresh cached credentials bypass the consumer gate, and a cancelled client does not recycle its refresh permit before the shared promise settles. Active-turn release on cancellation is unchanged.
  • Preserve stale-owner evidence when saturation refuses replacement admission. Retire only the exact matching intent after the old attempt finishes its result cleanup; retain a bounded slot on retirement IO failure. Late explicit rejection, successful rotation and backward clock changes remain covered.
  • Map capacity during initial auth and upstream-401 forced refresh to retryable HTTP 503 with Retry-After: 1, with cancellation taking precedence as 499. Adapter, passthrough and runTurn paths are covered. Already-committed runTurn SSE retains HTTP 200 and emits retryable capacity rather than an authentication error.
  • Add regression tests and update the owning runtime/failover documentation. No GUI, dependency, deployment or release changes.

This remains a Draft. It bounds the count of unresolved consumers, not retained bytes, provider refresh duration or capacity across multiple processes.

Exact revision

  • Upstream dev integrated and rechecked on 2026-10-15: 7640a942b35044c974e534b84d5a8049f4a8ae9a.
  • Current signed HEAD: b722d746f10dafb7eeea2aabcf8b94aec6a6a5db.
  • Current tree: e27b603e952dae196fe6689632fbc8e8e5613b97.
  • Signed replacement parent: 7640a942b35044c974e534b84d5a8049f4a8ae9a. The earlier three-commit history is preserved privately for traceability.
  • Diff against the pinned dev: 13 files, 573 insertions, 54 deletions; SHA-256 77b5db2322f0c8efb7a4a4194c395b5c8f5c49da8cb027efa43b226ee4d0e0c1 using full-index, no-renames, unified-3 Git diff.

Contributions and commit identity

This revision builds on luvs01/opencodex#735; the Kiro cancellation follow-up corrects regression-test assertions without changing production code.

Verification

Linux, existing Bun 1.4.3 and matching locked dependencies. The earlier pre-correction tree a76f8d0 passed 250 tests / 1,977 assertions across 10 files, with 18 new follow-up regressions. Earlier and repeated runs are not added to this count.

bun --no-install test --isolate --timeout 30000 --preload "$GUARD" tests/oauth/oauth-refresh-stale-capacity.test.ts tests/oauth/oauth-refresh.test.ts tests/oauth/oauth-refresh-generic-lock.test.ts tests/oauth/oauth-refresh-lock-multiprocess.test.ts tests/responses/responses-oauth-refresh-capacity.test.ts tests/responses/responses-devin-401-replay.test.ts tests/oauth/adapter-event-oauth-failover.test.ts tests/oauth/generic-oauth-failover.test.ts tests/test-layout.test.ts tests/test-layout-tooling.test.ts
bun --no-install --preload "$STATIC_GUARD" node_modules/typescript/bin/tsc --noEmit --pretty false
bun --no-install --preload "$STATIC_GUARD" scripts/structure-ssot.ts
bun --no-install --preload "$STATIC_GUARD" scripts/privacy-scan.ts
bun --no-install --preload "$STATIC_GUARD" scripts/file-size-ratchet.ts
git diff --check

All listed root/static gates passed. Commands used a scrubbed environment, isolated HOME/TMPDIR and the repository test-home preload. GUARD additionally rejected unmocked fetch calls; STATIC_GUARD rejected network fetches during static checks. No unmocked fetch was attempted. This is synthetic/application-level isolation, not an OS network-sandbox or live-provider claim.

The two reported follow-up failures were reproduced before correction: saturated stale ownership failed both pre/post-dispatch cases, and upstream-401 saturation/cancellation failed the original 12 transport cases. The final tests cover those paths plus late result settlement, retirement IO retry and committed SSE. Existing terminal-401, pause, 429 and account-failover cases remain passing. Follow-up code review found both reported P2s resolved and no new confirmed P1/P2; this does not substitute for required upstream approval or explicit security review.

Kiro cancellation CI follow-up

Hosted CI on the previous head exposed one stale 401 assertion after cancellation during credential recovery. It was reproduced locally, then corrected to the intended 499/client_cancelled contract with no production change. Both zero-lease assertions remain before body consumption; additional assertions check the cancellation type/code/message, absence of Retry-After, one refresh and no second inference dispatch.

The current tree passed 69 tests / 477 assertions across four files under the same offline/home-isolated guards, plus source typecheck, structure, privacy, filesize and diff checks. This is a separate run overlapping the earlier 250 tests; the totals must not be added. The new server-signed commit's hosted CI must be checked separately.

bun --no-install test --isolate --timeout 30000 --preload "$GUARD" tests/providers/kiro/kiro-leased-responses.test.ts tests/responses/responses-oauth-refresh-capacity.test.ts tests/oauth/oauth-refresh-stale-capacity.test.ts tests/responses/responses-devin-401-replay.test.ts

Scope exception and remaining evidence

The full application suite was not run: the parsed import graph reaches approximately 1,735 test files, disproportionate to this bounded private/offline verification. The focused regressions, adjacent transport/failover cases and layout checks above are the documented resource exception. Keep the PR in Draft until the required broader scope is accepted. Do not treat the unrun suite or skipped/older-head checks as passes.

  • Current-head hosted CI, required upstream reviews, and explicit security review remain pending. Any applicable maintainer sponsorship remains a merge condition, not a ban on preparing this Draft.
  • The new-test strict compiler check leaves one diagnostic in unchanged scripts/test-temp-lock.ts:78: TS2339, missing SyncSubprocess.signal in the installed typings. Neither new regression file has a remaining diagnostic. An earlier broader selected-test compiler run had 33 diagnostics on unchanged lines; a same-command original-tree compiler comparison was not performed, so these are not claimed to be conclusively baseline-only.
  • Earlier strict migration checks retained an unchanged source-read warning in tests/responses/ws-endpoint.test.ts:56; basic layout membership/tooling passes, but the stricter scan is not claimed newly clean.
  • Operational suitability of 256 waiters, actual HTTP/WebSocket saturation/recovery, live refresh, Windows/macOS acceptance and real client automatic retry were not established.
  • Unsaturated replacement can still lose inherited evidence if its stale-marker write fails. This path already exists in pinned upstream dev and was not established as a new regression from this patch. It remains a separate follow-up concern; the new IO regression covers saturated-owner settlement retirement only. No whole-refresh-lifecycle completion is claimed.
  • Further useful coverage includes mixed states across all 32 retained slots, foreign flight/generation intents, committed-SSE retryable/cancellation assertions and direct callback/lease-release assertions. Review identified these as coverage improvements, not confirmed new defects.

Checklist

  • Scope stays focused and preserves release-on-cancel behavior.
  • Owning runtime/failover documentation and explicit test membership are updated.
  • Focused regressions and root static gates pass with the scope disclosed above.
  • Required explicit security review and any applicable sponsorship are complete.
  • Required current-head hosted CI and remaining validation scope are accepted.

Review readiness checklist

  • Required local validation passed; commands, results, and any full-suite exception are documented.
  • I pushed my PR to a recent dev commit (at most 10 behind; a maintainer may still ask for the exact tip before merge).
  • I resolved all correct Codex and CodeRabbit findings.
  • My PR is ready for review.

@coderabbitai

coderabbitai Bot commented Oct 15, 2026

Copy link
Copy Markdown
Contributor

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

  • Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the bug Something isn't working label Oct 15, 2026
@luvs01
luvs01 force-pushed the codex/upstream-oauth-refresh-consumers-20261015 branch from a76f8d0 to f61b00d Compare October 15, 2026 04:45
Bound unresolved OAuth refresh consumers while preserving stale-flight ownership evidence across refused replacement admission. Report retryable capacity separately from authentication failures, and preserve cancellation priority and lease-release assertions.
@luvs01
luvs01 force-pushed the codex/upstream-oauth-refresh-consumers-20261015 branch from f61b00d to b722d74 Compare October 15, 2026 06:14
@lidge-jun

Copy link
Copy Markdown
Owner

Superseded by #7136, which carries this work with Co-authored-by credit. Thank you!

@lidge-jun lidge-jun closed this Oct 15, 2026
@lidge-jun

Copy link
Copy Markdown
Owner

Thank you, @luvs01. I've carried this onto an upstream branch in #7136, so the repository's full CI (including the macOS and Windows matrix) can run on it. Your commits are kept and credited with a Co-authored-by trailer. Follow-up fixes there address the open review findings; the description of #7136 lists them. I'm keeping this PR open until #7136 lands, then I'll close it as merged by carry.

lidge-jun added a commit that referenced this pull request Oct 15, 2026
) (#7136)

* fix(oauth): bound refresh consumers and preserve retryable capacity

Bound unresolved OAuth refresh consumers while preserving stale-flight ownership evidence across refused replacement admission. Report retryable capacity separately from authentication failures, and preserve cancellation priority and lease-release assertions.

(cherry picked from commit b722d74)

* fix(oauth): retire stale owners before admitting replacements

Keep stale registry ownership through definitive-result cleanup and durable retirement regardless of spare consumer capacity. Recheck the installed flight when concurrent replacements resume.

Co-authored-by: Epinephrine <luvs01@hanmail.net>

* fix(responses): preserve cancellation at refresh capacity

Observe both independent cancellation signals at initial refresh refusal and document retry behavior for OAuth capacity.

Co-authored-by: Epinephrine <luvs01@hanmail.net>

* fix(responses): link both client signals through runTurn recovery

Address the hosted Codex P2: request-only cancellation takes priority during refresh refusal, including committed SSE. Remove both listeners on cleanup.

Co-authored-by: Epinephrine <luvs01@hanmail.net>

* fix(oauth): report deferred refresh retirement without identity

Address the CodeRabbit observability finding with one bounded diagnostic per retained flight, a closed failure reason, and no account or credential fields.

Co-authored-by: Epinephrine <luvs01@hanmail.net>

* refactor(oauth): move refresh-flight retirement into its own module

Pure move of retireSettledOAuthRefreshFlight, the refresh-waiter gate and OAuthRefreshFlightEvidence out of src/oauth/index.ts, so this branch merged with current dev stays under the 2000-line file-size ratchet. The registry map is passed explicitly; behavior is unchanged.

---------

Co-authored-by: Epinephrine <luvs01@hanmail.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants