Skip to content

Add synchronous close() to TempFile and TempDir#15

Merged
sunsided merged 2 commits into
mainfrom
add-sync-close
Jun 2, 2026
Merged

Add synchronous close() to TempFile and TempDir#15
sunsided merged 2 commits into
mainfrom
add-sync-close

Conversation

@sunsided

@sunsided sunsided commented Jun 2, 2026

Copy link
Copy Markdown
Owner

What

Adds close(self) -> std::io::Result<()> to both TempFile and TempDir: the synchronous, error-observable sibling of drop_async.

Why

The existing manual-cleanup API has a gap:

  • drop_async(self) returns () — no way to observe a deletion failure.
  • The implicit Drop also swallows any error (it has nowhere to report it).

close() lets a caller delete now and find out whether the unlink/rmdir actually succeeded. Use drop_async in async code to avoid blocking the executor on the syscall; reach for close() when you want the result synchronously.

Semantics (identical to drop_async, minus the runtime)

  • Gates on Arc::into_inner: only the sole owner deletes. With live clones it returns Ok(()) and leaves cleanup to their Drop — no double-free.
  • Disarms the ownership flag only after a confirmed removal (or NotFound).
  • On error: leaves Drop armed as a backstop (which will retry); the returned Err is informational, not the only line of defense.

Tests (tests/drop_safety.rs)

  • close_deletes_owned_file_and_reports_ok
  • close_via_clone_leaves_file_for_remaining_reference (clone keeps it alive; dropped later → gone)
  • close_on_borrowed_file_is_a_noop
  • close_deletes_owned_dir_recursively

Review order

  1. src/tempfile.rs / src/tempdir.rs — the two close() impls (compare against the drop_async directly above each).
  2. tests/drop_safety.rs — the four behavior cases.
  3. README.md — one-line pointer.

Verification

cargo test, cargo test --features uuid, cargo clippy --all-features --all-targets, cargo fmt --check all clean.

Context

Rounds out the manual-closing API (keep / persist / drop_async / close) discussed in #8. True language-level AsyncDrop remains tracked in #1 (blocked on the unstable Rust feature).

`drop_async` deletes without blocking the runtime but returns `()`, and the
implicit `Drop` swallows any deletion error. Add `close(self) -> io::Result<()>`
as the synchronous, error-observable sibling so callers can detect a failed
unlink/rmdir instead of having it silently dropped.

Semantics mirror `drop_async` exactly, just synchronously:
- gates on `Arc::into_inner`, so only the sole owner deletes; with live clones
  it returns `Ok(())` and leaves cleanup to their `Drop`
- disarms the ownership flag only after a confirmed removal (or `NotFound`)
- on error, leaves `Drop` armed as a backstop to retry; the returned `Err` is
  informational

Rounds out the manual-closing API (keep / persist / drop_async / close) from
issue #8. Tests cover owned-file deletion, the live-clone no-delete path,
borrowed no-op, and recursive directory removal. README gains a one-line
pointer next to drop_async.
Copilot AI review requested due to automatic review settings June 2, 2026 21:50
@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 66.66667% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.48%. Comparing base (c7dca58) to head (7af3fe3).

Files with missing lines Patch % Lines
src/tempdir.rs 60.00% 6 Missing ⚠️
src/tempfile.rs 73.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
- Coverage   67.53%   67.48%   -0.05%     
==========================================
  Files           6        6              
  Lines         539      569      +30     
==========================================
+ Hits          364      384      +20     
- Misses        175      185      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread tests/drop_safety.rs Outdated
Comment thread README.md Outdated
@kilo-code-bot

kilo-code-bot Bot commented Jun 2, 2026

Copy link
Copy Markdown

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental Review

All issues from the previous review (at d67e3fe) have been resolved in this commit:

Previous Issue Status
WARNING tests/drop_safety.rs — open TempFile handle inside dir blocked remove_dir_all on Windows Fixed: .keep() now releases the handle while leaving the file on disk
SUGGESTION README.md — commented-out file.close() appeared to follow a consuming drop_async Fixed: comment now explicitly says "alternative to the line above (not an addition)"
Observation src/lib.rs:98AtomicOwnership doc omitted close from the list of disarming methods Fixed: close added to the doc comment
Inline src/tempfile.rs — doc claimed Drop stays armed on close() error Fixed: doc and code both disarm ownership before returning the error
Inline src/tempdir.rs — doc claimed Drop stays armed on close() error Fixed: doc and code both disarm ownership before returning the error
Files Reviewed (5 files)
  • src/tempfile.rs — 0 issues (error branch correctly calls set_borrowed() before returning; drop(file) before remove_file is correct)
  • src/tempdir.rs — 0 issues (mirrors tempfile.rs; drop(dir) before remove_dir_all is correct)
  • src/lib.rs — 0 issues (doc now lists all four disarming methods)
  • tests/drop_safety.rs — 0 issues (Windows portability fixed via .keep())
  • README.md — 0 issues (mutually-exclusive alternatives now clearly stated)

Overall assessment: The close() implementation is correct and consistent across both TempFile and TempDir. The semantics change (disarming ownership on error rather than leaving Drop armed) is well-reasoned and the documentation accurately describes the new behavior. The PR is ready to merge.


Reviewed by claude-4.6-sonnet-20260217 · 311,776 tokens

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a synchronous, error-reporting close(self) -> std::io::Result<()> API to TempFile and TempDir, complementing the existing async drop_async() by allowing callers to explicitly delete immediately and observe deletion failures.

Changes:

  • Implement TempFile::close() and TempDir::close() with semantics aligned to drop_async (sole-owner deletion via Arc::into_inner, disarm-after-success).
  • Add behavioral tests covering owned vs cloned vs borrowed close behavior, and recursive dir removal.
  • Update README with a pointer to the new synchronous close option.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

File Description
src/tempfile.rs Adds TempFile::close() and documents synchronous, error-observable deletion.
src/tempdir.rs Adds TempDir::close() and documents synchronous, error-observable recursive removal.
tests/drop_safety.rs Adds tests for close() semantics across ownership/cloning/borrowed cases.
README.md Mentions close() as a synchronous alternative to drop_async().

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

Comment thread tests/drop_safety.rs Outdated
Comment thread README.md Outdated
Comment thread src/tempfile.rs Outdated
Comment thread src/tempdir.rs Outdated
- close() now disarms on a deletion error instead of leaving Drop armed. A
  synchronous retry of the same failing syscall is pointless, and the previous
  "leave armed" path made the doc ("file/dir left in place") self-contradictory
  (Drop would immediately retry remove_*). close() now leaves the file/dir
  deterministically in place and returns the error for the caller to handle.
  Docs reworded to match, and to note remove_dir_all is non-atomic (partial
  removal possible on error). Contrast with drop_async (whose sync Drop backstop
  is a genuinely different mechanism after a possibly-cancelled async removal)
  is now explained.
- close_deletes_owned_dir_recursively: keep() the inner file so it stays on disk
  with its handle closed, instead of holding an open handle inside the dir. The
  open handle would block remove_dir_all on Windows. Now correct on all targets,
  no cfg guard needed.
- README: clarify the commented close() call is an alternative to drop_async
  (which consumes `file`), not an additional line.
- lib.rs: add close to the AtomicOwnership doc list of methods that disarm.
@sunsided

sunsided commented Jun 2, 2026

Copy link
Copy Markdown
Owner Author

Also addressed the out-of-diff observation from the review summary: src/lib.rs AtomicOwnership doc now lists close alongside keep/persist/drop_async as a method that disarms automatic deletion (7af3fe3).

@sunsided
sunsided merged commit a44c5ae into main Jun 2, 2026
11 of 13 checks passed
@sunsided
sunsided deleted the add-sync-close branch June 2, 2026 22:00
sunsided added a commit that referenced this pull request Jun 2, 2026
## What

Adds the three 0.8.0 changes that were missing from `CHANGELOG.md` but
shipped in the tagged + published `v0.8.0` (and are now on the [GitHub
release](https://github.com/sunsided/async-tempfile-rs/releases/tag/v0.8.0)):

- **Added** — `close()` on `TempFile`/`TempDir`, the synchronous
error-observable sibling of `drop_async` (#15)
- **Changed** — path-separator `prefix`/`suffix` rejected with the new
`Error::InvalidAffix`, preventing directory escape (#14)
- **Internal** — test coverage ~67% → ~94% (#16)

## Why

The `CHANGELOG.md` 0.8.0 section only covered the #13 work
(builder/keep/persist/drop_async/forbid-unsafe/unpredictable names). #14
and #15 landed afterward and were tagged into 0.8.0 without changelog
entries, so the in-repo changelog understated what 0.8.0 actually
contains. This brings it in line with the release notes and crates.io.

## Note

Docs-only; no code change. The version line stays `0.8.0` — these edits
describe the already-released version, they don't introduce a new one.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants