Skip to content

Commit 3109e14

Browse files
committed
fix(consensus): preserve transient error sources through startup chain
Address review: two startup signing paths reintroduced the alive-but-idle failure mode because their transient cause was lost before reaching is_retryable_startup_error. - validator_proof signing stringified the error via eyre::eyre!, breaking the downcast; wrap it with eyre::Report::new + wrap_err instead. - RetryExhausted dropped its underlying cause; carry it as #[source]. - Move retryability classification into RemoteSigningError, covering transient gRPC Status codes (Unavailable, DeadlineExceeded) and recursing through RetryExhausted.
1 parent 535016b commit 3109e14

6 files changed

Lines changed: 69 additions & 18 deletions

File tree

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/malachite-app/Cargo.toml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ reqwest = { workspace = true, features = ["json"] }
8989
serial_test = { workspace = true }
9090
tempfile = { workspace = true }
9191
tokio = { workspace = true, features = ["test-util"] }
92+
tonic.workspace = true
9293
tower = { workspace = true }
9394

9495
[lints]

‎crates/malachite-app/src/node.rs‎

Lines changed: 41 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -894,21 +894,13 @@ fn install_sigterm_handler(_handle: &Handle) {}
894894
/// authentication, malformed responses) return `false` so they still park for
895895
/// manual intervention rather than restart-looping.
896896
///
897-
/// Note: gRPC `Status` errors (`RemoteSigningError::Status`) are intentionally out
898-
/// of scope here — the failure this guards against is the eager `connect()` at
899-
/// startup, which surfaces as `Transport`. Keeping the set narrow avoids
900-
/// restart-looping on errors whose transience is ambiguous.
897+
/// The exact retryability rules live with `RemoteSigningError`, where tonic status
898+
/// codes and retry-exhaustion sources can be classified without string matching.
901899
fn is_retryable_startup_error(error: &eyre::Report) -> bool {
902-
error.chain().any(|cause| {
903-
matches!(
904-
cause.downcast_ref::<RemoteSigningError>(),
905-
Some(
906-
RemoteSigningError::Transport(_)
907-
| RemoteSigningError::ServiceUnavailable(_)
908-
| RemoteSigningError::Timeout(_)
909-
)
910-
)
911-
})
900+
error
901+
.chain()
902+
.filter_map(|cause| cause.downcast_ref::<RemoteSigningError>())
903+
.any(RemoteSigningError::is_retryable_startup_error)
912904
}
913905

914906
/// Wait for a termination signal (SIGTERM on Unix)
@@ -1049,6 +1041,14 @@ mod tests {
10491041
.wrap_err("Node failed to start")
10501042
}
10511043

1044+
fn wrapped_validator_proof_signing(err: RemoteSigningError) -> eyre::Report {
1045+
let signing_error = arc_consensus_types::signing::SigningError::from_source(err);
1046+
eyre::Report::new(signing_error)
1047+
.wrap_err("Failed to sign validator proof")
1048+
.wrap_err("Failed to create network identity with validator proof")
1049+
.wrap_err("Node failed to start")
1050+
}
1051+
10521052
#[test]
10531053
fn transient_remote_signer_errors_are_retryable() {
10541054
assert!(is_retryable_startup_error(&wrapped(
@@ -1057,6 +1057,22 @@ mod tests {
10571057
assert!(is_retryable_startup_error(&wrapped(
10581058
RemoteSigningError::Timeout("slow".into())
10591059
)));
1060+
assert!(is_retryable_startup_error(&wrapped(
1061+
RemoteSigningError::Status(Box::new(tonic::Status::unavailable("signer warming up")))
1062+
)));
1063+
assert!(is_retryable_startup_error(&wrapped(
1064+
RemoteSigningError::Status(Box::new(tonic::Status::deadline_exceeded(
1065+
"signer request timed out"
1066+
)))
1067+
)));
1068+
assert!(is_retryable_startup_error(
1069+
&wrapped_validator_proof_signing(RemoteSigningError::RetryExhausted {
1070+
retries: 3,
1071+
source: Box::new(RemoteSigningError::Status(Box::new(
1072+
tonic::Status::unavailable("signer warming up")
1073+
))),
1074+
})
1075+
));
10601076
}
10611077

10621078
#[test]
@@ -1067,6 +1083,17 @@ mod tests {
10671083
assert!(!is_retryable_startup_error(&wrapped(
10681084
RemoteSigningError::Authentication("denied".into())
10691085
)));
1086+
assert!(!is_retryable_startup_error(&wrapped(
1087+
RemoteSigningError::Status(Box::new(tonic::Status::unauthenticated("bad credentials")))
1088+
)));
1089+
assert!(!is_retryable_startup_error(
1090+
&wrapped_validator_proof_signing(RemoteSigningError::RetryExhausted {
1091+
retries: 3,
1092+
source: Box::new(RemoteSigningError::Status(Box::new(
1093+
tonic::Status::unauthenticated("bad credentials")
1094+
))),
1095+
})
1096+
));
10701097
}
10711098

10721099
#[test]

‎crates/malachite-app/src/validator_proof.rs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ use arc_consensus_types::codec::{network::NetCodec, Codec};
2525
use arc_consensus_types::signing::Signer;
2626
use arc_signer::ArcSigningProvider;
2727
use bytes::Bytes;
28-
use eyre::Result;
28+
use eyre::{Result, WrapErr};
2929
use tracing::info;
3030

3131
/// Create a signed validator proof binding the consensus public key to the P2P peer ID.
@@ -50,7 +50,8 @@ pub async fn create_validator_proof(
5050
let proof = signing_provider
5151
.sign_validator_proof(public_key_bytes, peer_id_bytes)
5252
.await
53-
.map_err(|e| eyre::eyre!("Failed to sign validator proof: {e}"))?;
53+
.map_err(eyre::Report::new)
54+
.wrap_err("Failed to sign validator proof")?;
5455

5556
let proof_bytes = NetCodec
5657
.encode(&proof)

‎crates/remote-signer/src/client.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,7 @@ impl RemoteSignerClient {
209209

210210
Err(RemoteSigningError::RetryExhausted {
211211
retries: config.retry_config.max_retries,
212+
source: Box::new(e),
212213
})
213214
}
214215
}
@@ -389,7 +390,7 @@ mod integration_tests {
389390
"Signing should fail with bad endpoint"
390391
);
391392

392-
if let Err(RemoteSigningError::RetryExhausted { retries }) = sign_result {
393+
if let Err(RemoteSigningError::RetryExhausted { retries, .. }) = sign_result {
393394
assert_eq!(retries, 2, "Should have exhausted exactly 2 retries");
394395
}
395396
}

‎crates/remote-signer/src/error.rs‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,11 +37,31 @@ pub enum RemoteSigningError {
3737
Timeout(String),
3838

3939
#[error("Retry exhausted after {retries} attempts")]
40-
RetryExhausted { retries: usize },
40+
RetryExhausted {
41+
retries: usize,
42+
#[source]
43+
source: Box<RemoteSigningError>,
44+
},
4145

4246
#[error("Configuration error: {0}")]
4347
Configuration(String),
4448

4549
#[error("Service unavailable: {0}")]
4650
ServiceUnavailable(String),
4751
}
52+
53+
impl RemoteSigningError {
54+
/// Returns `true` if this error represents a transient startup failure that a
55+
/// process restart is likely to recover from.
56+
pub fn is_retryable_startup_error(&self) -> bool {
57+
match self {
58+
Self::Transport(_) | Self::ServiceUnavailable(_) | Self::Timeout(_) => true,
59+
Self::Status(status) => matches!(
60+
status.code(),
61+
tonic::Code::Unavailable | tonic::Code::DeadlineExceeded
62+
),
63+
Self::RetryExhausted { source, .. } => source.is_retryable_startup_error(),
64+
Self::Authentication(_) | Self::Configuration(_) | Self::InvalidResponse(_) => false,
65+
}
66+
}
67+
}

0 commit comments

Comments
 (0)