Serialize MSAL cache updates across processes - #3
Open
gtbuchanan wants to merge 4 commits into
Open
Conversation
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) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe change adds cross-process advisory locking around token acquisition and cache updates. ChangesToken cache locking
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant main
participant token.WithLock
participant TokenCache
main->>token.WithLock: Acquire cache lock with command-specific timeout
token.WithLock->>TokenCache: Fetch token or perform interactive sign-in
TokenCache-->>token.WithLock: Return token or login error
token.WithLock-->>main: Release lock and return acquisition status
main-->>main: Print token, status, or error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
gtbuchanan
marked this pull request as ready for review
July 23, 2026 22:21
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/token/token_test.go`:
- Around line 84-120: Update TestWithLockContendedGivesUp to use an
os/exec-launched helper process that acquires and holds the lock, rather than a
goroutine. Have the parent attempt WithLock with an expired or short-timeout
context, verify it returns without acquiring the lock or invoking the callback,
then wait for the helper to exit and confirm the lock can be acquired afterward.
In `@main.go`:
- Around line 143-151: Update the !locked handling in the main command flow so
the non-login print path returns without writing anything to stderr after the
timeout; retain the existing timeout message and exit behavior for login.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 754ba782-aa5b-411a-a8d1-52117128307c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modinternal/token/token.gointernal/token/token_test.gomain.go
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/token/token_test.go`:
- Around line 136-140: The readiness wait around the child-process scanner must
be bounded and validate successful signaling. Update the lock-held wait in the
token test to enforce a timeout, detect scanner errors or EOF before
lockHeldSignal, and fail immediately with a clear diagnostic; only continue to
the contended-lock assertion after the signal is received.
- Around line 111-116: Add an explicit gosec G204 suppression annotation to the
exec.Command invocation in TestWithLockCrossProcess, documenting that os.Args[0]
re-executes the current test binary and is not externally tainted. Keep the
command and environment behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e8b00504-d3ab-40c8-aaa4-fa468a5fb492
📒 Files selected for processing (2)
internal/token/token_test.gomain.go
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/token/token_test.go`:
- Line 114: Update the cross-process command in TestWithLockCrossProcess to use
exec.CommandContext with a cancellable context, and invoke that cancellation
from the test’s existing cleanup so the helper process is stopped if the test
exits early. Preserve the current self-reexec arguments and behavior.
In `@internal/token/token.go`:
- Line 123: Update WithLock to capture and return any error from the deferred
fl.Unlock() call instead of suppressing it, while preserving the existing
callback result and ensuring unlock errors are reported to callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: bb83aab7-59fa-4da4-942b-2d985ce8ee24
📒 Files selected for processing (2)
internal/token/token.gointernal/token/token_test.go
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2.
Problem
FileCache.Exportwrites the MSAL cache atomically (temp + rename), which prevents torn reads but not lost updates. Two concurrentaz-devops-tokeninvocations can eachReplace(load) the cache, each perform a network refresh, and the laterExportclobbers the earlier writer's newer refresh-token/cache state. The realistic trigger is parallel mise shims that refresh concurrently; the forced-refresh path from #1 slightly widens the write window in a token's last ~20 min.Fix
Serialize the whole acquire operation across processes with an exclusive cross-process advisory file lock (
github.com/gofrs/flock), held at the application boundary.Why the app boundary rather than inside the cache callbacks: MSAL calls
ReplaceandExportas unpaired callbacks — the silent cache-hit path callsReplacewith no matchingExport, while the refresh path (AuthResultFromToken) does its ownReplace→Write→Export. A lock acquired inReplaceand released inExportwould therefore leak on the cache-hit path.maininstead holds one exclusive lock across the silent attempt and any interactive login.token.WithLock(ctx, lockPath, fn): ensures the parent dir exists (mirrorsExport), acquires the lock polling every 50 ms until acquired orctxis done, runsfn, and releases viadefer. Context expiry is a clean give-up (false, nil) so callers can degrade; a genuine lock error is surfaced.FileCache,Silent, and theDropAccessTokensforced-refresh from Guarantee minimum remaining token lifetime #1 are unchanged.Export's temp+rename stays (still guards torn reads).The lock is an OS advisory lock, released automatically on process death — a crash leaves no stale lock. Serialization is also self-improving: once a holder finishes its refresh+export and releases, a waiter's
AcquireTokenSilentfinds the fresh token already cached and skips a redundant network refresh.Alternatives considered
microsoft-authentication-extensions-for-go/cache— it solves cross-process locking, but couples it to OS-native encrypted storage (macOS Keychain, Windows DPAPI, Linux libsecret viakeybase/go-keychain), which needs D-Bus/cgo and would break the headless mise-shim/CI use case, and it exposes no hook for this tool'sDropAccessTokensforced-refresh. Notably, that library just vendorsgofrs/flock— which this PR uses directly.Export(issue Serialize MSAL cache updates across processes in FileCache #2 option 2) — avoids a dependency but requires merging MSAL's undocumented internal cache JSON by key; fragile.gofrs/flockis pinned tov0.12.1becausev0.13.0requires Go 1.24;v0.12.1needs only Go 1.21, keeping the module'sgo 1.23directive. Its only dependency isgolang.org/x/sys, already pulled in transitively.Testing
go build ./...,go vet ./...,go test ./...pass.WithLockunit tests: runsfnand releases the lock; creates the parent dir when missing (pre-login state); gives up under contention without runningfn.