fix(slack): refresh identity caches on user_change/team_join events - #637
Open
yzxcj797 wants to merge 3 commits into
Open
fix(slack): refresh identity caches on user_change/team_join events#637yzxcj797 wants to merge 3 commits into
yzxcj797 wants to merge 3 commits into
Conversation
In email identity mode (the default posture of a standard deployment — the CLI scaffold hardcodes SLACK_IDENTITY_EMAIL=1 for Fly and AWS), an own-team member whose email the directory can't currently see was classified isExternalGuest — an org owner with a just-set email masqueraded as an external guest for the ~39-minute Slack propagation window, and the refusal copy, logs, and audit all said "external" when the truth was "own-team, principal unresolved" (yc-software#626). classifyUser now keeps the third state distinct: email-less members with no Slack-side external evidence carry principalUnresolved (native flags still decide isExternalGuest, and email keying is unchanged — a restricted member with a visible email remains an email-keyed guest). The turn-handler gate refuses principalUnresolved members ahead of the external-guest gate — equally fail-closed, never a fallback Slack-ID principal — but with its own user-facing copy (names the directory refresh, tells the sender to retry) and its own log line. The external-guest refusal path now logs on success too: the incident in yc-software#626 was undiagnosable precisely because a successful refusal left no trace, leaving H1/H2/H3 indistinguishable from the record. Not included: preserving users.info ok:false at callers (defect 1b) and user_change/team_join event-driven refresh (defect 3) — separate follow-ups per the issue's fix ladder. Fixes defects 1 and 2 of yc-software#626
A transient users.info failure returned
{ actor: { isExternalGuest: true }, ok: false } — the failure flag
existed, but classifyActor stripped it (returning only .actor) and the
direct-intake path read classifyUserCached and ignored .ok too, so a
single failed lookup read to every consumer as a confident "this user
is an external guest" and refused the member outright. In the yc-software#626
incident an org owner was refused on exactly that shape.
- classifyUserCached's failure arm now returns a lookupFailed actor
(the id-based placeholder, named for what it is) instead of
asserting external guest — the failure state is distinguishable from
every checked state (guest, internal, principal-unresolved).
- classifyActor returns ActorAssertion & { ok } — the flag rides along
instead of being discarded; consumers decide what a failed lookup
means. The roster path already consumed ok (resolveChannelMembership
sets complete=false); the two remaining discard sites no longer can.
- The direct-intake path (turn-handler) acts on it: fall back to the
directory's last-known classification when the cache holds one
(time-bounded by the cache TTL — failures are never cached, so the
fallback is always a prior success), log the fallback, and with no
prior state refuse as principalUnresolved — the third-state path
from the stacked commit — never a laundered "external".
Stacked on the third-state commit (principalUnresolved + refusal
gates); together they cover yc-software#626's defect 1 including its 1b
sub-defect. The event-driven refresh (defect 3) remains a follow-up.
Fixes the ok-propagation half of yc-software#626 (defect 1b)
Classification updates previously waited out the 5-minute snapshot and lookup TTLs: a just-set email kept an own-team member refused for the directory-propagation window (the yc-software#626 incident's ~39 minutes), and a profile change showed stale names until the TTL lapsed. Neither user_change nor team_join was subscribed (absent from the manifest's bot_events) nor handled (yc-software#626 defect 3). The manifest now subscribes both events, and the handlers apply the event payload through Directory.updateUserFromEvent — the event carries the full fresh profile, so the handler re-classifies from it directly (no users.info round-trip) and overwrites BOTH caches the classifier reads: the LRU lookup entry (the transient path, including the lastKnownClassification fallback) and the snapshot row (the roster/membership path). Both must move together or a change shows up in one surface and not the other. Stacked on the third-state (yc-software#628) and ok-propagation (yc-software#629) commits; together the three close yc-software#626 completely. Fixes defect 3 of yc-software#626
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
1 participant
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.
Fixes defect 3 of #626 — the last of the three. Stacked on #628 (third state) and #629 (ok-propagation); the three together close #626 completely.
The gap
Classification updates only on the 5-minute snapshot/lookup TTLs. Neither
user_changenorteam_joinappears in the manifest'sbot_eventsor anywhere insrc/slack— so a just-set email kept an own-team member refused for the directory-propagation window (the incident's ~39 minutes was Slack-side propagation PLUS this TTL floor), and a profile change showed stale display names until a TTL lapsed.The fix
Manifest:
user_changeandteam_joinadded tobot_events. (Note for operators: the app needs a reinstall to receive the new subscriptions — the same README note the issue cites for scope changes.)Handlers (events.ts): both route the event payload into the new
Directory.updateUserFromEvent(user)— the payload carries the full fresh profile, so the handler re-classifies from it directly (nousers.inforound-trip) and overwrites both caches the classifier reads:classifyUserCachedpath, including thelastKnownClassificationfallback fix(slack): preserve users.info failures instead of laundering them #629 added;Both must move together or a change shows up in one surface and not the other until the TTLs lapse — exactly the split-state drift the issue's H1/H2/H3 diagnosis was blinded by.
Eviction (deleting the row) was deliberately not used: the event already holds the authoritative profile, so re-classifying from it is both immediate and cheaper than delete-then-refetch.
Test
updateUserFromEvent propagates a fresh profile into both caches (#626)— email-mode fixture starting in the incident's exact shape (email-less member →principalUnresolved: true), then auser_changecarrying the just-set email: the classification resolves immediately (externalIdkeyed on the fresh email), and thelastKnownClassificationfallback serves the updated state, not the stale unresolved one. Fails on the base (no such method; the TTL-only path keeps the member unresolved).Full identity suites: 30/30 on this stack; the suites' prior 120-test combined run stays 120/120 (
tsc --noEmitclean).Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.