Skip to content

fix(sdk/go): re-read edge token on every request #4914

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ericcurtin wants to merge 2 commits into
base: main
Choose a base branch
from
+59 −53
Open

fix(sdk/go): re-read edge token on every request #4914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
93dfbf1
fix(sdk/go): re-read edge token on every request
ericcurtin Oct 25, 2026
308bb8e
fix(sdk-go): address edge token review feedback
ericcurtin Nov 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/sdk/go.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ Omit `CAFile` to use system roots. For renewable OIDC service credentials, use
`oidc.NewClientCredentialsAuth` from
`github.com/NVIDIA/OpenShell/sdk/go/openshell/v1/oidc`.

When you load a gateway registered by the CLI, the SDK rereads the edge token
file on each request, so a token refreshed by `openshell gateway login` takes
effect without a restart. Keep the token file readable. After
`openshell gateway logout`, RPCs from a running client fail.

## Create and Use a Sandbox

Resource methods take an explicit workspace. The following example creates a
Expand Down
4 changes: 3 additions & 1 deletion sdk/go/openshell/v1/gateway/doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@
// /etc/openshell/gateways/<name>/metadata.json (system)
//
// Token files (edge_token, cf_token, oidc_token.json) sit alongside
// metadata.json and are loaded lazily on first authentication attempt.
// metadata.json and are read on each authentication attempt, so tokens
// refreshed by the CLI are picked up. A missing or empty file surfaces as
// [ErrTokenLoad] on the RPC that needs it.
//
// # Error Handling
//
Expand Down
2 changes: 1 addition & 1 deletion sdk/go/openshell/v1/gateway/gateway.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ func resolveAuthProvider(cfg *Config) (types.AuthProvider, error) {
// Token loading is deferred to GetRequestMetadata so that
// NewClient succeeds even when the token file is missing.
// The error surfaces on first authentication attempt (FR-007).
return &lazyEdgeAuth{loader: &edgeTokenLoader{dir: cfg.Dir}}, nil
return &lazyEdgeAuth{dir: cfg.Dir}, nil

case AuthModeOIDC:
src := newDiskTokenSource(cfg.Dir)
Expand Down
37 changes: 5 additions & 32 deletions sdk/go/openshell/v1/gateway/token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"os"
"path/filepath"
"strings"
"sync"
"time"

"golang.org/x/oauth2"
Expand All @@ -28,32 +27,6 @@ const (
oidcTokenFile = "oidc_token.json"
)

// edgeTokenLoader provides lazy, thread-safe loading of the edge token
// from disk. Successful loads are cached; failures are retried so a token
// created after client construction can be picked up without a restart.
type edgeTokenLoader struct {
dir string
mu sync.Mutex
token string
}

// load returns the edge token string, reading it from disk on the first
// call. It tries edge_token first, falling back to cf_token for legacy
// compatibility. Only a successful result is cached.
func (l *edgeTokenLoader) load() (string, error) {
l.mu.Lock()
defer l.mu.Unlock()
if l.token != "" {
return l.token, nil
}
token, err := readEdgeToken(l.dir)
if err != nil {
return "", err
}
l.token = token
return token, nil
}

// readEdgeToken reads the edge token from the given directory. It tries
// edge_token first, then cf_token as a legacy fallback. The file content
// is trimmed of surrounding whitespace.
Expand Down Expand Up @@ -158,14 +131,14 @@ func (d *diskTokenSource) Token() (*oauth2.Token, error) {
// NewClient succeeds even when the token file is missing on disk (FR-007).
// The error surfaces on the first authentication attempt.
type lazyEdgeAuth struct {
loader *edgeTokenLoader
dir string
}

// 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

@rhuss rhuss Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rhuss rhuss Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// returns it as a Bearer authorization header. Reading each time picks up
// 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)

@rhuss rhuss Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@rhuss rhuss Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if err != nil {
return nil, err
}
Expand Down
64 changes: 45 additions & 19 deletions sdk/go/openshell/v1/gateway/token_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
package gateway

import (
"context"
"os"
"path/filepath"
"testing"
Expand Down Expand Up @@ -83,41 +84,66 @@ func TestReadEdgeToken_EmptyFallbackFile(t *testing.T) {
assert.Contains(t, err.Error(), "empty")
}

func TestEdgeTokenLoader_Lazy(t *testing.T) {
func TestLazyEdgeAuth_ReloadsRewrittenToken(t *testing.T) {

@rhuss rhuss Nov 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dir := t.TempDir()
writeFile(t, dir, edgeTokenFile, "lazy-token")
writeFile(t, dir, edgeTokenFile, "old-token")

loader := &edgeTokenLoader{dir: dir}
auth := &lazyEdgeAuth{dir: dir}

// First call reads from disk.
token1, err := loader.load()
md, err := auth.GetRequestMetadata(context.Background())
require.NoError(t, err)
assert.Equal(t, "lazy-token", token1)
assert.Equal(t, "Bearer old-token", md["authorization"])

// Remove the file. Second call should return cached value.
require.NoError(t, os.Remove(filepath.Join(dir, edgeTokenFile)))
writeFile(t, dir, edgeTokenFile, "new-token")

token2, err := loader.load()
md, err = auth.GetRequestMetadata(context.Background())
require.NoError(t, err)
assert.Equal(t, "lazy-token", token2, "should return cached token")
assert.Equal(t, "Bearer new-token", md["authorization"])
}

func TestEdgeTokenLoader_LazyError(t *testing.T) {
func TestLazyEdgeAuth_PicksUpLateToken(t *testing.T) {
dir := t.TempDir()
// No token files exist.

loader := &edgeTokenLoader{dir: dir}
auth := &lazyEdgeAuth{dir: dir}

// First call fails.
_, err1 := loader.load()
require.Error(t, err1)
_, err := auth.GetRequestMetadata(context.Background())
require.ErrorIs(t, err, ErrTokenLoad)

// Write the file now. A transient load failure must not be cached.
writeFile(t, dir, edgeTokenFile, "late-token")

token, err2 := loader.load()
require.NoError(t, err2)
assert.Equal(t, "late-token", token)
md, err := auth.GetRequestMetadata(context.Background())
require.NoError(t, err)
assert.Equal(t, "Bearer late-token", md["authorization"])
}

func TestLazyEdgeAuth_TokenRemovedAfterSuccess(t *testing.T) {
tests := []struct {
name string
mutate func(t *testing.T, dir string)
}{
{"removed", func(t *testing.T, dir string) {
require.NoError(t, os.Remove(filepath.Join(dir, edgeTokenFile)))
}},
{"empty", func(t *testing.T, dir string) {
writeFile(t, dir, edgeTokenFile, "")
}},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
dir := t.TempDir()
writeFile(t, dir, edgeTokenFile, "tok")
auth := &lazyEdgeAuth{dir: dir}

_, err := auth.GetRequestMetadata(context.Background())
require.NoError(t, err)

tc.mutate(t, dir)

_, err = auth.GetRequestMetadata(context.Background())
require.ErrorIs(t, err, ErrTokenLoad)
})
}
}

// --- T015: diskTokenSource tests ---
Expand Down
Loading