bugfix(drapi): don't hammer the version API - #493
Conversation
… probes When GetAPIKey fails (e.g. expired token), token stays "" so every subsequent Get/Post/… call re-invokes VerifyToken, generating a /api/v2/version/ request for each API call in the process lifetime. Introduce tokenErr alongside the existing token memoization and a resolveToken() helper that caches both the success and the first failure. SetToken clears tokenErr so callers that inject a fresh token (e.g. auth flows) still work correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…en() Replace the duplicated inline token-fetch blocks in each verb helper with the new resolveToken() introduced in the previous commit, so auth failure memoization applies consistently across all outbound HTTP calls. Removes the now-unused "context" import from each file. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rename tokenErr → errToken to satisfy golangci-lint errname rule. Update resetTokenForTest to save/restore errToken alongside token so tests that run after an auth-failure test don't inherit the cached error. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Inline comments at each change point explain the why for future developers: why errToken is cleared in SetToken, why call sites delegate to resolveToken() rather than inlining GetAPIKey, and why the test helper resets errToken alongside token to prevent cross-test bleed. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sync.Once is the idiomatic primitive for "call at most once per process." It replaces the manual nil-check pattern in resolveToken() and makes the intent clearer. SetToken (a test seam) resets the Once so tests can inject a known token without going through GetAPIKey; the Do callback guards on token == "" so a pre-seeded value is never overwritten. Also removes a now-stale nolintlint directive in telemetry/userid.go. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Centralises token stubbing in testutil_test.go. StubAPIToken uses t.Cleanup so call sites drop the defer ...() pattern entirely. The old resetTokenForTest helper in post_test.go is deleted; all 19 call sites across auth, patch, delete, and post tests are updated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…StubAPIToken Moving the func var to config breaks the import cycle that previously prevented a shared test helper: testutil can now import config (which doesn't import drapi) without creating a cycle. - config/auth.go: declare GetAPITokenFunc alongside GetAPIKey - drapi/get.go: call config.GetAPITokenFunc; remove local var - drapi/testutil_test.go: delegate func swap to testutil.StubAPIToken - testutil/drapi.go: new shared StubAPIToken for black-box tests - telemetry/userid_test.go: replace local resetTokenForTest with testutil.StubAPIToken Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| // SetToken sets the cached API token. | ||
| func SetToken(value string) { | ||
| token = value | ||
| return token, errToken |
There was a problem hiding this comment.
Unsynchronized resolveToken races on new errToken variable
Medium Severity
resolveToken() reads and writes the new package-level errToken (and token) without any synchronization. Under concurrent access — multiple goroutines calling Get/Post/Delete/Patch — all of them can simultaneously observe errToken == nil and token == "", then all call config.GetAPITokenFunc, defeating the "at most once" memoization goal. With error caching, a race could also cause one goroutine to permanently cache a transient error, blocking all subsequent API calls. The PR's own AGENTS.md addition recommends sync.Once for exactly this pattern.
Triggered by project rule: Bugbot Rules for DataRobot CLI
Reviewed by Cursor Bugbot for commit d877a03. Configure here.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit f7ece89. Configure here.
| return nil, err | ||
| } | ||
| // resolveToken memoizes both success and failure; see get.go for rationale. | ||
| if token, err = resolveToken(); err != nil { |
There was a problem hiding this comment.
Data race: unsynchronized writes to package-level token
Medium Severity
Post(), Patch(), Delete(), and SetAuthHeaders() all assign resolveToken()'s return value back to the package-level token variable (if token, err = resolveToken()) outside the sync.Once protection. Concurrent callers race on that write. Get() correctly uses a local variable (tok, err := resolveToken()). The comment on resolveToken() claims it's "safe for concurrent callers," but the callers in these functions undermine that guarantee by performing unsynchronized writes to the shared token variable.
Additional Locations (2)
Triggered by project rule: Bugbot Rules for DataRobot CLI
Reviewed by Cursor Bugbot for commit f7ece89. Configure here.
1 participant
RATIONALE
When
GetAPIKeyfails (i.e. API token expires after 12 hours), the cachedinternal.drapi.get.tokenvariable stayed empty. This causes every subsequent API call in the process to invokeGET /api/v2/versionviaVerifyToken.We've seen in https://datarobot.slack.com/archives/C07GTBE7UAE/p1778257695531299 this possibly happening. Maybe it's a CI job gone awry, or a user's script or agent gone awry. Not sure.
This can be properly fixed by caching not just the API token, but also the error response when verifying a token.
CHANGES
track GetAPIKey failure in
errTokenalongsidetoken, which live ininternal/drapi.memoize these values by a new
resolveToken()function that lives ininternal/drapi.replace all duplicated cache miss logic throughout the package with
resolveToken()remove GetToken/SetToken test seams (which while documented as such, are still part of the "exported production interface" for
internal/drapi.add a new
GetAPITokenFuncfunction variable ininternal/configthat defaults toGetAPIKey(), but can be overridden in tests.override
GetAPITokenFuncas a generic test fixture, but also in ininternal/drapiwhere we have an additional cache to resetPR Automation
Comment-Commands: Trigger CI by commenting on the PR:
/trigger-smoke-testor/trigger-test-smoke- Run smoke tests/trigger-install-testor/trigger-test-install- Run installation testsLabels: Apply labels to trigger workflows:
run-smoke-testsorgo- Run smoke tests on demand (only works for non-forked PRs)Important
For Forked PRs: The
run-smoke-testslabel won't work. A required Smoke Tests check will block merge until a maintainer acts:/approve-smoke-teststo run smoke tests (results will set the check)/skip-smoke-teststo bypass the check without running testsPlease comment requesting a maintainer review if you need smoke tests to run.
Note
Medium Risk
Touches shared API authentication/caching used by all HTTP verbs; incorrect memoization or initialization order could break requests or mask token refresh behavior.
Overview
Fixes
internal/drapitoken caching to memoize both the API token and token-fetch failure viasync.Once(resolveToken+errToken), preventing repeatedGetAPIKey/verification calls when the token is missing/expired.Updates all verb helpers (
Get,Post,Patch,Delete) andSetAuthHeadersto use the new resolver, addsdrapi.Init(config.GetAPIKey)at CLI startup, and refactors tests to use a newStubAPITokenhelper (including a sharedinternal/testutilversion for non-drapipackages). Documentation is tweaked to emphasize avoiding global mutable state in tests and to call out memoizing failures in caches.Reviewed by Cursor Bugbot for commit f7ece89. Bugbot is set up for automated code reviews on this repo. Configure here.