Serialize U2M token refreshes across processes - #6759
Vivek1106-04 wants to merge 2 commits into
Conversation
Take an advisory lock on ~/.databricks/token-cache.lock around the read-refresh-write sequence, and re-read the store once it is held so a token another process just exchanged is reused instead of exchanged again.
|
An authorized user can trigger integration tests manually by following the instructions below: Trigger: Inputs:
Checks will be approved automatically on success. |
Approval status: pending
|
1 similar comment
Approval status: pending
|
This branch has not been deployed
1 participant
Changes
Adds cross-process coordination around the U2M read-refresh-write sequence, taking up the TODO in
PersistentAuth.refresh.storage.LockTokenStoretakes an advisory lock on~/.databricks/token-cache.lock(flockon Unix,LockFileExon Windows) and blocks until it is available. The lock file sits next totoken-cache.jsonand is used by the keyring backend too, which has no file of its own to lock. Once the lock is held,refreshre-reads the store: if the process that held the lock already exchanged the same refresh token, its result is returned instead of exchanging again.The lock is passed by the two places that build a store shared with other invocations,
cmd/auth/token.goandlibs/auth/credentials.go. An in-memory store has no other process to coordinate with, so it keeps the previous behavior.Why
Fixes #6051.
The CLI is stateless, so two invocations for the same profile load the same cached refresh token, both exchange it with the IdP, and race to write the result back.
PersistentAuth.refreshdocumented this as a known gap.Since the SDKs began appending
--force-refresh, the race fails hard rather than silently.ForceRefreshTokenhas no fallback to the cached token by design, so a losing writer surfaces asforced token refresh: cache update: exit status 45from the macOS keyring, breaking any automation that runs several CLI invocations at once.Two decisions worth a reviewer's attention:
--force-refreshsemantics are unchanged for late callers. The re-read only short-circuits a process whose token was superseded while it waited. An invocation that starts after a refresh completed still exchanges, so N sequential force-refreshes remain N exchanges. Collapsing those too would mean giving--force-refresha freshness window, which is a semantic change I did not want to make here.recoverStoreUpdateis left in place: it still covers a concurrent writer that does not take the lock, such as an older CLI or a direct SDK user.Tests
libs/auth/storage/lock_test.go: lock file creation, release, and contention. The contending holder is a re-executed test binary, because on Unix an flock is shared by every descriptor in the process that took it, so a second in-process acquisition would succeed and prove nothing.libs/auth/u2m/persistent_auth_test.go: a token refreshed by another process while waiting is reused without an exchange (an emptySliceTransportfails the test if one is attempted), an unchanged cache still exchanges and stores, and a lock error is surfaced.acceptance/cmd/auth/token/force-refresh-concurrent: five concurrentauth token --force-refreshinvocations for one profile all return a token and leave a valid cache entry. The exchange count is deliberately not asserted there: how many of the five read the cache before the first writer commits is timing-dependent, so it would be flaky. The unit tests pin the dedupe instead.flockfails the cross-process test; dropping the re-read under the lock fails the reuse test.go test ./libs/auth/... ./cmd/auth/...,go test ./acceptance -run 'TestAccept/cmd/auth',GOOS=windows go build ./...,GOOS=linux go build ./...,./task fmt,./task ws,./task lint(0 issues)../task test: 10415 tests, 6 failures, all reproducing on a cleanmain(internal/buildfetching from raw.githubusercontent, andlibs/dyn/jsonloaderasserting a Go version's JSON error text).The keyring collision itself (
exit status 45) cannot be reproduced in the acceptance suite, which has no OS keychain; the plaintext store surfaces the same race as redundant token exchanges instead.