feat: Use supervisor when running tedge-agent/tedge-mapper by jarhodes314 · Pull Request #4244 · thin-edge/thin-edge.io

jarhodes314 · 2026-07-08T17:03:03Z

Proposed changes

Use the supervisor for running a single-process tedge-agent/tedge-mapper. This means we get log reloading on SIGHUP.

For these processes, the supervisor doesn't attempt to restart the process on failure unlike in tedge run all, as we assume there is an init system in place to restart the process, and an internal restart will mask issues where the process is in a crash loop. We don't do anything on SIGUSR1 for these processes, unlike tedge run all where we restart the mappers.

When reviewing, you probably want to look at the second commit onwards. The first commit is merely renaming a file (to separate out the supervisor into its own crate), but github doesn't show it as that.

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s. You can activate automatic signing by running just prepare-dev once)
I ran just format as mentioned in CODING_GUIDELINES
I used just check as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

github-actions · 2026-07-08T17:53:19Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
953	0	7	953	100	2h49m47.753702999s

Bravo555 · 2026-07-09T11:01:07Z

@reubenmiller To me it seems to be a behaviour/feature change, not a refactoring (which ideally shouldn't change behaviour)

reubenmiller · 2026-07-09T11:02:14Z

@reubenmiller To me it seems to be a behaviour/feature change, not a refactoring (which ideally shouldn't change behaviour)

yeah you're write, I just couldn't find a better label, so maybe we're missing something.

Update: I've created a new "theme:runtime" label

Bravo555

I see there are some unit tests in the tedge_supervisor crate, to make sure its API works as expected, but I'd also like to see system tests to make sure that tedge-mapper and tedge-agent actually use the supervisor correctly and exhibit the new behaviour we expect, because if I understand correctly, currently we don't seem to be checking that.

Bravo555

All the changes seem correct, nothing to complain about, but I do believe some system tests would be necessary here.

And just some thoughts about specs: having scenarios and requirements in spec.mds would be really, really nice if there was some neat way to reference the specs in any test (unit or system test) and then you could find backlinks to that requirement so you could easily see from the spec document itself the tests that cover its requirements.

Bravo555 · 2026-07-09T11:44:49Z

+
+- SIGUSR1 mapper restart for standalone processes — this signal is meaningful only when the agent and mapper share a process, where it allows restarting the mapper without restarting the agent. A standalone mapper has nothing to coordinate with.
+- Changing the `build()` factory contract — standalone and `tedge run all` paths continue to share the same factory
+- Replacing systemd restart semantics — the supervisor's crash recovery complements systemd, it does not replace it


thought: Yeah, I was curious about that - I see in the last section that when we run tedge-mapper/tedge-agent using the supervisor and as a systemd unit, the service can restart up to supervisor × systemd times, because systemd doesn't know about restarting the supervisor does, but it's ok because supervisor restarting is faster and loses less state.

Though now there's probably no reason to, we could always make them both aware of each other. The supervisor could check INVOCATION_ID, which is set by systemd when running as a unit and disable restarts if we wish to add that type of configuration, and similarly we could also potentially support systemctl reload command since we now handle SIGHUP to reload log levels. But that's outside the scope of this PR and for future consideration.

Bravo555 · 2026-07-09T11:52:35Z

 [[package]]
 name = "quinn-udp"
-version = "0.5.12"
+version = "0.5.14"


nit: seems unrelated

jarhodes314 · 2026-07-09T12:30:21Z

I see there are some unit tests in the tedge_supervisor crate, to make sure its API works as expected, but I'd also like to see system tests to make sure that tedge-mapper and tedge-agent actually use the supervisor correctly and exhibit the new behaviour we expect, because if I understand correctly, currently we don't seem to be checking that.

Added in 39aa1f1

codecov · 2026-07-09T12:47:48Z

Codecov Report

❌ Patch coverage is 87.69953% with 131 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
crates/common/tedge_supervisor/src/lib.rs	91.56%	74 Missing and 9 partials ⚠️
crates/core/tedge_agent/src/lib.rs	0.00%	21 Missing ⚠️
crates/core/tedge_mapper/src/lib.rs	0.00%	15 Missing ⚠️
...s/common/tedge_config/src/system_toml/log_level.rs	81.25%	0 Missing and 6 partials ⚠️
crates/core/tedge/src/supervisor.rs	50.00%	5 Missing ⚠️
plugins/tedge_file_config_plugin/src/lib.rs	66.66%	1 Missing ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Move the supervisor implementation from crates/core/tedge/src/supervisor.rs to crates/common/tedge_supervisor/src/lib.rs so it can be reused by tedge-agent and tedge-mapper as standalone processes. Signed-off-by: James Rhodes <jarhodes314@gmail.com>

Extract the supervisor into its own tedge_supervisor crate and add support for running tedge-agent and tedge-mapper as standalone supervised processes, in addition to the existing multi-unit mode. Signed-off-by: James Rhodes <jarhodes314@gmail.com>

Bravo555

I see we also changed the crash of standalone components (i.e. only mapper or only agent) to not recover using supervisor but proceed to crash and be restarted by the init system; that is fine.
Tests were added, so approving.

rina23q

Approved, the system tests are convincing.

albinsuresh

Solid work. Well documented with extensive test coverage as well.

I just noticed one bug: The mapper mapper component is getting restarted by the systemctl kill --signal=SIGUSR1 tedge-mapper-c8y command, despite the specs hinting otherwise.

albinsuresh · 2026-07-13T06:22:21Z

+A supervised component that requires a restart (after a self-update, or an update
+of its own configuration) SHALL cause the supervisor to drain every component and
+exit the process with a non-zero code, since only re-executing the binary makes
+such an update effective.


I'm assuming we were forced to handle action = "restart-agent" this way due to the current contract of the await-agent-restart. For a self-update, this makes sense as the entire tedge binary is updated. But for a config_update that only affects the tedge-agent, this isn't ideal. We might have to rethink this in future where an agent restart can be handled as just the actor restart without affecting all other mappers.

albinsuresh · 2026-07-13T08:23:42Z

    # mapper fails fast on the lock, so this does not block.
    Execute Command    tedge-mapper c8y    exp_exit_code=!0

+A config update restarting the agent restarts the whole process


We'll have to rethink the config_update workflow itself to not expect a service restart but only a reload once the single process mode becomes the norm and tedge.toml updates are also handled by it.

albinsuresh · 2026-07-13T11:48:28Z

+    Service Log Should Not Contain    tedge-agent    EntityStoreServer: recv    date_from=${after}
+    [Teardown]    Remove Log Override
+
+


How about an additional test that validates that SIGUSR1 signal has no effect in the standalone mode?

albinsuresh · 2026-07-13T11:53:12Z

+
+- **WHEN** a standalone process is started with `--log-level`, `--debug`, or `RUST_LOG`
+- **AND** the process receives SIGHUP
+- **THEN** the signal is logged as ignored and log levels remain unchanged


I understand why the current impl gives precedence to the cli overrides over the value defined in the system.toml. But from a user's pov, it feels counterintuitive. Shouldn't a runtime update get precedence over settings that the process was started with? Since a runtime action implies explicit user intent to override the existing settings, irrespective of what was specified on startup? Otherwise there's no way to override the values passed in the cli, right?

jarhodes314 requested review from albinsuresh, didier-wenzek, reubenmiller and rina23q as code owners July 8, 2026 17:03

jarhodes314 requested a deployment to Test Pull Request July 8, 2026 17:03 — with GitHub Actions Waiting

jarhodes314 force-pushed the feat/standalone-supervisor branch from 1937e68 to b3cf23b Compare July 8, 2026 17:09

jarhodes314 temporarily deployed to Test Pull Request July 8, 2026 17:09 — with GitHub Actions Inactive

jarhodes314 force-pushed the feat/standalone-supervisor branch from b3cf23b to e865a13 Compare July 8, 2026 17:16

jarhodes314 requested a deployment to Test Pull Request July 8, 2026 17:17 — with GitHub Actions Waiting

Bravo555 self-requested a review July 9, 2026 08:57

Bravo555 self-assigned this Jul 9, 2026

reubenmiller added the refactoring Developer value label Jul 9, 2026

reubenmiller added theme:runtime General features regarding either the actor runtime, how processes are started etc. and removed refactoring Developer value labels Jul 9, 2026

Bravo555 reviewed Jul 9, 2026

View reviewed changes

Bravo555 removed their assignment Jul 9, 2026

jarhodes314 requested a review from a team as a code owner July 9, 2026 12:28

jarhodes314 requested a deployment to Test Pull Request July 9, 2026 12:28 — with GitHub Actions Waiting

jarhodes314 requested a deployment to Test Pull Request July 9, 2026 12:58 — with GitHub Actions Waiting

jarhodes314 had a problem deploying to Test Pull Request July 9, 2026 15:23 — with GitHub Actions Error

jarhodes314 temporarily deployed to Test Pull Request July 9, 2026 15:25 — with GitHub Actions Inactive

jarhodes314 temporarily deployed to Test Auto July 10, 2026 09:03 — with GitHub Actions Inactive

jarhodes314 added 2 commits July 10, 2026 11:10

jarhodes314 force-pushed the feat/standalone-supervisor branch from af36fa5 to 71c80e0 Compare July 10, 2026 10:10

jarhodes314 temporarily deployed to Test Pull Request July 10, 2026 10:11 — with GitHub Actions Inactive

jarhodes314 temporarily deployed to Test Auto July 10, 2026 10:20 — with GitHub Actions Inactive

Bravo555 approved these changes Jul 10, 2026

View reviewed changes

rina23q approved these changes Jul 10, 2026

View reviewed changes

jarhodes314 added this pull request to the merge queue Jul 13, 2026

Merged via the queue into thin-edge:main with commit 221053b Jul 13, 2026
35 checks passed

albinsuresh reviewed Jul 13, 2026

View reviewed changes

		Service Log Should Not Contain tedge-agent EntityStoreServer: recv date_from=${after}
		[Teardown] Remove Log Override

Uh oh!

Conversation

jarhodes314 commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

github-actions Bot commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

Bravo555 commented Jul 9, 2026

Uh oh!

reubenmiller commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Bravo555 left a comment

Choose a reason for hiding this comment

Uh oh!

Bravo555 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Bravo555 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Bravo555 Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

jarhodes314 commented Jul 9, 2026

Uh oh!

codecov Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Bravo555 left a comment

Choose a reason for hiding this comment

Uh oh!

rina23q left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

albinsuresh left a comment

Choose a reason for hiding this comment

Uh oh!

albinsuresh Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

albinsuresh Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

albinsuresh Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

albinsuresh Jul 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jarhodes314 commented Jul 8, 2026 •

edited

Loading

github-actions Bot commented Jul 8, 2026 •

edited

Loading

reubenmiller commented Jul 9, 2026 •

edited

Loading

codecov Bot commented Jul 9, 2026 •

edited

Loading