Repository navigation
Conversation
- Incomplete-body test: widen the 500ms connection deadline to a 15s failure bound. Under full-suite load the handler stalls opening the request log temp file before it writes any response. - Antigravity blocked-persistence test: stop closing releaseSave a second time in the final timeout branch, where the panic replaced the real failure message, and widen the 1s failure bounds. - pollOAuthStatusUntilDone: pace the status polls with a ticker instead of a busy loop that starves the OAuth flow goroutine, and widen the deadline. - Devin remote OAuth test: widen the 5s failure bounds. - Watcher config reload test: wait for the reload callback and for the reload to return instead of sleeping 400ms against the 150ms debounce timer.
waitDevinOAuthCallback ended the login with "Failed to read OAuth callback" on any read error other than not-exist. On Windows a read that lands while the callback handler renames the file into place fails with a sharing violation, so a login could fail although the callback arrived. Retry on the next tick, as the other OAuth flows already do; the context deadline and session state still bound the wait. This was the cause of the intermittent "token exchange did not start" in TestDevinRemoteOAuthFlow. The test now reports the session status on that failure.
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a50f54eb68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // into place fails with a sharing violation. The next read succeeds. | ||
| reads := 0 | ||
| originalRead := readDevinOAuthCallbackFile | ||
| readDevinOAuthCallbackFile = func(string) ([]byte, error) { |
There was a problem hiding this comment.
In a shuffled package run, TestStartOAuthV8TargetedSessionRegistrationAcrossBuiltinProviders can run immediately before this test. Its Devin subtest only calls CancelOAuthSession, which deletes the session without joining completeDevinOAuth, so that goroutine may still be reading the global hook at auth_files_devin_oauth.go:152 when it is replaced or restored here. This introduces a data race, and a leftover waiter reaching the stub can also consume its first read and break the reads == 2 assertion. Inject the reader per waiter rather than replacing a package-wide function.
Useful? React with 👍 / 👎.
waitDevinOAuthCallback ended the login with "Failed to read OAuth callback" on any read error other than not-exist. On Windows a read that lands while the callback handler renames the file into place fails with a sharing violation, so a login could fail although the callback arrived. Retry on the next tick, as the other OAuth flows already do. The context deadline and session state still bound the wait. This was the cause of the intermittent "token exchange did not start" in TestDevinRemoteOAuthFlow. The test now reports the session status on that failure, and its 5s failure bounds are 15s. Backport of upstream router-for-me#7482 (a50f54e), with the Devin test bound change from 97b8a03 that the regression test depends on.
This PR fixes five tests that fail intermittently when
go test ./...runs on a loaded Windows host. Four fixes change test files only. One changes production code. The Devin OAuth flow now retries a failed read of its callback file instead of ending the login.1. Incomplete request body test
TestServerRejectsIncompleteUnauthenticatedRequestBodiesPromptlyininternal/apifails ondevwithA goroutine dump at the failure showed the handler blocked in the
CreateFilesyscall inlogging.writeRequestInfoWithBody, before it wrote any response bytes. Under heavy parallel I/O, Windows stalls that file open for several seconds. The test allowed 500ms on the raw connection and 2s on the follow-up HTTP client.Both bounds are now 15s. The server sets no body read timeout, so a server that waits for the missing body still fails the test.
2. Antigravity blocked persistence test
TestAntigravityBlockedHintPersistenceDoesNotBlockPublicationOrOtherAccountsinsdk/cliproxyfails ondevwithThe last timeout branch closes
store.releaseSave, which the test already closed 22 lines earlier. A slow run panics there and the panic hides the real failure message.I removed the second close and raised the six 1s
time.Afterbounds in this test to 15s. Each of those selects already waits on a channel, so the bound only applies when the test fails. #6677 edits a different part of this test, andgit merge-treeof this branch with the #6677 head reports no conflict.3. OAuth status poll helper
pollOAuthStatusUntilDoneininternal/api/handlers/managementfails ondevwithFive tests use this helper. It requested the status endpoint in a loop with no pause and gave up after 5s. The OAuth flow it waits for checks its callback file every 500ms, and the busy loop took CPU from that goroutine.
The helper now waits on a 10ms ticker between requests and uses a 15s deadline timer, as
TestDevinRemoteOAuthFlowalready does. The session store has no completion signal a test could wait on.4. Devin OAuth callback read
TestDevinRemoteOAuthFlowfails withand the Devin step of
TestTargetedOAuthReloginClaudeXAIMetaDevinAndAuthIndexVerifyfails withThis one is a production bug. The callback handler writes a temp file and renames it into place. On Windows, a read of the destination path that lands during the rename fails with a sharing violation.
waitDevinOAuthCallbacktreated every read error except not-exist as fatal and set the session toFailed to read OAuth callback, so the login failed although the callback had arrived. A standalone program that reads a path while another goroutine renames a file onto it got 6775 of these errors in 3000 renames.waitDevinOAuthCallbacknow retries on the next 500ms tick for any read error. The Claude, Codex and Antigravity flows already do this. The context deadline and the session state still end the wait.TestWaitDevinOAuthCallbackRetriesTransientReadErrorcovers it and fails on the old code withtransient read error ended the login: Failed to read OAuth callback.TestDevinRemoteOAuthFlownow prints the session status when the exchange does not start, and its two 5s bounds are 15s.5. Watcher config reload test
TestHandleEventConfigChangeSchedulesReloadininternal/watcherfailed in an earlier full run withfollowed by a
TempDir RemoveAll cleanuperror onconfig.yaml. The test slept 400ms and then read the counter, which races the 150msconfigReloadDebouncetimer. When the timer fired late, the reload was still readingconfig.yamlduring cleanup.reloadCallbacknow signals a channel and the test waits on it. The test then takesconfigApplyMu, which the reload holds until it returns, so the reload has finished before the assertion and the cleanup. The sleep is gone.Verification
Windows 11, Go 1.27.2, 16 logical CPUs.
go test ./...on unmodifieddevat 05fd819 reproduced failures 1, 2 and 3 with the output quoted above.managementtests below each had 1 failure in 150 executions. Those are the two errors quoted in section 4.go build ./cmd/serverandgo veton the four touched packages pass.-count=50 -cpu=1,2,8, with the four packages running at the same time. All passed.internal/api, 1 test, 21.6ssdk/cliproxy, 1 test, 36.7sinternal/watcher, 1 test, 25.8sinternal/api/handlers/management, 9 tests, 474.5s. These areTestDevinRemoteOAuthFlow, bothTestWaitDevinOAuthCallback*tests,TestCompleteDevinOAuthFailures, the fourTestTargetedOAuthRelogin*tests andTestListAuthFiles_V8CodexTerminalRenewalRecoveryWorkflow_Issue5783.go test ./...ran twice on this branch. No test from this PR failed in either run.Both full runs still exit 1 on this host because of tests this PR does not touch. These six packages fail on unmodified
devtoo, in every run:cmd/server,TestTUIClientFailureExitsWithFailureandTestTUIStandaloneFailureExitsWithFailureinternal/home,internal/kvstoreandinternal/pluginhost, four tests withTempDir RemoveAll cleanuperrorsinternal/translator/claude/openai/responses,TestToolSearchDisabledMatchesDevGoldeninternal/util,TestInPlaceByteWritesAreReviewed, which Review weighted selector member copies in the no-copy guard #7467 coversRun 1 also had
TestServerResponsesWebsocketReauthorizesEachTurnfail on aTempDir RemoveAll cleanuperror, and twomanagementtests fail inwaitForOAuthStatus, a second status helper with a 5s deadline that this PR leaves alone.