fix: stop the commandocentrum audit call leaking an unhandled rejection
Gitea Actions Runner Test / test-job (push) Successful in 2s
CI / check (push) Successful in 27s
CI / tests-unit (push) Successful in 1m35s
CI / tests-integration (push) Successful in 1m37s
CI / tests-ui (push) Failing after 2m25s
CI / preflight (push) Skipped
CI / deploy (push) Skipped
Gitea Actions Runner Test / test-job (push) Successful in 2s
CI / check (push) Successful in 27s
CI / tests-unit (push) Successful in 1m35s
CI / tests-integration (push) Successful in 1m37s
CI / tests-ui (push) Failing after 2m25s
CI / preflight (push) Skipped
CI / deploy (push) Skipped
The full suite passed but exited non-zero, which fails CI: three unhandled rejections came out of commandocentrum's fire-and-forget audit call. The cause is a real defect, not a test artefact. `auditAction` wrapped `logAudit(...)` in a try/catch to honour "auditing must never fail the command it describes", but logAudit is async, so the catch can never see its rejection. A failing audit insert therefore surfaced as an unhandled rejection instead of being swallowed — in production that is a request taking down over a logging failure. The catch is now on the promise itself. The test now mocks the audit service explicitly instead of leaning on the fake db lacking `insert`, and asserts both that an entry is logged and that a rejecting audit still lets the command succeed.
This commit is contained in:
1 parent
759ae91745
commit
5cb42c8dfb
2 files changed
+39
-5
No files matched your search
@@ -52,6 +52,13 @@ const mockSetMotto = vi.hoisted(() => vi.fn());
|
|||||||
const mockSetRank = vi.hoisted(() => vi.fn());
|
const mockSetRank = vi.hoisted(() => vi.fn());
|
||||||
const mockExecuteCommand = vi.hoisted(() => vi.fn());
|
const mockExecuteCommand = vi.hoisted(() => vi.fn());
|
||||||
const mockSendGift = vi.hoisted(() => vi.fn());
|
const mockSendGift = vi.hoisted(() => vi.fn());
|
||||||
|
// The audit service is exercised separately; here it only has to be harmless.
|
||||||
|
// Mocked explicitly because the fake db has no `insert`, which used to leak an
|
||||||
|
// unhandled rejection out of the fire-and-forget audit call.
|
||||||
|
vi.mock("@/lib/services/audit", () => ({
|
||||||
|
logAudit: vi.fn(async () => undefined),
|
||||||
|
}));
|
||||||
|
|
||||||
vi.mock("@/lib/services/rcon", () => ({
|
vi.mock("@/lib/services/rcon", () => ({
|
||||||
rcon: {
|
rcon: {
|
||||||
send: mockSend,
|
send: mockSend,
|
||||||
@@ -455,3 +462,25 @@ describe("access control", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("auditing", () => {
|
||||||
|
it("logs an entry for a currency grant", async () => {
|
||||||
|
const { logAudit } = await import("@/lib/services/audit");
|
||||||
|
await giveCredits({ userId: 1, credits: 100 });
|
||||||
|
expect(logAudit).toHaveBeenCalledWith(
|
||||||
|
expect.objectContaining({ action: expect.any(String) }),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("completes the command even when auditing rejects", async () => {
|
||||||
|
const { logAudit } = await import("@/lib/services/audit");
|
||||||
|
vi.mocked(logAudit).mockRejectedValueOnce(new Error("audit table missing"));
|
||||||
|
await expect(giveCredits({ userId: 1, credits: 100 })).resolves.toEqual({
|
||||||
|
ok: true,
|
||||||
|
data: {},
|
||||||
|
});
|
||||||
|
// Let the fire-and-forget promise settle; an unhandled rejection here is
|
||||||
|
// exactly the failure this guards against.
|
||||||
|
await new Promise((r) => setTimeout(r, 0));
|
||||||
|
});
|
||||||
|
});
|
||||||
@@ -24,11 +24,16 @@ function auditAction(
|
|||||||
targetId: number,
|
targetId: number,
|
||||||
after: Record<string, unknown>,
|
after: Record<string, unknown>,
|
||||||
): void {
|
): void {
|
||||||
try {
|
// logAudit is async, so a surrounding try/catch cannot see its rejection —
|
||||||
logAudit({ userId, action, target: "User", targetId, after });
|
// it would surface as an unhandled rejection and, in production, take the
|
||||||
} catch {
|
// request down over a failing audit insert. Swallow it on the promise
|
||||||
/* auditing must never fail the command it describes */
|
// instead, which is what "auditing must never fail the command it
|
||||||
}
|
// describes" actually requires.
|
||||||
|
void Promise.resolve()
|
||||||
|
.then(() => logAudit({ userId, action, target: "User", targetId, after }))
|
||||||
|
.catch(() => {
|
||||||
|
/* auditing must never fail the command it describes */
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
async function requireRconOk(ok: boolean): Promise<void> {
|
async function requireRconOk(ok: boolean): Promise<void> {
|
||||||
|
|||||||
Reference in new issue
Block a user