From d7771887a1f3c8a169e68d34472bb21229bc2c23 Mon Sep 17 00:00:00 2001 From: xCyanGrizzly Date: Mon, 22 Jun 2026 08:57:25 +0200 Subject: [PATCH] fix(worker): silence false-positive integrity-test notifications MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The pre-upload integrity test is advisory (never blocks upload), but it raised a WARNING SystemNotification whenever `7z t` failed — most often because large 7z archives OOM-kill the test process (SIGKILL / exit 137) in the memory-limited worker container, which is a tool limitation, not corruption. Classify failures as encrypted | corrupt | inconclusive; suppress notifications for inconclusive (debug log only) while still proceeding with the upload as before. Genuine corruption now uses the INTEGRITY_AUDIT notification type instead of the misleading HASH_MISMATCH; encrypted archives still notify. Co-Authored-By: Claude Opus 4.8 --- worker/src/archive/integrity.ts | 77 ++++++++++++++++++++++++--------- worker/src/worker.ts | 65 ++++++++++++++++------------ 2 files changed, 94 insertions(+), 48 deletions(-) diff --git a/worker/src/archive/integrity.ts b/worker/src/archive/integrity.ts index 7bb9a6e..510cff4 100644 --- a/worker/src/archive/integrity.ts +++ b/worker/src/archive/integrity.ts @@ -5,21 +5,28 @@ import { childLogger } from "../util/logger.js"; const execFileAsync = promisify(execFile); const log = childLogger("integrity"); +export type IntegrityFailureKind = "encrypted" | "corrupt" | "inconclusive"; + export type IntegrityResult = | { ok: true } - | { ok: false; reason: string }; + | { ok: false; reason: string; kind: IntegrityFailureKind }; /** * Test that the archive can be read end-to-end without errors, BEFORE we - * spend bandwidth uploading it to the destination channel. Catches: - * - Truncated downloads (rare given our size check, but cheap to confirm) - * - CRC errors inside the archive - * - Bad central directories - * - Encrypted archives (we report them as failures rather than upload - * a file users can't extract) + * spend bandwidth uploading it to the destination channel. * - * Returns { ok: true } if the archive is intact. Returns - * { ok: false, reason } otherwise. Logs at warn level on failure. + * Failures are classified so the caller can react appropriately: + * - "encrypted" — password-protected; users can't extract it. Actionable. + * - "corrupt" — genuine CRC / structural error (truncated download, bad + * central directory, CRC mismatch). Actionable. + * - "inconclusive" — the test tool itself was killed (OOM) or timed out, + * typically on very large 7z archives in a memory-limited + * container (exit 137 / SIGKILL). This is a TOOL + * LIMITATION, not corruption — callers should NOT raise a + * user-facing alarm for it. + * + * Returns { ok: true } if the archive is intact, otherwise + * { ok: false, reason, kind }. * * For multipart archives, pass the first part. unzip / unrar / 7z all * auto-discover sibling parts. @@ -42,7 +49,7 @@ export async function testArchiveIntegrity( maxBuffer: 10 * 1024 * 1024, }); if (stderr && stderr.trim()) { - return { ok: false, reason: `unzip -t reported: ${stderr.slice(0, 500)}` }; + return { ok: false, kind: "corrupt", reason: `unzip -t reported: ${stderr.slice(0, 500)}` }; } return { ok: true }; } @@ -58,7 +65,7 @@ export async function testArchiveIntegrity( if (/All OK/i.test(combined)) { return { ok: true }; } - return { ok: false, reason: `unrar t did not report "All OK": ${combined.slice(-500)}` }; + return { ok: false, kind: "corrupt", reason: `unrar t did not report "All OK": ${combined.slice(-500)}` }; } if (archiveType === "SEVEN_Z") { @@ -70,24 +77,52 @@ export async function testArchiveIntegrity( if (/Everything is Ok/i.test(combined)) { return { ok: true }; } - return { ok: false, reason: `7z t did not report "Everything is Ok": ${combined.slice(-500)}` }; + return { ok: false, kind: "corrupt", reason: `7z t did not report "Everything is Ok": ${combined.slice(-500)}` }; } - return { ok: false, reason: `Unknown archive type: ${archiveType}` }; + return { ok: false, kind: "corrupt", reason: `Unknown archive type: ${archiveType}` }; } catch (err) { const msg = err instanceof Error ? err.message : String(err); - // execFile throws on non-zero exit. Try to extract the most useful part. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const stderr = (err as any)?.stderr as string | undefined; + // execFile throws on non-zero exit, on timeout, and when killed by a signal. + const e = err as { + stderr?: unknown; + stdout?: unknown; + signal?: string | null; + killed?: boolean; + code?: number | string | null; + }; + const stderr = typeof e.stderr === "string" ? e.stderr : ""; + const stdout = typeof e.stdout === "string" ? e.stdout : ""; const detail = stderr ? `: ${stderr.slice(0, 500)}` : ""; + const haystack = `${msg}\n${stdout}\n${stderr}`; - // Specifically flag encrypted archives so the caller can record a more - // specific SkipReason / notification. - if (/password|encrypted|need.*password/i.test(`${msg}${detail}`)) { - return { ok: false, reason: `Archive is encrypted (password protected): ${msg}${detail}` }; + // Encrypted archives — users can't extract them, so flag clearly. + if (/password|encrypted|wrong password|enter password/i.test(haystack)) { + return { ok: false, kind: "encrypted", reason: `Archive is encrypted (password protected): ${msg}${detail}` }; } + // Inconclusive — the test tool was killed or timed out rather than + // reporting corruption. Common on large 7z in memory-limited containers, + // where `7z t` gets OOM-killed (SIGKILL / exit 137) mid-decompression. + // That's a tool limitation, not a corrupt archive. + const killedBySignal = e.signal === "SIGKILL" || e.signal === "SIGTERM"; + const killedExitCode = e.code === 137 || e.code === 143; // 128 + SIGKILL/SIGTERM + const timedOut = e.killed === true; + const maxBufferExceeded = e.code === "ERR_CHILD_PROCESS_STDIO_MAXBUFFER"; + if (killedBySignal || killedExitCode || timedOut || maxBufferExceeded) { + log.debug( + { err, archiveType, firstPartPath, signal: e.signal, code: e.code, killed: e.killed }, + "Archive integrity test inconclusive (tool killed or timed out)" + ); + return { + ok: false, + kind: "inconclusive", + reason: `Integrity test could not complete (tool killed or timed out — likely OOM on a large archive): ${msg}`, + }; + } + + // Genuine failure: a real non-zero exit with error output. log.debug({ err, archiveType, firstPartPath }, "Archive integrity test failed"); - return { ok: false, reason: `Integrity test failed: ${msg}${detail}` }; + return { ok: false, kind: "corrupt", reason: `Integrity test failed: ${msg}${detail}` }; } } diff --git a/worker/src/worker.ts b/worker/src/worker.ts index 96cf0b9..fcd1be1 100644 --- a/worker/src/worker.ts +++ b/worker/src/worker.ts @@ -1941,34 +1941,45 @@ async function processOneArchiveSet( if (!isMultipartZip) { const integrity = await testArchiveIntegrity(archType, tempPaths[0]); if (!integrity.ok) { - // Detect encryption specifically — those won't extract for users - // even if we upload them. Surface clearly via notification but - // STILL proceed: the user can audit and decide what to do. - const isEncrypted = /encrypted/i.test(integrity.reason); - accountLog.warn( - { fileName: archiveName, reason: integrity.reason.slice(0, 200), isEncrypted }, - "Archive integrity test failed — proceeding with upload anyway (advisory check)" - ); - try { - await db.systemNotification.create({ - data: { - type: isEncrypted ? "UPLOAD_FAILED" : "HASH_MISMATCH", - severity: "WARNING", - title: isEncrypted - ? `Archive may be encrypted: ${archiveName}` - : `Integrity test reported issues: ${archiveName}`, - message: integrity.reason.slice(0, 1000), - context: { - fileName: archiveName, - sourceChannelId: channel.id, - sourceMessageId: Number(archiveSet.parts[0].id), - archiveType: archType, - advisory: true, + if (integrity.kind === "inconclusive") { + // The test tool was killed (OOM) or timed out — typically a large + // 7z in a memory-limited container. This is a tool limitation, NOT + // corruption, so log quietly and DON'T raise a notification. The + // upload proceeds exactly as before. + accountLog.debug( + { fileName: archiveName, reason: integrity.reason.slice(0, 200) }, + "Archive integrity test inconclusive — proceeding with upload (advisory check)" + ); + } else { + // Encrypted (won't extract for users) or genuinely corrupt — surface + // clearly via notification but STILL proceed: the user can audit and + // decide what to do. + const isEncrypted = integrity.kind === "encrypted"; + accountLog.warn( + { fileName: archiveName, reason: integrity.reason.slice(0, 200), kind: integrity.kind }, + "Archive integrity test failed — proceeding with upload anyway (advisory check)" + ); + try { + await db.systemNotification.create({ + data: { + type: isEncrypted ? "UPLOAD_FAILED" : "INTEGRITY_AUDIT", + severity: "WARNING", + title: isEncrypted + ? `Archive may be encrypted: ${archiveName}` + : `Integrity test reported issues: ${archiveName}`, + message: integrity.reason.slice(0, 1000), + context: { + fileName: archiveName, + sourceChannelId: channel.id, + sourceMessageId: Number(archiveSet.parts[0].id), + archiveType: archType, + advisory: true, + }, }, - }, - }); - } catch { - // Best-effort notification + }); + } catch { + // Best-effort notification + } } } }