Cap ck_files::read at 64 MiB with typed TooLarge error #41

Manually merged
atanabe merged 2 commits from feat/041-cap-ck-files-read-at-64-mib-with-typed-toolarg into master 2026-06-09 02:00:00 +02:00
Owner

Cap ck_files::read at 64 MiB with typed TooLarge error

Along the way:

  • Reconcile §17 with landed drift fixes for #6 and #10

Touches 6 files: ck-files/COVERAGE_EXEMPT.md, ck-files/README.md, ck-files/src/lib.rs, ck-files/tests/errors.rs, ck-files/tests/read_write.rs, ckeditor-files.md.

Cap ck_files::read at 64 MiB with typed TooLarge error Along the way: - Reconcile §17 with landed drift fixes for #6 and #10 Touches 6 files: `ck-files/COVERAGE_EXEMPT.md`, `ck-files/README.md`, `ck-files/src/lib.rs`, `ck-files/tests/errors.rs`, `ck-files/tests/read_write.rs`, `ckeditor-files.md`.
atanabe requested changes 2026-06-09 01:55:00 +02:00
Dismissed
atanabe left a comment

I do not want to merge while the error path in ck-files/tests/read_write.rs is silent. The rest is fine.

I do not want to merge while the error path in `ck-files/tests/read_write.rs` is silent. The rest is fine.
@ -195,2 +240,4 @@
/// ```
pub fn read(path: &Path) -> Result<String, FileError> {
let size = fs::metadata(path)?.len();
if size > MAX_FILE_SIZE {
Owner

I had to read MAX_FILE_SIZE twice. Nothing wrong with it, but one line of comment would save the next person the second read.

I had to read `MAX_FILE_SIZE` twice. Nothing wrong with it, but one line of comment would save the next person the second read.
@ -118,0 +166,4 @@
assert_eq!(p, path);
assert_eq!(size, over);
}
other => panic!("expected TooLarge, got {other:?}"),
Owner

This panics. Is other unreachable, or unreachable today? If it is the second I would rather see an error the caller can handle.

This panics. Is `other` unreachable, or unreachable today? If it is the second I would rather see an error the caller can handle.
@ -55,0 +62,4 @@
fn read_errors_on_a_directory_path() {
let fx = Fixture::new();
let path = fx.path("a-directory");
std::fs::create_dir(&path).unwrap();
Owner

std unwraps here. If this is reachable from user input it is a panic in front of a person rather than an error they can read. Is it provably there at this point, or should it carry a ??

`std` unwraps here. If this is reachable from user input it is a panic in front of a person rather than an error they can read. Is it provably there at this point, or should it carry a `?`?
atanabe approved these changes 2026-06-09 01:55:00 +02:00
atanabe left a comment

That is it for ck-files/tests/read_write.rs. Thanks for the second pass.

That is it for `ck-files/tests/read_write.rs`. Thanks for the second pass.
Author
Owner

It is reachable. The function in ck-files/tests/read_write.rs returns a Result now and the caller reports it, so the unwrap on std is gone.

It is reachable. The function in `ck-files/tests/read_write.rs` returns a Result now and the caller reports it, so the unwrap on `std` is gone.
Author
Owner

Unreachable today, which is not the same thing. other returns an error variant now, and ck-files/tests/errors.rs has a test that drives the path.

Unreachable today, which is not the same thing. `other` returns an error variant now, and `ck-files/tests/errors.rs` has a test that drives the path.
Author
Owner

Comment added on MAX_FILE_SIZE in ck-files/src/lib.rs. It was obvious when I wrote it and it will not be in a month.

Comment added on `MAX_FILE_SIZE` in `ck-files/src/lib.rs`. It was obvious when I wrote it and it will not be in a month.
Author
Owner

Done, and I added the test for ck-files/tests/read_write.rs that would have caught it.

Done, and I added the test for `ck-files/tests/read_write.rs` that would have caught it.
Closes the drift flagged in ckeditor-files.md §17 row #10. The v2
decision was to cap reads at 64 MiB with a typed error; the shipped
code never had the cap, so `ck_files::read` would happily allocate a
multi-GB string for a stray log file and OOM the editor on the first
read. An MCP fetch endpoint that eventually rides this primitive has
the same exposure.

The cap is a public const so callers and the design doc can anchor to
it; `read` probes `fs::metadata(path)?.len()` before the `fs::read`
call, so oversized files never allocate a buffer. A new
`FileError::TooLarge { path, size }` variant carries both the
offending path and the observed size so error messages do not have to
re-stat.

New tests in `ck-files/tests/errors.rs`:

- `max_file_size_is_exactly_64_mib` pins the literal constant value
  (67,108,864). Without this, a mutant that turned `64 * 1024 * 1024`
  into `64 + 1024 * 1024` would shrink the cap to ~1 MiB and slip past
  the symbolic at-cap / above-cap tests (both reference
  `MAX_FILE_SIZE` symbolically, so they scale with the mutant).
- `read_file_at_cap_succeeds` / `read_file_above_cap_errors` use
  sparse files (`File::set_len`) so the boundary assertion stays fast
  without writing 64 MiB of real bytes.
- `display_for_too_large_shows_the_path_and_size` and
  `error_source_for_too_large_is_none` round-trip the new variant
  through the Display and Error::source contracts.
- `read_errors_on_a_directory_path` in `tests/read_write.rs` covers
  the `fs::read(path)?` error arm that the happy-path reads don't
  touch (metadata succeeds on a directory, the cap check passes,
  fs::read fails with "is a directory").

Gate status: lint 0/0, `cargo test --workspace` all green,
`cargo llvm-cov --package ck-files` at 100% line / 99.61% region
(up from 99.39%, 2 regions exempt — the two `write_atomic` fsync
error arms that require kernel-level fault injection),
`cargo mutants --package ck-files` at 28/28 caught (100% kill, up
from 95.45%). No baseline movement.
Rows #6 (hash-and-ignore) and #10 (64 MiB cap) are no longer drifted
from the shipped code — commits 0365580 and ad87bdb landed the v2
choices on 2026-04-20. Move both rows out of the "Decisions overruled
by implementation" subsection and append an "implemented on DATE in
commit SHA" note to the corresponding rows in the main §17 table.

Updates the status preamble at the top of the doc to name which drifts
closed and which remain open (#8 closed-files-auto-open is still a
prose-reconciliation task, #13 canonicalized paths is still backlog).
Also drops the stale "`suspended`" reference in row #13 — that map is
gone as of commit 0365580.

Doc-only. No behaviour change. No baseline movement.
atanabe manually merged commit 370f3bed23 into master 2026-06-09 02:00:00 +02:00
Sign in to join this conversation.
No reviewers
No labels
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
northgale/ckeditor!41
No description provided.