fix(codex): fence device reauth cancellation - #4998
Conversation
Propagate the flow AbortSignal into the prepared native-main reauth commit and its exclusive claim so a cancelled flow cannot still replace auth.json or clear the reauth quarantine. Terminal settlements are now strictly first-write-wins: a commit completing after cancellation can no longer flip the flow back to succeeded.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe native reauthentication commit path now accepts an optional ChangesNative reauthentication cancellation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ActiveFlow
participant commit
participant withNativeMainExclusiveClaim
participant auth.json
ActiveFlow->>commit: pass flow.controller.signal
commit->>withNativeMainExclusiveClaim: wait with signal
ActiveFlow->>commit: cancel during claim wait
withNativeMainExclusiveClaim-->>commit: abort
commit-->>ActiveFlow: cancellation result
commit->>auth.json: write credentials only after the abort check
Merge Risk: ⚪ Minimal · up to The cancellation fencing, credential-publication boundary, and terminal settlement changes have no identified actionable risk. The change is mergeable after normal CI completion. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
Threading the abort signal into the commit is right: it lets a cancel abort a contended 30s claim wait, which the previous recheck could not do. Removing the publication exception from finish() is not. The signal is checked inside the claim immediately before a synchronous write, so it cannot be delivered between that check and the write, but it also cannot fence the gap after it: claim teardown and promise resolution both yield before finish() runs. A cancel arriving there made the flow report "cancelled" for an auth.json that had already been replaced and a reauth quarantine that had already been cleared. Restore the exception. The new in-flight cancellation test is unaffected because its commit rejects on abort and never publishes.
리뷰 · 우선순위 74 / 80이 PR은 네이티브 메인 디바이스 재인증( 고치는 축은 세 겹입니다. (1) 원래 기여자 커밋( types.ts/config.ts 스플릿 캠페인과는 무관하고, 중복 랜딩 PR도 보이지 않습니다. Preview deploy는 계획에 없습니다. 점수는 높게 잡습니다. 버그가 tip에 실재하고, claim 시그널 배선·문서·회귀 테스트·메인테이너 후속이 한 줄기로 맞습니다. 다만 브랜치가 아직 draft이고 base가 tip보다 뒤( src/codex/main-account.ts (commit 안 options.signal?.aborted) - abort 직후 메인테이너의 판단이 필요한 지점
너의 추천 CI 그린과 tip( 이 댓글은 grok-bot이 작성했습니다 |
2 participants
Summary
Fence device reauthentication cancellation through the credential commit. Previously a cancel delivered while the prepared commit was in flight could not reach the native-main exclusive claim or the filesystem write, so the in-flight commit could still replace ^Guth.json\ and clear the reauth quarantine — and a late success could then overwrite the cancelled terminal.
Every macrotask yield inside the commit (claim retry wait, claim-database open) is already signal-fenced by \withNativeMainExclusiveClaim; the claim operation itself is synchronous after the new in-operation recheck, and the claim release is synchronous, so a cancel can only settle before the write — never during or after it.
Verification
f9c24b37e4cb4a22fa428c64a3cf262e4d49d469(tree688dfe13cf665160fa5d85ed3ebded863099a7f4) on top of dev6467235a8.Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing. Local gates passed on this head (tsc, structure:check, privacy:scan, focused tests on the merged head).
I pushed my PR to the latest dev commit. The branch carries dev
4c0124acb, 4 behind tip - inside the 10-commit window.I resolved all correct Codex and CodeRabbit findings. No unresolved review threads on this head.
My PR is ready for review.
Remaining gates: fork CI run 35291461934 in progress (known macos control 30-minute cap, #4905).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation