Skip to content

fix: 0.5 followup#22

Open
al8n wants to merge 6 commits into
mainfrom
fix/0.5-followup
Open

fix: 0.5 followup#22
al8n wants to merge 6 commits into
mainfrom
fix/0.5-followup

Conversation

@al8n
Copy link
Copy Markdown
Owner

@al8n al8n commented May 5, 2026

No description provided.

al8n added 5 commits May 5, 2026 15:16
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
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 54.83871% with 28 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.58%. Comparing base (d3bd40e) to head (66b44f7).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
fmmap/src/disk.rs 66.66% 14 Missing ⚠️
fmmap/src/error.rs 0.00% 13 Missing ⚠️
fmmap/src/disk/smol_impl.rs 83.33% 1 Missing ⚠️
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     
Files with missing lines Coverage Δ
fmmap/src/disk/tokio_impl.rs 83.33% <100.00%> (ø)
fmmap/src/mmap_file/sync_impl.rs 88.48% <ø> (ø)
fmmap/src/disk/smol_impl.rs 90.00% <83.33%> (-10.00%) ⬇️
fmmap/src/error.rs 0.00% <0.00%> (ø)
fmmap/src/disk.rs 84.46% <66.66%> (-0.49%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 343c959...66b44f7. Read the comment docs.

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

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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, fmmap crate metadata, and CI minrust job).
  • Add DropRemoveError<T> and a recovery-aware try_drop_remove() for raw async disk mmap types; keep the existing trait drop_remove() behavior by flattening errors to io::Error.
  • Add a Unix regression test for cross-directory rename followed by external unlink to ensure conservative NotFound surfacing (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.
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