Make qlever start and qlever add-text-index notice when qlever-server or qlever-index fail - #334
Merged
Merged
Conversation
The command ran `qlever-index -A ... | tee <log>` via a plain `subprocess.run` with `check=True`, but without `pipefail` the exit code of the pipeline is that of `tee`, so a failure of the index binary (for example the index-format error) was silently swallowed and the command reported success. It now uses `run_command`, which runs the pipeline under `set -o pipefail`, like the `index` command does. The `2>&1` before the `tee` additionally puts error messages into the log file.
For a server started with `nohup` (the default), the readiness loop had no handle on the server process and polled forever when the server exited before becoming ready, for example because of the index-format error. The start command now captures the PID of the `nohup`ed process via `echo $!` and the liveness check watches it, like it already watches the process handle in the foreground case and the container runtime in the container case.
hannahbast
changed the title
qlever start fail when the server exits before becoming readyqlever start and qlever add-text-index notice when the QLever binary fails
There was a problem hiding this comment.
Pull request overview
This PR improves the robustness of qlever start when starting the server in the default background (nohup) mode by making the readiness loop fail if the server exits before becoming ready, instead of polling indefinitely.
Changes:
- Capture the PID of the backgrounded
nohupserver viaecho $!and use it for a liveness check during readiness polling. - Update the start-command unit test to reflect the new
run_command(..., return_output=True)behavior for PID capture. - Improve
add_text_indexoutput handling by redirecting stderr into the logged stream and running the command via the sharedrun_commandhelper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| test/qlever/commands/test_start_execute.py | Updates expectations for nohup ... & echo $! PID capture using return_output=True. |
| src/qlever/commands/start.py | Captures PID for background starts and uses it for liveness checks during readiness polling. |
| src/qlever/commands/add_text_index.py | Routes stderr into the tee’d log and executes via run_command (with pipefail semantics). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+461
to
+470
| if capture_pid: | ||
| output = run_command(start_cmd, return_output=True) | ||
| process = None | ||
| with contextlib.suppress(ValueError, AttributeError): | ||
| pid = int(output.strip().splitlines()[-1]) | ||
| else: | ||
| process = run_command( | ||
| start_cmd, | ||
| use_popen=args.run_in_foreground, | ||
| ) |
hannahbast
changed the title
qlever start and qlever add-text-index notice when the QLever binary failsqlever start and qlever add-text-index notice when qlever-server or qlever-index fail
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.
So far, when the QLever server exited with an error right after being started (for example, because the index has an incompatible format),
qlever startdid not notice. It kept waiting for the server to answer, forever, and had to be interrupted by hand. The reason is that the server is started in the background, and while waiting,qlever startonly asked whether the server answers, not whether it is still there. It now remembers the process ID of the started server, notices when the process is gone, and fails with the message "Server process exited before becoming ready". The error message of the server itself is visible right above, because the log is shown while waiting.While at it, fix the analogous problem in qlever add-text-index.