Fix UseSystemd() silently failing with ProtectProc=invisible#125520
Fix UseSystemd() silently failing with ProtectProc=invisible#125520CybCorv wants to merge 8 commits intodotnet:mainfrom
Conversation
GetIsSystemdService() used to read /proc/{ppid}/comm to verify the parent process is named "systemd". This file is hidden when the service runs with ProtectProc=invisible (recommended by systemd.exec(5)), causing the check to silently return false. As a result, no IHostLifetime is registered and READY=1 is never sent, leading to a systemd start timeout.
Fix this by using the SYSTEMD_EXEC_PID environment variable (systemd v248+), which is set to the PID of the main service process. Comparing it to the current PID is reliable and unaffected by ProtectProc=invisible.
The /proc fallback is kept for compatibility with older systemd versions (e.g. Ubuntu 20.04 / systemd 245, Debian 10 / systemd 241).
Partially addresses dotnet#88660
See dotnet#125368
|
Tagging subscribers to this area: @dotnet/area-extensions-hosting |
There was a problem hiding this comment.
Pull request overview
Updates Microsoft.Extensions.Hosting.Systemd systemd-service detection so UseSystemd() works when /proc access is restricted by ProtectProc=invisible, and adds tests for the new detection behavior.
Changes:
- Prefer
$SYSTEMD_EXEC_PID(systemd v248+) to detect whether the current process is the main systemd service process. - Keep the existing
/proc/{ppid}/commcheck as a legacy fallback for older systemd versions. - Add Linux-only
RemoteExecutortests covering$SYSTEMD_EXEC_PIDmatching/mismatching/missing/malformed and the existing static caching behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHelpers.cs | Adds $SYSTEMD_EXEC_PID-based detection before the legacy /proc fallback. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/SystemdHelpersTests.cs | New RemoteExecutor tests for detection behavior and caching. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/Microsoft.Extensions.Hosting.Systemd.Tests.csproj | Enables RemoteExecutor support for the test project and fixes a trailing space in the ProjectReference. |
src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/SystemdHelpersTests.cs
Outdated
Show resolved
Hide resolved
@dotnet-policy-service agree |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes UseSystemd() incorrectly not detecting systemd services when /proc/{ppid}/comm is inaccessible under ProtectProc=invisible, by switching to $SYSTEMD_EXEC_PID (systemd v248+) as the primary detection signal while keeping the /proc parent-name check as a legacy fallback.
Changes:
- Add
$SYSTEMD_EXEC_PID-based detection toSystemdHelpers.GetIsSystemdService()with legacy fallback retained. - Add Linux-only
RemoteExecutortests covering env-var detection and caching behavior. - Update the test project to include
RemoteExecutorinfrastructure and fix a malformedProjectReference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/SystemdHelpersTests.cs | Adds RemoteExecutor-isolated tests validating SYSTEMD_EXEC_PID behavior and caching. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/Microsoft.Extensions.Hosting.Systemd.Tests.csproj | Enables RemoteExecutor support and fixes ProjectReference formatting. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHelpers.cs | Uses SYSTEMD_EXEC_PID as the primary systemd detection method; retains /proc fallback. |
src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/SystemdHelpersTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
This PR fixes UseSystemd() incorrectly detecting “not running under systemd” when ProtectProc=invisible prevents reading /proc/<ppid>/comm, by preferring the SYSTEMD_EXEC_PID environment variable (systemd v248+) for detection and keeping the legacy /proc check as a fallback.
Changes:
- Update systemd-service detection to use
SYSTEMD_EXEC_PID(v248+) as the primary signal. - Keep the existing
/proc/<ppid>/commparent-name check as a legacy fallback path. - Add Linux-only unit tests (using
RemoteExecutor) to cover the new detection logic and the static caching behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHelpers.cs | Prefer SYSTEMD_EXEC_PID-based detection to avoid /proc access failures under ProtectProc=invisible, with legacy fallback retained. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/SystemdHelpersTests.cs | Adds RemoteExecutor-isolated tests for SYSTEMD_EXEC_PID match/mismatch/absent/malformed and caching behavior. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/Microsoft.Extensions.Hosting.Systemd.Tests.csproj | Enables RemoteExecutor usage for the new tests and fixes the project reference formatting. |
There was a problem hiding this comment.
Pull request overview
Updates Microsoft.Extensions.Hosting.Systemd’s systemd-service detection so UseSystemd() doesn’t silently no-op when /proc access is restricted (e.g., ProtectProc=invisible), and adds focused unit tests for the new detection behavior.
Changes:
- Prefer
$SYSTEMD_EXEC_PID(systemd v248+) to detect running under systemd without relying on/proc. - Retain existing
/proc/<ppid>/commparent-name check as a fallback for older systemd versions. - Add
RemoteExecutor-based tests to validateSYSTEMD_EXEC_PIDhandling and static caching; enable RemoteExecutor in the test project.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHelpers.cs | Adds $SYSTEMD_EXEC_PID-based detection with legacy /proc fallback. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/SystemdHelpersTests.cs | New RemoteExecutor tests covering matching/mismatching/absent/malformed values and caching. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/Microsoft.Extensions.Hosting.Systemd.Tests.csproj | Enables RemoteExecutor; fixes a stray space in the ProjectReference include. |
Manual integration testingI've validated the fix against a full test matrix using a dedicated test harness: https://github.com/CybCorv/systemd-dotnet-hosting-tests
|
|
Note This review was generated with the assistance of AI (GitHub Copilot). Code Review SummaryOverall this is a well-scoped, correct fix for a real production issue. The integration test matrix is thorough. A couple of points for discussion:
|
Update test accordingly.
|
Thanks for the thorough review @cincuranet . I agree with the fall-through approach for backwards compatibility. Returning false immediately on mismatch would silently break existing users. That said, the spec is clear that a mismatch means "you are not the main service process." The fall-through is a compromise imposed by the current design where Regarding the suggested test for I'll update the implementation and the affected test. I've added a manual integration test for the
|
I was aware of the NOTIFY_SOCKET-only approach discussed in #88660. I chose to split the work because The broader decoupling ( That said, I'm happy to do it all in one PR if that's the preferred direction. Your call. |
Could we extend what is in
If it is something you'd want to work on, I'd prefer you include it in this PR. |
UseSystemd() and AddSystemd() now register the notifier based on NOTIFY_SOCKET alone, and the log formatter based on IsSystemdService(). Unset NOTIFY_SOCKET after capture to prevent child process inheritance. Centralize environment variable names in SystemdConstants.
I've updated the PR to include the Manual integration test matrix:
Docker timeout is a pre-existing issue. Two open questions before marking ready for review:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes UseSystemd()/AddSystemd() not activating correctly when /proc visibility is restricted (e.g., ProtectProc=invisible) by splitting “systemd service detection” from “sd_notify availability” and improving the service detection logic.
Changes:
- Prefer
SYSTEMD_EXEC_PID(systemd v248+) forIsSystemdService()detection, with legacy/proc/{ppid}/commfallback. - Add
IsSystemdNotify()detection (based onNOTIFY_SOCKET) and use it to decide whether to registerSystemdLifetime/SystemdNotifier. - Expand/add tests using
RemoteExecutorand enableIncludeRemoteExecutorin the test project.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHelpers.cs | Adds SYSTEMD_EXEC_PID-based service detection and introduces cached IsSystemdNotify() detection. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdHostBuilderExtensions.cs | Registers logger based on IsSystemdService() and lifetime/notifier based on IsSystemdNotify(). |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdNotifier.cs | Switches to shared constants and clears NOTIFY_SOCKET after reading it. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdConstants.cs | Introduces shared env-var constant names and doc links. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/UseSystemdTests.cs | Adds matrix-style tests for logger vs lifetime registration based on SYSTEMD_EXEC_PID/NOTIFY_SOCKET. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/SystemdHelpersTests.cs | Adds coverage for SYSTEMD_EXEC_PID parsing/caching behaviors. |
| src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/Microsoft.Extensions.Hosting.Systemd.Tests.csproj | Enables RemoteExecutor and fixes a ProjectReference formatting issue. |
src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdConstants.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdConstants.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting.Systemd/src/SystemdNotifier.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting.Systemd/tests/SystemdHelpersTests.cs
Outdated
Show resolved
Hide resolved
Rename test to accurately describe what is verified. Add namespace to SystemdConstants.
👍 I will make some time early next week to review the PR. |
Summary
UseSystemd()silently fails when the service is configured withProtectProc=invisible(recommended bysystemd.exec(5)for most services).GetIsSystemdService()reads/proc/{ppid}/commto check whether the parent process isnamed
systemd. UnderProtectProc=invisible, this file is hidden for processes owned byother users, causing an exception that is silently swallowed — the method returns
false,no
IHostLifetimeis registered,READY=1is never sent, and systemd times out waiting forthe service to become ready.
Fix
Use
$SYSTEMD_EXEC_PID(introduced in systemd v248) as the primary detection method forIsSystemdService(). systemd sets this variable to the PID of the main service process;comparing it to
Environment.ProcessIdis reliable and unaffected byProtectProc.The existing
/proc/{ppid}/commcheck is kept as a fallback for older systemd versionsstill in use on maintained distributions (Ubuntu 20.04 ships systemd 245, Debian 10 ships
systemd 241).
UseSystemd()andAddSystemd()now register the notifier based onNOTIFY_SOCKETalone,independently of
IsSystemdService().NOTIFY_SOCKETis unset after capture to preventchild process inheritance. The log formatter is still conditioned by
IsSystemdService().Testing
Tests use
RemoteExecutorto isolate the static cache inSystemdHelpers._isSystemdService.Manual integration test matrix:
Out of scope
SystemdConsoleFormattershould use$JOURNAL_STREAM+fstatinstead ofunconditionally writing journal-formatted output — deferred to a follow-up PR.