feat: return clear error on storage-node disk full instead of hanging#3497
Open
halfprice wants to merge 2 commits into
Open
feat: return clear error on storage-node disk full instead of hanging#3497halfprice wants to merge 2 commits into
halfprice wants to merge 2 commits into
Conversation
The PUT sliver and PUT metadata endpoints awaited their RocksDB writes via `spawn_blocking` with no timeout. Because the write options never set `no_slowdown` and the metadata column family disables the write stall, a full disk can cause RocksDB to block indefinitely, hanging the request forever. When RocksDB did surface an error it collapsed into an opaque HTTP 500 with no signal that the node was out of space. Wrap the metadata and sliver write chokepoints in a configurable timeout and classify failures: when a write fails or times out and free disk space is below a configurable threshold, return a dedicated out-of-space error (HTTP 429 RESOURCE_EXHAUSTED) so clients get a clear signal that the node is full; otherwise return a write-timeout error (HTTP 504) instead of blocking. - add `OutOfSpace` and `StorageWriteTimeout` errors plus variants on `StoreMetadataError`/`StoreSliverError`, and a `StorageWriteFailure` classifier - add `Storage::available_disk_space` (statvfs via libc) - add `StorageWriteConfig` (write_timeout, min_available_disk_space) - wrap writes in `StorageNodeInner::run_storage_write` with timeout + disk-full classification at the request-path chokepoints
A timed-out `spawn_blocking` write cannot be cancelled: it occupies a blocking-pool thread until RocksDB returns. Under sustained disk-full, every request would stall for the full write timeout before returning 429, and each one would leak a blocked thread until the pool (512 threads) is exhausted — at which point all `spawn_blocking` work, including reads, queues indefinitely. Check available disk space before dispatching the write (a ~1µs statvfs call, negligible next to a RocksDB insert) and reject immediately with the out-of-space error, so a full node answers with an instant 429 instead of a 60-second stall and a leaked thread per request. Also extract the timeout-and-classify logic into a free `classify_storage_write` function so its behavior is unit-testable without a full storage node, and add tests covering the up-front rejection, timeout classification, error passthrough, and reclassification of failures that race with the disk filling up. Regenerate `node_config_example.yaml` and `storage_openapi.yaml` (golden-file tests) for the new `storage_write` config section and the 429/504 responses now documented on the PUT metadata/sliver endpoints.
halfprice
force-pushed
the
zhewu/reject_writes_when_disk_full
branch
from
282b8e6 to
9f832a5
Compare
Contributor
|
Warning: This PR modifies one of the OpenAPI files. Please consider the
|
Collaborator
Author
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f832a5db1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+3681
to
+3683
| self.run_storage_write(self.storage.put_verified_metadata(&metadata)) | ||
| .await | ||
| .context("unable to persist pending metadata") | ||
| .map_err(StoreMetadataError::Internal)?; | ||
| .map_err(StoreMetadataError::from)?; |
There was a problem hiding this comment.
When a blob was uploaded with pending=true and later becomes registered while the node is under the new disk-space threshold (or the write times out), this call returns OutOfSpace/WriteTimeout after the metadata was already removed by the surrounding pending_metadata_cache.remove(...). Unlike the sliver flush path, the metadata is not inserted back into the pending cache before returning, so a transient full-disk/timeout condition can permanently drop the buffered metadata and leave the pending upload unable to flush after space is freed.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The storage node's PUT sliver and PUT metadata endpoints awaited their RocksDB writes via
spawn_blockingwith no timeout. Combined with:no_slowdown, anddatabase_config.rs),a full disk can cause RocksDB to block indefinitely, hanging the request forever. There was no disk-space monitoring, and when RocksDB did return an error it collapsed into an opaque HTTP 500
Internalwith no signal that the node was out of space.Change
Bound the metadata and sliver write chokepoints with a configurable timeout, and check free disk space so a full node answers with a clear error:
statvfscall). If it is below the configured threshold, reject immediately with HTTP 429RESOURCE_EXHAUSTED("the storage node is out of disk space"). This matters because a timed-outspawn_blockingwrite cannot be cancelled — without the up-front check, a full node would stall every request for the full timeout and leak one blocked thread per request until the blocking pool (512 threads) is exhausted, hanging reads too.DEADLINE_EXCEEDEDinstead of blocking, and a genuine DB error still maps to HTTP 500 as before.Details
OutOfSpace(429) andStorageWriteTimeout(504) errors, added as variants onStoreMetadataError/StoreSliverError, plus aStorageWriteFailureclassifier; cross-error mappings preserve the disk-full/timeout semantics.Storage::available_disk_spacevialibc::statvfs(libc already a workspace dep; non-Unix falls back to "never full").StorageWriteConfig { write_timeout (default 60s), min_available_disk_space (default 1 GiB) }, configurable with serde defaults.classify_storage_write(free function, unit-testable) applies pre-check + timeout + classification;StorageNodeInner::run_storage_writewraps it at the three request-path chokepoints:store_sliver_unchecked,persist_verified_metadata,flush_pending_metadata.node_config_example.yamlandstorage_openapi.yamlgolden files (new config section; 429/504 documented on the PUT endpoints).Testing
cargo clippy --all-features --all-targetsclean (0 warnings); formatted.available_disk_space.Notes / follow-ups