Repository navigation
Conversation
Contributor
Author
|
Since 2.71 (build 1680) RoonServer.dll was renamed to RoonServer.exe |
Toastyice
changed the title
Contributor
Author
|
With build 1683 Roonserver doesn't have any extension. root@talos-worker01:/# ps -aux
USER PID %CPU %MEM VSZ RSS TTY STAT START TIME COMMAND
root 1 0.0 0.0 4064 3216 ? Ss 07:17 0:00 /bin/bash /Roon/app/RoonServer/start.sh
root 44 0.1 0.1 35095612 59076 ? Sl 07:17 0:00 /Roon/app/RoonServer/Server/RoonServer
root 66 20.3 3.2 40824996 1074608 ? SLl 07:17 1:07 /Roon/app/RoonServer/Appliance/RoonAppliance -watchdogport=36661
root 67 0.0 0.0 3372 1568 ? S 07:17 0:00 /Roon/app/RoonServer/Server/processreaper 66
root 112 0.3 0.2 35232440 80036 ? Sl 07:17 0:01 /Roon/app/RoonServer/Appliance/RAATServer
root 427 0.0 0.0 4328 3828 pts/0 Ss 07:20 0:00 bash
root 500 0.0 0.0 6400 3784 pts/0 R+ 07:22 0:00 ps -aux |
The Server head has been renamed twice (RoonServer.dll -> RoonServer.exe -> RoonServer), and the probe missed each rename. Since 2.71 b1683 the head is exec'd through Server/.roonhost/RoonServer, so no process carries a .dll or .exe on its command line at all and the container reported unhealthy forever. Keep the cmdline match from the previous commit — it is what covers the legacy shared-runtime packages, where dotnet is exec'd with RoonServer.dll as an argument and comm is just "dotnet" — and add a comm probe for the apphost layouts. Neither alone covers every package. The comm probe is anchored with -x deliberately. Matching "RoonServer" unanchored against cmdline also matches the /Roon/app/RoonServer *directory*, so PID 1 (/bin/bash /Roon/app/RoonServer/start.sh) satisfies it and the container reports healthy for its whole life regardless of whether Roon runs. runtime.sh could not catch any of this: it starts a container with --entrypoint sleep, so nothing references the install path and even a directory-matching probe passes. Add tests/healthcheck.sh, which drives the probe directly against all five shipped launch shapes plus the case that actually regressed — supervisor alive, head gone. It reads the probe out of the built image via docker inspect so it cannot drift from the Dockerfile. Verified against each historical probe: the .dll-only version fails 3 of 6 scenarios, the unanchored version fails the head-dead case, the .dll|.exe version fails the 2 extensionless shapes, and this change passes all 6.
There was a problem hiding this comment.
Pull request overview
This PR updates the Docker image HEALTHCHECK so it correctly detects the RoonServer “head” process across multiple package layouts (including extensionless RoonServer), and adds targeted CI tests to prevent regressions in the probe logic.
Changes:
- Update
DockerfileHEALTHCHECK to check/proc/*/commforRoonServer/RoonServer.exeand fall back to/proc/*/cmdlinefor legacydotnet ... RoonServer.dllshapes. - Add
tests/healthcheck.shto pin HEALTHCHECK correctness across known historical/current process shapes (including “supervisor alive, head dead”). - Update workflow and runtime test descriptions to run and reflect the new probe-specific test suite.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
tests/runtime.sh |
Rewords HEALTHCHECK runtime tests to focus on Docker wiring; probe correctness is delegated to the new suite. |
tests/healthcheck.sh |
New focused test suite that extracts the image’s configured HEALTHCHECK and validates it against staged process shapes. |
Dockerfile |
Improves the HEALTHCHECK probe to handle extensionless head processes and legacy dotnet-hosted shapes. |
.github/workflows/build.yml |
Adds the new healthcheck probe tests to CI between smoke and runtime tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+28
to
+33
| # The HEALTHCHECK directive lands in image config as ["CMD-SHELL", "<script>"]. | ||
| PROBE="$(docker inspect --format '{{index .Config.Healthcheck.Test 1}}' "$IMAGE")" | ||
| if [ -z "$PROBE" ]; then | ||
| echo " FAIL image declares no HEALTHCHECK" | ||
| exit 1 | ||
| fi |
| VOLUME /Roon /RoonBackups /Music | ||
|
|
||
| # Healthcheck uses /proc directly instead of pgrep to avoid procps dependency | ||
| # Healthcheck uses /proc directly instead of pgrep to avoid procps dependency. |
Collaborator
|
@Toastyice I'm closing this PR in favor of one I just created: #30 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.