Skip to content

Keep shared-app approval decisions atomic under concurrent changes #924

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
rudycelekli wants to merge 3 commits into
base: main
Choose a base branch
from
+411 −109
Open

Keep shared-app approval decisions atomic under concurrent changes #924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a1c23cd
Commit shared-account decisions together with the approval
rudycelekli Oct 31, 2026
79b7d74
Serialize shared-app approval decisions before writing either row
rudycelekli Oct 31, 2026
85717f9
Keep a concurrent owner grant from replacing administrator approval
rudycelekli Oct 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,15 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged.

## Unreleased

### A failed shared-app approval can be tried again

Deciding a shared-app request now commits the decision and the access it grants together. If the
approval or its audience cannot be saved, the request stays pending instead of disappearing as
approved without granting access. An administrator can retry after the write failure is resolved.
Concurrent direct approvals and request decisions take the same Bot/app transaction lock before
writing either row, so they do not deadlock by acquiring those rows in opposite orders. An owner
grant that races an administrator now leaves the administrator's approval and audience in place.

**Before upgrading.** Sixteen things change for an existing deployment:
- Bots now share one computer per person instead of each having its own. Migration
`0069_isolated_bots` adds `agent_profiles.isolated`, off for every existing Bot. With the
Expand Down
272 changes: 164 additions & 108 deletions server/src/plugins/shared-use-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import {
steeringOf,
} from "./shared-use";

type Transaction = Parameters<Parameters<Database["transaction"]>[0]>[0];

export class SharedUseRequestDecidedError extends Error {
constructor() {
super("That request has already been decided.");
Expand Down Expand Up @@ -141,8 +143,9 @@ export function createSharedUseStore(
*/
async function rowsOfApp(
serverId: string,
executor: Database | Transaction = database,
): Promise<{ appId: string; url: string | null; ids: string[] }> {
const [row] = await database
const [row] = await executor
.select({ url: mcpServers.url })
.from(mcpServers)
.where(eq(mcpServers.id, serverId))
Expand All @@ -151,7 +154,7 @@ export function createSharedUseStore(
const toolkit = url ? appOfUrl(url) : null;
if (!toolkit) return { appId: serverId, url, ids: [serverId] };
const canonical = canonicalUrlOf(toolkit);
const siblings = await database
const siblings = await executor
.select({ id: mcpServers.id })
.from(mcpServers)
.where(eq(mcpServers.url, canonical))
Expand All @@ -170,8 +173,11 @@ export function createSharedUseStore(
* read and write of an approval or a request goes through this first, and callers may hand in
* either id and land on the same key.
*/
async function appIdOf(serverId: string): Promise<string> {
return (await rowsOfApp(serverId)).appId;
async function appIdOf(
serverId: string,
executor: Database | Transaction = database,
): Promise<string> {
return (await rowsOfApp(serverId, executor)).appId;
}

/** The approval row for this Bot at the app, whoever set it, and whether it may be read now. */
Expand Down Expand Up @@ -241,86 +247,116 @@ export function createSharedUseStore(
};
}

async function setApproval(input: {
type ApprovalInput = {
botId: string;
serverId: string;
approval: SharedUseApproval;
by: string;
/** Who is setting it. Only {@link approveOwnerGrant} says `owner`. */
as?: "admin" | "owner";
}) {
const serverId = await appIdOf(input.serverId);
const approvedAs = input.as ?? "admin";
};

async function setApproval(input: ApprovalInput) {
await database.transaction(async (tx) => {
await tx
.insert(sharedUseApprovals)
.values({
const serverId = await lockApproval(tx, input.botId, input.serverId);
await writeApproval(tx, { ...input, serverId });
});
}

/** Lock the canonical Bot/app before either a request or its approval row is written. */
async function lockApproval(
tx: Transaction,
botId: string,
serverId: string,
) {
const appId = await appIdOf(serverId, tx);
await tx.execute(
sql`select pg_advisory_xact_lock(hashtext(${JSON.stringify(["shared-use-approval", botId, appId])}))`,
);
return appId;
}

/** Share the decision transaction; callers already hold the canonical Bot/app lock. */
async function writeApproval(tx: Transaction, input: ApprovalInput) {
const serverId = input.serverId;
const approvedAs = input.as ?? "admin";
const insertion = tx.insert(sharedUseApprovals).values({
agentId: input.botId,
serverId,
audience: input.approval.audience,
outsideInput: input.approval.outsideInput,
approvedBy: input.by,
approvedAs,
});
const target = [sharedUseApprovals.agentId, sharedUseApprovals.serverId];
// An owner may fill an absence, never replace an approval another transaction just committed.
const written =
approvedAs === "owner"
? await insertion
.onConflictDoNothing({ target })
.returning({ botId: sharedUseApprovals.agentId })
: await insertion
.onConflictDoUpdate({
target,
set: {
audience: input.approval.audience,
outsideInput: input.approval.outsideInput,
approvedBy: input.by,
approvedAs,
approvedAt: new Date(),
},
})
.returning({ botId: sharedUseApprovals.agentId });
if (written.length === 0) return false;
await tx
.delete(sharedUseApprovalMembers)
.where(
and(
eq(sharedUseApprovalMembers.agentId, input.botId),
eq(sharedUseApprovalMembers.serverId, serverId),
),
);
if (
input.approval.audience === "people" &&
input.approval.members.length > 0
) {
await tx.insert(sharedUseApprovalMembers).values(
input.approval.members.map((member) => ({
agentId: input.botId,
serverId,
audience: input.approval.audience,
outsideInput: input.approval.outsideInput,
approvedBy: input.by,
approvedAs,
})
.onConflictDoUpdate({
target: [sharedUseApprovals.agentId, sharedUseApprovals.serverId],
set: {
audience: input.approval.audience,
outsideInput: input.approval.outsideInput,
approvedBy: input.by,
approvedAs,
approvedAt: new Date(),
},
});
await tx
.delete(sharedUseApprovalMembers)
.where(
and(
eq(sharedUseApprovalMembers.agentId, input.botId),
eq(sharedUseApprovalMembers.serverId, serverId),
),
);
if (
input.approval.audience === "people" &&
input.approval.members.length > 0
) {
await tx.insert(sharedUseApprovalMembers).values(
input.approval.members.map((member) => ({
agentId: input.botId,
serverId,
...member,
})),
);
}
/* A pending request this approval now answers is closed as approved, so the inbox never asks twice. */
const pending = await tx
.select()
.from(sharedUseRequests)
.where(
and(
eq(sharedUseRequests.agentId, input.botId),
eq(sharedUseRequests.serverId, serverId),
eq(sharedUseRequests.status, "pending"),
),
);
for (const request of pending) {
const proposed = {
audience: request.proposedAudience,
outsideInput: request.proposedOutsideInput,
members: request.proposedMembers,
};
if (covers(input.approval, proposed)) {
await tx
.update(sharedUseRequests)
.set({
status: "approved",
decidedBy: input.by,
decidedAt: new Date(),
})
.where(eq(sharedUseRequests.id, request.id));
}
...member,
})),
);
}
/* A pending request this approval now answers is closed as approved, so the inbox never asks twice. */
const pending = await tx
.select()
.from(sharedUseRequests)
.where(
and(
eq(sharedUseRequests.agentId, input.botId),
eq(sharedUseRequests.serverId, serverId),
eq(sharedUseRequests.status, "pending"),
),
);
for (const request of pending) {
const proposed = {
audience: request.proposedAudience,
outsideInput: request.proposedOutsideInput,
members: request.proposedMembers,
};
if (covers(input.approval, proposed)) {
await tx
.update(sharedUseRequests)
.set({
status: "approved",
decidedBy: input.by,
decidedAt: new Date(),
})
.where(eq(sharedUseRequests.id, request.id));
}
});
}
return true;
}

/*
Expand Down Expand Up @@ -372,14 +408,17 @@ export function createSharedUseStore(
...exposureOf(await botFacts(input.botId)),
outsideInput: false,
};
await setApproval({
botId: input.botId,
serverId,
approval,
by: input.by,
as: "owner",
const written = await database.transaction(async (tx) => {
const appId = await lockApproval(tx, input.botId, serverId);
return writeApproval(tx, {
botId: input.botId,
serverId: appId,
approval,
by: input.by,
as: "owner",
});
});
return approval;
return written ? approval : null;
}

/**
Expand Down Expand Up @@ -738,33 +777,50 @@ export function createSharedUseStore(
by: string;
decision: "approve" | "decline";
}) {
const [claimed] = await database
.update(sharedUseRequests)
.set({
status: input.decision === "approve" ? "approved" : "declined",
decidedBy: input.by,
decidedAt: new Date(),
})
.where(
and(
eq(sharedUseRequests.id, input.id),
eq(sharedUseRequests.status, "pending"),
),
)
.returning();
if (!claimed) throw new SharedUseRequestDecidedError();
if (input.decision === "approve") {
await setApproval({
botId: claimed.agentId,
serverId: claimed.serverId,
by: input.by,
approval: {
audience: claimed.proposedAudience,
outsideInput: claimed.proposedOutsideInput,
members: claimed.proposedMembers,
},
});
}
await database.transaction(async (tx) => {
const [request] = await tx
.select({
botId: sharedUseRequests.agentId,
serverId: sharedUseRequests.serverId,
})
.from(sharedUseRequests)
.where(
and(
eq(sharedUseRequests.id, input.id),
eq(sharedUseRequests.status, "pending"),
),
)
.limit(1);
if (!request) throw new SharedUseRequestDecidedError();
const serverId = await lockApproval(tx, request.botId, request.serverId);
const [claimed] = await tx
.update(sharedUseRequests)
.set({
status: input.decision === "approve" ? "approved" : "declined",
decidedBy: input.by,
decidedAt: new Date(),
})
.where(
and(
eq(sharedUseRequests.id, input.id),
eq(sharedUseRequests.status, "pending"),
),
)
.returning();
if (!claimed) throw new SharedUseRequestDecidedError();
if (input.decision === "approve") {
await writeApproval(tx, {
botId: claimed.agentId,
serverId,
by: input.by,
approval: {
audience: claimed.proposedAudience,
outsideInput: claimed.proposedOutsideInput,
members: claimed.proposedMembers,
},
});
}
});
const [row] = await rows(eq(sharedUseRequests.id, input.id));
/*
* THE CLAIM ABOVE PROVES THE REQUEST ROW IS THERE, and this read still has to be checked: `rows`
Expand Down
Loading