Repository navigation
fix: isolate ChatGPT login attempts and handle cancellation - #696
marinamdrs wants to merge 1 commit into
Conversation
Match login completions by login ID instead of sharing a pending promise between attempts. Forward cancellation through ACP v1 and v2. Cancel the matching login and clean up its completion wait and elicitation. Add regression tests for cancellation, retries, and late events.
![jetbrains-air[bot]](https://avatars.githubusercontent.com/in/3314082?s=60&v=4){kind=small}
| import {logger} from "./Logger"; | ||
|
|
||
| /** Runs account logins with a separate completion and cancellation scope for each request. */ | ||
| export class AccountLogin { |
There was a problem hiding this comment.
Blocking: This class duplicates login isolation that main already has. #720 (e059ab7, merged 2026-11-02, after this PR was opened) added runAccountLogin in src/CodexAccountLogin.ts. It fixes the same bug in the same layer, and it adds two things AccountLogin lacks: CodexAcpClient.authenticate serializes attempts, and a cancelled API-key completion is drained before the next login starts. This PR now conflicts with main. If the conflict is resolved by keeping this class, the client ends up with two parallel login owners.
Without serialization, the claim "matches notifications by login ID" does not hold for API-key logins, because they all complete with loginId: null:
- API-key login A gets its start response, and the request is cancelled before
account/login/completedarrives. - API-key login B starts with an invalid key. Its start response sets
loginId = null. - A's late
{loginId: null, success: true}matches B. B returnstrue, and the client is told sign-in succeeded with a key that failed.
The trigger is unlikely, because the cancel has to land between the start response and the completion. But the cancel path is new in this PR. I reproduced it with a vitest case: it fails on this head and passes on main.
Fix: rebase onto main and drop src/AccountLogin.ts along with the CodexAcpClient, CodexAcpServer and router changes. main already passes ctx.signal through both routers and into the URL elicitation. Keep only the tests that add coverage beyond auth-request-lifetime.test.ts. When I ran your login-cancellation.test.ts against main, 18 of its 20 tests passed unchanged. The other two assert behavior that #720 does differently: the completion listener is dropped before a late start response, and no account/login/cancel is sent after the app-server disconnects. Port those two only if main is wrong there.
Produced by Air Automations. Name: Codex ACP Code Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/deba358f-772c-4cff-bf03-c95d1670abe9?run=3c66fc80-edaf-4f65-a1ff-83d8b800fc74
There was a problem hiding this comment.
Thanks for the work here — requesting changes, since the blocking inline comment still needs to be addressed before merge.
Produced by Air Automations. Name: Codex ACP Code Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/deba358f-772c-4cff-bf03-c95d1670abe9?run=f2fe5c27-10a7-4d30-8dd3-6e37c717df42
| import {logger} from "./Logger"; | ||
|
|
||
| /** Runs account logins with a separate completion and cancellation scope for each request. */ | ||
| export class AccountLogin { |
There was a problem hiding this comment.
Blocking: My earlier comment is still open. The head is still 53a6396 and has no reply. This class still duplicates runAccountLogin (src/CodexAccountLogin.ts), which #720 added to main.
Still missing: a rebase onto main that drops this parallel login owner. The PR still conflicts with main in src/AcpAgentRouter.ts, src/CodexAcpClient.ts and src/CodexAcpServer.ts. The API-key race is also still live on this head. I reran it with the PR's own createFixture harness:
- API-key login A gets its start response, and its request is aborted.
- API-key login B starts. Nothing serializes it behind A, and it waits on
loginId === null. - A's late
{loginId: null, success: true}resolves B, soauthenticatereturnstrueeven though B's own completion then reportssuccess: false.
The case fails on 53a6396 and passes on main (ab513aa).
Fix: same as before. Rebase onto main, drop src/AccountLogin.ts and the client, server and router changes, and keep only the tests that add coverage beyond auth-request-lifetime.test.ts.
Produced by Air Automations. Name: Codex ACP Code Review / Run: https://air.jetbrains.cloud/org/05cf1a7f-6ab5-713b-abd3-29d0c8a05e2d/automations/deba358f-772c-4cff-bf03-c95d1670abe9?run=f2fe5c27-10a7-4d30-8dd3-6e37c717df42
|
|
||
| afterEach(() => { vi.clearAllMocks(); }); | ||
|
|
||
| describe("Account login attempts", () => { |
There was a problem hiding this comment.
Following up on the rebase comment, here is which of these tests are worth porting to main. I compared them with src/__tests__/CodexACPAgent/auth-request-lifetime.test.ts and CodexAcpClient.test.ts on main.
Keep (not covered on main):
matches an early completion after the start response supplies the login ID:runAccountLoginbuffers early events, but no test covers it.cleans up after a browser launch failure and permits a device code retrydoes not start a login with an already cancelled signalremoves the listener when the login start failspreserves API key completions with a null login ID
Drop (already covered):
- The 8
X then Y ignores the stopped login's successcases repeatdoes not apply an old completion to a new login. If you want the method mix, one case per first method is enough. cancels a late start response without opening its browser…repeatscancels a late %s start without opening UI.Authentication cancellation over ACP v%s(4 tests) repeatsdismisses a %s URL interaction before the cancelled login response, which already runs over a real ACP v1/v2 connection.ends the login and its elicitation when the app-server disconnectsasserts the opposite ofmain'scancels the owned login when its connection closes. Drop it unless you thinkmainis wrong.
That removes about 120 of the file's 264 lines. The rest fits as a few extra cases in auth-request-lifetime.test.ts and can reuse its harness instead of a second createFixture.
|
since #720 already fixed the root issue of this problem, i will close this PR, as it is redundant |
After a user stops a ChatGPT login and starts another, a late completion from the first attempt can finish the second attempt. Both attempts reuse
pendingLoginCompleted, which accepts a notification without checking itsloginId. Authentication also drops the request cancellation signal.This change gives each attempt a separate completion wait, matches notifications by login ID, and forwards cancellation through ACP v1 and v2. It cancels the matching app-server login and cleans up its wait and elicitation. It also handles early notifications, cancellation before the start response, and browser-launch failures.
Validation: