Repository: skypilot-org/skypilot
Pull request: [UX] Fix unexpected output for ctrl-c during `sky launch`
<!-- Describe the changes in this PR -->

Fixes #2205.

The `os.killpg` might be applied to a subprocess that was attached under the root user, due the parent process being killed first. This will cause the `PermissionError: [Errno 1] Operation not permitted`

@concretevitamin Could you verify if this solve the problem?

<!-- Describe the tests ran -->
<!-- Unit tests (tests/test_*.py) are part of GitHub CI; below are tests that launch on the cloud. -->

Tested (run the relevant ones):

- [x] Code formatting: `bash format.sh`
- [x] Any manual or new tests for this PR (please specify below)
  - [x] `ctrl-c` during the `Launching` progress spinner
  - [x] `ctrl-c` during the `sky logs` (originally will show `ProcessLookupError`
- [ ] All smoke tests: `pytest tests/test_smoke.py` 
- [ ] Relevant individual smoke tests: `pytest tests/test_smoke.py::test_fill_in_the_name` 
- [ ] Backward compatibility tests: `bash tests/backward_comaptibility_tests.sh`


Problem statement:
Weird error messages after `ctrl-c` out a running job's log
```
sky launch --cpus 1+ --use-spot --down 'for i in $(seq 10000); do echo $i; sleep 1; done' -y
```
After the job starts running, press Ctrl-C. This will then show
```
...
<status table>
KeyboardInterrupt: 130

During handling of the above exception, another exception occurred:

PermissionError: [Errno 1] Operation not permitted
```

A few hours ago, I also saw this variant for a different command:
```
...
KeyboardInterrupt: 130

During handling of the above exception, another exception occurred:

ProcessLookupError: [Errno 3] No such process
```
IIRC (could be wrong) it was an autostopped cluster.

@Michaelvll could this be related to a recent change?

Hints from issue discussion:

Commit message: [UX] Fix unexpected output for ctrl-c

Patch to review:
diff --git a/sky/skylet/log_lib.py b/sky/skylet/log_lib.py
index 5debce93264..41ca95697ff 100644
--- a/sky/skylet/log_lib.py
+++ b/sky/skylet/log_lib.py
@@ -262,7 +262,12 @@ def run_with_log(
             # Send SIGINT to the process directly, otherwise, the underlying
             # process will only be killed after the python program exits,
             # causing the stream handling stuck at `readline`.
-            os.killpg(proc.pid, signal.SIGINT)
+            try:
+                os.killpg(proc.pid, signal.SIGINT)
+            except Exception:  # pylint: disable=broad-except
+                # The killing is ok to fail, as the proc might be finished
+                # already.
+                pass
             raise
 
 


Code review comments:
diff_hunk: @@ -262,7 +262,12 @@ def run_with_log(
             # Send SIGINT to the process directly, otherwise, the underlying
             # process will only be killed after the python program exits,
             # causing the stream handling stuck at `readline`.
-            os.killpg(proc.pid, signal.SIGINT)
+            try:
+                os.killpg(proc.pid, signal.SIGINT)
+            except Exception:  # pylint: disable=broad-except
path: sky/skylet/log_lib.py
text: @user1:
Thanks, I confirm it fixed ProcessLookupError: https://github.com/skypilot-org/skypilot/issues/2205#issuecomment-1629445037.

1. However, what was the cause of the echo job's error `PermissionError: [Errno 1] Operation not permitted`? It seems like ctrl-c should just work.

2. It seems a bit dangerous to ignore all exceptions. Is it ok to only catch ProcessLookupError, and surface real PermissionError's (different from the repro)?

@author:
> However, what was the cause of the echo job's error PermissionError: [Errno 1] Operation not permitted? It seems like ctrl-c should just work.

I am not exactly sure for the reason of the error, but my guess is that the parent process might be killed before the child processes, causing the child process owned by the `root` user and a `PermssionError` when `os.killpg` is called.

I now replaced the implementation with the `kill_children_processes` instead, to be align with our implementation in 
https://github.com/skypilot-org/skypilot/blob/c2e9395f65b0647cd0a5ceb18bcb6ae41085c6a8/sky/backends/cloud_vm_ray_backend.py#L3258
https://github.com/skypilot-org/skypilot/blob/c2e9395f65b0647cd0a5ceb18bcb6ae41085c6a8/sky/backends/backend_utils.py#L2479-L2487