fix: avoid reading external plugin stdout to memory by Bravo555 · Pull Request #4234 · thin-edge/thin-edge.io

Bravo555 · 2026-06-30T18:13:55Z

Proposed changes

Similar to 5fda661, stream result of external config plugin instead of loading everything to memory. Not as impactful as log plugin, because usually people won't be having config files bigger than available RAM, but nevertheless behaviour was changed to keep consistency between the two plugins.

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

#4222 (review)

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

codecov · 2026-06-30T18:25:19Z

Codecov Report

❌ Patch coverage is 42.04545% with 51 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...ates/extensions/tedge_config_manager/src/plugin.rs	38.18%	25 Missing and 9 partials ⚠️
.../core/tedge_api/src/workflow/log/logged_command.rs	63.63%	1 Missing and 7 partials ⚠️
...tes/core/tedge_api/src/workflow/log/command_log.rs	33.33%	4 Missing ⚠️
.../core/tedge_agent/src/operation_workflows/actor.rs	0.00%	3 Missing ⚠️
crates/extensions/tedge_log_manager/src/plugin.rs	0.00%	2 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.

github-actions · 2026-06-30T18:53:19Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
949	0	7	949	100	2h56m27.408601s

albinsuresh

LGTM. Just a few minor remarks/queries.

albinsuresh · 2026-07-01T06:10:43Z

+            tempfile::NamedTempFile::new_in(self.tmp_dir.as_std_path()).map_err(|err| {
+                self.plugin_error(format!(
+                    "Failed to create temporary file in {}: {err}",
+                    tempfile::env::temp_dir().to_string_lossy()


Suggested change

tempfile::env::temp_dir().to_string_lossy()

self.tmp_dir.as_str()

I see that it was inherited from crates/extensions/tedge_log_manager/src/plugin.rs in 5fda661.

fixed in b4c1681

albinsuresh · 2026-07-01T07:00:48Z

+        let child = command.spawn().map_err(|err| self.plugin_error(err))?;
+        let output = child
+            .wait_with_output(command_log)


Wondering why you took this "spawn child wait for output" approach here as opposed to the command.status() approach taken in crates/extensions/tedge_log_manager/src/plugin.rs. The command.status() approach felt more straightforward, especially about the intent of not caring about the stdout or stderr. I understand that the output.stdout/stderr would be None in this case as well, as they are redirected to the files. But, its' not as direct as the other.

Since you only wrote the simpler status based version for the log_manager/plugin.rs, there must have been a specific reason why you took a different approach here. Just trying to understand what that is.

Okay, the problem is that log plugin's get operation isn't logged, but config plugin's get operation is logged.
To verify that config plugin's get operation keeps being logged, I added a check to the config plugin's test in 782d86b.
The spawn + wait_with_output method here was chosen to pass the command_log to log stdout to the operation log, however this just doesn't work because stdout is routed to file, not pipe.
So to avoid reading all of stdout to memory and also log stdout to the operation log, we'll have to do something like this:

create tempfile for stdout

route process stdout to tempfile

call .status() to wait for process to exit

open both stdout and stderr tempfiles and copy their contents to operation logfile

persist operation logfile

then open and read stdout file again for upload

We write the file, then read it twice, keeping all its contents in a logfile, but we don't load all stdout into memory, so it's okay. I will implement these changes.

rina23q

LGTM

albinsuresh

Approving again with some minor suggestions.

albinsuresh · 2026-07-02T11:37:19Z

 }
+
+/// A command output that allows reading from it, no matter if output was captured to memory or attached to a file.
+pub struct CommandOutput<'a> {


That's a nice abstraction.

albinsuresh · 2026-07-02T11:38:33Z

-                    logger.write_all(b"EOF\n").await?;
+                let mut stdout = output.stdout;
+                if !stdout.fill_buf().await?.is_empty() {
+                    logger.write_all(b"stderr <<EOF\n").await?;


Suggested change

logger.write_all(b"stderr <<EOF\n").await?;

logger.write_all(b"stdout <<EOF\n").await?;

I think both this and below comments are referring to old commits. These don't exist now and also when I reviewed it.

albinsuresh · 2026-07-02T11:38:40Z

+                    logger.write_all(b"EOF\n\n").await?;
                } else {
-                    logger.write_all(b"stdout (EMPTY)\n").await?;
+                    logger.write_all(b"stderr (EMPTY)\n\n").await?;


Suggested change

logger.write_all(b"stderr (EMPTY)\n\n").await?;

logger.write_all(b"stdout (EMPTY)\n\n").await?;

Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>

Similar to 5fda661, stream result of external config plugin instead of loading everything to memory. Not as impactful as log plugin, because usually people won't be having config files bigger than available RAM, but nevertheless behaviour was changed to keep consistency between the two plugins. Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>

Bravo555 requested review from a team, albinsuresh, didier-wenzek, jarhodes314 and rina23q as code owners June 30, 2026 18:13

Bravo555 added the theme:configuration Theme: Configuration management label Jun 30, 2026

Bravo555 temporarily deployed to Test Pull Request June 30, 2026 18:14 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto June 30, 2026 18:24 — with GitHub Actions Inactive

albinsuresh approved these changes Jul 1, 2026

View reviewed changes

Bravo555 requested a deployment to Test Pull Request July 1, 2026 13:00 — with GitHub Actions Waiting

Bravo555 temporarily deployed to Test Pull Request July 1, 2026 17:55 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto July 1, 2026 18:05 — with GitHub Actions Inactive

Bravo555 force-pushed the improve/config-plugin-get-stream branch from d28787a to 8053ad6 Compare July 2, 2026 08:38

Bravo555 temporarily deployed to Test Pull Request July 2, 2026 08:38 — with GitHub Actions Inactive

rina23q approved these changes Jul 2, 2026

View reviewed changes

albinsuresh approved these changes Jul 2, 2026

View reviewed changes

Bravo555 added 3 commits July 2, 2026 12:55

fix invalid tempfile err message in log plugin

d456d45

Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>

tests: Check if config plugin stdout is logged

b05d563

Signed-off-by: Marcel Guzik <marcel.guzik@cumulocity.com>

Bravo555 force-pushed the improve/config-plugin-get-stream branch from 8053ad6 to adba130 Compare July 2, 2026 12:55

Bravo555 temporarily deployed to Test Pull Request July 2, 2026 12:55 — with GitHub Actions Inactive

Bravo555 temporarily deployed to Test Auto July 2, 2026 13:15 — with GitHub Actions Inactive

Bravo555 added this pull request to the merge queue Jul 2, 2026

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 2, 2026

Bravo555 added this pull request to the merge queue Jul 2, 2026

Merged via the queue into thin-edge:main with commit 28a6fdf Jul 2, 2026
34 checks passed

	tempfile::env::temp_dir().to_string_lossy()
	self.tmp_dir.as_str()

	logger.write_all(b"stderr <<EOF\n").await?;
	logger.write_all(b"stdout <<EOF\n").await?;

	logger.write_all(b"stderr (EMPTY)\n\n").await?;
	logger.write_all(b"stdout (EMPTY)\n\n").await?;

Uh oh!

Conversation

Bravo555 commented Jun 30, 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!

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

Uh oh!

Codecov Report

Uh oh!

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

Uh oh!

Robot Results

Uh oh!

albinsuresh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rina23q left a comment

Choose a reason for hiding this comment

Uh oh!

albinsuresh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Bravo555 commented Jun 30, 2026 •

edited

Loading

codecov Bot commented Jun 30, 2026 •

edited

Loading

github-actions Bot commented Jun 30, 2026 •

edited

Loading