From 1e79c93ea87a366053765d6041a14f7426ea2d1d Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Mon, 31 Aug 2026 22:50:52 +0000 Subject: [PATCH 1/5] acl: surface Trident error (kind/subkind/location) on the update-status annotation Adds an optional tridentError object to the ACL A/B update-status annotation schema, populated from TridentClientError::Remote when a stage, finalize, rollback, or commit operation fails against tridentd. The object carries kind, subkind, and an optional source location so downstream consumers can distinguish and correlate Trident-originated failures without parsing the free-form message field. --- .../src/annotations/orchestrator.rs | 19 +- .../src/annotations/protocol.rs | 181 +++++++++++++++++- .../src/core/trident/client.rs | 13 +- 3 files changed, 203 insertions(+), 10 deletions(-) diff --git a/crates/trident-acl-agent/src/annotations/orchestrator.rs b/crates/trident-acl-agent/src/annotations/orchestrator.rs index b2d15323e..a776bf367 100644 --- a/crates/trident-acl-agent/src/annotations/orchestrator.rs +++ b/crates/trident-acl-agent/src/annotations/orchestrator.rs @@ -231,6 +231,7 @@ impl Orchestrator { operation: invalid.operation, code: StatusCode::InvalidRequest, message: invalid.reason, + trident_error: None, from_version: None, to_version: None, started_utc: now, @@ -1535,7 +1536,8 @@ fn stage_result_to_status( to_version, started, Some(Utc::now()), - ), + ) + .with_trident_error(&err), } } @@ -1583,6 +1585,7 @@ fn finalize_failure_status( started, Some(Utc::now()), ) + .with_trident_error(err) } /// Maps a `commit()` (or `update_finalize()`/`rollback_finalize()` sharing @@ -1730,7 +1733,8 @@ fn reconstruct_commit_result_to_status( request.target_version.clone(), started, Some(Utc::now()), - ), + ) + .with_trident_error(&err), Err(err) => UpdateStatus::new( request, Operation::Commit, @@ -1741,7 +1745,8 @@ fn reconstruct_commit_result_to_status( request.target_version.clone(), started, Some(Utc::now()), - ), + ) + .with_trident_error(&err), } } @@ -1808,7 +1813,8 @@ fn commit_result_to_status( pending.to_version.clone(), pending.started_utc, Some(Utc::now()), - ), + ) + .with_trident_error(&err), Err(err) => UpdateStatus::new( &pending.request, Operation::Commit, @@ -1819,7 +1825,8 @@ fn commit_result_to_status( pending.to_version.clone(), pending.started_utc, Some(Utc::now()), - ), + ) + .with_trident_error(&err), } } @@ -1844,6 +1851,7 @@ fn rollback_stage_failure_status( started, Some(Utc::now()), ) + .with_trident_error(err) } /// Builds the terminal `UpdateStatus` for a successful rollback_finalize() @@ -1887,6 +1895,7 @@ fn rollback_finalize_failure_status( started, Some(Utc::now()), ) + .with_trident_error(err) } #[cfg(test)] diff --git a/crates/trident-acl-agent/src/annotations/protocol.rs b/crates/trident-acl-agent/src/annotations/protocol.rs index 515440cc4..fcf984330 100644 --- a/crates/trident-acl-agent/src/annotations/protocol.rs +++ b/crates/trident-acl-agent/src/annotations/protocol.rs @@ -19,7 +19,16 @@ use serde::{Deserialize, Serialize}; use url::Url; use uuid::Uuid; -use crate::core::{config::DEFAULT_ANNOTATION_PREFIX, error::AgentError}; +use crate::core::{ + config::DEFAULT_ANNOTATION_PREFIX, error::AgentError, trident::TridentClientError, +}; + +/// Sentinel `kind` used when Trident reported a remote failure without any +/// structured error at all (see `client.rs`'s `UNKNOWN_REMOTE_ERROR_SUBKIND` +/// for the matching `subkind` sentinel) - keeps `TridentErrorInfo::kind` +/// non-optional instead of adding a third `Option` layer for a case that +/// should already be rare/a Trident-side contract violation. +const UNKNOWN_ERROR_KIND: &str = "unknown"; /// Suffix (appended to the configured annotation prefix) for the request /// annotation, e.g. `acl.microsoft.com/update-request`. @@ -136,6 +145,8 @@ pub struct UpdateStatus { pub code: StatusCode, pub message: String, #[serde(default, skip_serializing_if = "Option::is_none")] + pub trident_error: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] pub from_version: Option, #[serde(default, skip_serializing_if = "Option::is_none")] pub to_version: Option, @@ -145,6 +156,30 @@ pub struct UpdateStatus { pub finished_utc: Option>, } +/// A structured Trident error surfaced on a failure status. Mirrors +/// `TridentError`'s `kind`/`subkind`/`location` - its `message`/ +/// `error_message` are not duplicated here since `UpdateStatus::message` +/// already carries the human-readable failure text. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct TridentErrorInfo { + /// TridentError kind, e.g. `SERVICING_ERROR`, `HEALTH_CHECKS_ERROR`. + pub kind: String, + /// Finer-grained identifier within `kind`, e.g. + /// `ab-update-reboot-check`. + pub subkind: String, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub location: Option, +} + +/// Location in Trident's source where a `TridentErrorInfo` originated. +#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq)] +#[serde(rename_all = "camelCase", deny_unknown_fields)] +pub struct ErrorLocation { + pub path: String, + pub line: u32, +} + impl UpdateRequest { /// Enforces the same constraints as the request annotation's formal /// JSON Schema: schemaVersion match, @@ -233,6 +268,7 @@ impl UpdateStatus { operation, code, message: truncate_message(message.into()), + trident_error: None, from_version, to_version, started_utc, @@ -248,6 +284,29 @@ impl UpdateStatus { refreshed } + /// Populates `trident_error` from a Trident-reported remote error, if + /// present. Non-`Remote` `TridentClientError` variants (connect/timeout/ + /// stream failures) and agent-generated failures (e.g. `InvalidRequest`) + /// carry no structured Trident error, so this is a no-op for them - + /// `trident_error` stays unset, since only a subset of failure codes + /// actually originate from Trident. + pub fn with_trident_error(mut self, err: &TridentClientError) -> Self { + if let Some(remote) = err.remote() { + self.trident_error = Some(TridentErrorInfo { + kind: remote + .kind + .map(|kind| kind.as_str_name().to_string()) + .unwrap_or_else(|| UNKNOWN_ERROR_KIND.to_string()), + subkind: remote.subkind.clone(), + location: remote.location.as_ref().map(|location| ErrorLocation { + path: location.path.clone(), + line: location.line, + }), + }); + } + self + } + /// Compares two statuses ignoring `last_updated_utc`. /// /// `publish_status` stamps a fresh `last_updated_utc` on every write via @@ -265,6 +324,7 @@ impl UpdateStatus { && self.operation == other.operation && self.code == other.code && self.message == other.message + && self.trident_error == other.trident_error && self.from_version == other.from_version && self.to_version == other.to_version && self.started_utc == other.started_utc @@ -757,6 +817,26 @@ mod tests { "operation": { "type": "string", "enum": ["stage", "finalize", "rollback", "commit"] }, "code": { "type": "string", "enum": ["InProgress", "Success", "AlreadyAtTarget", "NotStaged", "OperationFailed", "TargetBootFailed", "AgentInternalError", "InvalidRequest"] }, "message": { "type": "string", "maxLength": 2048 }, + "tridentError": { + "type": "object", + "additionalProperties": false, + "required": ["kind", "subkind"], + "description": "Structured Trident error, present only when the failure originated from a Trident remote error - a subset of failure codes carry this; most do not.", + "properties": { + "kind": { "type": "string", "description": "TridentError kind, e.g. SERVICING_ERROR, HEALTH_CHECKS_ERROR." }, + "subkind": { "type": "string", "description": "Finer-grained identifier within kind, e.g. ab-update-reboot-check." }, + "location": { + "type": "object", + "additionalProperties": false, + "required": ["path", "line"], + "description": "Location in Trident's source where the error originated.", + "properties": { + "path": { "type": "string" }, + "line": { "type": "integer" } + } + } + } + }, "fromVersion": { "type": "string" }, "toVersion": { "type": "string" }, "startedUtc": { "type": "string", "format": "date-time" }, @@ -900,6 +980,13 @@ mod tests { "property {name:?}: expected type {expected_type}, got {value:?}" )); } + // Recurse into nested object schemas (e.g. `tridentError`/`location`) + // so their own properties/required/additionalProperties are + // checked too, not just "is this an object". + if expected_type == "object" { + schema_validate(prop_schema, value) + .map_err(|err| format!("property {name:?}: {err}"))?; + } } if let Some(const_value) = prop_schema.get("const") { if value != const_value { @@ -1227,6 +1314,98 @@ mod tests { } } + #[test] + fn with_trident_error_populates_kind_subkind_and_location_and_conforms_to_schema() { + use trident_proto::v1::{FileLocation, TridentErrorKind}; + + let schema: Value = serde_json::from_str(DESIGN_DOC_STATUS_SCHEMA).unwrap(); + let request = UpdateRequest { + schema_version: SCHEMA_VERSION.to_string(), + node_update_id: Uuid::new_v4(), + operation_id: Uuid::new_v4().to_string(), + operation: RequestedOperation::Finalize, + target_version: Some("202606.29.0".to_string()), + server: None, + app_id: None, + track: None, + }; + + use crate::core::trident::RemoteError; + let remote_err = TridentClientError::Remote { + operation: "commit", + details: RemoteError { + kind: Some(TridentErrorKind::ServicingError), + subkind: "ab-update-reboot-check".to_string(), + message: "reboot check failed".to_string(), + error_message: "reboot check failed".to_string(), + location: Some(FileLocation { + path: "crates/trident/src/servicing.rs".to_string(), + line: 42, + }), + }, + }; + let status = UpdateStatus::new( + &request, + Operation::Commit, + request.operation_id.clone(), + StatusCode::TargetBootFailed, + format!("commit detected rollback to previous version: {remote_err}"), + Some("1.0.0".to_string()), + Some("2.0.0".to_string()), + fixed_time(0), + Some(fixed_time(5)), + ) + .with_trident_error(&remote_err); + + let trident_error = status + .trident_error + .as_ref() + .expect("trident_error must be populated"); + assert_eq!(trident_error.kind, "SERVICING_ERROR"); + assert_eq!(trident_error.subkind, "ab-update-reboot-check"); + let location = trident_error + .location + .as_ref() + .expect("location must be populated"); + assert_eq!(location.path, "crates/trident/src/servicing.rs"); + assert_eq!(location.line, 42); + schema_validate(&schema, &serde_json::to_value(&status).unwrap()) + .expect("status carrying trident_error must still conform to the formal schema"); + } + + #[test] + fn with_trident_error_is_noop_for_non_remote_errors() { + let request = UpdateRequest { + schema_version: SCHEMA_VERSION.to_string(), + node_update_id: Uuid::new_v4(), + operation_id: Uuid::new_v4().to_string(), + operation: RequestedOperation::Finalize, + target_version: Some("202606.29.0".to_string()), + server: None, + app_id: None, + track: None, + }; + + let timeout_err = TridentClientError::Timeout { + operation: "commit", + timeout: std::time::Duration::from_secs(5), + }; + let status = UpdateStatus::new( + &request, + Operation::Commit, + request.operation_id.clone(), + StatusCode::OperationFailed, + format!("commit failed: {timeout_err}"), + Some("1.0.0".to_string()), + Some("2.0.0".to_string()), + fixed_time(0), + Some(fixed_time(5)), + ) + .with_trident_error(&timeout_err); + + assert_eq!(status.trident_error, None); + } + #[test] fn agent_built_statuses_conform_to_formal_schema() { let schema: Value = serde_json::from_str(DESIGN_DOC_STATUS_SCHEMA).unwrap(); diff --git a/crates/trident-acl-agent/src/core/trident/client.rs b/crates/trident-acl-agent/src/core/trident/client.rs index 37432c793..5515b78e5 100644 --- a/crates/trident-acl-agent/src/core/trident/client.rs +++ b/crates/trident-acl-agent/src/core/trident/client.rs @@ -28,10 +28,10 @@ use tonic::{ use trident_proto::v1::{ commit_service_client::CommitServiceClient, rollback_service_client::RollbackServiceClient, servicing_response::Response as ResponseBody, update_service_client::UpdateServiceClient, - CommitRequest, FinalizeUpdateRequest, HostConfiguration, LogLevel, ManualRollbackKind, - RebootHandling, RebootManagement, RebootStatus, RollbackFinalizeRequest, RollbackStageRequest, - ServicingKind, ServicingResponse, StageUpdateRequest, StatusCode, TridentErrorKind, - UpdateRequest, + CommitRequest, FileLocation, FinalizeUpdateRequest, HostConfiguration, LogLevel, + ManualRollbackKind, RebootHandling, RebootManagement, RebootStatus, RollbackFinalizeRequest, + RollbackStageRequest, ServicingKind, ServicingResponse, StageUpdateRequest, StatusCode, + TridentErrorKind, UpdateRequest, }; use url::Url; @@ -47,6 +47,9 @@ pub struct RemoteError { pub subkind: String, pub message: String, pub error_message: String, + /// Location in Trident's source where the error originated, when + /// Trident's structured error reported one. + pub location: Option, } #[derive(Debug, Error)] @@ -395,12 +398,14 @@ async fn consume_servicing_stream( subkind: error.subkind, message: error.message, error_message: error.error_message, + location: error.location, }) .unwrap_or(RemoteError { kind: None, subkind: UNKNOWN_REMOTE_ERROR_SUBKIND.to_string(), message: format!("Trident {operation} failed without structured error"), error_message: String::new(), + location: None, }); return Err(TridentClientError::Remote { operation, details }); } From 5a562ae776c63dbbb7d9d71b55d1c94a611a04d9 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Tue, 1 Sep 2026 17:28:01 +0000 Subject: [PATCH 2/5] docs: document tridentError in the update-status annotation --- docs/Explanation/Trident-ACL-Agent.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index b48c2bee7..43b5bc63b 100644 --- a/docs/Explanation/Trident-ACL-Agent.md +++ b/docs/Explanation/Trident-ACL-Agent.md @@ -179,6 +179,13 @@ A status annotation (`/update-status` or match on (it may include error detail that varies run to run). - `fromVersion`/`toVersion` are the versions the operation moved between (`toVersion` is absent for `rollback`, whose target is implicit). +- `tridentError` is present only when the failure originated from a + structured Trident remote error (a subset of `OperationFailed` failures; + most `OperationFailed` statuses - and every other code - carry no + `tridentError`). It has `kind`/`subkind` (Trident's own error + classification, e.g. `SERVICING_ERROR`/`ab-update-reboot-check`) and an + optional `location` (`path`/`line` in Trident's source), letting an + orchestrator key off structured fields instead of parsing `message`. - `startedUtc`/`lastUpdatedUtc`/`finishedUtc` bound the operation: `lastUpdatedUtc` refreshes on a heartbeat cadence while `code` is `InProgress` (see [below](#pre-post-reboot-state-and-the-watchdog)); From 300d7cebcff82e200eccf464b836b4c2d128d7ea Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Tue, 1 Sep 2026 19:34:33 +0000 Subject: [PATCH 3/5] acl-agent: constrain error location line to u32 range in formal schema ErrorLocation::line and protobuf FileLocation::line are u32, but the formal JSON Schema allowed any integer (including negative and >u32::MAX values), so a schema-valid persisted status could fail to deserialize. Constrain line to 0..=4294967295 and extend the local schema validator (schema_validate_property) with minimum/maximum support to enforce it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../src/annotations/protocol.rs | 56 ++++++++++++++++++- 1 file changed, 55 insertions(+), 1 deletion(-) diff --git a/crates/trident-acl-agent/src/annotations/protocol.rs b/crates/trident-acl-agent/src/annotations/protocol.rs index fcf984330..9e8b8ffb0 100644 --- a/crates/trident-acl-agent/src/annotations/protocol.rs +++ b/crates/trident-acl-agent/src/annotations/protocol.rs @@ -832,7 +832,7 @@ mod tests { "description": "Location in Trident's source where the error originated.", "properties": { "path": { "type": "string" }, - "line": { "type": "integer" } + "line": { "type": "integer", "minimum": 0, "maximum": 4294967295 } } } } @@ -1037,6 +1037,22 @@ mod tests { )); } } + if let Some(minimum) = prop_schema.get("minimum").and_then(Value::as_f64) { + let n = value + .as_f64() + .ok_or_else(|| format!("property {name:?}: expected number to check minimum"))?; + if n < minimum { + return Err(format!("property {name:?}: {n} is below minimum {minimum}")); + } + } + if let Some(maximum) = prop_schema.get("maximum").and_then(Value::as_f64) { + let n = value + .as_f64() + .ok_or_else(|| format!("property {name:?}: expected number to check maximum"))?; + if n > maximum { + return Err(format!("property {name:?}: {n} exceeds maximum {maximum}")); + } + } if let Some(max_length) = prop_schema.get("maxLength").and_then(Value::as_u64) { let s = value .as_str() @@ -1373,6 +1389,44 @@ mod tests { .expect("status carrying trident_error must still conform to the formal schema"); } + #[test] + fn design_doc_status_schema_rejects_out_of_range_line() { + // Regression test: ErrorLocation::line and protobuf + // FileLocation::line are u32, so the formal JSON Schema must reject + // negative and > u32::MAX values for `line` - otherwise a + // schema-valid instance could fail to deserialize into + // ErrorLocation. + let schema: Value = serde_json::from_str(DESIGN_DOC_STATUS_SCHEMA).unwrap(); + let base = serde_json::json!({ + "schemaVersion": SCHEMA_VERSION, + "nodeUpdateId": Uuid::new_v4().to_string(), + "operationId": Uuid::new_v4().to_string(), + "operation": "commit", + "code": "TargetBootFailed", + "message": "commit failed", + "startedUtc": fixed_time(0).to_rfc3339(), + "lastUpdatedUtc": fixed_time(0).to_rfc3339(), + "finishedUtc": fixed_time(5).to_rfc3339(), + "tridentError": { + "kind": "SERVICING_ERROR", + "subkind": "ab-update-reboot-check", + "location": { "path": "crates/trident/src/servicing.rs", "line": 42 } + } + }); + + let mut negative = base.clone(); + negative["tridentError"]["location"]["line"] = serde_json::json!(-1); + schema_validate(&schema, &negative).expect_err("schema must reject a negative line number"); + + let mut too_large = base.clone(); + too_large["tridentError"]["location"]["line"] = serde_json::json!(4294967296u64); + schema_validate(&schema, &too_large) + .expect_err("schema must reject a line number beyond u32::MAX"); + + schema_validate(&schema, &base) + .expect("baseline instance with an in-range line must conform to the schema"); + } + #[test] fn with_trident_error_is_noop_for_non_remote_errors() { let request = UpdateRequest { From 0952ce90c3561d7ff1fb81111cb3bea46ec3444f Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Tue, 1 Sep 2026 19:37:22 +0000 Subject: [PATCH 4/5] docs: correct tridentError scope in update-status contract tridentError is not limited to OperationFailed - the post-reboot commit RPC also attaches it to TargetBootFailed when the commit call itself returns a Trident remote error. Also note the unknown/unknown sentinel used when Trident reports failure without a structured error payload. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- docs/Explanation/Trident-ACL-Agent.md | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/docs/Explanation/Trident-ACL-Agent.md b/docs/Explanation/Trident-ACL-Agent.md index 43b5bc63b..3de81e919 100644 --- a/docs/Explanation/Trident-ACL-Agent.md +++ b/docs/Explanation/Trident-ACL-Agent.md @@ -180,10 +180,14 @@ A status annotation (`/update-status` or - `fromVersion`/`toVersion` are the versions the operation moved between (`toVersion` is absent for `rollback`, whose target is implicit). - `tridentError` is present only when the failure originated from a - structured Trident remote error (a subset of `OperationFailed` failures; - most `OperationFailed` statuses - and every other code - carry no - `tridentError`). It has `kind`/`subkind` (Trident's own error - classification, e.g. `SERVICING_ERROR`/`ab-update-reboot-check`) and an + Trident call that returned an error response (a remote error); it can + appear on both `OperationFailed` and `TargetBootFailed` (the reboot + landed but the post-reboot commit call itself failed remotely) - + every other failure mode (timeouts, connection failures, agent-generated + errors like `InvalidRequest`) carries no `tridentError`. It has + `kind`/`subkind` (Trident's own error classification, e.g. + `SERVICING_ERROR`/`ab-update-reboot-check`, or `unknown`/`unknown` when + Trident reported failure without a structured error payload) and an optional `location` (`path`/`line` in Trident's source), letting an orchestrator key off structured fields instead of parsing `message`. - `startedUtc`/`lastUpdatedUtc`/`finishedUtc` bound the operation: From ad1da744acec3b12bd4e826a43fe4455f445fdd3 Mon Sep 17 00:00:00 2001 From: Brian Fjeldstad Date: Tue, 1 Sep 2026 19:40:28 +0000 Subject: [PATCH 5/5] acl-agent: reject fractional values for schema integer properties The test schema validators integer type check accepted any JSON number, so a fractional line value (e.g. 1.5) passed the new minimum/maximum bounds despite ErrorLocation::line being u32 and unable to deserialize it. Distinguish integer from number by requiring an exact i64/u64 representation. Covers a fractional line in the existing regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- crates/trident-acl-agent/src/annotations/protocol.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/crates/trident-acl-agent/src/annotations/protocol.rs b/crates/trident-acl-agent/src/annotations/protocol.rs index 9e8b8ffb0..60e5809a0 100644 --- a/crates/trident-acl-agent/src/annotations/protocol.rs +++ b/crates/trident-acl-agent/src/annotations/protocol.rs @@ -970,7 +970,8 @@ mod tests { "object" => value.is_object(), "array" => value.is_array(), "boolean" => value.is_boolean(), - "number" | "integer" => value.is_number(), + "number" => value.is_number(), + "integer" => value.is_i64() || value.is_u64(), other => panic!( "test schema validator does not support type {other:?} - extend schema_validate_property" ), @@ -1423,6 +1424,11 @@ mod tests { schema_validate(&schema, &too_large) .expect_err("schema must reject a line number beyond u32::MAX"); + let mut fractional = base.clone(); + fractional["tridentError"]["location"]["line"] = serde_json::json!(1.5); + schema_validate(&schema, &fractional) + .expect_err("schema must reject a fractional line number"); + schema_validate(&schema, &base) .expect("baseline instance with an in-range line must conform to the schema"); }