Skip to content

Align blob storage backend and root-container contracts - #3947

Open
vigoo wants to merge 4 commits into
mainfrom
gol-622-627-blob-backend-consistency
Open

vigoo wants to merge 4 commits into
mainfrom
gol-622-627-blob-backend-consistency

Conversation

@vigoo

@vigoo vigoo commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • align memory, filesystem, SQLite, and S3 with the documented namespace-root and blob/directory coexistence contract
  • make filesystem and SQLite deletion, metadata, listing, and missing-copy behavior consistent and preserve permanent host error classification
  • reject root-spelling container names across every host operation and verify permanent failures do not enter retry loops
  • update the Effect and TypeScript SDK container-creation contract, fakes, and executable tests
  • add a durable backend matrix plus worker host regressions for root paths, missing blobs, implicit directories, UTF-8 paths, and coexistence

Test coverage

  • cargo clippy -p golem-service-base -p golem-worker-executor --all-targets --no-deps -- -D warnings
  • filesystem, SQLite, and in-memory blob backend suites
  • worker blob service unit tests and durable executor root/missing-copy tests
  • Effect SDK blobstore tests and typecheck
  • TypeScript SDK blobstore tests, typecheck, lint, and formatting

Dependency

This PR is stacked on #3945 so the shared MinIO test-framework refactor lands first. It can be retargeted to main after #3945 merges.

Resolves GOL-622
Resolves GOL-627

vigoo and others added 4 commits September 23, 2026 09:30
Make namespace roots, blob-directory coexistence, and permanent error classification consistent across storage backends and SDKs. Add backend matrix and durable host regression coverage.

Amp-Thread-ID: https://ampcode.com/threads/T-01a0cd49-e92a-719f-b761-edfda02b7fe7
Co-authored-by: Amp <amp@ampcode.com>
@kmatasfp

Copy link
Copy Markdown
Contributor

Review of 46a1bc2. The mistake here is ours, not yours. GOL-622 and GOL-627 should have said to start only after the filesystem-snapshot branch lands on main. GOL-627 points at #3915, but #3915 (like #3891 and #3910) was merged into filesystem-snapshot, not main. That branch changed the blob storage contract, S3 and in-memory included:

  • one normalized path form on every backend (NormalizedBlobPath)
  • the full BlobNameError set (NotRelative, ParentDir, NotUtf8, NoName, TooLong, NulByte, DotSegment, Reserved)
  • BlobMissingError on every backend, S3 included
  • copy or move onto itself as a no-op
  • an in-memory backend that answers like S3
  • listing with sizes, put_raw_if_absent and BlobRangeError

This PR started from main without that, so it rebuilt part of the same contract under the same type names, with different rules in places. Merging the two now gives about 90 conflict hunks and a few breaks that merge cleanly and fail later.

Please hold this PR until the blob storage part of filesystem-snapshot is on main, then rebase it. After the rebase it should only need to change fs, SQLite and the executor's container_path.

Keep:

  • The fs and SQLite fixes: root paths, a blob over a created directory, a blob below a blob, fs delete of a missing path, and the typed missing copy. filesystem-snapshot still has those defects.
  • Your rule that a blob and a created directory can share a path (reads and exists prefer the blob, and each delete keeps the other). Ours says a write there is an error, but none of our backends actually enforce that, so we'll take your rule.
  • The new executor tests, the Effect SDK changes and the TS mock.

Drop, because filesystem-snapshot already covers them:

  • the memory.rs and s3.rs rewrites
  • the local BlobNameError, validate_relative_blob_path and reject_root_blob_path changes (use normalized_blob_path)
  • the list_dir rules "directly below" and "can occur twice". Ours are "each created directory at any depth" and "one time", which is what S3 does.
  • the TS test "whole-object read recovers when the backend treats end as exclusive" and the mock's endExclusive flag. The branch removed that fallback from src/blobstore.ts.

Code on the branch that the new layout breaks without a conflict:

  • fs list_blobs_below walks regular files, so it would list …/~blob and …/~dir, and the rustic snapshot backend would find no packs. It needs to report node paths.
  • fs put_raw_if_absent and get_raw_slice still use path_of, the old layout. They should use blob_of. If you remove path_of, the compiler will find every caller.
  • SQLite put_raw_if_absent uses ON CONFLICT(namespace, parent, name), which matches no constraint under your new key. Every call would fail, and the compressed oplog archive depends on it. It needs to become ON CONFLICT(namespace, parent, name, is_directory).

Three issues in the PR itself, which apply either way:

  1. An empty object name (write_data("c", "", …)) writes a blob at the container's path. After that, container_exists("c") is false on memory, fs and SQLite, while S3 still says Directory. This overlaps with GOL-635 (Reject root object names in blob storage #3942).
  2. list_dir(".") on fs returns ./c and ./x instead of c and x. The root test covers "." for the other operations but not for list_dir.
  3. A race in fs pruning: delete("a/b") can prune node a between put_raw("a") checking that the parent exists and writing the blob. The write then fails with a bare NotFound, which is treated as transient. create_dir has the same window.

Once it's rebased, I'd also move the coexistence scenarios off the MinIO dimension and onto the scripted S3 transport in storage/blob/s3/tests.rs, as GOL-622 asks. As it stands, MinIO's delete_dir with blob c and blob c/x returns false and leaves c/x in place, which shows nothing about how AWS behaves.

Sorry for the extra work. The fs and SQLite parts are what we need, and they carry over.

vigoo commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. We’ll keep this PR open and hold off on rebasing or further changes until the filesystem-snapshot work has been merged into main. After that, we’ll rebase and reconcile this change with the settled blob-storage contract.

@vigoo
vigoo force-pushed the gol-629-minio-test-framework branch from f905f1f to 2a4a400 Compare September 24, 2026 07:52
Base automatically changed from gol-629-minio-test-framework to main September 29, 2026 09:08
@vigoo
vigoo requested a review from a team September 29, 2026 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants