fix(heating-can): stop the native services before removing their units #477

2026-08-10T19:46:05Z

claude-bot commented

2026-08-10 19:46:05 +00:00

The migration in #476 did not stop anything.

It ran state: stopped + enabled: false + masked: true in one call with failed_when: false, then deleted the unit files. Masking a unit whose file still exists conflicts, so the call failed, failed_when hid it, nothing was stopped — and the next task deleted the unit files out from under two running processes.

That left systemctl is-enabled reporting not-found, which looks clean, while is-active still said active. I reported the cutover successful on the strength of the former.

What it caused

Two open3e instances issuing UDS requests on the same CAN bus, 19:28 → 19:37 on 2026-08-10, taking over each other's open3e-edge MQTT session:

Client open3e-edge [127.0.0.1:53831] disconnected: session taken over.
New client connected from 127.0.0.1:37575 as open3e-edge
Client open3e-edge [127.0.0.1:37575] disconnected: session taken over.

That is the exact failure the block's own comment said it existed to prevent.

Fix

Stop and disable first, and don't swallow the result. Only units that actually exist are touched (stat first), so a fresh host doesn't need an error hidden to get through.
No masking. It writes a symlink at the same path the next task removes, so the two cancel out — half of how the original failed. With the unit files gone there's nothing to start.
Assert the invariant that actually matters — no native process on the CAN bus — instead of inferring it from systemd reporting success. pgrep, then fail loudly with the surviving command lines and why it matters.

The last point is the real lesson: every check I had confirmed a proxy for the thing I cared about, and the proxy said what I expected while the bus had two readers on it.

Verification

Re-run against the live host after the fix:

edge-1.iot.renner.uno : ok=15 changed=1 unreachable=0 failed=0

with the assert passing — "No native CAN processes remain; safe to start the container" — and the run now idempotent: the single change is the compose up, not the teardown.

🤖 Generated with Claude Code

The migration in #476 did not stop anything. It ran `state: stopped` + `enabled: false` + `masked: true` in **one call with `failed_when: false`**, then deleted the unit files. Masking a unit whose file still exists conflicts, so the call failed, `failed_when` hid it, nothing was stopped — and the next task deleted the unit files out from under two running processes. That left `systemctl is-enabled` reporting `not-found`, which looks clean, while `is-active` still said `active`. I reported the cutover successful on the strength of the former. ## What it caused Two open3e instances issuing UDS requests on the same CAN bus, 19:28 → 19:37 on 2026-08-10, taking over each other's `open3e-edge` MQTT session: ``` Client open3e-edge [127.0.0.1:53831] disconnected: session taken over. New client connected from 127.0.0.1:37575 as open3e-edge Client open3e-edge [127.0.0.1:37575] disconnected: session taken over. ``` That is the exact failure the block's own comment said it existed to prevent. ## Fix - **Stop and disable first, and don't swallow the result.** Only units that actually exist are touched (`stat` first), so a fresh host doesn't need an error hidden to get through. - **No masking.** It writes a symlink at the same path the next task removes, so the two cancel out — half of how the original failed. With the unit files gone there's nothing to start. - **Assert the invariant that actually matters** — no native process on the CAN bus — instead of inferring it from systemd reporting success. `pgrep`, then fail loudly with the surviving command lines and why it matters. The last point is the real lesson: every check I had confirmed a *proxy* for the thing I cared about, and the proxy said what I expected while the bus had two readers on it. ## Verification Re-run against the live host after the fix: ``` edge-1.iot.renner.uno : ok=15 changed=1 unreachable=0 failed=0 ``` with the assert passing — *"No native CAN processes remain; safe to start the container"* — and the run now idempotent: the single change is the compose up, not the teardown. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

claude-bot added 1 commit 2026-08-10 19:46:06 +00:00

fix(heating-can): stop the native services before removing their units

ci/woodpecker/pr/pr-review Pipeline was successful

Details

ci/woodpecker/pr/ci-scripts-test Pipeline was successful

Details

1b7e3dc100

The migration did not stop anything. It ran stop + disable + mask in one
call with failed_when: false, then deleted the unit files. Masking a unit
whose file still exists conflicts, so the call failed, failed_when hid the
failure, nothing was stopped -- and the next task deleted the unit files out
from under two running processes.

That left systemctl is-enabled reporting "not-found", which looks clean,
while is-active still said "active". I reported the cutover successful on
the strength of the former.

On 2026-08-10 the result was two open3e instances issuing UDS requests on
the same CAN bus from 19:28 to 19:37, taking over each other's
open3e-edge MQTT session -- the exact failure the block's own comment said
it existed to prevent.

Fixed in three parts:

  - Stop and disable FIRST, and do not swallow the result. Only units that
    actually exist are touched (stat first), so a fresh host does not need
    an error hidden to get through.

  - No masking. It writes a symlink at the same path the next task removes,
    so the two cancel out. With the unit files gone there is nothing to
    start.

  - Assert the invariant that actually matters -- no native process on the
    CAN bus -- rather than inferring it from systemd reporting success.
    pgrep, then fail loudly with the surviving command lines and why it
    matters.

Verified against the live host: ok=15 changed=1 failed=0, the assert passing
with "No native CAN processes remain; safe to start the container", and the
run now idempotent (the single change is the compose up, not the teardown).

Co-Authored-By: Claude Bot <claude-bot@renner.uno>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

k.renner merged commit 6384223415 into master

2026-08-10 19:48:35 +00:00

k.renner referenced this issue from a commit

2026-08-10 19:48:36 +00:00

Merge pull request 'fix(heating-can): stop the native services before removing their units' (#477) from fix-native-teardown into master

claude-bot referenced this pull request

2026-08-10 19:49:22 +00:00

fix(heating-can): give supervisord a control socket #478

review-bot reviewed

2026-08-10 19:50:07 +00:00

review-bot left a comment

🤖 Automated review — model claude-sonnet-5 (complexity: normal), commit 1b7e3dc1000a1aa9525aec3f3e03489e45935bc0.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

This PR fixes the native-service teardown in ansible/roles/heating-can/tasks/main.yml, replacing a single systemd call that combined stop + disable + mask under failed_when: false (which silently swallowed a real failure and left both native processes running while their unit files were deleted) with: an existence check via stat, a stop/disable step that only touches units that exist and lets failures surface, removal of masking, and a pgrep-based assertion that no native open3e/e3oncan process survives before the container starts. The new design directly targets the root cause described in the PR body (asserting the actual invariant — no reader left on the bus — instead of inferring it from systemd's reported success), and the logic checks out against the rest of the file. No blocking issues found.

🟡 Suggestions

ansible/roles/heating-can/tasks/main.yml:34 — The pre-existing top comment ("the old units are stopped and masked rather than merely disabled -- masked so a stray systemctl start... cannot bring them back") was left untouched by this diff and now contradicts the new design: masking was deliberately removed (see the new comment at line 70, "Masking is deliberately NOT used"). Since unit-file removal now serves the same "can't be started" purpose, this stale comment should be updated in the same spirit as the rest of the PR (which is otherwise very careful about comments matching actual behavior) — otherwise a future reader will believe masking is still happening.
ansible/roles/heating-can/tasks/main.yml:86-91 — Check for surviving native CAN processes relies on pgrep's exit/stdout via failed_when: false. If the pgrep binary itself were missing on a host, the command module fails before running anything and registers no stdout/rc; the following assert (heating_can_survivors.stdout | default('') | length == 0) would then default to '' and silently report "No native CAN processes remain" without ever having checked. This is a low-probability edge case (procps/pgrep is present on essentially every Debian-family host this repo targets) but worth a thought, since the whole point of this PR is not to let a proxy check quietly stand in for the real invariant. Not blocking given how unlikely a missing pgrep is here.
ansible/roles/heating-can/tasks/main.yml:88 — The pgrep -af pattern edge-host/(open3e|e3oncan) assumes the native processes' command lines contain their venv path under {{ edge_base_dir }} (/opt/edge-host, confirmed in defaults/main.yml:7), which is a reasonable inference from the venv paths this same file removes a few lines below ({{ edge_base_dir }}/open3e/venv, etc.). The actual ExecStart line of the old native units isn't present anywhere in this checkout (they were removed by the prior migration and never templated in this repo), so this couldn't be verified directly against the unit definition — it's corroborated only by the PR body's report of a successful live re-run. Worth keeping in mind if a future host's native units were ever set up with a different ExecStart convention.

Tests

No automated tests exist for this role (no molecule/ansible-lint gate was found wired into .woodpecker/), which matches the rest of this repo's ansible roles — verification here is inherently a live-host concern. The PR body documents a live re-run against the affected host (ok=15 changed=1 ... failed=0) with the assert passing, which is reasonable evidence for an infrastructure change like this. Nothing further to add — a unit/molecule test isn't a realistic bar for this kind of systemd/process teardown logic.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `claude-sonnet-5` (complexity: normal), commit `1b7e3dc1000a1aa9525aec3f3e03489e45935bc0`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary This PR fixes the native-service teardown in `ansible/roles/heating-can/tasks/main.yml`, replacing a single `systemd` call that combined `stop` + `disable` + `mask` under `failed_when: false` (which silently swallowed a real failure and left both native processes running while their unit files were deleted) with: an existence check via `stat`, a stop/disable step that only touches units that exist and lets failures surface, removal of masking, and a `pgrep`-based assertion that no native `open3e`/`e3oncan` process survives before the container starts. The new design directly targets the root cause described in the PR body (asserting the actual invariant — no reader left on the bus — instead of inferring it from systemd's reported success), and the logic checks out against the rest of the file. No blocking issues found. ## 🟡 Suggestions - `ansible/roles/heating-can/tasks/main.yml:34` — The pre-existing top comment ("the old units are stopped and masked rather than merely disabled -- masked so a stray `systemctl start`... cannot bring them back") was left untouched by this diff and now contradicts the new design: masking was deliberately removed (see the new comment at line 70, "Masking is deliberately NOT used"). Since unit-file removal now serves the same "can't be started" purpose, this stale comment should be updated in the same spirit as the rest of the PR (which is otherwise very careful about comments matching actual behavior) — otherwise a future reader will believe masking is still happening. - `ansible/roles/heating-can/tasks/main.yml:86-91` — `Check for surviving native CAN processes` relies on `pgrep`'s exit/stdout via `failed_when: false`. If the `pgrep` binary itself were missing on a host, the `command` module fails before running anything and registers no `stdout`/`rc`; the following assert (`heating_can_survivors.stdout | default('') | length == 0`) would then default to `''` and silently report "No native CAN processes remain" without ever having checked. This is a low-probability edge case (procps/`pgrep` is present on essentially every Debian-family host this repo targets) but worth a thought, since the whole point of this PR is not to let a proxy check quietly stand in for the real invariant. Not blocking given how unlikely a missing `pgrep` is here. - `ansible/roles/heating-can/tasks/main.yml:88` — The `pgrep -af` pattern `edge-host/(open3e|e3oncan)` assumes the native processes' command lines contain their venv path under `{{ edge_base_dir }}` (`/opt/edge-host`, confirmed in `defaults/main.yml:7`), which is a reasonable inference from the venv paths this same file removes a few lines below (`{{ edge_base_dir }}/open3e/venv`, etc.). The actual `ExecStart` line of the old native units isn't present anywhere in this checkout (they were removed by the prior migration and never templated in this repo), so this couldn't be verified directly against the unit definition — it's corroborated only by the PR body's report of a successful live re-run. Worth keeping in mind if a future host's native units were ever set up with a different `ExecStart` convention. ## Tests No automated tests exist for this role (no molecule/ansible-lint gate was found wired into `.woodpecker/`), which matches the rest of this repo's ansible roles — verification here is inherently a live-host concern. The PR body documents a live re-run against the affected host (`ok=15 changed=1 ... failed=0`) with the assert passing, which is reasonable evidence for an infrastructure change like this. Nothing further to add — a unit/molecule test isn't a realistic bar for this kind of systemd/process teardown logic. ## Recommendation Looks reasonable — no blocking issues; comments above are optional.

Sign in to join this conversation.

2 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: konairius/homelab#477