mirror of
https://github.com/tinyhumansai/openhuman.git
synced 2026-07-27 21:08:00 +00:00
Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
sanil-23
+102
-2
@@ -82,8 +82,63 @@ pub async fn dispatch(
|
||||
return result;
|
||||
}
|
||||
|
||||
log::warn!("[rpc:dispatch] unknown_method method={}", method);
|
||||
Err(format!("unknown method: {method}"))
|
||||
// Tier 4: unrecognised method. The JSON-RPC response is unchanged — the
|
||||
// caller still receives a method-not-found error. Only the *severity* of
|
||||
// how the transport layer records it differs by class (see
|
||||
// `jsonrpc::rpc_handler`): known external probes (`is_known_probe_method`)
|
||||
// are debug-only and never reach Sentry (#3567), while any other unknown
|
||||
// method is downgraded to a warn-level capture (recorded for triage, no
|
||||
// page) instead of an error event. Log here at debug with the method name
|
||||
// so the path stays diagnosable without re-creating the Sentry noise.
|
||||
if is_known_probe_method(method) {
|
||||
log::debug!(
|
||||
"[rpc] unknown_method method={} class=known_probe (debug-only; not reported to Sentry)",
|
||||
method
|
||||
);
|
||||
} else {
|
||||
log::debug!(
|
||||
"[rpc] unknown_method method={} class=unrecognized (reported to Sentry at warn for triage)",
|
||||
method
|
||||
);
|
||||
}
|
||||
Err(format!("{UNKNOWN_METHOD_PREFIX}{method}"))
|
||||
}
|
||||
|
||||
/// Prefix of the error string returned for an unrecognised RPC method. Kept as
|
||||
/// a shared constant so the emit site (above) and the transport-layer
|
||||
/// classifier ([`unknown_method_name`]) cannot drift apart.
|
||||
pub const UNKNOWN_METHOD_PREFIX: &str = "unknown method: ";
|
||||
|
||||
/// Generic external probe / legacy method names that are never real RPC
|
||||
/// methods and never will be (issue #3567). Infra health-checks and JSON-RPC
|
||||
/// introspection clients poll these — `rpc.discover` (JSON-RPC service
|
||||
/// discovery), `list_methods`, liveness `status`, `auth.status`, `config/get`
|
||||
/// — and each miss previously produced a recurring Sentry ERROR event with
|
||||
/// zero user impact. The transport layer keeps these debug-only (never
|
||||
/// captured). The matching health-method *aliases* land separately in
|
||||
/// `legacy_aliases` (#3566), which depends on this severity change.
|
||||
const KNOWN_PROBE_METHODS: &[&str] = &[
|
||||
"rpc.discover",
|
||||
"list_methods",
|
||||
"status",
|
||||
"auth.status",
|
||||
"config/get",
|
||||
];
|
||||
|
||||
/// Returns `true` when `method` is a known external probe / legacy health name
|
||||
/// from [`KNOWN_PROBE_METHODS`]. Matched against the *resolved* method name
|
||||
/// (after legacy-alias rewrite), i.e. the name embedded in the
|
||||
/// [`UNKNOWN_METHOD_PREFIX`] error string.
|
||||
pub fn is_known_probe_method(method: &str) -> bool {
|
||||
KNOWN_PROBE_METHODS.contains(&method)
|
||||
}
|
||||
|
||||
/// Extracts the offending method name from an unknown-method error string, or
|
||||
/// `None` if `message` is not an unknown-method error. The transport layer uses
|
||||
/// this to classify the failure for Sentry severity without re-deriving the
|
||||
/// method from the request (which may differ post legacy-alias rewrite).
|
||||
pub fn unknown_method_name(message: &str) -> Option<&str> {
|
||||
message.strip_prefix(UNKNOWN_METHOD_PREFIX)
|
||||
}
|
||||
|
||||
/// Handles internal core-level RPC methods.
|
||||
@@ -314,6 +369,51 @@ mod tests {
|
||||
assert_eq!(out, json!({ "ok": true }));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn is_known_probe_method_matches_allow_list_exactly() {
|
||||
// Every allow-listed probe / legacy health name is recognised.
|
||||
for m in [
|
||||
"rpc.discover",
|
||||
"list_methods",
|
||||
"status",
|
||||
"auth.status",
|
||||
"config/get",
|
||||
] {
|
||||
assert!(is_known_probe_method(m), "{m} should be a known probe");
|
||||
}
|
||||
// Genuinely-unknown methods and near-misses are NOT allow-listed, so
|
||||
// they stay on the warn-for-triage path rather than being silenced.
|
||||
assert!(!is_known_probe_method("does.not.exist"));
|
||||
assert!(!is_known_probe_method("core.not_a_real_method"));
|
||||
assert!(!is_known_probe_method("Status")); // case-sensitive
|
||||
assert!(!is_known_probe_method("rpc.discover.extra")); // exact match only
|
||||
assert!(!is_known_probe_method(""));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn unknown_method_name_extracts_from_error_string_only() {
|
||||
// The classifier round-trips the exact string `dispatch` emits.
|
||||
let err = format!("{UNKNOWN_METHOD_PREFIX}rpc.discover");
|
||||
assert_eq!(unknown_method_name(&err), Some("rpc.discover"));
|
||||
// Unrelated error strings are not misclassified as unknown-method.
|
||||
assert_eq!(unknown_method_name("unknown param 'x' for ns.fn"), None);
|
||||
assert_eq!(unknown_method_name("Session expired"), None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn dispatch_probe_method_still_returns_unknown_method_error() {
|
||||
// Allow-listed probe names must not be silently "handled" — the caller
|
||||
// still gets a method-not-found error. Only the Sentry severity (in the
|
||||
// transport layer) changes; the dispatch contract is unchanged.
|
||||
let err = dispatch(test_state(), "rpc.discover", json!({}))
|
||||
.await
|
||||
.expect_err("probe methods are still unknown to the dispatcher");
|
||||
assert_eq!(unknown_method_name(&err), Some("rpc.discover"));
|
||||
assert!(is_known_probe_method(
|
||||
unknown_method_name(&err).expect("unknown-method error")
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn dispatch_legacy_alias_routes_to_registry() {
|
||||
// openhuman.get_analytics_settings should rewrite to openhuman.config_get_analytics_settings.
|
||||
|
||||
@@ -132,6 +132,31 @@ pub async fn rpc_handler(State(state): State<AppState>, Json(req): Json<RpcReque
|
||||
error = %redacted,
|
||||
"[rpc] transient downstream failure — not reporting to Sentry (message redacted)"
|
||||
);
|
||||
} else if let Some(unknown_method) =
|
||||
crate::core::dispatch::unknown_method_name(&display_message)
|
||||
{
|
||||
// An unrecognised RPC method is a transport-boundary mismatch
|
||||
// (infra probe traffic, or a client on a different release than
|
||||
// the running core), not an actionable core defect (#3567).
|
||||
// Known external probes never become real methods, so they are
|
||||
// debug-only and never reach Sentry; any other unknown method
|
||||
// is still recorded for triage but at warn severity (captured,
|
||||
// no page) rather than an error event. Either way the JSON-RPC
|
||||
// method-not-found response to the caller below is unchanged.
|
||||
if crate::core::dispatch::is_known_probe_method(unknown_method) {
|
||||
tracing::debug!(
|
||||
method = %method,
|
||||
elapsed_ms = ms as u64,
|
||||
"[rpc] unknown probe/legacy method (allow-listed) — debug only, not reporting to Sentry"
|
||||
);
|
||||
} else {
|
||||
crate::core::observability::report_warning_message(
|
||||
display_message.as_str(),
|
||||
"rpc",
|
||||
"invoke_method",
|
||||
&[("method", method.as_str()), ("elapsed_ms", &ms.to_string())],
|
||||
);
|
||||
}
|
||||
} else {
|
||||
crate::core::observability::report_error_or_expected(
|
||||
display_message.as_str(),
|
||||
|
||||
@@ -970,6 +970,123 @@ async fn thread_not_found_rpc_error_does_not_report_to_sentry() {
|
||||
events[0].tags.get("method").map(String::as_str),
|
||||
Some("core.not_a_real_method")
|
||||
);
|
||||
// #3567: an unrecognised (non-allow-listed) method is still recorded for
|
||||
// triage, but downgraded from error to *warning* severity so it no longer
|
||||
// pages. The JSON-RPC method-not-found response above is unchanged.
|
||||
assert_eq!(
|
||||
events[0].level,
|
||||
sentry::Level::Warning,
|
||||
"unknown-method events should be warn-level (triage, not paging)"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
async fn unknown_method_severity_split_by_probe_allow_list() {
|
||||
// #3567: prove the full severity split at the transport boundary —
|
||||
// (1) an allow-listed probe name is NOT captured to Sentry (debug-only),
|
||||
// (2) a genuinely-unknown method still surfaces at warn for triage,
|
||||
// (3) the JSON-RPC error response to the caller is unchanged in both cases.
|
||||
use axum::body::to_bytes;
|
||||
use axum::extract::State;
|
||||
use axum::Json;
|
||||
use sentry::test::TestTransport;
|
||||
use tracing::Level;
|
||||
use tracing_subscriber::layer::SubscriberExt;
|
||||
|
||||
let workspace = tempfile::tempdir().expect("workspace tempdir");
|
||||
let _env = EnvVarGuard::set_many(vec![(
|
||||
"OPENHUMAN_WORKSPACE",
|
||||
workspace.path().as_os_str().to_os_string(),
|
||||
)]);
|
||||
|
||||
let transport = TestTransport::new();
|
||||
let sentry_options = sentry::ClientOptions {
|
||||
dsn: Some("https://public@sentry.invalid/1".parse().unwrap()),
|
||||
transport: Some(Arc::new(transport.clone())),
|
||||
..Default::default()
|
||||
};
|
||||
let sentry_hub = Arc::new(sentry::Hub::new(
|
||||
Some(Arc::new(sentry_options.into())),
|
||||
Arc::new(Default::default()),
|
||||
));
|
||||
let _sentry_guard = sentry::HubSwitchGuard::new(sentry_hub);
|
||||
|
||||
let subscriber = tracing_subscriber::registry().with(
|
||||
sentry::integrations::tracing::layer().event_filter(|metadata| {
|
||||
// Mirror production: diagnostics from the report_* helpers are
|
||||
// captured directly via `sentry::capture_message`, so the bridge
|
||||
// must ignore their marker target to avoid double events.
|
||||
if metadata.target() == crate::core::observability::REPORT_ERROR_TRACING_TARGET {
|
||||
return sentry::integrations::tracing::EventFilter::Ignore;
|
||||
}
|
||||
match *metadata.level() {
|
||||
Level::ERROR => sentry::integrations::tracing::EventFilter::Event,
|
||||
Level::WARN | Level::INFO => sentry::integrations::tracing::EventFilter::Breadcrumb,
|
||||
_ => sentry::integrations::tracing::EventFilter::Ignore,
|
||||
}
|
||||
}),
|
||||
);
|
||||
let _subscriber_guard = tracing::subscriber::set_default(subscriber);
|
||||
|
||||
// (1) Allow-listed probe → debug-only, never reaches Sentry.
|
||||
let probe_request = crate::core::types::RpcRequest {
|
||||
jsonrpc: "2.0".to_string(),
|
||||
id: json!(1),
|
||||
method: "rpc.discover".to_string(),
|
||||
params: json!({}),
|
||||
};
|
||||
let response = rpc_handler(State(default_state()), Json(probe_request)).await;
|
||||
let body = to_bytes(response.into_body(), usize::MAX)
|
||||
.await
|
||||
.expect("response body");
|
||||
let body: serde_json::Value = serde_json::from_slice(&body).expect("json response");
|
||||
// (3) Response is the unchanged JSON-RPC method-not-found envelope.
|
||||
assert_eq!(body["error"]["code"], json!(-32000));
|
||||
assert_eq!(
|
||||
body["error"]["message"],
|
||||
json!("unknown method: rpc.discover")
|
||||
);
|
||||
assert_eq!(body["error"]["data"], serde_json::Value::Null);
|
||||
assert!(
|
||||
transport.fetch_and_clear_events().is_empty(),
|
||||
"allow-listed probe methods must not reach Sentry"
|
||||
);
|
||||
|
||||
// (2) Genuinely-unknown method → still captured, but at warn for triage.
|
||||
let unknown_request = crate::core::types::RpcRequest {
|
||||
jsonrpc: "2.0".to_string(),
|
||||
id: json!(2),
|
||||
method: "totally.made.up.method".to_string(),
|
||||
params: json!({}),
|
||||
};
|
||||
let response = rpc_handler(State(default_state()), Json(unknown_request)).await;
|
||||
let body = to_bytes(response.into_body(), usize::MAX)
|
||||
.await
|
||||
.expect("response body");
|
||||
let body: serde_json::Value = serde_json::from_slice(&body).expect("json response");
|
||||
// (3) Same unchanged method-not-found envelope for the unknown method.
|
||||
assert_eq!(body["error"]["code"], json!(-32000));
|
||||
assert_eq!(
|
||||
body["error"]["message"],
|
||||
json!("unknown method: totally.made.up.method")
|
||||
);
|
||||
assert_eq!(body["error"]["data"], serde_json::Value::Null);
|
||||
|
||||
let events = transport.fetch_and_clear_events();
|
||||
assert_eq!(
|
||||
events.len(),
|
||||
1,
|
||||
"genuinely-unknown methods should still be captured for triage"
|
||||
);
|
||||
assert_eq!(events[0].level, sentry::Level::Warning);
|
||||
assert_eq!(
|
||||
events[0].tags.get("domain").map(String::as_str),
|
||||
Some("rpc")
|
||||
);
|
||||
assert_eq!(
|
||||
events[0].tags.get("method").map(String::as_str),
|
||||
Some("totally.made.up.method")
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -1831,6 +1831,48 @@ pub(crate) fn report_error_message(
|
||||
);
|
||||
}
|
||||
|
||||
/// Capture a message to Sentry at **warning** severity with structured tags.
|
||||
///
|
||||
/// Mirror of [`report_error_message`] but at `sentry::Level::Warning`: the
|
||||
/// event is still recorded in Sentry (so it stays available for triage and
|
||||
/// dashboards) while warning-severity events do not trip the error-rate
|
||||
/// alert/paging rules that `Level::Error` events do (see the
|
||||
/// `sentry_tracing_layer` mapping in `core::logging`, where `ERROR` becomes a
|
||||
/// captured `Event` and `WARN`/`INFO` only a `Breadcrumb`). Use this for
|
||||
/// transport-boundary conditions worth seeing in aggregate that are never an
|
||||
/// actionable core defect — e.g. unrecognised RPC method names (#3567).
|
||||
///
|
||||
/// Like [`report_error_message`], capture is an explicit, synchronous
|
||||
/// `sentry::capture_message` rather than the `sentry-tracing` bridge; the
|
||||
/// accompanying diagnostic line is tagged with [`REPORT_ERROR_TRACING_TARGET`]
|
||||
/// so the production layer ignores it and we never double-report.
|
||||
pub(crate) fn report_warning_message(
|
||||
message: &str,
|
||||
domain: &str,
|
||||
operation: &str,
|
||||
extra: &[Tag<'_>],
|
||||
) {
|
||||
sentry::with_scope(
|
||||
|scope| {
|
||||
scope.set_tag("domain", domain);
|
||||
scope.set_tag("operation", operation);
|
||||
for (k, v) in extra {
|
||||
scope.set_tag(k, v);
|
||||
}
|
||||
},
|
||||
|| {
|
||||
sentry::capture_message(message, sentry::Level::Warning);
|
||||
tracing::warn!(
|
||||
target: REPORT_ERROR_TRACING_TARGET,
|
||||
domain = domain,
|
||||
operation = operation,
|
||||
message = %message,
|
||||
"[observability] {domain}.{operation} warning: {message}"
|
||||
);
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
/// Returns true when a Sentry event is a per-attempt provider HTTP failure
|
||||
/// that the reliable-provider layer already handles via retry + fallback.
|
||||
///
|
||||
|
||||
Reference in New Issue
Block a user