fix(core): restore FinishJob retries on transport errors#4089
fix(core): restore FinishJob retries on transport errors#4089ivannalisetska wants to merge 2 commits into
Conversation
- Only stop finish retries on 401/422 API responses - Keep retrying unknown transport errors such as http2 connection lost SUP: #7554 Co-authored-by: Cursor <cursoragent@cursor.com>
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn’t find any code-review issues in the diff. This changes the core job-finish retry path, so I’m leaving this as a comment rather than an approval.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 5355, then answer my questions about the findings.
I think this is what your change will result in (the yellow rows). If I understand correctly, the only change we really want is Row 10. Rows 2-4 and Row 6 would be regressions unless we truly consider those status codes to be retriable in this specific case. I think keeping the current use of |
There was a problem hiding this comment.
Agent sweep drives-by.
I agree with @isaacsu's feedback.
We should broadly consider http2: client connection lost transient in the shared error classification path.
There was a problem hiding this comment.
Updated with :
Keep BreakOnNonRetryable in FinishJob (avoids regressions on 402–421 / 423–428)
Classify http2: client connection lost as retriable in the shared error path
Also removed the old "Buildkite rejected the call to finish the job" log from the earlier approach.
- Keep FinishJob on BreakOnNonRetryable to avoid retrying non-retryable 4xx - Treat http2: client connection lost as a transient transport error - Add shared retry classification tests SUP: #7554 Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
I didn’t find any code-review issues. Adding this transport error to the shared classifier changes retry behavior for all of its API callers rather than only FinishJob, so that broader runtime impact needs a human sanity-check rather than an AI approval.
Want to dig deeper?
The full session log is attached to this Buildkite build. Download the session file and open a new pi session with it:
Download the buildsworth logs from build 6461, then answer my questions about the findings.
There was a problem hiding this comment.
Since this PR is no longer about fixing this one endpoint anymore, this test case might not be needed either.
And the PR title/description can use a bit update too after the direction change 🙏🏿
4 participants
SUP: #7554
Description
Successful jobs can exit 0 but then fail the Agent API finish call with a transient transport error such as http2: client connection lost. After the retry-policy cleanup in 3.124.0, FinishJob used BreakOnNonRetryable, which treated that error as non-retryable and gave up after one attempt. The agent then idled/disconnected and the job showed as agent lost (-1).
This PR restores the previous FinishJob retry behavior: only stop on 401/422 responses, and keep retrying unknown transport errors.
Alternative considered: adding http2: client connection lost to the shared IsRetryableError allowlist. That would help other API callers, but would not restore the original finish semantics (retry any non-401/422 failure). Finish is critical enough to keep the more resilient path.
Context
SUP-7554
Customer report after upgrade from agent 3.114.1 → 3.130.0: finish fails with http2: client connection lost, then idle disconnect marks the job agent lost (-1)
Changes
Restore FinishJob retry break conditions to 401/422 only in core/client.go
Add regression tests for retrying http2: client connection lost and not retrying 401
Testing
Tests have run locally (with go test ./...). Buildkite employees may check this if the pipeline has run automatically.
Disclosures / Credits
I compared the previous FinishJob retry logic with the post-cleanup version. AI helped revert the deleted behavior and add regression tests.