fix: local deployments preserve running components across bookkeeping loss

[aws-kevinrickard]

Summary

Fixes silent removal of running components by local deployments when thing-group bookkeeping has been lost, plus two related defects in group-state handling. Cloud deployment semantics are unchanged: a component is removed when the cloud deployment no longer contains it.

The defects

1. Local deployments removed running components on missing bookkeeping. The set of root components a deployment keeps is computed exclusively from local GroupToRootComponents records. A running root component whose record was absent — for any reason, including loss of persisted state — was silently removed by a local deployment's merge, even when ListThingGroupsForCoreDevice confirmed the device still belongs to the group. Local deployments are explicit add/remove deltas (rootComponentVersionsToAdd / rootComponentsToRemove); they have no authority for implicit removals. Observed in the field as a device's components being uninstalled by an auto-submitted startup local deployment right after a restart.
2. Unknown membership treated as empty. An Optional.empty() membership response (device not configured to talk to the cloud) was coerced to "member of no thing groups", dropping every recorded group from preservation and arming record cleanup.
3. Cleanup without evidence after restarts. DeploymentService.cleanupGroupData() deletes records absent from the GroupMembership snapshot. Deployments completing after a Nucleus restart (bootstrap, e.g. nucleus upgrades) run in a boot session where that snapshot may not have survived — deleting records of groups the device still belongs to. Multi-group devices were exposed on every nucleus upgrade.

The fix

A local deployment now removes a running root component only with positive evidence: the request explicitly lists it in rootComponentsToRemove, or membership freshly fetched from the cloud in the same run shows the device no longer belongs to any group the component is attributed to (via ComponentToGroups). Running roots lacking such evidence are preserved at their running version, with a warning log. Preservation is resolution-only (no bookkeeping writes), fails closed for versionless services, and excludes the nucleus component and builtin services. Unknown membership falls back to persisted membership info (same as the existing fetch-failure paths). Deployments completing after a restart skip record cleanup; the next regular deployment performs the deferred hygiene with fresh data.

Behavior notes

• Cloud deployments are intentionally unchanged, including in the bookkeeping-loss state: a running component that no record or document accounts for is still removed by a cloud deployment (pinned by a test). Recovering a device from bookkeeping loss remains one cloud deployment per group, which rebuilds each group's records.
• For local deployments, components with no bookkeeping (e.g. services provisioned via the install-time config file) are now preserved rather than incidentally removed; explicit removal still works.
• The legitimate flows are pinned by tests: a device that actually left a group has that group's components removed by the next deployment that fetches membership; explicit local removals are always honored; device-scoped (thing/) and local-deployment records remain exempt from membership-based cleanup.

Tests

• ThingGroupMembershipPreservationTest: 3 regression tests (verified failing with the fix disabled) — lost-record local deployment, unknown-membership handling, post-restart cleanup; plus control, legitimate-flow, fail-closed, and cloud-semantics pinning tests (verified passing both with and without the fix).
• GroupToRootComponentsCleanupPreservationTest: exercises the real DeploymentService cleanup path (the original revision inlined a copy of the logic and could not observe a fix).

The original revision's headline test asserted that a lost record's components could be restored from the membership response alone; that is not implementable (membership returns only group names, and no device data-plane API fetches a group's component list without its deployment ID, lost in the same scenarios). The reworked contract preserves what is running during local deployments instead.

mvn test run with checkstyle/PMD/spotbugs active: 1192 tests, the only failure is a pre-existing one unrelated to this change (verified identical on the unmodified branch). MultiGroupDeploymentTest integration suite at exact parity with the unmodified-branch baseline.

Related

Companion to #1824, which fixes the loss of persisted configuration (builtin service config protection) that produced the missing-record state in the field. #1824 prevents the state loss; this PR removes the local-deployment amplification of it. Also related: #1823 (characterization tests for the #1824 defect).

[MikeDombo]

Recommendation generated by Amazon CodeGuru Reviewer. Leave feedback on this recommendation by replying to the comment or by reacting to the comment using emoji.

The ObjectMapper.readValue() method can throw a JsonProcessingException, but this exception is not handled properly here. By simply logging the error and returning an empty value, the exception is ignored. This can mask potential JSON parsing issues, leading to unexpected application behavior or data corruption because the system may continue with invalid or incomplete data. To fix this, catch JsonProcessingException and either handle it appropriately or rethrow it as a custom runtime exception. https://www.ibm.com/support/pages/best-practice-catching-and-re-throwing-java-exceptions

[aws-kevinrickard]

Due to Finding 1 below, this change can't be made. Closing

Findings

1. (Discussion, should be acknowledged in the PR) After total bookkeeping loss, a cloud redeployment of a component's own group cannot remove it. If both GroupToRootComponents and ComponentToGroups are lost (the realistic loss scenario — they live in the same config subtree), attributedGroups is empty and hasPositiveRemovalEvidence returns false unconditionally. A cloud deployment to group A whose document drops component Y will not remove Y, because nothing ties Y to A. The only removal paths left are explicit local removal or the device leaving the group. The PR description says "the authoritative repair path (a cloud deployment for their group) is unchanged" — that's true for restoring bookkeeping (deploy with Y, then without), but a one-shot "deploy A without Y" no longer removes Y in the loss state, where it did before. This is the right conservative default (you can't distinguish "Y belonged to A" from "Y belonged to B"), but it's a genuine behavioral asymmetry the description soft-pedals; I'd state it explicitly and consider a doc note on the operator workflow (deploy-with-then-without, or local removal).

2. (Behavior change beyond the incident scenario) Components provisioned via initial config file are now preserved. Any non-builtin direct dependency of main with no bookkeeping — including services defined in the install-time config.yaml, which never had GroupToRootComponents records — will now survive cloud deployments instead of being removed by the first one. Arguably a long-requested improvement, but it's a customer-visible change that isn't in the PR description and has no test. Worth an explicit sentence in the description and ideally a test encoding it as intended (not accidental).

3. (Edge case) runningVersion == null → "*" requirement. Services defined in config files frequently lack a version key. A * requirement lets resolveComponentVersion negotiate any version — for a cloud-connected device this could resolve a same-named cloud component and effectively install/upgrade something the user never deployed, silently. Consider failing preservation closed for versionless services (skip + warn) or pinning to the local active recipe only.

4. (Tradeoff worth stating) Preservation converts silent removal into potential deployment failure. A preserved component is pinned to its exact running version; if a new deployment's dependency closure conflicts with that pin, resolution now throws NoAvailableComponentVersionException and the whole deployment fails — where previously it succeeded by silently dropping the component. Failing loudly is defensible and the currently-running-components label in the constraint diagnostics plus the warn log make it debuggable, but this is the operational cost of the fix and should be in the description.

5. (Nit) Test method naming deviates from repo convention. The codebase uses GIVEN_x_WHEN_y_THEN_z (~912 methods across 94 test files); the new classes use snake_case sentences. The new names are honestly more readable and the javadoc is excellent, but expect a maintainer to ask for consistency.

6. (Nits)

• ThingGroupMembershipPreservationTest uses fully-qualified new java.util.HashSet<>(java.util.Arrays.asList(...)) inline in one test — import instead.
• @SuppressWarnings("PMD.ExcessiveClassLength") on all of DeploymentService for a ~10-line addition ratifies the class's growth; fine pragmatically, but a targeted refactor ticket reference in the suppression comment would be better.
• The CodeGuru comment about swallowing JsonProcessingException in getComponentsExplicitlyRemovedByLocalRequest is mostly noise (the document already parsed upstream), but note the failure consequence is inverted from what CodeGuru implies: a swallow means an explicit removal is not honored and the component is preserved. Given upstream parse success is a precondition, the current handling is acceptable — the code comment already says why.

[aws-kevinrickard]

Reworked per the review findings and continued in #1826 (this PR could not be reopened after the rework was pushed to the branch). Key change: the fix is rescoped to local deployments only — cloud deployment removal semantics are unchanged, addressing finding 1. Findings 2–6 are also addressed; details in #1826's description.