fix: storage temp() file cleanup with RemoteProvider by swrosati · Pull Request #4189 · snakemake/snakemake

swrosati · 2026-05-07T20:11:04Z

fix: `temp()` files not removed from remote storage (S3)

When using temp() with a remote storage backend (e.g. S3) and a remote executor (e.g. AWS Batch), temporary files are never cleaned up from storage after they are no longer needed. Two bugs combine to cause this:

Bug 1 — is_needed_tempfile always returns True in the main process (6d7c8823)

is_needed_tempfile gated cleanup on storage_settings.unneeded_temp_files, which is only populated in spawned subprocesses via the --unneeded-temp-files CLI arg. In the main orchestrator process (where handle_temp runs), this set is always empty, so is_unneeded_outside was always False and temp files were never passed to remove(). The main process has full DAG knowledge and already correctly determines liveness via is_needed_by_subsequent_job, so the gate is unnecessary.

Bug 2 — set intersection loses the temp flag (4bf0afeb)

handle_temp computes files to remove via tempfiles & files — a set intersection between output _IOFile objects (carrying the temp flag) and input _IOFile objects from _dependencies (without the flag). Since _IOFile inherits from str, both sides hash/compare equal on the path string alone. Python's set.__and__ does not guarantee which side's objects are returned; when it returns the input objects, is_flagged(file, "temp") in remove() evaluates to False and managed_remove() is skipped.

Replaced with {f for f in tempfiles if f in files} to explicitly preserve the output _IOFile instances.

QC

The PR contains a test case for the changes or the changes are already covered by an existing test case.
The documentation (docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).
I, as a human being, have checked each line of code in this pull request

AI-assistance disclosure

I used AI assistance for:

Code generation (e.g., when writing an implementation or fixing a bug)
Test/benchmark generation
Documentation (including examples)
Research and understanding

Summary by CodeRabbit

Bug Fixes
- Refined temp-file cleanup logic: subprocess runs now always preserve temp files, while remote runs respect storage settings to determine which temp outputs can be removed; improves accuracy of cleanup and avoids incorrect deletions.
Tests
- Added tests validating temp-file needed/unneeded decisions across local, remote, and subprocess execution modes and a regression test for temp-file selection during cleanup.

coderabbitai · 2026-05-07T20:11:20Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

Add subprocess-exec early-return inside DAG.is_needed_tempfile; limit remote-storage consultation for is_unneeded_outside to remote runs; adjust handle_temp candidate iteration to a set comprehension; add tests covering downstream states, remote/subprocess modes, and a handle_temp regression.

Changes

Remote Temp File Cleanup

Layer / File(s)	Summary
Temp File Necessity Check `src/snakemake/dag.py`	`is_needed_tempfile` adds an early `workflow.subprocess_exec` return path and computes `is_unneeded_outside` by consulting `workflow.storage_settings.unneeded_temp_files` only for `remote_exec`, defaulting to `True` otherwise.
Temp Input Cleanup Filter `src/snakemake/dag.py`	`handle_temp` builds dependent-temp candidates with `{f for f in tempfiles if f in files}` and uses `partial(self.is_needed_tempfile, job_)` as the predicate for `filterfalse`.
Test Fixtures & Scenarios `tests/test_remote_temp_cleanup.py`	New module with `_make_tempfile`, `mock_dag` fixture, and tests for no downstream/unfinished/finished downstream states, `remote_exec` with empty/non-empty `unneeded_temp_files`, `subprocess_exec` override, and a handle_temp regression verifying the OUTPUT temp object is selected.

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main fix: storage temp file cleanup with RemoteProvider, which is the primary change addressing temp file removal issues in remote storage contexts.
Description check	✅ Passed	The PR description comprehensively documents both bugs, their root causes, the fixes applied, and includes completed QC checklist items and AI-assistance disclosures as required by the template.
Docstring Coverage	✅ Passed	Docstring coverage is 93.33% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@tests/test_remote_temp_cleanup.py`:
- Line 127: The assigned lambda "is_temp = lambda f: f.flags.get('temp', False)"
triggers Ruff E731; replace it with a named function (e.g., define function
is_temp(f) that returns f.flags.get("temp", False)) and use that function in
place of the lambda; update any references to "is_temp" unchanged so behavior
remains identical and ensure the new function is placed where the lambda was
defined (same scope).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 836baa99-9bda-46ee-a37a-196826962ec3

📥 Commits

Reviewing files that changed from the base of the PR and between 384b4a3 and 4bf0afe.

📒 Files selected for processing (3)

src/snakemake/dag.py
src/snakemake/io/__init__.py
tests/test_remote_temp_cleanup.py

The `is_needed_tempfile` method gated storage cleanup on `storage_settings.unneeded_temp_files`, which is only populated in spawned subprocesses via the `--unneeded-temp-files` CLI arg. In the main orchestrator process (where `handle_temp` runs), this set is always empty, so `is_unneeded_outside` was always False and temp files were never removed from remote storage. Remove the `remote_exec` branch that checked `unneeded_temp_files` and unconditionally set `is_unneeded_outside = True`. The main process has full DAG knowledge and already correctly determines whether a temp file is still needed via `is_needed_by_subsequent_job`. This works in conjunction with commit cb4e365 which added the `is_flagged(file, "temp")` condition to the `remove()` function, allowing `managed_remove()` to be called on storage-backed temp files. Add unit tests for `is_needed_tempfile` covering the remote execution regression case.

Python's set intersection (a & b) returns objects from the set being iterated (implementation-dependent, typically the smaller set). When tempfiles (output _IOFile with temp flag) is intersected with files (input _IOFile from _dependencies without temp flag), the result may contain the input _IOFile objects which lack the temp flag. This causes remove() to skip managed_remove() because is_flagged(file, 'temp') evaluates to False on the input _IOFile. Replace 'tempfiles & files' with a set comprehension that explicitly iterates tempfiles and checks membership in files, guaranteeing the output _IOFile objects (with temp and storage_object flags) are preserved.

swrosati · 2026-05-18T17:43:04Z

Hi @johanneskoester - I'm just checking to see if this PR needs any additions or edits? If so, please don't hesitate to let me know!

Copilot

Pull request overview

This PR fixes remote-storage cleanup for temp() outputs by ensuring the main DAG process uses liveness information directly and preserves temp-flagged output file objects when selecting files to remove.

Changes:

Updates is_needed_tempfile() so remote temp cleanup is not blocked by an empty unneeded_temp_files set in the main process.
Replaces set intersection in handle_temp() with a comprehension that preserves the output _IOFile instance carrying the temp flag.
Adds regression-style unit tests for temp-file liveness and flag preservation behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`src/snakemake/dag.py`	Adjusts temp-file liveness and cleanup selection logic for remote storage cleanup.
`tests/test_remote_temp_cleanup.py`	Adds tests covering remote temp cleanup decisions and preserving temp-flagged file objects.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

…ain process and use unneeded_temp_files again

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

tests/test_remote_temp_cleanup.py (1)
46-55: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Test passes for the wrong reason due to missing remote_exec = False.

The fixture defaults remote_exec=True with unneeded_temp_files=frozenset(). With these settings, is_unneeded_outside becomes False (line 972-974 in dag.py), causing the function to immediately return True at line 997 — regardless of downstream consumers.

To actually test the downstream-unfinished logic (the is_needed_by_subsequent_job check), this test needs remote_exec = False so that is_unneeded_outside = True and the downstream check is exercised.
Proposed fix
 def test_temp_file_needed_when_downstream_unfinished(mock_dag):
     """A temp file is still needed if a consuming job has not finished."""
     producer = MagicMock()
     consumer = MagicMock()
     tempfile = _make_tempfile("output.tmp")
+    mock_dag.workflow.remote_exec = False  # test main process downstream logic
 
     mock_dag.depending[producer] = {consumer: {tempfile}}
     mock_dag._needrun.add(consumer)
 
     assert mock_dag.is_needed_tempfile(producer, tempfile)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_remote_temp_cleanup.py` around lines 46 - 55, The test currently
exercises the downstream logic only accidentally because the fixture's default
remote_exec=True makes is_unneeded_outside False and causes is_needed_tempfile
to return True early; to fix, ensure the DAG is in remote_exec=False mode so
is_unneeded_outside is True and the downstream check runs: update
test_temp_file_needed_when_downstream_unfinished to set mock_dag.remote_exec =
False (or obtain the fixture with remote_exec=False) before populating
mock_dag.depending/_needrun and calling mock_dag.is_needed_tempfile so that the
is_needed_by_subsequent_job path is actually exercised.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@tests/test_remote_temp_cleanup.py`:
- Around line 46-55: The test currently exercises the downstream logic only
accidentally because the fixture's default remote_exec=True makes
is_unneeded_outside False and causes is_needed_tempfile to return True early; to
fix, ensure the DAG is in remote_exec=False mode so is_unneeded_outside is True
and the downstream check runs: update
test_temp_file_needed_when_downstream_unfinished to set mock_dag.remote_exec =
False (or obtain the fixture with remote_exec=False) before populating
mock_dag.depending/_needrun and calling mock_dag.is_needed_tempfile so that the
is_needed_by_subsequent_job path is actually exercised.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f9af988b-99ef-4453-9557-810f63dacfad

📥 Commits

Reviewing files that changed from the base of the PR and between 5e42338 and 92c37ed.

📒 Files selected for processing (2)

src/snakemake/dag.py
tests/test_remote_temp_cleanup.py

swrosati · 2026-05-29T14:05:22Z

Thank you, @johanneskoester! Much appreciated!

🤖 I have created a release *beep* *boop* --- ## [9.21.1](v9.21.0...v9.21.1) (2026-05-29) ### Bug Fixes * add default json function to benchmarks ([#4128](#4128)) ([41fab22](41fab22)) * do not rerun when checkpoint job missing but downstream file exists ([#4124](#4124)) ([a060b93](a060b93)) * ensure that error logs contain all available details ([#4183](#4183)) ([74a86e9](74a86e9)) * handle missing pss attribute in benchmark on Windows ([#4160](#4160)) ([da52080](da52080)) * implement Resources.setdefault ([#3968](#3968)) ([2413e99](2413e99)) * reporting remote nodes number ([#3978](#3978)) ([8c534f0](8c534f0)) * resolve pathvars before constructing storage queries ([#3969](#3969)) ([bd15237](bd15237)) * storage temp() file cleanup with RemoteProvider ([#4189](#4189)) ([898bad1](898bad1)) * tolerate FileNotFoundError in drop_iocache ([#4153](#4153)) ([#4191](#4191)) ([ce26b28](ce26b28)) ### Documentation * Added guide on debugging workflows ([#4029](#4029)) ([3d052ae](3d052ae)) * **cli:** Remove broken ref bold markup ([#4204](#4204)) ([1200ebf](1200ebf)) * remove duplicated resources attribute in rules.rst ([#4190](#4190)) ([6c8ecdd](6c8ecdd)) * **rules:** Update script type hint advice ([#4193](#4193)) ([6108712](6108712)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

swrosati requested a review from johanneskoester as a code owner May 7, 2026 20:11

coderabbitai Bot reviewed May 7, 2026

View reviewed changes

Comment thread tests/test_remote_temp_cleanup.py Outdated

swrosati changed the title ~~Fix storage temp() file cleanup~~ fix: storage temp() file cleanup with RemoteProvider May 7, 2026

swrosati force-pushed the fix/storage-temp-file-cleanup-s3-plugin branch 2 times, most recently from da60357 to da16857 Compare May 8, 2026 13:57

swrosati force-pushed the fix/storage-temp-file-cleanup-s3-plugin branch from da16857 to 5e42338 Compare May 8, 2026 14:10

Merge branch 'main' into fix/storage-temp-file-cleanup-s3-plugin

d7e562e

johanneskoester reviewed May 22, 2026

View reviewed changes

Comment thread src/snakemake/dag.py Outdated

johanneskoester self-assigned this May 22, 2026

johanneskoester requested a review from Copilot May 28, 2026 11:25

Copilot started reviewing on behalf of johanneskoester May 28, 2026 11:25 View session

Copilot AI reviewed May 28, 2026

View reviewed changes

Comment thread src/snakemake/dag.py

johanneskoester added 2 commits May 28, 2026 13:49

fix: properly distinguish between remote job snakemake runs and the m…

9259b90

…ain process and use unneeded_temp_files again

comment

92c37ed

coderabbitai Bot reviewed May 28, 2026

View reviewed changes

Merge branch 'main' into fix/storage-temp-file-cleanup-s3-plugin

cc85512

johanneskoester merged commit 898bad1 into snakemake:main May 29, 2026
59 checks passed

snakemake-bot mentioned this pull request May 29, 2026

chore(main): release 9.21.1 #4200

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: storage temp() file cleanup with RemoteProvider #4189

fix: storage temp() file cleanup with RemoteProvider #4189
johanneskoester merged 6 commits into
snakemake:mainfrom
swrosati:fix/storage-temp-file-cleanup-s3-plugin

swrosati commented May 7, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented May 7, 2026 •

edited

Loading

Reviews paused

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

swrosati commented May 18, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

swrosati commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

swrosati commented May 7, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

fix: temp() files not removed from remote storage (S3)

QC

AI-assistance disclosure

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swrosati commented May 18, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

swrosati commented May 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

swrosati commented May 7, 2026 •

edited by coderabbitai Bot

Loading

fix: `temp()` files not removed from remote storage (S3)

coderabbitai Bot commented May 7, 2026 •

edited

Loading