Skip to content

fix(usage): reject pre-cancelled quota waits - #734

Merged
testikun merged 2 commits into
openpi-dev:mainfrom
outlier27-cell:fix/issue-733-usage-cancel
Oct 23, 2026
Merged

testikun merged 2 commits into
openpi-dev:mainfrom
outlier27-cell:fix/issue-733-usage-cancel

Conversation

@outlier27-cell

@outlier27-cell outlier27-cell commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Closes #733. A pre-cancelled waitForUsage() can expose an already-fulfilled credential or quota result, crossing the caller's cancellation boundary.

Value

Cancellation takes precedence over stale quota results while Pi retains ownership of the accepted credential-refresh operation.

Approach

Observe rejection of an already-accepted refresh promise before rejecting a pre-cancelled wait, then check signal.throwIfAborted() before registering the race. Existing in-flight cancellation and listener cleanup remain intact. Deterministic tests cover fulfilled/rejected promises, cancellation timing, error identity and unhandled rejection cleanup.

The original contributor history is preserved. Current main f98d6a4737461ba8d8ee100f113b5a7c3a0001a0 is integrated with a merge commit; the resulting PR scope remains two files.

Validation

Integrated head: cde62a1d13e2d3e99cf0df8f5b7746d203f8beb3 (original head 162109c6e1fb6918a8c31f6b442c97a3a045d04c). Node 26.8.1, Bun 1.3.14.

  • bun install --frozen-lockfile: passed.
  • node --test --experimental-strip-types tests/extensions/usage/providers.test.ts: 17 passed, 0 failed.
  • Deterministic production-helper smoke: normal success and error identity preserved; pre-cancelled fulfilled result rejected; pre/in-flight cancellation retains original reason; zero leaked abort listeners or unhandled refresh rejections. No provider calls.
  • bun run check: passed, exit 0.
  • bun run test: passed, exit 0; Node 2463 total / 2454 passed / 9 skipped / 0 failed; UI 88 files / 1338 passed / 0 failed.
  • git diff --check: passed.

These are source tests and direct helper execution. No installed-Pi, live provider/model, or manual UI acceptance is claimed; the user's installed OpenPI source remains a different checkout.

  • Two independent final source reviews passed on this exact head. Current-head CI run 38694664766 passed all required checks, including Windows. Independent last-push approval remains required before merge.

Impact

  • User-visible: cancelled quota waits reject instead of returning stale results.
  • Model-visible context/tools: no schema changes.
  • Runtime/lifecycle: pre-cancelled admission respects the caller's signal; accepted refresh work retains its existing owner.
  • Persisted configuration/data: no changes or migration.
  • Compatibility/risk: scoped two-file change; reversible code update with no new dependency or configuration.

@outlier27-cell

Copy link
Copy Markdown
Contributor Author

@tt-a1i 请审核此 PR。#733 的最小取消边界修复已在 0cdac2c 完成,完整 exact-head GitHub CI(Node、Windows、UI、E2E)均通过;当前可合并,仅等待审核。

@outlier27-cell

Copy link
Copy Markdown
Contributor Author

@testikun @tt-a1i 请审核此 PR。最新提交的 CI 已通过,当前仅等待维护者审核与合并。

@tt-a1i tt-a1i left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact reviewed head: 0cdac2c3b2e0d1bcf217390382011c9bb0206145.

Spec:

  • [P2] Observe rejection of an already-started promise on pre-cancellation: Passing Promise.reject(new Error("quota failed")) with an already-aborted signal returns the cancellation reason, but still emits unhandledRejection for the accepted promise. Existing 13/13 tests passed and do not cover this branch. Preserve cancellation precedence while attaching a rejection observer to the already-started promise; add immediate and delayed rejection controls.

Verification: prior exact-head Standards and Spec evidence reused because this head is unchanged; current review/CI/integration metadata refreshed. No complete-suite rerun, installed-runtime/provider run, or visual acceptance performed in this update.

Maintainer fix follow-up

The requested rejection observation is implemented in 162109c6e1fb6918a8c31f6b442c97a3a045d04c, appended to the original contributor branch with a non-force push. Pre-cancellation preserves the exact cancellation reason while observing immediate and delayed rejections of the accepted input Promise; no Pi refresh ownership or abort listener is added. The regression fails on the original helper and passes after the fix. Independent Spec and Standards reviews have no open scoped findings.

Validation: focused 17/17 and bun run check passed. Local bun run test exited 1: 2,317 passed / 1 failed / 1 cancelled / 9 skipped, with UI not reached. The unchanged detached-workflow settlement and setup-lock recovery cases passed as isolated controls with the original owned files restored; load sensitivity remains an inference and the full-suite failure is retained. Exact-head remote CI is running.

The code finding is addressed; the current commit still requires exact-head CI and valid final-push approval before merge.

@tt-a1i tt-a1i left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exact reviewed head: 162109c6e1fb6918a8c31f6b442c97a3a045d04c.

Spec:

The pre-cancelled wait now observes immediate and delayed input-Promise rejection while preserving cancellation reason precedence. The failure-first regressions pass, normal rejection remains observable, and no pre-cancel abort listener is installed.

Standards:

Pi retains ownership of credential refresh; this patch only observes an already-accepted promise and does not cancel or widen authority. Independent automated Spec and Standards reviews have no open findings on the frozen two-file patch.

Validation:

Focused 17/17 and bun run check passed. Current-head remote CI completed 18/18 SUCCESS, including Runtime, Windows, UI and Web E2E. Local full-test failure remains recorded: 2,317 passed / 1 failed / 1 cancelled / 9 skipped, UI not reached; the two unchanged timeout cases passed as isolated original-head controls. The remote result is separate evidence, not a rewrite of the local failure.

Merge gate:

I pushed the final fix, so branch protection still needs valid approval for the last push from another eligible maintainer. This source approval does not bypass that requirement or authorize a merge.

tt-a1i added a commit to outlier27-cell/openpi that referenced this pull request Oct 15, 2026

@testikun testikun left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

审查提交:80a4e388441c71f4c8afab519ef02e9c2ccc253e

已审查本次提交,未发现阻塞问题,Approve。

取消等待的优先级正确;已接受的 credential refresh Promise 继续由原调用方管理,立即或延迟拒绝均被观察,不会产生 unhandled rejection。预取消分支不增加 abort listener,正常返回值及错误身份保持不变。此前 P2 已复现并确认修复。

本地验证:macOS / Node v24.20.0,独立 worktree;providers 17/17、usage 调用链 9/9;额外通过生产 helper 和 collectUsageReports 的受控探针验证正常、预取消、运行中取消、错误身份、listener 清理及不写入过期缓存。无 live provider 请求。git diff --check 通过。当前提交远端 CI 18/18 成功。

本次未重跑全量 check/test,未声称已安装 Pi 或 Windows 的本地验收。PR 当前落后于 main;此次批准针对下述审查提交,更新后的提交仍需满足仓库同步和 CI 要求。

@testikun
testikun force-pushed the fix/issue-733-usage-cancel branch from 80a4e38 to e84e1e9 Compare October 23, 2026 06:31

@testikun testikun left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed rebased head e84e1e9bd7afde1985e16a0365bcc4e2492e287c on main b2a82fc6551586dad139b40d6b262722bb54d103. No outstanding scoped P1/P2 findings. The two-file patch is byte-identical to the previously approved patch; the usage helper and both callers are unchanged by intervening main commits. Original contributor authors and author dates are retained by native rebase.

Validation: own detached worktree and frozen Bun 1.3.14 dependencies, Node v24.20.0; providers/index 26/26 passed, six controlled production-caller cancellation cases passed with zero network calls/unhandled rejection, git diff --check and bun run check passed. Exact-head remote CI passed after one evidenced flaky E2E rerun: the original thinking-picker reset test observed one duplicate request; the same test passed on current main and in three isolated exact-head local repetitions before the single rerun. Original failure retained.

Local full-suite limits remain explicit: initial inherited umask 0077 caused an unchanged worktree executable-mode assertion failure (2,403 passed / 1 failed / 8 skipped, UI not reached); the isolated case passed with umask 0022. Full rerun under 0022 passed all 2,404 Node tests (8 skipped), then UI reported 1,628 passed / 12 failed across seven unchanged files, including timeouts and rendering/order assertions. A one-worker control passed several affected files/cases before its 55-second harness cap; it is not claimed as a complete-suite pass. Remote CI is separate green evidence. No code/assertion fix, installed-user-runtime smoke, live-provider call, manual Windows or real TUI acceptance is claimed.

This approval concerns the exact head above and does not bypass repository gates. Squash metadata will retain both original contributors with explicit Co-authored-by trailers.

@testikun
testikun merged commit c029896 into openpi-dev:main Oct 23, 2026
30 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(usage): reject a pre-cancelled quota wait before returning a cached result

3 participants