fix(daemon): recover stale service PID files safely - #1469
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe daemon now uses typed PID mismatch errors and explicit process identity checks. Startup removes a confirmed stale PID file without signaling the unrelated process. Tests cover concurrent recovery, shell identity validation, and unavailable process identities. ChangesDaemon PID handling
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Start
participant Running
participant serviceProcessIdentity
participant PIDFile
Start->>Running: Check recorded PID
Running->>serviceProcessIdentity: Identify recorded PID
serviceProcessIdentity-->>Running: Confirm foreign process
Running-->>Start: Return servicePIDMismatchError
Start->>PIDFile: Re-read and remove unchanged stale PID
Start-->>Start: Launch service
Merge Risk: ⚪ Minimal · up to Daemon startup now safely replaces confirmed stale PID files without signaling unrelated processes, while preserving fail-closed handling for unknown identities. The covered recovery and idempotence behavior is ready to merge. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The kernel places the interpreted script after any shebang option, so "#!/bin/sh -e" puts the option at argv[1] and the script at argv[2]. Reading only argv[1] reported a live shell-backed service as foreign, and Start now removes a foreign PID file, so the wrapper would have started a second service against the running one. Scan for the first argument that is not a flag instead, and keep -c and -s rejected so a path in a command string stays data rather than identity. Resolved interpreters on buildroot images are also ash, ksh or zsh rather than a busybox symlink, so accept those.
Every platform wrapper checks Running before auto-starting and returns on any error, so a reused PID failed with "error checking service status" and Start was never called. The recovery this branch adds was unreachable from the only entry point users have. Treat the PID-file conflict as not running and let Start clear it under the start gate. Other Running errors, including unknown process identity, still abort as before.
All four platform wrappers repeated the same "is it running, and is this the recoverable stale PID file" predicate inline, in package main where nothing tests it. Move it behind RunningForAutoStart so the behaviour is covered once, in the package that owns it. Running still reports the conflict, so Stop and Restart keep refusing to touch an unrelated process.
RunningForAutoStart tolerates exactly one error, the recoverable stale PID conflict. Nothing asserted that it still surfaces the others, and reporting an unreadable PID file as "not running" would start a second service on top of whatever that file described.
1 participant
Summary
Start, under the existing start gate, without signaling that process or changing PID-file format.StopandRestartstill refuse unrelated PIDs.Ref ZaparooProject/Main_MiSTer#24. This is the Core-owned cleanup companion to Main's watchdog identity check; Core should ship before relying on automatic PID-reuse recovery.
Validated with
task test,task lint,task deadlock,task cross-lint:allandtask vulncheck. Original code failed the recovery regression. Vulnerability scan reports no called vulnerable code; it notes uncalled dependency advisories. No device service, deployment or release changes. Work isolated from the owner's active checkout.Summary by CodeRabbit