Make parallel chunk requests conditional on the probe's ETag
What does this MR do?
Sends the ETag of the bytes=0-0 probe in If-Match on every chunk request of the parallel cache download from a presigned URL and of the parallel artifact download, when that ETag is strong. A store that replaced the object after the probe answers 412 Precondition Failed, which fails the parallel download, and the existing fallback downloads the object with one GET: the cache extractor makes its sequential GET, as it does for a chunk answered with 200, and DownloadArtifacts uses its single download stream, as it does for any chunk that fails. A weak ETag never matches If-Match, so none is sent for it, and a store that sends no ETag gets the same chunk requests as before.
This MR builds on !7676 (merged), which is merged. It is rebased onto main, so its commits are its own: "Make parallel chunk requests conditional on the probe's ETag", the one that adds integration tests, and the change from review.
Why was this MR needed?
With FF_USE_PARALLEL_CACHE_TRANSFER or FF_USE_PARALLEL_ARTIFACT_TRANSFER enabled, every chunk is fetched with its own Range GET, and none of these requests said which version of the object it expected. When the object is replaced while the chunks are in flight, for example by a job that uploads the same cache key, the chunks requested after that come from the new version. Each answer is a correct 206 for its range, so the download succeeded with a file that matches neither version, and on the cache path the archive keeps the probe's Last-Modified, so a later run takes it as up to date. The cache probe read the ETag but only logged it (cache_extractor.go#L314, #L546), the artifact probe did not read it, and both chunk fetchers sent only Range (cache_extractor.go#L357, gitlab.go#L1219). The sequential download makes one GET and cannot mix versions.
The review of !7635 (merged) checked against SeaweedFS 3.85 and LocalStack 4.4.0 that a presigned GET accepts an unsigned If-Match header and answers 206 when it matches and 412 when it is stale, so the header works with presigned URLs. The integration tests of the second commit check it again.
transfer.StrongETag decides whether an ETag can go in If-Match: a quoted tag without the W/ prefix (RFC 9110, section 8.8.3). Both fetchers use it, so the rule is in one place.
Why it builds on !7676 (merged): on the artifact path the single download stream writes into the file the parallel attempt used. Without !7676 (merged), which empties that file first, a new version shorter than the old one leaves the end of the failed attempt after it, so the 412 would trade a file that mixes two versions for one that holds the new version and a tail of the old one. The two commits also change the same signature and call in network/gitlab.go, so on separate branches the second to merge would conflict.
Not covered here: the GoCloud path (S3 with RoleARN, Azure) reads its chunks through the driver and has no probe. Making those reads conditional needs ReaderOptions.BeforeRead for each driver, which I left out of this change.
What's the best way to test this MR?
The cache test has four cases: a strong ETag, a strong ETag with the object replaced after the first chunks, a weak ETag, and no ETag. The artifact test has those four and a fifth, with the object replaced by a shorter one (1024 bytes). For a replaced object, the httptest server answers the later chunks and a GET without Range from a new version with another ETag, and answers 412 to a stale If-Match.
TestCacheExtractorPresignedParallelTransferIfMatchincommands/helpers: every chunk request carriesIf-Matchwith the probe's strongETag, and none does in the other two cases. With the object replaced, the extractor makes one sequential GET and the archive is the new version.TestDownloadArtifactsParallelIfMatchinnetwork: the same throughDownloadArtifactswithFF_USE_PARALLEL_ARTIFACT_TRANSFER, with one GET withoutRangeand a file equal to the new version when the object is replaced, the shorter one included.TestStrongETaginhelpers/transfer.
go test -race -count=1 -run 'TestCacheExtractorPresignedParallelTransfer|TestDownloadArtifactsParallel|TestStrongETag' ./commands/helpers/ ./network/ ./helpers/transfer/With the code before the first commit (main with !7676 (merged)), the strong-ETag cases of both tests fail: no chunk carries If-Match, and with the object replaced the download succeeds without a GET without Range and leaves a file that mixes both versions. The weak and missing ETag cases pass there by design: they guard the behavior that must not change. With the first commit and without !7676 (merged), the shorter replacement fails: the file keeps 16777216 bytes where the new version has 1024. Sending any ETag, weak ones included, fails the weak-ETag case of both tests (and TestStrongETag), and treating a 412 on the cache path like any other failed chunk fails the replaced-object case of the cache test.
The second commit adds integration tests that Lachlan Grant wrote for the review of !7635 (merged) (note), where two of them failed on the gaps that this MR and the MR for #40108 (closed) close. As in that review, they use the integration build tag, read their endpoints from RANGE_IT_* variables and skip when those are unset, so CI skips them:
commands/helpers/cache_extractor_if_match_integration_test.go: the object replaced after the first chunk of a cache download, now with the archive required on disk, and a presigned GET with a matching and with a staleIf-Match.commands/helpers/cache_extractor_range_integration_test.go, already onmainfrom !7679 (merged), the MR for #40108 (closed): the helpers, and the tests that every fix of the parallel cache download keeps passing.network/gitlab_artifacts_range_integration_test.go: the parallel artifact download from a real store, directly and behind a load balancer that dropsRange. With this MR every chunk request there carriesIf-Match, and the direct download has to finish on the parallel path.
I ran them against SeaweedFS 3.85 and LocalStack 4.4.0 behind nginx 1.27.5 with the configuration of that note, three times on each store. With the code of !7676 (merged), TestPresignedParallelDownloadObjectReplacedDuringDownload fails every time on both stores, with an archive that matches neither version. With this MR the chunks after the replacement are answered 412, the extractor makes its sequential GET, and every test passes. The caching proxy of that configuration is in front of SeaweedFS only, so its test skips in the LocalStack run. After the review I ran them again on SeaweedFS 3.85 without nginx, so the four tests that need it skipped; the two of cache_extractor_if_match_integration_test.go ran three times, and every test that ran passes. I did not run them on LocalStack again.
I ran go test -race -count=1 on ./commands/helpers/, ./network/, ./helpers/transfer/ and ./commands/helpers/meter/, GOOS=linux, GOOS=windows and GOOS=darwin go vet on those packages, make lint in the runner-linters:2.12.2-go1.26.5 image the go:lint job uses (0 issues., test directives included), and the commands of the compile tests jobs for Linux and Windows. make lint does not build the integration files, so I also ran golangci-lint with the integration tag on ./commands/helpers/ and ./network/: no issues in the new files. After the rebase onto main I ran, in golang:1.26.10, go build ./..., go vet on ./commands/helpers/, ./helpers/transfer/ and ./network/ for linux, windows and darwin and with the integration tag on ./commands/helpers/ and ./network/, go test -race -count=1 on the three packages, the command above with -count=20, make check_test_directives, and golangci-lint 2.14.0 fmt --diff and run on the three packages (0 issues.; no issues in the new integration files with the integration tag).
Test output without this change
With the code of !7676 (merged) (main at 5d62e0cc and the commit of !7676 (merged)):
--- FAIL: TestCacheExtractorPresignedParallelTransferIfMatch
--- FAIL: .../strong_ETag
Error: Not equal: expected: 23, actual: 0
Messages: every chunk request should carry If-Match "\"v1\""
--- FAIL: .../strong_ETag,_object_replaced_during_the_download
Error: Not equal: expected: 23, actual: 0
Messages: every chunk request should carry If-Match "\"v1\""
Error: Not equal: expected: 1, actual: 0
Messages: sequential GETs
Error: Not equal: expected: []byte{0x32, 0x32, 0x32, ...}, actual: []byte{0x50, 0x4b, 0x3, ...}
--- PASS: .../weak_ETag
--- PASS: .../no_ETag
--- FAIL: TestDownloadArtifactsParallelIfMatch
--- FAIL: .../strong_ETag
Messages: every chunk request should carry If-Match "\"v1\""
--- FAIL: .../strong_ETag,_object_replaced_during_the_download
Messages: every chunk request should carry If-Match "\"v1\""
Messages: GETs without Range
Messages: the file is not the expected version of the object (33555456 bytes, want 33555456)
--- FAIL: .../strong_ETag,_object_replaced_by_a_shorter_one_during_the_download
Messages: every chunk request should carry If-Match "\"v1\""
Messages: GETs without Range
Messages: the file is not the expected version of the object (33555456 bytes, want 1024)
--- PASS: .../weak_ETag
--- PASS: .../no_ETagIn the cache case the new version is all 2 bytes (0x32), and the archive starts with the old version's zip header (PK).
With the first commit on main alone, without the commit of !7676 (merged):
--- FAIL: TestDownloadArtifactsParallelIfMatch
--- PASS: .../strong_ETag
--- PASS: .../strong_ETag,_object_replaced_during_the_download
--- FAIL: .../strong_ETag,_object_replaced_by_a_shorter_one_during_the_download
Messages: the file is not the expected version of the object (16777216 bytes, want 1024)
--- PASS: .../weak_ETag
--- PASS: .../no_ETagWith this MR:
ok gitlab.com/gitlab-org/gitlab-runner/network
ok gitlab.com/gitlab-org/gitlab-runner/commands/helpers
ok gitlab.com/gitlab-org/gitlab-runner/helpers/transfer
ok gitlab.com/gitlab-org/gitlab-runner/commands/helpers/meterEnvironment: Linux amd64, Go 1.26.5 (from go.mod).
Integration test output
go test -count=1 -v -tags integration on ./commands/helpers/ and ./network/, with the code of !7676 (merged) and the three new files, on SeaweedFS (LocalStack fails the same test with the same message, and skips the caching proxy):
Error: corrupt archive on disk
Messages: archive has 8400953 bytes and matches no expected object
--- FAIL: TestPresignedParallelDownloadObjectReplacedDuringDownload
--- PASS: TestPresignedRangeRequestHonorsIfMatch
--- PASS: TestPresignedParallelDownloadRealStore
--- PASS: TestPresignedParallelDownloadRangeDroppingLoadBalancer
--- PASS: TestPresignedParallelDownloadRangeBlindCachingProxy
--- PASS: TestPresignedDownloadServerWithoutRangeSupport
--- PASS: TestGoCloudParallelDownloadRealStore
--- PASS: TestDownloadArtifactsParallelRealStore
--- PASS: TestDownloadArtifactsParallelRangeDroppingLoadBalancerWith this MR, SeaweedFS:
level=info msg="Cache object changed during the parallel download, using sequential download" error="range request failed: 412 Precondition Failed: object changed during the parallel download"
--- PASS: TestPresignedParallelDownloadObjectReplacedDuringDownload
--- PASS: TestPresignedRangeRequestHonorsIfMatch
--- PASS: TestPresignedParallelDownloadRealStore
--- PASS: TestPresignedParallelDownloadRangeDroppingLoadBalancer
--- PASS: TestPresignedParallelDownloadRangeBlindCachingProxy
--- PASS: TestPresignedDownloadServerWithoutRangeSupport
--- PASS: TestGoCloudParallelDownloadRealStore
ok gitlab.com/gitlab-org/gitlab-runner/commands/helpers
level=info msg="Artifact download: parallel, 16 streams, 16 MiB chunk size" status=302 url="http://seaweed:8333/runner-range-it/TestDownloadArtifactsParallelRealStore-....zip"
--- PASS: TestDownloadArtifactsParallelRealStore
--- PASS: TestDownloadArtifactsParallelRangeDroppingLoadBalancer
ok gitlab.com/gitlab-org/gitlab-runner/networkLocalStack answers the stale chunks with 412 PRECONDITION FAILED, skips the caching proxy, and the rest is the same.
Environment: Linux amd64, Go 1.26.5, SeaweedFS 3.85 with one identity, LocalStack 4.4.0 with SERVICES=s3, nginx 1.27.5, each in Docker on one network.
What are the relevant issue numbers?
Closes #40109
Related to #40108 (closed), whose MR (!7679 (merged)) added cache_extractor_range_integration_test.go to main.