Fix resource leak in Daemon.run()

[wangp-h]

Fix resource leak in Daemon.run() when subprocess.Popen() fails

The Daemon.run() method opens file objects for stdin and stdout but
doesn't close them when subprocess.Popen() raises an OSError. This
causes file descriptor leaks, especially problematic when commands
are misconfigured or don't exist.

Add a finally block to ensure file objects are always closed,
regardless of whether Popen() succeeds or fails.

Summary by CodeRabbit

• Bug Fixes
  • Enhanced resource cleanup in daemon process to ensure stdin and stdout are properly closed in all scenarios, preventing resource leaks and improving system reliability.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

The change adds a finally block to the Daemon.run method in avocado/utils/sysinfo.py to ensure stdin and stdout file descriptors are closed after attempting to start a subprocess, guaranteeing resource cleanup regardless of whether the operation succeeds or raises an error. No public API signatures are modified.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

• Verify that stdin and stdout resources are reliably opened before the finally block executes, to prevent potential AttributeError or IOError during cleanup
• Confirm the finally block doesn't mask or suppress any exceptions that should be surfaced to callers
• Check for edge cases where the subprocess start may fail before resources are fully initialized

Pre-merge checks

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Fix resource leak in Daemon.run()' directly and specifically addresses the main change: adding a finally block to close stdin and stdout resources in the Daemon.run() method to prevent resource leaks.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

avocado/utils/sysinfo.py (1)

236-255: Leak fix in Daemon.run() looks good; consider tightening resource handling around open() and close()

The new finally block correctly ensures stdin and stdout are closed even when subprocess.Popen() raises OSError, which fixes the reported FD leak.

Two optional refinements you might consider while you’re here:

• Handle open() failure without leaking: as written, if stdout = open(logf_path, "w") fails after stdin is opened, stdin won’t be closed because the exception is raised before the try/finally. A with block (or moving the open() calls inside the try) would cover that path as well.
• Be resilient to close() errors: if stdin.close() or stdout.close() ever raises OSError, it will now propagate out of run() and potentially mask the original OSError from Popen. Other parts of the codebase (for example, avocado/core/output.py pipe closing) ignore OSError on close, so you may want to wrap the closes in their own try/except OSError: pass for consistency.

For example, you could simplify and harden this section like:

-        stdin = open(os.devnull, "r")  # pylint: disable=W1514, R1732
-        stdout = open(logf_path, "w")  # pylint: disable=W1514, R1732
-
-        try:
-            # pylint: disable=R1732
-            self.daemon_process = subprocess.Popen(
-                shlex.split(self.cmd),
-                stdin=stdin,
-                stdout=stdout,
-                stderr=subprocess.STDOUT,
-                shell=False,
-                env=env,
-            )
-        except OSError as os_err:
-            raise CollectibleException(
-                f'Could not execute "{self.cmd}": ' f"{os_err}"
-            ) from os_err
-        finally:
-            stdin.close()
-            stdout.close()
+        try:
+            with open(os.devnull, "r") as stdin, open(logf_path, "w") as stdout:  # pylint: disable=W1514
+                # pylint: disable=R1732
+                self.daemon_process = subprocess.Popen(
+                    shlex.split(self.cmd),
+                    stdin=stdin,
+                    stdout=stdout,
+                    stderr=subprocess.STDOUT,
+                    shell=False,
+                    env=env,
+                )
+        except OSError as os_err:
+            raise CollectibleException(
+                f'Could not execute "{self.cmd}": ' f"{os_err}"
+            ) from os_err

This keeps the new behavior (no leaked FDs when Popen fails), also closes stdin if stdout fails to open, and lets the context manager handle closes cleanly.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 74b7379 and 414b03c.

📒 Files selected for processing (1)

• avocado/utils/sysinfo.py (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

avocado/utils/sysinfo.py (2)

avocado/utils/iso9660.py (5)

• close (171-176)
• close (200-206)
• close (298-300)
• close (349-360)
• close (454-460)

avocado/core/output.py (3)

• close (277-278)
• close (386-395)
• close (644-649)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (42)

• GitHub Check: rpm-build:centos-stream-9-x86_64
• GitHub Check: rpm-build:fedora-41-x86_64
• GitHub Check: rpm-build:fedora-41-s390x
• GitHub Check: rpm-build:fedora-rawhide-x86_64
• GitHub Check: rpm-build:fedora-41-aarch64
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: rpm-build:fedora-43-x86_64
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:fedora-41-ppc64le
• GitHub Check: rpm-build:fedora-43-x86_64
• GitHub Check: rpm-build:fedora-41-x86_64
• GitHub Check: rpm-build:fedora-41-aarch64
• GitHub Check: rpm-build:fedora-rawhide-x86_64
• GitHub Check: rpm-build:centos-stream-9-x86_64
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:fedora-41-s390x
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: rpm-build:fedora-41-ppc64le
• GitHub Check: Linux with Python 3.12
• GitHub Check: Linux with Python 3.13
• GitHub Check: Linux with Python 3.9
• GitHub Check: Linux with Python 3.10
• GitHub Check: Linux with Python 3.11
• GitHub Check: rpm-build:fedora-43-x86_64
• GitHub Check: rpm-build:fedora-rawhide-x86_64
• GitHub Check: rpm-build:fedora-41-ppc64le
• GitHub Check: rpm-build:fedora-41-aarch64
• GitHub Check: rpm-build:centos-stream-9-x86_64
• GitHub Check: rpm-build:epel-9-x86_64
• GitHub Check: rpm-build:fedora-42-x86_64
• GitHub Check: rpm-build:fedora-41-s390x
• GitHub Check: rpm-build:fedora-41-x86_64
• GitHub Check: Egg task debian:12.4
• GitHub Check: Podman spawner with 3rd party runner plugin
• GitHub Check: Version task ubuntu:22.04
• GitHub Check: Fedora selftests
• GitHub Check: Windows with Python 3.10
• GitHub Check: macOS with Python 3.11
• GitHub Check: Version task debian:12.4
• GitHub Check: Static checks
• GitHub Check: Version task ubi:8.8
• GitHub Check: Code Coverage (3.11)

[codecov]

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.47%. Comparing base (74b7379) to head (414b03c).
⚠️ Report is 6 commits behind head on master.

Files with missing lines	Patch %	Lines
avocado/utils/sysinfo.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6249      +/-   ##
==========================================
- Coverage   73.48%   73.47%   -0.02%     
==========================================
  Files         206      206              
  Lines       22494    22496       +2     
==========================================
- Hits        16530    16529       -1     
- Misses       5964     5967       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[richtja]

Hi @wangp-h, these changes LGTM and the CI failure are not related to this PR. Thank you.