Skip to content

telemetry: remove asset_id, rename correlation_id to installation_id, add persistent servicing_id - #779

Draft
bfjelds (bfjelds) wants to merge 19 commits into
user/bfjelds/mjolnir/command-error-metricfrom
user/bfjelds/mjolnir/installation-servicing-id
Draft

telemetry: remove asset_id, rename correlation_id to installation_id, add persistent servicing_id#779
bfjelds (bfjelds) wants to merge 19 commits into
user/bfjelds/mjolnir/command-error-metricfrom
user/bfjelds/mjolnir/installation-servicing-id

Conversation

@bfjelds

@bfjelds bfjelds (bfjelds) commented Sep 4, 2026

Copy link
Copy Markdown
Member
  • Remove asset_id from telemetry (the DMI product UUID)
  • Rename correlation_id -> installation_id; and change when trident install starts.
  • Add new persistent servicing_id (like installation_id, stored in the datastore); and change when a servicing operation (install, update, or manual rollback) begins staging.

Related PRs in stack:

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@bfjelds bfjelds (bfjelds) changed the title telemetry: remove asset_id, rename correlation_id to installation_id (install-time creation), add persistent servicing_id telemetry: remove asset_id, rename correlation_id to installation_id, add persistent servicing_id Sep 4, 2026
--filepath "$METRICS_FILE" \
--mapping "$KUSTO_TABLE_MAPPING"
--mapping "$KUSTO_TABLE_MAPPING" \
--ignore-schema

@bfjelds bfjelds (bfjelds) Sep 5, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

adding ignore-schema because we removed asset_id which is a required field in the telemetry schema.

other options would be:

  • remove asset_id from required list in schema
  • use asset_id as installation_id

@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/installation-servicing-id branch 2 times, most recently from 1599407 to c96c0af Compare September 5, 2026 01:36
@bfjelds
bfjelds (bfjelds) requested a balanced review from Copilot September 5, 2026 01:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Install finalization overwrites the staged servicing ID, breaking cross-invocation correlation.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Updates telemetry identifiers to improve privacy and correlate servicing operations across invocations.

Changes:

  • Removes hardware-derived asset_id and renames correlation_id to installation_id.
  • Persists and emits per-servicing-operation servicing_id values.
  • Adjusts telemetry documentation and Kusto ingestion.
File summaries
File Description
docs/Reference/Agent-Configuration.md Documents telemetry identifiers.
crates/trident/src/main.rs Passes the installation-ID handle to App Insights.
crates/trident/src/logging/tracestream.rs Emits installation and servicing IDs.
crates/trident/src/logging/operation_context.rs Tracks servicing IDs per invocation.
crates/trident/src/logging/appinsights.rs Adds identifiers to App Insights events.
crates/trident/src/lib.rs Creates and restores identifiers during servicing.
crates/trident/src/engine/update.rs Creates servicing IDs for updates.
crates/trident/src/engine/manual_rollback/mod.rs Handles rollback servicing IDs.
crates/trident/src/datastore.rs Persists installation and servicing IDs.
.pipelines/templates/stages/testing_common/scripts/upload_metrics.sh Uploads metrics without schema validation.
Review details
  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread crates/trident/src/lib.rs Outdated
@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/command-error-metric branch from a97f856 to a2e921d Compare September 5, 2026 16:21
@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/installation-servicing-id branch from c96c0af to bcab5b0 Compare September 5, 2026 16:31
@bfjelds
bfjelds (bfjelds) requested a balanced review from Copilot September 5, 2026 16:32

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Servicing IDs are assigned before rollback eligibility is known and are not restored for post-reboot commit metrics.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 11/11 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread crates/trident/src/engine/manual_rollback/mod.rs Outdated
Comment thread crates/trident/src/lib.rs
@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/installation-servicing-id branch from bcab5b0 to 941e873 Compare September 5, 2026 17:02
@bfjelds
bfjelds (bfjelds) requested a balanced review from Copilot September 5, 2026 19:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The core servicing-ID serialization path lacks sink-level regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 11/11 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread crates/trident/src/logging/tracestream.rs
@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/command-error-metric branch 2 times, most recently from f6f0da6 to 5e7b215 Compare September 5, 2026 19:51
@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/installation-servicing-id branch from 941e873 to 6f8944b Compare September 5, 2026 19:58
@bfjelds
bfjelds (bfjelds) requested a balanced review from Copilot September 5, 2026 20:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The updated diff addresses the prior lifecycle and serialization gaps, with no remaining blocking correctness issues found.

Review details
  • Files reviewed: 11/11 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/command-error-metric branch from 5e7b215 to e6ac4c4 Compare September 5, 2026 20:36
@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/installation-servicing-id branch from 6f8944b to 7256f43 Compare September 5, 2026 20:41
@bfjelds
bfjelds (bfjelds) requested a balanced review from Copilot September 5, 2026 20:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Some daemon read requests and the initial CIH bootstrap update still emit telemetry without the intended installation ID.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 12/12 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread crates/trident/src/lib.rs
Comment thread crates/trident/src/server/tridentserver/mod.rs
@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/command-error-metric branch from fda62f2 to caaec51 Compare September 5, 2026 22:27
@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/installation-servicing-id branch from 3c922ef to 58467cb Compare September 5, 2026 22:36
@bfjelds
bfjelds (bfjelds) requested a balanced review from Copilot September 5, 2026 22:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

CIH bootstrap telemetry does not attach its newly persisted installation ID to the active trace stream.

Review details

Suppressed comments (1)

crates/trident/src/lib.rs:763

  • Issue: The CIH bootstrap persists an installation ID but never attaches it to the shared TraceStream, so this invocation's update_start and later events still omit installation_id. Evidence: Trident::new saw NotProvisioned and left the handle empty; this branch only calls create_installation_id(), while telemetry reads the separate in-memory handle. Suggestion: clone self.tracestream before entering execute_and_record_error, then call set_installation_id with the successfully created ID here (as install does).
                    if let Err(e) = datastore.create_installation_id() {
                        warn!("Failed to create installation ID during CIH bootstrap: {e:?}");
                    }
  • Files reviewed: 12/12 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Drops the DMI product UUID (/sys/class/dmi/id/product_uuid) from every
telemetry event. It was the one genuinely hardware-derived identifier in
PLATFORM_INFO; installation_id (next commit) already gives a stable,
random, non-hardware-derived per-host identifier for correlation, making
asset_id redundant from a correlation standpoint while being needlessly
more identifying.

Removes read_product_uuid/PRODUCT_UUID_FILE and their tests entirely.

(cherry picked from commit 65a83bd)
(cherry picked from commit 309be9a)
(cherry picked from commit 1f20777)
(cherry picked from commit c536f89)
(cherry picked from commit 5f25ac7)
(cherry picked from commit 41b6e8f)
(cherry picked from commit e9161ad)
…ll start, not datastore-creation); add persistent servicing_id

installation_id (renamed from correlation_id):
- DataStore::correlation_id() (get-or-create, called from Trident::new
  on every invocation) split into two: DataStore::installation_id()
  (read-only) and DataStore::create_installation_id() (get-or-create).
- Trident::new() now only reads (Trident::installation_id()); it never
  creates one. Trident::install() is the only caller of
  create_installation_id(), at the start of staging (after any multiboot
  datastore swap, so a multiboot install's own new datastore gets its own
  ID). Every other command/gRPC request now simply attaches whatever was
  already stamped at install time, instead of the previous behavior where
  literally the first-ever Trident::new() call on a fresh datastore
  (regardless of which command triggered it) would mint the ID.
- Added a `tracestream: TraceStream` field to the `Trident` struct so
  `install()` can attach the newly-created ID to telemetry (previously
  `Trident::new()` only used its `tracestream` parameter transiently).
- TraceStream::set_correlation_id/correlation_id_handle ->
  set_installation_id/installation_id_handle; AppInsightsSender's
  correlation_id field/param renamed to match. JSON field renamed
  correlation_id -> installation_id.
- New test: installation_id's read-only getter returns None before
  create_installation_id is called, Some after.

New persistent servicing_id:
- DataStore::new_servicing_id() (unconditional overwrite, unlike
  installation_id's insert-if-absent) + DataStore::servicing_id()
  (read-only) using the existing set_value/get_value generic key-value
  API (set_value's overwrite semantics were previously exercised only by
  tests; this is its first real caller).
- Attached via operation_context (not threaded through every engine::*
  function signature): CURRENT_OPERATION's thread-local state extended
  from (operation_id, command) to also carry an optional servicing_id,
  set via the new operation_context::set_servicing_id(). Both
  TraceSender's and AppInsightsSender's field-merging logic updated to
  include it when present.
- Created at the start of staging in all three places that stage:
  Trident::install() (alongside installation_id), engine::update::update()
  (at the same unconditional "start" point update_start already fires
  from), and engine::manual_rollback::execute_rollback()'s has_stage()
  branch (naturally gated to genuine staging, unlike install/update).
  finalize_rollback() reads the persisted ID back (via
  DataStore::servicing_id()) at its own start, so a resumed finalize-only
  invocation (e.g. after reboot) still tags its own metrics with the same
  servicing_id the staging invocation used.
- Known simplification: install/update mint a fresh servicing_id on every
  invocation that reaches their "start" point, including a possible
  finalize-only re-invocation of update() (which, per its existing
  design, unconditionally re-runs staging logic on every call regardless
  of whether it's a true first stage or a resumed one) -- so unlike
  manual rollback, a genuinely separate stage-then-finalize update
  sequence would not currently share one servicing_id across both calls.
  Documented in code comments; not fixed here to avoid a much larger
  change to update()'s existing stage/finalize semantics.

docs/Reference/Agent-Configuration.md updated: asset_id entry removed
(handled in the prior commit), correlation_id -> installation_id with its
new creation semantics, and a new servicing_id entry.

Verified: cargo build --workspace, clippy --all-targets --features
functional-test, fmt --check all clean. cargo test -p trident --lib:
422/422 pass (new tests: installation_id read-only-getter test in
datastore.rs; set_servicing_id tests in operation_context.rs).

(cherry picked from commit efd1405)
(cherry picked from commit ae5396d)
(cherry picked from commit f11fa98)
(cherry picked from commit 52fcc53)
(cherry picked from commit b1e10a6)
(cherry picked from commit e79950b)
(cherry picked from commit d777978)
…ation

Trident::update() has a case my earlier servicing_id placement in
engine::update::update() completely missed: when the Host Configuration
for this invocation is unchanged from what is already persisted
(datastore.host_status().spec == host_config) and the servicing state is
AbUpdateStaged/RuntimeUpdateStaged, it calls ab_update::finalize_update()/
runtime_update::finalize_update() *directly* -- bypassing
engine::update::update() (and therefore my new_servicing_id() call)
entirely. Every other update path does go through engine::update::update(),
which is correctly gated by Trident::update() to only run when staging is
actually happening (a changed config with has_stage(), or a retry of a
failed/no-op update), so this was the one path a fresh servicing_id was
never being created *or* read back for -- meaning a genuine, separate
finalize-only continuation of a staged update (the real two-step A/B
update flow, rebooting in between) would have no servicing_id at all.

Fix: read the persisted servicing_id back (DataStore::servicing_id()) at
the top of both direct-finalize match arms in Trident::update(), mirroring
what execute_rollback's finalize_rollback already does. This is safe
specifically because reaching those arms already required
datastore.host_status().spec == host_config -- i.e. this invocation's
config is provably identical to whatever was staged, so the persisted
servicing_id is guaranteed to belong to *this* operation, not an unrelated
one.

Also corrected the inline comment on the new_servicing_id() call in
engine::update::update() itself, which had incorrectly implied that
function handles finalize-only re-invocations too.

cargo build -p trident, clippy --all-targets --features functional-test,
fmt --check all clean. cargo test -p trident --lib: 422/422 pass
(unchanged -- no test exercises the true separate-process two-step update
flow yet; this is a targeted correctness fix for a path today's test
suite doesn't cover end-to-end).

(cherry picked from commit 671bb3e)
(cherry picked from commit bd4766b)
(cherry picked from commit 10d3140)
(cherry picked from commit ddf2577)
(cherry picked from commit 5ba46af)
(cherry picked from commit 1e52d68)
(cherry picked from commit ba36459)
set_servicing_id() only affects events fired *after* it runs on this
thread (operation_context is read at send-time, not retroactively) --
with new_servicing_id()/set_servicing_id() coming after update_start,
that metric itself never carried the very servicing_id it establishes.
Swapped the order so update_start (and everything else this invocation
emits) is tagged with it.

cargo build, clippy --all-targets --features functional-test, fmt --check
all clean. cargo test -p trident --lib: 422/422 pass.

Note: the same ordering issue exists in
engine::manual_rollback::execute_rollback (manual_rollback_start fires
before new_servicing_id, unconditionally at the top of the function,
before has_stage()/rollback-availability are even known) -- left as a
follow-up since fixing it cleanly needs more thought: the metric fires
before we know whether a fresh ID will be minted at all, so simply
reading back any already-persisted ID first risks tagging the event with
a stale ID from a prior, unrelated, already-completed operation.

(cherry picked from commit da288b0)
(cherry picked from commit 01739c3)
(cherry picked from commit 086183f)
(cherry picked from commit 58d1955)
(cherry picked from commit 6acf33a)
(cherry picked from commit 4ff8160)
(cherry picked from commit e8f99a6)
…d (same ordering bug as update_start)

manual_rollback_start fired unconditionally at the very top of
execute_rollback, before has_stage()/rollback-availability were even
known -- and the servicing_id mint (previously placed after rollback_type
was determined, deep inside the has_stage() branch) happened well after
that. Same root cause as the update_start fix: set_servicing_id only
affects events emitted after it runs.

Harder to fix than update_start though, since the metric fires before we
know whether this call will actually stage anything. Restructured so the
right servicing_id is determined first, then the metric fires:

- has_stage() requested: mint a fresh servicing_id immediately (moved up
  from after rollback_type is determined). This now also covers the rare
  "staging requested but nothing available to roll back" early-return
  case, which previously never reached the old mint call at all -- a
  fresh "this attempt" ID for that case is reasonable, and a later
  genuine stage overwrites it again.
- finalize-only (no has_stage()): read back whatever servicing_id is
  already persisted, mirroring finalize_rollback's existing read-back (a
  harmless redundant read if this same call both stages and finalizes).

Removed the now-redundant later new_servicing_id() call inside the
has_stage() branch.

cargo build --workspace, clippy --all-targets --features functional-test,
fmt --check all clean. cargo test -p trident --lib: 422/422 pass.

(cherry picked from commit 96d371a)
(cherry picked from commit 93dd46e)
(cherry picked from commit aac18bd)
(cherry picked from commit fc8619f)
(cherry picked from commit 2c7d8c6)
(cherry picked from commit c791100)
(cherry picked from commit 93b79e8)
Removing asset_id from the metrics/telemetry payload (earlier commit in
this PR) breaks kusto_ingestor.py's schema check against the existing
Kusto table mapping, which still expects that column. --ignore-schema
skips that check so metrics upload keeps working without needing a
coordinated Kusto table-mapping update in lockstep with this PR.

(cherry picked from commit 277f9ae)
(cherry picked from commit 6efffe0)
(cherry picked from commit e7ccbf0)
(cherry picked from commit ea153a5)
(cherry picked from commit 90b2573)
(cherry picked from commit 399be97)
(cherry picked from commit 78e15b3)
Artifact of rebasing this branch onto PR 774's daemon pre-warm fix
(added after the installation_id rename commit was originally authored,
so that rename never touched this new code). Caught by the immediate
build failure, not a real code change.

(cherry picked from commit 1656ea5)
(cherry picked from commit 951ab89)
(cherry picked from commit 74e79cf)
(cherry picked from commit 31298db)
(cherry picked from commit 6d21cae)
(cherry picked from commit 498b9f9)
(cherry picked from commit 66c6fbd)
(cherry picked from commit 9fca872)
(cherry picked from commit bf9ffa2)
(cherry picked from commit 256aec6)
(cherry picked from commit da00dba)
(cherry picked from commit f69049e)
(cherry picked from commit 5f30f06)
(cherry picked from commit 3c5477e)
…tall_finalize

Addresses Copilot review comment on PR 779: install_stage and
install_finalize both call Trident::install(), and it unconditionally
called new_servicing_id() before determining which of its three cases
applied (genuinely new install, resuming a staged install, or retrying a
failed/no-op one) -- so a separate install_finalize invocation resuming a
CleanInstallStaged host would clobber the servicing_id install_stage had
already minted and persisted, making the two halves uncorrelatable.
Exactly the same class of bug as the update_finalize one fixed earlier
in this PR.

Moved servicing_id creation into only the two branches that genuinely
start staging a new install (config changed + has_stage(), and
NotProvisioned retry); the CleanInstallStaged (finalize-only) branch now
reads the persisted servicing_id back via DataStore::servicing_id()
instead, mirroring the update_finalize fix and manual rollback's
finalize path. Safe for the same reason: reaching the CleanInstallStaged
arm already requires datastore.host_status().spec == host_config, so the
persisted ID is guaranteed to belong to *this* staged install, not an
unrelated one.

installation_id creation (get-or-create, idempotent) is unaffected --
still happens unconditionally before branching, since calling it
repeatedly for the same datastore is harmless.

cargo build --workspace, clippy --all-targets --features functional-test,
fmt --check all clean. cargo test -p trident --lib: 428/428 pass
(unchanged -- no existing test exercises the true separate-process
two-step install flow, same caveat as the update_finalize fix).

(cherry picked from commit 7a6a6c0)
(cherry picked from commit aca1f7a)
(cherry picked from commit ce1e1d8)
(cherry picked from commit f8c8cc9)
(cherry picked from commit c8b0964)
(cherry picked from commit 50bf0b3)
(cherry picked from commit a9df91b)
Artifact of rebasing this branch onto PR 778's new CLI pre-warm fix
(added after the installation_id rename commit was originally authored,
so that rename never touched this new code). Caught by the immediate
build failure, not a real code change.

(cherry picked from commit 66595cb)
(cherry picked from commit 011e94d)
(cherry picked from commit 2699642)
(cherry picked from commit 7bede3d)
(cherry picked from commit 98408ed)
(cherry picked from commit 5a8e103)
(cherry picked from commit 56452c8)
…*_success metrics

Addresses Copilot review comment on PR 779: A/B update, clean install,
and manual A/B rollback all finalize with ExitKind::NeedsReboot, then
Trident::commit() (in a separate, post-reboot process invocation) calls
rollback::validate_boot, which fires the corresponding *_success metric
(clean_install_success/ab_update_success/manual_rollback_success) --
but commit() never called servicing_id/set_servicing_id, so those
completion events could never be correlated back to the servicing
operation that produced them (unlike the finalize-only continuation
paths for update/install/manual-rollback, which do this).

Reads the persisted servicing ID back right after commit()'s early
validity check (which already confirms servicing_state is one of the
*Finalized/*HealthCheckFailed states), before calling validate_boot.

cargo build --workspace, clippy --all-targets --features functional-test,
fmt --check all clean. cargo test -p trident --lib: 428/428 pass.

(cherry picked from commit 98c7c08)
(cherry picked from commit 16489a1)
(cherry picked from commit da6e1ea)
(cherry picked from commit 625dbc4)
(cherry picked from commit 7bf6e0a)
(cherry picked from commit 2c1c9ab)
(cherry picked from commit 8ab67f9)
…ng a real rollback operation

Addresses Copilot review comment on PR 779: my earlier fix for
manual_rollback_start's missing servicing_id (moving ID determination to
the top of execute_rollback, before the metric fires) went too far the
other way -- it selected/persisted an ID before confirming a rollback
could actually proceed. A stage request rejected for an invalid state,
last_error set, or "no rollback available" still minted a fresh ID (and
overwrote any previous one) even though nothing was staged. A
finalize-only request from Provisioned (nothing staged) attached the
*previous* operation's ID before being rejected, misattributing that
telemetry.

Restructured so validation comes first in both branches:
- Stage path: mint the servicing ID and fire manual_rollback_start only
  after state/last_error validation *and* resolving a real rollback
  target (right where rollback_type becomes known) -- not before.
- Finalize-only path: restore the persisted ID and fire
  manual_rollback_start only after confirming servicing_state actually
  matches a real ManualRollback*Staged state -- not before. Skipped
  entirely when this same call also staged above, since that branch
  already handled it for the combined operation.

cargo build --workspace, clippy --all-targets --features functional-test,
fmt --check all clean. cargo test -p trident --lib: 428/428 pass.

(cherry picked from commit 941e873)
(cherry picked from commit 6d14023)
(cherry picked from commit 01676a6)
(cherry picked from commit 7a2733a)
(cherry picked from commit 8675ffe)
(cherry picked from commit a66aa12)
(cherry picked from commit 1f262ba)
…ields

Mirrors the existing installation_id regression test: wraps a metric
event in run_with_operation + set_servicing_id and asserts the
serialized metrics line carries additional_fields.servicing_id, so a
future refactor of merge_operation_context cannot silently drop it
without a test failure.

(cherry picked from commit 6f8944b)
(cherry picked from commit 8b35a11)
(cherry picked from commit 6392a2c)
(cherry picked from commit 17d595b)
(cherry picked from commit a756fee)
(cherry picked from commit ce1a966)
…ation_id

(cherry picked from commit 7256f43)
(cherry picked from commit 8ffe621)
(cherry picked from commit 3bb1f69)
(cherry picked from commit e55a872)
(cherry picked from commit 081342f)
…nstall events

The telemetry field reference implied every event carries installation_id,
but the first trident install run's own command_start, any preflight
failure, and trident_start all fire before Trident::install reaches the
point of creating the ID -- so those specific events are sent without it.

(cherry picked from commit a6b2280)
(cherry picked from commit c64716d)
(cherry picked from commit 9154e85)
(cherry picked from commit ca5d718)
A read-only installation_id lookup left already-provisioned hosts whose
datastore predates this key permanently without one: create_installation_id
is only ever called by Trident::install, but offline initialization and
the CIH update bootstrap both create/adopt a datastore directly, without
going through install. Mint one as a one-time migration when the
datastore is provisioned (servicing_state != NotProvisioned) but has no
installation ID yet, while leaving a genuinely unprovisioned datastore
(temporary, or not yet installed) read-only as before.

(cherry picked from commit fc6a548)
(cherry picked from commit 7a4a493)
(cherry picked from commit 63ec787)
…onfirmed

command_start fires before any operation context exists, and
set_servicing_id only runs after staging or finalize validation confirms
a real servicing operation -- so command_start, preflight failures, and
no-op commands are all sent without this field.

(cherry picked from commit a8b1e7f)
(cherry picked from commit 3c922ef)
(cherry picked from commit 3e8ed47)
Trident::new's migration check ran while a CIH-bootstrapped datastore
was still NotProvisioned (this promotion, inside Trident::update, only
happens later) so it never fired for this invocation -- the entire
bootstrap update (trident_start, update_start, and everything after)
ran with no installation ID, which was only minted whenever some later,
separate invocation happened to run. Mint it now, synchronously, right
after this same promotion, so update_start (fired later in this same
invocation) and everything after it carries it. Known remaining gap:
this invocation's own command_start/trident_start (fired earlier, in
the CLI/daemon dispatch and Trident::new respectively) still can't --
Trident holds no TraceStream handle at this point to attach it to
those.

(cherry picked from commit 58467cb)
The CIH bootstrap persisted a new installation ID via
create_installation_id, but never attached it to the shared TraceStream,
so this invocation's update_start and later events still omitted
installation_id: Trident::new's own attach-if-present check already ran
and found nothing, since this datastore was still NotProvisioned at that
point. Clone self.tracestream before entering execute_and_record_error
(same pattern install already uses for its own newly-created IDs) and
call set_installation_id here. The one remaining, unavoidable gap:
command_start/trident_start, fired even earlier in the CLI/daemon
dispatch and Trident::new respectively, still can't carry it.
@bfjelds
bfjelds (bfjelds) force-pushed the user/bfjelds/mjolnir/installation-servicing-id branch from 58467cb to ddd7fda Compare September 5, 2026 23:00
@bfjelds
bfjelds (bfjelds) requested a balanced review from Copilot September 5, 2026 23:00
@bfjelds

Copy link
Copy Markdown
Member Author

Addressed the suppressed review finding: the CIH bootstrap persisted a new installation ID via create_installation_id but never attached it to the shared TraceStream, so this invocation's update_start and later events still omitted installation_id -- Trident::new's own attach-if-present check already ran and found nothing, since this datastore was still NotProvisioned at that point. Cloned self.tracestream before entering execute_and_record_error (the same pattern install already uses for its own newly-created IDs) and called set_installation_id right after the mint succeeds. One gap remains, and is unavoidable without a larger signature change: this invocation's own command_start/trident_start, fired even earlier in the CLI/daemon dispatch and Trident::new respectively, still can't carry it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

Clean-install servicing IDs are persisted before preflight checks confirm that staging will begin.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details
  • Files reviewed: 12/12 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread crates/trident/src/lib.rs
Comment on lines +618 to +622
let servicing_id = datastore
.new_servicing_id()
.message("Failed to create servicing ID")?;
info!("Servicing ID: {servicing_id}");
set_servicing_id(servicing_id.to_string());
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.

2 participants