Skip to content

Retry the registry record on transient failures - #151

Merged
jugonzalez12 merged 2 commits into
mainfrom
jugonzalez/record-release-retry
Oct 14, 2026
Merged

jugonzalez12 merged 2 commits into
mainfrom
jugonzalez/record-release-retry

Conversation

@jugonzalez12

Copy link
Copy Markdown
Contributor

The registry record step made one POST and failed the job on any answer other than 200 or 409. The registry validates a record by fetching each asset's signature and certificate files from the CDN, and a transient 503 there comes back as HTTP 400 with the upstream status in its message:

{"code":3, "message":"asset \"windows-amd64-msi\" detached signature
files are required: fetch certificate: GET .../name_v1.2.3_windows_amd64.msi.cert
returned HTTP 503"}

A release that was fully published then needed a manual re-run of the failed job, and a false "release failed" notification went out.

The POST now retries, re-sending the same body after 10, 20, 40 and 80 s, on a transport error, a 5xx, a 429, or a 400 whose message reports an upstream 5xx. Any other 4xx is final on the first attempt. The response handling, the flags and the 30 s per-request timeout are unchanged.

The registry record step made one POST and failed the job on any answer
other than 200 or 409. The registry validates a record by fetching each
asset's signature and certificate files from the CDN, and a transient
503 there comes back as HTTP 400 with the upstream status in its
message:

    {"code":3, "message":"asset \"windows-amd64-msi\" detached signature
    files are required: fetch certificate: GET .../name_v1.2.3_windows_amd64.msi.cert
    returned HTTP 503"}

A release that was fully published then needed a manual re-run of the
failed job, and a false "release failed" notification went out.

The POST now retries, re-sending the same body after 10, 20, 40 and 80 s,
on a transport error, a 5xx, a 429, or a 400 whose message reports an
upstream 5xx. Any other 4xx is final on the first attempt. The response
handling, the flags and the 30 s per-request timeout are unchanged.

Follow-up outside this repository: the registry API should answer 503 for
a failed validation fetch instead of 400; once it does, the body check
here can go.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit c25b05aea301

General PR Review: Retry the registry record on transient failures

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: full
View review run

Review Summary

The single POST in cmd/record-release is replaced by postWithRetry. It makes up to 5 attempts, waiting 10, 20, 40 and 80 s between them, and builds a fresh request with the same body each time. It retries on a transport error, a 5xx, a 429, or a 400 whose body contains returned HTTP 5. The existing 200/409/other handling in main is unchanged. I scanned the full PR diff (main.go, main_test.go, docs/release-workflow.md) for security and correctness and checked the code it touches: authTransport clones each request, so re-sending is safe. A retry after a lost-but-committed POST would get 409, which main already treats as success. The release step has no timeout-minutes, so the worst case of about 5 minutes fits. No repo-local criteria loaded, so I applied only the base criteria.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • New (medium confidence) cmd/record-release/main.go:375-378: if io.ReadAll(resp.Body) fails, the function returns immediately with no retry. That covers a connection reset mid-body, or the 30s client timeout expiring during the read. This doesn't match the stated "retries on a transport error" behavior.
  • New (low confidence, test gap) cmd/record-release/main_test.go:313-354: TestPostWithRetry covers only HTTP status codes. Nothing tests a transport error that is retried, or the "failed after N attempts" error after the last one. A regression in the err != nil branches of postWithRetry would go unnoticed.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `cmd/record-release/main.go`:
- Around line 375-378: In postWithRetry, an io.ReadAll error on the response body returns immediately ("reading response body") without retrying. Treat this like a transport error instead: keep the error in `err` and fall through to the retry/backoff logic. After the last attempt, return a wrapped error ("reading response body after %d attempts: %w") rather than returning on the first failure.

In `cmd/record-release/main_test.go`:
- Around line 313-354: Add a TestPostWithRetry case for transport errors. For example, use an httptest server whose handler hijacks and closes the connection on the first call and returns 200 on the second; assert that 2 calls happen and no error is returned. Add a second case where every attempt fails at the transport level; assert that postWithRetry returns a non-nil error mentioning "after 5 attempts".

Comment thread cmd/record-release/main.go Outdated
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit a506ced1d6cf

General PR Review: Retry the registry record on transient failures

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: full
View review run

Review Summary

The single POST in cmd/record-release is replaced by postWithRetry. It makes up to 5 attempts, waiting 10, 20, 40 and 80 s between them, and builds a fresh request with the same body each time. It retries on a transport error, a 5xx, a 429, or a 400 whose body contains returned HTTP 5. The existing 200/409/other handling in main is unchanged. I scanned the full PR diff (main.go, main_test.go, docs/release-workflow.md) for security and correctness and checked the code it touches: authTransport clones each request, so re-sending is safe. A retry after a lost-but-committed POST would get 409, which main already treats as success. The release step has no timeout-minutes, so the worst case of about 5 minutes fits. No repo-local criteria loaded, so I applied only the base criteria.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

  • New (medium confidence) cmd/record-release/main.go:375-378: if io.ReadAll(resp.Body) fails, the function returns immediately with no retry. That covers a connection reset mid-body, or the 30s client timeout expiring during the read. This doesn't match the stated "retries on a transport error" behavior.
  • New (low confidence, test gap) cmd/record-release/main_test.go:313-354: TestPostWithRetry covers only HTTP status codes. Nothing tests a transport error that is retried, or the "failed after N attempts" error after the last one. A regression in the err != nil branches of postWithRetry would go unnoticed.
Prompt for AI agents
Verify each finding against the current code and only fix it if needed.

## Suggestions

In `cmd/record-release/main.go`:
- Around line 375-378: In postWithRetry, an io.ReadAll error on the response body returns immediately ("reading response body") without retrying. Treat this like a transport error instead: keep the error in `err` and fall through to the retry/backoff logic. After the last attempt, return a wrapped error ("reading response body after %d attempts: %w") rather than returning on the first failure.

In `cmd/record-release/main_test.go`:
- Around line 313-354: Add a TestPostWithRetry case for transport errors. For example, use an httptest server whose handler hijacks and closes the connection on the first call and returns 200 on the second; assert that 2 calls happen and no error is returned. Add a second case where every attempt fails at the transport level; assert that postWithRetry returns a non-nil error mentioning "after 5 attempts".

Reviewed commit: c25b05aea301

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found — see the full review report

A connection dropped after the headers arrived surfaced as a read error
and ended the record attempt at once, while a drop before the headers
was retried. Both are the same transient failure; the read error now
falls through to the retry path, and after the last attempt is reported
with the attempt count like a failed request.

Tests add the transport cases: a drop before the headers then 200, a body
cut after the headers then 200, and five drops in a row ending in the
"after 5 attempts" error. Restoring the early return fails only the
body-cut case.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Superseded — see the current review report for commit a506ced1d6cf

General PR Review: Retry the registry record on transient failures

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since c25b05a
View review run

Review Summary

This PR wraps the registry record POST in postWithRetry. It waits 10/20/40/80s between attempts and retries on transport errors, 5xx, 429, or a 400 whose body contains returned HTTP 5. Any other 4xx fails on the first attempt. The new commit (a506ced) makes a body-read failure (io.ReadAll) count as a retryable transport error rather than failing at once. It also adds TestPostWithRetryTransportErrors, which covers three cases: a connection dropped before headers, a body cut short (unexpected EOF), and five drops in a row, which must end with the "after 5 attempts" error. I scanned the full PR diff (main.go, main_test.go, docs/release-workflow.md) for security and correctness and found nothing new. The incremental diff was complete, with no dropped or truncated paths. No repo-local criteria loaded, so I applied only the base criteria.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

  • Fixed: a failed body read returned immediately without a retry. At cmd/record-release/main.go:377-381 the ReadAll error now stays in err, so it goes through the retry and backoff path. After the last attempt it is wrapped as HTTP request failed after %d attempts (main.go:383-386).
  • Fixed: test gap, since transport errors were never tested. cmd/record-release/main_test.go:356-417 now covers a retried connection drop, a truncated body, and the final error after the last attempt.

@github-actions

github-actions Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

General PR Review: Retry the registry record on transient failures

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: none loaded - .claude/skills/ci-review.md was not found at trusted base ce5fc28e14cb.
Review mode: incremental since c25b05a
View review run

Review Summary

This PR wraps the registry record POST in postWithRetry. It waits 10/20/40/80s between attempts and retries on transport errors, 5xx, 429, or a 400 whose body contains returned HTTP 5. Any other 4xx fails on the first attempt. The new commit (a506ced) makes a body-read failure (io.ReadAll) count as a retryable transport error rather than failing at once. It also adds TestPostWithRetryTransportErrors, which covers three cases: a connection dropped before headers, a body cut short (unexpected EOF), and five drops in a row, which must end with the "after 5 attempts" error. I scanned the full PR diff (main.go, main_test.go, docs/release-workflow.md) for security and correctness and found nothing new. The incremental diff was complete, with no dropped or truncated paths. No repo-local criteria loaded, so I applied only the base criteria.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

  • Fixed: a failed body read returned immediately without a retry. At cmd/record-release/main.go:377-381 the ReadAll error now stays in err, so it goes through the retry and backoff path. After the last attempt it is wrapped as HTTP request failed after %d attempts (main.go:383-386).
  • Fixed: test gap, since transport errors were never tested. cmd/record-release/main_test.go:356-417 now covers a retried connection drop, a truncated body, and the final error after the last attempt.

Reviewed commit: a506ced1d6cf

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found — see the full review report

@jugonzalez12
jugonzalez12 requested a review from gontzess October 14, 2026 18:32
@jugonzalez12
jugonzalez12 merged commit f92102e into main Oct 14, 2026
2 checks passed
@jugonzalez12
jugonzalez12 deleted the jugonzalez/record-release-retry branch October 14, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants