[History Server] Cover GetContent object paths for the s3 and azureblob backends - #5075
Open
stantheman0128 wants to merge 1 commit into
Open
Conversation
…ob backends Neither backend had any test touching GetContent. s3_test.go and azureblob_test.go held only TestTrim and TestWalk, which assert nothing and never construct a handler, so the object path that GetContent builds from rootDir, cluster prefix and file name was unverified on both. That path is easy to get wrong: the aliyunoss backend shipped without the root dir in it (ray-project#4820), and the same mistake in either of these would break every log fetch on a deployment that configures a root dir. Add two tests per backend, driving the real SDK against an httptest server so the assertion is on the path that reaches the wire rather than on a helper's return value. s3 uses path style addressing, which the MinIO support already relies on. The second test in each pair covers the list-and-retry fallback, whose listing prefix has to be rooted as well. No production code changes.
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why are these changes needed?
GetContentbuilds its object path from three pieces,rootDir, the cluster path prefix, and the file name. Neither the s3 nor the azureblob backend had a single test touching that.pkg/storage/s3/s3_test.goandpkg/storage/azureblob/azureblob_test.gocontained onlyTestTrimandTestWalk, which never construct a handler, never call a storage method, and contain no assertion that can fail.This is not a hypothetical gap. The aliyunoss backend shipped without the root dir in that path, which is #4820: on a deployment started with
--ray-root-dir, listing the log tree worked and every fetch from it returnedNoSuchKey. The same mistake in s3 or azureblob would produce the same silent breakage, and nothing in the suite would notice.This PR adds two tests per backend. There is no production change.
How the tests work
Each test drives the real SDK against an
httptestserver, so the assertion is on the object path that actually reaches the wire rather than on a helper's return value. For s3 that means path style addressing, which the backend already supports for MinIO. For azureblob the container client is pointed at the test server with no credential.The first test in each pair pins the full object path. The second covers the list-and-retry fallback, whose listing prefix has to be rooted as well, otherwise the retry has nothing to find.
The two backends need slightly different fallback setups because their matching rules differ. s3 matches a listed candidate by base name, so the test puts the object one directory deeper. azureblob matches on the full path, so its test misses the first download instead. That one uses
BlobNotFoundrather than a server error on purpose: the SDK retries server errors, which would satisfy the download before the fallback ever ran.Related issue number
None. The gap was found while investigating #4820. It is not part of the #5065 follow-up epic, whose six items are all something else.
Labels
doc-updates-requiredlabel.breaking-changelabel.Checks
Evidence
New tests pass on both backends:
A passing test against already correct code proves nothing on its own, so both backends were mutation tested. The production file was swapped in via
go test -overlay, leaving the tree untouched.Mutation A, drop the root dir and cluster prefix from the object path. Both tests go red on both backends:
Mutation B, leave the object path correct and break only the fallback listing prefix. Only the fallback test goes red, on both backends:
Mutation B is the useful one: it shows the two tests in each pair are not redundant, since the second catches a defect the first cannot see.
Rest of the module.
go vet ./...is clean and every non e2e package passes. The one failure,TestGetSessionDir_FatalError_FailsFastinpkg/utils, fails identically on a cleanorigin/mastercheckout on this machine and is unrelated to this change: it depends on symlink behavior this Windows host does not provide.What was not tested
delimiterparameter, so flat versus hierarchical listing is not covered. Changing the s3 fallback from_listFiles(dirPath, "", false)to_listFiles(dirPath, "/", false)still passes. These tests are scoped to the object path, not to listing behavior.go test -race, whichmake testruns in CI, needs a C toolchain this machine does not have. Shared state in the tests is behind a mutex, since the servers handle requests on their own goroutines.This change was developed with AI assistance from Claude. I reviewed and verified it before submitting.