fix: prevent builtin service config loss on dependency graph damage - #1824
Merged
aws-kevinrickard merged 7 commits intoJul 20, 2026
Merged
Conversation
|
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 7a2f726 |
|
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 7a2f726 |
aws-kevinrickard
force-pushed
the
v228-dependency-default-flag
branch
from
July 17, 2026 00:53
240215f to
3a647ea
Compare
Builtin autostart services are injected into main's dependency map at launch as default dependencies. When a deployment writes the services/main/dependencies topic, setupDependencies re-added every listed service with isDefault=false, silently erasing that protection. Any later topic update lacking the builtins then evicted them from the dependency graph while their services kept running. This happens in practice when a device's first-ever deployment fails after activation and rolls back: the rollback snapshot's dependencies topic value predates the deployment and lists only the nucleus, so replaying it evicts every builtin service. Because deployment merge protection for builtin services is keyed on kernel.orderedDependencies(), the next deployment then deletes the builtins' entire config subtrees (including DeploymentService group membership state and the FleetStatusService sequence number) while the services keep writing to the detached config nodes. Subsequent tlog truncations omit the detached subtrees, so the loss surfaces only at the next restart, where the device loses its thing group state and deployments tear down installed components. Preserve the existing isDefault flag when a dependency is re-added from the topic so default dependencies can never be evicted by a topic update. The flag was not read anywhere except the eviction filter, and launch() re-injects builtins as default at every boot, so no behavior other than the erroneous eviction is affected.
Deployment merges protect builtin services' config with per-service MERGE overrides keyed on kernel.orderedDependencies(). The live dependency graph is not a safe key: a builtin service can be erroneously missing from it while still running, in which case the services-level REPLACE wildcard silently deletes the builtin's entire config subtree (deployment group state, fleet status sequence number, telemetry state) out from under the running service. Derive the protected name set from static builtin identity (KernelLifecycle autostart builtin annotations) and union it with the graph-derived set. Autostart plugin builtins, whose names are not statically known, remain covered by the graph-derived set. Protecting a builtin that is not currently in the graph is harmless since a MERGE override only retains existing config. Also add the same builtin protection to the rollback merge behavior, which previously had none: replaying a snapshot that lacks a builtin's config (e.g. one dumped while the builtin's subtree was detached) discarded the builtin's entire config. Values present in the snapshot are restored exactly as before; the only behavior change is that nodes newly created under a builtin during the failed deployment are retained rather than discarded. createRollbackMergeBehavior is widened from private to protected for testability, matching createDeploymentMergeBehavior.
KernelConfigResolver.getMainConfig carried builtin services into main's resolved dependencies only when they were present in the live dependency graph (kernel.getMain().getDependencies()). If a builtin was erroneously missing from the graph while its service was still running, every subsequent deployment resolution omitted it, so the damage was self-sustaining: the dependencies topic never listed the builtins again until the next restart re-injected them at launch. Union the static autostart builtin names (as name:HARD) with the graph-derived carry-forward. On a healthy device the graph already contains every autostart builtin, so the resolved output is unchanged. On a damaged device, merging the resolved config now repairs the dependency graph — the dependencies topic subscriber re-adds the missing builtins — instead of perpetuating the damage until reboot. Builtins present in the graph keep their actual dependency type; the static union only applies to names absent from the graph. Autostart builtins are compiled into the nucleus, so a statically added name always resolves.
When a deployment resolution repairs a damaged dependency graph by listing a previously evicted autostart builtin in main's dependencies topic, the subscriber re-added it as a non-default dependency, since no prior dependency info existed to preserve the flag from. The healed state was therefore weaker than a freshly launched one: a later topic update omitting the builtins (for example another rollback) could evict them again. Treat the statically known autostart builtin names as default dependencies when main re-adds them from the topic. This is exactly the set launch() injects as default at every boot, so healing now restores launch-equivalent protection. Other services declaring the same names keep the existing removable-dependency behavior.
Deduplicate the static-plus-graph builtin name union used by both the deployment and rollback merge behaviors into a single helper so the two protection sites cannot drift. Also guard the static name derivation against a builtin class missing its annotation, and fix test import ordering.
Document in getBuiltinServiceNamesToProtect the residual asymmetry for annotation-discovered plugin autostart builtins (protected only while present in the dependency graph and not healed by the static carry-forward). Assert the preserved default flag directly in the flag-preservation test, pinning that contract in isolation from the static-name default restoration for main. Use getServiceName consistently in the new code paths.
Add two baseline tests for the eviction-filter behavior the builtin protection layers rest on: a default dependency which was never listed in the dependencies topic survives a topic write omitting it (with its default flag untouched), and re-adding an evicted dependency with isDefault=true restores its protection against subsequent omissions — the mechanism by which launch() repairs the dependency graph at boot. Adapted from the characterization test suite in aws-greengrass#1823, which this change set supersedes. Co-authored-by: saranyailla <nukai@amazon.com>
aws-kevinrickard
force-pushed
the
v228-dependency-default-flag
branch
from
July 17, 2026 18:17
3a647ea to
7a2f726
Compare
saranyailla
approved these changes
Jul 17, 2026
junfuchen99
approved these changes
Jul 17, 2026
This was referenced Jul 20, 2026
Member
Author
|
Nit (non-blocking): Long term: |
yitingb
pushed a commit
that referenced
this pull request
Aug 3, 2026
…1824) * fix: preserve default flag when re-adding dependencies from topic Builtin autostart services are injected into main's dependency map at launch as default dependencies. When a deployment writes the services/main/dependencies topic, setupDependencies re-added every listed service with isDefault=false, silently erasing that protection. Any later topic update lacking the builtins then evicted them from the dependency graph while their services kept running. This happens in practice when a device's first-ever deployment fails after activation and rolls back: the rollback snapshot's dependencies topic value predates the deployment and lists only the nucleus, so replaying it evicts every builtin service. Because deployment merge protection for builtin services is keyed on kernel.orderedDependencies(), the next deployment then deletes the builtins' entire config subtrees (including DeploymentService group membership state and the FleetStatusService sequence number) while the services keep writing to the detached config nodes. Subsequent tlog truncations omit the detached subtrees, so the loss surfaces only at the next restart, where the device loses its thing group state and deployments tear down installed components. Preserve the existing isDefault flag when a dependency is re-added from the topic so default dependencies can never be evicted by a topic update. The flag was not read anywhere except the eviction filter, and launch() re-injects builtins as default at every boot, so no behavior other than the erroneous eviction is affected. * fix: key builtin merge protection on static builtin names Deployment merges protect builtin services' config with per-service MERGE overrides keyed on kernel.orderedDependencies(). The live dependency graph is not a safe key: a builtin service can be erroneously missing from it while still running, in which case the services-level REPLACE wildcard silently deletes the builtin's entire config subtree (deployment group state, fleet status sequence number, telemetry state) out from under the running service. Derive the protected name set from static builtin identity (KernelLifecycle autostart builtin annotations) and union it with the graph-derived set. Autostart plugin builtins, whose names are not statically known, remain covered by the graph-derived set. Protecting a builtin that is not currently in the graph is harmless since a MERGE override only retains existing config. Also add the same builtin protection to the rollback merge behavior, which previously had none: replaying a snapshot that lacks a builtin's config (e.g. one dumped while the builtin's subtree was detached) discarded the builtin's entire config. Values present in the snapshot are restored exactly as before; the only behavior change is that nodes newly created under a builtin during the failed deployment are retained rather than discarded. createRollbackMergeBehavior is widened from private to protected for testability, matching createDeploymentMergeBehavior. * fix: carry builtin services forward in main config by static identity KernelConfigResolver.getMainConfig carried builtin services into main's resolved dependencies only when they were present in the live dependency graph (kernel.getMain().getDependencies()). If a builtin was erroneously missing from the graph while its service was still running, every subsequent deployment resolution omitted it, so the damage was self-sustaining: the dependencies topic never listed the builtins again until the next restart re-injected them at launch. Union the static autostart builtin names (as name:HARD) with the graph-derived carry-forward. On a healthy device the graph already contains every autostart builtin, so the resolved output is unchanged. On a damaged device, merging the resolved config now repairs the dependency graph — the dependencies topic subscriber re-adds the missing builtins — instead of perpetuating the damage until reboot. Builtins present in the graph keep their actual dependency type; the static union only applies to names absent from the graph. Autostart builtins are compiled into the nucleus, so a statically added name always resolves. * fix: restore default flag for autostart builtins re-added to main When a deployment resolution repairs a damaged dependency graph by listing a previously evicted autostart builtin in main's dependencies topic, the subscriber re-added it as a non-default dependency, since no prior dependency info existed to preserve the flag from. The healed state was therefore weaker than a freshly launched one: a later topic update omitting the builtins (for example another rollback) could evict them again. Treat the statically known autostart builtin names as default dependencies when main re-adds them from the topic. This is exactly the set launch() injects as default at every boot, so healing now restores launch-equivalent protection. Other services declaring the same names keep the existing removable-dependency behavior. * refactor: extract shared builtin protection name set helper Deduplicate the static-plus-graph builtin name union used by both the deployment and rollback merge behaviors into a single helper so the two protection sites cannot drift. Also guard the static name derivation against a builtin class missing its annotation, and fix test import ordering. * refactor: address review feedback on builtin protection Document in getBuiltinServiceNamesToProtect the residual asymmetry for annotation-discovered plugin autostart builtins (protected only while present in the dependency graph and not healed by the static carry-forward). Assert the preserved default flag directly in the flag-preservation test, pinning that contract in isolation from the static-name default restoration for main. Use getServiceName consistently in the new code paths. * test: pin default dependency eviction filter invariants Add two baseline tests for the eviction-filter behavior the builtin protection layers rest on: a default dependency which was never listed in the dependencies topic survives a topic write omitting it (with its default flag untouched), and re-adding an evicted dependency with isDefault=true restores its protection against subsequent omissions — the mechanism by which launch() repairs the dependency graph at boot. Adapted from the characterization test suite in #1823, which this change set supersedes. Co-authored-by: saranyailla <nukai@amazon.com> --------- Co-authored-by: saranyailla <nukai@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #, if available: n/a
Description of changes:
Defense-in-depth fix for a silent config-loss failure chain, as five small commits:
GreengrassService.setupDependencies): preserve the existingisDefaultflag when re-adding a dependency listed in thedependenciestopic, instead of downgrading it tofalse. Default (builtin) dependencies can no longer be evicted by a topic update.DeploymentActivator): key builtin merge protection on static builtin identity (newKernelLifecycle.AUTOSTART_BUILTIN_SERVICE_NAMES) unioned with the live dependency graph, and add the same protection to the rollback merge behavior, which previously had none. Even with a damaged dependency graph, deployments and rollbacks can no longer delete builtin services' config.KernelConfigResolver.getMainConfig): union the static autostart builtin names (name:HARD) into main's resolved dependencies, so a deployment on an already-damaged device repairs the dependency graph instead of perpetuating the damage until reboot.GreengrassService.setupDependencies): when main re-adds a statically known autostart builtin that is not currently a default dependency, restore launch-equivalent default protection (isDefault=true). Applies only to main — other services declaring builtin names keep normal removable-dependency behavior.Why is this change necessary:
The autostart builtin services (DeploymentService, FleetStatusService, TelemetryAgent, UpdateSystemPolicyService — the builtins with
autostart=true) are injected into main's in-memory dependency map at every launch as default dependencies, protected from eviction by topic updates. That protection was fragile, and everything downstream keyed off the same mutable runtime state:services/main/dependenciestopic contains only the nucleus; the builtins exist only in the in-memory map.isDefault=false— silently erasing the launch protection.kernel.orderedDependencies(), so the next deployment deleted the evicted builtins' entire config subtrees (thing-group membership state, fleet-status sequence number). The running services kept writing to the detached config nodes, so nothing looked wrong.Observed in the field as: fleet-status sequence number resetting to 0 and
fleetConfigArnsbecoming empty after a routine restart, followed by a startup-time local deployment removing the customer's components.TokenExchangeService and DockerApplicationManager are builtins but not autostart: they are not launch-injected and not in the static set; they remain covered by the graph-derived protection as before.
How was this change tested:
Each behavioral commit adds a regression test verified in both polarities (fails without the fix, passes with it):
SetupDependencyTest: a default dependency survives a topic update omitting it (real config + subscriber path); a builtin re-added to main gets launch-equivalent protection, while a non-main service declaring the same name keeps removable behavior.DefaultActivatorTest: with no builtins inorderedDependencies(), a deployment merge retains builtin config (and still removes absent non-builtins); a rollback whose snapshot lacks builtin config retains it (and still discards absent non-builtins).KernelConfigResolverTest:resolve()with an empty main dependency map still emits every autostart builtin asname:HARD.Healthy-graph behavior is unchanged: all pre-existing tests in the touched areas pass unmodified (
KernelConfigResolverTest,DefaultActivatorTest,KernelUpdateActivatorTest,DeploymentActivatorFactoryTest,SetupDependencyTest,GreengrassServiceTest,KernelTest,KernelLifecycleTest,LifecycleTest,MergeTest), with checkstyle/PMD active. The full failure chain was additionally reproduced end-to-end against unmodified v2.10.2 config/lifecycle classes; re-running that reproduction against this patched build confirms the chain breaks at step 2 (no flag erasure, no eviction, no config deletion, all state survives restart).Any additional information or context required to review the change:
@ImplementsService(autostart=true)builtins — the same criterionlaunch()uses — so on a healthy device the unions add nothing. Protecting a name absent from a device's graph is harmless (MERGE only retains existing config), and statically added names always resolve since autostart builtins are compiled into the nucleus.createRollbackMergeBehaviorwidened from private to protected for testability, matchingcreateDeploymentMergeBehavior.cleanupGroupDataone-way ratchet that amplifies this loss into component removal) and fix: Use USE_LONG_FOR_INTS to prevent unnecessary config.tlog writes #1820 (fix for the LogManager-driven config churn that drove near-constant tlog truncation, shrinking the loss-exposure window). This PR supersedes test: builtin service dependencies #1823 (characterization tests demonstrating the pre-fix behavior, by @saranyailla): its two bug demonstrations are covered here as inverted regression tests, and its two baseline invariant tests are incorporated (with attribution) in the final test commit.Documentation Checklist:
Compatibility Checklist:
any deprecated method or type.
Refer to Compatibility Guidelines for more information.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
"Simple" Explanation of the Situation:
Background: the builtin services
The nucleus runs four internal "builtin" services compiled into it —
DeploymentService (processes deployments), FleetStatusService
(reports device status to the cloud), TelemetryAgent, and
UpdateSystemPolicyService. They are not deployed as components;
instead, at every boot the nucleus adds them to its in-memory service
dependency graph with a "shield flag" that exists only in memory
(🛡️ below) — nothing about them is persisted to disk.
They matter here for two reasons:
Their config holds critical device state. DeploymentService's config
records which thing groups this device belongs to and which components
each group wants running. If that record is lost, the next deployment
treats those components as orphaned and removes them.
Their protection is fragile. Two things guard them — the shield
flag (🛡️), a memory-only marker guarding their place in the graph,
and the protection list, built fresh at each deployment to guard
their config during the merge. Both are computed from current runtime
state rather than from the fixed builtin list, and breaking the first
silently disables the second:
Background: deployments — the merge, and cloud vs local
Each deployment applies its configuration by merging it over the
existing config tree ("the merge" below). Two deployment types matter:
cloud (thing-group) deployments carry the target group's full
component list from the cloud; local deployments (submitted on the
device itself — some fleets run a component that auto-submits one at
every startup) carry only their own changes, and preserve every other
group's components solely from the device's local records. (Both
types fetch group membership from the cloud — lab-verified — but
membership alone cannot rebuild a missing local record of a group's
components.) That difference decides 💥 vs ✅ in the scenarios below.
Background: state - kept in three places
attached/DETACHED = whether the subtree is still reachable from the
config root (see primer below)
persisted copy of main's dependency list stored inside it (a "topic" is
a node in the nucleus config tree — Greengrass jargon, not MQTT). The
file = last periodic full rewrite (a root walk, every ~21 min on this
device) plus individual appends since — writes to detached nodes still
append, but each rewrite discards them and omits detached data
Background: attached vs detached config
Like a reference to a linked list item which is no longer in the list:
the running service keeps reading and writing its subtree through its own
pointer — writes even still append lines to config.tlog — but anything that
persists config by walking the tree from the root (truncation rewrites,
effectiveConfig.yaml) no longer sees it. The subtree evaporates for good
when the process exits.
Scenario A — the failure path (the incident)
The two windows
Scenario A passed through two named degraded states — they differ in
whether the damage has reached the disk, which decides what a restart does:
W1 is transient (hours, typically); W2 can persist for weeks while the
device looks perfectly healthy — one restart away from impact.
Scenario B — first deployment succeeds (the normal fleet path, safe)
The whole vulnerability lives in one corner: only a rollback to the
pre-first-deployment snapshot restores a builtin-less list. After one
successful deployment, the persisted list includes the builtins and
rollbacks are harmless.
Related safe fork: a first deployment that fails without rollback
(failure handling policy DO_NOTHING) is also harmless — no snapshot
replay means no builtin-less list is ever restored, so nothing is
evicted. Scenario A specifically requires fail + rollback.
Scenario C — restart during W1 (safe, heals)
Same start as Scenario A, but the device restarts before any further
deployment runs:
The heal works because in W1 the disk still holds everything: the restart
discards only the damaged in-memory graph (rebuilt at boot) and loses no
data. W1 is transient though — it ends at the next deployment (→ A③), and
fleets with auto-submitting components deploy often.
Scenario D — restart during W2, but no deployment afterward (latent)
On the incident device, this window lasted 18 seconds — the gap between
the restart and the component's auto-submitted local deployment.
Scenario E — cloud deployment after the restart (recovers)
Yes — this succeeds and recovers the device, because the cloud deployment
carries the group's component list with it: it doesn't need the (missing)
local record. This is why the remediation guidance is "deploy to the
degraded device" and it is essentially how the incident device was
recovered. Lab-verified addendum: the safety of this path is inherent,
not timing luck — the resumed/processed cloud deployment persists the
group record before any startup local deployment can be processed
(deployments are handled serially, in order).
Variants: multi-group devices → Scenario F; the recovery deployment itself
failing → Scenario G.
Scenario F — multi-group device, cloud deployment after the restart (partial recovery, collateral removal)
Device belongs to TWO thing groups, Alpha and Beta, each deploying its own
components:
The membership lookup protects a group's record from cleanup, but a
missing record is never rebuilt from the cloud — component preservation
still reads only local state. This one-way property is what we call the
"ratchet": group records are destroyed automatically, but only ever
rebuilt by a deployment targeting that specific group. So recovery is
per-group, and each group's deployment removes the still-unrecovered
groups' components in the meantime.
Scenario G — recovery deployment fails and rolls back (re-arms the bug)
Single-group device; the recovery attempt itself fails after activation:
The pre-first-deployment corner generalizes: on an unfixed nucleus, ANY
rollback whose snapshot holds a builtin-less deps topic re-evicts the
builtins. A degraded device's snapshots hold exactly that — so every
failed deployment on a degraded device re-arms the cycle.
Scenario H — nucleus UPGRADE fails on a W2 device
A nucleus upgrade is a bootstrap deployment: it reboots the device
mid-deployment, and a failure rolls back by REBOOTING AGAIN into the old
nucleus from a rollback snapshot. On a W2 device that snapshot is
poisoned:
Unlike Scenario G (ordinary deployment rollback — no reboot, memory
survives, nothing new lost), the upgrade's rollback path REBOOTS, so the
detached in-memory group state evaporates and the loss lands immediately.
Operational implication (confirmed): upgrades of degraded devices should
be monitored, and a failed upgrade followed immediately by a fresh
thing-group deployment — lab-verified to fully restore the device
(with a component-down window in between).
Contrast: the same failed upgrade on a W1 device is harmless — its
snapshot is complete (everything still attached), the rollback reboot
heals it like any W1 restart (→ C), and a W1 device has no deployed
components to lose anyway (its first deployment never succeeded).
Status: Reproduced. A W2-degraded device upgraded
toward a fixed build (deployment also carried a broken component) hit
FAILED_ROLLBACK_COMPLETE; post-rollback, DeploymentService booted with
empty group state, and the first local deployment removed the running
component. Notably, the cloud membership fetch SUCCEEDED and did not
help — the decisive variable is the missing local record of the group's
components, which membership alone cannot rebuild. The fix status of the
upgrade target is irrelevant: the rollback executes old-nucleus code.
Scenario I — reboot(s) before the first deployment, which then fails (same as A)
Pre-deployment reboots change nothing: each boot recreates the same
state. The vulnerable corner — a builtin-less deps topic on disk — exists
from provisioning until the first deployment writes builtins into the
topic, no matter how many reboots happen in between. Only a deployment
can close the corner (→ B) or spring it (→ A).
Scenario J — the FIRST deployment is a bootstrap deployment that fails (safe!)
A bootstrap deployment (e.g. one changing the nucleus version) REBOOTS
the device mid-deployment, before activation completes. Assume the
rollback itself does not bootstrap/reboot (no bootstrap-on-rollback):
The eviction in Scenario A requires the flag-erasure (②) and the
snapshot replay (rollback) to happen within one uninterrupted boot
session. A bootstrap deployment inserts a reboot between them, which
re-arms the shields — so it is immune. This is also why the incident's
trigger had to be an ordinary component/config deployment: those never
reboot, so nothing restores the flags before the rollback replays the
poisoned list.