Repository navigation
fix(sdk/go): re-read edge token on every request - #4914
ericcurtin wants to merge 1 commit into
Conversation
Closes NVIDIA#4515 Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
@TaylorMutch @SDAChess could you please triage #4515 so the issue check can pass? PTAL, and |
|
@krishicks @pimlock could you please accept #4515 so the issue check can pass? PTAL, and |
|
Heads-up: the Go SDK moves to ai-openshell/sdk-go on 2026-11-03 (details in #5031). For this PR that means:
Nothing to do on your side either way. cc @TaylorMutch, a review by Monday 2026-11-02 would let this land in core. |
|
The Go SDK freeze is tomorrow, Tuesday 2026-11-03, at the end of the day (#5031). This fix is small and SDK-only, so it would be good to land it in core before then. Two things block it:
@TaylorMutch @pimlock, could one of you accept the issue and review? If it doesn't make it, it gets ported to ai-openshell/sdk-go with the original commits, as announced above. |
|
/ok to test 93dfbf1 |
Compatibility review (advisory)New or updated breaking changes; see the full report for all findings. 1. go-sdk — No explicit risk acceptance foundgateway.NewClient with cloudflare_jwt previously cached a successfully loaded edge_token or cf_token, but now rereads it for every RPC and fails when it is missing, empty, or unreadable.
Review details
Advisory review; publication gates are unchanged. |
rhuss
left a comment
rhuss
left a comment
There was a problem hiding this comment.
cc-review Summary
This review was prepared with AI reviewer agents that read the diff and the surrounding code. The failure scenarios in the comments were derived from reading the code and were not executed.
Thanks for the small, focused fix. Making the edge path reread the token like the OIDC path does is a nice simplification. All five findings below are non-blocking suggestions (four Minor, one Notable). Most of them come down to a doc tweak, a sentence in the PR description, or a follow-up, so once those are sorted this looks ready for a maintainer.
What Went Well
lazyEdgeAuth.GetRequestMetadata(token.go:140) now matchesdiskTokenSource.Token(token.go:86): both auth modes reread the CLI-managed token file per call, so a rotated or revoked token takes effect without a restart.- Removing
edgeTokenLoaderdrops a mutex, a cached field and thesyncimport. The provider is now stateless, with nothing left to race on across concurrent RPCs. readEdgeToken(token.go:33-61) wraps every failure inErrTokenLoad, names the cause, keeps token contents out of error strings, and falls back tocf_tokenonly whenedge_tokenis absent, so an unreadable primary file cannot silently downgrade to a stale legacy credential.- The new tests go through the public
GetRequestMetadata, assert the exactBearer <token>header, and userequire.ErrorIs(err, ErrTokenLoad)instead of a barerequire.Error.TestLazyEdgeAuth_ReloadsRewrittenTokenencodes the #4515 reproduction directly. RequireTransportSecuritystill returnstrue, andNewClientstill succeeds without a token file (FR-007), withTestResolveAuthProvider_CloudflareJWTcovering that construction.
Findings
| Severity | File | Description | Source |
|---|---|---|---|
| Minor | sdk/go/openshell/v1/gateway/token.go:137 | doc.go:55-56 still describes a first-attempt-only token load |
architecture |
| Notable | sdk/go/openshell/v1/gateway/token.go:137 | edge.NewTunnelProxy still sends a fixed edge token; scope not stated in the PR |
goal-alignment |
| Minor | sdk/go/openshell/v1/gateway/token.go:141 | Per-call reads can overlap the CLI's truncate-then-write of edge_token |
correctness |
| Minor | sdk/go/openshell/v1/gateway/token.go:141 | Failing after logout is a behavior change not declared in the PR description | goal-alignment |
| Minor | sdk/go/openshell/v1/gateway/token_test.go:87 | No test covers the token file being removed after a successful load | test-quality |
All findings are posted as inline comments.
Review Details
- Findings posted: 5
- Findings reviewed and not posted: 0
- Gate outcome: PASS
- Participating reviewers: architecture, correctness, goal-alignment, production, test-quality, gator (security also ran and reported no findings)
| // GetRequestMetadata loads the edge token lazily and returns it as a | ||
| // Bearer authorization header. The first load is cached by the | ||
| // underlying edgeTokenLoader. | ||
| // GetRequestMetadata reads the edge token from disk on every call and |
There was a problem hiding this comment.
Minor: With per-call reads, the package doc in doc.go:55-56 still says token files "are loaded lazily on first authentication attempt". That no longer holds for edge_token and cf_token, which are now read on every RPC. (doc.go is outside this diff, hence the comment here.)
Why this matters: Go SDK consumers learn the contract from the godoc. Reading it, they would expect a one-time load and would not expect that rotating or deleting the file affects a client that has already authenticated.
Suggested fix: Reword doc.go:55-56 to say token files are read on each authentication attempt, so tokens refreshed by the CLI are picked up. The lazyEdgeAuth comment at lines 131-132 is still accurate for the missing-at-construction case. If you touch it anyway, something like "A missing or empty token file surfaces as ErrTokenLoad on the RPC that needs it" covers both cases.
Source: architecture agent
| // GetRequestMetadata loads the edge token lazily and returns it as a | ||
| // Bearer authorization header. The first load is cached by the | ||
| // underlying edgeTokenLoader. | ||
| // GetRequestMetadata reads the edge token from disk on every call and |
There was a problem hiding this comment.
Notable: The reload now covers gRPC request metadata, but edge.NewTunnelProxy (edge/tunnel.go:85) still takes the edge token as a fixed string, stores it, and sends it on every WebSocket dial (lines 244-245). A tunnel that reconnects after a CLI re-login keeps presenting the old token. The triage notes on #4515 name the tunnel as part of the same problem, and the PR description doesn't say whether it is in or out of scope.
Why this matters: The acceptance criterion of #4515 (subsequent RPCs from an existing client use the new token) is met for gateway.NewClient, so this is about completeness, not a defect in the changed code. A long-running process that uses the tunnel still sees the stale-token behavior.
Suggested fix: Either add a line to the PR description saying the tunnel is a follow-up (a tracking issue would help it survive the move to ai-openshell/sdk-go), or let NewTunnelProxy accept a token supplier function so both paths reread the file.
Source: goal-alignment agent
| // tokens rewritten by the CLI without a process restart. | ||
| func (a *lazyEdgeAuth) GetRequestMetadata(_ context.Context, _ ...string) (map[string]string, error) { | ||
| token, err := a.loader.load() | ||
| token, err := readEdgeToken(a.dir) |
There was a problem hiding this comment.
Minor: Reading edge_token on every RPC now overlaps with the CLI's writer. store_edge_token in crates/openshell-bootstrap/src/edge_token.rs:29 uses std::fs::write, which truncates and then writes, with no temp file and rename. An RPC issued while openshell gateway login (or the legacy cf_token migration) rewrites the file can read it empty, and readEdgeToken then returns ErrTokenLoad ("edge_token file is empty"). The removed cache used to shield an authenticated client from that window.
Why this matters: The window is narrow and the realistic outcome is one retryable RPC failure, so this is hardening rather than a regression. The OIDC path on main already has the same pattern (store_oidc_token also uses std::fs::write, and diskTokenSource.Token rereads per call).
Suggested fix: The cleanest fix is on the writer side: have store_edge_token (and ideally store_oidc_token) write to a temp file in the same directory and rename it over the target, which makes every reader safe. Keeping the last good token in lazyEdgeAuth for the empty-file case would also work, but a broad "fall back on any read error" would undo logout revoking a running client (see the other comment on this line), so keep it narrow if you go that way. Either is fine as a follow-up.
Source: correctness agent (also flagged by: production agent, gator agent)
| // tokens rewritten by the CLI without a process restart. | ||
| func (a *lazyEdgeAuth) GetRequestMetadata(_ context.Context, _ ...string) (map[string]string, error) { | ||
| token, err := a.loader.load() | ||
| token, err := readEdgeToken(a.dir) |
There was a problem hiding this comment.
Minor: Dropping the cache also changes what happens after a successful load. If edge_token and cf_token later go missing, empty, or unreadable (for example after openshell gateway logout, which deletes both files), every following RPC now fails with ErrTokenLoad. The advisory compatibility review on this PR flagged this as a go-sdk behavior change with no explicit risk acceptance, and the description only says the loader was dropped to match the OIDC path.
Why this matters: Failing after logout is arguably the right outcome and matches diskTokenSource, but SDK consumers with long-running clients should learn about it from the PR rather than discover it in production. A short note also answers the compatibility review.
Suggested fix: Add a sentence to the PR description that accepts the change and gives the reason (logout now revokes a running client, as the OIDC path already does when oidc_token.json is gone). If maintainers would rather keep the old resilience, fall back to the last good token only on transient read errors, so rewrites still take effect.
Source: goal-alignment agent
| } | ||
|
|
||
| func TestEdgeTokenLoader_Lazy(t *testing.T) { | ||
| func TestLazyEdgeAuth_ReloadsRewrittenToken(t *testing.T) { |
There was a problem hiding this comment.
Minor: The removed TestEdgeTokenLoader_Lazy pinned what happens when the token file disappears after a successful load. The two new tests cover a rewritten file and a token that shows up late, but nothing covers a successful call followed by the file being removed or emptied.
Why this matters: That is the exact behavior the compatibility review asks about. Whichever way it gets settled, the contract can change without any test failing.
Suggested fix: Add something like TestLazyEdgeAuth_TokenRemovedAfterSuccess: write edge_token, call GetRequestMetadata, then os.Remove the file (and in a second case write it empty), and assert the chosen outcome, for the current design require.ErrorIs(t, err, ErrTokenLoad).
Source: test-quality agent
|
Label |
purp
left a comment
purp
left a comment
There was a problem hiding this comment.
gator-agent
PR Review Status
The independent code review found no blocking defects in this focused fix for accepted issue #4515. The new reload tests cover replacing a token and creating one after the client starts; current Branch Checks, including Go SDK, pass.
Thanks @ericcurtin for flagging the issue-acceptance gate. I checked #4515 and it now has state:accepted; the failing check predates acceptance and its log explicitly requests a rerun.
Action required: A maintainer should rerun PR Issue Acceptance, approve the pending Trivy Changes workflow, and click Re-run all jobs on Branch E2E Checks now that test:e2e is applied. Before pipeline handoff, add a short published docs note describing token rereading and the need to keep the token file readable, or provide a maintainer-authored explanation that docs are unnecessary. Maintainer approval of the PR is still outstanding.
Blocking code findings: None.
Carried findings: None.
Gator metadata
- Validation: Concentrated Go SDK bug fix implementing accepted issue #4515.
- Docs: No Fern docs changes; direct authentication behavior change requires a docs note or maintainer explanation.
- Checks: Branch Checks and Helm Lint pass; DCO and vouch pass. Issue Acceptance has a stale failure. Trivy Changes is action_required.
- E2E: test:e2e applied for gateway credential behavior. Mirror matches the head. Label-help bot requires a rerun; prior E2E success skipped the runtime suites without this label. Workflow reruns were not explicitly authorized for this invocation.
- Head SHA:
93dfbf112e6e07d2c522f645b14e781348e846a5 - Base SHA:
c4688aa8715ea75d6c39fe468361dbbc16c691fa - Merge base SHA:
c4688aa8715ea75d6c39fe468361dbbc16c691fa - Patch ID:
44785ed029d2e819f0564e973ee100044196e5dd - Gator payload:
13 - Review mode:
initial - Previous reviewed SHA:
none - Review budget exhausted:
no - Maintainer decision required:
no - Next state:
gator:blocked - Blocked reason:
test_dispatch_required
Summary
Re-read
edge_tokenon every request so a rewritten token is used without a restart.Related Issue
Closes #4515
Changes
edgeTokenLoader;lazyEdgeAuthreads the file per call, like the OIDC path.Testing
Ran
go test ./...andgolangci-lint runinsdk/go.Checklist