Cap non-editor unbounded async_fs reads (APP-4801 remainder) - #15450
Open
warp-agent-staging[bot] wants to merge 1 commit into
Open
Cap non-editor unbounded async_fs reads (APP-4801 remainder)#15450warp-agent-staging[bot] wants to merge 1 commit into
warp-agent-staging[bot] wants to merge 1 commit into
Conversation
APP-4801 documents a jemalloc-symbolized heap profile pattern (async_fs::read_to_string -> std::fs::read_to_string -> String::try_reserve_exact) where reading a pathologically large file reserves its entire on-disk size (tens of GiB) in one shot, tripping the "Excessive memory usage detected" Sentry alert (issue 7259255054). This is the remainder of #15052 (closed unmerged), which was consolidated into #15038 (fix: cap unbounded file reads in FileModel, APP-4519) for the editor/FileModel path only. #15038 does not cover the sites below, which are outside APP-4519's scope; re-proposing them here per #15052's closing comment. Added the shared `warp_util::file::read_to_string_capped`/`read_capped` helpers: open the file once and enforce the byte ceiling during the read itself (via `AsyncReadExt::take`), never via a separate stat-based accept/reject decision. This avoids the fail-open TOCTOU gap of a stat-then-reread-by-path pattern (a file can grow/be atomically replaced between the two, or be a FIFO/device that reports an unrelated `stat` length). Capped call sites, each with a doc comment justifying its cap: - app/src/drive/import/nodes.rs::parse_file - Drive import Notebook (read_to_string) and Workflow (read) reads, 25 MiB. - app/src/ai/mcp/file_mcp_watcher.rs::parse_mcp_config_file - MCP config reads, 1 MiB. Oversized -> FileMCPConfigDiagnosticKind::Read, not Missing. - crates/repo_metadata/src/repositories.rs::find_git_repo - .git gitfile reads, 1 MiB (sized for Windows long-path gitdir lines). Oversized is treated like any other unreadable gitfile. - app/src/settings/import/alacritty_parser.rs::from_file_bounded_depth - Alacritty config reads, 1 MiB. Oversized -> ConfigError::FileIOError, not FileNotFoundError. - app/src/ai/metadata_project_rules.rs::read_project_rule_contents - project rule file reads, 1 MiB (matches REMOTE_CONTEXT_MAX_FILE_BYTES). - app/src/ai/skills/file_watchers/skill_watcher.rs::read_local_project_skill_contents - local skill file reads (previously synchronous fs::read_to_string with the same bug), 1 MiB, matching the remote path's existing cap. Regression tests carried over for every site above, plus the warp_util helper's own tests (including a /dev/zero and a FIFO test proving the cap is enforced independent of what `stat` reports). Note: `crates/warp_util/src/file.rs` overlaps with #15038, which adds its own `MAX_LOADABLE_FILE_SIZE_BYTES` / `FileLoadError::TooLarge` there. This change is additive and independent (new free functions, no changes to existing types), but the file will conflict textually with #15038 - flagging so whoever lands second can reconcile easily. Co-Authored-By: Warp Agent <agent@warp.dev>
Contributor
Author
|
This PR was generated with Warp. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.


Description
APP-4801 documents a jemalloc-symbolized heap profile pattern (
async_fs::read_to_string->std::fs::read_to_string->String::try_reserve_exact) where reading a pathologically large file reserves its entire on-disk size (tens of GiB) in one shot, tripping the "Excessive memory usage detected" Sentry alert (issue 7259255054).History: #15052 originally implemented this ticket end-to-end, including the
crates/warp_files/FileModeleditor-buffer read path. #15052 was closed unmerged: another factory run consolidated theFileModeleditor path into #15038 (fix: cap unbounded file reads in FileModel to prevent multi-GB memory spikes, APP-4519, branchoz/app-5343-cap-file-read-size), which is still an open draft. #15038's own closing comment on #15052 noted that #15052 "also caps several unrelated read paths ... that are outside APP-4519's scope and not covered by #15038 ... worth re-proposing separately (APP-4801) if still wanted." This PR is that re-proposal: it carries forward only the sites #15038 does not cover.crates/warp_files/FileModel(the editor buffer load path) is intentionally NOT included here — that's #15038's territory. Do not re-add it in this PR.Linked Issue
Linear: APP-4801
Closed predecessor: #15052
ready-to-spec/ready-to-implementlabel applicable.Changes
crates/warp_util/src/file.rs: sharedread_to_string_capped/read_cappedhelpers. Each opens the file once and reads at mostmax_bytes + 1bytes from that single handle viaAsyncReadExt::take, rejecting if that limit is reached;metadata()is still queried but only to pre-size the buffer (itself clamped to the cap), never to decide accept/reject. This avoids the fail-open TOCTOU gap of stat-then-reread-by-path (a file can grow/be atomically replaced between the two calls, or be a FIFO/character device that reports an unrelatedstatlength).MAX_LOADABLE_FILE_SIZE_BYTESconstant and aFileLoadError::TooLarge { size_bytes, limit_bytes }variant (via acheck_not_too_largestat-then-reread guard — the same fail-open pattern this PR avoids). This PR's additions are independent (new free functions, no changes to existing types), but the file will conflict textually with fix: cap unbounded file reads in FileModel to prevent multi-GB memory spikes (APP-4519) #15038. Whoever lands second should reconcile by keeping both: this PR'sread_to_string_capped/read_cappedalongside fix: cap unbounded file reads in FileModel to prevent multi-GB memory spikes (APP-4519) #15038'sFileLoadError::TooLarge/MAX_LOADABLE_FILE_SIZE_BYTES.FileLoadError::TooLargevariant rather than a genericio::Error::other, which lets callers distinguish "too large" from other I/O failures. Adopting that shape for this PR's six sites would require changingread_to_string_capped/read_capped's return type away fromio::Result, which ripples into every call site's existingerr.kind() == ErrorKind::NotFoundbranching. Kept the currentio::Error-based shape to avoid that churn; noting as a follow-up once fix: cap unbounded file reads in FileModel to prevent multi-GB memory spikes (APP-4519) #15038 lands and the two files reconcile.app/src/drive/import/nodes.rs: caps theFileType::Notebook(read_to_string) andFileType::Workflow(read) reads inparse_fileat 25 MiB. No server-side Drive upload size limit was found in this repo to align with; picked a cap generous enough for hand-authored markdown/YAML but well below the pathological range.app/src/ai/mcp/file_mcp_watcher.rs: caps MCP config reads (.mcp.json, Claude/Codex/Agents configs) at 1 MiB. An oversized file now returnsFileMCPConfigParseOutcome::Error(FileMCPConfigDiagnosticKind::Read), notMissing, so a temporarily-huge config file doesn't look like the user removed their MCP servers.crates/repo_metadata/src/repositories.rs: caps.gitgitfile reads (worktrees/submodules) at 1 MiB (sized for Windows long-path installations, whosegitdir:line can run well beyond a few KiB — a tighter cap would makefind_git_repotreat a valid worktree/submodule as "not a gitfile" and silently walk past the real repository). An oversized/unreadable file is treated like any other unreadable gitfile (not a valid gitfile — keep walking up).app/src/settings/import/alacritty_parser.rs: caps Alacritty config reads at 1 MiB. An oversized file maps toConfigError::FileIOError, notFileNotFoundError.app/src/ai/metadata_project_rules.rs: caps project rule file reads at 1 MiB (matchingREMOTE_CONTEXT_MAX_FILE_BYTES, used for the same kind of content on the remote path). The local-read loop is extracted intoread_local_rule_contentsfor direct unit testing.app/src/ai/skills/file_watchers/skill_watcher.rs:read_local_project_skill_contentsused a synchronousfs::read_to_stringwith the same unbounded-reservation bug. Made it async and capped it atREMOTE_CONTEXT_MAX_FILE_BYTES(1 MiB) to match the remote read path's existing cap.Testing
warp_util::file, Drive importparse_file, MCP config parsing,find_git_repo, Alacritty config parsing, project rule reading,skill_watcher.rs's local skill read) asserting an oversized file is rejected/skipped rather than read wholesale, and that pre-existing error semantics (e.g.NotFoundvs. other IO errors) are preserved.warp_util::file's own tests include a/dev/zerocase (character device reporting astatlength of 0 while yielding unbounded data) and a FIFO case (alsostatlength 0, content that grows past the cap while the read is in progress) — both correctly reject rather than hanging/growing unbounded../script/format— clean.cargo clippy --workspace --exclude warp_completer --all-targets --tests -- -D warnings— clean.cargo clippy -p warp --all-targets --tests -- -D warnings— clean.cargo clippy -p warp_completer --all-targets --tests -- -D warnings— clean.cargo nextest run --no-fail-fast -p warp_util -p repo_metadata --features local_fs,test-util— 258/258 pass (2 skipped); targeted regression tests in-p warp— 27/27 pass.cargo nextest run --no-fail-fast -p warp --features local_fs(full crate) — 6566/6569 pass; the 3 failures are pre-existing/environment-only and unrelated to this diff (e.g. an unauthorized sandboxnscnamespace CLI call).Agent Mode
CHANGELOG-BUG-FIX: Fixed several file-import and config-reading code paths that could reserve gigabytes of memory up front when reading a pathologically large file.