hotfix(cli): replace raw subprocess tracebacks with styled error messages and fix restart race condition #96
zach
Reference in New Issue
Block a user
Delete Branch "hotfix-restart-race-condition"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.