fix(api): eager archive errors, capped count-only listing, rollup tests#5411
Conversation
dosubot
Bot
added
the
size:L
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe mount service adds cursor-based count-only listing, excludes folder markers from totals, and separates archive work-list construction from ZIP member streaming. The archive route passes the precomputed work list to the streaming service, with expanded unit coverage. ChangesMount file operations
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant FastAPIArchive as FastAPI archive route
participant MountsService
participant ObjectStore
participant ZipStream as ZIP stream
FastAPIArchive->>MountsService: build_archive_work_list(project_id, mounts)
MountsService->>ObjectStore: list archive source objects
ObjectStore-->>MountsService: ordered file metadata
MountsService-->>FastAPIArchive: archive work list
FastAPIArchive->>MountsService: iter_archive_members(work)
MountsService->>ObjectStore: prefetch file bytes
ObjectStore-->>MountsService: file bytes
MountsService-->>ZipStream: ordered archive members
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Live QA recording: paging termination, special-char folders, download-all. Details in the PR body. pr5400-fixes-qa.mp4 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ab6f001d-359e-429f-8229-7baf550e7cd9
📒 Files selected for processing (3)
api/oss/src/apis/fastapi/mounts/utils.pyapi/oss/src/core/mounts/service.pyapi/oss/tests/pytest/unit/test_mounts_file_ops.py
| async def build_archive_work_list( | ||
| self, | ||
| *, | ||
| project_id: UUID, | ||
| mounts: List[Tuple[UUID, str, str]], | ||
| concurrency: int = _ARCHIVE_READ_CONCURRENCY, | ||
| ) -> AsyncIterator[Tuple[str, int, Optional[int], bytes]]: | ||
| """Yield ``(zip_path, size, mtime, raw_bytes)`` for the files in the given mounts, in order — | ||
| the basis for a STREAMING archive. Each mount is a ``(mount_id, zip_prefix, source_path)``: | ||
| ) -> List[Tuple[str, str, int, Optional[int]]]: | ||
| """Build the ordered archive work list for the given mounts. | ||
|
|
||
| Each mount is a ``(mount_id, zip_prefix, source_path)``: | ||
| ``source_path`` scopes it to a FOLDER within the mount ("" = the whole mount, for "download | ||
| all"); ``zip_prefix`` places its files under ``prefix/`` in the zip (e.g. "agent-files" for | ||
| the folded agent mount). Folder markers are skipped. Reads up to ``concurrency`` files AHEAD | ||
| (bounded ordered prefetch) — never buffering the zip whole nor hammering the store. | ||
| the folded agent mount). Folder markers are skipped. Each work item is a | ||
| ``(zip_path, storage_key, size, mtime)`` tuple. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Return a named DTO instead of raw tuples for the archive work list.
build_archive_work_list returns List[Tuple[str, str, int, Optional[int]]] and iter_archive_members (Lines 1297-1305) consumes/yields the same positional-tuple shape. Define a named DTO (e.g. ArchiveWorkItem with zip_path, storage_key, size, mtime) in api/oss/src/core/mounts/dtos.py and use it here; downstream unpacking in api/oss/src/apis/fastapi/mounts/utils.py and test_zip_paths_include_mount_prefix would switch to attribute access.
As per coding guidelines: "Do not return raw dicts or tuples from service methods or clients; define named DTOs in core/{domain}/dtos.py instead."
Source: Coding guidelines
Railway Preview Environment
|
…of a broken 200 The download-all archive resolved each mount lazily inside the streaming generator, after the 200 headers were sent, so MountNotFound / MountStorageUnavailable never reached @handle_mount_exceptions and a bad mount id yielded a corrupt zip. Split iter_archive_members into build_archive_work_list (resolve + list, awaited before the StreamingResponse is built) and iter_archive_members (prefetch/yield only), so those errors surface as real 404/503 through the decorators. Claude-Session: https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU
The raw (non-git-aware) count-only path ran a full uncapped list_objects_v2 and hardcoded truncated=False, so a huge tree was enumerated whole. Page it bounded at _COUNT_CAP and report total_capped=True when more remain, matching the git-aware branch's bounded-count contract. Re-adds the generic ObjectStore.list_objects_page pagination primitive (removed alongside the flat Files view); this bounded count is now its sole consumer. Claude-Session: https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU
Adds pure-function tests for the recent-files rollup: clone-then-edit collapses to the top directory, an old-plus-fresh directory does not collapse, an untimed leaf blocks collapse, a single-batch history produces no rollup, and shallow-to-deep resolution picks repo/ over repo/web/. Pins current behavior; no algorithm change. Claude-Session: https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU
Switch validate_file_path from a character allowlist to a denylist so real folder names (route groups, npm scopes, `c++`, `#`, `~`, non-ASCII) that the lazy-browse flow round-trips no longer 422 on every expand. Reject only unsafe input: absolute paths, empty/`.`/`..` segments, NUL and control characters. Harden the archive path in the same pass (opposite direction): validate ArchiveMount prefix/path, sanitize `..` out of zip entry names to prevent zip-slip, and make the Content-Disposition fallback ASCII-only so a non-latin-1 filename stops 500ing on Starlette's latin-1 header encoding. Claude-Session: https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU
"archive" is this router's soft-delete lifecycle verb (archive_mount two registrations below), and the collision is what forced the route-ordering hack. Rename the download-all zip route to the non-lifecycle /files/export and update the operation_id, handler, and all three frontend touchpoints (driveMedia streaming + buffered paths, and the hand-added client method, which stays hand-written until the Fern regen ticket). Claude-Session: https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU
`depth` advertised ge=1 but only depth==1 is implemented; depth=2 silently fell through to the most expensive full-tree browse branch. Constrain it to le=1 so unsupported depths 422. Type `order` as a Literal so a typo 422s at the boundary instead of returning the flat view unsorted. Claude-Session: https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU
Replace the archive path's anonymous `(mount_id, prefix, path)` triples with a MountArchiveSource DTO, per the typed-DTO convention. Thread it through the service, archive utils, router, and unit tests. (The companion MountFilePage DTO from the original change is dropped: the flat paged listing it named was removed with the Files drawer's flat view.) Claude-Session: https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU
`total` had three undocumented computed meanings; state the one contract explicitly (entries the queried view returns before any limit: leaf files in flat/recency modes, files-plus-folders in shallow/browse). Also stop the download-all docstring from promising >4 GB files, which are out of product scope. Claude-Session: https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU
test_mounts_service.py still asserted the old allowlist rejected angle brackets; under the denylist they are legitimate filename characters. Assert they (and route-group names) are accepted, and pin control-char rejection. Claude-Session: https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU
c93b418 to
d7d029c
Compare
dosubot
Bot
added
size:M
mmabrouk
changed the title
|
Rebased both PRs onto Arda's latest tip |
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
api/oss/src/core/mounts/service.py (1)
1089-1104: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReturn a named DTO instead of raw tuples for the archive work list.
build_archive_work_listreturnsList[Tuple[str, str, int, Optional[int]]]anditer_archive_membersconsumes the same positional-tuple shape. Define a named DTO (e.g.ArchiveWorkItemsubclassing Pydantic'sBaseModel) inapi/oss/src/core/mounts/dtos.pyand use it here.As per coding guidelines: "Service methods must return typed DTOs (Pydantic
BaseModelsubclasses), not raw dicts, tuples, orAny; useOptional[DTO]for missing entities andList[DTO]for collections."Also applies to: 1130-1140
Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ed44346-4d94-4896-aa12-66ec6599f080
📒 Files selected for processing (4)
api/oss/src/apis/fastapi/mounts/utils.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/store/storage.pyapi/oss/tests/pytest/unit/test_mounts_file_ops.py
🚧 Files skipped from review as they are similar to previous changes (1)
- api/oss/src/apis/fastapi/mounts/utils.py
fix(api): relax drive path validation, harden archive, rename export route
dosubot
Bot
added
size:L
2 participants
Context
Part 1 of 2. These two stacked PRs implement the pre-merge fixes agreed for #5400 ("[AGE-3965] refactor(drive): virtualize surfaces + unify Files drawer" by @ardaerzin) so that PR can merge on a sound base. A four-lens backend review (Claude Fable 5) and an independent Codex review, run without seeing each other, converged on the same set of blockers. This PR carries the correctness fixes; the contract and validation fixes follow in #5412, stacked on top of this one.
The surviving story is silent failure on the archive path.
POST /files/export(then/archive) built itsStreamingResponsefirst and resolved each mount lazily inside the generator, after the 200 headers were already on the wire. A request naming a missing mount, or arriving while storage was down, returned HTTP 200 with a truncated or empty zip and no visible error. This is the exact silent-corruption class #5400 itself diagnosed and fixed for the mtime bug. Alongside it, the plain count-only listing walked the whole tree with no cap, and the recency-rollup heuristics that decide which entries collapse had no tests pinning them. This PR fixes all three, with tests.Changes
Resolve archive mounts before streaming. The endpoint now resolves every requested mount and builds the work list eagerly, before constructing the response, so
MountNotFoundandMountStorageUnavailablereach the exception decorators and return a real 404 or 503 instead of a 200 with a broken zip.Cap the plain count-only listing. The git-aware descent already stopped counting at
_COUNT_CAPand settotal_capped=True. The plain (non-git-aware) count-only branch enumerated the entire tree with no cap and hardcodedtruncated=False, so a raw count on a huge mount walked every object. It now honors the same_COUNT_CAPand reportstotal_capped=True, matching the git-aware branch.Pin the recency-rollup heuristics.
_rollup_recent_entriesdecides which recent entries collapse into their parent, and it had no test coverage. This PR adds five tests: clone-then-edit collapse, old-plus-fresh no collapse, untimed-leaf blocks collapse, single-batch behavior, and shallow-to-deep resolution.Scope note
The review's headline finding was a verified infinite loop in the flat paged listing (
list_files_page): when the cursor stepped over a gitignored directory it appended a U+FFFF sentinel, assuming that byte sequence sorted after every key underneath, but astral-plane code points (emoji, CJK Extension B) encode fromF0up and sort after the sentinel, so a single astral-named file under an ignored directory looped forever. That fix shipped in this PR's first version. Arda then removed the flat paged view entirely in387c62ebef(the flat listing was dropped by design), which moots the fix: its target code no longer exists. It was dropped in the rebase onto387c62ebef.The count-cap fix above re-adds exactly one thing that
387c62ebefdeleted as dead code: the genericObjectStore.list_objects_pagestore primitive. The cap needs a bounded paging primitive to count in fixed-size pages, and after the rebase that primitive now has a live caller again. No flat-view code (the servicelist_files_page, its route,MountFilePageResponse, or the webdriveFlatFiles.ts) returns.Verification
Live QA (recorded). A 60-second UI recording and a wire matrix were run on an isolated docker stack against the pre-
387c62ebeftip and attached as a comment on this PR. Read them with that caveat: the paging-termination scenarios are now moot (the flat view is gone), while the export/404, special-character folder paths, constraint 422s, and CJK-header checks all target surviving code and remain valid.Tests. The full API unit suite is green on the rebased tip (1373 passed). The mounts file-ops suite covers the count cap and the five rollup heuristics (
uv run pytest oss/tests/pytest/unit/test_mounts_file_ops.py -q).Independent review. Codex reviewed the pre-rebase diff at high reasoning effort; its confirmed catches were fixed and regression-tested. The paging catch it confirmed is moot with the flat view removed.
Merge order
Merge this PR into
fe-refactor/drive-surfacesfirst, then the stacked contract PR into this branch, then #5400 intomain. Merging is Mahmoud's decision. Ready for review, do not merge without Mahmoud's go.https://claude.ai/code/session_01PTkjdoAPbhSEFR3ubpgzBU