Add lazy Namespace storage client - #4257
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| Version int `json:"version"` // Metadata schema version | ||
| } | ||
|
|
||
| func validateFilePath(filePath string) error { |
There was a problem hiding this comment.
This was moved unchanged from nsc.go as it's not required by the API but still used by the local file store for validation.
| "testing" | ||
| ) | ||
|
|
||
| func TestValidateFilePath(t *testing.T) { |
There was a problem hiding this comment.
similarly to file.go, all these tests were just moved from nsc_test.go to here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 948571aaff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
The direct download path drops the CLI downloader's retry behavior, leaving transient stream failures to abort cache restores. Details are inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20459, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. Unless repository policy already allows L2 approval, grant it by mentioning @buildsworth-bk (see approval ceiling and L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only when repository policy or a verified grant allows it; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
948571a to
f836ac4
Compare
There was a problem hiding this comment.
The previous download-retry blocker is still open; the latest revision only renames the Namespace client types and fields.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20467, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
|
@buildsworth-bk review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
The retry blocker remains open for one transient stream-failure class; details are in the existing thread. I found no other new concerns in this pass.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20481, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
|
@buildsworth-bk review |
This comment was marked as resolved.
This comment was marked as resolved.
|
@buildsworth-bk review |
1 similar comment
|
@buildsworth-bk review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
The HTTP/2 reset path is fixed, but one part of the existing download-retry blocker remains open; details are in its thread. I found no other new concerns in this pass.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20502, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
|
@buildsworth-bk review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2d4afccac
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous download-retry concern is fixed. The signed-download HTTP 404 miss-mapping thread is blocking this revision.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20522, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
|
@buildsworth-bk review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Previous concerns have been addressed.
There was a problem hiding this comment.
The signed-download HTTP 404 mapping now addresses the previous blocker, and I found no new concerns. This cache transport and shared client-lifecycle change is above the L1 approval ceiling, so I’m leaving this as a comment.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 20557, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
jamiemonserrate
left a comment
There was a problem hiding this comment.
- Have you tested back portability? I'm just curious if our existing pipelines (bk/bk) will continue to work, once we move to this. It should - but just curious if you tested.
- Any reason you opted to go for a single stream download? This is going to be significantly slower that the exisiting nsc CLI. And it seems we lose all the chunk based retry, and the exponential backoff that nsc CLI gave us. Any reason for dropping this?
These are the three options I see:
Given that I'll be doing regression testing, if the results are tolerable with the current implementation (option 1), I will go with that. Wdyt? |
Yeah - my inclination is to continue using the But this is based on the assumption that there is a 75% drop in speed for large artifacts. Would love to see what you find. |
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous download handling concerns remain addressed, and I found no new code concerns in the retry-limit update. The direct cache transport and shared Namespace client lifecycle are above the L1 approval ceiling; the branch also currently conflicts with main in go.mod/go.sum, and build #14096 is failing.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 21111, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
Amp-Thread-ID: https://ampcode.com/threads/T-01a0313b-c877-708c-a5f4-8de217bd8da9 Co-authored-by: Kate Sy <kate.sy@buildkite.com>
9c7480c to
9488a06
Compare
Previous concerns have been addressed.
There was a problem hiding this comment.
The rebase resolves the previous go.mod/go.sum conflict, and the download-handling concerns remain addressed; I found no new code concerns. The direct cache transport and shared Namespace client lifecycle remain above the L1 approval ceiling, and build #14097 is currently failing.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 21125, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
Description
Replace
nsc artifactsubprocesses with the Namespace Storage API, removing the runtime dependency on the NSC CLI while preserving cache expiry, misses, metrics, and integrity checks.Context
A-1435
Plan
Changes
Testing
go test ./...).go tool gofumpt -extra -w .).Disclosures / Credits
Implemented with Amp.