Fix >4GiB log bundle exports and harden create_log_bundle_zip (APP-5496) - #15267
Fix >4GiB log bundle exports and harden create_log_bundle_zip (APP-5496)#15267warp-agent-staging[bot] wants to merge 2 commits into
Conversation
Sentry's heap profile for this issue attributed ~8 GB of live heap to
zip::write::GenericZipWriter::switch_to, called from
create_log_bundle_zip. Measuring the actual function (release build,
VmHWM before/after) shows peak memory stays flat (~4.5 MB) whether the
input is 150 MB or 6 GB across multiple log files: std::io::copy already
streams through a fixed buffer, and switch_to frees the outgoing
compressor before returning. The leaf-attributed leak does not
reproduce; the profile's leaf/immediate-caller pairing looks
mis-attributed for this call site.
What does reproduce, and what this fixes:
- Log files can exceed 4 GiB in a long verbose session
(LogConfig::max_file_size_bytes defaults to None in production), and
SimpleFileOptions::default() leaves large_file: false, so ZipWriter
aborted the entry once it crossed ZIP64_BYTES_THR ("Large file option
has not been set"). Now large_file is decided per entry from the
source file's metadata length, so oversized entries use ZIP64 while
small entries stay in the more widely-compatible non-ZIP64 format.
- The crate pulled in zip's full default feature set (bzip2, lzma, xz,
zstd, aes-crypto, deflate64, and zopfli), none of which this
write-only, Deflate-only code path uses. Trimmed to
default-features = false with only deflate-flate2 (+ flate2's
rust_backend). Note: crates/node_runtime (pulled in via crates/lsp)
also depends on zip with full default features, and Cargo unifies
package features across a build's whole dependency graph, so the
shipped `app` binary still links the fuller zip feature set today via
that unrelated crate; this trim only takes full effect for targets
that don't also pull in node_runtime.
- Added an explicit comment documenting that the copy loop is already
bounded/O(1) in input size (fixed-size buffer via std::io::copy).
- Added an in-flight guard so a second concurrent/repeated export
request fails fast with a clear error instead of racing a second
archive build over the same files; both existing front-end callers
already surface Result errors as toasts/hints, so no caller changes
were needed.
Tests: a sparse-file regression test for the >4 GiB entry path, and a
test for the in-flight export guard (rejects concurrent calls, and
recovers after a failed export).
Co-Authored-By: Warp <agent@warp.dev>
There was a problem hiding this comment.
Overview
Selects ZIP64 per entry so log bundles no longer fail once a log passes 4 GiB, and adds a concurrency guard plus a zip feature trim alongside it. The ZIP64 fix is correct, tested, and should land; two of the changes around it need a decision from you.
Concerns
- The in-flight guard is a fix for a different bug, and it reports itself to Sentry. Both call sites funnel the error into
report_error!(app/src/workspace/view.rs:6718,crates/warp_tui/src/terminal_session_view.rs:4600), so an ordinary double-click on "View Warp logs" now files a Sentry event — on a ticket that arrived from Sentry noise. The mechanism itself is sound, but the underlying bug is a TOCTOU betweenzip_path.exists()andFile::createatnative.rs:516, whichFile::create_newor a unique suffix would fix in less code and would let the second export succeed instead of refusing. - The feature trim changes nothing in any build this workspace produces.
cargo tree -e featuresconfirmsbzip2,lzma,xz,zstd,aes-crypto,deflate64anddeflate-zopfliall stay active becausecrates/node_runtime(reached fromappviacrates/lsp) still pullszipwith default features, and the workspace ispublish = false, so no external consumer buildswarp_loggingalone. Keeping it as the correct minimal declaration is defensible; so is deferring it to a PR that also trimsnode_runtimeand can show a binary-size delta. - CI has never run on this branch. All substantive jobs report
skippingwhile the PR is a draft (.github/workflows/ci.yml:46-51), so every platform claim about the new 4 GiB test is still unvalidated — notably onwindows-latest-large, where NTFS does not makeset_lensparse and the file reserves 4 GiB of real disk.
Verdict
Checks: build pass, tests pass (26/26 in warp_logging locally), CI not run (draft — all jobs skipped), visual proof n/a (backend-only)
Found: 0 critical, 5 important, 5 suggestions, 3 nits. The two decisions above are yours; the remaining findings (changelog command name, an over-claiming doc comment, a missing success-path assertion, a ZIP64 size round-trip assertion, and the 4 GiB test's CI cost) are being revised now.
Responding as wilson: Open session · View factory task
| if LOG_BUNDLE_EXPORT_IN_PROGRESS.swap(true, Ordering::AcqRel) { | ||
| return Err(anyhow::anyhow!( | ||
| "A log bundle export is already in progress" | ||
| )); | ||
| } | ||
| let _guard = LogBundleExportGuard; |
There was a problem hiding this comment.
The guard is correct on every path I checked — swap cannot interleave with the let _guard, and the RAII drop fires on all ? early returns. My concern is its blast radius: both callers pass this error to report_error!, so a user double-clicking "View Warp logs" now files a Sentry event, and in the TUI they never even see the reason (terminal_session_view.rs:4601 renders a fixed LOG_BUNDLE_FAILED_HINT). The bug worth fixing here is the TOCTOU at line 516, where two exports in the same second both pass zip_path.exists() and then both File::create the same path; File::create_new or a unique suffix fixes that in less code and lets the second export succeed.
| zip = { version = "2.1", default-features = false, features = ["deflate-flate2", "flate2"] } | ||
| flate2 = { version = "1.0", default-features = false, features = ["rust_backend"] } |
There was a problem hiding this comment.
The declaration is right, including the non-obvious part: zip pins flate2 with default-features = false, so the separate direct flate2 dependency is genuinely load-bearing for rust_backend. But it reads as an unused direct dependency — please add a comment saying it exists solely to select zip's DEFLATE backend. Separately, this trim has no effect on any binary built here while node_runtime still pulls zip with default features, so it is worth deciding whether it belongs in this PR or in one that trims both and can demonstrate the size delta.
… tests - Extract needs_zip64() with a 64 MiB margin below ZIP64_BYTES_THR, since the active log can grow mid-copy and zip separately checks compressed size against the same threshold. Unit-test the boundary. - Ignore bundle_zip_supports_entries_over_4gib by default (writes a real 4 GiB+ entry; not sparse on Windows/NTFS and >99% of the crate's suite time), matching the existing #[ignore] convention. Strengthen it to assert the resulting entry's uncompressed size. - Clean up the export failure path: remove the orphaned partial zip on error, and add .with_context() so IO errors name the file involved. - Fix the LOG_BUNDLE_EXPORT_IN_PROGRESS doc comment: it only covers a repeated call within one process (LOG_STATE/the static are process-local), not the GUI and TUI racing across processes. Correct the TUI command name to /view-logs. - Strengthen create_log_bundle_zip_rejects_concurrent_exports_and_recovers_after_failure to also assert the guard clears after a successful export, and note why this test needs process isolation (safe under nextest). - Trim comments that narrated the investigation rather than the code: the copy()-adjacent comment now just says to keep it streaming; the Cargo.toml comment keeps the "why deflate-flate2 only" rationale and drops the zopfli-internals explanation. Document the flate2 line as load-bearing (it supplies the backend zip's feature doesn't select). Co-Authored-By: Warp <agent@warp.dev>
Description
Fixes APP-5496 in
warp_logging::imp::create_log_bundle_zip(crates/warp_logging/src/native.rs), the function behind "View Warp logs" (Help menu) and the Warp Agent CLI's/view-logscommand:LogConfig::max_file_size_bytesdefaults toNonein production, so a long, verbose session'swarp.logcan exceed 4 GiB.SimpleFileOptions::default()leftlarge_file: false, soZipWriteraborted the entry once it crossedZIP64_BYTES_THRwith"Large file option has not been set". Fixed by decidinglarge_fileper entry from the source file's metadata length (with a margin below the raw threshold, since the active log can still be growing mid-copy and zip separately checks compressed size against the same threshold), so oversized entries use ZIP64 while small entries stay in the more widely-compatible non-ZIP64 format.CompressionMethod::Deflated, but the crate pulled in zip's full default feature set (bzip2,lzma,xz,zstd,aes-crypto,deflate64, and thezopfliDEFLATE backend). Trimmed todefault-features = falsewith onlydeflate-flate2(+flate2'srust_backend, the backend that feature needs but doesn't select on its own). Caveat, left for the reviewer to weigh in on:crates/node_runtime(pulled into theapp/GUI binary transitively viacrates/lsp) also depends onzipwith full default features, and Cargo unifies package features across a build's whole dependency graph — confirmed the shippedappbinary links both. So this trim is the correct, minimal declaration forwarp_loggingitself, but doesn't reduce the feature set actually linked intoapptoday.std::io::copycall was already O(1) in input size; left a one-line comment so a future change doesn't turn it into a full in-memory buffer.AtomicBoolguard (RAII-released, so a failed or panicking export doesn't wedge future ones) so a second concurrent request in the same process fails fast with"A log bundle export is already in progress"rather than racing a second archive build over the same files. Didn't touch either caller: the GUI toast already interpolates{err}and will show this text; the TUI hint (crates/warp_tui/src/terminal_session_view.rs:4601) renders a fixedLOG_BUNDLE_FAILED_HINTstring, so this specific message won't reach a Warp Agent CLI user today — whether to plumb it through is part of the requester's call..zipbehind, and I/O errors now name the file that failed rather than surfacing a bare OS error.I also measured whether this function can actually produce the ~8 GB live-heap growth Sentry's profile attributed to
zip::write::GenericZipWriter::switch_to: peak RSS stayed flat (~4.5 MB) across 150 MB and 6 GiB of input in this single-threaded, sequential-entry loop, so that leak does not reproduce here; recorded on the Linear ticket, and the fixes above stand on their own regardless.Two open review threads (inline on this PR) are left for the requester to decide: the in-flight guard's scope/blast radius, and whether the zip feature trim belongs in this PR given the
node_runtimecaveat above.Linked Issue
Linear: APP-5496
ready-to-spec/ready-to-implementGitHub issue exists for this.Testing
needs_zip64_respects_margin_below_threshold(new): unit-tests the extracted threshold predicate at its boundary.bundle_zip_supports_entries_over_4gib(new,#[ignore]d — writes a real 4 GiB+ entry, ~6-15s and not sparse on Windows/NTFS, so it's excluded from the defaultcargo nextestrun): exercises the >4 GiB path end-to-end and asserts the resulting entry's uncompressed size.create_log_bundle_zip_rejects_concurrent_exports_and_recovers_after_failure(new): rejects a concurrent call, and confirms the in-flight guard clears after both a failed and a successful export.cargo nextest run -p warp_logging: 26/26 passed (the ignored test verified separately, passing in ~6s)../script/format --check: clean.cargo clippy -p warp_logging --all-targets --tests -- -D warnings: clean.cargo clippy --workspace --exclude warp_completer --all-targets --tests -- -D warnings: clean.cargo clippy -p warp --all-targets --tests -- -D warnings: clean (confirms theappbinary, which links bothnode_runtimeandwarp_logging, still builds under the trimmed features)../script/presubmit(clang-format/wgslfmt/PowerShell lint/full-workspacenextest) — this change only touchescrates/warp_logging, and the targeted checks above cover it; the full workspace test suite was not re-run end-to-end due to time/resource constraints in this sandbox../script/run— not done; this is headless, non-UI logic and the automated tests above exercise the actual code path directly.Screenshots / Videos
Not applicable — backend-only change, no UI surface.
Agent Mode