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

RELEASE_NOTES_2026.09.2.md

@@ -244,7 +244,20 @@ is published. Responsibly reported by **[@rexpository](https://github.com/rexpos
    that `PUT /api/v1/retention/:id` replaces the whole row, so a request that
    omits `database` is now a 400 rather than silently storing an empty name,
    which used to make that policy enumerate every database.
-5. **Query response envelopes gained two optional keys** (`rows_capped`,
+5. **Retention will delete a backlog on its first run if you use an S3 prefix.**
+   If `storage.s3_prefix` is set, retention has been deleting nothing at all
+   since v26.03.2 (see *Retention deleted nothing on S3 deployments with a
+   configured prefix* below), while reporting every run as completed. After
+   upgrading, the first run of each policy will remove everything already past
+   its cutoff, which on a long-running deployment can be most of the data in the
+   affected measurements. This is the policy doing what it was configured to do,
+   but it is not a small delete and it is not reversible. Before upgrading,
+   check what each policy would remove with a dry run
+   (`POST /api/v1/retention/:id/execute` with `{"dry_run": true}`, which is now
+   accurate where it previously reported zero), and confirm the retention window
+   is still the one you want. Deployments on local storage, on Azure, or on S3
+   without a prefix are unaffected: retention has been working correctly there.
+6. **Query response envelopes gained two optional keys** (`rows_capped`,
    `row_cap`), emitted only when an Enterprise governance row cap truncated
    the result. Conforming JSON and msgpack decoders are unaffected: the keys
    are absent from every uncapped response, and a capped msgpack envelope
@@ -401,6 +414,109 @@ The filename is now the job id alone, which is already unique and already
 carries the partition. Nothing reads these names, so manifests written by an
 earlier version are still found and recovered, and no migration is needed.
 
+### Retention deleted nothing on S3 deployments with a configured prefix ([#746](https://github.com/Basekick-Labs/arc/issues/746))
+
+If `storage.s3_prefix` was set, retention never deleted a single file.
+
+Retention lists the files for a measurement, then asks DuckDB for each file's
+maximum timestamp to decide whether it is past the cutoff. It built that file's
+URL as `s3://{bucket}/{key}` and left the configured prefix out, while every
+write goes under the prefix. So the URL named an object that does not exist,
+the read failed, and the per-file error handler logged a warning and moved on to
+the next file. Every file took that path, so nothing was ever eligible.
+
+Nothing said so. The policy run then recorded itself as `completed` with a
+deleted count of zero, which is also what the API and the execution history
+reported, so an operator checking whether retention was working saw a series of
+successful runs. The only symptom was data that never aged out, and one
+`Failed to read file metadata` line per file per cycle.
+
+Live since **v26.03.2**, when the prefix option was added. That change updated
+the identical URL builder in the delete path and did not update this one.
+Unprefixed S3, Azure and local deployments were never affected.
+
+The root cause was that the same builder existed in several places by hand.
+There is now one, `storage.ObjectURI`, which every component that reads the
+object store directly goes through, so a backend's prefix cannot be honoured in
+one place and forgotten in another. See the upgrade note above before
+upgrading: the first cycle after this fix will delete everything that
+accumulated while retention was doing nothing.
+
+### Partition pruning silently stopped working on S3 deployments with a configured prefix ([#746](https://github.com/Basekick-Labs/arc/issues/746))
+
+Also only with `storage.s3_prefix` set, and from the same cause in the opposite
+direction.
+
+Partition pruning narrows a time-bounded query to the hour and day directories
+it needs, then checks which of them exist before handing the list to DuckDB.
+Converting a directory URL back into something it could list stripped the
+scheme and the bucket but not the configured prefix, and the listing call adds
+the prefix itself. The listing therefore ran against `prefix/prefix/...` and
+came back empty **with no error**, so every partition was judged absent and the
+query fell back to scanning the measurement's full glob.
+
+Results stayed correct throughout; only the optimisation was lost. On a large
+measurement that is the difference between reading one hour and reading
+everything, so affected deployments should see time-bounded queries get faster
+after upgrading.
+
+Tiered queries were not affected: per-tier pruning already trimmed the tier's
+own root and carries a comment describing this exact hazard. Only the
+single-tier path was doing scheme-and-bucket surgery. It now trims the storage
+root it parsed out of the measurement's own glob, the same way the tiered path
+does, and a test pins that the parsed root and the URL builder agree. A URL that
+does not sit under that root no longer falls back to stripping the scheme: that
+produced a prefix the backend would re-prefix and list against some other key
+space, reporting every partition absent with full confidence. Existence
+filtering is skipped instead, so the query falls back to the full glob.
+
+### The query path now validates database and measurement names against the storage key contract ([#746](https://github.com/Basekick-Labs/arc/issues/746))
+
+No operator-visible behaviour changes here for valid names. This closes the
+structural gap the two fixes above came out of.
+
+The previous release made the key rule a property of every storage backend
+(#743). That covers writes. Reads never go through a backend at all: Arc builds
+an `s3://` or `azure://` URL and hands it to DuckDB, which reads the object
+store itself, and the Iceberg exporter hands paths to its own file layer the
+same way. A name that a backend would refuse could therefore still reach the
+object store through a query.
+
+Reads now go through the same contract, plus one rule that writes do not need.
+A key containing `*`, `?`, `[` or `{` names exactly one object to a write, so
+the contract accepts it; interpolated into a query it is a pattern, and
+`cpu*` would read every measurement whose name starts with `cpu`. Read paths
+reject those characters, writes still accept them.
+
+A name that cannot be turned into a path now fails the query with an explicit
+error. The alternative, substituting a path that matches nothing, would have
+been reported to the client as a successful query over an empty table, because
+Arc deliberately treats "no files matched" as an empty result rather than an
+error. Listing endpoints, whose names come from enumerating storage rather than
+from a request, leave the displayed path empty instead of failing the listing.
+
+Also in this change, all of it internal:
+
+- The five separate implementations of "is this name safe as a path" are
+  reconciled. They disagreed with each other: the cluster file-fetch validator
+  was believed to be stricter than the contract and was measurably looser,
+  accepting backslashes and over-long keys the contract refuses, and the delete
+  endpoint refused any name containing `..` anywhere, so a measurement called
+  `a..b` could be written and queried but not deleted from. They now share one
+  rule, with the two deliberate divergences documented and pinned by a test:
+  the Raft manifest validator stays looser because it runs during log replay
+  and tightening it would make different versions of Arc build different state
+  from one log, and the HTTP API additionally hides dot-prefixed names, which is
+  a display rule rather than a storage one.
+- Retention now reports when it had to skip a file whose stored path it cannot
+  read, instead of counting the run as a clean success. Such a file is skipped
+  on every future run too, so its data never ages out, and that is exactly the
+  silent shape of the prefix bug above.
+- The documented list of ways a list prefix is allowed to be looser than an
+  object key was one entry out of date, and is now checked by a test rather
+  than only stated in a comment.
+- Three unused S3 URL builders were removed rather than left as further copies
+  of the builder above.
 
 ### Every storage backend now enforces the same key contract ([#743](https://github.com/Basekick-Labs/arc/issues/743))
 

internal/api/continuous_query.go

@@ -788,7 +788,10 @@ func wrapSourceMeasurement(query, database, measurement, readParquetExpr string)
 // the window START (bucket-start convention), so only startTime is needed.
 func (h *ContinuousQueryHandler) executeAggregation(ctx context.Context, cq *ContinuousQuery, query string, startTime, _ time.Time) (int64, error) {
 	// Build storage path for source measurement (supports local, S3, Azure)
-	measurementPath := storage.GetStoragePath(h.storage, cq.Database, cq.SourceMeasurement)
+	measurementPath, err := storage.GetStoragePath(h.storage, cq.Database, cq.SourceMeasurement)
+	if err != nil {
+		return 0, fmt.Errorf("continuous query %q has an unusable source: %w", cq.Name, err)
+	}
 
 	// Extract CTE names to avoid replacing them with read_parquet paths
 	cteNames := extractCTENames(query)

internal/api/databases.go

@@ -469,24 +469,31 @@ func (h *DatabasesHandler) handleDelete(c *fiber.Ctx) error {
 // Helper functions
 
 // isSafeStoragePathSegment reports whether name can be used as one segment of a
-// storage key.
+// storage key by an API caller.
+//
+// The storage half is storage.ValidateKeySegment, the contract every Backend
+// enforces, rather than a private re-spelling of it (#746). Only the
+// leading-dot rule is local, and it stays local deliberately: it is an API
+// visibility rule, not a property of the key contract. Arc's own storage root
+// holds dot-prefixed entries (`.arc-database` markers, and the query layer's
+// `.arc-invalid-quoted-identifier` sentinel), so the storage layer must keep
+// accepting them while the API declines to name them.
 //
 // Deliberately looser than isValidDatabaseName, which governs what a NEW
 // database may be called. Directories already in the storage root were not all
 // created through that route: an edge-sync hub writes each spoke's namespace
 // there, and validateSpokeID permits a leading digit, interior dots and up to
 // 128 bytes. Those directories are listed by GET /api/v1/databases, so gating
 // the per-name routes on the create-time rule would return 400 for something
-// the list endpoint just reported. This rule asks only what the storage layer
-// asks: is it one segment, and does it name something inside the root.
+// the list endpoint just reported.
+//
+// Note this carries no glob rule. A caller whose name reaches a DuckDB path
+// needs storage.ValidateGlobSafe as well; see handleDelete.
 func isSafeStoragePathSegment(name string) bool {
-	if name == "" || len(name) > 255 {
-		return false
-	}
-	if name == "." || name == ".." || strings.HasPrefix(name, ".") {
+	if strings.HasPrefix(name, ".") {
 		return false
 	}
-	return !strings.ContainsAny(name, "/\\\x00")
+	return storage.ValidateKeySegment(name) == nil
 }
 
 func isValidDatabaseName(name string) bool {

internal/api/delete.go

@@ -259,15 +259,26 @@ func (h *DeleteHandler) handleDelete(c *fiber.Ctx) error {
 		})
 	}
 
-	// Reject path traversal in database/measurement names — these values are
-	// concatenated directly into storage prefixes and DuckDB paths.
-	if strings.ContainsAny(req.Database, "/\\") || strings.Contains(req.Database, "..") {
+	// These values are concatenated into storage prefixes and DuckDB paths, so
+	// they must be usable as one path segment. isSafeStoragePathSegment is the
+	// same rule the database endpoints apply, rather than a fourth local
+	// spelling of it (#746): the previous check here rejected any ".."
+	// SUBSTRING, so it refused "a..b", which every other layer accepts and
+	// which names exactly one directory. That is the raw-substring shape #737
+	// and #741 were each fixed for.
+	//
+	// ValidateGlobSafe is applied alongside it because these names end up in a
+	// DuckDB read_parquet() path, where "*" and friends are pattern operators
+	// rather than characters. Without it the request is accepted here and dies
+	// several layers down at path resolution, and the query endpoints reject
+	// the same name cleanly: one name, two verdicts at two depths.
+	if !isSafeStoragePathSegment(req.Database) || storage.ValidateGlobSafe(req.Database) != nil {
 		return c.Status(fiber.StatusBadRequest).JSON(DeleteResponse{
 			Success: false,
 			Error:   "database name contains invalid characters",
 		})
 	}
-	if strings.ContainsAny(req.Measurement, "/\\") || strings.Contains(req.Measurement, "..") {
+	if !isSafeStoragePathSegment(req.Measurement) || storage.ValidateGlobSafe(req.Measurement) != nil {
 		return c.Status(fiber.StatusBadRequest).JSON(DeleteResponse{
 			Success: false,
 			Error:   "measurement name contains invalid characters",
@@ -545,12 +556,20 @@ func (h *DeleteHandler) findAffectedFiles(ctx context.Context, database, measure
 	// Filter for parquet files and convert to query paths
 	var parquetFiles []fileInfo
 	for _, f := range files {
-		if strings.HasSuffix(strings.ToLower(f), ".parquet") {
-			parquetFiles = append(parquetFiles, fileInfo{
-				queryPath:    h.getQueryPath(f),
-				relativePath: f,
-			})
+		if !strings.HasSuffix(strings.ToLower(f), ".parquet") {
+			continue
+		}
+		queryPath, err := readParquetPath(h.storage, f)
+		if err != nil {
+			// A delete must not silently leave matching rows behind, so an
+			// unusable key aborts rather than being skipped: the caller reports
+			// how many rows it removed, and skipping would make that a lie.
+			return nil, fmt.Errorf("listed file %q has no usable storage path: %w", f, err)
 		}
+		parquetFiles = append(parquetFiles, fileInfo{
+			queryPath:    queryPath,
+			relativePath: f,
+		})
 	}
 
 	if len(parquetFiles) == 0 {
@@ -916,20 +935,6 @@ func (h *DeleteHandler) rewriteS3File(ctx context.Context, s3Path, relativePath,
 	return deleted, &s3RewriteResult{sizeBytes: sizeBytes, sha256: sha256hex}, nil
 }
 
-// getQueryPath converts a storage-relative path to a DuckDB-compatible path
-func (h *DeleteHandler) getQueryPath(relativePath string) string {
-	switch backend := h.storage.(type) {
-	case *storage.LocalBackend:
-		return filepath.Join(backend.GetBasePath(), relativePath)
-	case *storage.S3Backend:
-		return fmt.Sprintf("s3://%s/%s%s", backend.GetBucket(), backend.GetPrefix(), relativePath)
-	case *storage.AzureBlobBackend:
-		return fmt.Sprintf("azure://%s/%s", backend.GetContainer(), relativePath)
-	default:
-		return relativePath
-	}
-}
-
 // isRemoteBackend returns true if the storage backend requires a remote rewrite
 // (download to local temp file, then re-upload). Covers S3 and Azure Blob.
 func (h *DeleteHandler) isRemoteBackend() bool {