fix: do not treat unknown thing group membership as empty membership - #1826
Open
aws-kevinrickard wants to merge 2 commits into
Open
fix: do not treat unknown thing group membership as empty membership#1826aws-kevinrickard wants to merge 2 commits into
aws-kevinrickard wants to merge 2 commits into
Conversation
MikeDombo
reviewed
Jul 20, 2026
aws-kevinrickard
force-pushed
the
regression/thing-group-membership-preservation
branch
from
July 21, 2026 00:06
b18bbc9 to
47c1d5c
Compare
aws-kevinrickard
changed the title
|
Unit Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 5729e76 |
|
Integration Tests Coverage Report
Minimum allowed coverage is Generated by 🐒 cobertura-action against 5729e76 |
alter-mage
reviewed
Jul 24, 2026
| "A non-thing-group record must not be treated as a thing group membership by the " | ||
| + "unknown-membership fallback"); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Suggestion (non-blocking): Would you consider adding a test in this class for the throw-path (case 2 — listThingGroupsForDevice() throws an exception, e.g. network unreachable)? The PR description notes that path is already correct, but pinning it here would complete the four-corner contract and guard against future regressions that accidentally weaken the catch-block fallback. Something like:
@Test
void GIVEN_membership_lookup_throws_WHEN_local_deployment_THEN_recorded_group_components_are_preserved()
throws Exception {
when(mockThingGroupHelper.listThingGroupsForDevice(anyInt()))
.thenThrow(new IOException("network unreachable"));
// ... same assertion pattern as the unknown-membership test
}Not a blocker — the existing exception handling is untouched by this change — but it would make this test class a single source of truth for all membership-resolution outcomes.
ThingGroupHelper.listThingGroupsForDevice() returns Optional.empty() when the device is not configured to talk to the cloud - no lookup is attempted, so the membership is unknown. The deployment task coerced that unknown to an empty set, i.e. an authoritative "member of no thing groups". One local deployment processed in that state removes every recorded thing group's components from the device (their groups are dropped from the merge's preserved root set) and rebuilds the GroupMembership snapshot empty, which lets the post-deployment cleanup delete every thing group's records. No prior loss of state is required: a device whose cloud provisioning config is missing or invalid (including lost or corrupted config) running a local deployment is sufficient. Unknown membership now falls back to the membership implied by the persisted group records, exactly as the existing fetch-failure paths do. An authoritative lookup is unaffected, including one returning an empty set: a device that genuinely belongs to no thing groups still has its group components removed and records cleaned up.
A real membership lookup (ListThingGroupsForCoreDevice) can only return thinggroup/-prefixed names, so the persisted-records fallback must not imply membership in anything else. The previous filter (everything except LOCAL_DEPLOYMENT and thing/-prefixed records) also resurrected records whose group name is not a thing group, such as a raw configuration ARN that failed to parse. Such a record's pinned root components were then added to the merge's preserved root set, where they can impose stale version constraints on later deployments and make dependency resolution unsatisfiable. For devices deployed through the cloud APIs the two filters are equivalent: configuration ARNs always parse, so persisted records are thinggroup/-prefixed, thing/-prefixed, or LOCAL_DEPLOYMENT.
aws-kevinrickard
force-pushed
the
regression/thing-group-membership-preservation
branch
from
August 12, 2026 00:27
5729e76 to
e76d9cc
Compare
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.
Summary
Fixes a defect where an unknown thing-group membership response was treated as an authoritative "member of no thing groups" — causing a single local deployment to remove every recorded thing group's components from the device and delete the groups' bookkeeping records.
The defect
During deployment processing,
DefaultDeploymentTask.getNonTargetGroupToRootPackagesMap()fetches the device's current thing-group membership viaThingGroupHelper.listThingGroupsForDevice()to decide which recorded thing groups' components to preserve. That lookup can end three different ways, and it is important to distinguish them:ListThingGroupsForCoreDevicecall succeeds, and the response (possibly an empty list) is the truth about membership.FORBIDDEN, retries exhausted). This surfaces as an exception and is handled by existing fallback paths that use the persisted group records instead. This path was already correct and is not changed by this PR.Optional.empty()) —listThingGroupsForDevice()has a guard at the top: ifisDeviceConfiguredToTalkToCloud()is false, it returnsOptional.empty()without making any cloud call. This means the device's cloud provisioning configuration (thing name, IoT data/credential endpoints, certificate path, private key path, root CA path, AWS region) is missing or invalid, so the question "which groups am I in?" cannot even be asked.When does case 3 actually come up? A device can be in that state because:
Note that a device that is merely offline (network down, cloud unreachable) does not produce
Optional.empty()— its config still validates, so the call is attempted, throws, and takes the already-safe fallback path (case 2).Optional.empty()occurs specifically when the configuration itself is invalid.Because case 3 returns an empty
Optionalrather than throwing, none of the case-2 fallbacks fire. The buggy code coerced the unknown into an empty membership set — an authoritative "member of no thing groups". Consequences of one local deployment processed in that state:GroupMembershipsnapshot is rebuilt empty → the post-deployment cleanup deletes every thing group'sGroupToRootComponents/GroupToLastDeploymentrecords, so the components stay gone even after the configuration is repaired, until each group is redeployed from the cloud.No prior loss of deployment state is required. A plausible end-to-end trigger: a device's cloud config becomes invalid, and a component auto-submits a local deployment at startup (a common fleet pattern) — that single local deployment uninstalls every thing-group component on the device.
The fix
Unknown membership (
Optional.empty()) now falls back to the membership implied by the persisted group records — exactly the behavior of the existing attempted-and-failed fallback paths. "We could not determine membership" is no longer treated as "we determined the device has no memberships". One code path, nine lines.Optional.empty())FORBIDDEN)Tests
ThingGroupMembershipPreservationTest:GroupMembershipfrom persisted info.Optional.empty()vsOptional.of(emptySet)distinction, guarding against over-preservation (passes with and without the fix).thinggroup/-prefixed records; a record whose name is not a thing group is not resurrected as a membership.Fallback scope
The fallback returns only
thinggroup/-prefixed record names — the only names a real membership lookup can return. Without that restriction, a record whose group name is not a thing group (e.g. a raw configuration ARN that failed to parse) would be treated as a membership, and its pinned root components would enter the merge's preserved root set, where they can impose stale version constraints on later deployments and make dependency resolution unsatisfiable. For devices deployed through the cloud APIs this is behavior-neutral: configuration ARNs always parse, so persisted records arethinggroup/-prefixed,thing/-prefixed, orLOCAL_DEPLOYMENT.Full
mvn clean testwith checkstyle/PMD/spotbugs active: 1196 tests, the only failure is pre-existing and unrelated (identical on the unmodified branch).Scope history
This PR was originally broader (running-component preservation for local deployments and a post-restart cleanup deferral). After further lab testing showed those changes address no remaining demonstrated failure once #1824 is deployed, they were narrowed out of this PR and parked on the
local-deployment-preservation-parkedbranch for a possible future defense-in-depth PR. This defect is the one remaining demonstrated bug not covered by #1824.Related
#1824 fixes the loss of persisted configuration (builtin service config protection) observed in the field. This defect is independent of it — it needs no prior state loss.