diff --git a/turbo/apps/api/src/lib/slack-webhook-context.ts b/turbo/apps/api/src/lib/slack-webhook-context.ts index 294bff450c8..a5ebe41bbd6 100644 --- a/turbo/apps/api/src/lib/slack-webhook-context.ts +++ b/turbo/apps/api/src/lib/slack-webhook-context.ts @@ -4,6 +4,7 @@ import { fetchSlackUserInfoMap, formatSenderBlock, type SlackUserInfo, + type SlackUserInfoResolver, } from "../signals/external/slack-message-client"; export interface SlackFile { @@ -101,6 +102,11 @@ export interface SlackConversationContextObserver { ) => void; } +interface SlackConversationContextOptions { + readonly observer?: SlackConversationContextObserver; + readonly userInfoResolver?: SlackUserInfoResolver; +} + async function fetchThreadContext( client: WebClient, channel: string, @@ -473,8 +479,9 @@ export async function fetchConversationContexts( channelId: string, threadTs: string | undefined, currentMessageTs?: string, - observer?: SlackConversationContextObserver, + options?: SlackConversationContextOptions, ): Promise<{ readonly executionContext: string }> { + const observer = options?.observer; const measureContext = async ( phase: SlackConversationContextPhase, operation: () => T | Promise, @@ -541,7 +548,11 @@ export async function fetchConversationContexts( ), }); const userInfoMap = await measureContext("user_info", async () => { - return await fetchSlackUserInfoMap(client, userInfoIds); + return await fetchSlackUserInfoMap( + client, + userInfoIds, + options?.userInfoResolver, + ); }); const { channelContextPrefix, threadExecContext } = await measureContext( "format", @@ -570,6 +581,7 @@ export async function enrichMessageContent(opts: { readonly files: readonly SlackFile[] | undefined; readonly client: WebClient; readonly userId: string; + readonly userInfoResolver?: SlackUserInfoResolver; }): Promise<{ readonly prompt: string; readonly userInfoExtras: { @@ -583,10 +595,11 @@ export async function enrichMessageContent(opts: { } const mentionedIds = extractMentionedUserIds([{ text: opts.messageContent }]); - const userInfoMap = await fetchSlackUserInfoMap(opts.client, [ - opts.userId, - ...mentionedIds, - ]); + const userInfoMap = await fetchSlackUserInfoMap( + opts.client, + [opts.userId, ...mentionedIds], + opts.userInfoResolver, + ); prompt = resolveUserMentions(prompt, userInfoMap); const currentUser = userInfoMap.get(opts.userId); diff --git a/turbo/apps/api/src/signals/external/slack-message-client.ts b/turbo/apps/api/src/signals/external/slack-message-client.ts index aaa61370d78..2620d22735a 100644 --- a/turbo/apps/api/src/signals/external/slack-message-client.ts +++ b/turbo/apps/api/src/signals/external/slack-message-client.ts @@ -6,7 +6,7 @@ import { } from "@slack/web-api"; import { optionalEnv } from "../../lib/env"; -import { settle } from "../utils"; +import { onRejection, settle } from "../utils"; type OpenDmResult = | { readonly kind: "ok"; readonly channelId: string } @@ -191,6 +191,20 @@ export interface SlackUserInfo { readonly timezone?: string; } +export interface SlackUserInfoResolverStats { + readonly requestedCount: number; + readonly cacheHitCount: number; + readonly missCount: number; + readonly inFlightHitCount: number; +} + +export interface SlackUserInfoResolver { + readonly resolveMany: ( + userIds: readonly string[], + ) => Promise>; + readonly stats: () => SlackUserInfoResolverStats; +} + export function formatSenderBlock(info: SlackUserInfo): string { const parts = [`id: ${info.id}`]; if (info.name) { @@ -231,10 +245,93 @@ async function fetchSlackUserInfo( }; } +export function createSlackUserInfoResolver( + client: WebClient, +): SlackUserInfoResolver { + const cache = new Map(); + const inFlight = new Map>(); + let requestedCount = 0; + let cacheHitCount = 0; + let missCount = 0; + let inFlightHitCount = 0; + + const startLookup = (userId: string): Promise => { + const promise = onRejection( + (async () => { + const info = await fetchSlackUserInfo(client, userId); + if (info) { + cache.set(userId, info); + } + inFlight.delete(userId); + return info; + })(), + () => { + inFlight.delete(userId); + }, + ); + inFlight.set(userId, promise); + return promise; + }; + + const resolveOne = async ( + userId: string, + ): Promise => { + const cached = cache.get(userId); + if (cached) { + cacheHitCount += 1; + return cached; + } + + const active = inFlight.get(userId); + if (active) { + inFlightHitCount += 1; + return await active; + } + + missCount += 1; + return await startLookup(userId); + }; + + return { + async resolveMany(userIds) { + const map = new Map(); + const uniqueIds = [...new Set(userIds)]; + requestedCount += uniqueIds.length; + const results = await Promise.allSettled( + uniqueIds.map(async (id) => { + const info = await resolveOne(id); + return { id, info }; + }), + ); + + for (const result of results) { + if (result.status === "fulfilled" && result.value.info) { + map.set(result.value.id, result.value.info); + } + } + + return map; + }, + stats() { + return { + requestedCount, + cacheHitCount, + missCount, + inFlightHitCount, + }; + }, + }; +} + export async function fetchSlackUserInfoMap( client: WebClient, userIds: readonly string[], + resolver?: SlackUserInfoResolver, ): Promise> { + if (resolver) { + return await resolver.resolveMany(userIds); + } + const map = new Map(); const uniqueIds = [...new Set(userIds)]; const results = await Promise.allSettled( diff --git a/turbo/apps/api/src/signals/routes/__tests__/integrations.bdd.test.ts b/turbo/apps/api/src/signals/routes/__tests__/integrations.bdd.test.ts index f173242c775..6bb2bcdb3ce 100644 --- a/turbo/apps/api/src/signals/routes/__tests__/integrations.bdd.test.ts +++ b/turbo/apps/api/src/signals/routes/__tests__/integrations.bdd.test.ts @@ -1193,6 +1193,7 @@ describe("INT-01: Slack app deep webhook flows", () => { "api_dispatch_pre_create_zero_slack_build_run_params_fetch_conversation_context_history", "api_dispatch_pre_create_zero_slack_build_run_params_fetch_conversation_context_user_info", "api_dispatch_pre_create_zero_slack_build_run_params_fetch_conversation_context_format", + "api_dispatch_pre_create_zero_slack_build_run_params_user_info_resolver", "api_dispatch_pre_create_zero_slack_build_run_params_assemble", "api_dispatch_pre_create_zero_slack_create_run", ]) { diff --git a/turbo/apps/api/src/signals/routes/__tests__/test-telegram-state.test.ts b/turbo/apps/api/src/signals/routes/__tests__/test-telegram-state.test.ts index 3feb77f1de8..feca37c7c39 100644 --- a/turbo/apps/api/src/signals/routes/__tests__/test-telegram-state.test.ts +++ b/turbo/apps/api/src/signals/routes/__tests__/test-telegram-state.test.ts @@ -1,6 +1,7 @@ import { randomUUID } from "node:crypto"; import { http, HttpResponse } from "msw"; +import { onTestFinished } from "vitest"; import type { TestSlackStateResponse } from "@vm0/api-contracts/contracts/test-slack-state"; import type { TestTelegramStateResponse, @@ -15,6 +16,7 @@ import { testSlackDispatchProbeRoutes } from "../test-slack-dispatch-probe"; import { testSlackStateRoutes } from "../test-slack-state"; import { testTelegramDispatchProbeRoutes } from "../test-telegram-dispatch-probe"; import { testTelegramStateRoutes } from "../test-telegram-state"; +import { createDeferredPromise, settle } from "../../utils"; import { createFixtureTracker } from "./helpers/zero-route-test"; const context = testContext(); @@ -38,6 +40,17 @@ interface SlackFixture { readonly slackUserId: string; } +interface SlackUserInfoMockResponse { + readonly ok: true; + readonly user: { + readonly profile: { + readonly display_name: string; + readonly email: string; + }; + readonly tz: string; + }; +} + interface RecentRun { readonly id: string; readonly triggerSource: string | null; @@ -109,6 +122,13 @@ function sandboxOperationActionTypes( ); } +function slackUserInfoCallCount(userId: string): number { + return context.mocks.slack.users.info.mock.calls.filter((call) => { + const request = call[0]; + return isRecord(request) && request.user === userId; + }).length; +} + function mockClerkTestUser(args: { readonly userId: string; readonly orgId: string; @@ -143,6 +163,21 @@ function mockTelegramTyping(): void { ); } +function slackUserInfoResponse( + displayName = "Slack User", +): SlackUserInfoMockResponse { + return { + ok: true, + user: { + profile: { + display_name: displayName, + email: "slack@example.com", + }, + tz: "UTC", + }, + }; +} + function configureSlackDispatchMocks(): void { mockOptionalEnv("RUNNER_DEFAULT_GROUP", "vm0/test"); mockOptionalEnv("VM0_API_URL", "http://localhost:3000"); @@ -169,16 +204,7 @@ function configureSlackDispatchMocks(): void { ok: true, messages: [], }); - context.mocks.slack.users.info.mockResolvedValue({ - ok: true, - user: { - profile: { - display_name: "Slack User", - email: "slack@example.com", - }, - tz: "UTC", - }, - }); + context.mocks.slack.users.info.mockResolvedValue(slackUserInfoResponse()); } function postTelegramState(body: Record): Promise { @@ -322,11 +348,10 @@ async function dispatchTelegramMessage(args: { ).resolves.toStrictEqual({ ok: true }); } -async function dispatchSlackMessage(args: { +async function postSlackDispatchProbe(args: { readonly fixture: SlackFixture; readonly text: string; }): Promise { - configureSlackDispatchMocks(); const response = await requestApp(SLACK_DISPATCH_PROBE_ROUTE, { method: "POST", headers: { "content-type": "application/json" }, @@ -345,6 +370,13 @@ async function dispatchSlackMessage(args: { ).resolves.toStrictEqual({ ok: true }); } +async function dispatchSlackMessage(args: { + readonly fixture: SlackFixture; + readonly text: string; +}): Promise { + configureSlackDispatchMocks(); + await postSlackDispatchProbe(args); +} describe("GET /api/test/telegram-state", () => { it("returns 404 when the test endpoint is not allowed", async () => { mockEnv("ENV", "production"); @@ -731,6 +763,7 @@ describe("POST /api/test/telegram-state", () => { "api_dispatch_pre_create_zero_slack_build_run_params_fetch_conversation_context_history", "api_dispatch_pre_create_zero_slack_build_run_params_fetch_conversation_context_user_info", "api_dispatch_pre_create_zero_slack_build_run_params_fetch_conversation_context_format", + "api_dispatch_pre_create_zero_slack_build_run_params_user_info_resolver", "api_dispatch_pre_create_zero_slack_build_run_params_assemble", "api_dispatch_pre_create_zero_slack_create_run", ]) { @@ -773,6 +806,143 @@ describe("POST /api/test/telegram-state", () => { expect(serializedTimingEvents).not.toContain(forbiddenValue); } }); + + it("coalesces overlapping Slack user info lookups during run creation", async () => { + mockEnv("ENV", "development"); + const userId = uniqueId("user"); + const orgId = uniqueId("org"); + const email = `${randomUUID()}@example.test`; + const slack = await seedSlackFixture({ userId, orgId, email }); + const prompt = "slack user info coalescing"; + const contextMentionedUserId = "U_CONTEXT_MENTIONED"; + configureSlackDispatchMocks(); + const historyResponse = createDeferredPromise<{ + readonly ok: true; + readonly messages: readonly Record[]; + }>(context.signal); + const currentUserInfoResponse = + createDeferredPromise(context.signal); + const releasePendingMocks = (): void => { + if (!historyResponse.settled()) { + historyResponse.resolve({ ok: true, messages: [] }); + } + if (!currentUserInfoResponse.settled()) { + currentUserInfoResponse.resolve(slackUserInfoResponse()); + } + }; + context.mocks.slack.conversations.history.mockReturnValue( + historyResponse.promise, + ); + context.mocks.slack.users.info.mockImplementation( + async (request: unknown) => { + const requestedUserId = + isRecord(request) && typeof request.user === "string" + ? request.user + : ""; + if (requestedUserId === slack.slackUserId) { + return await currentUserInfoResponse.promise; + } + return slackUserInfoResponse(`Slack User ${requestedUserId}`); + }, + ); + + const responsePromise = (async () => { + await postSlackDispatchProbe({ fixture: slack, text: prompt }); + })(); + onTestFinished(async () => { + releasePendingMocks(); + await settle(responsePromise); + }); + + await expect + .poll(() => { + return slackUserInfoCallCount(slack.slackUserId); + }) + .toBe(1); + await expect + .poll(() => { + return context.mocks.slack.conversations.history.mock.calls.length; + }) + .toBe(1); + historyResponse.resolve({ + ok: true, + messages: [ + { + user: slack.slackUserId, + text: `previous context from the same Slack user and <@${contextMentionedUserId}>`, + ts: "1709999999.000000", + }, + ], + }); + await expect + .poll(() => { + return slackUserInfoCallCount(contextMentionedUserId); + }) + .toBe(1); + expect(slackUserInfoCallCount(slack.slackUserId)).toBe(1); + + currentUserInfoResponse.resolve(slackUserInfoResponse()); + await responsePromise; + + expect(slackUserInfoCallCount(slack.slackUserId)).toBe(1); + const slackState = await readSlackState(slack.teamId); + const run = recentRuns(slackState.recent_runs).find((candidate) => { + return candidate.promptPreview === prompt; + }); + if (!run) { + throw new Error("Expected Slack dispatch probe to create a run"); + } + const timingEvents = sandboxOperationEventsForRun(run.id); + expect(timingEvents).toStrictEqual( + expect.arrayContaining([ + expect.objectContaining({ + op_type: + "api_dispatch_pre_create_zero_slack_build_run_params_user_info_resolver", + slack_user_info_resolver_requested_count_bucket: "2_4", + slack_user_info_resolver_cache_hit_count_bucket: "0", + slack_user_info_resolver_miss_count_bucket: "2_4", + slack_user_info_resolver_in_flight_hit_count_bucket: "1", + }), + ]), + ); + expect(JSON.stringify(timingEvents)).not.toContain(contextMentionedUserId); + }); + + it("keeps creating Slack runs when Slack user info lookup rejects", async () => { + mockEnv("ENV", "development"); + const userId = uniqueId("user"); + const orgId = uniqueId("org"); + const email = `${randomUUID()}@example.test`; + const slack = await seedSlackFixture({ userId, orgId, email }); + const prompt = "slack user info failure should not fail run creation"; + configureSlackDispatchMocks(); + context.mocks.slack.conversations.history.mockResolvedValue({ + ok: true, + messages: [ + { + user: slack.slackUserId, + text: "context from the same Slack user", + ts: "1709999999.000000", + }, + ], + }); + context.mocks.slack.users.info.mockRejectedValue( + new Error("Slack users.info failed"), + ); + + await postSlackDispatchProbe({ fixture: slack, text: prompt }); + + expect(slackUserInfoCallCount(slack.slackUserId)).toBeGreaterThan(0); + const slackState = await readSlackState(slack.teamId); + expect(recentRuns(slackState.recent_runs)).toStrictEqual( + expect.arrayContaining([ + expect.objectContaining({ + triggerSource: "slack", + promptPreview: prompt, + }), + ]), + ); + }); }); describe("DELETE /api/test/telegram-state", () => { diff --git a/turbo/apps/api/src/signals/services/api-dispatch-timing.service.ts b/turbo/apps/api/src/signals/services/api-dispatch-timing.service.ts index af5ec5418c3..77f349dfc39 100644 --- a/turbo/apps/api/src/signals/services/api-dispatch-timing.service.ts +++ b/turbo/apps/api/src/signals/services/api-dispatch-timing.service.ts @@ -49,6 +49,7 @@ export type ApiDispatchTimingActionType = | "api_dispatch_pre_create_zero_slack_build_run_params_fetch_conversation_context_history" | "api_dispatch_pre_create_zero_slack_build_run_params_fetch_conversation_context_user_info" | "api_dispatch_pre_create_zero_slack_build_run_params_fetch_conversation_context_format" + | "api_dispatch_pre_create_zero_slack_build_run_params_user_info_resolver" | "api_dispatch_pre_create_zero_slack_build_run_params_assemble" | "api_dispatch_pre_create_zero_slack_create_run" | "api_dispatch_pre_create_zero_workflow_trigger_entrypoint_gap" diff --git a/turbo/apps/api/src/signals/services/zero-slack-webhooks.service.ts b/turbo/apps/api/src/signals/services/zero-slack-webhooks.service.ts index ea496fff379..d464f4115d2 100644 --- a/turbo/apps/api/src/signals/services/zero-slack-webhooks.service.ts +++ b/turbo/apps/api/src/signals/services/zero-slack-webhooks.service.ts @@ -61,10 +61,12 @@ import { request$ } from "../context/hono"; import { waitUntil } from "../context/wait-until"; import { createSlackClient, + createSlackUserInfoResolver, openView, postMessage, publishAppHome, setThreadStatus, + type SlackUserInfoResolverStats, } from "../external/slack-message-client"; import { now, nowDate } from "../external/time"; import { writeDb$, type Db } from "../external/db"; @@ -1417,6 +1419,40 @@ const SLACK_CONVERSATION_CONTEXT_PHASE_ACTION_TYPES = { ApiDispatchTimingActionType >; +function slackUserInfoResolverCountBucket(count: number): string { + if (count <= 0) { + return "0"; + } + if (count === 1) { + return "1"; + } + if (count <= 4) { + return "2_4"; + } + if (count <= 8) { + return "5_8"; + } + if (count <= 16) { + return "9_16"; + } + return "17_plus"; +} + +function slackUserInfoResolverDimensions( + stats: SlackUserInfoResolverStats, +): ApiDispatchTimingDimensions { + return { + slack_user_info_resolver_requested_count_bucket: + slackUserInfoResolverCountBucket(stats.requestedCount), + slack_user_info_resolver_cache_hit_count_bucket: + slackUserInfoResolverCountBucket(stats.cacheHitCount), + slack_user_info_resolver_miss_count_bucket: + slackUserInfoResolverCountBucket(stats.missCount), + slack_user_info_resolver_in_flight_hit_count_bucket: + slackUserInfoResolverCountBucket(stats.inFlightHitCount), + }; +} + function createSlackConversationContextObserver( timing: ApiDispatchTimingCollector, ): { @@ -1818,6 +1854,7 @@ const buildRunAgentParams$ = command( const conversationContextObserver = createSlackConversationContextObserver( args.timing, ); + const userInfoResolver = createSlackUserInfoResolver(resolved.client); const { inputs, threadContext } = await loadSlackRunParamBundle({ timing: args.timing, db: args.db, @@ -1830,6 +1867,7 @@ const buildRunAgentParams$ = command( files: args.files, client: resolved.client, userId: args.slackUserId, + userInfoResolver, }); }, resolveModelRoute: async () => { @@ -1856,12 +1894,24 @@ const buildRunAgentParams$ = command( args.channelId, args.threadTs, args.messageTs, - conversationContextObserver.observer, + { + observer: conversationContextObserver.observer, + userInfoResolver, + }, ); }, fetchConversationContextDimensions: conversationContextObserver.dimensions, }); + await measureApiDispatchTiming( + args.timing, + "api_dispatch_pre_create_zero_slack_build_run_params_user_info_resolver", + "nested", + () => { + return undefined; + }, + slackUserInfoResolverDimensions(userInfoResolver.stats()), + ); return await measureApiDispatchTiming( args.timing,