diff --git a/src/lib/api/sourcemaps.ts b/src/lib/api/sourcemaps.ts index 24e69a511..f345aa226 100644 --- a/src/lib/api/sourcemaps.ts +++ b/src/lib/api/sourcemaps.ts @@ -31,7 +31,7 @@ import { ApiError } from "../errors.js"; import { logger } from "../logger.js"; import { resolveOrgRegion } from "../region.js"; import { getSdkConfig } from "../sentry-client.js"; -import { ZipWriter } from "../sourcemap/zip.js"; +import { type ZipCompression, ZipWriter } from "../sourcemap/zip.js"; import { apiRequestToRegion } from "./infrastructure.js"; const gzipAsync = promisify(gzipCb); @@ -288,12 +288,22 @@ async function uploadChunk(params: { * * @param outputPath - Where to write the ZIP file * @param files - Source files and sourcemaps to include - * @param options - Bundle metadata (org, project, release) + * @param options.org / project / release - Bundle metadata + * @param options.compression - ZIP entry compression. Use `"stored"` + * when the wire layer will compress the chunks (zstd or gzip); + * compressing twice wastes CPU and barely helps wire size. + * Defaults to `"deflate"` so callers without a wire codec still + * ship reasonably-sized payloads. */ export async function buildArtifactBundle( outputPath: string, files: ArtifactFile[], - options: { org: string; project: string; release?: string } + options: { + org: string; + project: string; + release?: string; + compression?: ZipCompression; + } ): Promise { // Build manifest.json const filesManifest: Record< @@ -329,7 +339,9 @@ export async function buildArtifactBundle( }); // Stream ZIP entries to disk - const zip = await ZipWriter.create(outputPath); + const zip = await ZipWriter.create(outputPath, { + compression: options.compression, + }); try { await zip.addEntry("manifest.json", Buffer.from(manifest, "utf-8")); @@ -396,16 +408,31 @@ export async function uploadSourcemaps(options: UploadOptions): Promise { // Step 1: Get chunk upload configuration const serverOptions = await getChunkUploadOptions(org); + // Pick the wire codec up-front so the ZIP can skip its own + // compression pass when the wire layer will handle it. Re-compressing + // an already-DEFLATE'd ZIP with zstd/gzip burns CPU for ~0% wire + // savings; STORED + zstd saves both CPU and a few percent wire bytes. + // Without a wire codec (kill-switch / unsupported codecs) we keep + // DEFLATE so the ZIP itself stays small. + const encoding = pickUploadEncoding(serverOptions.compression); + const zipCompression: ZipCompression = encoding ? "stored" : "deflate"; + // Step 2: Build artifact bundle ZIP to a temp file, then upload const tmpZipPath = join(tmpdir(), `sentry-artifact-bundle-${Date.now()}.zip`); try { - await buildArtifactBundle(tmpZipPath, files, { org, project, release }); + await buildArtifactBundle(tmpZipPath, files, { + org, + project, + release, + compression: zipCompression, + }); await uploadArtifactBundle({ tmpZipPath, org, project, release, serverOptions, + encoding, }); } finally { // Always clean up the temp file @@ -428,8 +455,9 @@ async function uploadArtifactBundle(opts: { project: string; release: string | undefined; serverOptions: ChunkServerOptions; + encoding: UploadEncoding | undefined; }): Promise { - const { tmpZipPath, org, project, release, serverOptions } = opts; + const { tmpZipPath, org, project, release, serverOptions, encoding } = opts; // Step 3: Split into chunks, hash each chunk + compute overall checksum const { chunks, overallChecksum } = await hashChunks( tmpZipPath, @@ -477,7 +505,6 @@ async function uploadArtifactBundle(opts: { const missingChunks = chunks.filter((c) => missingSet.has(c.sha1)); if (missingChunks.length > 0) { - const encoding = pickUploadEncoding(serverOptions.compression); const limit = pLimit(serverOptions.concurrency); // Use the CLI's authenticated fetch for chunk uploads const { fetch: authFetch } = getSdkConfig(regionUrl); diff --git a/src/lib/sourcemap/zip.ts b/src/lib/sourcemap/zip.ts index 458e55de9..838019ac1 100644 --- a/src/lib/sourcemap/zip.ts +++ b/src/lib/sourcemap/zip.ts @@ -5,8 +5,10 @@ * compressed data is held in memory at a time. Produces valid ZIP * archives that can be extracted by standard tools (unzip, 7z, etc.). * - * Uses raw DEFLATE (method 8) for compression and the CRC-32 - * function from `node:zlib` for checksums. + * Per-archive compression method (DEFLATE or STORED) is chosen at + * {@link ZipWriter.create} time. STORED is preferred when the + * caller will compress the bytes again at the wire layer (e.g. + * chunk-upload with zstd) — see {@link ZipCompression}. */ import type { FileHandle } from "node:fs/promises"; @@ -57,13 +59,23 @@ type EntryRecord = { localHeaderOffset: number; }; +/** + * Per-archive compression strategy. `"deflate"` mirrors the original + * behavior; `"stored"` skips entry compression entirely and is the + * right choice when the bytes will be re-compressed by an outer codec + * (e.g. chunk-upload with zstd/gzip on the wire). Compressing an + * already-compressed payload is wasted CPU and can even slightly + * inflate the output. + */ +export type ZipCompression = "deflate" | "stored"; + /** * Streaming ZIP archive writer. * - * Entries are compressed and flushed to disk one at a time via - * {@link addEntry}, keeping peak memory proportional to a single - * file's compressed output. Call {@link finalize} after all entries - * have been added to write the central directory and close the file. + * Entries are written to disk one at a time via {@link addEntry}, + * keeping peak memory proportional to a single entry's payload. Call + * {@link finalize} after all entries have been added to write the + * central directory and close the file. * * @example * ```ts @@ -82,8 +94,11 @@ export class ZipWriter { private readonly fh: FileHandle; - private constructor(fh: FileHandle) { + private readonly compression: ZipCompression; + + private constructor(fh: FileHandle, compression: ZipCompression) { this.fh = fh; + this.compression = compression; } /** @@ -100,11 +115,17 @@ export class ZipWriter { * valid archive and release the file handle. * * @param outputPath - Filesystem path for the output ZIP file. + * @param options.compression - Per-entry compression method + * (`"deflate"` default; `"stored"` to skip compression — see + * {@link ZipCompression}). * @returns A ready-to-use writer instance. */ - static async create(outputPath: string): Promise { + static async create( + outputPath: string, + options: { compression?: ZipCompression } = {} + ): Promise { const fh = await open(outputPath, "w"); - const writer = new ZipWriter(fh); + const writer = new ZipWriter(fh, options.compression ?? "deflate"); // Write the SourceBundle magic header before any ZIP data. // Format: 4-byte magic "SYSB" + 4-byte LE version (2). @@ -136,9 +157,9 @@ export class ZipWriter { /** * Add a file entry to the archive. * - * The data is compressed with raw DEFLATE and written to disk - * immediately, so only one entry's compressed payload is buffered - * at a time. + * The data is written using the writer's configured compression + * method (DEFLATE or STORED). Empty entries are always STORED + * because some extractors mishandle DEFLATE of empty input. * * @param name - File path inside the archive (forward-slash separated). * @param data - Uncompressed file contents. @@ -147,8 +168,7 @@ export class ZipWriter { const nameBytes = Buffer.from(name, "utf-8"); const checksum = crc32(data); - // Use STORE for empty files (DEFLATE of empty input can confuse extractors) - const useStore = data.length === 0; + const useStore = this.compression === "stored" || data.length === 0; const method = useStore ? METHOD_STORE : METHOD_DEFLATE; const payload = useStore ? data : await deflateRaw(data); diff --git a/test/lib/api/sourcemaps.test.ts b/test/lib/api/sourcemaps.test.ts index abcb2426d..49a11d1a8 100644 --- a/test/lib/api/sourcemaps.test.ts +++ b/test/lib/api/sourcemaps.test.ts @@ -1,6 +1,10 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; import { gunzipSync } from "node:zlib"; import { + buildArtifactBundle, ChunkServerOptionsSchema, encodeChunk, pickUploadEncoding, @@ -57,6 +61,97 @@ describe("encodeChunk", () => { }); }); +describe("buildArtifactBundle", () => { + // ZIP local file header format: at offset 8 (LE u16) is the + // compression method (0 = STORED, 8 = DEFLATE). The CLI prefixes + // every artifact bundle with an 8-byte SYSB SourceBundle header. + const SYSB_HEADER_BYTES = 8; + const LOCAL_HEADER_METHOD_OFFSET = SYSB_HEADER_BYTES + 8; + + let tmpDir: string; + + beforeEach(async () => { + tmpDir = await mkdtemp(join(tmpdir(), "bundle-test-")); + }); + + afterEach(async () => { + await rm(tmpDir, { recursive: true, force: true }); + }); + + async function makePair(): Promise< + { + path: string; + debugId: string; + type: "minified_source" | "source_map"; + url: string; + sourcemapFilename?: string; + }[] + > { + const jsPath = join(tmpDir, "app.js"); + const mapPath = join(tmpDir, "app.js.map"); + // Highly redundant content so DEFLATE has work to do — the size + // assertions below depend on this. + await writeFile(jsPath, "console.log('hello');\n".repeat(500)); + await writeFile(mapPath, JSON.stringify({ version: 3, sources: [] })); + return [ + { + path: jsPath, + debugId: "00000000-0000-0000-0000-000000000001", + type: "minified_source" as const, + url: "~/app.js", + sourcemapFilename: "app.js.map", + }, + { + path: mapPath, + debugId: "00000000-0000-0000-0000-000000000001", + type: "source_map" as const, + url: "~/app.js.map", + }, + ]; + } + + test("default compression is DEFLATE", async () => { + const files = await makePair(); + const out = join(tmpDir, "bundle-default.zip"); + await buildArtifactBundle(out, files, { org: "o", project: "p" }); + + const bytes = await readFile(out); + expect(bytes.readUInt16LE(LOCAL_HEADER_METHOD_OFFSET)).toBe(8); + }); + + test("compression: 'stored' writes entries uncompressed", async () => { + const files = await makePair(); + const out = join(tmpDir, "bundle-stored.zip"); + await buildArtifactBundle(out, files, { + org: "o", + project: "p", + compression: "stored", + }); + + const bytes = await readFile(out); + expect(bytes.readUInt16LE(LOCAL_HEADER_METHOD_OFFSET)).toBe(0); + }); + + test("STORED archive is larger than DEFLATE for the same redundant input", async () => { + const files = await makePair(); + const deflateOut = join(tmpDir, "bundle-deflate.zip"); + const storedOut = join(tmpDir, "bundle-stored-size.zip"); + await buildArtifactBundle(deflateOut, files, { org: "o", project: "p" }); + await buildArtifactBundle(storedOut, files, { + org: "o", + project: "p", + compression: "stored", + }); + + const deflateSize = (await readFile(deflateOut)).length; + const storedSize = (await readFile(storedOut)).length; + // Sanity: redundant input should DEFLATE meaningfully smaller than + // STORED. If this ever fails, either the ZIP layout changed or the + // payload stopped being compressible. + expect(storedSize).toBeGreaterThan(deflateSize * 2); + }); +}); + describe("ChunkServerOptionsSchema", () => { test("accepts compression: [] (server opt-out)", () => { const result = ChunkServerOptionsSchema.safeParse({ diff --git a/test/lib/sourcemap/zip.test.ts b/test/lib/sourcemap/zip.test.ts index 24f079a83..346566d8b 100644 --- a/test/lib/sourcemap/zip.test.ts +++ b/test/lib/sourcemap/zip.test.ts @@ -98,17 +98,20 @@ describe("ZipWriter", () => { }); }); -describe("property: ZipWriter round-trip", () => { - test("arbitrary string content survives compress → extract", async () => { +describe.each([ + "deflate", + "stored", +] as const)("property: ZipWriter round-trip (%s)", (compression) => { + test("arbitrary string content survives write → extract", async () => { await fcAssert( asyncProperty( string({ minLength: 0, maxLength: 10_000 }), async (input) => { const zipPath = join( tmpDir, - `prop-${Date.now()}-${Math.random()}.zip` + `prop-${compression}-${Date.now()}-${Math.random()}.zip` ); - const zip = await ZipWriter.create(zipPath); + const zip = await ZipWriter.create(zipPath, { compression }); await zip.addEntry("data.txt", Buffer.from(input, "utf-8")); await zip.finalize(); @@ -121,7 +124,7 @@ describe("property: ZipWriter round-trip", () => { ); }); - test("arbitrary binary content survives compress → extract", async () => { + test("arbitrary binary content survives write → extract", async () => { await fcAssert( asyncProperty( // minLength: 1 — empty files have a separate unit test; @@ -130,9 +133,9 @@ describe("property: ZipWriter round-trip", () => { async (input) => { const zipPath = join( tmpDir, - `prop-bin-${Date.now()}-${Math.random()}.zip` + `prop-bin-${compression}-${Date.now()}-${Math.random()}.zip` ); - const zip = await ZipWriter.create(zipPath); + const zip = await ZipWriter.create(zipPath, { compression }); await zip.addEntry("data.bin", Buffer.from(input)); await zip.finalize(); @@ -146,6 +149,54 @@ describe("property: ZipWriter round-trip", () => { }); }); +describe("ZipWriter compression mode", () => { + // The local file header records the entry's compression method at + // offset 8 (16-bit LE): 0 = STORED, 8 = DEFLATE. Drift between this + // and the central directory copy would surface as an `unzip -p` + // failure in the extraction tests below. + const SYSB_HEADER_BYTES = 8; + const LOCAL_HEADER_METHOD_OFFSET = SYSB_HEADER_BYTES + 8; + + test("default is DEFLATE (back-compat)", async () => { + const zipPath = join(tmpDir, "default.zip"); + const repeating = "abcdef".repeat(1000); + const zip = await ZipWriter.create(zipPath); + await zip.addEntry("data.txt", Buffer.from(repeating)); + await zip.finalize(); + + const data = await readFile(zipPath); + expect(data.readUInt16LE(LOCAL_HEADER_METHOD_OFFSET)).toBe(8); + // Highly-redundant input should compress. + expect(data.length).toBeLessThan(repeating.length); + }); + + test("compression: 'stored' writes entries uncompressed", async () => { + const zipPath = join(tmpDir, "stored.zip"); + const repeating = "abcdef".repeat(1000); + const zip = await ZipWriter.create(zipPath, { compression: "stored" }); + await zip.addEntry("data.txt", Buffer.from(repeating)); + await zip.finalize(); + + const data = await readFile(zipPath); + expect(data.readUInt16LE(LOCAL_HEADER_METHOD_OFFSET)).toBe(0); + // STORED archive contains the raw bytes verbatim plus headers, + // so it must be at least as large as the input. + expect(data.length).toBeGreaterThan(repeating.length); + }); + + test("compression: 'stored' produces an extractable archive", async () => { + const zipPath = join(tmpDir, "stored-extract.zip"); + const content = "The quick brown fox\n".repeat(50); + const zip = await ZipWriter.create(zipPath, { compression: "stored" }); + await zip.addEntry("text.txt", Buffer.from(content)); + await zip.finalize(); + + const proc = Bun.spawnSync(["unzip", "-p", zipPath, "text.txt"]); + expect(proc.exitCode).toBe(0); + expect(new TextDecoder().decode(proc.stdout)).toBe(content); + }); +}); + describe("ZipWriter binary format", () => { test("starts with SYSB header followed by local file header", async () => { const zipPath = join(tmpDir, "sig.zip");