Skip to content

node: exit non-zero on write-behind IO and re-ask after a store fault - #1232

Merged
reardencode merged 3 commits into
masterfrom
node/store-fault-exit
Oct 27, 2026
Merged

reardencode merged 3 commits into
masterfrom
node/store-fault-exit

Conversation

@rearden-grok

@rearden-grok rearden-grok Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

A scripthash or index write-behind IO error used the SIGTERM shutdown and exited 0, so Restart=on-failure left the unit down. That path now marks the shutdown failed and exits non-zero after the same flush. SIGTERM, Ctrl+C, and RPC stop still exit 0.

A tip-connect store fault still keeps the session and does not exit. on_block and drain_pending_once release the hub body ask, so the hash can be fetched again. The block is not cached invalid.

Fixes #1228.

Targeted shutdown_flag_and_node_handle_smoke and store_fault_on_block_releases_the_body_ask passed, and cargo fmt --all is clean. Workspace cargo test was not run on this host.

Scripthash and index write-behind used the SIGTERM shutdown, so a full
disk flushed and exited 0. Restart=on-failure then left the unit down.
SIGTERM, Ctrl+C, and RPC stop still exit 0.
A truncated body kept the session and left the block uncached, but the
hub ask stayed claimed until the cap. on_block and drain_pending_once
forget that ask so the same hash can be fetched again. The process still
exits only when write-behind IO fails.

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

1 bug, 0 suggestions, 0 nits.

Summary

Write-behind IO now calls Shutdown::fail before request, and run_p2p joins both write-behind threads before shutdown_result. failed() is therefore visible, process_main maps the Err to ExitCode::FAILURE (exit 1), and SIGTERM, Ctrl+C, and RPC stop still call request only. NodeError::Init only changes the log prefix (Error: …); the exit code and restart behavior stay the same as any other run_p2p error. A full block or a drain_pending_once store fault releases the hub ask without caching the block invalid. The truncated-txout.body spend is a real NetError::Store (pread short, not the “not found” substring), the test inserts requested_blocks so on_block actually accepts, and both claim_block_ask asserts fail if that arm does not forget. Store-not-found and consensus rejects still do not forget; a store-not-found accept therefore leaves the hub ask held until the 4096 cap. The same stuck ask remains on the near-tip compact path, which is how a tip block is fetched once sendcmpct v2 is on.

Disposition: request changes

}
Err(e @ NetError::Store(_)) => {
rbitcoin_log::warn!("p2p: accept dropped {hash} (local fault — keep session): {e}");
note_store_fault_keep_session(hub, hash, &e);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

[bug] note_store_fault_keep_session runs only from on_block_accept and drain_pending_once. A near-tip body is requested with Inventory::CompactBlock (getdata_use_compact) and queue_block_getdata inserts the hash into asked_blocks. When that cmpctblock reconstructs, on_cmpct_got_block removes it from requested_blocks and then accepts. NetError::Store falls through the _ arm, which does not call forget_asked_block (success, parent-needed, and Ok(_) do). The block is not put in pending_blocks, so the following drain_pending cannot hit the new drain arm either. already_have_or_asked_block stays true, claim_block_ask refuses the hash, and maybe_expire_block_requests never sees it because it is no longer in requested_blocks. A later inv of a header this node already knows does not send getdata. The tip stays frozen until asked_blocks hits 4096 and clears. A blocktxn store fault is not this case: on_blocktxn_unconnectable still asks for a witness block, and that on_block does forget.

Suggestion: On NetError::Store in on_cmpct_got_block, call note_store_fault_keep_session (and do not treat it as a compact-decode failure). Leave the consensus and store-not-found arms as they are. A pin that claims a compact ask, feeds a reconstructed block through on_cmpct_got_block against hub_with_truncated_body, and asserts claim_block_ask succeeds again would lock it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. A rebuilt compact block took the hash out of the in-flight list, and a store read error then left the ask in place. The node would not ask for that block again until the ask table filled up and cleared itself.

That path now drops the ask on a store read error, the same as a full block. A missing store row and a consensus reject still keep the ask. The new test builds a compact block, runs it through a hub whose txout file was cut to nothing, and checks the hash can be claimed again.

7493972

A reconstructed compact block dropped the hash from the in-flight set
and then left the hub ask in place on a store IO error. A later inv of
a header we already know did not send getdata until the ask table
cleared at 4096. on_cmpct_got_block now forgets that ask. A missing
store row and a consensus reject still do not.

@rearden-grok rearden-grok Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

0 bugs, 0 suggestions, 0 nits.

Summary

Write-behind IO now calls Shutdown::fail before the same cooperative shutdown as SIGTERM, and run_p2p maps that flag through shutdown_result to NodeError::Init, which cli_main turns into a non-zero exit. Signal handlers, RPC stop, and a single tip-connect NetError::Store still leave the flag clear, so those stay exit 0. The compact fix holds: on_cmpct_got_block drops the hash from requested_blocks before accept, then note_store_fault_keep_session forgets a real NetError::Store that is not a missing row, without parking the body or sending getheaders. StoreError::NotFound is the only store display that matches the guard (record not found); a short read, disk-full IO error, or corrupt record does not. on_blocktxn still leaves the ask in requested_blocks and the witness fallback forgets it on the same store fault, so that path does not stick until the 4096 cap. The new compact test claims the ask, feeds the truncated-txout.body spend that accept_received_block already classifies as NetError::Store, and asserts claim_block_ask again, so the pin fails if the arm does not forget.

Disposition: comment

@reardencode
reardencode merged commit 6e6dc9a into master Oct 27, 2026
18 checks passed
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.

Disk full (ENOSPC) makes rbitcoin-node exit with status 0, so Restart=on-failure never restarts it

1 participant