Skip to content

feat(service): register engines at an explicit scope, with reboot survival reported - #51

Open
MichaelTaylor3d wants to merge 11 commits into
mainfrom
fix/526-service-scope
Open

feat(service): register engines at an explicit scope, with reboot survival reported#51
MichaelTaylor3d wants to merge 11 commits into
mainfrom
fix/526-service-scope

Conversation

@MichaelTaylor3d

@MichaelTaylor3d MichaelTaylor3d commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Closes #526
Closes #1863
Closes #919
Closes #1774

What changed

The scope model (src/svcscope.rs) — new, pure, and the primary evidence

Every decision takes its OS, its privilege and its filesystem facts as PARAMETERS: engine_scope, engine_scope_for_program, scope_args, deregister_scopes, survives_reboot_without_login, agent_disposition, shadowing_units_to_remove, engine_registration, is_unknown_scope_flag_rejection. Nothing reads cfg!, the real uid, or the disk, so every arm is asserted for all three operating systems from an unelevated Windows dev box (#1774). Reboot survival with no login session comes from exactly three mechanisms — the systemd multi-user.target.wants symlink, a launchd system-domain RunAtLoad, and the SCM AUTO_START — and that fact is now written down once.

OS component elevated + protected root elevated + --bin-dir unelevated
Linux engine System User (forced) + "will NOT survive a reboot" User
macOS engine System User (gui/<uid>) + note User
Windows engine System (SCM start= auto) same refused by the elevation gate
any dig-app per-user login autostart, never a daemon same same

W3 — scope plumbing + the compat fallback (src/service.rs)

install/start/uninstall take a scope; ServiceInstallOutcome reports requested_scope, scope_flag_accepted and reboot_survival. The argument surface is exactly two tokens, --scope system / --scope user — byte-identical with dig-node's --scope <auto|system|user>; auto is never passed, because "whatever the component defaults to" is the defect being closed.

Because the installer downloads the LATEST dig-node with no version pin, a build that rejects the unknown flag (clap, non-zero exit, before any side effect) is retried WITHOUT it, and reboot_survival then reports what that build ACTUALLY does — svcscope::legacy_default_scope(os), which is User on unix (a downgrade, warned about) but System on Windows (no downgrade, since the SCM has no per-user domain). The retry is gated on a message naming --scope: retrying any other failure unflagged would silently downgrade a system install to a login-gated one and still report success.

W4 — a failed install is no longer freely tolerated

The old rule ("only a start failure is fatal") is exactly how a user-level-under-root registration failure reached DIG is ready. Tolerance is now judged by a scope-EXPLICIT PRESENCE probe (svc::registration_in_scope, built on the new pure svc::scope_query): tolerated only when the REQUESTED scope already holds a registration, and Unknown is not tolerance. install_service_tolerates_an_install_failure_when_start_is_not_requested encoded the old contract and was amended deliberately (rationale in commit 0cae749); the task-#232 case it existed for is retained as its own test.

W5 — shadowing + adoption (src/units.rs)

After a system-scope register, every unpackaged per-user unit — the invoking user's AND /root/.config/systemd/user/, which a pre-#526 sudo install wrote — is removed and NAMED in service.shadowing_units_removed. Moving the registration into the system domain closes nothing while the user unit still WINS its own resolution: systemd --user starts it at the next login and both bind the node's port.

An already-ENABLED packaged unit (the apt.dig.net .deb) is ADOPTED instead of double-registered; a present-but-DISABLED one starts nothing and is no reason to skip. A packaged unit's enablement is read from the .wants symlink rather than by spawning systemctl, because a spawn that fails on a host without systemd would answer "not enabled" for a unit that is — and that answer decides whether to skip an install.

W6 (#1863) — the plan-independent deregistration defect, for the three services

src/rearm.rs extracts PR #49's beacon fix as a generic rearm_after_migration, guard order verbatim (plan-selects → was-deregistered → root-is-protected → register → report), with a per-component RearmAdvice so the beacon keeps its exact SPEC §5 wording (a generic sentence was a regression, and the existing test caught it). The beacon is refitted onto it and every one of its pre-existing tests passes UNCHANGED — the proof the refactor is behaviour-preserving. Three service call sites added, each recording InstallAction::ServiceRegistered(id) so the existing LIFO rollback deregisters a restored registration by canonical id, each retracting its report claim when that happens.

regaudit::privileged_regs listed dig-node/dig-relay on Windows only, so this defect was live on Windows today and becomes live on unix the moment the scope flips. It now returns all four on every OS — which is what its doc already claimed ("mirrors paths::is_privileged_component", which listed them on unix all along). The unix test encoding the old set is amended, and a new test asserts the two sets AGREE instead of asserting it in prose.

W7 — uninstall symmetry

Engine services are deregistered at EVERY scope on every OS, unconditionally, with per-scope reporting; macOS boots out gui/<uid>/<id> as well as system/<id>. The authoritative signal is the scope-explicit END state, never the verbs' exit codes; a scope still holding a registration is a reported failure, and a scope that could not be reached is reported even when the end state is clean.

W8 (#919) — a Linux autostart that actually autostarts

Linux autostart had never worked once: the installer wrote ~/.config/systemd/user/dig-app.service and never enabled it, and could not have — systemctl --user enable needs the target user's session bus, which an elevated install does not have. enable_command merely PRINTED a command a human had to run. Linux now writes ~/.config/autostart/dig-app.desktop (no enable, no bus, no privilege) and REMOVES the stale unit on install and uninstall, or a user who ever ran systemctl --user enable gets two agents at login. enable_command/enable_command_when are deleted, and the invoker privilege-boundary census drops 8 → 7 deliberately.

Headless hosts SKIP with a reported disposition (skip-headless), write nothing, and never enable linger. is_headless(os, &SessionFacts) is pure and errs toward REGISTERING on purpose — the mistakes are asymmetric: a wrong "headless" silently denies a desktop user their agent, a wrong "graphical" leaves one inert file no session reads. A sudo install on a workstation whose DISPLAY was not forwarded is therefore NOT headless. An elevated run that cannot name the invoking account skips LOUDLY (skip-no-target-user) instead of aiming a "per-user" artifact at root (#1748).

Blast radius checked

gitnexus is disabled in the dev loop (CLAUDE.md §2.0 override), so this was done with ripgrep plus direct reads over the full caller set. Every caller is updated in this diff:

  • service::{install_args,start_args,uninstall_args} → callers are service.rs internals and its own tests; the signature change is crate-internal.
  • service::install_servicelib.rs::register_dig_node (plus the new reregister_engine_service). service::install_relay_servicelib.rs::register_relay (plus the same). service::uninstall_servicelib.rs::uninstall_dig_node (:3016) and SystemActions::stop_services (:3412).
  • ServiceResult / RelayResult gained fields → every literal updated (lib.rs production plus the running_service() test fixture). Both are --json payload types, so SPEC.md §3.1 documents the additions. Additive only — no field removed or renamed, so an existing --json consumer is unaffected, and every new field goes through the existing report plumbing rather than a raw println!.
  • regaudit::privileged_regsregaudit::audit, migrate::migrate_from_legacy_roots. This is the highest-risk edit in the diff — see the warning below.
  • autostart::register_for gained a parameter; install_systemd_user_unitinstall_autostart_entry; enable_command* deleted. Callers: autostart.rs itself, lib.rs (via register), lib.rs::undo_install_action (via deregister), and the invoker boundary census, which counts call sites by grep and was amended.
  • svc::deregister_service_command / stop_service_command (macOS arm) → svc::deregister_service / stop_serviceregaudit::PrivilegedReg::deregister, undo_install_action, migrate.
  • rearm_beacon_after_migration kept its exact signature so its existing tests are untouched.
  • svcscope, rearm, units are new; all three added to sources::all() (the structural-scan inventory has a test that fails otherwise).

WARNING — the one HIGH-risk change: regaudit::privileged_regs now returns dig-node + dig-relay on Linux/macOS. Two consequences on unix hosts that were previously untouched. (1) The #565 legacy-root migration will now stop and deregister a dig-node/dig-relay registration whose binary resolves under ~/.dig/bin — intended, and the new W6 re-arm is what puts it back when the plan declines the component. (2) regaudit::audit will now FAIL READINESS for a unix host whose dig-node registration points at a legacy user-writable root. The second is deliberate hardening, but it will make some previously-green upgrades report NOT ready until they migrate. Worth a reviewer's specific attention.

Two real defects CI caught in this diff, both mine

Recorded because they are the interesting part of the review surface, not footnotes.

1. A run-state probe cannot answer "is anything registered here?" systemctl is-active <unit> prints inactive for a unit that DOES NOT EXIST, and this crate's parse_systemctl_is_active maps inactive to Stopped — documented as "a real registration exists there, just not running". My scope probe was built on it, so it over-reported presence in both directions that matter: it would have TOLERATED a failed install against a unit that was never created (the exact false-ready #526 exists to close), and it reported a successfully completed uninstall as residual — which is how the ubuntu e2e failed. Replaced with svc::registration_in_scope -> Presence {Present, Absent, Unknown}; Linux asks is-enabled, whose vocabulary distinguishes an existing unit (enabled/disabled/static/masked, all registrations) from a missing one. Unknown is neither: unverified on uninstall, and not tolerance on install.

2. Not every refused --scope is a downgrade. The e2e pins dig-node 0.29.0, which predates the flag, so the compat path is what CI exercises — and on Windows the installer told the user its service "only starts once someone LOGS IN". False: the SCM has no per-user domain and install sets start= auto. svcscope::legacy_default_scope(os) now names what a pre---scope build actually does (System on Windows, User on unix); reboot_survival and the wording follow it. The fallback is still disclosed on every OS — only the login warning is confined to where it is true.

3. A clean end state is not a completed uninstall when every attempt failed. The root-gate job caught uninstall_dig_node_surfaces_a_missing_binary_without_panicking: with the launcher binary missing, every scope's attempt failed, the probe found nothing, and my code reported a clean uninstall. Nothing was deregistered and nothing could be asked, so the state is UNKNOWN — the stance lib.rs already takes for a launcher-gone uninstall. A partial failure with a clean end state stays tolerated and reported.

A fourth CI failure was mechanical: gui/app/src-tauri/Cargo.lock pinned dig-installer 0.34.0 while every GUI job builds --locked.

How verified

  • cargo fmt --check — clean.
  • cargo clippy --all-targets -- -D warnings — clean.
  • All 29 PR checks GREEN at 049c079 (gh pr checks 51), including both installer-e2e legs, the #1748 root gate (the whole suite as root), all three CodeQL analyses, and every GUI job.
  • cargo test706 passed, 1 failed locally. The failure is secure::tests::windows_verify_runs_on_a_program_files_dir_with_appcontainer_aces, which fails identically at the base commit b016269 (v0.34.0) on this Windows box — verified in a throwaway worktree. It is environmental (this host's Program Files ACLs, unelevated) and touches no file in this diff. cargo test --lib -- --skip windows_verify_runs_on_a_program_files → 706 passed, 0 failed. (It passes in CI, where the host is a fresh runner.) Integration binaries: cli 21, cross_browser_forcelist 5, nightly_release_workflow_shape 10 — all green.
  • GUI (gui/app): npm run lint → 0 errors, 1 pre-existing warning; npm test → 49 passed across 9 files. No GUI source changed.
  • Every new decision was mutation-proved load-bearing (committed first, reverted by file copy, never git checkout): gating engine_scope on elevation alone; survives_reboot always true; deregistering one scope only; skipping the headless check; dropping root's unit from the shadow set; ignoring enabled in adoption; dropping the --scope requirement from the compat matcher; collapsing scope_args to one token — 8/8 caught. Removing the #1863 call sites → 3 red; dropping its rollback record → 1 red. Linux headless ignoring installed desktop sessions; leaving the stale systemd unit; bypassing the disposition gate → 3/3 caught.

Fixture notes — where a false green would have hidden

  • The install-failure test's probe reports a registration in the OTHER scope, the shape a pre-#526 host is genuinely in, so a scope-BLIND probe would tolerate the failure: the fixture distinguishes the fix from the bug. A fixture reporting nothing anywhere would have passed against either implementation.
  • Unknown gets its own row, because a mock that can only answer Running/NotFound cannot express "could not ask" — which is the arm that produces the false-ready.
  • The shadow fixture carries TWO user units (the real user's and root's) plus a system unit plus an enabled packaged unit, so a filter at the wrong layer ("all but the last", "only the first", "everything user-ish") differs from the correct answer on it.
  • The headless table includes sudo on a workstation (no DISPLAY, desktop sessions installed) specifically because that is the row where the cheap implementation is wrong in the harmful direction.
  • The headless test drives a REAL (non-dry) run against a temp dir and asserts the reported artifact path does not exist — a dry run could not prove nothing was written.

Not done, and for the reviewer

  • The e2e now represents an interactive session (DISPLAY on Linux, SESSIONNAME on Windows) so the job exercises the autostart WRITE path rather than the headless skip. That is a simulation of a desktop session, stated plainly; the skip itself is unit-tested for all three operating systems, and the assertion still rejects the shape a genuine registration failure has (registered: false with disposition: register).
  • Not run: a real headless-Linux install. Everything above is proven by unit tests plus mutation on a Windows host; the actual systemd end state on a headless box is unverified here, and it is what #526's acceptance ultimately needs.
  • Cross-repo, NOT written here (reported instead, per this lane's scope): dig-app's own dig_app::autostart::linux still renders a systemd user unit, so the byte-identical-sibling contract in autostart.rs's module docs now covers macOS only. dig-app should adopt the .desktop entry, and SYSTEM.md needs the scope contract and the --scope argument surface recorded.
  • Version bumped 0.34.0 → 0.35.0 (minor): new capability, no public API removed. W4 and W8 change observable behaviour, but both tighten a contract that was silently not being met rather than removing a capability.

MichaelTaylor3d and others added 8 commits July 31, 2026 06:48
Lane anchor for the service-registration/deregistration batch (#526, #1863, #919, #1774).

Co-Authored-By: Claude <noreply@anthropic.com>
The scope, reboot-survival, shadow-removal, packaged-adoption and agent-
disposition decisions for every OS, as pure functions taking OS, privilege
and filesystem facts as parameters.

Purity is the point: a cfg(unix)-only test makes its guard unfalsifiable
on a Windows host, so the mutation stays green and the test proves nothing
(dig_ecosystem#1774). Every decision here is asserted for all three
operating systems from any host, unelevated.

Co-Authored-By: Claude <noreply@anthropic.com>
…allback

Plumb `--scope <system|user>` through install/start/uninstall for dig-node and
dig-relay, chosen by `svcscope::engine_scope` from the OS, this run's privilege,
and whether the binary landed in the protected root (a `--bin-dir` run is forced
to user scope per #565). The outcome now carries `reboot_survival` rather than a
note that could only ever imply it.

Compat: dig-installer downloads the LATEST dig-node with no version pin, so an
older binary is a real state. clap rejects an unknown flag with a non-zero exit
BEFORE any side effect, so that specific failure — and only that one, matched on
a message naming `--scope` — retries unflagged and records reboot_survival =
false with a plain-language note. Any other failure propagates: retrying it
unflagged would silently downgrade a system install to a login-gated one.

AMENDED CONTRACT (deliberate): `install_service` no longer swallows an `install`
failure. `install_service_tolerates_an_install_failure_when_start_is_not_
requested` encoded the old task-#232 rule — "an install failure alone is never
fatal" — which is exactly how a user-level-under-root registration failure
reached "DIG is ready": nothing asked whether a registration existed WHERE this
run needed one, and a leftover user-scope unit could even answer `start`. The
tolerance is now scope-explicit (svc::service_run_state_in_scope): tolerated only
when the REQUESTED scope already holds a registration. The task-#232 case it
existed for is kept as
`an_install_failure_is_tolerated_when_the_requested_scope_already_has_a_
registration`; `Unknown` is not tolerance, because "could not ask" is not "it is
there".

Uninstall now visits BOTH scopes on every OS, and macOS boots out `gui/<uid>`
as well as `system/` — a system-only teardown left an unelevated install's
LaunchAgent relaunching at every login under a clean uninstall report.

The spawn-based service tests are replaced by mock-runner tests taking the OS as
data: no spawn, no elevation (dig_ecosystem#1865), and no cfg-gated arm left
unfalsifiable on a Windows host (#1774).

Refs #526
Co-Authored-By: Claude <noreply@anthropic.com>
…run vacated

The #565 legacy-root migration deregisters privileged registrations
UNCONDITIONALLY, while each component's install step re-registers only when the
plan SELECTS it — so a re-run that DECLINED a component silently deleted its
working service. PR #49 fixed exactly this for the beacon; the three services
were left with the defect.

Extracts PR #49's fix into `src/rearm.rs` as a generic `rearm_after_migration`,
keeping its guard order verbatim (plan-selects -> was-deregistered ->
root-is-protected -> register -> report) and its exact user-facing wording via a
per-component `RearmAdvice` (the beacon's "auto-updates are now DISABLED" +
`dig-installer --auto-update` is a SPEC §5 requirement, so a generic sentence
would have been a regression — the existing test caught that). The beacon is
refitted onto the generic and every one of its pre-existing tests passes
UNCHANGED, which is the proof the refactor is behaviour-preserving.

Adds the dig-node/dig-relay/dig-dns call sites, each recording
`InstallAction::ServiceRegistered(id)` so the existing LIFO rollback deregisters
a restored registration by canonical id, and each retracting its report claim
when that happens (`retract_service_rearm_claims`, mirroring the beacon's).

`regaudit::privileged_regs` listed dig-node/dig-relay on WINDOWS ONLY, so this
defect was live on Windows today and would become live on unix the moment #526
flips the scope. It now returns all four on every OS, which is what its doc
already claimed ("mirrors paths::is_privileged_component" — which listed them on
unix all along). The unix test encoding the old set is amended deliberately, with
the rationale in place, and a new test asserts the two sets AGREE rather than
asserting the claim in prose.

Refs #1863 #526
Co-Authored-By: Claude <noreply@anthropic.com>
…dless hosts

Linux autostart had never worked once. The installer wrote
`~/.config/systemd/user/dig-app.service` and then never enabled or started it —
an unlinked user unit is inert — and it could not have: `systemctl --user enable`
needs the TARGET user's session bus, which a root install does not have (root has
no `--user` bus at all). So `enable_command` merely PRINTED a command a human had
to run. An install step whose success depends on the user reading a log line is
not an install step.

Linux now writes an XDG autostart desktop entry
(`~/.config/autostart/dig-app.desktop`), which every desktop session reads at
login with no enable, no session bus and no privilege. The stale systemd user unit
is REMOVED on upgrade and on uninstall — leaving it means a user who ever ran
`systemctl --user enable dig-app.service` gets two agents at login.
`enable_command`/`enable_command_when` and their printed advice are gone.

Headless hosts now SKIP the agent with a reported reason
(`AgentDisposition::SkipHeadless`) and write nothing — and never enable linger to
force a tray agent onto a server nobody is looking at. The verdict is a pure
`is_headless(os, &SessionFacts)` asserted for all three operating systems from any
host, and it errs toward REGISTERING on purpose: wrongly deciding "headless"
silently denies a desktop user the agent they installed, while wrongly deciding
"graphical" leaves one inert file no session reads. A `sudo` install on a
workstation whose DISPLAY was not forwarded is therefore NOT headless — desktop
sessions are installed on the box.

An elevated run that cannot name the account it is acting for now skips LOUDLY
(`SkipNoTargetUser`) instead of aiming a "per-user" artifact at root (#1748).

`invoker`'s privilege-boundary census drops from 8 to 7 sites, deliberately: the
removed site was `enable_command`, and its replacement has no boundary decision to
make.

Refs #919 #526
Co-Authored-By: Claude <noreply@anthropic.com>
…at shadow it

A system-scope registration is not enough on its own. Two live collisions, each
ending with two units for one service and both binding the node's port:

- a leftover per-user unit (`~/.config/systemd/user/dignetwork-dig-node.service`,
  or `/root/.config/systemd/user/…` from a pre-#526 `sudo` run) that
  `systemd --user` starts at the next login ALONGSIDE the system unit. Moving the
  registration into the system domain closes nothing while that unit still WINS
  its own resolution, so the check is positional: after a system-scope register,
  every unpackaged per-user unit is removed and NAMED in the report;
- the apt.dig.net `.deb`'s already-enabled `net.dignetwork.dig-node.service`,
  which is now ADOPTED rather than registered on top of. A present-but-DISABLED
  packaged unit is no reason to skip, since it starts nothing.

`src/units.rs` only ENUMERATES what is on disk; every decision about it is the
pure svcscope (already tested for all three operating systems from any host).
Enablement is read from the `multi-user.target.wants` symlink rather than
`systemctl is-enabled`, because a spawn that fails on a host without systemd would
answer "not enabled" for a unit that is — and that answer decides whether to skip
an install.

Refs #526
Co-Authored-By: Claude <noreply@anthropic.com>
… re-arm contract

SPEC.md gains §2.1a (the per-OS x component x privilege scope table, the three
reboot-survival mechanisms, the `--scope` argument surface + the compat fallback,
the scope-explicit install-failure tolerance, shadow removal, packaged-unit
adoption, and the uninstall-both-scopes rule), §3.1 (the new report fields) and
§3.11a (the re-arm guard order). §1.11 replaces the Linux systemd user unit with
the XDG autostart entry and states the headless dispositions.

runbooks/local-running.md: how to verify the scope and reboot survival per OS, what
the installer removes or adopts on your behalf, why `survives_reboot` may be false,
and that uninstall visits both scopes.

DEVELOPMENT_LOG.md: root has no systemd --user bus under sudo (so a user-level
service is unreachable there, and a scope-blind probe answers the wrong question);
and a written-but-never-enabled systemd user unit is not autostart.

Refs #526 #919 #1863
Co-Authored-By: Claude <noreply@anthropic.com>
MINOR per SemVer: new capability (explicit service scope, packaged-unit adoption,
headless-aware autostart, the service re-arm), no public API removed. The observable
behaviour changes in W4 (a failed install at the requested scope is now an error) and
W8 (the Linux autostart mechanism) tighten a contract that was silently not being met
rather than removing a capability, so neither is a breaking change.

Refs #526 #1863 #919 #1774
Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d MichaelTaylor3d changed the title fix(service): register services at a reboot-surviving scope and deregister them completely feat(service): register engines at an explicit scope, with reboot survival reported Jul 31, 2026
@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review July 31, 2026 15:46
MichaelTaylor3d and others added 3 commits July 31, 2026 08:56
…art + scope contract

Three real CI failures on this branch, all genuine:

1. `gui/app/src-tauri/Cargo.lock` pinned `dig-installer 0.34.0` while the crate
   moved to 0.35.0, and every GUI job builds with `--locked`. Re-pinned.

2. The e2e asserted `autostart.registered == true` unconditionally. A CI runner has
   no graphical session, so #919's headless skip fires and the assertion fails —
   correctly. The install step now NAMES a session (`DISPLAY` on Linux,
   `SESSIONNAME` on Windows) so the job still exercises the REGISTERING path (the
   write, its ownership, the artifact location) rather than the skip, and the
   assertion accepts "registered" or "skipped for a stated reason" while still
   REJECTING the shape a genuine failure has (`registered: false` with
   `disposition: register`). The Linux artifact assertion moves to the `.desktop`
   entry and additionally requires the stale systemd user unit to be GONE — two
   mechanisms would start two agents at login.

3. `uninstall_engine_service` treated "the end state is clean" as success even when
   EVERY scope's attempt failed, which turned a missing launcher binary into a
   reported-complete uninstall (caught by
   `uninstall_dig_node_surfaces_a_missing_binary_without_panicking` in the root-gate
   job). If no scope could be deregistered we never removed anything and cannot ask
   either, so the state is UNKNOWN — the same stance `lib.rs` already takes for a
   launcher-gone uninstall. A partial failure with a clean end state stays tolerated
   and reported.

The e2e also now asserts the #526 scope contract from both sides: this leg passes
`--bin-dir`, so unix must report user scope with `survives_reboot: false` (a run that
silently registered machine-wide from a caller-chosen root FAILS), and Windows must
report machine-wide with `survives_reboot: true`.

Refs #526 #919
Co-Authored-By: Claude <noreply@anthropic.com>
…ack really did

Two real defects the installer-e2e caught (run 30645063625), both mine:

1. `systemctl is-active <unit>` prints `inactive` for a unit that DOES NOT EXIST,
   and `parse_systemctl_is_active` maps `inactive` to `Stopped` — documented as "a
   real registration exists there, just not running". So the scope probe could not
   answer "is anything registered here?" at all. It over-reported presence in both
   directions that matter: it would have TOLERATED a failed install against a unit
   that was never created (the exact false-ready #526 closes), and it reported a
   successfully completed uninstall as residual, which is how the e2e failed.

   Replaced with `svc::registration_in_scope -> Presence {Present, Absent,
   Unknown}`. Linux asks `is-enabled`, whose vocabulary distinguishes an existing
   unit (`enabled`/`disabled`/`static`/`masked`/…, all registrations) from a missing
   one ("No such file or directory"). `Unknown` is neither: on uninstall it is
   reported as unverified rather than counted as removed, and on install-failure
   tolerance it is not tolerance.

2. The compat fallback assumed every refused `--scope` is a downgrade, so a Windows
   install pinned to an older dig-node was told its service "only starts once
   someone LOGS IN" — false. The Windows SCM has no per-user domain and `install`
   sets `start= auto` there. `svcscope::legacy_default_scope(os)` now names what a
   pre-`--scope` build actually does (System on Windows, User on unix), and
   `reboot_survival` plus the wording follow it. The fallback is still disclosed on
   every OS; only the login warning is now confined to where it is true.

Refs #526
Co-Authored-By: Claude <noreply@anthropic.com>
… one

Co-Authored-By: Claude <noreply@anthropic.com>

@MichaelTaylor3d MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGES-REQUIRED - reviewed at head df070b0e497c02eaa0530145867c6c2f870ef612

All 29 check runs exist and are SUCCESS at this head; zero review threads were open before this review. The scope model in src/svcscope.rs is genuinely good work - pure, OS-parameterised, and its fixtures do discriminate the fix from the nearest wrong implementation. The #1863 re-arm extraction is correct and behaviour-preserving. Five things block merge.

1. BLOCKS - Closes #526 is not earned: dig-node ships no --scope flag, and DIG-Network/dig-node#123 is still OPEN

A grep for the flag over dig-node origin/main (v0.68.1, the version the superproject tracks) returns nothing, and crates/dig-node-service/src/service.rs:99-101 still reads const PREFERS_USER_LEVEL: bool = true; unconditionally for not(windows). The producer half of this cross-repo argument contract is unmerged (dig-node#123, opened today).

Concrete production sequence on an elevated Linux install, today: engine_scope gives System, so the installer runs dig-node install --scope system, clap rejects it, the compat retry goes unflagged, and dig-node registers a user-level unit as root. #526 is bit-for-bit intact. The diff makes the defect honestly reported; it does not fix it. That is valuable, but it is not Closes.

This also inverts release-first (CLAUDE.md 4.1/1.3b): the consumer is landing before the contract exists. And SPEC.md:1033-1036 states the contract in the present tense ("The value set is byte-identical with dig-node's own --scope <auto|system|user>") - a reimplementer reads that as an existing surface. Either land + release dig-node#123 first and re-point this PR at it, or drop Closes #526 and mark the SPEC paragraph as the contract this installer requires of a future dig-node release.

2. BLOCKS - the positional shadow check runs on 1 of 4 reachable paths, and not on the one that needs it most

remove_shadowing_units has exactly ONE call site (src/lib.rs:3039, the dig-node install-Ok arm). It is not called from:

  • the AdoptPackaged arm (src/lib.rs:3002-3016) - which sets installed = true; survives_reboot = true and returns. Scenario: an Ubuntu host with the apt.dig.net .deb (enabled packaged system unit) and a leftover ~/.config/systemd/user/dignetwork-dig-node.service from an earlier unelevated run. sudo dig-installer adopts, removes no shadow, and at the next login systemd --user starts a second dig-node alongside the packaged one - both binding the node port. That is verbatim the harm src/svcscope.rs:239-262 and src/units.rs:1-17 exist to prevent, and existing is already in scope there with the records needed.
  • the already-up-to-date Skip arm (src/lib.rs:3017-3028) - which also never re-scopes an existing registration. On the most common upgrade shape (host already on the latest dig-node, registered user-scope) the run reports installed: true and leaves #526 in place with an empty scope_note.
  • the dig-relay path (src/lib.rs:2694) - relay gets survives_reboot + scope_note but no shadow removal, though units::existing_units and shadowing_units_to_remove serve it identically.

3. BLOCKS - a D-Bus failure is classified as Absent, so an unverifiable uninstall reports clean

src/svc.rs:303-308 returns Presence::Absent for any output containing no such file. systemctl --user is-enabled <unit> under sudo with no user bus prints Failed to connect to bus: No such file or directory - the most likely error on exactly the host class #526 describes. It lands in the Absent arm.

Consequence in src/service.rs:416-436: the user scope is counted as clean, unverified stays empty, and the uninstall reports "uninstalled from every scope" for a scope it could not ask - while root's own user-scope registration (which a pre-#526 sudo install wrote) may still be present. That is the false-clean this PR's own W7 rationale forbids, and it is the bool-that-cannot-say-"I-could-not-check" shape. Related: query_systemctl_presence (src/svc.rs:329-341) discards o.status entirely, so "empty output + non-zero exit" is indistinguishable from "empty output + success" and also resolves to Absent (src/svc.rs:313). Reserve Absent for the recognised not-found signal on a successful query; everything else is Unknown. There is no test for a bus-failure string in is_enabled_distinguishes_an_existing_unit_from_a_missing_one.

4. BLOCKS (honesty of Closes #1774) - the e2e proves neither ticket's bar

The single install leg passes --bin-dir "$BIN_DIR" and pins DIG_NODE_VERSION: "0.29.0". So on unix the only exercised path is forced user scope against a pre---scope binary, and the new assertions are scope == "user" / survives_reboot == false - satisfied identically by the pre-PR code path. That names an outcome, not the property. No CI run anywhere exercises system scope, /etc/systemd/system, or multi-user.target.wants.

Worse for #526 specifically: installer-e2e.yml:190-198 still runs loginctl enable-linger root + systemctl start user@0.service + XDG_RUNTIME_DIR=/run/user/0. That workaround is the thing #526 was filed to eliminate ("without requiring a CI-only workaround"), and it is still load-bearing because the exercised path is still user-scope-under-root.

For #1774 the bar is a default-path run installing five components with each health-gated by running it, plus a simulated reboot on headless Linux. This leg passes --no-digstore, has no reboot step anywhere in the workflow, and never executes dig-app. The fabricated session (DISPLAY=:99 / SESSIONNAME=Console) is disclosed and is a reasonable way to reach the write path - the file-existence + ownership + no-stale-unit assertions around it are real properties - but registered == true itself asserts the fabrication.

5. BLOCKS (trivial) - the note every unix install prints today is malformed

src/service.rs:204 and :209 contain a literal 14-space run ("so it registered in {} instead"). Since no released dig-node accepts --scope, line 204 is the headline message every unix install emits. Same defect at src/autostart.rs:468 ("with no further step") and in the rollback note in src/lib.rs (~:943).

Follow-up, non-gating (I resolve these threads myself)

  1. svcscope::is_unknown_scope_flag_rejection's --scope-naming gate is vacuous in production: run_capturing (src/service.rs:605-610) embeds the joined args in every error, so the substring is always present and the classification collapses to the phrase list. a_real_registration_failure_never_downgrades_the_scope (src/svcscope.rs:638-655) proves it with an "unexpected argument '--bogus' found" string that production cannot generate. Fail-safety survives (the unflagged retry propagates its own error), so this is a claim/test-vacuity issue, not a live downgrade - but SPEC.md:1042 asserts a guard that does not discriminate.
  2. masked and static map to Present (src/svc.rs:309-312). Both are registrations, so this is right for uninstall verification, but on the install-tolerance side it excuses a failed install against a unit that can never start (masked) or is not boot-enabled (static, no [Install] section).
  3. user_config_homes (src/lib.rs:2601-2613) hardcodes <home>/.config, so a stale unit under a non-default $XDG_CONFIG_HOME is never enumerated - while autostart.rs:399 honours that variable for the unelevated write. The two disagree about where a user's config lives.
  4. The PR body says one test "was amended deliberately". Eleven were deleted: five install_service_*, three uninstall_service_*, all three install_relay_service_*, plus the stub_exit harness. Net count rose 25 to 28, but the public install_service / uninstall_service / install_relay_service entry points now have no test at all - the mock-based replacements exercise the internal generic and never reach GuardedCommand/root_exec_guard. Please state the deletions in the PR body and restore one spawn-path test per entry point.
  5. Doc defects in touched code: the 34-line block at src/lib.rs:3582-3615 documenting the beacon re-arm is now orphaned onto beacon_rearm_advice() while rearm_beacon_after_migration (:3631) has no doc comment; the intra-doc link retract_rearm_claims (:3534) names a function that does not exist (retract_service_rearm_claims); src/autostart.rs:177 has a duplicated doc fragment on one line; Exec= (src/autostart.rs:244) is unquoted, so a --bin-dir path containing a space produces an invalid entry; autostart::systemd_user_unit (the renderer) is now referenced only by tests.

Verified and cleared

  • W4 tolerance tightening is a genuine tightening. The task-#232 case (install is not idempotent, so a re-install over a live registration hard-fails while the registration is fine) is retained as an_install_failure_is_tolerated_when_the_requested_scope_already_has_a_registration (src/service.rs:972-999) and still passes; the amended test's fixture reports a registration in the OTHER scope, which is what makes it discriminate a scope-blind probe. Unknown is not tolerance (:1001-1026).
  • #1863. Guard order in src/rearm.rs:129-146 is PR #49's verbatim (plan-selects, was-deregistered, root-is-protected, register, report); src/beacon.rs is untouched and no beacon test changed; all three services go through one call site (src/lib.rs:3536-3579) that records InstallAction::ServiceRegistered only on applied and retracts the report claim via retract_service_rearm_claims.
  • #919 Linux mechanism. The desktop entry (src/autostart.rs:236-249) has [Desktop Entry], Type=Application, Name, Exec, Hidden=false, under $XDG_CONFIG_HOME/autostart/ - valid and sufficient, no enable step. The stale systemd user unit is removed on install (:468) and on uninstall (:574-578).
  • regaudit::privileged_regs widening is correct hardening, and audit_failures (src/regaudit.rs:228-239) does carry a runnable remediation.
  • --json: no new println! in the diff; every new field goes through the report types.

Does each Closes hold?

ticket earns it? what is missing
#526 NO dig-node has no --scope (PR #123 open, unreleased), so the mechanism the ticket asks for is never reached in production; the CI-only root-linger workaround the ticket names is still in the workflow; no system-scope or reboot evidence on any platform.
#1863 YES Nothing. Guard order preserved, beacon tests unchanged, three call sites with rollback + claim retraction, privileged_regs widened so the defect is reachable on unix.
#919 PARTIALLY The per-user agent half is genuinely fixed (the ticket's real content here). The engine SYSTEM daemon half depends on #526 and is not achieved; the ticket's per-OS install/headless/uninstall evidence is unit tests plus one --bin-dir user-scope e2e leg.
#1774 NO Not a default-path five-component run (--no-digstore), no component health-gated by running (dig-app never executed), no simulated reboot, headless Linux unverified - which the PR body itself states under "Not done".

The #1863 work would merge on its own today. Recommend: split #1863 out, or hold the family until dig-node#123 is released and this PR consumes it, and reduce the Closes set to what the diff actually proves.

Comment thread SPEC.md

**The argument surface.** The installer passes `--scope <system|user>` to the component's
`install`/`start`/`uninstall` verbs, as two tokens. The value set is byte-identical with dig-node's
own `--scope <auto|system|user>`; the installer never passes `auto`, because "whatever the component

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKS (finding 1). This states the contract in the present tense, but dig-node origin/main (v0.68.1) has no --scope flag anywhere and still has PREFERS_USER_LEVEL = true unconditionally on unix. The producer, DIG-Network/dig-node#123, is still OPEN. Either release that first and consume it, or phrase this as the surface this installer REQUIRES of a future dig-node release. As written, a reimplementer builds against a flag that does not exist.

Comment thread src/lib.rs
}

#[test]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKS (finding 2). The AdoptPackaged arm returns without calling remove_shadowing_units, even though existing is right here and holds the user-scope records. Host with the apt.dig.net .deb (enabled packaged system unit) plus a leftover ~/.config/systemd/user/dignetwork-dig-node.service: this reports installed: true, survives_reboot: true and leaves both units, so systemd --user starts a second dig-node at the next login and both bind the node port. Same gap on the already-up-to-date Skip arm below and on the dig-relay path (~:2694).

Comment thread src/svc.rs
let lower = trimmed.to_lowercase();
if lower.contains("no such file")
|| lower.contains("does not exist")
|| lower.contains("not found")

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKS (finding 3). systemctl --user is-enabled <unit> under sudo with no user bus prints "Failed to connect to bus: No such file or directory", which matches this substring and returns Absent. That is the most likely error on exactly the host class #526 describes, and it makes uninstall_engine_service report "uninstalled from every scope" for a scope it could not ask (service.rs:416-436 - unverified stays empty). Also query_systemctl_presence discards the exit status, so empty-output-plus-nonzero-exit lands in Absent too. Reserve Absent for the recognised not-found reply on a SUCCESSFUL query; everything else is Unknown. No test covers a bus-failure string.

Comment thread src/service.rs
let reboot_survival = svcscope::survives_reboot_without_login(os, effective_scope);
if !scope_flag_accepted && effective_scope != scope {
note.push_str(&format!(
" — but this {label} build does not understand `--scope`, so it registered in {} instead, which only starts once someone LOGS IN. The service will NOT come back on its own after a reboot until {label} is updated",

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKS (finding 5, trivial). Literal 14-space run inside the string: "so it registered in {} instead". Because no released dig-node accepts --scope, this is the headline note EVERY unix install prints today. Same at :209, autostart.rs:468, and the rollback note in lib.rs (~:943).

echo "SESSIONNAME=Console" >> "$GITHUB_ENV"
fi

- name: Install (dig-node + dig-dns + the auto-update beacon, pinned versions)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKS (finding 4). This leg passes --bin-dir and pins dig-node 0.29.0, so the only path exercised on unix is forced-user-scope against a pre---scope binary, and the new assertions (scope == "user", survives_reboot == false) are satisfied identically by the pre-PR code. Meanwhile the loginctl enable-linger root + user@0.service steps above are still here - that workaround IS what #526 was filed to remove. There is no system-scope leg, no reboot simulation, --no-digstore, and dig-app is never executed, so neither #526 nor #1774 has its bar met.

Comment thread src/svcscope.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant