fix: resolve secret refs created after store startup - #2459
Conversation
📝 WalkthroughWalkthrough
ChangesSecret reference lookup
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant SecretStore
participant byRef
participant SecretCollection
SecretStore->>byRef: Look up workspace/ref key
SecretStore->>SecretCollection: Validate cached secret with GetByID
SecretStore->>SecretCollection: Scan persisted records on cache miss
SecretStore->>byRef: Store discovered secret ID
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/persis/store/secret.go (1)
405-420: 🚀 Performance & Scalability | 🔵 TrivialMonitor the full-collection miss path.
findByRefdecodes every persisted record for each unresolved reference. This is acceptable as a fallback, but can become an O(N) latency/CPU hotspot for large collections or repeated unknown-reference lookups. Add metrics around this path and consider a persisted lookup index if secret volume grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/persis/store/secret.go` around lines 405 - 420, Instrument the full-collection fallback in SecretStore.findByRef, recording metrics when listAll succeeds but no matching secret is found, including collection-scan activity and unresolved-reference outcomes. Keep the existing lookup behavior unchanged, and use the repository’s established metrics conventions; leave persisted lookup-index work out of this change unless an existing index mechanism is already available.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/persis/store/secret.go`:
- Around line 173-179: Update the cache write in the method containing findByRef
so it does not overwrite a mapping created while the scan was in progress: under
s.mu, check whether s.byRef[rk] is still absent before assigning sec.ID.
Preserve concurrent newer mappings and keep the existing lookup error handling
unchanged.
---
Nitpick comments:
In `@internal/persis/store/secret.go`:
- Around line 405-420: Instrument the full-collection fallback in
SecretStore.findByRef, recording metrics when listAll succeeds but no matching
secret is found, including collection-scan activity and unresolved-reference
outcomes. Keep the existing lookup behavior unchanged, and use the repository’s
established metrics conventions; leave persisted lookup-index work out of this
change unless an existing index mechanism is already available.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 51f6ad1d-7988-4107-9587-c54b8845cf01
📒 Files selected for processing (2)
internal/persis/store/secret.gointernal/persis/store/secret_test.go
| sec, err := s.findByRef(ctx, workspace, ref) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return s.GetByID(ctx, id) | ||
| s.mu.Lock() | ||
| s.byRef[rk] = sec.ID | ||
| s.mu.Unlock() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not overwrite a newer reference mapping after the persistence scan.
findByRef runs outside s.mu, so a concurrent write can install a newer byRef[rk] while the scan is in progress. The unconditional assignment at Line 178 can then poison the cache with an older or deleted secret ID, causing later lookups to return stale data.
Install the result only if the key is still absent, or use a generation/compare-and-swap approach.
Proposed minimal fix
s.mu.Lock()
-s.byRef[rk] = sec.ID
+if _, exists := s.byRef[rk]; !exists {
+ s.byRef[rk] = sec.ID
+}
s.mu.Unlock()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| sec, err := s.findByRef(ctx, workspace, ref) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return s.GetByID(ctx, id) | |
| s.mu.Lock() | |
| s.byRef[rk] = sec.ID | |
| s.mu.Unlock() | |
| sec, err := s.findByRef(ctx, workspace, ref) | |
| if err != nil { | |
| return nil, err | |
| } | |
| s.mu.Lock() | |
| if _, exists := s.byRef[rk]; !exists { | |
| s.byRef[rk] = sec.ID | |
| } | |
| s.mu.Unlock() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/persis/store/secret.go` around lines 173 - 179, Update the cache
write in the method containing findByRef so it does not overwrite a mapping
created while the scan was in progress: under s.mu, check whether s.byRef[rk] is
still absent before assigning sec.ID. Preserve concurrent newer mappings and
keep the existing lookup error handling unchanged.
Summary
Root cause
The frontend and coordinator construct separate
SecretStoreinstances over the same file-backed collection. Each instance has its own in-memory ref index, so a coordinator that started before a frontend-created secret treated an index miss as authoritative even though the secret was already persisted.Impact
A running coordinator can resolve managed secrets created through the frontend/API without requiring a restart.
Testing
make fmtmake test TEST_TARGET="./internal/persis/store ./internal/service/coordinator"Closes #2458
Summary by cubic
Fixes secret ref lookups when multiple
SecretStoreinstances share the same persisted collection. Coordinators now resolve secrets created after startup without a restart, while preserving concurrent ref index updates.GetByRef, validate and evict stalebyRefentries before returning.findByRef, then conditionally add tobyRefto avoid clobbering concurrent mappings.SecretStoreinstances.Written for commit 8b3cd39. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests