Fix runtime install wait race#24
Conversation
|
I don't think this is the right fix. It essentially bakes the smoke tests into the wait function (adds all the file tests to the wait) instead of maintaining the separation of concern between :
I prefer the cleanliness of my proposed fix - wait for the log signal (which already a function of the smoke harness) that indicates that the installation completed. Make no assumptions about what was installed. Then allow the smoke tests to validate the installation itself. Just my opinion. |
|
Thanks — agreed on keeping install-complete signaling separate from the artifact assertions. I updated the PR in Verification here: |
|
There is a very simple approach to this, which is what I proposed in the issue I created. It's just to change wait_for_install to this. That's the entire fix. It's not branch specific, it has no opinion about what the installation completed state is. It just simply waits until the installation is done and returns. If the installation is already done, it sees that and returns immediately. This is up to the maintainers but I think you should consider modifying your PR to do just this. And test it, too, please. I've tested it pretty extensively and it works. |
|
Updated again in Verification now completed locally:
|
|
Thanks for adapting this @alceops! Very much appreciated! |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
There was a problem hiding this comment.
Pull request overview
Fixes a race condition in tests/runtime.sh where wait_for_install returned as soon as the VERSION file appeared in the tarball extraction, but later artifacts (e.g. Server/RoonServer) might still be missing, causing spurious test failures (#23). The function now waits for the entrypoint's RoonServer installed successfully log line, which is emitted only after tar finishes extracting.
Changes:
- Replace filesystem polling for
VERSIONwithwait_for_logon theRoonServer installed successfullymessage. - Remove the per-iteration sleep/elapsed loop and its progress output.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review feedback. After switching the body to a log-based wait, the dir positional was bound but never read, and the function silently relied on the outer-scope CONTAINER. Removing the parameter makes the signature truthful and updates all seven call sites.
|
Thanks @alceops and @gtunes-dev |
Summary
Server/RoonServerlauncher andRoonDotnetruntime directory before the fresh-install runtime assertions runVERSIONas part of the readiness check, but no longer treat it as the sole install-complete sentinelFixes #23.
Verification
bash -n tests/runtime.shgit diff --checkI did not run the full Docker runtime workflow in this worker; the change is limited to the shell readiness predicate used by that workflow.