Skip to content

Commit ebd8efb

Browse files
authored
fix(memory): read note access from an unlocked snapshot and defer unavailable cleanup (#9102)
* fix(memory): read note access from an unlocked snapshot and defer unavailable cleanup - Resolve memory note access in a read-only repeatable-read transaction with no row or advisory locks; the shared Project helpers gain a snapshot mode and keep their hold behavior for every existing caller. - Resolve workspace access once per check and derive shared mutation from the effective permission. - Read the projects flag before the transaction. - Defer memory cleanup hourly without spending attempts when the worker has no cleanup endpoint or the processor lacks the key or destination, instead of dead-lettering. - Drop the credential and source-env equality checks; replay the enqueuing source env and accept rows queued with the retired fingerprint field. - Enqueue cleanup events in one bulk insert. * fix(memory): keep the Projects-off refusal after the chat checks and defer cleanup on pre-route 401 * refactor(memory): reuse the shared role check and one cleanup wait path * test(memory): use plain latches the app TypeScript lib supports * fix(memory): accept additive fields in note access and cleanup answers
1 parent 1469aa2 commit ebd8efb

10 files changed

Lines changed: 223 additions & 141 deletions

File tree

‎apps/sim/lib/core/application/workspace-authorization.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,10 @@ export function requireAllowedWorkspacePrincipal<O extends WorkspaceOperation>(
191191
}
192192
}
193193

194-
function requirePermission(permission: PermissionType | null, required: PermissionType): void {
194+
function requirePermission(
195+
permission: PermissionType | null,
196+
required: PermissionType
197+
): asserts permission is PermissionType {
195198
if (permission === null) {
196199
throw new NoWorkspaceAccessError()
197200
}
@@ -324,7 +327,7 @@ export async function requireCurrentHumanRole<C extends WorkspaceAuthorizationCo
324327
context: C,
325328
required: PermissionType,
326329
options?: WorkspaceAuthorizationOptions<C>
327-
): Promise<void> {
330+
): Promise<PermissionType> {
328331
const permission = await resolveEffectiveWorkspacePermission(
329332
userId,
330333
context.workspaceId,
@@ -333,6 +336,7 @@ export async function requireCurrentHumanRole<C extends WorkspaceAuthorizationCo
333336
{ forUpdate: options?.forUpdate }
334337
)
335338
requirePermission(permission, required)
339+
return permission
336340
}
337341

338342
/**

‎apps/sim/lib/mothership/generated/memory-notes-access.ts‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,13 @@
33

44
import { z } from "zod";
55

6-
const permissionsSchema = z.strictObject({
6+
// Answers tolerate fields a newer peer adds; requests stay strict.
7+
const permissionsSchema = z.object({
78
read: z.literal(true),
89
privateMutation: z.literal(true),
910
sharedMutation: z.boolean(),
1011
});
11-
const projectSchema = z.strictObject({
12+
const projectSchema = z.object({
1213
scope: z.literal("project"),
1314
id: z.uuid(),
1415
name: z.string(),
@@ -25,17 +26,17 @@ export const MemoryNotesAccessInput = z.strictObject({
2526
});
2627
export type MemoryNotesAccessInput = z.infer<typeof MemoryNotesAccessInput>;
2728

28-
export const MemoryNotesAccessResult = z.strictObject({
29+
export const MemoryNotesAccessResult = z.object({
2930
success: z.literal(true),
3031
version: z.literal(1),
31-
actor: z.strictObject({ userId: z.string().min(1), chatId: z.uuid() }),
32+
actor: z.object({ userId: z.string().min(1), chatId: z.uuid() }),
3233
target: z.discriminatedUnion("scope", [
33-
z.strictObject({
34+
z.object({
3435
scope: z.literal("personal"),
3536
permissions: permissionsSchema.extend({ sharedMutation: z.literal(false) }),
3637
}),
3738
projectSchema,
38-
z.strictObject({
39+
z.object({
3940
scope: z.literal("workspace"),
4041
id: z.uuid(),
4142
name: z.string(),
@@ -55,4 +56,4 @@ export const MemoryCleanupInput = z.strictObject({
5556
]),
5657
});
5758
export type MemoryCleanupInput = z.infer<typeof MemoryCleanupInput>;
58-
export const MemoryCleanupResult = z.strictObject({ success: z.literal(true) });
59+
export const MemoryCleanupResult = z.object({ success: z.literal(true) });

‎apps/sim/lib/mothership/memories/application/resolve-access.integration.ts‎

Lines changed: 37 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
} from '@sim/db/schema'
1313
import { deleteWorkspaceFixture, insertWorkspaceFixture } from '@sim/db/testing/workspace-fixtures'
1414
import { featureFlagsMock, featureFlagsMockFns } from '@sim/testing/mocks/feature-flags.mock'
15+
import { sleep } from '@sim/utils/helpers'
1516
import { generateId } from '@sim/utils/id'
1617
import { and, eq, inArray } from 'drizzle-orm'
1718
import { NextRequest } from 'next/server'
@@ -26,7 +27,7 @@ import {
2627
resolveMemoryNotesAccess,
2728
} from '@/lib/mothership/memories/application/resolve-access'
2829
import { executeSimControl } from '@/lib/mothership/transport/control'
29-
import { splitForkProject } from '@/lib/projects/membership'
30+
import { lockProject, splitForkProject } from '@/lib/projects/membership'
3031
import { POST } from '@/app/api/mothership/memories/access/route'
3132

3233
vi.mock('@/lib/core/config/feature-flags', () => featureFlagsMock)
@@ -255,6 +256,41 @@ it('resolves overlapping workspace checks that share one Project without deadloc
255256
expect(result.target).toMatchObject({ scope: 'workspace', currentProject: { id: projectId } })
256257
})
257258

259+
it('answers while a writer holds the account row and the Project exclusively', async () => {
260+
let release = () => {}
261+
const released = new Promise<void>((resolve) => {
262+
release = resolve
263+
})
264+
let held = () => {}
265+
const holding = new Promise<void>((resolve) => {
266+
held = resolve
267+
})
268+
const writer = db.transaction(async (tx) => {
269+
await tx.select({ id: user.id }).from(user).where(eq(user.id, actorId)).for('update')
270+
await lockProject(tx, projectId)
271+
held()
272+
await released
273+
})
274+
await holding
275+
try {
276+
for (const organizationChat of [false, true]) {
277+
const result = await Promise.race([
278+
resolveMemoryNotesAccess.execute({
279+
principal: principal(organizationChat),
280+
input: { target: { scope: 'workspace', workspaceId }, action: 'read' },
281+
}),
282+
sleep(2_000).then(() => {
283+
throw new Error('Memory access waited on a writer')
284+
}),
285+
])
286+
expect(result.target).toMatchObject({ currentProject: { id: projectId } })
287+
}
288+
} finally {
289+
release()
290+
await writer
291+
}
292+
})
293+
258294
it('does not turn write in one environment into shared Project write', async () => {
259295
await db
260296
.update(permissions)

‎apps/sim/lib/mothership/memories/application/resolve-access.ts‎

Lines changed: 35 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,12 @@
11
import type { OrganizationDelegatedPrincipal, SubjectDelegatedPrincipal } from '@sim/auth/principal'
22
import { db } from '@sim/db'
3-
import { copilotChats, workspace } from '@sim/db/schema'
3+
import { copilotChats, project, workspace } from '@sim/db/schema'
4+
import { permissionSatisfies } from '@sim/platform-authz/workspace'
45
import { and, eq, isNull } from 'drizzle-orm'
56
import { defineWorkspaceOperation } from '@/lib/core/application'
67
import { authorizeOrganizationOperation } from '@/lib/core/application/organization-authorization'
78
import { defineOrganizationOperation } from '@/lib/core/application/organization-operation'
8-
import {
9-
InsufficientWorkspacePermissionsError,
10-
requireCurrentHumanRole,
11-
} from '@/lib/core/application/workspace-authorization'
12-
import { isFeatureEnabled } from '@/lib/core/config/feature-flags'
9+
import { requireCurrentHumanRole } from '@/lib/core/application/workspace-authorization'
1310
import { OrchestrationError } from '@/lib/core/orchestration/types'
1411
import type { DbTransaction } from '@/lib/db/types'
1512
import { defineAuthorizedChatUseCase } from '@/lib/mothership/chat/application/authorized-chat-use-case'
@@ -21,15 +18,13 @@ import type {
2118
} from '@/lib/mothership/generated/memory-notes-access'
2219
import { assertWorkspaceCapability } from '@/lib/permission-groups/capability-assertions'
2320
import { PermissionGroupCapabilityError } from '@/lib/permission-groups/capability-error'
24-
import { acquirePermissionGroupOrgReadLock } from '@/lib/permission-groups/locks'
2521
import { loadProjectAccess } from '@/lib/projects/application/authorization'
2622
import {
2723
requireCurrentCopilotProjectInvocation,
2824
requireProjectFileOwnerCapabilities,
2925
requireProjectFileOwnerRole,
3026
} from '@/lib/projects/files/application/authorization'
31-
import { lockWorkspaceProject } from '@/lib/projects/membership'
32-
import { requireProjectApiEnabled } from '@/lib/projects/rollout.server'
27+
import { isProjectsEnabled, requireProjectApiEnabled } from '@/lib/projects/rollout.server'
3328
import { requireCurrentFileSubject } from '@/lib/workspace-files/application/subject'
3429

3530
export const MEMORY_NOTES_ACCESS_AUDIENCE = 'sim:memory-access'
@@ -52,6 +47,18 @@ const organizationOperation = defineOrganizationOperation({
5247
delegationAudience: MEMORY_NOTES_ACCESS_AUDIENCE,
5348
})
5449
type MemoryPrincipal = SubjectDelegatedPrincipal | OrganizationDelegatedPrincipal
50+
/** The worker acts on the answer after commit, so a consistent unlocked snapshot is enough. */
51+
const READ_SNAPSHOT = { isolationLevel: 'repeatable read', accessMode: 'read only' } as const
52+
53+
async function workspaceProject(tx: DbTransaction, workspaceId: string) {
54+
const [row] = await tx
55+
.select({ id: project.id, organizationId: project.organizationId })
56+
.from(workspace)
57+
.innerJoin(project, eq(project.id, workspace.projectId))
58+
.where(eq(workspace.id, workspaceId))
59+
.limit(1)
60+
return row ?? null
61+
}
5562

5663
async function workspaceAccess(tx: DbTransaction, userId: string, workspaceId: string) {
5764
const [target] = await tx
@@ -63,25 +70,17 @@ async function workspaceAccess(tx: DbTransaction, userId: string, workspaceId: s
6370
})
6471
.from(workspace)
6572
.where(and(eq(workspace.id, workspaceId), isNull(workspace.archivedAt)))
66-
.for('share')
6773
.limit(1)
6874
if (!target) throw new OrchestrationError('not_found', 'Workspace not found')
69-
await requireCurrentHumanRole(userId, target, 'read', { executor: tx })
75+
const permission = await requireCurrentHumanRole(userId, target, 'read', { executor: tx })
7076
await assertWorkspaceCapability(
7177
userId,
7278
workspaceId,
7379
'copilot.use',
7480
target.workspaceOrganizationId,
7581
tx
7682
)
77-
let sharedMutation = true
78-
try {
79-
await requireCurrentHumanRole(userId, target, 'write', { executor: tx })
80-
} catch (error) {
81-
if (!(error instanceof InsufficientWorkspacePermissionsError)) throw error
82-
sharedMutation = false
83-
}
84-
return { target, sharedMutation }
83+
return { target, sharedMutation: permissionSatisfies(permission, 'write') }
8584
}
8685

8786
async function projectAccess(
@@ -99,22 +98,21 @@ async function projectAccess(
9998
...(scope.organizationId ? { organizationId: scope.organizationId } : {}),
10099
...(scope.workspaceId ? { workspaceId: scope.workspaceId } : {}),
101100
},
102-
'shared'
101+
'snapshot'
103102
)
104103
if (scope.organizationId !== access.record.organizationId)
105104
throw new OrchestrationError('not_found', 'Project not found in this conversation')
106-
if (access.record.organizationId)
107-
await acquirePermissionGroupOrgReadLock(tx, access.record.organizationId)
108105
await requireCurrentCopilotProjectInvocation(
109106
tx,
110107
{
111108
subjectUserId: principal.subjectUserId,
112109
invocation: { kind: 'chat', chatId },
113110
},
114111
access,
115-
scope
112+
scope,
113+
'snapshot'
116114
)
117-
const owner = await requireProjectFileOwnerRole(tx, principal, access, 'read')
115+
const owner = await requireProjectFileOwnerRole(tx, principal, access, 'read', 'snapshot')
118116
let sharedMutation = owner.canWrite
119117
if (sharedMutation) {
120118
try {
@@ -167,8 +165,9 @@ const resolveAccess = defineAuthorizedChatUseCase({
167165
},
168166
},
169167
async execute({ principal, context, input }): Promise<MemoryNotesAccessResult> {
168+
const projectsEnabled = input.target.scope !== 'personal' && (await isProjectsEnabled())
170169
return db.transaction(async (tx) => {
171-
await requireCurrentFileSubject(tx, principal)
170+
await requireCurrentFileSubject(tx, principal, 'snapshot')
172171
const [chat] = await tx
173172
.select({
174173
userId: copilotChats.userId,
@@ -179,7 +178,6 @@ const resolveAccess = defineAuthorizedChatUseCase({
179178
})
180179
.from(copilotChats)
181180
.where(and(eq(copilotChats.id, context.chatId), isNull(copilotChats.deletedAt)))
182-
.for('share')
183181
.limit(1)
184182
if (
185183
!chat ||
@@ -204,9 +202,11 @@ const resolveAccess = defineAuthorizedChatUseCase({
204202
}
205203
const origin =
206204
chat.workspaceId && input.target.scope !== 'personal'
207-
? await lockWorkspaceProject(tx, chat.workspaceId)
205+
? await workspaceProject(tx, chat.workspaceId)
208206
: null
209-
if (chat.workspaceId) await workspaceAccess(tx, principal.subjectUserId, chat.workspaceId)
207+
const originAccess = chat.workspaceId
208+
? await workspaceAccess(tx, principal.subjectUserId, chat.workspaceId)
209+
: null
210210
const scope = {
211211
organizationId: chat.organizationId ?? origin?.organizationId ?? null,
212212
...(chat.workspaceId ? { workspaceId: chat.workspaceId } : {}),
@@ -218,7 +218,7 @@ const resolveAccess = defineAuthorizedChatUseCase({
218218
permissions: { read: true, privateMutation: true, sharedMutation: false },
219219
}
220220
} else if (input.target.scope === 'project') {
221-
await requireProjectApiEnabled()
221+
if (!projectsEnabled) await requireProjectApiEnabled()
222222
if (chat.workspaceId && origin?.id !== input.target.projectId)
223223
throw new OrchestrationError('not_found', 'Project not found in this conversation')
224224
target = await projectAccess(tx, principal, context.chatId, input.target.projectId, scope)
@@ -227,13 +227,15 @@ const resolveAccess = defineAuthorizedChatUseCase({
227227
throw new OrchestrationError('not_found', 'Workspace not found in this conversation')
228228
const membership = chat.workspaceId
229229
? origin
230-
: await lockWorkspaceProject(tx, input.target.workspaceId)
230+
: await workspaceProject(tx, input.target.workspaceId)
231231
if (!membership || membership.organizationId !== scope.organizationId)
232232
throw new OrchestrationError('not_found', 'Workspace not found in this conversation')
233-
const access = await workspaceAccess(tx, principal.subjectUserId, input.target.workspaceId)
233+
const access =
234+
originAccess ??
235+
(await workspaceAccess(tx, principal.subjectUserId, input.target.workspaceId))
234236
if (access.target.workspaceOrganizationId !== scope.organizationId)
235237
throw new OrchestrationError('not_found', 'Workspace not found in this conversation')
236-
const currentProject = (await isFeatureEnabled('projects'))
238+
const currentProject = projectsEnabled
237239
? await projectAccess(tx, principal, context.chatId, membership.id, scope)
238240
: null
239241
target = {
@@ -252,7 +254,7 @@ const resolveAccess = defineAuthorizedChatUseCase({
252254
actor: { userId: principal.subjectUserId, chatId: context.chatId },
253255
target,
254256
}
255-
})
257+
}, READ_SNAPSHOT)
256258
},
257259
})
258260

‎apps/sim/lib/mothership/memories/cleanup-outbox.test.ts‎

Lines changed: 2 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ vi.mock('@/lib/mothership/server/agent-url', () => mothershipAgentUrlMock)
1515

1616
import { enqueueMemoryCleanup } from '@/lib/mothership/memories/cleanup-outbox'
1717

18-
const { mockEnqueueOutboxEvent } = outboxServiceMockFns
18+
const { mockEnqueueOutboxEvents } = outboxServiceMockFns
1919
const executor = { insert: vi.fn() }
2020
const targets = [
2121
{ kind: 'private_owner' as const, userId: 'user-1' },
@@ -35,17 +35,6 @@ describe('enqueueMemoryCleanup', () => {
3535

3636
it('queues nothing when this Sim has no worker credential', async () => {
3737
await enqueueMemoryCleanup(executor, targets)
38-
expect(mockEnqueueOutboxEvent).not.toHaveBeenCalled()
39-
})
40-
41-
it('queues one credential-bound event per destination and target', async () => {
42-
setEnv({ COPILOT_API_KEY: 'configured-key' })
43-
await enqueueMemoryCleanup(executor, targets)
44-
expect(mockEnqueueOutboxEvent).toHaveBeenCalledTimes(targets.length)
45-
for (const [, , payload] of mockEnqueueOutboxEvent.mock.calls)
46-
expect(payload).toMatchObject({
47-
baseUrl: 'https://agent.example.test',
48-
credentialFingerprint: expect.any(String),
49-
})
38+
expect(mockEnqueueOutboxEvents).not.toHaveBeenCalled()
5039
})
5140
})

0 commit comments

Comments
 (0)