fix: 0.5 followup#22
Conversation
Adds a regression for the documented "External rename + unlink elsewhere" residual race (README's path-reuse-limitations). A concurrent actor renames our file into a different directory, then unlinks it there. The pin's nlink drops to 0, but the actual unlink happened in the other directory's journal — fsyncing OUR parent doesn't make it crash-durable. fmmap surfaces NotFound rather than claiming durable success. Verifies the round-36 contract: explicit `remove()` only returns Ok if fmmap itself observed `unlinkat` succeed in the parent it then fsynced. nlink == 0 alone is not enough. Closes #16.
The async pin-extraction helpers had two divergent shapes:
- mmap_file/{tokio,smol}_impl.rs: `extract_pin_or_err(File) -> Result<File, (AsyncFile, Error)>` (with file recovery on dup failure)
- disk/{tokio,smol}_impl.rs: `extract_inode_pin(File) -> io::Result<File>` (no recovery)
Same purpose, different return types. Consolidate to the
recovery-aware shape across both module pairs (and a shared name,
`extract_pin_or_err`). Tokio's helper still always succeeds (Ok via
into_std); smol's helper returns the file on dup failure for caller
recovery. The disk macro currently discards the recovered file
(trait method's `Result<()>` shape can't return `Self`); the
recovery-aware variant is now ready for a future `try_drop_remove`
inherent method (see #20).
Towards #17.
The raw `AsyncDiskMmapFileMut::drop_remove(self)` consumed `self`
unconditionally — including on the only practical recoverable
failure mode, smol's `fcntl_dupfd_cloexec` returning `EMFILE` while
extracting the inode pin. Caller had no way to retry.
New inherent method:
\`\`\`rust
impl AsyncDiskMmapFileMut {
pub async fn try_drop_remove(self)
-> Result<(), DropRemoveError<Self>>;
}
\`\`\`
`DropRemoveError<T>` (in `fmmap::error`) has two variants:
- `Recoverable(T, Error)` — failed before any destructive op (only
smol-EMFILE in practice). `Self` is preserved so the caller can
retry once fd pressure subsides.
- `Terminal(Error)` — failed after destructive op (probe / unlink /
parent fsync). `Self` is gone; treat like `Result<(), Error>`.
The trait method `AsyncMmapFileMutExt::drop_remove` now delegates to
`try_drop_remove` and discards the recovered value via
`DropRemoveError::into_error` for users who don't need recovery.
The trait signature is unchanged (`Result<()>`).
Tokio's `into_std()` allocates no fd, so the `Recoverable` variant
is unreachable on tokio; this is purely additive there.
Closes #20.
The `DropRemoveError<T>` type added by 82db381 implements `core::error::Error`, which was stabilized in Rust 1.81. The crate's previous MSRV was 1.75, so users on the documented MSRV would hit E0658. Bump everywhere: - `fmmap/Cargo.toml`: rust-version = "1.81.0" - `README.md`: "Rust **1.81** or later" - `fmmap/src/disk.rs`: lock-API rationale comment - `.github/workflows/ci.yml`: minrust job pins to 1.81.0 minrust-smol stays at 1.85 (smol's transitive dependencies require that floor independently of the crate-level MSRV). Verified `cargo +1.81.0 check` clean for sync and tokio features.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #22 +/- ##
==========================================
- Coverage 88.94% 88.58% -0.36%
==========================================
Files 27 28 +1
Lines 4016 4048 +32
==========================================
+ Hits 3572 3586 +14
- Misses 444 462 +18
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR follows up on the 0.5 line by (1) raising the crate MSRV to Rust 1.81 across docs/metadata/CI and (2) refining raw async durable-delete ergonomics by introducing a recovery-aware try_drop_remove API, plus a regression test covering the documented cross-directory rename+unlink race.
Changes:
- Bump MSRV to Rust 1.81 (README,
fmmapcrate metadata, and CI minrust job). - Add
DropRemoveError<T>and a recovery-awaretry_drop_remove()for raw async disk mmap types; keep the existing traitdrop_remove()behavior by flattening errors toio::Error. - Add a Unix regression test for cross-directory rename followed by external unlink to ensure conservative
NotFoundsurfacing (no false durable-delete success).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates documented MSRV to 1.81. |
| fmmap/Cargo.toml | Updates rust-version to 1.81.0 to match the new MSRV. |
| .github/workflows/ci.yml | Updates the minrust toolchain and cargo-hack invocation to 1.81.0. |
| fmmap/src/error.rs | Introduces DropRemoveError<T> used by recovery-aware raw async deletion. |
| fmmap/src/disk.rs | Refactors raw async drop_remove to delegate to new try_drop_remove recovery API. |
| fmmap/src/disk/tokio_impl.rs | Renames/aligns inode-pin extraction helper and adds a try_drop_remove happy-path test. |
| fmmap/src/disk/smol_impl.rs | Renames/aligns inode-pin extraction helper, makes dup failure recoverable, and adds a try_drop_remove happy-path test. |
| fmmap/src/mmap_file/sync_impl.rs | Adds regression coverage for the documented external rename+unlink-elsewhere residual race. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… open Codex finding: `open_parent_for_sync` is a second pre-destructive fd allocation that can fail with EMFILE. Round-1 of `try_drop_remove` moved the parent open *inside* `run_blocking_io` (after Self had been destructured), so a parent-open EMFILE would surface as `DropRemoveError::Terminal` even though it's a strictly pre-destructive failure mode. This affected tokio too — `into_std()` avoids the dup, but the parent fd is still allocated later. Restructure: - Phase 1 (recoverable): `open_parent_for_sync` BEFORE touching any of self's owned resources. EMFILE here returns `Recoverable(self, e)` with self fully intact. - Phase 2 (recoverable): extract the inode pin (`extract_pin_or_err`). EMFILE here restores `self.file`, drops the previously-opened `parent_handle` (fresh fd, no-op), and returns `Recoverable`. - Phase 3 (terminal): destructure self, drop mmap, run the blocking unlink + fsync sequence using the already-open `parent_handle`. The closure no longer calls `open_parent_for_sync` itself; the already-open handle is moved into the blocking task. Tests: sync 96, tokio 65, smol 59. Clippy + fmt clean under -Dwarnings. Windows MSVC + 1.81 MSRV cross-checks clean.
No description provided.