From 1b1ded058ced9aec8b9d2f214eecfe4e36c36e53 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 23 Jul 2026 17:15:39 -0500 Subject: [PATCH 1/4] Serialize MSAL cache updates across processes FileCache.Export wrote the cache atomically (temp + rename), which prevents torn reads but not lost updates: two concurrent invocations could each load the cache, refresh, and have the later Export clobber a peer's newer refresh-token/cache write. MSAL calls Replace and Export as unpaired callbacks (the silent cache-hit path calls Replace with no matching Export), so a lock scoped to those callbacks would leak. Instead, serialize the whole acquire at the app boundary: hold an exclusive cross-process advisory lock (via gofrs/flock) around the silent attempt and any interactive login. The lock is OS-released on process death, so a crash leaves no stale lock, and it dedupes concurrent refreshes since a waiter finds the fresh token already cached on release. The print path waits briefly then degrades to empty output (exit 0) so mise never hangs; the login path waits longer then errors. Pin gofrs/flock to v0.12.1 to keep the go 1.23 directive. Closes #2 Co-Authored-By: Claude Opus 4.8 (1M context) --- go.mod | 7 +++- go.sum | 14 ++++++- internal/token/token.go | 46 +++++++++++++++++++++ internal/token/token_test.go | 77 ++++++++++++++++++++++++++++++++++++ main.go | 76 ++++++++++++++++++++++++++++------- 5 files changed, 202 insertions(+), 18 deletions(-) diff --git a/go.mod b/go.mod index 657f938..669fbe6 100644 --- a/go.mod +++ b/go.mod @@ -2,12 +2,15 @@ module github.com/energyworldnet/az-devops-token go 1.23 -require github.com/AzureAD/microsoft-authentication-library-for-go v1.7.2 +require ( + github.com/AzureAD/microsoft-authentication-library-for-go v1.7.2 + github.com/gofrs/flock v0.12.1 +) require ( github.com/golang-jwt/jwt/v5 v5.2.2 // indirect github.com/google/uuid v1.3.0 // indirect github.com/kylelemons/godebug v1.1.0 // indirect github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 // indirect - golang.org/x/sys v0.5.0 // indirect + golang.org/x/sys v0.22.0 // indirect ) diff --git a/go.sum b/go.sum index 4dac208..82f695d 100644 --- a/go.sum +++ b/go.sum @@ -1,5 +1,9 @@ github.com/AzureAD/microsoft-authentication-library-for-go v1.7.2 h1:RHK7bS+HQMslb1sZpAokUt+zTVmue0hKSs2C791hhzU= github.com/AzureAD/microsoft-authentication-library-for-go v1.7.2/go.mod h1:HKpQxkWaGLJ+D/5H8QRpyQXA1eKjxkFlOMwck5+33Jk= +github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= +github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= +github.com/gofrs/flock v0.12.1 h1:MTLVXXHf8ekldpJk3AKicLij9MdwOWkZ+a/jHHZby9E= +github.com/gofrs/flock v0.12.1/go.mod h1:9zxTsyu5xtJ9DK+1tFZyibEV7y3uwDxPPfbxeeHCoD0= github.com/golang-jwt/jwt/v5 v5.2.2 h1:Rl4B7itRWVtYIHFrSNd7vhTiz9UpLdi6gZhZ3wEeDy8= github.com/golang-jwt/jwt/v5 v5.2.2/go.mod h1:pqrtFR0X4osieyHYxtmOUWsAWrfe1Q5UVIyoH402zdk= github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= @@ -8,6 +12,12 @@ github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8 h1:KoWmjvw+nsYOo29YJK9vDA65RGE3NrOnUtO7a+RF9HU= github.com/pkg/browser v0.0.0-20210911075715-681adbf594b8/go.mod h1:HKlIX3XHQyzLZPlr7++PzdhaXEj94dEiJgZDTsxEqUI= +github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= +github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsTg= +github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY= golang.org/x/sys v0.0.0-20210616045830-e2b7044e8c71/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= -golang.org/x/sys v0.5.0 h1:MUK/U/4lj1t1oPg0HfuXDN/Z1wv31ZJ/YcPiGccS4DU= -golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.22.0 h1:RI27ohtqKCnwULzJLqkv897zojh5/DwS/ENaMzUOaWI= +golang.org/x/sys v0.22.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= +gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= diff --git a/internal/token/token.go b/internal/token/token.go index 2877f64..00772dd 100644 --- a/internal/token/token.go +++ b/internal/token/token.go @@ -18,6 +18,7 @@ import ( "github.com/AzureAD/microsoft-authentication-library-for-go/apps/cache" "github.com/AzureAD/microsoft-authentication-library-for-go/apps/public" + "github.com/gofrs/flock" ) // adoResource is the public, global Azure DevOps resource ID (identical for @@ -32,6 +33,11 @@ const adoResource = "499b84ac-1321-427f-aa17-267ca6975798" // feed tokens live ~1h, so this leaves ample refresh headroom. const MinLifetime = 20 * time.Minute +// lockRetryDelay is how long WithLock sleeps between attempts to acquire a +// contended cross-process lock. Operations under the lock are sub-second, so a +// short poll keeps a waiter's latency low without busy-spinning. +const lockRetryDelay = 50 * time.Millisecond + // Scopes returns the delegated scopes requested for a feed token. func Scopes() []string { return []string{adoResource + "/vso.packaging"} } @@ -81,6 +87,46 @@ func (c *FileCache) Export(_ context.Context, m cache.Marshaler, _ cache.ExportH return os.Rename(tmp, c.Path) } +// WithLock runs fn while holding an exclusive cross-process advisory lock on +// lockPath, then releases it. It serializes the whole reload → refresh → +// export sequence across concurrent invocations so a later writer's Export +// cannot clobber a newer refresh-token/cache update a peer just wrote. The +// lock is an OS advisory lock, so it is released automatically if the process +// dies mid-operation — a crash leaves no stale lock. +// +// It blocks (polling every lockRetryDelay) until the lock is acquired or ctx is +// done. When ctx expires first it returns (false, nil) without running fn, so a +// caller on a latency-sensitive path can degrade rather than hang. A genuine +// lock error (e.g. the lock directory cannot be created) is returned as-is. +func WithLock(ctx context.Context, lockPath string, fn func()) (bool, error) { + /* The lock is acquired before Export creates the cache directory, so + ensure the parent exists first (mirrors Export). Otherwise the first run + on a machine with a not-yet-created cache dir couldn't create the + lockfile. */ + if err := os.MkdirAll(filepath.Dir(lockPath), 0o700); err != nil { + return false, err + } + fl := flock.New(lockPath) + locked, err := fl.TryLockContext(ctx, lockRetryDelay) + if err != nil { + /* A context error means we simply ran out of time waiting for a peer to + release — report it as a clean give-up (false, nil) so the caller can + degrade, not as a failure. */ + if errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + return false, nil + } + return false, err + } + if !locked { + return false, nil + } + /* The lock is advisory and process-scoped; an Unlock failure is not + actionable and the OS releases the lock on exit regardless. */ + defer fl.Unlock() //nolint:errcheck + fn() + return true, nil +} + // StripAccessTokens removes the access-token entries from a serialized MSAL // cache, leaving the refresh token and everything else intact. MSAL's silent // path then finds no cached access token and redeems the refresh token for a diff --git a/internal/token/token_test.go b/internal/token/token_test.go index 479cf41..94cfe37 100644 --- a/internal/token/token_test.go +++ b/internal/token/token_test.go @@ -1,7 +1,9 @@ package token_test import ( + "context" "encoding/json" + "path/filepath" "testing" "time" @@ -42,6 +44,81 @@ func TestStripAccessTokensRejectsInvalidJSON(t *testing.T) { } } +func TestWithLockRunsFnAndReleases(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "cache.lock") + + ran := false + locked, err := token.WithLock(context.Background(), lockPath, func() { ran = true }) + if err != nil { + t.Fatalf("WithLock: %v", err) + } + if !locked { + t.Fatal("expected to acquire an uncontended lock") + } + if !ran { + t.Error("fn should have run while the lock was held") + } + + // The lock must be released on return: a second acquisition succeeds. + locked, err = token.WithLock(context.Background(), lockPath, func() {}) + if err != nil { + t.Fatalf("WithLock (second): %v", err) + } + if !locked { + t.Error("lock should be released after WithLock returns") + } +} + +func TestWithLockCreatesParentDir(t *testing.T) { + // A not-yet-created cache dir (the pre-login state) must not stop the lock. + lockPath := filepath.Join(t.TempDir(), "nested", "cache.lock") + locked, err := token.WithLock(context.Background(), lockPath, func() {}) + if err != nil { + t.Fatalf("WithLock: %v", err) + } + if !locked { + t.Error("expected to acquire the lock after creating the parent dir") + } +} + +func TestWithLockContendedGivesUp(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "cache.lock") + + held := make(chan struct{}) + release := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + _, err := token.WithLock(context.Background(), lockPath, func() { + close(held) + <-release + }) + if err != nil { + t.Errorf("holder WithLock: %v", err) + } + }() + <-held // the lock is now held by the goroutine + + // A second acquisition under an expired context must give up without running + // fn, so a latency-sensitive caller can degrade rather than block. + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + ran := false + locked, err := token.WithLock(ctx, lockPath, func() { ran = true }) + if err != nil { + t.Fatalf("contended WithLock: %v", err) + } + if locked { + t.Error("should not acquire a lock another process holds") + } + if ran { + t.Error("fn must not run when the lock is not acquired") + } + + close(release) + <-done +} + func TestNeedsForcedRefresh(t *testing.T) { now := time.Unix(1_700_000_000, 0) cases := []struct { diff --git a/main.go b/main.go index 832cc18..04cc40a 100644 --- a/main.go +++ b/main.go @@ -97,29 +97,77 @@ func main() { return } - // A cached/refreshable token satisfies both paths: it's what `print` emits, - // and it makes `login` idempotent (no prompt when already signed in). - silentCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) - tok, ok := token.Silent(silentCtx, app, fc) - cancel() - if ok { - if login { - fmt.Fprintln(os.Stderr, "already signed in") + // Serialize the whole acquire across processes so a peer's reload → refresh → + // export can't interleave with ours and clobber a newer cache write. The lock + // is held from the silent attempt through any interactive sign-in, so two + // logins can't both prompt and race their writes; it also dedupes concurrent + // refreshes, since a waiter finds the fresh token already cached on release. + lockPath := cachePath() + ".lock" + + // Bound the wait for the lock. `print` shares mise's tolerance for a slow + // config load; `login` may queue behind an in-progress interactive sign-in, + // which is user-paced and can take a while. + acquireTimeout := 30 * time.Second + if login { + acquireTimeout = 2 * time.Minute + } + lockCtx, cancelLock := context.WithTimeout(context.Background(), acquireTimeout) + defer cancelLock() + + var ( + tok string + haveToken bool + loginErr error + ) + locked, lockErr := token.WithLock(lockCtx, lockPath, func() { + // A cached/refreshable token satisfies both paths: it's what `print` emits, + // and it makes `login` idempotent (no prompt when already signed in). + silentCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + tok, haveToken = token.Silent(silentCtx, app, fc) + cancel() + if haveToken || !login { return } - fmt.Print(tok) + // No cached token on the login path: sign in interactively, still under the + // lock. context.Background() (not lockCtx) bounds this — the browser round + // trip must not be cut short by the acquire timeout. + loginErr = interactiveLogin(context.Background(), app) + }) + if lockErr != nil { + fmt.Fprintf(os.Stderr, "cache lock: %v\n", lockErr) + if login { + os.Exit(1) + } + return + } + if !locked { + // A peer held the lock past our budget. On `print`, stay silent so mise's + // config load falls through to a clean 401; on `login`, surface it. + if login { + fmt.Fprintln(os.Stderr, "timed out waiting for another sign-in to finish") + os.Exit(1) + } + fmt.Fprintln(os.Stderr, "another az-devops-token invocation is busy — no token emitted") return } if !login { - // No usable token — degrade to empty output so mise's config load and the - // consuming package manager fall through to a clean 401 rather than erroring. - fmt.Fprintln(os.Stderr, "no token — run `az-devops-token login`") + if !haveToken { + // No usable token — degrade to empty output so mise's config load and the + // consuming package manager fall through to a clean 401 rather than erroring. + fmt.Fprintln(os.Stderr, "no token — run `az-devops-token login`") + return + } + fmt.Print(tok) return } - if err := interactiveLogin(context.Background(), app); err != nil { - fmt.Fprintf(os.Stderr, "login failed: %v\n", err) + if haveToken { + fmt.Fprintln(os.Stderr, "already signed in") + return + } + if loginErr != nil { + fmt.Fprintf(os.Stderr, "login failed: %v\n", loginErr) os.Exit(1) } fmt.Fprintln(os.Stderr, "signed in") From 46cab9450a54fb9d9f8845bcff93a8a134705455 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 23 Jul 2026 17:30:01 -0500 Subject: [PATCH 2/4] Silence contended print path; test lock cross-process Address CodeRabbit review on the cache-lock change: - main.go: on a lock-acquisition timeout the print path now returns silently (empty output, exit 0) instead of writing a diagnostic to stderr, matching the adjacent comment and the clean-degrade contract. The login path keeps its timeout message and exit 1. - token_test.go: replace the goroutine-based contention test with one that re-execs the test binary as a separate lock holder, so it exercises the genuine cross-process advisory lock and confirms the lock frees once the holder exits. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/token/token_test.go | 91 ++++++++++++++++++++++++++++-------- main.go | 6 +-- 2 files changed, 75 insertions(+), 22 deletions(-) diff --git a/internal/token/token_test.go b/internal/token/token_test.go index 94cfe37..cd588d6 100644 --- a/internal/token/token_test.go +++ b/internal/token/token_test.go @@ -1,8 +1,13 @@ package token_test import ( + "bufio" "context" "encoding/json" + "fmt" + "io" + "os" + "os/exec" "path/filepath" "testing" "time" @@ -81,26 +86,60 @@ func TestWithLockCreatesParentDir(t *testing.T) { } } -func TestWithLockContendedGivesUp(t *testing.T) { - lockPath := filepath.Join(t.TempDir(), "cache.lock") +// lockHolderEnv names the env var that switches TestWithLockCrossProcess into +// its child (lock-holder) role. The child holds the lock on the named path +// until the parent closes its stdin. +const lockHolderEnv = "AZ_DEVOPS_TOKEN_TEST_LOCK_PATH" + +// lockHeldSignal is written to stdout by the child once the lock is held, so +// the parent only starts contending after acquisition. +const lockHeldSignal = "LOCK_HELD" - held := make(chan struct{}) - release := make(chan struct{}) - done := make(chan struct{}) - go func() { - defer close(done) - _, err := token.WithLock(context.Background(), lockPath, func() { - close(held) - <-release +func TestWithLockCrossProcess(t *testing.T) { + if lockPath := os.Getenv(lockHolderEnv); lockPath != "" { + // Child role: hold the lock until the parent closes our stdin (EOF), + // signalling readiness first. + _, _ = token.WithLock(context.Background(), lockPath, func() { + fmt.Println(lockHeldSignal) + _, _ = io.Copy(io.Discard, os.Stdin) }) - if err != nil { - t.Errorf("holder WithLock: %v", err) - } - }() - <-held // the lock is now held by the goroutine + return + } + + lockPath := filepath.Join(t.TempDir(), "cache.lock") + + // Re-exec this test binary as a separate process that holds the lock, to + // exercise the genuinely cross-process advisory lock rather than same-process + // scheduling. + cmd := exec.Command(os.Args[0], "-test.run=^TestWithLockCrossProcess$") + cmd.Env = append(os.Environ(), lockHolderEnv+"="+lockPath) + cmd.Stderr = os.Stderr + stdin, err := cmd.StdinPipe() + if err != nil { + t.Fatalf("StdinPipe: %v", err) + } + stdout, err := cmd.StdoutPipe() + if err != nil { + t.Fatalf("StdoutPipe: %v", err) + } + if err := cmd.Start(); err != nil { + t.Fatalf("start lock holder: %v", err) + } + // Safety net: if an assertion below fails early, don't leave the child + // blocked (and the lockfile undeletable). + t.Cleanup(func() { + _ = stdin.Close() + _ = cmd.Process.Kill() + _ = cmd.Wait() + }) - // A second acquisition under an expired context must give up without running - // fn, so a latency-sensitive caller can degrade rather than block. + // Wait until the child reports the lock is held. + scanner := bufio.NewScanner(stdout) + for scanner.Scan() && scanner.Text() != lockHeldSignal { + } + + // The lock is held by another process: acquisition must give up within the + // deadline without running fn, so a latency-sensitive caller can degrade. ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) defer cancel() ran := false @@ -115,8 +154,22 @@ func TestWithLockContendedGivesUp(t *testing.T) { t.Error("fn must not run when the lock is not acquired") } - close(release) - <-done + // Release the holder and wait for it to exit, so the lock is fully released. + if err := stdin.Close(); err != nil { + t.Fatalf("close holder stdin: %v", err) + } + if err := cmd.Wait(); err != nil { + t.Fatalf("lock holder exited with error: %v", err) + } + + // The lock is acquirable again now that the holder is gone. + locked, err = token.WithLock(context.Background(), lockPath, func() {}) + if err != nil { + t.Fatalf("post-release WithLock: %v", err) + } + if !locked { + t.Error("lock should be acquirable after the holder exits") + } } func TestNeedsForcedRefresh(t *testing.T) { diff --git a/main.go b/main.go index 04cc40a..760812c 100644 --- a/main.go +++ b/main.go @@ -141,13 +141,13 @@ func main() { return } if !locked { - // A peer held the lock past our budget. On `print`, stay silent so mise's - // config load falls through to a clean 401; on `login`, surface it. + // A peer held the lock past our budget. On `print`, stay silent (empty + // output, exit 0) so mise's config load falls through to a clean 401; on + // `login`, surface it. if login { fmt.Fprintln(os.Stderr, "timed out waiting for another sign-in to finish") os.Exit(1) } - fmt.Fprintln(os.Stderr, "another az-devops-token invocation is busy — no token emitted") return } From 6f853060725e23fc5baf4bbaf2b1c0bf374304c7 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 23 Jul 2026 17:39:11 -0500 Subject: [PATCH 3/4] Harden cross-process lock test wait Address CodeRabbit review on the cross-process lock test: - Bound the readiness wait: read the child's stdout in a goroutine and select against a timeout, so a stalled child or one that exits before signalling (EOF) fails fast with a clear diagnostic instead of hanging or falling through to a misleading contended-lock assertion. Also check scanner.Err(). - Annotate the re-exec with //nolint:gosec: exec.Command(os.Args[0], ...) re-runs this test binary itself, a standard helper-process idiom that gosec's G204 flags as a false positive. Add a reason to the existing errcheck directive too. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/token/token.go | 4 +--- internal/token/token_test.go | 31 +++++++++++++++++++++++++++---- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/internal/token/token.go b/internal/token/token.go index 00772dd..2e75e83 100644 --- a/internal/token/token.go +++ b/internal/token/token.go @@ -120,9 +120,7 @@ func WithLock(ctx context.Context, lockPath string, fn func()) (bool, error) { if !locked { return false, nil } - /* The lock is advisory and process-scoped; an Unlock failure is not - actionable and the OS releases the lock on exit regardless. */ - defer fl.Unlock() //nolint:errcheck + defer fl.Unlock() //nolint:errcheck // advisory lock; unlock errors aren't actionable and the OS releases it on exit fn() return true, nil } diff --git a/internal/token/token_test.go b/internal/token/token_test.go index cd588d6..a8fa87e 100644 --- a/internal/token/token_test.go +++ b/internal/token/token_test.go @@ -111,7 +111,7 @@ func TestWithLockCrossProcess(t *testing.T) { // Re-exec this test binary as a separate process that holds the lock, to // exercise the genuinely cross-process advisory lock rather than same-process // scheduling. - cmd := exec.Command(os.Args[0], "-test.run=^TestWithLockCrossProcess$") + cmd := exec.Command(os.Args[0], "-test.run=^TestWithLockCrossProcess$") //nolint:gosec // re-execs this test binary itself, not external input cmd.Env = append(os.Environ(), lockHolderEnv+"="+lockPath) cmd.Stderr = os.Stderr stdin, err := cmd.StdinPipe() @@ -133,9 +133,32 @@ func TestWithLockCrossProcess(t *testing.T) { _ = cmd.Wait() }) - // Wait until the child reports the lock is held. - scanner := bufio.NewScanner(stdout) - for scanner.Scan() && scanner.Text() != lockHeldSignal { + // Wait until the child reports the lock is held, bounding the wait so a + // stalled child (never signals) or one that exits before signalling (EOF) + // fails fast with a clear diagnostic instead of hanging or falling through + // to a misleading contended-lock assertion. + ready := make(chan error, 1) + go func() { + scanner := bufio.NewScanner(stdout) + for scanner.Scan() { + if scanner.Text() == lockHeldSignal { + ready <- nil + return + } + } + if err := scanner.Err(); err != nil { + ready <- fmt.Errorf("reading lock holder output: %w", err) + return + } + ready <- fmt.Errorf("lock holder exited before signalling it held the lock") + }() + select { + case err := <-ready: + if err != nil { + t.Fatalf("waiting for lock holder: %v", err) + } + case <-time.After(10 * time.Second): + t.Fatal("timed out waiting for lock holder to acquire the lock") } // The lock is held by another process: acquisition must give up within the From af6675a428acbeaeb6ba3b8e825190d8fceb5243 Mon Sep 17 00:00:00 2001 From: Taylor Buchanan Date: Thu, 23 Jul 2026 17:45:07 -0500 Subject: [PATCH 4/4] Use CommandContext in lock test; drop errcheck nolint Address CodeRabbit review on the cache-lock change: - token_test.go: switch the cross-process helper to exec.CommandContext with a cancellable context cancelled from t.Cleanup, so the child is stopped if the test exits early (satisfies noctx and removes the explicit Process.Kill). - token.go: replace the //nolint:errcheck on the deferred Unlock with an explicit best-effort ignore and a rationale comment. The unlock error is deliberately not surfaced: this is a short-lived CLI about to exit, the OS releases the advisory lock and closes the descriptor on exit regardless, and folding a release hiccup into WithLock's return would make main discard an already-acquired token. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/token/token.go | 6 +++++- internal/token/token_test.go | 8 ++++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/internal/token/token.go b/internal/token/token.go index 2e75e83..d92aa7c 100644 --- a/internal/token/token.go +++ b/internal/token/token.go @@ -120,7 +120,11 @@ func WithLock(ctx context.Context, lockPath string, fn func()) (bool, error) { if !locked { return false, nil } - defer fl.Unlock() //nolint:errcheck // advisory lock; unlock errors aren't actionable and the OS releases it on exit + /* Release is best-effort: an Unlock failure is not actionable here — this is + a short-lived CLI about to exit, and the OS releases the advisory lock and + closes the descriptor on exit regardless. Folding it into the return would + also make a rare release hiccup discard an already-acquired token. */ + defer func() { _ = fl.Unlock() }() fn() return true, nil } diff --git a/internal/token/token_test.go b/internal/token/token_test.go index a8fa87e..372ea8e 100644 --- a/internal/token/token_test.go +++ b/internal/token/token_test.go @@ -110,8 +110,9 @@ func TestWithLockCrossProcess(t *testing.T) { // Re-exec this test binary as a separate process that holds the lock, to // exercise the genuinely cross-process advisory lock rather than same-process - // scheduling. - cmd := exec.Command(os.Args[0], "-test.run=^TestWithLockCrossProcess$") //nolint:gosec // re-execs this test binary itself, not external input + // scheduling. The context stops the helper if the test exits early. + holderCtx, cancelHolder := context.WithCancel(context.Background()) + cmd := exec.CommandContext(holderCtx, os.Args[0], "-test.run=^TestWithLockCrossProcess$") //nolint:gosec // re-execs this test binary itself, not external input cmd.Env = append(os.Environ(), lockHolderEnv+"="+lockPath) cmd.Stderr = os.Stderr stdin, err := cmd.StdinPipe() @@ -128,8 +129,7 @@ func TestWithLockCrossProcess(t *testing.T) { // Safety net: if an assertion below fails early, don't leave the child // blocked (and the lockfile undeletable). t.Cleanup(func() { - _ = stdin.Close() - _ = cmd.Process.Kill() + cancelHolder() _ = cmd.Wait() })