You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
feat(racer-object): add S3 origin and HTTP sidecar - #877
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The origin timeout flag does not bound complete AWS operations as documented.
0 open findings
Previously missed (1)
In code that hasn't changed since last review
Request timeout ignores retries and the configured operation deadline
cmd/racer-object/main.go:250
--request-timeout is described as an origin request timeout, but setting it only on http.Client.Timeout bounds each AWS HTTP attempt. SDK retries can exceed the configured value, while racersdk.ServeOrigin still applies its fixed 60-second operation deadline (pkg/racersdk/origin.go:111-117), so values above one minute have no effect. Propagate this setting through the operation context (including the returned GET body), or rename and validate it as a per-attempt timeout.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Published images currently retain unknown source revision and build metadata, reducing artifact provenance.
0 open findings
Previously missed (1)
In code that hasn't changed since last review
Pass commit and build timestamp metadata into published images
images/racer-object/Dockerfile:13
The image workflow now discovers this Dockerfile, but it passes only VERSION and CONTAINER_REGISTRY (.github/workflows/images.yaml:143-145). Consequently every image published through that workflow keeps these defaults: racer-object --version reports an unknown commit/build time, and the OCI revision label is unknown. Pass the workflow SHA (and build timestamp, if retained in the version output) into the build so published artifacts have usable provenance.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
Request timeouts cannot reliably return 503 responses, and published images lose commit/build provenance.
0 open findings
Previously missed (2)
In code that hasn't changed since last review
Generic build omits Docker image commit and build timestamp
.github/workflows/images.yaml:66
This fallback newly sends images/racer-object/Dockerfile through the generic build job, but that job passes only VERSION and CONTAINER_REGISTRY. Consequently the Dockerfile's GIT_COMMIT and BUILD_TIME remain unknown, so published images have an unknown OCI revision and racer-object --version cannot identify the source build. Pass the workflow SHA and a build timestamp as build arguments, as the local Make target already does.
WriteTimeout expires before handler can send 503 response
cmd/racer-object/main.go:409
WriteTimeout starts when net/http finishes reading the request headers, before the handler creates requestCtx. Therefore, when requestCtx expires during Stat or Get, the connection's write deadline has already expired and the intended 503 response cannot be sent; clients receive a connection failure instead. Keep the handler timeout shorter than the socket write deadline, or remove the server-level write timeout and rely on Object.WriteTo's context-aware write deadlines.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The new security-sensitive data path spans S3 consistency, HTTP semantics, socket lifecycle, image publishing, and Kubernetes deployment, warranting final human validation.
This troubleshooting step points users to logs that the adapter does not emit for read failures: the AWS logger is discarded in cmd/racer-object/main.go:262, and origin callback errors are only classified and returned over Racer. Remove the origin logs advice (or add sanitized request-failure logging) so operators are not sent to an empty diagnostic source.
The reason will be displayed to describe this comment to others. Learn more.
Follow-up at 87c7065d. Thanks for the fixes; most of my earlier comments are addressed. The two items below are the Major ones that still need work. One thread was resolved without a code or doc change, so I've re-raised it here as a new thread. I haven't repeated the three threads that are still open (VersionId pinning, delete-marker 405, invalid Range).
Checked by reading the code at this head; I did not run tests.
Reversed byte ranges at int64 boundary incorrectly return 416
internal/racer/object/sidecar.go:599
sidecarRangeNumber saturates both bounds to MaxInt64, so this comparison loses their original ordering. For a 10-byte object, Range: bytes=9223372036854775808-9223372036854775807 therefore returns 416 instead of ignoring the reversed range and serving the full object with 200. This contradicts the parser's documented behavior and the ordinary reversed-range case in sidecar_test.go:319. Compare the validated decimal bounds without saturation (ignoring leading zeros), and add a regression case at the int64 boundary.
The reason will be displayed to describe this comment to others. Learn more.
🔵 Needs a closer look
The new streaming, transport ownership, and shutdown boundaries warrant human review and runtime validation.
0 open findings
🧠 Review effort: Balanced
This branch has not been deployed
No deployments
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
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.
Add the racer-object process with a S3 client and server implementation, more APIs coming soon.
The idea is to bridge between Racer's low-level UDS API and higher level applications using efficient
splice()syscalls under the hood.