Repository navigation
fix(usage): reject pre-cancelled quota waits - #734
Conversation
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
testikun
left a comment
There was a problem hiding this comment.
审查提交: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 要求。
80a4e38 to
e84e1e9
Compare
testikun
left a comment
There was a problem hiding this comment.
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.
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
f98d6a4737461ba8d8ee100f113b5a7c3a0001a0is integrated with a merge commit; the resulting PR scope remains two files.Validation
Integrated head:
cde62a1d13e2d3e99cf0df8f5b7746d203f8beb3(original head162109c6e1fb6918a8c31f6b442c97a3a045d04c). 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.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.
Impact