[michieldwitte]

gcov writes its json output before it bails out on a gcno whose version it does not support, and that output holds a garbage working directory string. gcov 12 and 15 name it after the gcno stem instead of after the whole file name, so grcov does not find it where it looks for it and walks the working dir instead, which took the leftover for the output of the next gcno: the unwrap of its json then killed the parser thread and made grcov exit 1.

Remove what a failed gcov run left behind, and report a malformed json as a parsing error rather than panicking on it.

[marco-c]

An alternative would be to write files in a temporary directory and then move it.
WDYT @calixteman?

[calixteman]

Yep, agreed it'd be a way better.

[michieldwitte]

Done, with one deviation: each gcov run now gets a tempfile::tempdir_in(working_dir) and the output is parsed straight out of it, so there's nothing to move — the move would only serve to get the files into a directory we then walk, and the walk can just target the temp directory.
A failed run's leftovers go out with the TempDir drop, so the error path needs no cleanup either.

[marco-c]

This way we create a new temporary directory for each file that we have to parse, it will be quite expensive

[michieldwitte]

The test was run on Linux
(kinda worth testing on Windows cause as far as I can remember the file/folder operations in Windows are slow compared to Linux).

Let me know your preference and I'll update the MR 👍

Claude:

I implemented and benchmarked both approaches.

Short version: they perform the same, so the choice is a design one, and the benchmark turned up a separate accumulation bug worth fixing either way.

A — run gcov in a temp directory and parse out of it (what this PR does now)

let gcov_tmp_dir = match tempfile::tempdir_in(working_dir) {
    Ok(dir) => dir,
    Err(e) => {
        error!("Error when creating the gcov directory: {e}");
        continue;
    }
};
let gcov_dir = gcov_tmp_dir.path();
if let Err(e) = run_gcov(&gcno_path, branch_enabled, gcov_dir) {
    error!("Error when running gcov: {e}");
    continue;
}

gcov_dir then replaces working_dir where the output is read — the gcov_path join and WalkDir::new(…). Nothing needs cleaning up: the TempDir drop takes the directory and its contents on the error path and the happy path alike, so
remove_dir_files, both fs::remove_file(…).unwrap() calls and use std::fs all go away.

2 files changed, +17 −8.

B — write to a temp directory, then move the output into the working dir

// beside the working dir, not inside it — see the trap below
let gcov_tmp_parent = working_dir.parent().unwrap_or(working_dir);
let gcov_tmp_dir = match tempfile::tempdir_in(gcov_tmp_parent) { /* … */ };
if let Err(e) = run_gcov(&gcno_path, branch_enabled, gcov_tmp_dir.path()) {
    error!("Error when running gcov: {e}");
    continue;
}
if let Ok(entries) = fs::read_dir(gcov_tmp_dir.path()) {
    for entry in entries.flatten() {
        let _ = fs::rename(entry.path(), working_dir.join(entry.file_name()));
    }
}

Everything downstream stays as it is today: parse from working_dir, remove each file by hand.

1 file changed, +22 −1 — but +40 −23 across 2 files once it also carries the removal fix it needs to match A.

Performance: equal

libgit2 at HEAD built with gcc-15 --coverage -O0 (701 TUs, 128k lines of C), its offline ctest suite run so the counters are real, plus a 6× replica for resolution. Release builds, interleaved rounds, byte-identical output in every run.

	master	A (parse in place)	B (write + move)
701 gcno, --threads 1, 10 rounds	2.346 s	2.380 s	2.311 s
4206 gcno, --threads 1, 8 rounds	13.976 s	14.097 s	13.966 s
4206 gcno, 19 threads, 20 rounds	1.250 s	1.255 s	1.256 s

Every difference is ≤1.5 % and inside the run-to-run spread (σ 0.16–0.41), and the ordering is not even consistent — A is marginally slower in two of the three, B in the other. For scale, a temp-dir create+remove is 8.5 µs on tmpfs and 17.8 µs
on ext4, against a gcov run of ~830 µs: about 1 % of the process it accompanies, and less on a real filesystem.

One trap in B

With the temp directory inside working_dir, WalkDir::new(working_dir).min_depth(1) yields the directory itself. .tmpXXXXXX has no extension, so
gcov_path.extension().unwrap() panics in every consumer thread, and the producer then blocks forever on the bounded channel — grcov hangs with
nothing on stderr. Putting the temp dir beside working_dir avoids it. A cannot hit it, because the walk only ever sees one run's output.

A separate bug this turned up

try_parse! expands to continue, which in the MultipleFiles branch skips the fs::remove_file(gcov_path).unwrap() below it. So gcov output that fails to parse stays in working_dir, and every later gcov run in that thread re-walks and
re-parses it. On the libgit2 build that was 12,374 logged parse errors against 132 genuinely bad files — about 94 re-parses each — and 14.0 s → 2.3 s once fixed.

A fixes this implicitly: a dropped directory cannot leak. B needs the removal moved so that it runs even when the parse fails. It is worth landing on its own merits regardless of which approach wins here.

The unparseable files are #908 — gcc-15 emits negative counts in its gcov JSON. Without this PR's parser.rs change they are unwrap() panics, and on that libgit2 build master exits 1 having written no coverage at all; with --threads 1 it
deadlocks.

What each approach actually fixes

	failed gcov leaves output behind	unparseable output re-walked forever
remove_dir_files (this PR's first version)	✅	❌
master + removal fix	❌	✅
A — parse in place	✅	✅
B — write + move, + removal fix	✅	✅

The removal fix alone is not a substitute: a leftover from a bailed-out gcov parses fine and simply gets attributed to the next gcno.

Recommendation

A, since speed does not separate them: it fixes both leaks for free, has no placement constraint, and gets there in +17 −8 where B needs +40 −23. Happy to switch to B if you prefer it, and happy to push the benchmark variants if you want
to reproduce any of this.

Reproduction

git clone --depth 1 https://github.com/libgit2/libgit2
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug -DCMAKE_C_COMPILER=gcc-15 \
      -DCMAKE_C_FLAGS="--coverage -O0" -DBUILD_TESTS=ON -DBUILD_CLI=OFF \
      -DUSE_HTTPS=OFF -DUSE_NTLMCLIENT=OFF -DUSE_SSH=OFF
ninja -C build && (cd build && ctest -j8)   # the offline suite; online/proxy/auth need fixtures
grcov . -t covdir -o /dev/null              # 701 gcno, gcov 15.2

The 6× corpus is the same tree's *.gcno/*.gcda copied into six independent roots.

[marco-c]

I guess there's no difference because in one case you're creating a new directory for each file to parse, in the other case you're creating the directory once but then moving files one by one.
What if you instead create a temporary directory once, and then move the whole directory?

[michieldwitte]

Claude:

Implemented and measured it — it comes out the same, and the reason is that B already does what you're describing.

B creates the directory once per gcov run, not per file, and it moves exactly one file. gcov -i writes a single .gcov.json.gz per gcno, headers included — I checked on real libgit2 translation units (worktree.c, tree.c, tree-cache.c, transport.c, transaction.c, trailer.c): one output file each. So "moving files one by one" is already one rename, and moving the directory instead swaps one rename for one rename.

Measured anyway, on 4206 real gcno (libgit2 built with gcc-15 --coverage, offline ctest suite run, ×6 for resolution). All four produce byte-identical output and exactly 132 parse errors:

variant	--threads 1, 8 rounds	σ	19 threads, 24 rounds (median)
master + removal fix	13.755 s	0.243	1.032 s
A — parse in place	13.916 s	0.179	1.017 s
B — move the files	13.851 s	0.173	1.009 s
C — move the whole directory	13.973 s	0.186	1.038 s

Sequential spread is ≤1.6 %, all inside σ. I'm quoting medians for the threaded runs because those go bimodal — individual runs land near either 1.0 s or 2.0 s regardless of variant, which swamps the means; the medians agree to within 3 %.

The part that can't be optimised away: the directory can't be created only once if each run is to be isolated. Once the contents — or the directory itself — have been moved out of the way, the next run needs an empty directory again, so it's a fresh mkdir per run either way. The alternative is reusing one directory and clearing it before each run, which costs the same unlinks, and that's the remove_dir_files version this PR started from.

A wrinkle specific to C: after the rename, the MultipleFiles walk has to target the moved directory rather than working_dir — otherwise it yields a directory entry and gcov_path.extension().unwrap() panics in every consumer thread, and the producer then blocks on the bounded channel (grcov hangs, nothing on stderr — I hit this first time round). But once the walk targets the moved directory, the rename has bought nothing: that is A with an extra rename in front of it.

Which is the general shape of it. Any variant that ends in "parse from a directory holding only this run's output" is A plus extra steps — the move exists only to preserve "parse from working_dir", and preserving that is exactly what creates the directory-in-the-walk hazard.

One place you're right that C beats B: the MultipleFiles case this code already documents, where some GCC versions emit several files per gcno. One rename instead of N. It still doesn't get below A, which does none.