Skip to content

fix(store): normalize trailing slashes in the S3 key prefix - #2

Merged
xjtu-ctgg merged 1 commit into
mainfrom
fix/s3-key-prefix-trailing-slash
Nov 2, 2026
Merged

xjtu-ctgg merged 1 commit into
mainfrom
fix/s3-key-prefix-trailing-slash

Conversation

@xjtu-ctgg

@xjtu-ctgg xjtu-ctgg commented Oct 26, 2026 •

Copy link
Copy Markdown
Collaborator

Description

PhysicalPrefix() appends "/objects/" to MOONCAKE_S3_KEY_PREFIX as-is, so a prefix ending in / produces <prefix>//objects/. OSS folds // on PUT and GET but not in ListObjectsV2. As a result:

  • ScanMeta finds nothing.
  • Providers never re-register LOCAL_DISK replicas.
  • Every cold read through a reader pinned with MC_STORE_REQUIRED_OFFLOAD_ENDPOINT fails.

The fix strips trailing / once, in S3ObjectStorageConfig::FromEnvironment() and in the adapter constructor. PhysicalPrefix, WriterPrefix, DeletionIntentKey and the durable-delete namespace then share one value. Validate() rejects a prefix made only of slashes.

No migration is needed: objects written earlier were stored under the folded single-slash path, and they are found as-is.

Upgrade note. The durable-delete namespace identity includes key_prefix, and the Master journals it with each delete. A deployment that ran with a trailing-slash prefix and still has an unfinished durable delete at upgrade time will keep failing that delete with UNAVAILABLE_IN_CURRENT_STATUS. The reason: no provider reports the old p/ identity any more. Nothing is deleted or resurrected wrongly. To avoid it, finish or drain pending durable deletes before upgrading.

Tracked in MindLab-Research/mint#1775 (this fork has issues disabled). The Mint deploy-side normalization is in a separate Mint PR.

Module

  • Mooncake Store (mooncake-store)

Type of Change

  • Bug fix

How Has This Been Tested?

Built on a26dedb1 (this branch's base) with GCC 13, -O3.

Unit tests: object_storage_adapter_test passes 22/22. It has two new cases:

  • KeyPrefixTrailingSlashesNameOneNamespace: p, p/ and p/// give the same prefix, including through FromEnvironment().
  • RejectsPrefixThatIsOnlySlashes

With the normalization removed, both new cases fail.

Real OSS (Aliyun cn-beijing, existing validation prefix with a trailing / added; read-only):

  • One fresh provider with no local state.
  • A reader pinned to that provider's offload endpoint.
  • Three objects written on 09-08 (64 KiB, 4 MiB, 16 MiB), checked against the SHA-256 recorded at write time.
Build ScanMeta Cold reads SHA-256
a26dedb1 total_keys=0 0/3 (rc=-704) —
this branch total_keys=12 3/3 3/3 match

Control: a26dedb1 with the prefix without a trailing / also finds total_keys=12.

Format and spelling: scripts/code_format.sh --check --changed-lines --base origin/main (clang-format 20.1.8) and typos are clean.

Fork CI (run 40322874200): format, spell-check, Ruff, docs, build-flags and every wheel build (standard, cu13, EFA ×3, ROCm ×2) pass. Two jobs fail, neither in code touched here:

  • CTest unit tests: master_service_test.cpp and master_service_group_test.cpp don't compile. They call WrappedMasterService::GetReplicaList as a plain return, but it returns Lazy<…> on a26dedb1. Unmodified main fails identically (run 40329230656).
  • test-wheel-ubuntu (22.04, 3.12): pip resolved torch 2.14.1, which the wheel's Mooncake PG wasn't built against, so 6 PG tests fail on import. The same job on 24.04 / 3.10 passes.

PhysicalPrefix() appends "/objects/" to MOONCAKE_S3_KEY_PREFIX verbatim, so a
prefix ending in "/" yields "<prefix>//objects/". OSS folds "//" on PUT and
GET but not in ListObjectsV2, so ScanMeta finds nothing, providers never
re-register LOCAL_DISK replicas, and readers pinned with
MC_STORE_REQUIRED_OFFLOAD_ENDPOINT fail every cold read.

Strip trailing '/' once, in FromEnvironment() and in the adapter constructor,
so PhysicalPrefix, WriterPrefix, DeletionIntentKey and the durable-delete
namespace all share one value. Validate() rejects a prefix that is only
slashes. Objects already written under the folded single-slash path are
discovered as-is; no migration.

Tracked in MindLab-Research/mint#1775.
@xjtu-ctgg
xjtu-ctgg marked this pull request as ready for review October 26, 2026 12:06
Copilot AI balanced review requested due to automatic review settings October 26, 2026 12:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.

@xjtu-ctgg xjtu-ctgg closed this Oct 26, 2026
@xjtu-ctgg xjtu-ctgg reopened this Oct 26, 2026
@xjtu-ctgg
xjtu-ctgg merged commit 94140ee into main Nov 2, 2026
18 of 21 checks passed
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