Skip to content

Commit b44596f

Browse files
authored
fix(knowledge): treat Google Workspace users without Gmail or Calendar as out of scope, not listing failures (#8168)
* fix(knowledge): treat Google Workspace users without Gmail or Calendar as out of scope, not listing failures Admin-mode Gmail recorded a Directory user without a mailbox, and Calendar recorded a 403 notACalendarUser, as per-user listing failures. Both are standing account properties, so the connector stayed partial, deletion reconciliation never ran, and the scheduler re-probed the same accounts every retry window. - Directory enumeration no longer schedules Gmail users whose mailbox is not set up; the hourly Directory refresh picks them up once provisioned. A partition queued earlier completes with an empty page instead of a failure. - A Calendar 403 whose only reason is notACalendarUser completes the user's partition cleanly and re-probes it no sooner than the Directory refresh (permissions keep their own refresh cadence). Bare forbidden and mixed-reason 403s stay retryable failures. * fix(knowledge): keep a Google Workspace user's visible documents until a missing service outlasts propagation Google applies service and organizational-unit changes within 24 hours, so a missing mailbox or notACalendarUser can be transient for a user whose documents are already indexed, and a mid-listing answer does not prove the whole account lacks the service. - The scheduler skips a service-not-enabled user only on their first provider page, and only when readers see none of their documents or the condition was first observed at least 24 hours ago. Otherwise it is a retained failure, as before, whose first observation is kept in the partition failure; after 24 hours a mid-listing user restarts from their first page. - Permission passes skip on the first page, since a retained failure refreshes nothing. - Gmail Directory enumeration keeps scheduling a user without a mailbox while readers still see their mail; the crawl reports the missing mailbox to the scheduler instead of completing the user. - Visibility is read through doc_acl_gin_idx for the user's token behind an OFFSET 0 fence, bounded by that user's grants. * fix(knowledge): read a user's visible documents from the ACL index and cover the probe in PostgreSQL The visibility probe moves to its own module so it can run against a real database. Planned inline, LIMIT 1 made a sequential scan of the document table look cheaper than doc_acl_gin_idx, because PostgreSQL cannot estimate array overlap. A materialized CTE now reads the user's grants from the index first, bounding the probe by that user's grants. The email goes through userToken, so a mixed-case or padded directory address matches the normalized ACL token. The new PostgreSQL integration test covers fresh, stale and missing permission evidence, another user's grant, another connector, excluded, archived and deleted documents, and email normalization, and runs in the Search progress PostgreSQL CI step.
1 parent aaca125 commit b44596f

12 files changed

Lines changed: 456 additions & 74 deletions

‎.github/workflows/test-build.yml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -271,6 +271,7 @@ jobs:
271271
lib/knowledge/__integration__/connector-deferral.integration.ts
272272
lib/knowledge/__integration__/stored-document-recovery.integration.ts
273273
lib/knowledge/__integration__/connector-partition-work.integration.ts
274+
lib/knowledge/__integration__/user-document-visibility.integration.ts
274275
lib/knowledge/__integration__/listing-continuation.integration.ts
275276
lib/knowledge/__integration__/member-scope-renewal.integration.ts
276277
lib/knowledge/__integration__/slack-empty-threads.integration.ts

‎apps/sim/connectors/google-workspace/company-crawl.test.ts‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,11 @@
22
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
33
import { GoogleApiError, readGoogleApiError } from '@/connectors/google-workspace/api-errors'
44
import {
5+
GoogleWorkspaceMailboxNotSetup,
56
getGoogleWorkspaceDocument,
67
InvalidGoogleWorkspaceCursor,
78
listGoogleWorkspaceDocuments,
9+
serviceNotEnabledFailure,
810
validateGoogleWorkspaceConfig,
911
} from '@/connectors/google-workspace/company-crawl'
1012
import type { ConnectorConfig, ExternalDocument } from '@/connectors/types'
@@ -483,44 +485,52 @@ describe('Google Workspace per-user central crawl', () => {
483485
}
484486
)
485487

486-
it('skips an explicitly unprovisioned Gmail mailbox before requesting a token', async () => {
488+
it('leaves an explicitly unprovisioned Gmail mailbox to the scheduler before requesting a token', async () => {
487489
directory([USER('alice', undefined, { isMailboxSetup: false }), USER('bob')])
488-
const ctx = context()
489-
const first = await list(ctx)
490-
expect(first.listingFailures?.samples[0]).toEqual({
491-
scope: 'alice@corp.com',
490+
const ctx: Record<string, unknown> = context()
491+
const error = await list(ctx).catch((caught: unknown) => caught)
492+
expect(error).toBeInstanceOf(GoogleWorkspaceMailboxNotSetup)
493+
expect(serviceNotEnabledFailure(error)).toEqual({
492494
operation: 'directory.users.get',
493495
reasons: ['mailboxNotSetup'],
494496
})
497+
expect(ctx.reconciliationUnsafe).toBeUndefined()
495498
expect(ctx.getDelegatedAccessToken).not.toHaveBeenCalled()
496-
const second = await list(context(), first.nextCursor)
497-
expect(second.documents[0].acl).toEqual(['u:bob@corp.com'])
499+
expect(listUserDocuments).not.toHaveBeenCalled()
498500
})
499501

500502
it('does not use Gmail mailbox eligibility for Calendar', async () => {
501503
directory([USER('alice', undefined, { isMailboxSetup: false })])
502504
expect((await list(context(), undefined, CONFIG, 'google_calendar')).documents).toHaveLength(1)
503505
})
504506

505-
it.each(['forbidden', 'notACalendarUser'])(
506-
'isolates explicit Calendar list access failures (%s) without claiming a disabled service',
507-
async (reason) => {
507+
it.each([{ reasons: ['forbidden'] }, { reasons: ['forbidden', 'notACalendarUser'] }])(
508+
'isolates explicit Calendar list access failures ($reasons) without claiming a disabled service',
509+
async ({ reasons }) => {
508510
listUserDocuments.mockRejectedValueOnce(
509-
new GoogleApiError('calendar.events.list', 403, [reason])
511+
new GoogleApiError('calendar.events.list', 403, reasons)
510512
)
511513
const first = await list(context(), undefined, CONFIG, 'google_calendar')
512514
expect(first.listingFailures?.samples[0]).toEqual({
513515
scope: 'alice@corp.com',
514516
operation: 'calendar.events.list',
515517
status: 403,
516-
reasons: [reason],
518+
reasons,
517519
})
518520
const second = await list(context(), first.nextCursor, CONFIG, 'google_calendar')
519521
expect(second.documents[0].acl).toEqual(['u:bob@corp.com'])
520522
expect(second.reconciliationSafe).toBe(false)
521523
}
522524
)
523525

526+
it('leaves a user without the Calendar service to the scheduler instead of recording a failure', async () => {
527+
const error = new GoogleApiError('calendar.events.list', 403, ['notACalendarUser'])
528+
listUserDocuments.mockRejectedValueOnce(error)
529+
const ctx: Record<string, unknown> = context()
530+
await expect(list(ctx, undefined, CONFIG, 'google_calendar')).rejects.toBe(error)
531+
expect(ctx.reconciliationUnsafe).toBeUndefined()
532+
})
533+
524534
it.each([{ error: { code: 403 } }, { error: { code: 403, errors: [], details: [] } }])(
525535
'propagates a Calendar 403 without reason codes: %j',
526536
async (body) => {
@@ -626,10 +636,9 @@ describe('Google Workspace per-user central crawl', () => {
626636
})
627637

628638
it('bounds retained failure samples while counting every unavailable user', async () => {
629-
directory(
630-
Array.from({ length: 15 }, (_, index) =>
631-
USER(`user-${index}`, undefined, { isMailboxSetup: false })
632-
)
639+
directory(Array.from({ length: 15 }, (_, index) => USER(`user-${index}`)))
640+
listUserDocuments.mockRejectedValue(
641+
new GoogleApiError('gmail.threads.list', 400, ['failedPrecondition'])
633642
)
634643
let cursor: string | undefined
635644
let final
@@ -643,7 +652,7 @@ describe('Google Workspace per-user central crawl', () => {
643652
reconciliationSafe: false,
644653
})
645654
expect(final?.listingFailures?.samples).toHaveLength(10)
646-
expect(listUserDocuments).not.toHaveBeenCalled()
655+
expect(listUserDocuments).toHaveBeenCalledTimes(15)
647656
})
648657
})
649658

‎apps/sim/connectors/google-workspace/company-crawl.ts‎

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -161,23 +161,52 @@ function ownerDocument(document: ExternalDocument, access: DelegatedUser): Exter
161161
return { ...document, acl: [`u:${access.user.email}`] }
162162
}
163163

164-
/** Isolates narrow user-list failures; delegation, known scope errors and quota errors still fail. */
164+
/** A Directory user without a Gmail mailbox; the user scheduler decides whether to skip them. */
165+
export class GoogleWorkspaceMailboxNotSetup extends Error {
166+
constructor() {
167+
super('Google Workspace user has no Gmail mailbox')
168+
this.name = 'GoogleWorkspaceMailboxNotSetup'
169+
}
170+
}
171+
172+
/**
173+
* Evidence that a user lacks the service itself: no Gmail mailbox, or a Calendar list 403 whose
174+
* only reason is `notACalendarUser`. Admin changes to a user's services can take up to a day to
175+
* settle, so the user scheduler, which can see indexed documents, decides when this is a skip.
176+
*/
177+
export function serviceNotEnabledFailure(
178+
error: unknown
179+
): Omit<ExternalListingFailures['samples'][number], 'scope'> | null {
180+
if (error instanceof GoogleWorkspaceMailboxNotSetup)
181+
return { operation: 'directory.users.get', reasons: ['mailboxNotSetup'] }
182+
return error instanceof GoogleApiError &&
183+
error.reasonsComplete &&
184+
error.status === 403 &&
185+
error.diagnostic?.operation === 'calendar.events.list' &&
186+
error.diagnostic.reasons.length === 1 &&
187+
error.diagnostic.reasons[0] === 'notACalendarUser'
188+
? { operation: 'calendar.events.list', status: 403, reasons: ['notACalendarUser'] }
189+
: null
190+
}
191+
192+
/** Isolates narrow user-list failures; a missing Calendar service propagates to the scheduler. */
165193
function userListingFailure(
166194
error: unknown,
167195
provider: GoogleWorkspaceProvider
168196
): Omit<ExternalListingFailures['samples'][number], 'scope'> | null {
169197
if (!(error instanceof GoogleApiError) || !error.diagnostic || !error.reasonsComplete) return null
170198
const reasons = error.diagnostic.reasons
171199
const isolated =
172-
provider === 'gmail'
200+
!serviceNotEnabledFailure(error) &&
201+
(provider === 'gmail'
173202
? error.diagnostic.operation === 'gmail.threads.list' &&
174203
error.status === 400 &&
175204
reasons.length > 0 &&
176205
reasons.every((reason) => reason === 'failedPrecondition')
177206
: error.diagnostic.operation === 'calendar.events.list' &&
178207
error.status === 403 &&
179208
reasons.length > 0 &&
180-
reasons.every((reason) => reason === 'forbidden' || reason === 'notACalendarUser')
209+
reasons.every((reason) => reason === 'forbidden' || reason === 'notACalendarUser'))
181210
return isolated
182211
? { operation: error.diagnostic.operation, status: error.status, reasons: [...reasons] }
183212
: null
@@ -317,9 +346,8 @@ export async function listGoogleWorkspaceDocuments(
317346
})
318347
return emptyPage(advance())
319348
}
320-
if (provider === 'gmail' && user.isMailboxSetup === false) {
321-
return failedUser({ operation: 'directory.users.get', reasons: ['mailboxNotSetup'] })
322-
}
349+
if (provider === 'gmail' && user.isMailboxSetup === false)
350+
throw new GoogleWorkspaceMailboxNotSetup()
323351
const access: PageAccess = {
324352
provider,
325353
user,

‎apps/sim/connectors/listing-failures.ts‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,17 +4,17 @@ import { CONNECTOR_SOURCE_REASON_STATES } from '@/connectors/source-error'
44
export const MAX_LISTING_FAILURE_SAMPLES = 10
55

66
/** Persist only bounded scope identifiers and provider-owned codes, never raw errors or content. */
7+
export const listingFailureSampleSchema = z.object({
8+
scope: z.string().min(1).max(254),
9+
operation: z.string().min(1).max(96),
10+
status: z.number().int().min(100).max(599).optional(),
11+
reasons: z.array(z.string().min(1).max(64)).max(16),
12+
reasonState: z.enum(CONNECTOR_SOURCE_REASON_STATES).optional(),
13+
/** When a standing account condition was first observed, bounding how long it is retained. */
14+
since: z.string().datetime().optional(),
15+
})
16+
717
export const listingFailuresSchema = z.object({
818
count: z.number().int().positive().max(Number.MAX_SAFE_INTEGER),
9-
samples: z
10-
.array(
11-
z.object({
12-
scope: z.string().min(1).max(254),
13-
operation: z.string().min(1).max(96),
14-
status: z.number().int().min(100).max(599).optional(),
15-
reasons: z.array(z.string().min(1).max(64)).max(16),
16-
reasonState: z.enum(CONNECTOR_SOURCE_REASON_STATES).optional(),
17-
})
18-
)
19-
.max(MAX_LISTING_FAILURE_SAMPLES),
19+
samples: z.array(listingFailureSampleSchema).max(MAX_LISTING_FAILURE_SAMPLES),
2020
})

‎apps/sim/connectors/types.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,6 +205,8 @@ export interface ExternalListingFailures {
205205
status?: number
206206
reasons: string[]
207207
reasonState?: ConnectorSourceReasonState
208+
/** When a standing account condition was first observed, bounding how long it is retained. */
209+
since?: string
208210
}[]
209211
}
210212

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
/** Real PostgreSQL coverage for the probe that keeps a user's visible documents from being skipped. */
2+
import { db } from '@sim/db'
3+
import { document, knowledgeConnector, organization, user, workspace } from '@sim/db/schema'
4+
import { generateId } from '@sim/utils/id'
5+
import { eq, inArray } from 'drizzle-orm'
6+
import { afterAll, afterEach, beforeEach, describe, expect, it } from 'vitest'
7+
import { seedKnowledgeAclFixture } from '@/lib/knowledge/__integration__/seed-source-access-fixture'
8+
import { hasVisibleUserDocuments } from '@/lib/knowledge/connectors/user-document-visibility'
9+
10+
const DAY_MS = 24 * 60 * 60 * 1000
11+
12+
describe('Visible user documents in PostgreSQL', () => {
13+
let owner: Awaited<ReturnType<typeof seedKnowledgeAclFixture>>
14+
let otherConnectorId: string
15+
const email = () => `${owner.aliceId}@fixture.test`
16+
17+
beforeEach(async () => {
18+
owner = await seedKnowledgeAclFixture(undefined, { connectorType: 'google_drive' })
19+
otherConnectorId = generateId()
20+
await db.insert(knowledgeConnector).values({
21+
id: otherConnectorId,
22+
knowledgeBaseId: owner.knowledgeBaseId,
23+
connectorType: 'google_drive',
24+
sourceConfig: {},
25+
accessMode: 'admin',
26+
status: 'active',
27+
credentialId: owner.credentialId,
28+
})
29+
})
30+
afterEach(async () => {
31+
await db.delete(workspace).where(eq(workspace.id, owner.workspaceId))
32+
await db.delete(organization).where(eq(organization.id, owner.organizationId))
33+
await db.delete(user).where(inArray(user.id, [owner.aliceId, owner.bobId]))
34+
})
35+
afterAll(() => db.$client.end())
36+
37+
const insert = (overrides: Partial<typeof document.$inferInsert> = {}) =>
38+
db.insert(document).values({
39+
id: generateId(),
40+
knowledgeBaseId: owner.knowledgeBaseId,
41+
connectorId: owner.connectorId,
42+
externalId: generateId(),
43+
filename: 'Event.txt',
44+
mimeType: 'text/plain',
45+
fileUrl: '',
46+
fileSize: 0,
47+
acl: [`u:${email()}`],
48+
aclVerifiedAt: new Date(),
49+
...overrides,
50+
})
51+
52+
it('finds a live, included document granted to the user with fresh permission evidence', async () => {
53+
await insert()
54+
expect(await hasVisibleUserDocuments(owner.connectorId, email())).toBe(true)
55+
})
56+
57+
it('matches a mixed-case, padded directory email to the normalized ACL token', async () => {
58+
await insert()
59+
expect(await hasVisibleUserDocuments(owner.connectorId, ` ${email().toUpperCase()} `)).toBe(
60+
true
61+
)
62+
})
63+
64+
it.each([
65+
{
66+
label: 'permission evidence older than the freshness limit',
67+
overrides: () => ({ aclVerifiedAt: new Date(Date.now() - DAY_MS - 60_000) }),
68+
},
69+
{ label: 'no permission evidence', overrides: () => ({ aclVerifiedAt: null }) },
70+
{
71+
label: 'a grant to a different user',
72+
overrides: () => ({ acl: [`u:${owner.bobId}@fixture.test`] }),
73+
},
74+
{
75+
label: 'a document in another connector',
76+
overrides: () => ({ connectorId: otherConnectorId }),
77+
},
78+
{ label: 'a user-excluded document', overrides: () => ({ userExcluded: true }) },
79+
{ label: 'an archived document', overrides: () => ({ archivedAt: new Date() }) },
80+
{ label: 'a deleted document', overrides: () => ({ deletedAt: new Date() }) },
81+
])('ignores $label', async ({ overrides }) => {
82+
await insert(overrides())
83+
expect(await hasVisibleUserDocuments(owner.connectorId, email())).toBe(false)
84+
})
85+
86+
it('refuses an address that cannot be a user token', async () => {
87+
await insert()
88+
expect(await hasVisibleUserDocuments(owner.connectorId, ' ')).toBe(false)
89+
})
90+
})

0 commit comments

Comments
 (0)