fix: memoize ISP auth to eliminate duplicate auth cycles#43
Merged
Conversation
…uest submit bootstrapISPAuth() now uses sync.Once so profile load + Authenticate runs exactly once per process, shared by bootstrapSCAService and bootstrapWorkflowsService. Also passes refreshAuth=false so cached, unexpired keyring tokens skip the network round-trip entirely. Reduces StartAuthentication calls from 3 to 1 for `grant request submit` and cuts keyring ops from ~18 to ~6 per invocation.
There was a problem hiding this comment.
Pull request overview
This PR reduces startup latency for commands like grant request submit by memoizing ISP profile load + authentication so multiple service bootstraps within a single CLI invocation reuse the same auth cycle and can reuse cached keyring tokens.
Changes:
- Introduces a
bootstrapISPAuth()helper incmd/root.gothat memoizes ISP auth/profile viasync.Once, shared across service bootstraps. - Updates
bootstrapWorkflowsService()to reuse the memoized ISP auth instead of re-authenticating. - Adds a unit test asserting memoization behavior, and documents the fix in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| cmd/root.go | Adds process-wide memoization for ISP auth/profile and wires it into SCA service bootstrap. |
| cmd/root_test.go | Adds a unit test verifying memoization across repeated bootstrapISPAuth() calls. |
| cmd/request.go | Switches workflows bootstrap to reuse the memoized ISP auth. |
| CHANGELOG.md | Documents the authentication-cycle reduction and performance impact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| t.Cleanup(func() { bootstrapImpl = origImpl }) | ||
|
|
||
| calls := 0 | ||
| stubAuth := sdkauth.NewIdsecISPAuth(true) // never Authenticate'd — used only for identity |
Comment on lines
+166
to
+168
| // resetBootstrapCache clears the memoized auth state. Intended for tests. | ||
| func resetBootstrapCache() { | ||
| bootstrapOnce = sync.Once{} |
- Use *sync.Once (pointer) instead of sync.Once value to avoid copylocks concern when resetting in tests - Use NewIdsecISPAuth(false) in test stub (non-caching; Authenticate never called so no keyring ops in CI) - Rename test to TestBootstrapISPAuth_MemoizesRepeatCalls to accurately describe what is tested
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
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.
Summary
grant request submitwas performing 2–3 full ISP auth cycles before the workspace picker appeared (~2.5s overhead), one perbootstrapSCAService/bootstrapWorkflowsServicecallbootstrapISPAuth()withsync.Oncememoization so profile load +Authenticateruns exactly once per process, shared by both bootstrap helpersrefreshAuthfromtrue→falseso cached, unexpired keyring tokens short-circuit the network round-trip entirely (verified by readingidsec_auth.go:144—refreshAuth=trueforced a live refresh even on valid tokens)bootstrapImplis avarfunc for test overriding without touchingsync.OnceResult (verified via
--verbose)StartAuthenticationcallsTest plan
make buildmake test— newTestBootstrapISPAuth_MemoizesAcrossServiceBootstrapsassertsbootstrapImplcalled exactly once across 3bootstrapISPAuth()invocationsmake lint./grant request submit --verbose— singleStartAuthenticationin log, workspace picker appears immediately