Add a --server-log-mode option to qlever start - #336
Merged
Conversation
Until now, `qlever start` removed the server log of the previous run before starting the server. This has repeatedly destroyed the evidence needed to diagnose a server crash. The new option controls what happens to an existing `<name>.server-log.txt`: `rotate` (the new default) moves it to `<log>.1`, shifting older generations up (all are kept); `overwrite` removes it as before; `append` keeps it and appends (only the new run is followed on the console); `no-log` writes no server log at all. Can also be set via `SERVER_LOG_MODE` in the `[server]` section of the Qleverfile.
There was a problem hiding this comment.
🟡 Changes recommended
--server-log-mode no-log in foreground currently won’t actually stream output to the terminal (and may block on piped output), and there are shell-quoting/shell=True issues around log handling that should be fixed.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR adds a new --server-log-mode option to qlever start (also configurable via the Qleverfile) to control how an existing <name>.server-log.txt is handled, preventing loss of crash evidence from previous runs.
Changes:
- Added
--server-log-mode(rotatedefault;append,overwrite,no-log) and wired it intoqlever start. - Implemented server-log rotation and adjusted log tailing behavior to optionally follow only newly appended output.
- Added tests covering
append/no-logcommand construction and log rotation behavior.
File summaries
| File | Description |
|---|---|
| test/qlever/commands/test_start_other_methods.py | Adds server_log_mode to the list of Qleverfile-relevant arguments tested. |
| test/qlever/commands/test_start_execute.py | Adds tests for append/no-log command construction and rotate_server_log. |
| src/qlever/util.py | Extends tail_log_file with from_beginning control for append-follow semantics. |
| src/qlever/qleverfile.py | Adds CLI parsing/help for --server-log-mode. |
| src/qlever/commands/start.py | Implements the new log-mode behaviors in command construction and execution flow (rotation, append follow, no-log). |
Review details
Suppressed comments (1)
src/qlever/util.py:764
- The
tail_log_filedocstring still states it always tails “from the beginning” and that the old log file should be deleted first, but the newfrom_beginningparameter (andappendmode) intentionally tails only new lines without deleting/rotating the existing file. Please update the docstring to reflect the new behavior.
from_beginning: bool = True,
) -> subprocess.Popen | None:
"""
Wait for the log file to appear and start tailing it from the
beginning. The old log file should be deleted before calling this
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+102
to
105
| else: | ||
| redirect = ">>" if args.server_log_mode == "append" else ">" | ||
| start_cmd += f" {redirect} {args.name}.server-log.txt 2>&1" | ||
| return start_cmd |
Comment on lines
+513
to
+517
| if args.run_in_foreground: | ||
| log.info( | ||
| "No server log is written, the server output goes to " | ||
| "this terminal (Ctrl-C stops the server)" | ||
| ) |
Comment on lines
+780
to
782
| tail_from = "+1" if from_beginning else "0" | ||
| tail_cmd = f"exec tail -n {tail_from} -f {log_file}" | ||
| return subprocess.Popen(tail_cmd, shell=True) |
Comment on lines
301
to
+303
| on_interrupt() | ||
| log_proc.terminate() | ||
| if log_proc is not None: | ||
| log_proc.terminate() |
With `--server-log-mode no-log` and `--run-in-foreground`, the server output now really goes to the terminal (without a redirection in the command, it went to a pipe that nobody reads, which would eventually block the server). Also fix the type annotation of `log_proc`, which can be `None` now.
There was a problem hiding this comment.
🟡 Changes recommended
Foreground no-log output is captured instead of displayed, while append mode can miss startup output.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
src/qlever/commands/start.py:101
- In foreground
no-logmode this does not actually send output to the terminal.executelaunches the command withrun_command(..., use_popen=True), whose defaults capture both streams insubprocess.PIPE(src/qlever/util.py:82-95), and nothing drains those pipes. The output is therefore hidden and a verbose server can block once the pipe buffers fill. Launch this mode withshow_output=Trueandshow_stderr=True.
if args.server_log_mode == "no-log":
# No log file is written. In the foreground, the server output
# goes to the terminal; in the background, it is discarded.
if not args.run_in_foreground:
start_cmd += " > /dev/null 2>&1"
src/qlever/commands/start.py:528
- The follower is started only after the server command has returned/launched, so
tail -n 0seeks to EOF after some startup output may already have been appended. Those lines are from the new run but will never be shown, contrary to append mode's contract. Record the pre-launch byte offset and tail from it, or start the follower before launching the server.
tail_proc = tail_log_file(
log_file, from_beginning=args.server_log_mode != "append"
)
src/qlever/commands/start.py:303
- This guard makes
Nonea supported value, andexecutenow passesNoneinno-logmode, but the parameter remains annotated assubprocess.Popen. Update the type contract to includeNone.
if log_proc is not None:
log_proc.terminate()
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
| def tail_log_file( | ||
| log_file: Path, | ||
| max_wait_seconds: int = 30, | ||
| from_beginning: bool = True, |
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.
Until now,
qlever startremoved the server log of the previous run before starting the server. This has repeatedly destroyed the evidence needed to diagnose a server crash. The new option--server-log-mode(also settable viaSERVER_LOG_MODEin the[server]section of the Qleverfile) controls what happens to an existing<name>.server-log.txt:rotate(the new default): move it to<log>.1, shifting older generations up; all generations are kept.overwrite: remove it (the behavior so far).append: keep it and append; only the output of the new run is followed on the console.no-log: write no server log at all; when the server runs in the foreground, its output goes to the terminal.