fix(invocation-plane): guard bearer credentials Close with sync.Once#303
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughBearer credential shutdown in the gRPC proxy and rate limiter now uses ChangesBearer credential close safety
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Comment |
BearerTokenCredentials.Close closed watcherDone unconditionally, so a second Close call panicked with close of closed channel. Guard the close with sync.Once in both the grpc-proxy and ratelimiter copies and add a double-Close test to each package. Closes NVIDIA#301 Signed-off-by: mesutoezdil <mesudozdil@gmail.com>
404cf85 to
c334d89
Compare
balajinvda
left a comment
There was a problem hiding this comment.
LGTM — sync.Once guard correctly fixes the double-Close panic (#301); tests added in both credential packages. CI green.
|
🎉 This PR is included in version nvcf-grpc-proxy-v1.30.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version nvcf-ratelimiter-v1.15.6 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
TL;DR
BearerTokenCredentials.Close closed watcherDone unconditionally, so a second Close panicked with close of closed channel. Added a sync.Once guard to both the grpc-proxy and ratelimiter copies plus a TestCloseTwice in each package. go build and go test pass; the new tests panic without the fix.
Issues
Closes #301
Summary by CodeRabbit
Bug Fixes
Tests