Skip to content

Commit e273529

Browse files
daniduongDaniel Duong
andauthored
fix(autox): fix 500 error after many duplicate file uploads (#7220)
* fix(autox): return 409 for too many duplicate file uploads * test(autox): update collision resolution tests * fix(autox): display human-readable error message for 409 collisions * test(autox): add ui tests to validate 409 collision error handling * fix: address comment * Revert "fix: address comment" This reverts commit bc3b631. --------- Co-authored-by: Daniel Duong <danielduong@ibm.com>
1 parent b9ff75c commit e273529

11 files changed

Lines changed: 186 additions & 30 deletions

File tree

packages/automl/bff/internal/api/s3_handler.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,10 @@ type resolvedS3 struct {
3232

3333
var trailingNumberPattern = regexp.MustCompile(`^(.*)-(\d+)$`)
3434

35+
// ErrMaxCollisionsExceeded is returned when resolveNonCollidingS3Key exhausts all attempts
36+
// to find a unique object key due to naming collisions.
37+
var ErrMaxCollisionsExceeded = errors.New("max collision attempts exceeded")
38+
3539
// resolveS3Client resolves S3 credentials (from DSPA context or an explicit secretName),
3640
// creates an S3 client via the factory, and resolves the bucket.
3741
// Returns false if an error response was already written to w.
@@ -268,6 +272,12 @@ func (app *App) PostS3FileHandler(w http.ResponseWriter, r *http.Request, _ http
268272
bucket := s3.bucket
269273
resolvedKey, err := resolveNonCollidingS3Key(ctx, s3.client, bucket, key, app.effectivePostS3CollisionAttempts())
270274
if err != nil {
275+
if errors.Is(err, ErrMaxCollisionsExceeded) {
276+
app.conflictResponse(w, r,
277+
fmt.Sprintf("unable to find unique filename after %d attempts; try a different base name",
278+
app.effectivePostS3CollisionAttempts()))
279+
return
280+
}
271281
app.serverErrorResponse(w, r, fmt.Errorf("error resolving S3 key for upload: %w", err))
272282
return
273283
}
@@ -395,7 +405,7 @@ func resolveNonCollidingS3Key(
395405
}
396406
nextIndex++
397407
}
398-
return "", fmt.Errorf("failed to resolve non-colliding S3 key after %d attempts", maxCollisionAttempts)
408+
return "", ErrMaxCollisionsExceeded
399409
}
400410

401411
func splitS3ObjectPath(key string) (dir string, name string) {

packages/automl/bff/internal/api/s3_handler_test.go

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1971,7 +1971,7 @@ func TestPostS3FileHandler_TwoFileParts_UsesFirstPartOnly(t *testing.T) {
19711971
assert.Equal(t, []byte("first-content"), capture.uploaded)
19721972
}
19731973

1974-
func TestPostS3FileHandler_CollisionResolutionExhausted_Returns500(t *testing.T) {
1974+
func TestPostS3FileHandler_CollisionResolutionExhausted_Returns409(t *testing.T) {
19751975
t.Parallel()
19761976
secret := mockS3Secret("aws-secret-1", "test-namespace")
19771977
k8sFactory := &mockKubernetesClientFactoryForSecrets{client: &mockKubernetesClientForSecrets{secrets: []corev1.Secret{secret}}}
@@ -1994,12 +1994,13 @@ func TestPostS3FileHandler_CollisionResolutionExhausted_Returns500(t *testing.T)
19941994
res := rr.Result()
19951995
defer res.Body.Close()
19961996

1997-
assert.Equal(t, http.StatusInternalServerError, res.StatusCode)
1997+
assert.Equal(t, http.StatusConflict, res.StatusCode)
19981998
var env ErrorEnvelope
19991999
require.NoError(t, json.Unmarshal(rr.Body.Bytes(), &env))
20002000
require.NotNil(t, env.Error)
2001-
assert.Equal(t, "500", env.Error.Code)
2002-
assert.Contains(t, env.Error.Message, "the server encountered a problem")
2001+
assert.Equal(t, "409", env.Error.Code)
2002+
assert.Contains(t, env.Error.Message, "unable to find unique filename")
2003+
assert.Contains(t, env.Error.Message, "5 attempts")
20032004
}
20042005

20052006
func TestResolveNonCollidingS3Key_PreservesDirectoryPrefix(t *testing.T) {

packages/automl/frontend/src/app/components/configure/AutomlConfigure.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -342,7 +342,15 @@ function AutomlConfigure(): React.JSX.Element {
342342
setValue('train_data_file_key', uploadResult.key, { shouldValidate: true });
343343
} catch (err) {
344344
if (uploadRequestId === trainingDataUploadSeqRef.current) {
345-
notification.error('Failed to upload file', err instanceof Error ? err.message : '');
345+
const errorMessage = err instanceof Error ? err.message : String(err);
346+
const isConflict = errorMessage.toLowerCase().includes('unique filename');
347+
348+
notification.error(
349+
'Failed to upload file',
350+
isConflict
351+
? 'A file with this name already exists and no unique name could be generated. Please rename your file or delete existing files with similar names.'
352+
: errorMessage,
353+
);
346354
}
347355
} finally {
348356
if (uploadRequestId === trainingDataUploadSeqRef.current) {

packages/automl/frontend/src/app/components/configure/__tests__/AutomlConfigure.spec.tsx

Lines changed: 35 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,13 @@ import { FormProvider, useForm } from 'react-hook-form';
77
import { useNavigate, useParams } from 'react-router';
88
import AutomlConfigure from '~/app/components/configure/AutomlConfigure';
99
import type { Files } from '~/app/components/common/FileExplorer/FileExplorer';
10-
import { useS3FileUploadMutation } from '~/app/hooks/mutations';
1110
import { useS3GetFileSchemaQuery } from '~/app/hooks/queries';
1211
import { createConfigureSchema, TASK_TYPES } from '~/app/schemas/configure.schema';
1312

1413
const mockNotificationError = jest.fn();
1514

15+
const mockS3MutateAsync = jest.fn().mockResolvedValue({ uploaded: true, key: 'uploaded-key.csv' });
16+
1617
jest.mock('react-router', () => ({
1718
...jest.requireActual('react-router'),
1819
useNavigate: jest.fn(),
@@ -28,30 +29,18 @@ jest.mock('~/app/hooks/useNotification', () => ({
2829
}),
2930
}));
3031

31-
jest.mock('~/app/hooks/mutations', () => {
32-
const mockS3MutateAsync = jest
33-
.fn()
34-
.mockResolvedValue({ uploaded: true, key: 'uploaded-key.csv' });
35-
const stableS3UploadMutation = {
32+
jest.mock('~/app/hooks/mutations', () => ({
33+
...jest.requireActual<typeof import('~/app/hooks/mutations')>('~/app/hooks/mutations'),
34+
useS3FileUploadMutation: jest.fn(() => ({
3635
mutateAsync: mockS3MutateAsync,
3736
isPending: false,
3837
reset: jest.fn(),
39-
variables: undefined as { file: File } | undefined,
40-
};
41-
return {
42-
...jest.requireActual<typeof import('~/app/hooks/mutations')>('~/app/hooks/mutations'),
43-
useS3FileUploadMutation: jest.fn(() => stableS3UploadMutation),
44-
};
45-
});
38+
variables: undefined,
39+
})),
40+
}));
4641

4742
function getMockS3MutateAsync(): jest.Mock {
48-
const result = jest.mocked(useS3FileUploadMutation).mock.results[0]?.value as
49-
| { mutateAsync: jest.Mock }
50-
| undefined;
51-
if (!result?.mutateAsync) {
52-
throw new Error('useS3FileUploadMutation was not called; render AutomlConfigure first');
53-
}
54-
return result.mutateAsync;
43+
return mockS3MutateAsync;
5544
}
5645

5746
// Mock S3FileExplorer component
@@ -239,6 +228,8 @@ describe('AutomlConfigure', () => {
239228
} as unknown as ReturnType<typeof useS3GetFileSchemaQuery>);
240229
mockUseNavigate.mockReturnValue(jest.fn());
241230
mockUseParams.mockReturnValue({ namespace: 'test-namespace' });
231+
// Reset the S3 upload mock to default resolved value
232+
mockS3MutateAsync.mockResolvedValue({ uploaded: true, key: 'uploaded-key.csv' });
242233
});
243234

244235
describe('initial state - no secret selected', () => {
@@ -376,6 +367,30 @@ describe('AutomlConfigure', () => {
376367
expect(mockNotificationError).not.toHaveBeenCalled();
377368
});
378369

370+
it('should show human-readable error for max collision attempts (409)', async () => {
371+
renderComponent();
372+
fireEvent.click(screen.getByTestId('aws-secret-selector-select-secret-1'));
373+
fireEvent.click(screen.getByRole('button', { name: 'Upload file' }));
374+
375+
const fileInput = document.querySelector('input[type="file"]') as HTMLInputElement | null;
376+
expect(fileInput).not.toBeNull();
377+
378+
const file = new File(['hello'], 'collision.csv', { type: 'text/csv' });
379+
getMockS3MutateAsync().mockClear();
380+
getMockS3MutateAsync().mockRejectedValue(
381+
new Error('unable to find unique filename after 10 attempts'),
382+
);
383+
384+
fireEvent.change(fileInput!, { target: { files: [file] } });
385+
386+
await waitFor(() => {
387+
expect(mockNotificationError).toHaveBeenCalledWith(
388+
'Failed to upload file',
389+
'A file with this name already exists and no unique name could be generated. Please rename your file or delete existing files with similar names.',
390+
);
391+
});
392+
});
393+
379394
it('should show the newly selected secret name when switching secrets', () => {
380395
renderComponent();
381396

packages/autorag/bff/internal/api/s3_handler.go

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ type S3FilesEnvelope Envelope[models.S3ListObjectsResponse, None]
2828

2929
var trailingNumberPattern = regexp.MustCompile(`^(.*)-(\d+)$`)
3030

31+
// ErrMaxCollisionsExceeded is returned when resolveNonCollidingS3Key exhausts all attempts
32+
// to find a unique object key due to naming collisions.
33+
var ErrMaxCollisionsExceeded = errors.New("max collision attempts exceeded")
34+
3135
// resolvedS3 holds a ready-to-use S3 client and the resolved bucket name.
3236
type resolvedS3 struct {
3337
client s3int.S3ClientInterface
@@ -284,6 +288,12 @@ func (app *App) PostS3FileHandler(w http.ResponseWriter, r *http.Request, _ http
284288
bucket := s3.bucket
285289
resolvedKey, err := resolveNonCollidingS3Key(ctx, s3.client, bucket, key, app.effectivePostS3CollisionAttempts())
286290
if err != nil {
291+
if errors.Is(err, ErrMaxCollisionsExceeded) {
292+
app.conflictResponse(w, r,
293+
fmt.Sprintf("unable to find unique filename after %d attempts; try a different base name",
294+
app.effectivePostS3CollisionAttempts()))
295+
return
296+
}
287297
app.serverErrorResponse(w, r, fmt.Errorf("error resolving S3 key for upload: %w", err))
288298
return
289299
}
@@ -415,7 +425,7 @@ func resolveNonCollidingS3Key(
415425
}
416426
nextIndex++
417427
}
418-
return "", fmt.Errorf("failed to resolve non-colliding S3 key after %d attempts", maxCollisionAttempts)
428+
return "", ErrMaxCollisionsExceeded
419429
}
420430

421431
func splitS3ObjectPath(key string) (dir string, name string) {

packages/autorag/bff/internal/api/s3_handler_test.go

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1364,6 +1364,50 @@ func TestPostS3FileHandler_ResolvesCollidingNumericSuffix(t *testing.T) {
13641364
assert.Equal(t, "file-6.pdf", collisionClient.uploadedKey)
13651365
}
13661366

1367+
func TestPostS3FileHandler_CollisionResolutionExhausted_Returns409(t *testing.T) {
1368+
t.Parallel()
1369+
secret := validS3Secret("aws-secret-1", "test-namespace")
1370+
k8sFactory := &mockKubernetesClientFactoryForSecrets{client: &mockKubernetesClientForSecrets{secrets: []corev1.Secret{secret}}}
1371+
s3Factory := s3mocks.NewMockClientFactory()
1372+
s3Factory.SetMockClient(&alwaysExistsS3Client{})
1373+
identity := &kubernetes.RequestIdentity{UserID: "test-user"}
1374+
body, contentType := buildMultipartFileUpload(t, "file", "a.pdf", []byte("x"))
1375+
1376+
rr := setupS3ApiTestWithBody(
1377+
http.MethodPost,
1378+
"/api/v1/s3/file?namespace=test-namespace&secretName=aws-secret-1&bucket=my-bucket&key=a.pdf",
1379+
body,
1380+
contentType,
1381+
k8sFactory,
1382+
s3Factory,
1383+
identity,
1384+
&s3HandlerTestAppOptions{S3PostMaxCollisionAttempts: 5},
1385+
nil,
1386+
)
1387+
res := rr.Result()
1388+
defer res.Body.Close()
1389+
1390+
assert.Equal(t, http.StatusConflict, res.StatusCode)
1391+
var env ErrorEnvelope
1392+
err := json.Unmarshal(rr.Body.Bytes(), &env)
1393+
assert.NoError(t, err)
1394+
assert.NotNil(t, env.Error)
1395+
assert.Equal(t, "409", env.Error.Code)
1396+
assert.Contains(t, env.Error.Message, "unable to find unique filename")
1397+
assert.Contains(t, env.Error.Message, "5 attempts")
1398+
}
1399+
1400+
// alwaysExistsS3Client always returns true for ObjectExists to simulate exhausting collision attempts.
1401+
type alwaysExistsS3Client struct{ s3mocks.MockS3Client }
1402+
1403+
func (*alwaysExistsS3Client) ObjectExists(context.Context, string, string) (bool, error) {
1404+
return true, nil
1405+
}
1406+
1407+
func (*alwaysExistsS3Client) UploadObject(context.Context, string, string, io.Reader, string) error {
1408+
return s3int.ErrObjectAlreadyExists
1409+
}
1410+
13671411
func TestResolveNonCollidingS3Key_PreservesDirectoryPrefix(t *testing.T) {
13681412
t.Parallel()
13691413
client := newKeyCollisionS3Client("folder/sub/file.pdf", "folder/sub/file-1.pdf")

packages/autorag/frontend/src/app/components/configure/AutoragConfigure.tsx

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -329,7 +329,15 @@ function AutoragConfigure(): React.JSX.Element {
329329
setValue('input_data_key', uploadResult.key, { shouldValidate: true });
330330
} catch (err) {
331331
if (uploadRequestId === inputDataUploadSeqRef.current) {
332-
notification.error('Failed to upload file', err instanceof Error ? err.message : '');
332+
const errorMessage = err instanceof Error ? err.message : String(err);
333+
const isConflict = errorMessage.toLowerCase().includes('unique filename');
334+
335+
notification.error(
336+
'Failed to upload file',
337+
isConflict
338+
? 'A file with this name already exists and no unique name could be generated. Please rename your file or delete existing files with similar names.'
339+
: errorMessage,
340+
);
333341
}
334342
} finally {
335343
if (uploadRequestId === inputDataUploadSeqRef.current) {

packages/autorag/frontend/src/app/components/configure/AutoragEvaluationSelect.tsx

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,9 +40,14 @@ function AutoragEvaluationSelect(): React.JSX.Element {
4040
try {
4141
response = await uploadToStorageMutation.mutateAsync({ file, onProgress: setProgress });
4242
} catch (error) {
43+
const errorMessage = error instanceof Error ? error.message : String(error);
44+
const isConflict = errorMessage.toLowerCase().includes('unique filename');
45+
4346
notification.error(
4447
'Failed to upload file',
45-
error instanceof Error ? error.message : String(error),
48+
isConflict
49+
? 'A file with this name already exists and no unique name could be generated. Please rename your file or delete existing files with similar names.'
50+
: errorMessage,
4651
);
4752
setStatus('danger');
4853
return;

packages/autorag/frontend/src/app/components/configure/__tests__/AutoragConfigure.spec.tsx

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -268,6 +268,8 @@ describe('AutoragConfigure', () => {
268268
mockNotificationError.mockClear();
269269
mockUseNavigate.mockReturnValue(jest.fn());
270270
mockUseParams.mockReturnValue({ namespace: 'test-namespace' });
271+
// Reset the S3 upload mock to default resolved value
272+
mockS3MutateAsync.mockResolvedValue({ uploaded: true, key: 'uploaded-key.txt' });
271273
});
272274

273275
describe('initial state - no secret selected', () => {
@@ -443,6 +445,30 @@ describe('AutoragConfigure', () => {
443445
expect(mockNotificationError).not.toHaveBeenCalled();
444446
});
445447

448+
it('should show human-readable error for max collision attempts (409)', async () => {
449+
renderComponent();
450+
fireEvent.click(screen.getByTestId('aws-secret-selector-select-secret-1'));
451+
fireEvent.click(screen.getByRole('button', { name: 'Upload file' }));
452+
453+
const fileInput = document.querySelector('input[type="file"]') as HTMLInputElement | null;
454+
expect(fileInput).not.toBeNull();
455+
456+
const file = new File(['hello'], 'collision.txt', { type: 'text/plain' });
457+
getMockS3MutateAsync().mockClear();
458+
getMockS3MutateAsync().mockRejectedValue(
459+
new Error('unable to find unique filename after 10 attempts'),
460+
);
461+
462+
fireEvent.change(fileInput!, { target: { files: [file] } });
463+
464+
await waitFor(() => {
465+
expect(mockNotificationError).toHaveBeenCalledWith(
466+
'Failed to upload file',
467+
'A file with this name already exists and no unique name could be generated. Please rename your file or delete existing files with similar names.',
468+
);
469+
});
470+
});
471+
446472
it('should show the newly selected secret name when switching secrets', () => {
447473
renderComponent();
448474

packages/autorag/frontend/src/app/components/configure/__tests__/AutoragEvaluationSelect.spec.tsx

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,26 @@ describe('AutoragEvaluationSelect', () => {
263263
});
264264
});
265265

266+
it('should show human-readable error for max collision attempts (409)', async () => {
267+
const user = userEvent.setup();
268+
const file = new File(['test content'], 'test.json', { type: 'application/json' });
269+
const error = new Error('unable to find unique filename after 10 attempts');
270+
271+
mockUploadMutateAsync.mockRejectedValue(error);
272+
273+
const { container } = renderWithProviders(<AutoragEvaluationSelect />);
274+
275+
const uploadInput = container.querySelector('input[type="file"]') as HTMLInputElement;
276+
await user.upload(uploadInput, file);
277+
278+
await waitFor(() => {
279+
expect(mockNotificationError).toHaveBeenCalledWith(
280+
'Failed to upload file',
281+
'A file with this name already exists and no unique name could be generated. Please rename your file or delete existing files with similar names.',
282+
);
283+
});
284+
});
285+
266286
it('should not open S3FileExplorer initially', () => {
267287
renderWithProviders(<AutoragEvaluationSelect />);
268288

0 commit comments

Comments
 (0)