unkin/artifactapi
3
0
Fork 0
Code Issues 5 Pull Requests 2 Actions Packages Projects Releases 1 Wiki Activity

Fail GitHub scans on asset errors and drop sync state with its remote #135

Open
unkin-agent wants to merge 4 commits from benvin/github-asset-fail-etag into master
pull from: benvin/github-asset-fail-etag
merge into: unkin:master
Conversation 4 Commits 4 Files Changed 13
+502 -51
unkin-agent commented 2026-10-09 23:29:10 +11:00
Member
Copy Link
Copy Source

A GitHub release scan that hit a transient asset error still saved the release-list ETag, so the failed asset was never retried. Assets that are corrupt, deleted or truncated upstream fail on every retry, so they must not keep the remote failed.

  • fail the rpm/deb/alpine scan on transient asset fetch, short read or metadata insert errors, so the ETag is not saved and the backoff retry re-derives the asset
  • add provider.ErrInvalidPackage for unparseable packages and 404/410/416 asset downloads; such assets are logged and skipped
  • decide a ranged fetch holds the whole object from Content-Length or the Content-Range total, or a clean EOF when the total is unknown
  • add a NOT VALID cascading FK from the GitHub sync-state tables to remotes
A GitHub release scan that hit a transient asset error still saved the release-list ETag, so the failed asset was never retried. Assets that are corrupt, deleted or truncated upstream fail on every retry, so they must not keep the remote failed. - fail the rpm/deb/alpine scan on transient asset fetch, short read or metadata insert errors, so the ETag is not saved and the backoff retry re-derives the asset - add `provider.ErrInvalidPackage` for unparseable packages and 404/410/416 asset downloads; such assets are logged and skipped - decide a ranged fetch holds the whole object from Content-Length or the Content-Range total, or a clean EOF when the total is unknown - add a NOT VALID cascading FK from the GitHub sync-state tables to `remotes`
unkin-agent added 1 commit 2026-10-09 23:29:11 +11:00
Fail GitHub scans on asset errors and drop sync state with its remote
ci/woodpecker/pr/pre-commit Pipeline was successful
Details
ci/woodpecker/pr/test Pipeline was successful
Details
ci/woodpecker/pr/build Pipeline was successful
Details
b111c2e57f 
- fail the scan when a release asset fails to derive or insert, so the ETag is kept and the retry re-derives it
- cascade sync-state rows on remote delete (migration 0003)
unkin-agent commented 2026-10-09 23:39:51 +11:00
Author
Member
Copy Link
Copy Source
  • internal/provider/{rpm,deb,alpine}/github.go (derive-error branch in scanWithState) — a permanently failing asset (corrupt rpm → "parse rpm header") fails every scan forever: etag never saved, so no 304 ever, each retry (every ≤ttl/4) does a full releases fetch plus header range requests for the broken asset, and the remote stays in failed state. New releases are still recorded (loop continues) → fail the scan only for transient errors (fetch/insert); log and skip permanent parse errors, or cap retries per asset.
  • migrations/0003_github_sync_state_cascade.sql:3 — NOT VALID leaves existing orphan rows, so a recreated remote still inherits their stale ETag (the bug this fixes) → DELETE orphans first (DELETE FROM github_*_sync_state WHERE remote_name NOT IN (SELECT name FROM remotes)), then the constraint can be validated.
  • nit: internal/provider/rpm/syncer_test.go — only the rpm path tests the failed-asset-fails-scan behaviour; deb and alpine changes are untested → add a failing-asset case for each.
- internal/provider/{rpm,deb,alpine}/github.go (derive-error branch in scanWithState) — a permanently failing asset (corrupt rpm → "parse rpm header") fails every scan forever: etag never saved, so no 304 ever, each retry (every ≤ttl/4) does a full releases fetch plus header range requests for the broken asset, and the remote stays in failed state. New releases are still recorded (loop continues) → fail the scan only for transient errors (fetch/insert); log and skip permanent parse errors, or cap retries per asset. - migrations/0003_github_sync_state_cascade.sql:3 — NOT VALID leaves existing orphan rows, so a recreated remote still inherits their stale ETag (the bug this fixes) → DELETE orphans first (`DELETE FROM github_*_sync_state WHERE remote_name NOT IN (SELECT name FROM remotes)`), then the constraint can be validated. - nit: internal/provider/rpm/syncer_test.go — only the rpm path tests the failed-asset-fails-scan behaviour; deb and alpine changes are untested → add a failing-asset case for each.
unkin-agent added 1 commit 2026-10-09 23:47:33 +11:00
Skip invalid GitHub packages instead of failing the scan
ci/woodpecker/pr/pre-commit Pipeline was successful
Details
ci/woodpecker/pr/test Pipeline was successful
Details
ci/woodpecker/pr/build Pipeline was successful
Details
d9f9fd3bca 
Wrap package parse errors in provider.ErrInvalidPackage so only transient
asset and insert errors fail the scan; cover rpm, deb and alpine.
unkin-agent commented 2026-10-09 23:51:24 +11:00
Author
Member
Copy Link
Copy Source
  • internal/provider/rpm/github.go:608 (same in deb/github.go:607, alpine/github.go rangeGet; also computeSHA256 status check) — a permanent asset HTTP status (404/410 on a listed asset, 403) is returned unwrapped, so it is treated as transient and fails the whole remote scan on every retry; the ETag never advances and healthy releases are re-scanned until the release list drops the asset → wrap 404/410 (and 416) from the asset fetch as permanent (or a sibling sentinel) and skip, keep 5xx/429/network transient.
  • internal/provider/rpm/github.go:608 (deb/alpine same) — full := len(body) < n is the only "whole object" signal; a clean short body (proxy/CDN closing early with no transport error) is taken as the full file, then reported as ErrInvalidPackage (rpm truncated header, deb/alpine "not found within") and skipped permanently → compare against Content-Range total / Content-Length (or require 206 total <= n) before setting full; otherwise return a plain error.
  • internal/provider/{rpm,deb,alpine}/*syncer_test.go — the new tests never hit the metadata-insert failure branch (failed = append after InsertXMetadata) and never assert errors.Is(err, provider.ErrInvalidPackage) for the other wrapped paths (deb "control missing Package", alpine "missing pkgname/arch", header-size-exceeded) → add a failing-inserter case asserting retry, and table cases per wrapped branch.
  • nit: internal/provider/rpm/github.go:346 (and deb/alpine) — the same 2-line comment is copy-pasted three times; keep it once on ErrInvalidPackage in syncretry.go.
- internal/provider/rpm/github.go:608 (same in deb/github.go:607, alpine/github.go rangeGet; also computeSHA256 status check) — a permanent asset HTTP status (404/410 on a listed asset, 403) is returned unwrapped, so it is treated as transient and fails the whole remote scan on every retry; the ETag never advances and healthy releases are re-scanned until the release list drops the asset → wrap 404/410 (and 416) from the asset fetch as permanent (or a sibling sentinel) and skip, keep 5xx/429/network transient. - internal/provider/rpm/github.go:608 (deb/alpine same) — `full := len(body) < n` is the only "whole object" signal; a clean short body (proxy/CDN closing early with no transport error) is taken as the full file, then reported as ErrInvalidPackage (rpm truncated header, deb/alpine "not found within") and skipped permanently → compare against Content-Range total / Content-Length (or require 206 total <= n) before setting full; otherwise return a plain error. - internal/provider/{rpm,deb,alpine}/*syncer_test.go — the new tests never hit the metadata-insert failure branch (`failed = append` after InsertXMetadata) and never assert `errors.Is(err, provider.ErrInvalidPackage)` for the other wrapped paths (deb "control missing Package", alpine "missing pkgname/arch", header-size-exceeded) → add a failing-inserter case asserting retry, and table cases per wrapped branch. - nit: internal/provider/rpm/github.go:346 (and deb/alpine) — the same 2-line comment is copy-pasted three times; keep it once on ErrInvalidPackage in syncretry.go.
unkin-agent added 1 commit 2026-10-09 23:57:44 +11:00
Skip missing GitHub assets and treat short reads as transient
ci/woodpecker/pr/pre-commit Pipeline was successful
Details
ci/woodpecker/pr/build Pipeline was successful
Details
ci/woodpecker/pr/test Pipeline was successful
Details
09ab49ef42
unkin-agent commented 2026-10-10 00:01:06 +11:00
Author
Member
Copy Link
Copy Source
  • internal/provider/syncretry.go:ReadPrefix — a 200 with unknown length (ContentLength -1, chunked) whose body is shorter than n gets total=-1, so full=false and it returns "short read" → a small complete asset served chunked fails the scan on every retry (regression: old code treated len<n as full). Treat ContentLength<0 on a 200 (and Content-Range "/*") with a clean EOF as full; add a test with a chunked 200.
- internal/provider/syncretry.go:ReadPrefix — a 200 with unknown length (ContentLength -1, chunked) whose body is shorter than n gets total=-1, so full=false and it returns "short read" → a small complete asset served chunked fails the scan on every retry (regression: old code treated len<n as full). Treat ContentLength<0 on a 200 (and Content-Range "/*") with a clean EOF as full; add a test with a chunked 200.
unkin-agent added 1 commit 2026-10-10 01:10:21 +11:00
Treat a clean EOF with unknown total as the whole asset
ci/woodpecker/pr/test Pipeline was successful
Details
ci/woodpecker/pr/pre-commit Pipeline was successful
Details
ci/woodpecker/pr/build Pipeline was successful
Details
fdbf322ca8
unkin-agent commented 2026-10-10 01:12:16 +11:00
Author
Member
Copy Link
Copy Source
  • internal/provider/syncretry.go:84 — the comment "net/http errors on a premature close" only holds for chunked and Content-Length bodies. A 200 with no Content-Length and no chunking (HTTP/1.0 or close-delimited) also gives ContentLength -1, and a mid-body connection drop there is a clean EOF, so it now counts as the whole object → nit: reword the comment to say chunked only, or accept the risk explicitly.
  • internal/provider/syncretry_test.go:88 — nit: no case where the chunked stream is truncated mid-body (handler hijacks and closes before the terminating chunk). That case is what backs the "premature close errors" assumption → add one wantErr case.
- internal/provider/syncretry.go:84 — the comment "net/http errors on a premature close" only holds for chunked and Content-Length bodies. A 200 with no Content-Length and no chunking (HTTP/1.0 or close-delimited) also gives ContentLength -1, and a mid-body connection drop there is a clean EOF, so it now counts as the whole object → nit: reword the comment to say chunked only, or accept the risk explicitly. - internal/provider/syncretry_test.go:88 — nit: no case where the chunked stream is truncated mid-body (handler hijacks and closes before the terminating chunk). That case is what backs the "premature close errors" assumption → add one wantErr case.
All checks were successful
ci/woodpecker/pr/test Pipeline was successful
Required
Details
ci/woodpecker/pr/pre-commit Pipeline was successful
Required
Details
ci/woodpecker/pr/build Pipeline was successful
Required
Details
This pull request can be merged automatically.
You are not authorized to merge this pull request.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin benvin/github-asset-fail-etag:benvin/github-asset-fail-etag
git checkout benvin/github-asset-fail-etag
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
benvin (BenVincent) unkin-agent (Unkin Agent) unkinben (Ben Vincent)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: unkin/artifactapi#135
Delete Branch "benvin/github-asset-fail-etag"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?