fix: gracefully handle out of disk space failures (#3425)#3908
Conversation
When disk fills during chain writes, avoid committing a DB tip that points at incomplete MMR state, and surface a clear DiskFull error instead of opaque I/O failures. - Detect ENOSPC / StorageFull via is_out_of_disk_space helpers - Harden save_via_temp_file: fsync parent dir, clean up temp on error, leave original file intact if the write fails - AppendOnlyFile::flush keeps buffer/rewind backup until write+sync succeed so discard() can roll back after a failed flush - Sync PMMR backends before committing the DB batch on extension commit; discard extension if sync fails - Add store::Error::DiskFull and chain::Error::DiskFull with operator-facing guidance (free space, avoid --clean unless needed) Tests: store disk_full suite + existing store/chain regression tests.
| // heed/lmdb may surface ENOSPC as a map full / I/O error string. | ||
| let lower = msg.to_lowercase(); | ||
| if lower.contains("map full") | ||
| || lower.contains("mdb_map_full") |
There was a problem hiding this comment.
map full means it should be resized, not necessary it can be full disk reason, need to check disk space on specific error instead of parsing its messages
| return true; | ||
| } | ||
| let msg = err.to_string().to_lowercase(); | ||
| msg.contains("no space left") || msg.contains("disk full") || msg.contains("not enough space") |
There was a problem hiding this comment.
We can no handle all possible messages here, so need to check disc space exactly to decide if it was a reason
wiesche89
left a comment
wiesche89
left a comment
There was a problem hiding this comment.
cargo fmt --all -- --check still reports formatting changes in store/src/pmmr.rs and store/src/types.rs.
| e | ||
| ); | ||
| } | ||
| // Keep buffer + buffer_start_pos_bak so discard() can roll back cleanly. |
There was a problem hiding this comment.
discard() only restores the in memory state here. set_len() may already have truncated the old tail, and write_all() may have written part of the new buffer. Could we make the original file state recoverable and test this partial write and restart path?
| // If disk is full, the batch stays uncommitted so chain head | ||
| // cannot point at incomplete on-disk MMR state. | ||
| if let Err(e) = (|| -> Result<(), std::io::Error> { | ||
| trees.output_pmmr_h.backend.sync()?; |
There was a problem hiding this comment.
These syncs are not atomic. If output succeeds and rangeproof, kernel, or the later DB commit fails, discard() cannot undo the already flushed output. Could we add a real recovery and reopen path and fault injection tests at each commit boundary?
| @@ -0,0 +1,126 @@ | |||
| // Copyright 2021 The Grin Developers | |||
| } | ||
|
|
||
| #[test] | ||
| fn data_file_flush_happy_path_after_hardening() { |
There was a problem hiding this comment.
This only exercises the normal flush path. Could we inject failures during a partial append, between PMMR syncs, and before the DB commit, then reopen the chain? Those are the actual #3425 recovery paths.
| // On some platforms (e.g. certain Windows configs) directory sync | ||
| // may not be supported; treat that as best-effort. | ||
| if let Ok(dir) = OpenOptions::new().read(true).open(parent) { | ||
| let _ = dir.sync_all(); |
There was a problem hiding this comment.
The comment promises a durable rename, but parent directory open and sync errors are ignored. Could we handle real I/O failures explicitly, or document this as best effort?
| pub fn map_io_err(err: io::Error) -> io::Error { | ||
| if is_out_of_disk_space(&err) { | ||
| io::Error::new( | ||
| io::ErrorKind::StorageFull, |
There was a problem hiding this comment.
map_io_err can wrap an existing StorageFull error repeatedly, so the final message may contain the same guidance several times. Could we keep it unchanged once it is already mapped?
| // limitations under the License. | ||
|
|
||
| //! Tests for out-of-disk-space detection and durable temp-file writes (#3425). | ||
|
|
There was a problem hiding this comment.
Could we move these tests into the existing test modules next to the code they cover, in lib.rs and types.rs? A separate integration-test file feels unnecessary here.
| /// data has been successfully written to disk. | ||
| /// | ||
| /// On failure the error is mapped to a clear out-of-disk-space message when | ||
| /// appropriate (#3425). Callers should discard the extension and not commit |
There was a problem hiding this comment.
The same #3425 rationale is repeated across the chain, PMMR, file backend, and tests. Could we keep the background in the PR description and leave only comments that explain a non obvious invariant?
| /// Creates temporary file with name created by adding `temp_suffix` to `path`. | ||
| /// Applies writer function to it and renames temporary file into original specified by `path`. | ||
| /// | ||
| /// Durability notes (see also #3352 / #3425): |
| /// | ||
| /// On failure (including out-of-disk-space), the in-memory buffer and rewind | ||
| /// backup are preserved so the caller can `discard()` the extension rather | ||
| /// than treating a half-written file as committed. See #3425. |
3 participants
Summary
Addresses #3425: out-of-disk-space during chain writes could leave the node in a state where restart failed with
failed to find head hashand operators resorted togrin --clean.Changes
store::is_out_of_disk_space/map_io_err) forErrorKind::StorageFulland UnixENOSPC.save_via_temp_file): fsync temp file (existing), fsync parent dir after rename, remove temp on failure so the original file is preserved.AppendOnlyFile::flush: do not clear the rewind backup / buffer until write+sync succeed; log a clear error if the write fails.store::Error::DiskFullandchain::Error::DiskFullwith guidance to free space and avoid wiping chain data unless recovery fails.Related: #3352 (fsync/leaf_set durability notes), unmerged #3266 (checkpoint idea — not implemented here).
Testing
cargo test -p grin_store(includes newdisk_fulltests)cargo test -p grin_chain --libcargo test -p grin_chain --test mine_simple_chainCompatibility
Closes #3425