fix: caching logic for external entities - #2155
Merged
Merged
Conversation
Contributor
WalkthroughReplaces untyped Changes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
BenchstatBase: 📊 2 minor regression(s) (all within 5% threshold)
✅ 3 improvement(s)
Full benchstat output |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
db/external_entities.go (2)
85-115:⚠️ Potential issue | 🟠 MajorPopulate caches with post-merge winner IDs.
This writes cache entries before applying
idMap, so a loser ID deleted at Line 87 can be re-added at Line 90 and aliases can point back to merged-away IDs. Cache winner IDs first, and keep loser→winner entries in the ID cache. Apply the same pattern for group/role merge maps if those merge functions can return losers.🐛 Proposed fix direction
if scraperID != nil { - for loserID := range idMap { - ExternalUserIDCache.Delete(loserID.String()) + for loserID, winnerID := range idMap { + ExternalUserIDCache.Set(loserID.String(), winnerID) } for _, u := range resolvedUsers { - ExternalUserIDCache.Set(u.ID.String(), u.ID) + winnerID := u.ID + if winner, ok := idMap[u.ID]; ok { + winnerID = winner + } + ExternalUserIDCache.Set(winnerID.String(), winnerID) for _, alias := range u.Aliases { - ExternalUserCache.Set(alias, u.ID) + ExternalUserCache.Set(alias, winnerID) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/external_entities.go` around lines 85 - 115, The code is populating caches before applying the idMap rewrites so losers get re-added or aliases point to merged-away IDs; change the order in the block that updates ExternalUserIDCache/ExternalUserCache (and the corresponding ExternalGroup*/ExternalRole* caches) to first rewrite resolvedUsers/resolvedGroups/resolvedRoles IDs using idMap (the same logic in the for i := range resolvedUsers loop) and then populate caches using the post-merge winner IDs; additionally, when processing idMap entries ensure the ID cache records loser→winner mappings (e.g., set ExternalUserIDCache.Set(loser.String(), winner)) while setting aliases to the winner ID so aliases never point to stale loser IDs (apply the same pattern for resolvedGroups/resolvedRoles and their caches).
703-719:⚠️ Potential issue | 🟠 MajorUse the merge winner when caching ensured users.
merge_and_upsert_external_usersmay merge the deterministicidinto an existing winner, but the cache still storesidfor both ID and alias lookups. That can make follow-up access log resolution use a stale loser ID.🐛 Proposed fix
if err := tx.Commit().Error; err != nil { return fmt.Errorf("failed to commit: %w", err) } - ExternalUserIDCache.Set(id.String(), id) + winnerID := id + for _, merge := range merges { + if merge.LoserID == id { + winnerID = merge.WinnerID + break + } + } + + ExternalUserIDCache.Set(id.String(), winnerID) + ExternalUserIDCache.Set(winnerID.String(), winnerID) for _, alias := range aliases { - ExternalUserCache.Set(alias, id) + ExternalUserCache.Set(alias, winnerID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/external_entities.go` around lines 703 - 719, The cache is being populated with the original deterministic id even when merge_and_upsert_external_users returned a different winner; adjust caching to use the merge winner(s) instead of the original id by inspecting the merges slice returned from tx.Raw("SELECT * FROM merge_and_upsert_external_users(?::TEXT)", tempTable) (the merges struct with LoserID and WinnerID) after commit: for any merge where LoserID == id, replace id with WinnerID (or build a mapping from loser->winner) and populate ExternalUserIDCache.Set and ExternalUserCache.Set for aliases using the final winner ID; also ensure you still populate cache entries for any losers if you want redirects (map losers to winner) so subsequent lookups resolve to the winner rather than the stale loser.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/external_cache.go`:
- Around line 222-224: aliasCache hits currently mark seen[id] directly, but
idCache stores canonical remappings (ID→winner) so a stale alias can return a
merged-away loser ID and bypass remapping; change the alias-cache branch to take
the id returned from aliasCache.Get(alias), then consult idCache (e.g.,
idCache.Get/Lookup(id)) to obtain the canonical winner ID if present, and mark
seen[winnerID] = true (not the raw alias id) before continuing so alias hits are
canonicalized through idCache.
---
Outside diff comments:
In `@db/external_entities.go`:
- Around line 85-115: The code is populating caches before applying the idMap
rewrites so losers get re-added or aliases point to merged-away IDs; change the
order in the block that updates ExternalUserIDCache/ExternalUserCache (and the
corresponding ExternalGroup*/ExternalRole* caches) to first rewrite
resolvedUsers/resolvedGroups/resolvedRoles IDs using idMap (the same logic in
the for i := range resolvedUsers loop) and then populate caches using the
post-merge winner IDs; additionally, when processing idMap entries ensure the ID
cache records loser→winner mappings (e.g., set
ExternalUserIDCache.Set(loser.String(), winner)) while setting aliases to the
winner ID so aliases never point to stale loser IDs (apply the same pattern for
resolvedGroups/resolvedRoles and their caches).
- Around line 703-719: The cache is being populated with the original
deterministic id even when merge_and_upsert_external_users returned a different
winner; adjust caching to use the merge winner(s) instead of the original id by
inspecting the merges slice returned from tx.Raw("SELECT * FROM
merge_and_upsert_external_users(?::TEXT)", tempTable) (the merges struct with
LoserID and WinnerID) after commit: for any merge where LoserID == id, replace
id with WinnerID (or build a mapping from loser->winner) and populate
ExternalUserIDCache.Set and ExternalUserCache.Set for aliases using the final
winner ID; also ensure you still populate cache entries for any losers if you
want redirects (map losers to winner) so subsequent lookups resolve to the
winner rather than the stale loser.
🪄 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
Run ID: 4ee70f00-eaf9-4e0f-9ad0-bfbcf6bd7357
📒 Files selected for processing (4)
db/external_cache.godb/external_entities.godb/external_resolver.godb/update.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@db/external_cache.go`:
- Around line 133-137: Warm-up currently writes empty alias keys into
aliasCache, causing multiple rows with empty aliases to collide; update the loop
that iterates row.Aliases (in db/external_cache.go where rows, row.Aliases are
processed and table.aliasCache.Set is called) to skip empty strings (e.g., if
alias == "" or len(alias) == 0 continue) before calling
table.aliasCache.Set(alias, row.ID) so only non-empty aliases are cached; leave
the table.idCache.Set(row.ID.String(), row.ID) behavior unchanged.
- Around line 31-49: The Set, Delete and Flush methods on typedCache[T]
currently swallow errors from c.inner (used in Set, Delete, Clear) leading to
silent failures; update these methods to surface failures by returning error
(change signatures of typedCache.Set, Delete, Flush to return error) or at
minimum log errors from c.inner.Set/ Delete/ Clear using the project's logger,
and update all callers to handle the new error return; ensure Get continues
returning (T, bool) but other methods either return error or call logger when
c.inner.* returns non-nil, referencing typedCache[T], methods Set/Delete/Flush
and field inner so you can locate and change the implementations consistently.
🪄 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
Run ID: d45e2622-a4c7-4ac8-8e7f-57978f7ef7b4
📒 Files selected for processing (1)
db/external_cache.go
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
2 participants
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 by CodeRabbit
Refactor
Bug Fixes / Performance