chore(autorag+automl): improvements - S3 timeout error handling + FileExplorer UX#7219
Conversation
Functional changes: - Add 10s connect/TLS timeout to S3 HTTP client via custom transport so unreachable endpoints (air-gapped, misconfigured) fail fast instead of hanging until the OpenShift route 30s gateway timeout. - Set RetryMaxAttempts=1 on the AWS config to avoid compounding the timeout on dead endpoints. - Add isS3ConnectivityError() to classify net.Error timeouts, net.OpError, and net.DNSError as connectivity failures. - Add s3ConnectivityErrorMessage() to return a user-facing 503 message explaining the likely cause and remediation steps. - Wire connectivity error detection into all S3 handlers (GetS3File, PostS3File, GetS3Files, GetS3FileSchema) so they return 503 with the actionable message instead of a generic 500. Test coverage: - isS3ConnectivityError: 9-case table-driven test covering timeout, OpError, DNSError, wrapped variants, nil, and non-connectivity errors (access denied). - s3ConnectivityErrorMessage: asserts bucket name and key phrases appear in the message. - Handler-level 503 tests: GetS3File, GetS3Files, PostS3File (on key resolution), and PostS3File (on upload) all return 503 with the connectivity error message when the S3 endpoint is unreachable. - s3ConnectTimeout constant: asserts the value is 10s. - NewRealS3Client: verifies client construction succeeds with the new timeout-aware HTTP transport. All changes are mirrored across both automl and autorag BFFs. Generated-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds S3 connectivity classification and bucket-scoped 503 responses, introduces a 15s metadata deadline for read-only S3 calls, and configures a connect/TLS handshake timeout plus reduced retry attempts in the S3 HTTP transport. Backend S3 handlers in automl and autorag now detect network/DNS/timeout errors via Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Security & Code Quality Issues
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
Generated-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Generated-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
GAUNSD
changed the title
|
/cc @nickmazzi @daniduong |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
packages/autorag/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx (1)
233-243: Scope dropdown menu assertions to the opened menu container.Lines 234–235 and 240–241 use global
screen.queryByText()andscreen.getByText()assertions. When multiple menu instances exist in the DOM (especially if menus remain mounted but hidden after closing), these assertions can become brittle and match unintended elements. Scope assertions to the active dropdown usingwithin(screen.getByRole('menu')).Proposed test hardening
// file-2 is currently being viewed — its kebab should NOT have "View details" fireEvent.click(within(selectedFilesList).getByLabelText('file-2.json overflow menu')); - expect(screen.queryByText('View details')).not.toBeInTheDocument(); - expect(screen.getByText('Remove selection')).toBeInTheDocument(); + const viewedFileMenu = screen.getByRole('menu'); + expect( + within(viewedFileMenu).queryByRole('menuitem', { name: 'View details' }), + ).not.toBeInTheDocument(); + expect( + within(viewedFileMenu).getByRole('menuitem', { name: 'Remove selection' }), + ).toBeInTheDocument(); // Close the dropdown by clicking the toggle again fireEvent.click(within(selectedFilesList).getByLabelText('file-2.json overflow menu')); // file-1 is NOT being viewed — its kebab SHOULD have "View details" fireEvent.click(within(selectedFilesList).getByLabelText('file-1.json overflow menu')); - expect(screen.getByText('View details')).toBeInTheDocument(); - expect(screen.getByText('Remove selection')).toBeInTheDocument(); + const otherFileMenu = screen.getByRole('menu'); + expect( + within(otherFileMenu).getByRole('menuitem', { name: 'View details' }), + ).toBeInTheDocument(); + expect( + within(otherFileMenu).getByRole('menuitem', { name: 'Remove selection' }), + ).toBeInTheDocument();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/autorag/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx` around lines 233 - 243, The assertions in FileExplorer.spec.tsx are too broad because they use global screen.getByText()/queryByText() after toggling kebab menus for items like 'file-2.json' and 'file-1.json'; narrow these checks to the active dropdown container by using within on the currently opened menu (e.g., within(screen.getByRole('menu')) or within(theOpenedMenuElement) after calling fireEvent.click on within(selectedFilesList).getByLabelText('file-2.json overflow menu') and the 'file-1.json overflow menu') so that the expectations (presence/absence of 'View details' and 'Remove selection') target the correct menu instance rather than any matching element in the DOM.packages/automl/bff/internal/api/s3_handler.go (1)
177-187: Don’t make the UI branch on this English message.PR context says the frontend detects the unreachable-endpoint state by substring-matching the text produced here. That contract is fragile: any copy edit, localization, or automl/autorag drift will silently break the UX. Add a stable machine-readable reason in the error payload and have the UI switch on that instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/bff/internal/api/s3_handler.go` around lines 177 - 187, The UI should not rely on English text from s3ConnectivityErrorMessage for branching; add a stable machine-readable reason to the error payload (e.g. constant "ReasonS3EndpointUnreachable" or "s3_endpoint_unreachable") and return it alongside the human-facing string. Update the response/error construction code that currently calls s3ConnectivityErrorMessage so it populates the new reason field (modify the error response struct or the API response wrapper used by the S3 handlers), export the reason constant, and ensure callers still send the original message for display while the frontend switches to the new reason value for logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/automl/bff/internal/api/s3_handler_test.go`:
- Around line 2198-2334: Add a new table-style unit test mirroring the existing
connectivity tests that targets GetS3FileSchemaHandler: create
TestGetS3FileSchemaHandler_ConnectivityError_Returns503 that sets up a mock
secret (mockS3Secret), mock Kubernetes factory
(mockKubernetesClientFactoryForSecrets), s3 client factory with
connectivityErrorS3Client
(s3mocks.NewMockClientFactory().SetMockClient(&connectivityErrorS3Client{})),
and identity, then call setupS3ApiTestWithBody with http.MethodGet against the
schema endpoint (e.g.
"/api/v1/s3/file/schema?namespace=test-namespace&secretName=aws-secret-1&bucket=my-bucket&key=file.csv"),
assert res.StatusCode == http.StatusServiceUnavailable, unmarshal into
ErrorEnvelope and assert env.Error.Code == "503" and the error message contains
the bucket name and the expected connectivity phrase (e.g. "Unable to connect")
to exercise GetS3FileSchemaHandler's 503 branch.
In `@packages/automl/bff/internal/api/s3_handler.go`:
- Around line 164-175: The connectivity classifier is missing a check for the
net.ErrClosed sentinel, so isS3ConnectivityError currently returns false for
closed keep-alive sockets; update isS3ConnectivityError to treat errors.Is(err,
net.ErrClosed) as a connectivity error (return true), in addition to the
existing timeout/*net.OpError*//*net.DNSError*/ checks, and apply the same
change to the autorag copy of isS3ConnectivityError so closed-socket errors map
to 503 as intended.
In `@packages/automl/bff/internal/integrations/s3/client_test.go`:
- Around line 140-155: The test NewRealS3Client_SetsRetryMaxAttemptsToOne is
network-dependent and doesn't actually assert RetryMaxAttempts; update it to
remove DNS by using a literal IP for EndpointURL (e.g., replace
"https://s3.amazonaws.com" with a literal https://<public-ip>) and then either
(A) make NewRealS3Client return or expose the underlying aws.Config so the test
can directly assert that RetryMaxAttempts == 1 (e.g., return config alongside
the client or add an accessor to inspect the built config), or (B) if you don't
want to change production signatures, rename the test to not claim it verifies
RetryMaxAttempts and instead only asserts successful construction without DNS
dependency; reference NewRealS3Client, S3Credentials, S3ClientOptions, and
RetryMaxAttempts when making the changes.
In `@packages/automl/bff/internal/integrations/s3/client.go`:
- Around line 103-119: The transport only limits TCP connect and TLS handshake
(t.DialContext and t.TLSHandshakeTimeout) but leaves the response phase
unbounded; update the HTTP transport created via
awshttp.NewBuildableClient().WithTransportOptions (the
httpClient/WithTransportOptions block) to also set a response-header deadline
(e.g., set t.ResponseHeaderTimeout to a new s3ResponseHeaderTimeout constant) or
ensure every S3 call using this cfg enforces a request context deadline; modify
the transport in the httpClient creation or add caller-side context timeouts so
responses cannot hang indefinitely.
In `@packages/autorag/bff/internal/api/s3_handler.go`:
- Around line 33-44: The isS3ConnectivityError helper currently checks for
net.Error, *net.OpError, and *net.DNSError but misses the sentinel
net.ErrClosed; update isS3ConnectivityError to also return true when
errors.Is(err, net.ErrClosed) so closed-connection errors are classified as
connectivity issues (locate and modify the isS3ConnectivityError function to add
the errors.Is(err, net.ErrClosed) check alongside the existing checks).
In `@packages/autorag/bff/internal/integrations/s3/client_test.go`:
- Around line 166-181: The test NewRealS3Client_SetsRetryMaxAttemptsToOne is
DNS-dependent and doesn't actually verify RetryMaxAttempts; change the test to
avoid DNS by using a literal IP for EndpointURL (e.g., an S3 IP) or a loopback
test server, and then either (a) modify NewRealS3Client to optionally return or
expose the constructed aws.Config (so the test can assert cfg.Retry.MaxAttempts
== 1) or (b) if changing signatures is undesirable, rename the test to reflect
it only verifies client creation and add a new unit test that inspects the built
config by calling the internal builder (e.g., the function that builds
aws.Config used by NewRealS3Client) to assert RetryMaxAttempts == 1; reference
NewRealS3Client, S3Credentials and the RetryMaxAttempts config value in the
change.
In `@packages/autorag/bff/internal/integrations/s3/client.go`:
- Around line 83-99: The transport only caps TCP connect and TLS handshake
timeouts (see httpClient created via awshttp.NewBuildableClient and the
transport options) but does not bound the response phase; update the transport
to set a response-header deadline (e.g., Transport.ResponseHeaderTimeout) so the
client fails fast if headers never arrive, or ensure callers use a context with
an overall request deadline when invoking the S3 client; modify the transport
configuration in the httpClient creation (the WithTransportOptions closure) to
include the response timeout or add request-deadline enforcement at the S3 call
sites.
---
Nitpick comments:
In `@packages/automl/bff/internal/api/s3_handler.go`:
- Around line 177-187: The UI should not rely on English text from
s3ConnectivityErrorMessage for branching; add a stable machine-readable reason
to the error payload (e.g. constant "ReasonS3EndpointUnreachable" or
"s3_endpoint_unreachable") and return it alongside the human-facing string.
Update the response/error construction code that currently calls
s3ConnectivityErrorMessage so it populates the new reason field (modify the
error response struct or the API response wrapper used by the S3 handlers),
export the reason constant, and ensure callers still send the original message
for display while the frontend switches to the new reason value for logic.
In
`@packages/autorag/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx`:
- Around line 233-243: The assertions in FileExplorer.spec.tsx are too broad
because they use global screen.getByText()/queryByText() after toggling kebab
menus for items like 'file-2.json' and 'file-1.json'; narrow these checks to the
active dropdown container by using within on the currently opened menu (e.g.,
within(screen.getByRole('menu')) or within(theOpenedMenuElement) after calling
fireEvent.click on within(selectedFilesList).getByLabelText('file-2.json
overflow menu') and the 'file-1.json overflow menu') so that the expectations
(presence/absence of 'View details' and 'Remove selection') target the correct
menu instance rather than any matching element in the DOM.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4e006577-50b0-48f2-a95b-6ce2d788d542
📒 Files selected for processing (16)
packages/automl/bff/internal/api/s3_handler.gopackages/automl/bff/internal/api/s3_handler_test.gopackages/automl/bff/internal/integrations/s3/client.gopackages/automl/bff/internal/integrations/s3/client_test.gopackages/automl/frontend/src/app/components/common/FileExplorer/FileExplorer.tsxpackages/automl/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsxpackages/automl/frontend/src/app/components/common/S3FileExplorer/S3FileExplorer.tsxpackages/automl/frontend/src/app/components/common/S3FileExplorer/__tests__/S3FileExplorer.spec.tsxpackages/autorag/bff/internal/api/s3_handler.gopackages/autorag/bff/internal/api/s3_handler_test.gopackages/autorag/bff/internal/integrations/s3/client.gopackages/autorag/bff/internal/integrations/s3/client_test.gopackages/autorag/frontend/src/app/components/common/FileExplorer/FileExplorer.tsxpackages/autorag/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsxpackages/autorag/frontend/src/app/components/common/S3FileExplorer/S3FileExplorer.tsxpackages/autorag/frontend/src/app/components/common/S3FileExplorer/__tests__/S3FileExplorer.spec.tsx
automl: Add TestGetS3FileSchemaHandler_ConnectivityError_Returns503 to exercise the 503 branch when GetCSVSchema hits an unreachable S3 endpoint. Added GetCSVSchema override to connectivityErrorS3Client. automl+autorag: Extract buildS3AWSConfig() from NewRealS3Client so RetryMaxAttempts can be directly asserted in tests. The AWS SDK does not expose RetryMaxAttempts on the constructed *s3.Client, so the previous test could only verify client creation — not the config value. The extracted function returns the raw aws.Config, enabling a real assertion (cfg.RetryMaxAttempts == 1) without DNS or network dependencies. Rename TestNewRealS3Client_SetsRetryMaxAttemptsToOne to TestNewRealS3Client_CreatesClientWithValidCredentials and switch endpoint to a literal IP to remove DNS dependency. Generated-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7219 +/- ##
==========================================
+ Coverage 63.92% 64.98% +1.06%
==========================================
Files 2502 2447 -55
Lines 77696 76159 -1537
Branches 19756 19216 -540
==========================================
- Hits 49664 49489 -175
+ Misses 28032 26670 -1362 see 79 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Add net.ErrClosed sentinel check to isS3ConnectivityError() in both automl and autorag BFFs. A closed keep-alive socket can surface as net.ErrClosed without a *net.OpError wrapper, causing it to fall through to a 500 instead of the intended 503. The new check catches both direct and wrapped net.ErrClosed errors. Includes unit tests for both direct and wrapped net.ErrClosed cases in both packages. Generated-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
Add per-call context deadlines (s3MetadataTimeout = 15s) on read-only S3 handler operations to bound the response-header phase. net/http's WriteTimeout sets a conn deadline but does NOT cancel r.Context(), so if an S3 endpoint accepts the TCP connection but never sends response headers, metadata calls could hang indefinitely. File-transfer handlers (GetObject, UploadObject) are intentionally excluded because legitimate large payloads can exceed any static timeout. Handlers protected: - autorag: GetS3FilesHandler (ListObjects) - automl: GetS3FilesHandler (ListObjects), GetS3FileSchemaHandler (GetCSVSchema) Tests added: - Positive: verify ListObjects/GetCSVSchema receive a deadline-aware context - Negative: verify GetObject does NOT get a handler-imposed deadline Generated-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/automl/bff/internal/api/s3_handler.go (1)
316-324:⚠️ Potential issue | 🟠 MajorTime out the pre-upload
HeadObject/collision check.
resolveNonCollidingS3Key()is still running onr.Context(), so theObjectExists/HeadObjectpreflight can hang until the route timeout if the endpoint accepts the socket and then stalls. That leaves the upload path outside the new fail-fast behavior, and this 503 branch will never execute until much later.Suggested patch
- ctx := r.Context() bucket := s3.bucket - resolvedKey, err := resolveNonCollidingS3Key(ctx, s3.client, bucket, key, app.effectivePostS3CollisionAttempts()) + metadataCtx, cancel := context.WithTimeout(r.Context(), s3MetadataTimeout) + defer cancel() + resolvedKey, err := resolveNonCollidingS3Key(metadataCtx, s3.client, bucket, key, app.effectivePostS3CollisionAttempts())Then keep the actual upload on the request context:
if err := s3.client.UploadObject(r.Context(), bucket, resolvedKey, limitedFile, contentType); err != nil {As per coding guidelines, "HTTP clients to external services must set timeouts and use TLS verification."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/automl/bff/internal/api/s3_handler.go` around lines 316 - 324, The pre-upload collision check is using the request context (r.Context()) and can hang until the route timeout; change resolveNonCollidingS3Key to run with a short, bounded context (create ctxPreflight, cancel := context.WithTimeout(r.Context(), <reasonableDuration>); defer cancel()) and pass that ctxPreflight into resolveNonCollidingS3Key so HeadObject/ObjectExists calls time out quickly; afterwards keep the actual upload on the original request context (call s3.client.UploadObject with r.Context() and the resolvedKey) so the upload path observes the request's fail-fast behavior and the 503 branch (isS3ConnectivityError) can trigger promptly.packages/autorag/bff/internal/api/s3_handler.go (1)
332-340:⚠️ Potential issue | 🟠 MajorBound the upload preflight with
s3MetadataTimeout.The new timeout only covers list operations.
resolveNonCollidingS3Key()still doesObjectExists/HeadObjectcalls onr.Context(), so an endpoint that stalls after connect can still block uploads until the route timeout before any 503 handling runs.Suggested patch
- ctx := r.Context() bucket := s3.bucket - resolvedKey, err := resolveNonCollidingS3Key(ctx, s3.client, bucket, key, app.effectivePostS3CollisionAttempts()) + metadataCtx, cancel := context.WithTimeout(r.Context(), s3MetadataTimeout) + defer cancel() + resolvedKey, err := resolveNonCollidingS3Key(metadataCtx, s3.client, bucket, key, app.effectivePostS3CollisionAttempts())Then call
UploadObjectwithr.Context()so the upload itself stays unbounded for legitimate large transfers.As per coding guidelines, "HTTP clients to external services must set timeouts and use TLS verification."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/autorag/bff/internal/api/s3_handler.go` around lines 332 - 340, resolveNonCollidingS3Key is currently using r.Context(), so its ObjectExists/HeadObject calls can hang until the route timeout; wrap the preflight/key-resolution in a context with the s3MetadataTimeout (create ctx, cancel := context.WithTimeout(r.Context(), s3MetadataTimeout); defer cancel()) and pass that bounded ctx into resolveNonCollidingS3Key (and any downstream ObjectExists/HeadObject calls) to ensure quick 503s on metadata stalls; keep the actual UploadObject call using the original r.Context() so large uploads remain unbounded, and verify the S3 HTTP client used by s3.client respects timeouts and TLS verification per guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/automl/bff/internal/integrations/s3/client_test.go`:
- Around line 143-147: The test uses AWS-like credential-shaped literals in the
S3Credentials passed to NewRealS3Client (AccessKeyID and SecretAccessKey), which
trigger secret scanners; update the test to use neutral, non-credential
placeholders instead (e.g., "test-access-key" and "test-secret-key" or similar)
when constructing the S3Credentials in the NewRealS3Client call so the
client/err setup remains the same but no AWS-formatted keys are present.
In `@packages/autorag/bff/internal/api/s3_handler_test.go`:
- Around line 1666-1678: The helper newTestS3Secret currently seeds a
realistic-looking AWS access key ("AKIA...") which triggers secret scanners;
change the AWS_ACCESS_KEY_ID value to a non-AWS-looking placeholder (e.g.,
"AKIAEXAMPLE" or "ACCESS_KEY_PLACEHOLDER") or derive it from the input name so
it is not a real-format credential, and update
AWS_SECRET_ACCESS_KEY/AWS_DEFAULT_REGION/AWS_S3_ENDPOINT similarly to benign
placeholders if needed; locate and edit the newTestS3Secret function to replace
those Data map entries accordingly without adding any new AKIA-style fixtures.
In `@packages/autorag/bff/internal/integrations/s3/client_test.go`:
- Around line 169-173: The test uses credential-shaped fixture values in the
S3Credentials passed to NewRealS3Client (AccessKeyID, SecretAccessKey, Region,
EndpointURL) which triggers secret scanners; replace them with clearly
synthetic/non-credential-shaped placeholders (e.g., "TEST_ACCESS_KEY",
"TEST_SECRET", "test-region", "https://example.invalid") in the S3Credentials
struct used in the client_test.go to avoid false-positive scanner alerts while
keeping the test semantics the same.
---
Outside diff comments:
In `@packages/automl/bff/internal/api/s3_handler.go`:
- Around line 316-324: The pre-upload collision check is using the request
context (r.Context()) and can hang until the route timeout; change
resolveNonCollidingS3Key to run with a short, bounded context (create
ctxPreflight, cancel := context.WithTimeout(r.Context(), <reasonableDuration>);
defer cancel()) and pass that ctxPreflight into resolveNonCollidingS3Key so
HeadObject/ObjectExists calls time out quickly; afterwards keep the actual
upload on the original request context (call s3.client.UploadObject with
r.Context() and the resolvedKey) so the upload path observes the request's
fail-fast behavior and the 503 branch (isS3ConnectivityError) can trigger
promptly.
In `@packages/autorag/bff/internal/api/s3_handler.go`:
- Around line 332-340: resolveNonCollidingS3Key is currently using r.Context(),
so its ObjectExists/HeadObject calls can hang until the route timeout; wrap the
preflight/key-resolution in a context with the s3MetadataTimeout (create ctx,
cancel := context.WithTimeout(r.Context(), s3MetadataTimeout); defer cancel())
and pass that bounded ctx into resolveNonCollidingS3Key (and any downstream
ObjectExists/HeadObject calls) to ensure quick 503s on metadata stalls; keep the
actual UploadObject call using the original r.Context() so large uploads remain
unbounded, and verify the S3 HTTP client used by s3.client respects timeouts and
TLS verification per guidelines.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6c7c8c69-e22f-477f-92c7-4b7dc96cf68c
📒 Files selected for processing (8)
packages/automl/bff/internal/api/s3_handler.gopackages/automl/bff/internal/api/s3_handler_test.gopackages/automl/bff/internal/integrations/s3/client.gopackages/automl/bff/internal/integrations/s3/client_test.gopackages/autorag/bff/internal/api/s3_handler.gopackages/autorag/bff/internal/api/s3_handler_test.gopackages/autorag/bff/internal/integrations/s3/client.gopackages/autorag/bff/internal/integrations/s3/client_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/automl/bff/internal/integrations/s3/client.go
|
What was going to be a simple tweak to enhance error handling for inaccessible S3 connections turned out to be a larger change that has higher risk for this release. Marking it as draft for now. Will still address review feedback. |
Generated-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
Ready for review! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/autorag/bff/internal/api/s3_handler.go (2)
29-36: Nit: timer not released early; relies on 15s expiry.
s3MetadataTimeoutdoc is fine. Side note on the consumer at Line 336–338:defer metadataCancel()keeps the child context (and its timer) alive for the entirePostS3FileHandlerbody even though the deadline is only needed forresolveNonCollidingS3Key. SinceUploadObjectusesr.Context()directly, you can callmetadataCancel()right after the resolve call to release the timer sooner. Bounded by 15s anyway, so purely cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/autorag/bff/internal/api/s3_handler.go` around lines 29 - 36, The child context and its timer created in PostS3FileHandler for calling resolveNonCollidingS3Key are kept alive by a deferred metadataCancel(), so change the flow to call metadataCancel() immediately after resolveNonCollidingS3Key returns (instead of deferring) to release the timer earlier; locate the metadataCancel/resolveNonCollidingS3Key invocation in PostS3FileHandler and move the cancel call to directly follow the resolveNonCollidingS3Key result before proceeding to UploadObject which should continue to use r.Context().
72-82: Cross-package duplication — symmetric copy lives inpackages/automl/bff/internal/api/s3_handler.go.
isS3ConnectivityError,s3ConnectivityErrorMessage, ands3MetadataTimeoutare being maintained in two BFF packages with no shared module between them. Any future change (e.g., widening thenet.OpErrorclassification, or adjusting the user-facing message) now has two landing sites and will drift. Not a blocker — the modules are intentionally independent — but consider extracting to a sharedbff-commonmodule when module consolidation is on the table.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/autorag/bff/internal/api/s3_handler.go` around lines 72 - 82, The three functions isS3ConnectivityError, s3ConnectivityErrorMessage, and s3MetadataTimeout are duplicated across BFF packages; extract them into a single shared bff-common module (or package) and update both packages to import and call the shared implementations instead of maintaining separate copies; ensure the shared package exposes clearly named functions (e.g., IsS3ConnectivityError, S3ConnectivityErrorMessage, S3MetadataTimeout) and update any references in the local s3_handler.go files to use the new package to avoid drift on future changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/autorag/bff/internal/api/s3_handler.go`:
- Around line 48-70: Update the docstring for isS3ConnectivityError to
explicitly state it only detects pre-request network failures (e.g., context
deadline, DNS failure, dial timeout, connection refused) and does NOT cover
post-dial errors like TLS handshake failures or mid-request connection resets;
replace the current ambiguous comment with the suggested clearer wording so
future readers know this function intentionally only maps those pre-dial
failures to 503 while other errors remain 500.
---
Nitpick comments:
In `@packages/autorag/bff/internal/api/s3_handler.go`:
- Around line 29-36: The child context and its timer created in
PostS3FileHandler for calling resolveNonCollidingS3Key are kept alive by a
deferred metadataCancel(), so change the flow to call metadataCancel()
immediately after resolveNonCollidingS3Key returns (instead of deferring) to
release the timer earlier; locate the metadataCancel/resolveNonCollidingS3Key
invocation in PostS3FileHandler and move the cancel call to directly follow the
resolveNonCollidingS3Key result before proceeding to UploadObject which should
continue to use r.Context().
- Around line 72-82: The three functions isS3ConnectivityError,
s3ConnectivityErrorMessage, and s3MetadataTimeout are duplicated across BFF
packages; extract them into a single shared bff-common module (or package) and
update both packages to import and call the shared implementations instead of
maintaining separate copies; ensure the shared package exposes clearly named
functions (e.g., IsS3ConnectivityError, S3ConnectivityErrorMessage,
S3MetadataTimeout) and update any references in the local s3_handler.go files to
use the new package to avoid drift on future changes.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 83b9eef6-b0ed-4388-b2bc-7a88c2794f57
📒 Files selected for processing (10)
packages/automl/bff/internal/api/s3_handler.gopackages/automl/bff/internal/api/s3_handler_test.gopackages/automl/bff/internal/integrations/s3/client.gopackages/automl/bff/internal/integrations/s3/client_test.gopackages/automl/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsxpackages/autorag/bff/internal/api/s3_handler.gopackages/autorag/bff/internal/api/s3_handler_test.gopackages/autorag/bff/internal/integrations/s3/client.gopackages/autorag/bff/internal/integrations/s3/client_test.gopackages/autorag/frontend/src/app/components/common/FileExplorer/__tests__/FileExplorer.spec.tsx
✅ Files skipped from review due to trivial changes (2)
- packages/automl/frontend/src/app/components/common/FileExplorer/tests/FileExplorer.spec.tsx
- packages/automl/bff/internal/integrations/s3/client_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/autorag/frontend/src/app/components/common/FileExplorer/tests/FileExplorer.spec.tsx
- packages/autorag/bff/internal/integrations/s3/client_test.go
- packages/automl/bff/internal/integrations/s3/client.go
- packages/autorag/bff/internal/integrations/s3/client.go
- packages/automl/bff/internal/api/s3_handler.go
- packages/automl/bff/internal/api/s3_handler_test.go
Generated-by: Claude <noreply@anthropic.com> Co-authored-by: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/automl/bff/internal/api/s3_handler.go`:
- Around line 334-337: The new isS3ConnectivityError branch is indented with
spaces causing gofmt/golangci-lint failures; update the indentation of the block
containing isS3ConnectivityError(err) { ... } so it uses tabs to match the
surrounding code (the block that calls app.serviceUnavailableResponseWithMessage
and s3ConnectivityErrorMessage), then run gofmt/gofmt -w or golangci-lint to
verify formatting; ensure the isS3ConnectivityError,
app.serviceUnavailableResponseWithMessage, and s3ConnectivityErrorMessage calls
remain unchanged except for indentation.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1653f460-ed3a-4db5-b351-47076738ae4c
📒 Files selected for processing (6)
packages/automl/bff/internal/api/s3_handler.gopackages/automl/bff/internal/api/s3_handler_test.gopackages/automl/bff/internal/integrations/s3/client.gopackages/autorag/bff/internal/api/s3_handler.gopackages/autorag/bff/internal/api/s3_handler_test.gopackages/autorag/bff/internal/integrations/s3/client.go
✅ Files skipped from review due to trivial changes (1)
- packages/autorag/bff/internal/api/s3_handler.go
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/automl/bff/internal/integrations/s3/client.go
- packages/autorag/bff/internal/integrations/s3/client.go
- packages/automl/bff/internal/api/s3_handler_test.go
Automated tests clean:
AI-assisted review clean:Code ReviewNo blocking, high, or medium severity issues. Manual testing underway. |
|
@GAUNSD do we also want to disable or hide the view details action from the main file list component when the details are already active for that specific file? 
|
Good catch @chrjones-rh! I'll add that now |
|
To avoid a situation where the overflow menu for a table row is empty ("View details" is hidden for files that are being viewed and "Remove selection" is only shown selected files). I have three options:
Will go with C that way we don't have a case where an empty overflow menu is rendered and instead we have another useful way of selecting a file |
|
Actually @chrjones-rh I'll get it added in a future PR.
|
- Hide "View details" kebab action when file is already being viewed - Add "Select file"/"Select folder" kebab action for unselected items - Delegate onRemoveSelection to parent handler to clear filesToView - Add table row kebab actions test suite (7 tests) - Fix eye icon test that relied on redundant "View details" click Generated-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
|
With the other related PR now merged in I just pushed some fixes and addressed your earlier point @chrjones-rh. |
|
Automated tests are clean: Manual tests look good: 
/lgtm |
|
/retest |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrjones-rh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit b87f1ee
into
opendatahub-io:main
2 participants
Issue
Description
This PR addresses two items; one major one minor.
🔴 Major: Unreachable S3 connections in air-gapped environments
none. This loses over the general 30second timeout typically found in openshift clusters🔵 Minor: UX improvements for 'View details' CTA in the FileExplorer
How Has This Been Tested?
Test Impact
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
mainSummary by CodeRabbit
Bug Fixes
New Features
Tests