fix(housekeeping): release authority lock before activity

src/features/housekeeping/domains/people/services/moderation-mutations.ts

@@ -80,6 +80,7 @@ export interface PeopleModerationMutationDependencies {
 		userId: number,
 		context: PeopleModerationMutationContext,
 		execute: () => Promise<void>,
+		afterRelease?: () => Promise<void>,
 	) => Promise<void>;
 	readonly transport: ModerationTransport;
 	readonly executeModerationAction: (
@@ -406,22 +407,27 @@ export function createPeopleModerationMutationExecutor(
 				banId,
 				snapshot,
 				() =>
-					runWithLockedTargetAuthority(userId, context, async () => {
+					runWithLockedTargetAuthority(
-						let delivered = true;
+						userId,
-						if (username !== null) {
+						context,
-							delivered = await transport.disconnectUser(userId, username);
+						async () => {
-						}
+							let delivered = true;
-						await logStaffActivity({
+							if (username !== null) {
-							staffId: context.capability.actor.id,
+								delivered = await transport.disconnectUser(userId, username);
-							action: "user_ban",
+							}
-							description: `Banned user #${userId} (${type}, ${
+							await requireRcon(delivered);
-								hours > 0 ? `${hours}h` : "permanent"
+						},
-							}): ${reason}`,
+						() =>
-							targetType: "user",
+							logStaffActivity({
-							targetId: userId,
+								staffId: context.capability.actor.id,
-						});
+								action: "user_ban",
-						await requireRcon(delivered);
+								description: `Banned user #${userId} (${type}, ${
-					}),
+									hours > 0 ? `${hours}h` : "permanent"
+								}): ${reason}`,
+								targetType: "user",
+								targetId: userId,
+							}),
+					),
 			);
 		}
 

src/features/housekeeping/domains/people/services/mutations-production-workflows.test.ts

@@ -319,6 +319,110 @@ describe("People production workflow adapter", () => {
 		).toEqual(["intent", "partial"]);
 	});
 
+	it.each([
+		[
+			"user.trade-lock",
+			{ userId: 7, untilUnix: 200 },
+			[[target()], [], [], [target()]],
+			"setTradeLock",
+		],
+		[
+			"ban.create",
+			{ userId: 7, hours: 24, type: "account", reason: "Abuse" },
+			[[target()], [target()]],
+			"disconnectUser",
+		],
+	] as const)(
+		"releases the user lock before legacy activity for %s",
+		async (operation, input, selectedRows, transportMethod) => {
+			let transactionDepth = 0;
+			let dispatchDepth = -1;
+			let activityDepth = -1;
+			mocks.selectQueue.push(...selectedRows.map((rows) => [...rows]));
+			const transaction = async (
+				callback: (tx: ReturnType<typeof databaseFacade>) => Promise<unknown>,
+			) => {
+				transactionDepth += 1;
+				try {
+					return await callback(databaseFacade());
+				} finally {
+					transactionDepth -= 1;
+				}
+			};
+			mocks.transaction
+				.mockImplementationOnce(transaction)
+				.mockImplementationOnce(transaction);
+			mocks.rcon[transportMethod].mockImplementationOnce(async () => {
+				dispatchDepth = transactionDepth;
+				return true;
+			});
+			mocks.logStaffActivity.mockImplementationOnce(async () => {
+				activityDepth = transactionDepth;
+			});
+
+			const result = await peopleMutationService.execute(
+				{ ...invocation, legacy: false },
+				operation,
+				input,
+			);
+
+			expect(result).toMatchObject({ ok: true });
+			expect(dispatchDepth).toBe(1);
+			expect(activityDepth).toBe(0);
+			expect(mocks.logStaffActivity).toHaveBeenCalledTimes(1);
+		},
+	);
+
+	it("preserves known completion when released activity also fails", async () => {
+		let transactionDepth = 0;
+		let dispatchCompleted = false;
+		let activityDepth = -1;
+		mocks.selectQueue.push([target()], [], [], [target()]);
+		mocks.transaction
+			.mockImplementationOnce(async (callback) => callback(databaseFacade()))
+			.mockImplementationOnce(async (callback) => {
+				transactionDepth += 1;
+				try {
+					const result = await callback(databaseFacade());
+					if (dispatchCompleted) {
+						throw new Error("read-only authority commit failed");
+					}
+					return result;
+				} finally {
+					transactionDepth -= 1;
+				}
+			});
+		mocks.rcon.setTradeLock.mockImplementationOnce(async () => {
+			dispatchCompleted = true;
+			return true;
+		});
+		mocks.logStaffActivity.mockImplementationOnce(async () => {
+			activityDepth = transactionDepth;
+			throw new Error("activity transport failed");
+		});
+
+		const result = await peopleMutationService.execute(
+			{
+				...invocation,
+				legacy: false,
+				correlationId: "released-activity-failed",
+			},
+			"user.trade-lock",
+			{ userId: 7, untilUnix: 200 },
+		);
+
+		expect(activityDepth).toBe(0);
+		expect(result).toMatchObject({
+			ok: true,
+			completion: {
+				status: "partial",
+				external: "completed",
+				audit: "persisted",
+			},
+		});
+		expect(mocks.rcon.setTradeLock).toHaveBeenCalledTimes(1);
+	});
+
 	it("rejects forged, closed, and non-user CFH sanctions before mutation", async () => {
 		for (const [ticket, input, code] of [
 			[

src/features/housekeeping/domains/people/services/mutations.ts

@@ -435,8 +435,10 @@ async function runWithLockedTargetAuthority(
 	userId: number,
 	context: PeopleMutationContext,
 	execute: () => Promise<void>,
+	afterRelease?: () => Promise<void>,
 ): Promise<void> {
 	let externalCompleted = false;
+	let completedFailure: CompletedExternalEffectFailure | undefined;
 	try {
 		await db.transaction(async (tx) => {
 			await loadTarget(userId, context, true, tx, true);
@@ -444,9 +446,15 @@ async function runWithLockedTargetAuthority(
 			externalCompleted = true;
 		});
 	} catch (error) {
-		if (externalCompleted) throw new CompletedExternalEffectFailure(error);
+		if (!externalCompleted) throw error;
-		throw error;
+		completedFailure = new CompletedExternalEffectFailure(error);
 	}
+	try {
+		await afterRelease?.();
+	} catch (error) {
+		completedFailure ??= new CompletedExternalEffectFailure(error);
+	}
+	if (completedFailure) throw completedFailure;
 }
 
 async function assertBulkTargetHierarchy(
@@ -1247,29 +1255,36 @@ async function executeUserMutation(
 		userId,
 		snapshot,
 		() =>
-			runWithLockedTargetAuthority(userId, context, async () => {
+			runWithLockedTargetAuthority(
-				await requireRcon(await rcon.setTradeLock(userId, locked));
+				userId,
-				await requireRcon(
+				context,
-					await rcon.alertUser(
+				async () => {
-						userId,
+					await requireRcon(await rcon.setTradeLock(userId, locked));
-						locked
+					await requireRcon(
-							? "Trading has been disabled by staff."
+						await rcon.alertUser(
-							: "Trading has been re-enabled by staff.",
+							userId,
-					),
+							locked
-				);
+								? "Trading has been disabled by staff."
-				if (target.online === "1") {
+								: "Trading has been re-enabled by staff.",
-					await requireRcon(await rcon.disconnectUser(userId, target.username));
+						),
-				}
+					);
-				await logStaffActivity({
+					if (target.online === "1") {
-					staffId: context.capability.actor.id,
+						await requireRcon(
-					action: locked ? "trade_lock" : "trade_unlock",
+							await rcon.disconnectUser(userId, target.username),
-					description: locked
+						);
-						? `Trade-locked ${target.username} (#${userId}) until ${untilUnix}`
+					}
-						: `Cleared trade lock for ${target.username} (#${userId})`,
+				},
-					targetType: "user",
+				() =>
-					targetId: userId,
+					logStaffActivity({
-				});
+						staffId: context.capability.actor.id,
-			}),
+						action: locked ? "trade_lock" : "trade_unlock",
+						description: locked
+							? `Trade-locked ${target.username} (#${userId}) until ${untilUnix}`
+							: `Cleared trade lock for ${target.username} (#${userId})`,
+						targetType: "user",
+						targetId: userId,
+					}),
+			),
 	);
 }
 async function executeBulkMutation(