fix(server): upload filters answer multer once on a throwing callback; positive call-graph gate test

This commit is contained in:
jubnl
2026-09-22 08:10:23 +02:00
parent ae2af05cad
commit de2d13b1af
5 changed files with 72 additions and 15 deletions
@@ -0,0 +1,23 @@
/**
* Fixture for `db-call-graph.mjs --sync --tx --root scripts/__fixtures__`: one class that
* still touches the database synchronously and opens a raw transaction, so the two gates
* CI runs (`--sync`, `--tx`) each have a known-bad case to report. `unawaited-sample.ts`
* beside it is clean under both flags.
*/
interface RawDb {
get(sql: string, ...args: unknown[]): unknown;
transaction<T>(fn: () => T): T;
}
export class SyncSample {
private db: RawDb = { get: () => undefined, transaction: (fn) => fn() };
readSync(id: number): unknown {
return this.db.get('SELECT 1 WHERE id = ?', id); // sync DB-touching method
}
writeInTx(): void {
this.db.transaction(() => undefined); // raw transaction site
}
}
+10 -3
View File
@@ -58,8 +58,8 @@ export function filesUploadFileFilter(allowedTypes: AllowedFileTypesService): Op
// detached async function that always answers through `cb` — multer holds
// the file stream until it does (make-middleware.js says as much). A
// rejection refuses the file, so the filter still fails closed. Only the
// await is wrapped in try/catch, so a throw from cb() itself is never
// re-routed into a second cb() call.
// await is wrapped in try/catch; a throw from cb() itself lands in the
// trailing .catch, which logs it and answers multer one final time.
(async () => {
let allowed: string[];
try {
@@ -73,7 +73,14 @@ export function filesUploadFileFilter(allowedTypes: AllowedFileTypesService): Op
reject();
})().catch((err: unknown) => {
logError(`files upload fileFilter: ${err instanceof Error ? err.message : String(err)}`);
cb(err instanceof Error ? err : new Error(String(err)));
// cb() itself is the only thing that can reach here (every await is
// inside the try above); answer once more so multer never hangs, but a
// second throw from cb() means multer already answered — swallow it.
try {
cb(err instanceof Error ? err : new Error(String(err)));
} catch {
/* multer already answered */
}
});
};
}
+10 -3
View File
@@ -88,8 +88,8 @@ export function journeyImageFileFilter(allowedTypes: AllowedFileTypesService): O
// allowed-extension list is now an async read. The decision runs in a
// detached async function that always answers through `cb`; a rejection
// refuses the file, so the filter still fails closed. Only the await is
// wrapped in try/catch, so a throw from cb() itself is never re-routed
// into a second cb() call.
// wrapped in try/catch; a throw from cb() itself lands in the trailing
// .catch, which logs it and answers multer one final time.
(async () => {
let allowed: string[];
try {
@@ -101,7 +101,14 @@ export function journeyImageFileFilter(allowedTypes: AllowedFileTypesService): O
cb(null, true);
})().catch((err: unknown) => {
logError(`journey photo fileFilter: ${err instanceof Error ? err.message : String(err)}`);
cb(err instanceof Error ? err : new Error(String(err)));
// cb() itself is the only thing that can reach here (every await is
// inside the try above); answer once more so multer never hangs, but a
// second throw from cb() means multer already answered — swallow it.
try {
cb(err instanceof Error ? err : new Error(String(err)));
} catch {
/* multer already answered */
}
});
};
}
+1 -1
View File
@@ -62,7 +62,7 @@ export async function getActiveNoticesFor(
// `evaluate` runs inside a .filter(), which cannot await, so the addon flags
// the registry is able to ask about are resolved up front and handed to it as
// a lookup. The ids come from the notices themselves, so every question the
// conditions can ask has an answer here and the fallback is unreachable.
// conditions can ask has an answer here.
const addonFlags = new Map<string, boolean>();
for (const condition of SYSTEM_NOTICES.flatMap(n => n.conditions)) {
if (condition.kind === 'addonEnabled' && !addonFlags.has(condition.addonId)) {
@@ -32,10 +32,26 @@ interface UnawaitedReport {
unawaitedCallCount: number;
}
interface SyncDbMethod {
file: string;
class: string;
method: string;
line: number;
direct: boolean;
}
interface TransactionSite {
file: string;
class: string;
method: string;
line: number;
receiver: string;
}
interface SyncTxReport {
syncDbMethods: unknown[];
syncDbMethods: SyncDbMethod[];
syncDbMethodCount: number;
transactionSites: unknown[];
transactionSites: TransactionSite[];
transactionSiteCount: number;
}
@@ -87,14 +103,18 @@ describe('db-call-graph.mjs', () => {
expect(byMethod.good3).toBeUndefined();
});
it('CALLGRAPH-002: --sync --tx finds nothing in the fixture tree (it touches no database) and exits 0', () => {
it('CALLGRAPH-002: --sync --tx reports exactly the sync-db fixture (two sync DB methods, one raw transaction) and exits 1', () => {
const { status, report } = run(['--root', 'scripts/__fixtures__', '--sync', '--tx', '--json']);
const { syncDbMethods, syncDbMethodCount, transactionSites, transactionSiteCount } = report as SyncTxReport;
expect(status).toBe(0);
expect(syncDbMethodCount).toBe(0);
expect(syncDbMethods).toEqual([]);
expect(transactionSiteCount).toBe(0);
expect(transactionSites).toEqual([]);
// Non-empty lists exit 1 — that is what makes the CI step fail on a regression.
expect(status).toBe(1);
expect(syncDbMethodCount).toBe(2);
expect(syncDbMethods.map((m) => `${m.class}.${m.method}`).sort()).toEqual(['SyncSample.readSync', 'SyncSample.writeInTx']);
expect(syncDbMethods.every((m) => m.file.endsWith('sync-db-sample.ts') && m.direct)).toBe(true);
expect(transactionSiteCount).toBe(1);
expect(transactionSites).toEqual([expect.objectContaining({ class: 'SyncSample', method: 'writeInTx', receiver: 'this.db' })]);
// The unawaited fixture beside it stays clean under both flags.
expect(syncDbMethods.some((m) => m.class === 'Sample')).toBe(false);
});
});