Skip to content

Commit 668e5f4

Browse files
doublegateclaude
andcommitted
fix(libretro): sync the save directory after the FDS rename
agy's review of #561 (the round after 5ddf520): the FDS save now fsyncs its data before the rename, but a rename is a directory change, and without syncing the directory the new entry may not survive a power loss. The desktop's `write_atomic` already does this (`sync_parent_dir`), so the libretro helper now matches it on Unix: open the parent, `sync_all`, and treat EINVAL / ENOTSUP / EBADF as "this filesystem offers no directory barrier" rather than a failure -- the same set, for the same reason (a network mount must not fail every save on a path that fully succeeded). Any other error is returned even though the file was replaced. Here that only leaves the disk marked dirty, so the next flush writes it again; the desktop documents the same trade-off at length in atomic_write.rs. Not unit-tested, as with the data fsync: the failure it guards is a power loss. libretro tests 38/38, clippy clean. Answered on the PR without code: its BPS "integer overflow" is refuted at patch.rs:400 (`length > target_size - out.len()`, a subtraction under the invariant `out.len() <= target_size`); `rawset(mt, "__gc", f)` after `setmetatable` is pinned as never honoured; the wasm heap is NF-09 (documented, piccolo 0.3.3 has no limit to set). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014qfTKi2M3swo7qnwvYCkDj
1 parent 87fc6ef commit 668e5f4

2 files changed

Lines changed: 29 additions & 2 deletions

File tree

‎crates/rustynes-libretro/src/lib.rs‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1642,6 +1642,14 @@ fn fds_save_path(
16421642
/// could leave an empty `.fds.sav` in place of the old one -- the outcome the
16431643
/// temporary file exists to prevent. A failed write removes the temporary
16441644
/// file rather than leaving it beside the save.
1645+
///
1646+
/// On Unix the parent directory is synced after the rename (agy on #561), as
1647+
/// the desktop's `write_atomic` does: the rename is a directory change, and
1648+
/// without it the new entry may not survive a power loss. A filesystem that
1649+
/// does not offer a directory `fsync` (`EINVAL`, `ENOTSUP`, `EBADF`) is not a
1650+
/// failure. Any other error is returned even though the file was replaced,
1651+
/// which here only leaves the disk marked dirty, so the next flush writes it
1652+
/// again.
16451653
fn write_file_atomically(path: &std::path::Path, bytes: &[u8]) -> std::io::Result<()> {
16461654
use std::io::Write as _;
16471655
if let Some(dir) = path.parent() {
@@ -1660,7 +1668,26 @@ fn write_file_atomically(path: &std::path::Path, bytes: &[u8]) -> std::io::Resul
16601668
}
16611669
std::fs::rename(&tmp, path).inspect_err(|_| {
16621670
let _ = std::fs::remove_file(&tmp);
1663-
})
1671+
})?;
1672+
#[cfg(unix)]
1673+
{
1674+
let dir = path
1675+
.parent()
1676+
.filter(|p| !p.as_os_str().is_empty())
1677+
.unwrap_or_else(|| std::path::Path::new("."));
1678+
match std::fs::File::open(dir).and_then(|d| d.sync_all()) {
1679+
Err(e)
1680+
if !matches!(
1681+
e.kind(),
1682+
std::io::ErrorKind::InvalidInput | std::io::ErrorKind::Unsupported
1683+
) && e.raw_os_error() != Some(libc::EBADF) =>
1684+
{
1685+
return Err(e);
1686+
}
1687+
_ => {}
1688+
}
1689+
}
1690+
Ok(())
16641691
}
16651692

16661693
/// Hand the frontend the disk-control interface: the extended one (with

‎docs/audits/libretro-disposition.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ was shown red first and mutation-checked (commit bodies carry the evidence).
6161
| NL-10 | `retro_serialize` returned true for an undersized buffer | FIXED | Refused; `<` → `<=` fails 5 tests | v2.9.0 | `aff38af9` |
6262
| NL-05 | Core options not re-declared after deinit + re-init | FIXED | Declared from `on_set_environment` once per init cycle; red "left 1 right 2"; 2 mutations caught | v2.9.0 | `2619d9b2` |
6363
| NL-07 | Only the basic disk-control interface registered | FIXED | Extended interface when the frontend reports version >= 1, basic otherwise; the core's own `disk_image_label` null-checks, bounds, truncates and NUL-terminates, so L-1.5 stays unreachable. `set_initial_image` / `get_image_path` deliberately NULL (an FDS game must boot from side A). 3 mutations caught | v2.9.0 | `f22ae711` |
64-
| NL-03 | FDS disk writes never persisted | FIXED | Written to `<save>/RustyNES/<sha256>.fds.sav` -- the desktop's name and bytes, so a save moves between them -- atomically 60 frames after the disk is dirtied, and at unload / deinit / next load; read before the console is built. Tested with a synthetic BIOS that writes through `$4023/$4024/$4025`. No save directory means no disk save, and the core logs it. The temporary file is `sync_all`ed before the rename and removed on a failed write (agy on #561) | v2.9.0 | `af78b72a` |
64+
| NL-03 | FDS disk writes never persisted | FIXED | Written to `<save>/RustyNES/<sha256>.fds.sav` -- the desktop's name and bytes, so a save moves between them -- atomically 60 frames after the disk is dirtied, and at unload / deinit / next load; read before the console is built. Tested with a synthetic BIOS that writes through `$4023/$4024/$4025`. No save directory means no disk save, and the core logs it. The temporary file is `sync_all`ed before the rename and removed on a failed write, and on Unix the directory is synced after it, as the desktop's `write_atomic` does (agy on #561) | v2.9.0 | `af78b72a` |
6565
| NL-08 | Vs. System: no coin/service/DIP input, wrong palette | FIXED (DIP user setting DECLINED for now) | The Vs. database's PPU type and DIP default applied at load to both consoles of a cabinet; RetroPad L = coin (3-frame pulse, as the desktop), R = service, named in the input descriptors while a Vs. game is loaded. A local-only test shows the presented frame equals the palette-applied oracle for *Stroke & Match Golf (VS)*. User-set DIP switches as a core option are not done (eight switches do not fit the legacy `SET_VARIABLES` form cleanly; written up in `advanced_features.md`) | v2.9.0 | `02508296` |
6666
| NL-04 | `.info` said `core_options = "false"`; stale description | FIXED (local) | Local `.info` only; the audit now derives `core_options`, `disk_control`, `memory_descriptors` and `input_descriptors` from the source and rejects the retired scheduler wording. Upstream at v3.0.0 (ADR 0041) | v2.9.0 | `d32b2abd` |
6767
| NL-09 | Dual-cabinet save states slow and allocating | RECORDED, not changed | Two independent runs, same sign in 12 of 12 rounds: dual `snapshot()` 314-1,630 us against 46-78 us for a pooled candidate. `ab_check.sh` judges only the `full_frame` workloads, so it cannot rule on a serialize change; the fix belongs in `VsDualSystem` beside the NC-03 restore work. `architecture.md`'s "does not allocate" corrected | v2.9.1 | `1aee5565` |

0 commit comments

Comments
 (0)