refactor(server): migrate aiofiles to object storage use obstore , drop S3 native versioning - #244
Open
JonnyTran wants to merge 4 commits into
Open
refactor(server): migrate aiofiles to object storage use obstore , drop S3 native versioning#244JonnyTran wants to merge 4 commits into
aiofiles to object storage use obstore , drop S3 native versioning#244JonnyTran wants to merge 4 commits into
Conversation
Object versioning was enabled on every workspace bucket but was never a working feature, and in several places it actively misled callers: - `GET /api/v1/file/...` accepted `version_id` and never forwarded it to S3, so asking for an old version silently returned the current one. - The handler never emitted the `Version-Id`/`Is-Latest`/`Version-Tag` headers, because `FileObjectResponse.http_headers` had no caller. The frontend read exactly those headers, so `is_latest` was permanently null and the "Fetch latest schema" button could never render. - Deletion was modelled with delete markers that three separate call sites reconstructed from the same `etag == "" and size == 0` heuristic; the CLI integration test for upload/download/delete has been skipped ever since with "buckets with versioning enabled still list deleted files". It is un-skipped here. - `delete_bucket` drained with `list_objects_v2` and issued unversioned deletes, so noncurrent versions survived and `DeleteBucket` 500'd with BucketNotEmpty. - `SchemaVersion.object_version_id` was written best-effort and never read. Schema versioning is unaffected: a version's identity is `(dataset_id, version)`, allocated under a row lock so each version lands on its own key, and its integrity is `checksum`. Neither depended on the object store. `ListObjectsResponse.objects` becomes `list` rather than `Iterable` — the deleted `assign_version_id` validator was the only thing coercing it, and both `__len__` and `__getitem__` already assumed a list. Breaking: `include_version` and `version_id` query params are gone (422 on old SDKs), as are `is_latest`/`version_id`/`version_tag` on `ObjectMetadata` and the `--versions`/`--version-id` CLI flags.
Replaces the aioboto3/botocore dict-shape contract and the hand-rolled local backend with one obstore port. `contexts/files.py` now talks to `S3Store` in production and `LocalStore` in local dev, behind an `ObjectStorage` that resolves a bucket name to a store (obstore binds a store to one bucket at construction, so the per-call `bucket` argument becomes a lookup). What this deletes: - `aiofiles`, entirely. Its only user was `LocalFileClient`, ~430 lines subclassing aiobotocore's generated client to fake S3 over a directory tree. It was untested, and derived paths with `Path.with_suffix`, so any key containing a dot did not round-trip (`pdf/a.b.pdf` came back as `pdf/a`). It also silently lacked `head_bucket`, so `bucket_exists` was always false locally. - The read-whole-file-into-RAM path in `GET /file`. `head_object` + a full `get_object` + a 10MB chunking branch collapse into one `get_async` and `StreamingResponse(result.stream())`. `aioboto3` survives only in the new `contexts/buckets.py`: obstore binds to an existing bucket and has no admin API, so CreateBucket/DeleteBucket/HeadBucket cannot move. That is 3 functions and 4 call sites rather than the app-wide contract it was. Details worth knowing: - obstore raises the builtin `FileNotFoundError` for a missing key, not `obstore.exceptions.NotFoundError`. - An HTTP range is inclusive of its end and obstore's is exclusive, so the range handler passes `(start, end + 1)`. `Range: bytes=0-0` must return one byte. - `LocalStore` rejects `put` attributes outright, so local mode recovers the content type from the key. The keys we mint (`pdf/`, `thumbnails/`) carry no extension, hence the prefix map -- without it a local-dev PDF is served as octet-stream and the viewer breaks. - `ObjectMetadata.metadata` no longer strips an `x-amz-meta-` prefix. obstore returns user attributes under their own keys, so that stripping discarded every one of them. Verified against MinIO. Also drops the ES mapping properties for the removed version fields. The `table` field object sets `dynamic: false`, which overrides the index-level `dynamic: strict`, so a legacy response still carrying those keys is ignored rather than rejected -- no reindex needed. tests/unit/contexts/test_files_store.py is new and drives a real `LocalStore`. Nothing previously exercised a storage backend end to end; every other test patches `contexts.files.*`.
Two HIGH findings from review of the obstore port (roborev #354, #355). `contexts/imports.py` read `shared_resources["storage"]` directly. Removing `create_s3_client()` from the lifespan left nothing to populate it eagerly, so a bulk upload arriving before any other storage-dependent request raised "S3 client not available" -- and an RQ worker, which never runs the lifespan at all, would have failed every time. It now awaits `get_storage()`, which resolves and caches on first use. `contexts/buckets.delete` drained with obstore's `list()`, which returns current objects only. Removing `put_bucket_versioning` does not disable versioning on buckets that already have it, so a workspace created before this branch keeps its noncurrent versions and delete markers and `DeleteBucket` fails with `BucketNotEmpty`. Worse, a key hidden behind a delete marker does not appear in the current-object listing at all, so the drain removed nothing. Deletion now pages `list_object_versions` and batch-deletes every version and delete marker, falling back to the old drain on a backend without version listing. The previous commit message claimed this case was already fixed; it was only ever true of buckets created after the change. For the buckets that already exist, `suspend_versioning` stops them minting new versions. It runs when `create` lands on a pre-existing bucket and is exposed as a `bucket_versioning` workspace-doctor check, which is the operator-facing migration path. This also closes the leak opened by dropping the `expire-noncurrent-layout-versions` lifecycle rule: layout JSON is rewritten on every commit, so a still-versioned bucket grew without bound and nothing reaped it. Verified against MinIO on a bucket seeded with 3 versions and 1 delete marker: the old path fails with BucketNotEmpty, the new path drains and deletes it, and suspension leaves the bucket `Suspended`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Warning Review limit reached
Next review available in: 36 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (50)
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 |
JonnyTran
changed the title
aiofiles to object storage use obstore , drop S3 native versioning
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Replaces
aiofiles+ the app-wideaioboto3contract with a single obstore-backed storageport, and removes S3 native object versioning from the server, frontend and SDK.
Net -1408 / +985 across 51 files.
Why
Two separate problems that turned out to be entangled.
aiofilesearned nothing. Its only user wasLocalFileClient— ~430 lines subclassingaiobotocore's generated client to fake S3 over a directory tree, with a
.versions/dir,symlinks and sidecar
.metadata.json. It was untested, derived paths withPath.with_suffix(so any key containing a dot did not round-trip:
pdf/a.b.pdfcame back aspdf/a), andsilently lacked
head_bucket, sobucket_existswas always false in local dev.S3 native versioning was not merely unused — it was lying to users.
GET /api/v1/file/...acceptedversion_idand never forwarded it to S3. Asking for an oldversion silently returned the current one.
Version-Id/Is-Latest/Version-Tagheaders, becauseFileObjectResponse.http_headershad no caller. The frontend read exactly those headers, sois_latestwas permanentlynulland the "Fetch latest schema" button could never render.the same
etag == "" and size == 0heuristic. The CLI integration test forupload/download/delete has been skipped ever since with "buckets with versioning enabled
still list deleted files" — un-skipped here.
SchemaVersion.object_version_idwas written best-effort and never read.Schema versioning is untouched and unaffected: a version's identity is
(dataset_id, version),allocated under a row lock so each version lands on its own key, and its integrity is
checksum.Neither ever depended on the object store.
What changed
aiofilesaioboto3contexts/buckets.py(3 functions, 4 call sites) — obstore binds to an existing bucket and has no admin API, so bucket lifecycle cannot moveGET /fileresult.stream()instead of loading whole files into RAMLocalStoreinstead of a hand-rolled mockThings worth a reviewer's attention
FileNotFoundErrorfor a missing key, notobstore.exceptions.NotFoundError(which exists, but is not what gets raised). Catching thelatter would turn every 404 into a 500.
(start, end + 1).Range: bytes=0-0must return one byte.LocalStorerejectsputattributes outright, so local mode recovers the content typefrom the key. The keys we mint (
pdf/,thumbnails/) carry no extension, hence the prefixmap — without it a local-dev PDF is served as
octet-streamand the viewer breaks.ObjectMetadata.metadatano longer strips anx-amz-meta-prefix. obstore returns userattributes under their own keys, so that stripping discarded every one of them. Caught only by
the MinIO run, not by any unit test.
tablefield setsdynamic: false, which overrides the index-leveldynamic: strictfor that subtree. A legacy response still carrying the removed version keysis ignored rather than rejected, so no reindex is needed.
Legacy buckets
Removing
put_bucket_versioningdoes not disable versioning on buckets that already have it.Those keep their noncurrent versions and delete markers, and a key hidden behind a delete marker
does not appear in a current-object listing at all — so the drain removed nothing and
DeleteBucketfailed withBucketNotEmpty.Deletion now pages
list_object_versionsand batch-deletes every version and delete marker.suspend_versioningstops existing buckets minting new ones; it runs whencreatelands on apre-existing bucket and is exposed as a
bucket_versioningworkspace-doctor check. That alsocloses the leak opened by dropping the
expire-noncurrent-layout-versionslifecycle rule —layout JSON is rewritten on every commit, so a still-versioned bucket grew without bound.
Breaking
Pre-1.0, so removed outright rather than deprecated:
include_versionandversion_idquery params (422 on old SDKs)is_latest/version_id/version_tagonObjectMetadata--versions/--version-idCLI flagsschema_versions.object_version_id(migrationa1c9f4e2b703)Verification
test_files_store.pyLocalStore— nothing previously exercised a backend end to endtest_buckets.pyNotImplementedfallback, suspensionBucketNotEmpty, confirmed the fix drains and deletesupgrade -> downgrade -1 -> upgradeclean, column confirmed droppedFollow-up, not in this PR
extralit-hf-space/extralit_ocr/jobs.pyimportsget_s3_client, which this rename breaks. It isa separate repo (submodule at detached HEAD), so the two-line fix
(
get_s3_client->get_storage) needs its own PR there and cannot land atomically with this one.