[feat] Add a mounts move endpoint so files and folders rename server-side - #6883
ashrafchowdury wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 SummarySummary by CodeRabbit
WalkthroughChangesMounts now support moving files and folders through a new API endpoint. The service validates paths, copies source keys, deletes source keys, and reports conflicts or incomplete deletion. Object-store copying and failed-key reporting are supported. Unit and acceptance tests cover the operation. Mount file move
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MountsRouter
participant MountsService
participant ObjectStore
Client->>MountsRouter: Submit move request
MountsRouter->>MountsService: Call move_path
MountsService->>ObjectStore: Copy source keys
MountsService->>ObjectStore: Delete source keys
ObjectStore-->>MountsService: Return failed key names
MountsService-->>MountsRouter: Return MountFileMoved
MountsRouter-->>Client: Return MountFileMovedResponse
Merge Risk: 🟡 Moderate · up to Large folder moves can exhaust worker memory, and a failed move can remove another writer's destination object during rollback. These correctness and availability risks should be addressed before release. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 71baceb8-daf4-42ec-92a0-941c0bdc20c0
📒 Files selected for processing (8)
api/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/core/store/storage.pyapi/oss/tests/pytest/acceptance/mounts/test_mounts_basics.pyapi/oss/tests/pytest/unit/test_mounts_file_ops.py
Included review availability: Your plan provides up to 8 included reviews per hour; 6 remain after this review.
Railway Preview Environment
|
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: 7de9910a-3620-47ad-acaa-59dff74e7fcf
📒 Files selected for processing (4)
api/oss/src/core/mounts/service.pyapi/oss/src/core/store/storage.pyapi/oss/tests/pytest/unit/mounts/test_protected_mount_policy.pyapi/oss/tests/pytest/unit/test_mounts_file_ops.py
Included review availability: Your plan provides up to 8 included reviews per hour; 5 remain after this review.
d612e70 to
713d023
Compare
713d023 to
f05893d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve the delete_prefix count and failure semantics. · test_mounts_file_ops.py:281-283
api/oss/tests/pytest/unit/test_mounts_file_ops.py:281-283
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve the
delete_prefixcount and failure semantics.
FakeMountStorage.delete_prefix()declares-> int, but it returns theList[str]fromdelete_keys().delete_keys()returns keys that the store refused, so returninglen(objects)would count failed deletions as successful. Match the production implementation by subtracting the failed keys.Proposed fix
async def delete_prefix(self, *, bucket: str, prefix: str) -> int: objects = await self.list_objects_v2(bucket=bucket, prefix=prefix) - return await self.delete_keys(bucket=bucket, keys=[o.key for o in objects]) + failed = await self.delete_keys( + bucket=bucket, keys=[o.key for o in objects] + ) + return len(objects) - len(failed)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Advanced
Run ID: ad5424ab-b543-4862-ba15-61a0e8795809
📒 Files selected for processing (9)
api/oss/src/apis/fastapi/mounts/models.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/mounts/types.pyapi/oss/src/core/store/storage.pyapi/oss/tests/pytest/acceptance/mounts/test_mounts_basics.pyapi/oss/tests/pytest/unit/mounts/test_protected_mount_policy.pyapi/oss/tests/pytest/unit/test_mounts_file_ops.py
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review.
| copies = [ | ||
| asyncio.create_task(_copy(key, target)) | ||
| for key, target in zip(source_keys, targets) | ||
| ] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1520,1665p' api/oss/src/core/mounts/service.py
rg -n '_LIST_CONCURRENCY|async def list_objects|def list_objects|_path_keys' api/oss/src/core/mounts/service.py api/oss/src/core/store/storage.pyRepository: Agenta-AI/agenta
Length of output: 6670
🏁 Script executed:
sed -n '410,515p' api/oss/src/core/store/storage.py
rg -n --glob '*.py' 'move_path\(|MountFileMoved|/move|move.*path|path.*move' api/oss/src
sed -n '900,1030p' api/oss/src/core/mounts/service.py
sed -n '1125,1195p' api/oss/src/core/mounts/service.pyRepository: Agenta-AI/agenta
Length of output: 17755
🏁 Script executed:
sed -n '240,280p' api/oss/src/apis/fastapi/mounts/router.py
sed -n '600,655p' api/oss/src/apis/fastapi/mounts/router.py
sed -n '80,130p' api/oss/src/apis/fastapi/mounts/models.py
rg -n --glob '*.py' '_MAX_.*(FILE|OBJECT|KEY)|max.*(file|object|key)|move.*(limit|cap)|limit.*move' api/oss/src/core/mounts api/oss/src/apis/fastapi/mountsRepository: Agenta-AI/agenta
Length of output: 5519
Bound task creation for large folder moves.
move_path creates one live asyncio.Task for every source key before awaiting completion. The semaphore limits storage calls inside _copy; it does not limit task allocation. _path_keys recursively materializes every source object, and the /files/move endpoint has no move-specific object limit. A sufficiently large folder can therefore exhaust worker memory through retained task and coroutine state.
Process source-target pairs in chunks or use a fixed worker pool so task creation remains bounded by _LIST_CONCURRENCY. Avoid adding another full pairs list, since source_keys and targets are already materialized.
Proposed chunked copy
- copies = [
- asyncio.create_task(_copy(key, target))
- for key, target in zip(source_keys, targets)
- ]
try:
- await asyncio.gather(*copies)
+ for offset in range(0, len(source_keys), _LIST_CONCURRENCY):
+ copies = [
+ asyncio.create_task(_copy(key, target))
+ for key, target in zip(
+ source_keys[offset : offset + _LIST_CONCURRENCY],
+ targets[offset : offset + _LIST_CONCURRENCY],
+ )
+ ]
+ await asyncio.gather(*copies)
except BaseException:
# gather leaves the siblings running; stop them so nothing lands after the failure.
for task in copies:
task.cancel()
…lder Object stores have no rename, so a move is: enumerate the path's keys (the exact key, or a folder's marker and everything under it — the same walk delete_path does, now shared as _path_keys), server-side copy each to its new key, then delete the old ones. Copies first, delete last, so a failed copy leaves the source intact. A folder marker is re-created rather than copied: SeaweedFS refuses a trailing-slash key as a copy source. `path` and `to` are both full mount-relative paths, validated like every file op; the same path or a folder into itself is 422, a missing source 404, an occupied destination 409 (MountFileConflict — the store never overwrites). EDIT_MOUNTS, like the other writes. ObjectStore gains copy_object over miniopy's CopySource. Unit tests cover a file rename, a folder move with marker and descendants (a prefix sibling stays), and every refusal; acceptance tests run the route end to end.
… failure, surface failed deletes The destination conflict check listed whole sibling subtrees (prefix without a slash); one bounded page per check now settles it, and the shared key enumeration uses the same for its exact-key membership. A failed copy cancels the in-flight siblings so nothing lands after the error. delete_keys returns the count the store actually removed, and a move whose source keys survive raises instead of reporting success.
…ed deletes retry once delete_keys now returns the keys the store refused instead of a count, so move_path can retry them and delete_path reports the real count. The protected-mount fake store gains the bounded page listing move_path uses.
…mmediate child's The shallow lister keeps the marker's mtime, and the with_counts pass folds in the newest child it already reads, so a fresh folder sorts as new in the Files pane.
f05893d to
d76f4b5
Compare
1 participant
Context
Renaming a file in the Files pane downloads its bytes to the browser, uploads them under the new name and deletes the old key. Every byte round-trips through the client, and folders cannot be renamed at all (a just-created empty folder is faked by create + delete). The mounts API had no move: folder create, upload, write, download, list and delete only.
Changes
One new endpoint,
POST /mounts/{mount_id}/files/move?path=<from>&to=<full new path>, permissionEDIT_MOUNTS. It renames or moves a file or a whole folder server-side.tois the complete new path (a rename ispath=a/b.md&to=a/c.md, a move isto=x/b.md), never a folder to drop into.Object stores have no rename, so the service enumerates the keys the path stands for (the same enumeration
delete_pathused, now shared as_path_keys), copies each to its new key with S3 CopyObject, then deletes the old keys. Copies run first and the delete last, so a failed copy leaves the source intact; a failed copy also cancels its in-flight siblings so nothing lands after the error.Responses:
Two store-level details: SeaweedFS refuses an empty trailing-slash key (a folder marker) as a copy source, so markers are re-created with the same zero-byte write
create_folderdoes. Anddelete_keysnow returns the count the store actually removed instead oflen(keys);delete_pathreports that honest count, and a move whose source keys survive raises instead of claiming success.The existence checks use one bounded page (
list_objects_page(max_keys=1)) rather than a recursive listing, so checking whethersrcis free no longer walks a 50k-filesrc-old/next to it.Not in this PR: a copy flag (Duplicate keeps its client path), cross-mount moves, a sessions-router mirror. The web client regenerates from
/openapi.jsonin the follow-up that points the Files pane's rename at this route.Tests
test_mounts_file_ops.py): file rename; folder move carrying the marker and descendants whilesrcs/stays put; missing, occupied, same-path and into-itself refusals with nothing moved;src-old/sorting betweensrcandsrc/fools neither side; a store that refuses to delete makes the move raise. Full API unit suite green.test_mounts_basics.py, against the local EE dev stack): folder move shows up in the listing, 409 on an occupied name, 404 on a missing source, 422 on... 35/35 in the file.