fix: resolve concurrency races in RuntimeContext#330
Conversation
- getAPIClient: replace check-then-act with sync.OnceValues, matching the factory_default.go convention; use NewAPIClientWithConfig to avoid post-construction config override; fall back to direct construction for test contexts that bypass newRuntimeContext. - outputErr: guard first-error capture with sync.Once to prevent data races if Out() is ever called from concurrent goroutines. Change-Id: I99c94c3dcb7663fa61571c9720163e41a5fc0e36
github-actions
bot
added
the
size/M
📝 WalkthroughWalkthroughRuntimeContext in Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR resolves two concurrency hazards in Confidence Score: 5/5PR is safe to merge — the concurrency fixes are correct and the behavioral change in auth.go is properly tested. No P0 or P1 findings. The sync.OnceValues and sync.Once usage is idiomatic and correct. The auth.go migration maintains equivalent token-scoping behavior. The new regression test provides direct coverage of the changed auth path. All prior thread concerns are either addressed or intentionally deferred. No files require special attention.
|
| Filename | Overview |
|---|---|
| shortcuts/common/runner.go | Concurrency fixes: replaces check-then-act API client caching with sync.OnceValues; guards first jq-error capture in Out() with sync.Once; fallback path for test contexts remains intentionally un-cached. |
| cmd/auth/auth.go | Migrates getAppInfo from f.LarkClient().Do() to f.NewAPIClient().DoSDKRequest(core.AsBot), making auth consistent with the rest of the codebase; DoSDKRequest sets SupportedAccessTokenTypes for tenant implicitly. |
| cmd/auth/auth_test.go | Adds TestAuthScopesRun_UsesTenantAccessTokenFromCredentialProvider; verifies exactly one TAT resolution and correct Authorization header on the mocked HTTP stub. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[runShortcut] --> B[newRuntimeContext]
B --> C["apiClientFunc = sync.OnceValues(...)"]
B --> D[larkSDK eagerly initialized]
A --> E[s.Execute called]
E --> F["getAPIClient()"]
F --> G{apiClientFunc != nil?}
G -- yes --> H["apiClientFunc() — runs once, cached"]
G -- no --> I["Factory.NewAPIClientWithConfig(ctx.Config) — test fallback"]
E --> J["Out() / OutFormat() with JqExpr"]
J --> K["JqFilter error?"]
K -- yes --> M["outputErrOnce.Do → write outputErr once"]
A --> N["return rctx.outputErr"]
Reviews (2): Last reviewed commit: "fix: use tenant token for auth scopes" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shortcuts/common/runner.go (1)
80-87: Fallback path lacks thread-safety for test contexts.When
apiClientFuncis nil (test contexts bypassingnewRuntimeContext), the fallback callsFactory.NewAPIClientWithConfigdirectly without caching. If tests invokegetAPIClient()concurrently on the sameRuntimeContext, each call creates a newAPIClientwithout synchronization.This is likely intentional for simple test helpers, but worth documenting to prevent future test authors from assuming thread-safety in all code paths.
📝 Consider adding a comment to clarify the fallback behavior
// getAPIClient returns the cached APIClient, creating it on first use. // Thread-safe via sync.OnceValues (initialized in newRuntimeContext). -// Falls back to direct construction for test contexts that bypass newRuntimeContext. +// Falls back to direct construction for test contexts that bypass newRuntimeContext; +// the fallback is NOT thread-safe if called concurrently. func (ctx *RuntimeContext) getAPIClient() (*client.APIClient, error) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner.go` around lines 80 - 87, The fallback in RuntimeContext.getAPIClient (when apiClientFunc == nil) calls Factory.NewAPIClientWithConfig directly and is not synchronized, so concurrent test calls can create multiple APIClient instances; update the function's comment to explicitly state this non-thread-safe fallback behavior, mention that newRuntimeContext (and sync.OnceValues) provides the thread-safe path, and advise test authors to set apiClientFunc or construct the context via newRuntimeContext if they need thread-safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@shortcuts/common/runner.go`:
- Around line 80-87: The fallback in RuntimeContext.getAPIClient (when
apiClientFunc == nil) calls Factory.NewAPIClientWithConfig directly and is not
synchronized, so concurrent test calls can create multiple APIClient instances;
update the function's comment to explicitly state this non-thread-safe fallback
behavior, mention that newRuntimeContext (and sync.OnceValues) provides the
thread-safe path, and advise test authors to set apiClientFunc or construct the
context via newRuntimeContext if they need thread-safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fe202db6-78b6-4156-80b7-8350afacd47d
📒 Files selected for processing (1)
shortcuts/common/runner.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d55afe2d39dab27a8c1600f32e75c97892a3b0f2🧩 Skill updatenpx skills add larksuite/cli#fix/runtime-context-concurrency -y -g |
Change-Id: I83bb677e9a33e906e207679b2ba8d0364bc20fe3
github-actions
bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/auth/auth_test.go`:
- Around line 239-245:
TestAuthScopesRun_UsesTenantAccessTokenFromCredentialProvider can be influenced
by a real CLI config; before creating the stub credential provider in that test,
call t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) to isolate CLI config
state so credential.NewCredentialProvider will use the injected
authScopesTokenResolver instead of any real config; place this call at the start
of TestAuthScopesRun_UsesTenantAccessTokenFromCredentialProvider just before
setting f.Credential.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a86cc5af-e5ed-462a-96f1-32493c229237
📒 Files selected for processing (2)
cmd/auth/auth.gocmd/auth/auth_test.go
2 participants
Summary
Fix RuntimeContext concurrency hazards in shortcut execution by making API client initialization and deferred jq error capture safe for concurrent access.
Changes
Test Plan
Related Issues
Summary by CodeRabbit