Skip to content

Add lazy Namespace storage client - #4257

Draft
buildkate wants to merge 15 commits into
mainfrom
A-1435-namespace-storage-api-phase-1
Draft

Add lazy Namespace storage client#4257
buildkate wants to merge 15 commits into
mainfrom
A-1435-namespace-storage-api-phase-1

Tidy dependencies after rebase

9488a06
Select commit
Loading
Failed to load commit list.
buildsworth-bk-app / Buildsworth review succeeded Aug 26, 2026 in 4m 8s

Review submitted — commented

PR #4257 review summary

Change reviewed

This PR replaces the nsc artifact subprocess used by the nsc:// cache store with direct Namespace Storage API calls. It adds a lazily initialized Namespace client shared across all cache workers in one save or restore command, then closes that client after the workers finish. Uploads retain the 24-hour expiry; downloads stream to a fixed temporary filename, retry selected transient failures up to three times, map missing artifacts to cache misses, verify the downloaded blob digest in the existing cache layer, and refresh expiry on successful restores.

The change also adds the Namespace SDK and generated protobuf dependencies, moves file-path validation that remains relevant to the local file store, and excludes Namespace's generated Buf modules from automated acknowledgements collection because those modules do not publish license files.

The latest revision rebases the branch onto current main, resolves the earlier go.mod/go.sum conflict, and runs dependency tidying. Relative to the previously reviewed head (9c7480c), the PR-owned cache and acknowledgements files are unchanged; the only PR dependency-tree delta is removal of a now-unused protovalidate module after the rebase.

Evidence examined

  • Read the complete 13-file PR diff and the full changed implementation and test files.
  • Traced both cache command entry points through worker completion and client cleanup, and traced NewBlobStore through save and restore call sites.
  • Examined the pinned namespacelabs.dev/integrations source to verify client creation/closure, workload-token defaults, upload versioning, expiry options, signed-download behavior, and error shapes used by the retry and miss classifiers.
  • Compared the direct implementation with Namespace's current CLI downloader, including its ranged-download and retry behavior.
  • Rechecked the existing review threads. The earlier transient-download, HTTP/2 reset, HTTP 429/5xx, signed-download 404, and local path traversal concerns remain addressed and their threads are resolved.
  • Reviewed the new unit coverage for lazy/concurrent initialization, one-time closure, upload options, expiry refresh, missing artifacts, transient resolve and stream-copy retries, destination truncation, and fixed local download paths.
  • Read Linear issue A-1435 and the PR conversation, including the open human discussion about single-stream download performance and planned regression testing.

Validation and CI

  • bash -n scripts/generate-acknowledgements.sh passed.
  • git diff --check origin/main HEAD passed for the PR files.
  • GitHub reports the PR as mergeable with no current merge conflict.
  • Local Go tests could not be run because the Go toolchain is unavailable in this review environment.
  • Buildkite build #14097 is failing. The Buildkite API credentials available to this session could not access that organization, so failed-job details and logs could not be verified.

Review result

No new inline code findings were posted. The current implementation preserves the previously corrected download handling, and the rebase/tidy update introduced no new PR-owned code changes. The single-stream performance trade-off remains under active human review and was not duplicated as a bot finding.

A COMMENT review was submitted rather than an approval because replacing the cache transport and adding shared client lifecycle behavior is beyond the supplied low-risk approval ceiling; the failing Buildkite build was also noted. The review contains no inline comments or requested changes.

Trigger source: automatic.