test: builtin service dependencies - #1823
Draft
saranyailla wants to merge 1 commit into
Draft
Conversation
8 tasks
aws-kevinrickard
added a commit
to aws-kevinrickard/aws-greengrass-nucleus
that referenced
this pull request
Jul 17, 2026
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
added a commit
to aws-kevinrickard/aws-greengrass-nucleus
that referenced
this pull request
Jul 17, 2026
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>
This was referenced Jul 20, 2026
aws-kevinrickard
added a commit
that referenced
this pull request
Jul 20, 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>
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
1 participant
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:
Description of changes:
Why is this change necessary:
How was this change tested:
Any additional information or context required to review the change:
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.