Restart component when a revision changes its generated unit

[allanli4]

Description

A component revision that keeps the same version but edits the recipe rewrites the generated systemd
unit and then never applies it. RequiresPrivilege is the visible case, since it maps to the unit's
User=/Group=: the file on disk gets User=root, the process keeps running as ggcore, and the
deployment reports success.

Root cause. convert_to_unit() (deployment_handler.c:3607) regenerates units unconditionally,
above the redeploy gate. The gate at :3620 was component_updated || is_component_config_updated(...),
where component_updated is decided by version-string equality alone (:3310-3334, next to a TODO
naming this exact gap) and is_component_config_updated() inspects only the deployment document's
configurationUpdate. Neither consults the recipe. On a same-version revision both are false,
gghealthd reports the component RUNNING, and :3674 logs "Component ... is already running. Will not redeploy." — so the component never enters components_to_deploy and everything behind
if (components_to_deploy.map.len != 0) is skipped as a set: the systemctl stop, the link, the
enable, and the only daemon-reload. The trailing systemctl start greengrass-lite.target is a
no-op on an already-active unit.

This also explains the reporter's workaround: removing the component deletes services/<name> wholesale
(stale_component.c:207), so the next deployment finds no previous version and treats it as new.

The change. convert_to_unit() now reports whether generating the units actually changed any unit
file, via a new HasPhase.unit_changed, and the gate honours it. The comparison must happen inside unit
generation because create_unit_file() opens with O_TRUNC: old and new content coexist only until it
runs, and nothing persists a digest between deployments, so afterwards no caller could recover it.

Everything the issue asks for — regenerate, daemon-reload, restart — is then done by code that already
existed and was simply unreached. The production change is one conditional, one comparison helper,
and one struct field.

Keying on "did the generated unit change?" rather than on RequiresPrivilege specifically is both
smaller and more general: it covers every recipe field the unit encodes, and it makes the no-op case
free, because an unchanged recipe generates byte-identical content and so restarts nothing.

Related Issue

Fixes #954

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update
• Performance improvement
• Code refactoring

Checklist

• Code passes all the quality test. Can try nix flake check -L locally
  — could not be run: nix is not installed on the dev host, nor are clang-format,
  clang-tidy, cmake-format, gersemi, prettier, cspell or shellcheck. So clang-tidy,
  iwyu, cmake-lint, spelling, build-clang, build-musl-pi, the formatter check and UAT run
  here in CI for the first time. Style was conformed to .editorconfig and surrounding code by
  inspection. Flagging rather than ticking.
• Documentation updated (if applicable) — not applicable, verified rather than assumed: no prose
  documentation describes the redeploy decision or RequiresPrivilege adoption semantics.
  RequiresPrivilege appears only in three example fixtures under
  docs/examples/supported_lifecyle_types/, and docs/design/ggdeploymentd-design.md has no
  description of restart or redeploy behaviour.
• Tests added/updated in
  aws-greengrass-testing
  (if applicable) — not added there. Two inline unit tests ship in this PR, and the end-to-end
  behaviour was verified on a device (below). Happy to add a UAT case if you'd prefer it there.

Documentation Updates

• Updated README.md if needed — not needed; no README statement is affected.
• Updated relevant documentation in docs/ folder — not needed; see above.
• Requested to update public documentation if applicable (aws internal only) — not requested. If the
  reboot consequence below is accepted as intended behaviour, it may deserve a public-doc note.

Testing

• Existing tests pass
• Added tests to
  aws-greengrass-testing repository
• New functionality is covered by tests

Unit — two inline tests in modules/recipe2unit/src/parser.c; recipe2unit-inline-test goes 4 → 6
tests, 0 failures. revised_unit_content_reported_as_changed covers the reported case and gates the
first deployment as a hard precondition, so it cannot pass because no unit was ever written.
unrevised_unit_content_reported_as_unchanged covers the no-op case.

Each test was verified to bind one direction of the behaviour: neutralising the comparison to always
report changed fails only the second test, and to never report changed fails only the first. Full suite
and build are unchanged from base — the same 3 pre-existing Not Run targets and the same 13
pre-existing failed objects, all missing system libraries on the dev host (OpenSSL 3.x headers,
systemd/sd-bus.h, old uriparser). Zero diagnostics in the touched files.

On a device (EC2, Ubuntu 24.04, both .debs built on the host, torn down afterwards). Same version
1.0.0 both steps, recipe edited in place, component deployed alone:

	unit file	process owner	MainPID
base d3c2fa3f step 1	User=ggcore	ggcore	14441
base d3c2fa3f step 2	User=root	ggcore	14441
patched step 1	User=ggcore	ggcore	2494
patched step 2	User=root	root	2884

The MainPID is the discriminator: unchanged on base proves the process was never restarted; changed on
the patched build proves it was. Both directions passed the same precondition gate, and the running
ggdeploymentd binary was asserted by hash against the expected artifact in each.

Note for anyone reproducing: a version bump does not reproduce this. It leaves component_updated
true and routes around the defect entirely.

Additional Notes

Two consequences worth your decision, neither hidden:

1. A same-version recipe edit can now reboot the device, for components that declare a bootstrap
   phase. Entering components_to_deploy means process_bootstrap_phase() runs, and it ends in
   systemctl reboot when it counts any bootstrap component (bootstrap_manager.c:632). Since
   ComponentDescription is embedded in the unit as Description=, even a cosmetic edit qualifies. A
   version bump on such a component already reboots today, so this makes same-version edits behave like
   version bumps — but it is a policy call about how aggressively lite should adopt a revision, and it's
   yours, not mine. The narrower alternative is to exclude cosmetic fields from the comparison; I did
   not take it because it needs a field allowlist that is silently wrong the moment a field is added.
2. The signal is not idempotent across a failed-then-retried deployment. services/<name>/version
   is written unconditionally at :3384, well before the gate, so if a deployment fails after unit
   generation, a retry sees a matching version and matching on-disk units and skips the component again.
   This is not introduced here — at base the same-version case never worked at all — but it bounds the
   fix. The robust form persists a digest of the generated units alongside the version, which belongs
   with the standing TODO at :3709 about HasPhase being populated and then ignored.

One question for you: HasPhase.unit_changed puts a field that is not about phases on a type called
HasPhase. I chose it over a new out-param because HasPhase is already the out-param bundle the
caller declares, so no signature change is needed and the test_modules/recipe2unit-test example driver
keeps compiling (verified with -DBUILD_EXAMPLES=1). Happy to rename the type if you'd rather.

Correctness note: the comparison uses gg_file_read, which returns the true byte count at EOF. The
sibling gg_file_read_exact would have made every sub-2048-byte unit report "changed", silently turning
this into "restart everything on every deployment". Failure directions are all conservative — an absent
or unreadable unit counts as changed — so the fix can cause an unnecessary restart but never miss a real
change. This does rely on unit generation being deterministic for a fixed recipe, which it is today
(no timestamp, pid or ordering dependence); a future non-deterministic field would break the no-op case.

Out of scope, filed separately in spirit: unit_file_generator.c:761 fchowns a component's work
directory to the configured posixUser unconditionally, never consulting is_root, so a privileged
component runs as root with a ggcore-owned work directory. That is the second half of what #954
reports, but it is wrong on a first deployment too, so it is an independent defect and folding it in
would have widened this diff.

This PR was prepared with AI assistance; every claim above was verified by execution on the device or
in the test suite.

By submitting this pull request, I confirm that you can use, modify, copy, and
redistribute this contribution, under the terms of your choice.