Skip to content

Commit 1e2fe83

Browse files
filer: also catch 409 ALREADY_EXISTS on /workspace/import (concurrent writes)
The /workspace/import endpoint reports an existing path with two different error codes depending on contention: - 400 RESOURCE_ALREADY_EXISTS: sequential conflict (overwrite=false on existing path) Message: "<path> already exists. Please pass overwrite=true to overwrite it." - 409 ALREADY_EXISTS: concurrent contention (e.g. multiple deploy locks racing) Message: "Node with name <path> already exists. Please pass overwrite=true to update it." Verified by direct probe against aws-prod and aws-prod-ucws — sequential conflicts produce the first shape, but a 5-goroutine race on the same path produces the second shape with status 409 and error_code ALREADY_EXISTS. The original PR's errors.Is(err, ErrResourceAlreadyExists) only catches the first shape, leaving locker.Lock() returning the raw API error instead of falling through to assertLockHeld() — which is what TestLock asserts. Add errors.Is(err, ErrAlreadyExists) for the second shape; both inherit from apierr.ErrResourceConflict but we use the specific sentinels rather than the broader parent (ErrResourceConflict also covers ErrAborted, which is a different concept). Co-authored-by: Isaac
1 parent 6907378 commit 1e2fe83

2 files changed

Lines changed: 20 additions & 7 deletions

File tree

libs/filer/workspace_files_client.go

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -198,14 +198,18 @@ func (w *WorkspaceFilesClient) Write(ctx context.Context, name string, reader io
198198
return w.Write(ctx, name, bytes.NewReader(body), sliceWithout(mode, CreateParentDirectories)...)
199199
}
200200

201-
// File already exists at the path.
202-
if errors.Is(err, apierr.ErrResourceAlreadyExists) {
201+
// File already exists at the path. The /workspace/import endpoint reports this
202+
// with two different error_codes depending on whether the conflict was detected
203+
// sequentially (400 RESOURCE_ALREADY_EXISTS) or under concurrent contention
204+
// (409 ALREADY_EXISTS, observed in TestLock). Both are already-exists from the
205+
// caller's perspective.
206+
//
207+
// Existing-object-with-mismatched-node-type (e.g. uploading a regular .py when a
208+
// NOTEBOOK is at the path) surfaces as 400 INVALID_PARAMETER_VALUE with a
209+
// "Requested node type" message — also already-exists from the caller's perspective.
210+
if errors.Is(err, apierr.ErrResourceAlreadyExists) || errors.Is(err, apierr.ErrAlreadyExists) {
203211
return fileAlreadyExistsError{absPath}
204212
}
205-
206-
// Existing object's node type doesn't match the upload (e.g. uploading a regular .py
207-
// when a NOTEBOOK is at the path, or vice versa). From the caller's perspective there
208-
// is something at that path, so surface as already-exists.
209213
if errors.Is(err, apierr.ErrInvalidParameterValue) {
210214
var aerr *apierr.APIError
211215
if errors.As(err, &aerr) && strings.Contains(aerr.Message, "Requested node type") {

libs/filer/workspace_files_client_test.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -173,7 +173,16 @@ func TestWorkspaceFilesClientWriteErrorMapping(t *testing.T) {
173173
apiErr: &apierr.APIError{
174174
StatusCode: http.StatusBadRequest,
175175
ErrorCode: "RESOURCE_ALREADY_EXISTS",
176-
Message: "Path (/dir/file.txt) already exists.",
176+
Message: "/dir/file.txt already exists. Please pass overwrite=true to overwrite it.",
177+
},
178+
expectErrTarget: fileAlreadyExistsError{},
179+
},
180+
{
181+
name: "409 ALREADY_EXISTS (concurrent contention) maps to fileAlreadyExistsError",
182+
apiErr: &apierr.APIError{
183+
StatusCode: http.StatusConflict,
184+
ErrorCode: "ALREADY_EXISTS",
185+
Message: "Node with name /dir/file.txt already exists. Please pass overwrite=true to update it.",
177186
},
178187
expectErrTarget: fileAlreadyExistsError{},
179188
},

0 commit comments

Comments
 (0)