fix(storage): route the read path through the key contract (#746)

[xe-nvdk]

Closes #746.

What this is

#743 made the key rule a property of every Backend. That covers writes. Reads never go through a Backend at all: Arc builds an s3:// or azure:// URI and hands it to DuckDB, and iceberg-go writes through its own FileIO. The contract did not reach either.

The issue framed this as a structural gap. Auditing it turned up two live bugs, both on deployments with storage.s3_prefix set, and both measured against a live MinIO with DuckDB httpfs rather than reasoned about.

Live bug 1: retention deleted nothing

buildParquetPath built s3://{bucket}/{key} with no GetPrefix(), while every write goes under the prefix. So the URL named an object that does not exist, getFileMaxTimeAndRowCount errored, and deleteOldFiles logged a warning and skipped the file. Every file took that path.

Measured, prefix tenant:

retention (before)  s3://arctest/probedb/cpu/2020/01/01/00/probe.parquet
                    -> HTTP 404 Not Found, rows=0
delete   (correct)  s3://arctest/tenant/probedb/cpu/2020/01/01/00/probe.parquet
                    -> err=nil, rows=1

Nothing said so. runPolicy then called recordExecutionComplete(..., "completed", 0, ...), so the API and the execution history reported a successful run. An operator checking whether retention was working saw green.

Provenance, not inferred: git log -S puts the builder at fe5a294 (#170); the prefix option arrived later in 409fe0b (#258), and git show --stat 409fe0b touches internal/api/delete.go, which is where getQueryPath got its GetPrefix(), and does not touch internal/api/retention.go. git tag --contains 409fe0b puts it live in v26.03.2 through v26.09.1.

That is exactly the failure #746 is about: two longhand copies of one builder, one updated, the other not found. Patching the single line would have left the next copy to be missed the same way.

Live bug 2: single-tier partition pruning was dead

extractStoragePrefix stripped the scheme and bucket but not the configured prefix, and List/ListDirectories prepend the prefix themselves. Measured:

parentDir  : s3://arctest/tenant/pdb/cpu/2026/09/12
extracted  : "tenant/pdb/cpu/2026/09/12"     <- prefix still attached
ListDirectories("tenant/pdb/cpu/2026/09/12") -> []        (listed tenant/tenant/...)
ListDirectories("pdb/cpu/2026/09/12")        -> [13]      (correct)

Empty with no error, so every partition was judged absent and OptimizeTablePath fell back to the unpruned glob. Results stayed correct; the optimisation silently never applied. tier_pruning.go:8-12 documents this exact hazard and the tiered path avoids it. Only the single-tier path was left doing scheme-and-bucket surgery.

The structural change

• storage.ObjectURI is the one validated key-to-location builder. Retention, delete and iceberg use it, so a prefix cannot be honoured in one copy and forgotten in another.
• storage.GetStoragePath returns an error and validates its arguments as single segments. ValidateKey(db + "/" + m) accepts a measurement of a/b and reads a different directory.
• storage.ValidateKeySegment is the primitive whose absence caused the sprawl: five callers needed a per-segment rule, this package exported none, so each wrote its own.
• storage.ValidateGlobSafe is the rule reads need and writes do not. ValidateKey accepts *, ?, [, { because a write of such a key names exactly one object; in read_parquet they are pattern operators, so cpu* reads every measurement starting with cpu. It is applied at the sinks that interpolate into a DuckDB path, not inside ObjectURI, because iceberg-go and os.Open read the same key literally.

Reachable rather than theoretical: validateSpokeID has no character allowlist, and a spoke ID is the first path segment of everything that spoke writes.

validateSpokeID("rocket*01") = <nil>

Why an error and not an inert path

My first draft resolved a rejected name to a glob matching nothing, following the arcInvalidIdentifierSentinel precedent. That was wrong here. isNoFilesFoundError turns a no-match glob into Success: true, RowCount: 0 and calls m.IncQuerySuccess(), so a rejected name would have been reported as a successful query over an empty table, and cached. The sentinel's own doc says it is a backstop for a case ValidateSQLRequest already 400s; a GetStoragePath failure has no upstream rejection.

The error leaves the regexp-driven transform through a context-carried struct, the same escape hatch pruning.WithVolatileResult already uses for the same reason, so no rewriter changed shape.

Validator reconciliation

The issue asked for this. They disagreed with each other:

	vs the contract
cluster.sanitizeFetchPath	believed stricter, measurably looser: accepted ., backslashes, 256-byte segments, 1401-byte keys
delete.go inline .. substring	refused a..b, which is writable and queryable but was not deletable
api.isSafeStoragePathSegment	stricter by a leading-dot rule, which is legitimate and stays
raft.ValidateManifestPath	looser on six axes, deliberately, and must stay so

They now share one rule. The two deliberate divergences are pinned by a test that asserts direction rather than an exception list, because raft is looser on six independent axes and a list degenerates into an allowlist of inputs someone happened to think of.

That test earned itself twice. It found that raft is stricter on one axis, rejecting any colon, while validateSpokeID accepts rocket:01, so those files are stored and can never be registered in the manifest. Out of scope here, filed as #751 and pinned by the test so it cannot widen silently. Also filed #752 for the four spellings of the DuckDB path escaper.

And on the rebase onto #744 it caught a stale claim in freshly merged code: #744 reserved PartSuffix in ValidateKey, while ValidateListPrefix's doc still said it was looser "in exactly two ways". The behaviour is right (a prefix cannot name an object, and refusing one ending in .part would make the staged partials that suffix was reserved for unlistable), so the comment is corrected and the list is now checked by the test rather than only asserted in prose.

Rebased onto #744 and #747

Both landed while this was open and both touched internal/storage/local.go. Two things changed as a result:

• My LocalBackend.List/ListObjects filtering is dropped: Remaining many-to-one path mappings outside LocalBackend.validatePath #744 already added it, with a ToSlash mine lacked. Only the test coverage noted above survives.
• validateKeyBody's segment-length check now uses Remaining many-to-one path mappings outside LocalBackend.validatePath #744's MaxUsableKeySegmentLen, which leaves headroom for the staging suffix, so the extracted checkSegment follows it rather than reintroducing MaxKeySegmentLen.

Tests

Written against #743's lesson that an accept/reject-set test cannot see a mapping bug.

• Two objectstore-tagged regressions against real MinIO, one per live bug. Both verified to fail against the old code: reintroducing the missing prefix reproduces the same 404.
• TestObjectURIAndGetStoragePathShareOneRoot and TestGlobParseYieldsTheBackendRoot pin the three participants (builder, glob, and the pruner's regex parse of that glob) against each other. The seam between them is where both bugs lived.
• TestObjectURINamesTheFileTheBackendWrites asserts against the filesystem, not against a string this package also computes.
• Direction-asserting reconciliation tests, described above.

Also in here

• Retention reports files it had to skip rather than recording a clean success. Such a file is skipped on every future run too, so its data never ages out, which is the same silent shape as the prefix bug.
• Local listings gained coverage for ListObjects and for an unusable key that is not a write-staging partial. The filtering itself is Remaining many-to-one path mappings outside LocalBackend.validatePath #744's; these are the cases its own tests do not reach, and ListObjects matters because the reconciler prefers it over List.
• A storage root of / no longer disables partition pruning; the glob parser required a non-empty root.
• Removed three unused S3 URI builders rather than leave further copies of the thing this PR consolidates.

Upgrade note

On prefixed S3, the first retention run after this fix will delete the whole backlog that accumulated while retention was doing nothing. That is the policy doing what it was configured to do, but it is not small and not reversible. The release notes tell operators to dry-run each policy first; the dry run is now accurate where it previously reported zero.

Verification

go build, go vet and go test -race all with -tags=duckdb_arrow, which is what the Makefile and CI use, plus the objectstore suite against MinIO. All green.