fix(iceberg): warehouseRelKey must trim the storage root, not the warehouse by xe-nvdk · Pull Request #534 · Basekick-Labs/arc

xe-nvdk · 2026-07-14T22:30:36Z

Follow-up to #532, from a Gemini finding on the #533 tracking PR. Real bug, reproduced before fixing.

Summary

Storage backends' Read/Write take keys relative to the storage root, but warehouseRelKey trimmed e.warehouse. Those two bases are the same string only when iceberg.warehouse is the storage root — which is the default, and is why every test and every prior binary run missed this.
Pointing iceberg.warehouse at a subdirectory (a supported config: the key exists and is passed straight through when set) silently dropped that subdirectory from every key. version-hint.text and the v<N>.metadata.json reader copies landed at the storage root instead of inside the warehouse, so directory-based readers (DuckDB, Spark) could not resolve the current snapshot.
Fix: keep gating on the warehouse prefix (unchanged "is this ours?" semantics), but derive the key by trimming DefaultWarehouse(e.backend).
Also returns ok=false when the warehouse sits entirely outside the storage root — the backend cannot address it, and the suggested patch would have produced a key that silently escaped the root.

Reproduced first

With warehouse=<root>/warehouse:

before: version-hint.text -> <root>/arc_mydb.db/cpu/metadata/version-hint.text          (leaked to root)
after:  version-hint.text -> <root>/warehouse/arc_mydb.db/cpu/metadata/version-hint.text (correct)

Test plan

TestCustomWarehouseSubdir regression test: hint lands inside the configured warehouse, does not leak to the storage root, and the data file is still registered.
Full internal/iceberg suite green under -race (default-warehouse path unaffected).
gofmt / go vet / go build -tags duckdb_arrow ./cmd/... ./internal/... clean.
Binary-verified with iceberg.warehouse set to a subdirectory: warehouse writes under data/warehouse/, nothing leaks to the storage root, and duckdb iceberg_scan resolves via version-hint.text and reads 40 rows — the exact path that failed before the fix.

🤖 Generated with Claude Code

…ehouse Storage backends' Read/Write take keys relative to the STORAGE ROOT, but warehouseRelKey trimmed e.warehouse. Those bases coincide only when iceberg.warehouse is the storage root (the default), so pointing iceberg.warehouse at a SUBDIRECTORY silently dropped that subdirectory from every key: version-hint.text and the v<N>.metadata.json reader copies landed at the storage root instead of inside the warehouse, and directory-based readers (DuckDB, Spark) could not resolve the current snapshot. Now: gate on the warehouse prefix (unchanged semantics for "is this ours?"), but derive the key by trimming DefaultWarehouse(e.backend). Also returns ok=false when the warehouse sits outside the storage root entirely, since the backend cannot address it — previously that would have produced a key that silently escaped the root. Reproduced first: with warehouse=<root>/warehouse, the hint landed at <root>/arc_mydb.db/... instead of <root>/warehouse/arc_mydb.db/... Verified on the running binary with iceberg.warehouse set to a subdirectory: the warehouse writes under data/warehouse/, nothing leaks to the storage root, and DuckDB iceberg_scan resolves via version-hint.text and reads 40 rows — the exact path that failed before. Regression test: TestCustomWarehouseSubdir (hint inside the warehouse, no leak to the root, file still registered). Found by Gemini on PR #533. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

xe-nvdk · 2026-07-14T22:30:49Z

@gemini-code-assist please review

gemini-code-assist

Code Review

This pull request updates "warehouseRelKey" to resolve metadata URI keys relative to the storage root rather than the warehouse, preventing files from leaking outside the configured warehouse when it is set to a subdirectory. A new test case "TestCustomWarehouseSubdir" has been added to verify this behavior. The review feedback points out a potential path prefix matching bug where "strings.HasPrefix" is used without ensuring trailing slashes, which could lead to incorrect matches for directories sharing a prefix, and provides a code suggestion to address it.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist

Code Review

This pull request updates warehouseRelKey to handle cases where the Iceberg warehouse is configured as a subdirectory of the storage root, ensuring metadata files are written inside the warehouse rather than leaking to the storage root. A new test has been added to verify this behavior. Feedback highlights two main issues: first, the prefix matching in warehouseRelKey could incorrectly match overlapping directory names, which can be resolved by checking path boundaries; second, raw string concatenation in the test may produce malformed URIs on Windows, and should be replaced with the localFileURI helper.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

…i on #534) The gate I added in the previous commit used a bare strings.HasPrefix, which matches mid-segment: warehouse "file:///data/wh" accepted "file:///data/wh-other/arc_db.db/..." — a DIFFERENT warehouse's metadata treated as ours. Same flaw on the storage-root trim (rel == metaLoc misses a shared name prefix like /data vs /data-other). Not reachable today — all three callers pass tbl.MetadataLocation() from our own catalog — but this function is the "is this path ours?" boundary check, and pruneOldVersionFiles DELETES files under the key it derives, so it should not be left loaded. Extracted isUnderDir(p, dir): p == dir || strings.HasPrefix(p, dir+"/"), with the trailing slash normalized once, used for both the warehouse gate and the root trim. Also build the test's warehouse URI with localFileURI instead of "file://" + filepath.Join, so it matches DefaultWarehouse (absolute + forward slashes) rather than producing file://C:\... on Windows. Tests: TestIsUnderDir pins the boundary cases (dir itself, beneath, trailing slash, and the sibling prefixes wh-other/wharf/whx that must be rejected). Re-verified on the running binary with a subdirectory warehouse: hint lands under data/warehouse/, DuckDB iceberg_scan reads 40 rows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

xe-nvdk · 2026-07-14T22:36:07Z

Both findings taken in ffd75e0 — the prefix one is a fair hit on code I had just written.

Path-boundary matching. Confirmed the bug concretely: with warehouse file:///data/wh, a bare strings.HasPrefix accepts file:///data/wh-other/arc_db.db/cpu/metadata/v1.metadata.json — a different warehouse's metadata treated as ours, yielding the key wh-other/arc_db.db/.... The same flaw applied to my rel == metaLoc root check (misses /data vs /data-other).

Being straight about severity: it is not reachable today — all three callers pass tbl.MetadataLocation() from our own catalog, so no foreign path ever reaches it. But this function is the "is this path ours?" boundary check and pruneOldVersionFiles deletes files under the key it derives, so a gate that can say yes to the wrong directory is not something to leave loaded. Fixed.

I extracted a helper rather than inlining the check at both sites (the trailing-slash normalization then lives in one place):

func isUnderDir(p, dir string) bool {
	dir = strings.TrimSuffix(dir, "/")
	return p == dir || strings.HasPrefix(p, dir+"/")
}

TestIsUnderDir pins the cases: the dir itself, paths beneath it, a trailing-slash-configured dir, and the sibling prefixes (wh-other, wharf, whx) that must be rejected.

localFileURI in the test. Also correct — "file://" + filepath.Join(...) would emit file://C:\... on Windows and mismatch DefaultWarehouse. Switched to localFileURI, which is exactly what DefaultWarehouse uses.

Re-verified on the running binary after the change (the gate is in the live path, so tests alone are not enough): with iceberg.warehouse at a subdirectory, the hint lands under data/warehouse/, nothing leaks to the storage root, and duckdb iceberg_scan reads 40 rows.

@gemini-code-assist please review the follow-up.

gemini-code-assist

Code Review

This pull request addresses an issue where setting the Iceberg warehouse to a subdirectory of the storage root caused metadata files to be written outside the warehouse. It updates warehouseRelKey to correctly resolve storage-relative keys and introduces a path-boundary aware helper isUnderDir to prevent matching sibling directories with similar prefixes. Corresponding unit tests have been added to verify these changes. There are no review comments, so we have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

…tBackend (#320, #560) (#561) * fix(compaction): preserve S3 prefix in subprocess; drop dead ResilientBackend (#320, #560) #560 -- compaction runs in a forked subprocess for DuckDB memory isolation, and the child rebuilds its storage backend from the parent's Type() and ConfigJSON(). S3Backend.ConfigJSON emits "prefix", but the subprocess parse struct had no matching field, so json.Unmarshal silently discarded it and the rebuilt backend carried an empty prefix. The prefix is applied via prefixedKey() to every key the S3 backend touches -- read, write, delete, list -- so on any deployment with storage.s3_prefix set, compaction subprocesses operated against the bucket root instead of the configured prefix, with no error raised. It defaults to empty, so "prefixed" and "unprefixed" were the same string everywhere the code was exercised. This is the config-key-at-its-default shape from #534. The prefix is now parsed and forwarded. A round-trip test drives the real createStorageBackendFromConfig and compares the rebuilt backend's ConfigJSON against the parent's, so a field added to ConfigJSON without a matching parse field fails immediately. Azure and local have no equivalent gap: each emits exactly the fields the subprocess parses. #320 -- internal/storage/resilient.go wrapped a backend with retry and a circuit breaker, but nothing ever constructed one: no call sites in cmd/ or internal/, no tests, no type assertions, and a single commit dating to the original Go migration. Having drifted behind the Backend interface (missing ReadToAt, StatFile, Type, ConfigJSON) it no longer satisfied the interface it was written against, which is what surfaced it. Removed rather than completed. Retry and circuit-breaking around cloud storage are worth having, but an unused and untested wrapper would drift again at the next interface change; a future implementation should be written against the interface as it stands then and actually wired in. The build is unchanged -- the code was unreachable. Verified against a running binary: 3 files compacted to 1 through the real subprocess path, all rows still queryable, zero storage-config errors, clean graceful shutdown. * build: add .dockerignore to keep local data out of the build context Building the image from a working tree that has been used for local testing copied the whole tree into the build context, including a 27GB data/ directory of parquet and 137MB of release tarballs. The builder ran out of disk before compiling the binary. Everything excluded is either gitignored (data/), a build artifact (zarf-package-*.tar.zst, uds/), or not needed to compile and run the binary (docs/, deploy/, helm/, *.md). Found while running an end-to-end MinIO test for the S3 prefix fix. ---------

Merges main (23 commits) into the Iceberg branch and fixes the issues a review of PR Basekick-Labs#533 turned up. The Iceberg subsystem itself is sound -- the config guardrails, the compaction-race handling, and the fingerprint cache all hold up. These are the gaps. BLOCKER: iceberg.catalog_db_path at a non-default value silently excluded the Iceberg SQL catalog from backup. main.go hardcoded cfg.Auth.DBPath when constructing the backup manager while Iceberg read cfg.Iceberg.CatalogDBPath. Both default to ./data/arc.db, so they are the same file at the default and diverge the moment an operator sets the key -- the Basekick-Labs#534 shape, in the package Basekick-Labs#534 was filed against. A restore then brought back Parquet and warehouse metadata whose tables no longer resolved, contradicting the "backup-aware" claim. The catalog is now backed up as metadata/iceberg-catalog.db when it is a separate file, and restored alongside; older backups without it are skipped, not an error. BLOCKER: a failed version-hint.text write was never retried. The scheduler short-circuits on an unchanged fingerprint before reaching ReconcileMeasurement, and cached the fingerprint regardless of whether the best-effort discovery-file writes succeeded. Once the file set went quiet -- the steady state -- the hint stayed stale forever and directory-based readers (DuckDB iceberg_scan, Spark hadoop-format) resolved the wrong version indefinitely. Publishing now reports success, a converged pass republishes, and the scheduler declines to cache when it failed. BLOCKER: merging main produced a silently broken backup path. backup.go auto-merged with NO conflict, combining the PR's second copyDataFiles call (Iceberg metadata) with main's skip-ratio guard from Basekick-Labs#557. Two defects, both reproduced with tests before fixing: SkippedFiles used Store rather than Add, so the second call erased the first call's skips and the manifest claimed a complete backup; and the ratio was evaluated per call, so one stale entry in a 3-file metadata set (33%) aborted a backup whose data files had all copied. Skips now accumulate and the ratio is evaluated once over every file group. HIGH: pruneOldVersionFiles kept exactly `retain` v<N>.metadata.json copies, so a directory reader that resolved version-hint.text just before a commit could find its version already deleted -- continuous at retain_snapshots=1. Keeps retain+1 now, bounding the race to one reconcile interval for a few KB. MEDIUM: retain_snapshots=0 silently meant "keep every snapshot forever". Rejected at config load, consistent with the other Iceberg guardrails. MEDIUM: a warehouse outside the storage root disabled discovery-file publishing permanently and logged only at Debug. Now warns once per table and names what is degraded (directory readers) and what is not (catalog readers). LOW: the exporter opened a seventh SQLite handle on the shared file, reintroducing what Basekick-Labs#329/Basekick-Labs#562 consolidated. It borrows the auth manager's handle when the catalog lives in the shared database, and owns one only when the operator points the key elsewhere. Verified against the running binary with catalog_db_path and retain_snapshots both non-default: separate catalog file created, table reconciled, backup contains metadata/iceberg-catalog.db with has_iceberg_catalog=true, version-hint published, zero errors, clean shutdown with no database-is-closed cascade from the borrowed handle. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

S3 has had storage.s3_prefix since the beginning, so one bucket could hold Arc's data under arc/ beside something else, or two deployments under disjoint prefixes. Azure had no equivalent: the key was the blob name, so a container could hold exactly one Arc deployment and nothing else. Adds storage.azure_prefix and tiered_storage.cold.azure_prefix, both defaulting to empty, which is the container root and byte-identical to today. All fourteen key- and prefix-taking methods on AzureBlobBackend route through one prefixedKey or prefixedListPrefix, mirroring S3Backend, and all five enumerators strip the prefix before validating so callers keep speaking unprefixed keys. Four places outside the backend build an Azure location or hand an Azure config to something else, and a miss in any of them is silent rather than a compile error, which is the same shape as the broken iceberg.warehouse key (#534): - ConfigJSON and the azure case of the compaction subprocess. Compaction runs in a separate process that rebuilds the backend from that JSON. The S3 case already forwarded its prefix; the Azure case had never been tested at all. Missing it reroots compaction at the container root. - iceberg.DefaultWarehouse, which type-switches on the backend to build the warehouse root. Not reachable today, since config.Load refuses iceberg.enabled unless the primary backend is local, so both object-store arms are dead code. Fixed anyway. - The DuckDB secret scope and the sandbox allowlist, which now carry the prefix as S3 does. Without that, a primary store and a cold tier sharing one container distinguished only by prefix produce two secrets with byte-identical scopes. - Two cmd/arc call sites plus the cold tier's own DuckDB secret. Every prefix key is now validated when the configuration loads, with the offending value named. That matters most for the cold-tier keys: an unusable one failed backend construction at a site that logs and continues with a nil cold backend, so the tier was silently dead, and the error text is one the global log sanitiser masks. Arc also warns at startup when a prefix has three or more segments and the last looks like a year, because the code that reads a database and measurement off a storage path scans backwards for a year-shaped segment and finds the prefix instead, making tiered queries return zero rows (#1108). ValidateS3Prefix becomes ValidateObjectPrefix in its own file, with no alias. All three backends share it, internal/ is module-private so no out-of-tree caller exists, and two names for one function is how the next reader comes to believe there are two rules. fix(backup): fail loudly when compaction recovery state cannot be copied (#1100) The three backup copy paths disagreed about an over-long destination key. Data files checked and skipped; the Iceberg warehouse checked and failed the run; compaction recovery state did not check, so the overrun surfaced from inside the storage write and failed the run with a message about storage rather than about the key. It now checks before writing and still fails, naming the file, the destination size, the limit and the length a source key must come under. The behaviour is deliberately not the data path's skip: the paths differ on whether absence is detectable by a restore. A skipped data file is counted, named in the skip sample, carried into the INCOMPLETE marker and still held by the source. A skipped recovery manifest is counted in a field the restore path never reads, and restoring without it brings back a compacted output next to the inputs it replaced with nothing to reconcile them, so that partition serves every row twice, permanently (#930). That reasoning is in the code above both functions. fix(backup): compensate a failed write to a non-staging destination (#1101) Cleanup returned early unless the backend staged its writes. Verified against the SDK versions in go.mod, a failed upload commits nothing on either S3 or Azure, so the case this reaches is a commit that succeeded while its response was lost. The comment says that, and says what it cannot do: it cannot abort a multipart abandoned by a cancelled context, so a bucket taking Arc backups wants an AbortIncompleteMultipartUpload rule. The metadata-database copy, which had no cleanup at all, is covered too. Restore cleanup deliberately did not change. A fourth caller passes the live data backend with a live data key, and since a failed overwrite leaves the previous object intact, an unconditional delete there would destroy a registered file whose manifest entry nothing re-registers. The two are now separate functions and the backup one takes no backend, because every key it can be given is minted under a run's own backup ID. feat(backup): make the backup and restore timeout configurable (#1085) backup.operation_timeout replaces two hardcoded two-hour timeouts, read with time.ParseDuration and a load-time error naming a bad value, as the only other duration key in the repo is. Zero and negative are errors, not unbounded operations.

…1114) S3 has had storage.s3_prefix since the beginning, so one bucket could hold Arc's data under arc/ beside something else, or two deployments under disjoint prefixes. Azure had no equivalent: the key was the blob name, so a container could hold exactly one Arc deployment and nothing else. Adds storage.azure_prefix and tiered_storage.cold.azure_prefix, both defaulting to empty, which is the container root and byte-identical to today. All fourteen key- and prefix-taking methods on AzureBlobBackend route through one prefixedKey or prefixedListPrefix, mirroring S3Backend, and all five enumerators strip the prefix before validating so callers keep speaking unprefixed keys. Four places outside the backend build an Azure location or hand an Azure config to something else, and a miss in any of them is silent rather than a compile error, which is the same shape as the broken iceberg.warehouse key (#534): - ConfigJSON and the azure case of the compaction subprocess. Compaction runs in a separate process that rebuilds the backend from that JSON. The S3 case already forwarded its prefix; the Azure case had never been tested at all. Missing it reroots compaction at the container root. - iceberg.DefaultWarehouse, which type-switches on the backend to build the warehouse root. Not reachable today, since config.Load refuses iceberg.enabled unless the primary backend is local, so both object-store arms are dead code. Fixed anyway. - The DuckDB secret scope and the sandbox allowlist, which now carry the prefix as S3 does. Without that, a primary store and a cold tier sharing one container distinguished only by prefix produce two secrets with byte-identical scopes. - Two cmd/arc call sites plus the cold tier's own DuckDB secret. Every prefix key is now validated when the configuration loads, with the offending value named. That matters most for the cold-tier keys: an unusable one failed backend construction at a site that logs and continues with a nil cold backend, so the tier was silently dead, and the error text is one the global log sanitiser masks. Arc also warns at startup when a prefix has three or more segments and the last looks like a year, because the code that reads a database and measurement off a storage path scans backwards for a year-shaped segment and finds the prefix instead, making tiered queries return zero rows (#1108). ValidateS3Prefix becomes ValidateObjectPrefix in its own file, with no alias. All three backends share it, internal/ is module-private so no out-of-tree caller exists, and two names for one function is how the next reader comes to believe there are two rules. fix(backup): fail loudly when compaction recovery state cannot be copied (#1100) The three backup copy paths disagreed about an over-long destination key. Data files checked and skipped; the Iceberg warehouse checked and failed the run; compaction recovery state did not check, so the overrun surfaced from inside the storage write and failed the run with a message about storage rather than about the key. It now checks before writing and still fails, naming the file, the destination size, the limit and the length a source key must come under. The behaviour is deliberately not the data path's skip: the paths differ on whether absence is detectable by a restore. A skipped data file is counted, named in the skip sample, carried into the INCOMPLETE marker and still held by the source. A skipped recovery manifest is counted in a field the restore path never reads, and restoring without it brings back a compacted output next to the inputs it replaced with nothing to reconcile them, so that partition serves every row twice, permanently (#930). That reasoning is in the code above both functions. fix(backup): compensate a failed write to a non-staging destination (#1101) Cleanup returned early unless the backend staged its writes. Verified against the SDK versions in go.mod, a failed upload commits nothing on either S3 or Azure, so the case this reaches is a commit that succeeded while its response was lost. The comment says that, and says what it cannot do: it cannot abort a multipart abandoned by a cancelled context, so a bucket taking Arc backups wants an AbortIncompleteMultipartUpload rule. The metadata-database copy, which had no cleanup at all, is covered too. Restore cleanup deliberately did not change. A fourth caller passes the live data backend with a live data key, and since a failed overwrite leaves the previous object intact, an unconditional delete there would destroy a registered file whose manifest entry nothing re-registers. The two are now separate functions and the backup one takes no backend, because every key it can be given is minted under a run's own backup ID. feat(backup): make the backup and restore timeout configurable (#1085) backup.operation_timeout replaces two hardcoded two-hour timeouts, read with time.ParseDuration and a load-time error naming a bad value, as the only other duration key in the repo is. Zero and negative are errors, not unbounded operations.

xe-nvdk mentioned this pull request Jul 14, 2026

feat(iceberg): Apache Iceberg export (zero-copy, opt-in) (#532) #533

Merged

gemini-code-assist Bot reviewed Jul 14, 2026

View reviewed changes

Comment thread internal/iceberg/exporter.go Outdated

gemini-code-assist Bot reviewed Jul 14, 2026

View reviewed changes

Comment thread internal/iceberg/exporter.go Outdated

Comment thread internal/iceberg/scheduler_test.go Outdated

gemini-code-assist Bot reviewed Jul 14, 2026

View reviewed changes

xe-nvdk merged commit faa625d into 26.09.1 Jul 14, 2026

xe-nvdk mentioned this pull request Jul 30, 2026

fix(iceberg): address review blockers; merge main (#533) #567

Merged

6 tasks done

xe-nvdk mentioned this pull request Aug 7, 2026

feat(edgesync): air-gap bundle export (#569) #581

Merged

7 tasks done

This was referenced Oct 7, 2026

perf(iceberg): collapse a table's manifest set once it crosses a threshold (#1106) #1124

Merged

chore(iceberg): move Iceberg export to iceberg-go v0.7.0 #1131

Merged

Navigation Menu

Repository navigation

fix(iceberg): warehouseRelKey must trim the storage root, not the warehouse - #534

fix(iceberg): warehouseRelKey must trim the storage root, not the warehouse#534
xe-nvdk merged 2 commits into
26.09.1from
fix/iceberg-custom-warehouse-relkey

xe-nvdk commented Jul 14, 2026

Uh oh!

xe-nvdk commented Jul 14, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

xe-nvdk commented Jul 14, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

xe-nvdk commented Jul 14, 2026

Summary

Reproduced first

Test plan

Uh oh!

xe-nvdk commented Jul 14, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

xe-nvdk commented Jul 14, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant