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

Merged

benvin merged 5 commits from benvin/github-asset-fail-etag into master 2026-10-18 02:33:01 +11:00

Conversation 4 Commits 5 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 ... 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 ... 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 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 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.

unkin-agent referenced this pull request 2026-10-17 12:31:56 +11:00

Add local cargo registry type #144

unkin-agent added 1 commit 2026-10-17 12:55:43 +11:00

merge master 238118ea12

benvin merged commit d88a047788 into master 2026-10-18 02:33:01 +11:00

benvin deleted branch benvin/github-asset-fail-etag 2026-10-18 02:33:01 +11:00

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

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: unkin/artifactapi#135