fix: don't lose an upload when an image host is slow on one screenshot - #330
fix: don't lose an upload when an image host is slow on one screenshot#330nitrobass24 wants to merge 4 commits into
Conversation
min_successful_image_uploads is parsed from config and shipped in the example config, but nothing ever read it. Any single image an image host failed to accept failed the whole host batch, which records a tracker-scoped image-hosting failure and removes that tracker from the downstream set. With one selected tracker that empties the upload plan. Image hosts drop individual uploads under concurrency, and larger screenshots are dropped far more often, so a 2160p release could lose an otherwise complete upload to one refused image. Resolve the configured floor in internal/core, which is the only layer that sees reused plus newly published images for a target, and accept a partially failed host batch once the target has published at least that many images. Zero keeps the previous strict behavior, and a requested count below the floor cannot clear it.
…tracker set When image hosting fails for every downstream tracker, resolveDownstreamTrackerSet returned an empty set with a nil error. That set travels into the upload plan and only surfaces at contract validation as "upload dry run requires target tracker IDs", which names neither the stage that emptied it nor the cause. Fail with ErrInvalidTransition naming the blocked trackers when image hosting is what removed the last one. Sets that were already empty for other reasons are untouched.
📝 WalkthroughWalkthroughThe change adds a configurable minimum for successful image uploads. Qualifying partial uploads continue as successful. Upload failures that block every downstream tracker now return an invalid transition error listing the affected trackers. The upload timeout increases to 120 seconds. ChangesUpload handling and release eligibility
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant UploadFlow
participant ImageHostingService
participant ReleaseWorkflow
UploadFlow->>ImageHostingService: upload requested images
ImageHostingService-->>UploadFlow: published links and partial error
UploadFlow->>UploadFlow: validate minimum successful count
UploadFlow->>ReleaseWorkflow: provide upload result
ReleaseWorkflow->>ReleaseWorkflow: remove trackers with image-host failures
ReleaseWorkflow-->>UploadFlow: return invalid transition if all trackers are blocked
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/core/media_test.go`:
- Around line 453-461: Extend the table-driven test around the mediaModule setup
to exercise repository link reuse before a partial upload error: configure a
test case with reusable links and newly published links, ensure their combined
count satisfies ScreenshotHandlingConfig.MinSuccessfulUploads, and assert the
operation succeeds. Keep the existing direct-upload cases unchanged and retain
coverage for the repository-reuse partial-upload path.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 95dd9d89-eb6c-4e52-ab3e-df163913f4d1
📒 Files selected for processing (6)
internal/config/config.gointernal/config/defaults/example.yamlinternal/core/media.gointernal/core/media_test.gointernal/releaseworkflow/eligibility.gointernal/releaseworkflow/eligibility_test.go
UploadTimeout is one constant shared by every image host in the uploader registry, and the 60s whole-request deadline was too tight for all of them. The same timeout failure was seen on pixhost and imgbox before reelflix hosting was configured, so this is not one host being slow. The deadline is not a transfer budget. The failure looks like: Post "https://<host>/api/1/upload": context deadline exceeded (Client.Timeout exceeded while awaiting headers) "while awaiting headers" means the multipart body was already sent and the deadline expired waiting on the host to answer, so what the ceiling has to cover is host-side processing of a 5-8MB image. Every host is slower at that than at a 1080p screenshot, which is why this shows up as a 2160p problem across hosts rather than a per-host one. imgbox spends the same budget three times per batch: its CSRF fetch and token generation each clone the client at UploadTimeout before the upload itself runs, so a slow host burns the ceiling on requests that publish nothing. The batch with recorded timings reported mean_attempt_duration=42.02s across 6 attempts, and since attemptDurations records failed attempts too, the 5 successes averaged around 38s. That is barely 1.5x headroom for an operation whose duration is set by how fast a remote host answers. 120s restores roughly 3x. UploadTimeout is consumed only by internal/imagehosting; every tracker upload path uses DefaultTimeout, so no other deadline widens.
…llowance The floor lives in internal/core because that is the only layer that sees reused plus newly published images for a target. Two cases pin that: a host that publishes fewer images than the floor still succeeds once reuse makes up the difference, and reuse that is still short of the floor fails. Applying the floor in internal/imagehosting, which only receives the missing subset, would fail the first case.
fb22fbd to
92df420
Compare
One image an image host refuses aborts the entire release upload, even when
min_successful_image_uploadsis satisfied. The workflow dies atreview-uploadswithupload dry run requires target tracker IDs, which describes an empty tracker set rather than the actual cause.Observed with
screens: 6,min_successful_image_uploads: 3:Same failure on the host-fallback path, so it is not specific to one host or to fallback:
The dropped image was a client timeout waiting on the host:
while awaiting headersmeans the request body was already sent and the deadline expired waiting on the host to answer, so this is host-side processing time rather than local transfer. The same timeout has been seen on pixhost and imgbox as well, so it is not specific to any one host.Cause
Three separate defects.
1.
min_successful_image_uploadsis never read. It is parsed atinternal/config/config.go:151and shipped ininternal/config/defaults/example.yaml, but the only other references in the tree are a persistence test and the e2e harness fixture.internal/imagehosting/service.go:707fails the batch whenever any single image failed, regardless of the configured floor. The successfully published links are returned alongside that error and are still usable, but the error alone drives everything downstream.2. Any image-hosting failure removes the tracker from the downstream set.
internal/releaseworkflow/eligibility.go:317-321, for the descriptions and upload stages:With a single selected tracker,
baseempties,resolveDownstreamTrackerSetreturns an empty set and a nil error, and the emptyTrackerIDsonly fails much later atpkg/api/workflow_contracts_validation.go:1283.The filter does not distinguish a total host outage from a host that refused one image out of six, even though
internal/releaseworkflow/module.go:1129-1141already models that distinction asStageStatusPartial, andTrackerImageHostFailure's doc comment describes it as a terminal failure.Reproduced against
resolveDownstreamTrackerSetwith a media snapshot carrying five hosted artifacts and one image-hosting failure for the same tracker:3. The image-host deadline leaves almost no headroom.
internal/httpclient/httpclient.go:14sets a fixed 60s whole-request deadline, andUploadTimeoutis one constant shared by every host in the uploader registry. The batch above reportedmean_attempt_duration=42.02sacross 6 attempts, and sinceattemptDurationsrecords failed attempts too, the 5 successes averaged around 38s. That is barely 1.5x headroom for an operation whose duration is set by how fast a remote host answers.imgbox spends that same budget three times per batch:
imgboxGetCsrfAndCookieand the token generation each clone the client atUploadTimeoutbefore the upload itself runs, so a slow host burns the ceiling on requests that publish nothing.Impact
Every host is slower to process a 5-8MB image than a 1080p screenshot, so this shows up as a 2160p problem across hosts rather than a per-host one. In a 25 item batch of mixed content every 1080p release succeeded and three 2160p releases failed this way. Since one dropped image is enough, a complete and otherwise valid upload is lost, and the surfaced error gives no indication that image hosting was involved.
Changes
Four commits, each can be backed out alone.
fix(core): honor min_successful_image_uploads when a host drops uploadsinternal/config/config.go:ResolvedMinSuccessfulUploads(), matching the existingResolvedMaxMenuItems()idiom. Zero disables the allowance and keeps the previous strict behavior.internal/core/media.go:partialHostUploadIsUsableaccepts a partially failed host batch once the target has published at least the floor. This lives ininternal/coredeliberately, since it is the only layer that sees reused plus newly published images for a target.internal/imagehostingonly receives the missing subset, so a threshold there would under-count reuse and fail a target that already has enough images.internal/config/defaults/example.yaml: comment describing what the knob does.fix(releaseworkflow): name image-host exhaustion instead of an empty tracker setinternal/releaseworkflow/eligibility.go: when image hosting is what removed the last downstream tracker, fail withErrInvalidTransitionnaming the blocked trackers instead of returning an empty set that only breaks at contract validation. Sets that were already empty for other reasons are untouched.fix(httpclient): raise image-host upload deadline to 120sinternal/httpclient/httpclient.go: 60s to 120s, restoring roughly 3x headroom over the observed host response time.UploadTimeoutis consumed only byinternal/imagehosting; every tracker upload path usesDefaultTimeout, so no other deadline widens.test(core): cover repository link reuse in the partial image-upload allowanceinternal/core: a host that publishes fewer images than the floor still succeeds once repository reuse makes up the difference, and reuse still short of the floor fails. Applying the floor ininternal/imagehosting, which only receives the missing subset, would fail the first.Notes for review
max_concurrent_uploads: 1and 6 screenshots that is a 12 minute worst case per release instead of 6. Commit 1 limits the damage, since the release no longer dies from it.refreshMutatedMediaStatusviarequiredScreenshots, so that gate is intact, but you may prefer the floor to interact with tracker requirements rather than sit beside them.Verification
make test-gofull race suite greenmake lint0 issuesmake logpolicy,make pathpolicy,make gofix-check-changed,git diff --checkcleanSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Improvements