fix(core): restore FinishJob retries on transport errors

[ivannalisetska]

• Only stop finish retries on 401/422 API responses
• Keep retrying unknown transport errors such as http2 connection lost

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.

[buildsworth-bk-app]

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.

[isaacsu]

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 api.BreakOnNonRetryable and just adding http2: client connection lost as retriable could be sufficient to solve the customer issue while maintaining the desired current behaviour for Rows 2-4 and 6?