All commands in shell.py use subprocess.run(..., check=True). When qs exits non-zero, CalledProcessError raises with an ugly raw traceback instead of a clean error message.
Additionally, restart sends a kill signal but immediately tries to start a new instance without waiting for the old process to release its lock file, producing: An instance of this configuration is already running.
And start silently succeeds when an instance is already running — qs exits 0 without flagging the conflict.
Changes
Error handling (all commands)
Replace check=True with capture_output=True, check the return code, and raise click.ClickException with qs's error message. Typer renders this as a rich styled error panel matching the rest of the CLI:
╭─ Error ─────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ No running instance to kill. │
╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
start — instance conflict detection
Add pre-check via qs -c zshell ipc show (exit 0 = running). If an instance is already running, show error immediately instead of silently succeeding.
restart — race condition fix
After killing, poll qs -c zshell kill up to 50× (2.5s, 50ms intervals) until exit code 255 (no instances), ensuring the old process fully terminated and released its lock before launching the new instance.
## Problem
All commands in `shell.py` use `subprocess.run(..., check=True)`. When `qs` exits non-zero, `CalledProcessError` raises with an ugly raw traceback instead of a clean error message.
Additionally, `restart` sends a kill signal but immediately tries to start a new instance without waiting for the old process to release its lock file, producing: `An instance of this configuration is already running.`
And `start` silently succeeds when an instance is already running — `qs` exits 0 without flagging the conflict.
## Changes
### Error handling (all commands)
Replace `check=True` with `capture_output=True`, check the return code, and raise `click.ClickException` with `qs`'s error message. Typer renders this as a rich styled error panel matching the rest of the CLI:
╭─ Error ─────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ No running instance to kill. │
╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
### `start` — instance conflict detection
Add pre-check via `qs -c zshell ipc show` (exit 0 = running). If an instance is already running, show error immediately instead of silently succeeding.
### `restart` — race condition fix
After killing, poll `qs -c zshell kill` up to 50× (2.5s, 50ms intervals) until exit code 255 (no instances), ensuring the old process fully terminated and released its lock before launching the new instance.
### Files changed
`cli/src/zshell/subcommands/shell.py` — all 7 commands (kill, start, restart, show, log, lock, call)
AramJonghu
changed title from fix(cli): replace raw subprocess tracebacks with styled error messages and fix restart race condition to hotfix(cli): replace raw subprocess tracebacks with styled error messages and fix restart race condition2026-05-23 21:08:07 +02:00
The IPC call in the start() function is the most pressing thing we should remove in my opinion. There is no need for a check like that when Quickshell has one built-in.
The IPC call in the `start()` function is the most pressing thing we should remove in my opinion. There is no need for a check like that when Quickshell has one built-in.
The start command should not need to run qs -c zshell ipc call, IPC calls are used to trigger states and functions within the QML code. The -n flag already checks if there's a running instance of the shell.
The start command should not need to run `qs -c zshell ipc call`, IPC calls are used to trigger states and functions within the QML code. The `-n` flag already checks if there's a running instance of the shell.
Is that not an messier implementation? A user would want a restart to be quick (quicker than running the kill and start command separately). 2.5 or higher stock sleep timer would make little sense.
time.monotonic() could be used to hide the ugliness, but same logic/idea.
Is that not an messier implementation? A user would want a restart to be quick (quicker than running the kill and start command separately). 2.5 or higher stock sleep timer would make little sense.
time.monotonic() could be used to hide the ugliness, but same logic/idea.
Running a loop would keep a CPU thread busy until it's able to launch the shell, while a sleep timer wouldn't. I don't really see much benefit in looping upwards of 50 times just to make sure we can run the start command exactly when it's available.
Running a loop would keep a CPU thread busy until it's able to launch the shell, while a sleep timer wouldn't. I don't really see much benefit in looping upwards of 50 times just to make sure we can run the start command exactly when it's available.
It was to have separate tests. Simpler in my eyes (at least at time of writing). I can simplify it by having a start_instance() that start uses outright, and is reused by restart. This could satisfy testing and reuse of code, and perhaps future code changes.
It was to have separate tests. Simpler in my eyes (at least at time of writing). I can simplify it by having a `start_instance()` that start uses outright, and is reused by restart. This could satisfy testing and reuse of code, and perhaps future code changes.
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
zach
Problem
All commands in
shell.pyusesubprocess.run(..., check=True). Whenqsexits non-zero,CalledProcessErrorraises with an ugly raw traceback instead of a clean error message.Additionally,
restartsends a kill signal but immediately tries to start a new instance without waiting for the old process to release its lock file, producing:An instance of this configuration is already running.And
startsilently succeeds when an instance is already running —qsexits 0 without flagging the conflict.Changes
Error handling (all commands)
Replace
check=Truewithcapture_output=True, check the return code, and raiseclick.ClickExceptionwithqs's error message. Typer renders this as a rich styled error panel matching the rest of the CLI:╭─ Error ─────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ No running instance to kill. │
╰─────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
start— instance conflict detectionAdd pre-check via
qs -c zshell ipc show(exit 0 = running). If an instance is already running, show error immediately instead of silently succeeding.restart— race condition fixAfter killing, poll
qs -c zshell killup to 50× (2.5s, 50ms intervals) until exit code 255 (no instances), ensuring the old process fully terminated and released its lock before launching the new instance.Files changed
cli/src/zshell/subcommands/shell.py— all 7 commands (kill, start, restart, show, log, lock, call)Since the text formatting does not show it well, here is an image.
fix(cli): replace raw subprocess tracebacks with styled error messages and fix restart race conditionto hotfix(cli): replace raw subprocess tracebacks with styled error messages and fix restart race conditionThe IPC call in the
start()function is the most pressing thing we should remove in my opinion. There is no need for a check like that when Quickshell has one built-in.@@ -14,3 +21,3 @@@app.command()def start(no_daemon: bool = False):subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), check=True)check = subprocess.run(args + ["ipc"] + ["show"], capture_output=True)The start command should not need to run
qs -c zshell ipc call, IPC calls are used to trigger states and functions within the QML code. The-nflag already checks if there's a running instance of the shell.Was not aware of this. Remaining functions use ipc, which is why I did not bat an eye. Resolving now.
Change occurred since I wanted an error block such as in the screenshot. But with that info, ipc is indeed not needed in the code.
@@ -21,2 +34,2 @@subprocess.run(args + ["kill"], check=False)subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), check=True)subprocess.run(args + ["kill"])for _ in range(50):This seems ugly. Personally think we should just set a sleep timer of ~5 seconds, then run code.
Is that not an messier implementation? A user would want a restart to be quick (quicker than running the kill and start command separately). 2.5 or higher stock sleep timer would make little sense.
time.monotonic() could be used to hide the ugliness, but same logic/idea.
Running a loop would keep a CPU thread busy until it's able to launch the shell, while a sleep timer wouldn't. I don't really see much benefit in looping upwards of 50 times just to make sure we can run the start command exactly when it's available.
Is that not okay for a rare command as restart?
We could change the loop to check every 250ms instead, so it is 10 checks instead of 50.
That's a good middle ground I think, agreed.
Changed it.
@@ -23,0 +37,4 @@if result.returncode == 255:breaktime.sleep(0.05)result = subprocess.run(args + ["-n"] + ([] if no_daemon else ["-d"]), capture_output=True)Here we can just call
start(), right? Not sure ifTyperallows calling command functions.It was to have separate tests. Simpler in my eyes (at least at time of writing). I can simplify it by having a
start_instance()that start uses outright, and is reused by restart. This could satisfy testing and reuse of code, and perhaps future code changes.You may resolve conversations if satisfied.