Repository: PrefectHQ/prefect
Pull request: Server should exit with non-0 on error
<!-- 
Thanks for opening a pull request to Prefect! 
If this is your first contribution, please make sure to review our contribution guidelines: https://docs.prefect.io/latest/contributing/overview/
-->

<!-- Include an overview of the proposed changes here -->
Closes https://github.com/PrefectHQ/prefect/issues/16474 .

Just `sys.exit(process.returncode)` would be fine here, but for the sake of consistency with the rest of the code, I went for the `exit_with_error` utility. The server process is expected to run for ever until stopped, so we should never get a `0` exit code from the process, hence there is no need for a ternary condition here.

I also went ahead and removed the bare except right above - all it did was log the error and continue, and this only hides potential errors and causes issues similar to the one I faced.

In my view, this is a bugfix, but I suppose some clients could depend on the 0 exit code here in theory? I have not familiarized with your versioning process so let me know if I should bump the patch version or something as part of this.

**Help needed**: Seems like the CI pipeline times out after 15 minutes. Could be my new test was "the straw that broke the camels back"? Any advice welcome.

### Checklist
<!-- These boxes may be checked after opening the pull request. -->

- [x] This pull request references any related issue by including "closes `<link to issue>`"
  - If no issue exists and your change is not a small fix, please [create an issue](https://github.com/PrefectHQ/prefect/issues/new/choose) first.
- [x] If this pull request adds new functionality, it includes unit tests that cover the changes
- [x] If this pull request removes docs files, it includes redirect settings in `mint.json`.
- [x] If this pull request adds functions or classes, it includes helpful docstrings.


Problem statement:
Server command exits with exit code 0 even when failing due to database connectivity
### Bug summary

I noticed this when deploying via helm: The pod reaches status `Completed` when an exception is thrown. It should exit with non-0 such that kubernetes will restart the pod.

### Version info

```Text
2024.12.19222717 (helm)
```


### Additional context

I can potentially fix the issue myself if you'll accept a contribution

Hints from issue discussion:
@martimors we'd love a contribution, either to the main prefect library or https://github.com/PrefectHQ/prefect-helm! We have [some contribution docs here](https://docs.prefect.io/contribute/dev-contribute) if that's helpful to get setup. Let us know if we can help
> @martimors we'd love a contribution, either to the main prefect library or https://github.com/PrefectHQ/prefect-helm! We have [some contribution docs here](https://docs.prefect.io/contribute/dev-contribute) if that's helpful to get setup. Let us know if we can help

I gave it a stab https://github.com/PrefectHQ/prefect/pull/16476

Let me know what you think

Commit message: Merge branch 'main' into bugfix/exit-with-non-0-on-error

Patch to review:
diff --git a/src/prefect/cli/server.py b/src/prefect/cli/server.py
index fb66042be919..72c37daee814 100644
--- a/src/prefect/cli/server.py
+++ b/src/prefect/cli/server.py
@@ -311,10 +311,10 @@ async def start(
 
     except anyio.EndOfStream:
         logging.error("Subprocess stream ended unexpectedly")
-    except Exception as e:
-        logging.error(f"An error occurred: {str(e)}")
 
-    app.console.print("Server stopped!")
+    # This should only be reached if the server crashed, or was forcibly terminated,
+    # hence we exit with an error
+    exit_with_error("Server stopped!", code=process.returncode)
 
 
 @server_app.command()
diff --git a/tests/cli/test_start_server.py b/tests/cli/test_start_server.py
index ccbb4a747fb8..2066a4e35900 100644
--- a/tests/cli/test_start_server.py
+++ b/tests/cli/test_start_server.py
@@ -14,6 +14,7 @@
 from prefect.cli.server import PID_FILE
 from prefect.context import get_settings_context
 from prefect.settings import (
+    PREFECT_API_DATABASE_CONNECTION_URL,
     PREFECT_API_URL,
     PREFECT_HOME,
     PREFECT_PROFILES_PATH,
@@ -413,3 +414,16 @@ def test_choose_when_multiple_profiles_have_same_api_url(self):
 
         profiles = load_profiles()
         assert profiles.active_name == "local"
+
+
+class TestServerExitCodes:
+    def test_misconfigured_server_exits_nonzero(self) -> None:
+        invalid_connection_url = "FOOBAR!"
+        with temporary_settings(
+            {PREFECT_API_DATABASE_CONNECTION_URL: invalid_connection_url}
+        ):
+            invoke_and_assert(
+                command=["server", "start"],
+                expected_code=1,
+                expected_output_contains=invalid_connection_url,
+            )


Code review comments:
diff_hunk: @@ -413,3 +414,16 @@ def test_choose_when_multiple_profiles_have_same_api_url(self):
 
         profiles = load_profiles()
         assert profiles.active_name == "local"
+
+
+class TestServerExitCodes:
+    def test_misconfigured_server_exits_nonzero(self) -> None:
+        invalid_connection_url = "FOOBAR!"
+        with temporary_settings(
+            {PREFECT_API_DATABASE_CONNECTION_URL: invalid_connection_url}
+        ):
+            invoke_and_assert(
+                command=["server", "start"],
+                expected_code=1,
+                expected_output_contains=invalid_connection_url,
+            )
path: tests/cli/test_start_server.py
text: @user1:
When I run this test locally, it hangs. It think the `temporary_settings` isn't propagating to the process running the server in this test. It works with this update:
```suggestion
    def test_misconfigured_server_exits_nonzero(self, monkeypatch) -> None:
        monkeypatch.setenv("PREFECT_API_DATABASE_CONNECTION_URL", "FOOBAR!")

        invoke_and_assert(
            command=["server", "start"],
            expected_code=1,
            expected_output_contains="FOOBAR!",
        )
```

@author:
Implemented here [7756c13](https://github.com/PrefectHQ/prefect/pull/16476/commits/7756c1379224fe64926416d970dcc590d546003a)

All the tests pass for me locally (ubuntu 24.04, python 3.12, probably SQLite?). However, it seems like some test fails for whatever reason in the pipeline - I'm not sure what's going on there, but maybe you can advice?

@user1:
It might be better to update some existing tests in that case. If you update the tests in `TestUvicornSignalForwarding` in the same file to check the return code, then I think that covers the change well. You can do that by updating each place we wait for a server process to exit:
```python
await server_process.wait()
```
to verify the return code is non-zero:
```python
assert await server_process.wait() != 0, "Server process exited with zero code"
```
Assertions like that fail on `main` but pass with your PR.