sort: keep to its own temporary directory and files by abendrothj · Pull Request #15135 · uutils/coreutils

abendrothj · 2026-10-06T11:28:31Z

sort created, reopened and deleted its temporary files by path, and its cleanup emptied the temporary directory by path too. Once that path led to another directory, sort read the files there as its own and deleted them.

The temporary directory and each file are now identified by device and inode when they are created. Files are created and deleted through a descriptor of the directory that is checked first, and reopened only if they are still the file sort created, including the copy made when the output file is also an input. Cleanup, at exit and on Ctrl-C, deletes only the files sort created. If the path is replaced while sort runs, sort still stops with an error, but leaves the other directory alone. Redox and non-Unix systems keep the path-based code.

The directory descriptor is only open for a moment, and a merge now creates its output before opening its inputs, so sort needs no more file descriptors than before: sort -m, a sort that spills to disk and --compress-program work at the same limits as on main. test_temporary_files_with_few_file_descriptors checks the lowest that hold everywhere: 6 for -m and a spilling sort, 10 for --compress-program, which needs two more descriptors on musl and older glibc while the program starts.

test_tmp_dir_replaced_by_a_symlink fails on main and passes here. In GNU's tests/sort, the same two tests fail as on main (sort, sort-compress-proc), and sort-merge-fdlimit passes.

Closes #15134

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

codspeed · 2026-10-06T13:48:49Z

Merging this PR will not alter performance

⚠️

28 benchmarks spent significant time in system calls

System calls cannot be consistently instrumented, so they are not included in the measure, which understates the real cost. Please switch to the Walltime instrument to accurately measure system calls.

Measurement and system calls

sort_long_common_prefix_utf8_locale

sort_long_line[10000]

sort_accented_data[500000]

sort_ascii_utf8_locale

merge_pre_sorted_files_utf8_locale

sort_numeric[500000]

merge_single_file_utf8_locale

sort_case_sensitive[500000]

merge_pre_sorted_files

sort_very_long_lines_utf8_locale

…and 18 more

ℹ️ 1 benchmark has no flame graph

The flamegraph could not be generated because too much time was spent in runtime internals, making the visualization not useful for performance analysis.

sort_dictionary_order[500000]

✅ 58 untouched benchmarks
⏩ 398 skipped benchmarks¹

_{Comparing abendrothj:sort-remove-tmp-dir-at (b6d6b10) with main (46567bd)²}

398 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
No successful run was found on main (6292c03) during the generation of this report, so 46567bd was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

sylvestre · 2026-10-06T20:17:18Z

-            assert!(batch.len() < batch_size);
-            let merger = merge_without_limit(batch.into_iter(), settings)?;
-
+        let mut files = files;


the batching change could be its own commit, no?
and mut files: F in the signature would do

done, it's the first commit now, with mut files: F.

sylvestre · 2026-10-06T20:17:26Z

+/// The directory created by [`TmpDirWrapper`]. Its files are named `0`, `1`, ...
+/// in the order they were created.
+///
+/// On Unix (except Redox), files are only created and removed through a


please make this comment shorter :)

shortened to three lines.

sylvestre · 2026-10-06T20:17:29Z

+}
+
+/// The device and inode of a file.
+#[cfg(all(unix, not(target_os = "redox")))]


all(unix, not(target_os = "redox")) is repeated about 15 times here, could we use a cfg alias or move the unix part into its own module?

moved the descriptor-based code into its own module next to the path-based one, so the cfg is only on the two module lines now.

sylvestre · 2026-10-06T20:17:34Z

+impl SortTmpDir {
+    #[cfg_attr(
+        not(all(unix, not(target_os = "redox"))),
+        allow(clippy::unnecessary_wraps)


please don't silence clippy, can new be split per platform instead?

done: each module has its own new, which also creates the directory, so it's fallible on both and there's no allow.

sylvestre · 2026-10-06T20:17:49Z

+    }
+
+    /// Create the file `path` of this directory, which must not exist yet.
+    fn create(self: &Arc<Self>, path: PathBuf) -> io::Result<(File, TmpPath)> {


why not pass the index here? then you wouldn't need the file_name().unwrap() below

done, create takes the index and the name comes from it.

sylvestre · 2026-10-06T20:18:12Z

+        input
+            .write_all("willow\ncedar\n".repeat(8192).as_bytes())
+            .unwrap();
+        let tmp_dir = (0..100)


polling with sleeps for 2s, i am worried this will be flaky in CI

it now waits up to 60s and checks every 10ms, so it only fails if sort never writes its first file. it still polls: sort writes that file from another thread, with nothing to wait on.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-07T01:47:51Z

the fd-limit test failed on musl and the cross gnu images (glibc 2.23): there, starting the compress program takes two more descriptors for a moment, for the pipe that reports a failed exec. main needs the same, so the compress case now runs at 10 instead of 8; GNU 9.12 needs 20 for that command on that input. the aarch64 failure I couldn't reproduce outside CI.

sylvestre · 2026-10-07T08:08:29Z

+        fs::rename(tmp, dir.path().join("moved")).unwrap();
+        std::os::unix::fs::symlink(other.path(), tmp).unwrap();
+
+        if let Ok(mut file) = open_input(&files[0], copy.as_ref()) {


assert!(open_input(&files[0], copy.as_ref()).is_err()) would be simpler, no?

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

abendrothj · 2026-10-10T04:27:15Z

Rebased. The aarch64 failure isn't from sort: commands inherit the PTYs of terminal-simulated tests running at the same time, which leaves sort too few descriptors. Fixed in #15217.

When a merge has more inputs than it can open at once, each batch is merged into a temporary file. That file was created after the batch's inputs had been opened. Create it first, so that creating it doesn't compete with them for file descriptors, and take each batch straight from the input iterator instead of collecting it first.

Temporary files were created, reopened and deleted by path, and cleanup emptied the temporary directory by path. Once the directory's path led to another directory, sort read the files there as its own and deleted them. The directory and each file are identified by device and inode when created. Files are created and deleted through a descriptor of the directory that is checked first, and reopened only if they are still the file sort created. Cleanup deletes only the names sort created. Redox and non-Unix systems keep the path-based code. The directory descriptor is closed after each use, so with merges creating their output before opening their inputs, sort needs no more file descriptors than before.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

sylvestre · 2026-10-14T20:28:23Z

Thanks for your PR

Copilot AI balanced review requested due to automatic review settings October 6, 2026 11:28

Copilot AI reviewed Oct 6, 2026

sylvestre reviewed Oct 6, 2026

View reviewed changes

abendrothj force-pushed the sort-remove-tmp-dir-at branch from 2bdb7bd to 7ab395f Compare October 7, 2026 01:47

Copilot AI balanced review requested due to automatic review settings October 7, 2026 01:47

Copilot AI reviewed Oct 7, 2026

sylvestre reviewed Oct 7, 2026

View reviewed changes

abendrothj force-pushed the sort-remove-tmp-dir-at branch from 7ab395f to 42a43a8 Compare October 7, 2026 22:12

Copilot AI balanced review requested due to automatic review settings October 7, 2026 22:12

Copilot AI reviewed Oct 7, 2026

Copilot AI balanced review requested due to automatic review settings October 10, 2026 04:26

abendrothj force-pushed the sort-remove-tmp-dir-at branch from 42a43a8 to 02e7e21 Compare October 10, 2026 04:26

Copilot AI reviewed Oct 10, 2026

This was referenced Oct 10, 2026

tests: simulated terminals leak into commands that other tests start #15216

Closed

uutests: open simulated terminals close-on-exec #15217

Merged

abendrothj added 2 commits October 12, 2026 03:07

Copilot AI balanced review requested due to automatic review settings October 12, 2026 10:21

abendrothj force-pushed the sort-remove-tmp-dir-at branch from 02e7e21 to b6d6b10 Compare October 12, 2026 10:21

Copilot AI reviewed Oct 12, 2026

sylvestre merged commit c76f272 into uutils:main Oct 14, 2026
100 checks passed

Conversation

abendrothj commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

codspeed Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

abendrothj commented Oct 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

abendrothj commented Oct 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sylvestre commented Oct 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

abendrothj commented Oct 6, 2026 •

edited

Loading

codspeed Bot commented Oct 6, 2026 •

edited

Loading