detect_binary_per_byte reads every changed file to EOF (no early exit, no size cap on watcher path)

[realhasanshoaib]

Which fff frontend?

Node SDK (@ff-labs/fff-node). The code is in fff-core, so it affects every frontend.

Has logs

Stack samples are below. I didn't enable the fff log file for this.

Description

FileItem::detect_binary_per_byte (crates/fff-core/src/types.rs, unchanged on main at 89c1927) reads the whole file to EOF. It has two problems:

1. No early exit. When a chunk contains a NUL byte, it calls set_binary(true) and keeps reading until EOF. One NUL byte is enough to decide.
2. No size cap on the watcher path. The bulk scan (bigram_filter.rs) skips files above MAX_FFFILE_SIZE. But handle_create_or_modify and add_new_file call detect_binary_per_byte on every created or modified file of any size. So each write to a large text file re-reads the whole file: think of an append-only log, a JSONL trace or a large generated file under the root.

In practice this is the main CPU cost when the root is a large folder that changes a lot. T3 Code embeds fff for its workspace index. With a ~1.65M-file non-git parent folder of active repos (macOS, Apple M4), sample of the host process showed:

fff-watcher-own (busy in ~50% of samples):
  FilePicker::handle_file_modify
    -> FileItem::detect_binary_per_byte -> open()   1308
    -> FileItem::detect_binary_per_byte -> read()   1252

fff-scan (busy in 100% of samples):
  ScanJob::run -> bigram_filter::sniff_binary_for_non_indexable
    -> FileItem::detect_binary_per_byte -> read()   4229
    -> FileItem::detect_binary_per_byte -> open()   1549

I ran a standalone fff-node harness on that folder with T3's options (disableContentIndexing: true, disableMmapCache: true). Upgrading 0.9.4 → 0.11.0 brought the process from 80% to 47% of a core, thanks to the #751 rescan throttle, which is great. In both versions, most of the remaining CPU is this sniff, not the scans: 7 scans of about 2–4 s each took 191 s of CPU in 0.9.4.

Suggested fix (a few lines):

Ok(n) => {
    if detect_binary_content(&chunk[..n]) {
        self.set_binary(true);
        break;
    }
}

Also cap the bytes read: either skip files above MAX_FFFILE_SIZE on the watcher path, as the scan already does, or read only the first N KB as git does (it checks the first 8000 bytes). The second option changes classification for text files that have a NUL byte late in the file. A cap on the watcher path alone keeps today's results for anything the scan already classifies.

Downstream report: pingdotgg/t3code#14536

[dmtrKovalenko]

I agree largely, I've been researching the way this actually works in the linux file command for example and there is unfortunately no other way to detect if file is not a binary if you are scanning every single byte till the end.

But I agree this is proably too much I will warp my head around it how to make it faster

[dmtrKovalenko]

Yes I think MAX_FFFILE_SIZE fix is the correct safeguard @gustav-fff go fix it

[gustav-fff]

[triage-bot] LINKED PR: #14543 (open) — pingdotgg/t3code#14543

Only linked PR is downstream in pingdotgg/t3code, not in this repo. It bumps @ff-labs/fff-node 0.9.4 -> 0.11.0 to pick up the watcher rescan throttle from #751, re-targets the vendored patch and fixes the Windows native-load probe path. It does not touch FileItem::detect_binary_per_byte and its own body says so explicitly: the sniff cost reported here stays unfixed.

So: no upstream PR exists for this issue. crates/fff-core/src/types.rs still reads to EOF with no early exit and no size cap on the watcher path. @dmtrKovalenko greenlit the MAX_FFFILE_SIZE safeguard above - that work is unclaimed, not covered by #14543.

Honk-Honk 🪿

[dmtrKovalenko]

@gustav-fff I mean in the detect binary path!

[gustav-fff]

[triage-bot] DIRECTED: fix landed in #899

Guard is now inside FileItem::detect_binary_per_byte (crates/fff-core/src/types.rs:563), not at call sites:

• size > MAX_FFFILE_SIZE -> return before open(). Covers watcher modify (file_picker.rs:1694) and add (file_picker.rs:1741); scan path already skipped these, so its results don't change.
• break after first chunk containing NUL.

Downstream consumers (prefilter.rs:78, bigram_filter.rs:1123, file_picker.rs:2154, :2228) all check size anyway, so >10 MiB files staying is_binary=false changes nothing. The scan already left them that way.

Regression test detect_binary_tests::skips_files_above_max_size fails without the guard:

assertion failed: !classify(&content)

cargo test -p fff-search green.

Honk-Honk 🪿