Run executable as Windows Services#170
Conversation
|
Warning Review limit reached
More reviews will be available in 29 minutes and 19 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR refactors the backend CLI entrypoint from a monolithic module into a modular package structure, adds Windows service lifecycle management with SCM integration and crash-dump handling, introduces comprehensive E2E testing for Windows service scenarios, reorganizes CLI unit tests into a dedicated test directory with cleanup fixtures, and updates infrastructure tooling versions and configuration files across the repository. ChangesBackend entrypoint refactoring and Windows service support
Infrastructure tooling and configuration updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/settings/permissions/bash.jsonc:
- Line 79: Replace the wildcard permission "Bash(pnpm test*)" with explicit
allowed commands by removing the wildcard and enumerating each safe test script
(e.g., "Bash(pnpm test)", "Bash(pnpm test:unit)", "Bash(pnpm test:integration)"
— list the actual script names used in package.json), ensuring you delete
"Bash(pnpm test*)" and add one entry per approved script so only those exact
commands are auto-approved.
In `@pyproject.toml`:
- Around line 21-24: The current per-index setting exclude-newer = "1 day" under
[[tool.uv.index]] risks skipping packages released within the last 24 hours
(delaying urgent fixes); revisit this tradeoff and either remove or shorten the
duration (e.g., to a few minutes) or make it configurable globally so critical
security or hotfix releases are not ignored; update the pyproject.toml entry for
[[tool.uv.index]] by changing or removing the exclude-newer value and document
the chosen rationale in project configuration notes.
In `@template/`{% if has_backend %}backend{% endif
%}/src/backend_api/entrypoint/{% if install_as_windows_service
%}win_service.py{% endif %}:
- Around line 48-52: The service is re-slicing process-global sys.argv when
constructing service_argv so "service debug" yields ["debug"] passed to
parser.parse_args; instead persist the already-split runtime args passed into
the Service at construction by storing args on the instance (e.g.
self._runtime_args) in __init__ and change the code that builds service_argv /
calls parser.parse_args (the block that currently slices sys.argv and sets
service_argv before parser.parse_args) to use self._runtime_args rather than
sys.argv[2:], and ensure the thread worker/stop logic still uses the saved tuple
type.
- Around line 56-59: SvcDoRun currently calls self._worker.join() unbounded (and
also does a single join(timeout=30) on the control thread), so if the worker
blocks the service never returns to let SCM force-stop; change the stop/wait
logic to use bounded waits and polling: when stopping, signal the worker to
stop, then repeatedly call self._worker.join(timeout=1) (or loop checking
self._worker.is_alive() with short timeout sleeps) up to the total desired
timeout (e.g., 30s) so the method can return when the timeout expires; update
both the control-thread join and the main worker join sites (references:
SvcDoRun, self._worker.join, self._worker.is_alive) to use this bounded/polled
join approach or use a second event to wake the main thread so the timeout
branch can exit.
In `@template/`{% if has_backend %}backend{% endif %}/tests/{% if
install_as_windows_service %}windows_service{% endif
%}/test_service_lifecycle.py:
- Around line 187-194: The test reads sc query once which can hit a transient
STOP_PENDING state; change the assertions in test_service_lifecycle.py to poll
_run_sc("query", APP_NAME, check=True) in a short retry loop (e.g., sleep
between attempts, stop after a small timeout or max attempts) until the output
contains "STOPPED" (or timeout/fail), then assert that
_ERROR_SERVICE_SPECIFIC_ERROR appears in the final output; use a local
max_wait_seconds/max_attempts variable and short sleep to avoid flakes while
keeping the existing symbols (_run_sc, APP_NAME, _ERROR_SERVICE_SPECIFIC_ERROR)
and semantics.
- Around line 168-170: The teardown currently unconditionally deletes
crash_dump_path (crash_dump_path.exists() / crash_dump_path.unlink()), which
removes failure evidence; change the fixture finalizer to only remove the crash
dump when the test passed (e.g., check the pytest outcome via
request.node.rep_call.passed in the fixture finalizer) or remove the unlink call
entirely so CI can preserve dumps for failed runs—locate the fixture finalizer
where crash_dump_path.unlink() is called and wrap the unlink in a conditional
that only runs on successful test completion.
In `@template/`{% if has_backend %}backend{% endif
%}/tests/unit/entrypoint/test_entrypoint.py:
- Line 39: Replace the string literal "exception" in the spy call with the
method's __name__ to keep the test rename-safe: change the call spied_logger =
mocker.spy(cli.logger, "exception") to use cli.logger.exception.__name__ (i.e.,
spied_logger = mocker.spy(cli.logger, cli.logger.exception.__name__)), leaving
the rest of the test unchanged and targeting the cli.logger.exception symbol.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aac9b914-7a9b-4194-9ec6-cb6f77de161a
📒 Files selected for processing (32)
.claude/settings/permissions/bash.jsonc.copier-answers.yml.devcontainer/devcontainer.json.devcontainer/install-ci-tooling.pyAGENTS.mdcopier.ymlextensions/context.pypyproject.tomltemplate/.claude/settings/permissions/bash.jsonctemplate/.coveragerctemplate/.devcontainer/devcontainer.json.jinjatemplate/.github/workflows/ci.yaml.jinjatemplate/AGENTS.mdtemplate/{% if has_backend %}backend{% endif %}/pyproject.toml.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/app_runner.pytemplate/{% if has_backend %}backend{% endif %}/src/backend_api/cli.pytemplate/{% if has_backend %}backend{% endif %}/src/backend_api/entrypoint/__init__.pytemplate/{% if has_backend %}backend{% endif %}/src/backend_api/entrypoint/cli.py.jinjatemplate/{% if has_backend %}backend{% endif %}/src/backend_api/entrypoint/parser.pytemplate/{% if has_backend %}backend{% endif %}/src/backend_api/entrypoint/{% if install_as_windows_service %}win_service.py{% endif %}template/{% if has_backend %}backend{% endif %}/src/entrypoint.pytemplate/{% if has_backend %}backend{% endif %}/tests/unit/entrypoint/__init__.pytemplate/{% if has_backend %}backend{% endif %}/tests/unit/entrypoint/fixtures.pytemplate/{% if has_backend %}backend{% endif %}/tests/unit/entrypoint/test_arg_parsing.pytemplate/{% if has_backend %}backend{% endif %}/tests/unit/entrypoint/test_entrypoint.pytemplate/{% if has_backend %}backend{% endif %}/tests/unit/entrypoint/test_stopping_behavior.pytemplate/{% if has_backend %}backend{% endif %}/tests/unit/entrypoint/{% if install_as_windows_service %}test_service_subcommand.py{% endif %}template/{% if has_backend %}backend{% endif %}/tests/unit/test_cli.pytemplate/{% if has_backend %}backend{% endif %}/tests/{% if install_as_windows_service %}windows_service{% endif %}/__init__.pytemplate/{% if has_backend %}backend{% endif %}/tests/{% if install_as_windows_service %}windows_service{% endif %}/conftest.pytemplate/{% if has_backend %}backend{% endif %}/tests/{% if install_as_windows_service %}windows_service{% endif %}/test_service_lifecycle.pytemplate/{% if has_backend %}backend{% endif %}/{% if deploy_as_executable %}pyinstaller.spec{% endif %}.jinja
💤 Files with no reviewable changes (2)
- template/{% if has_backend %}backend{% endif %}/src/backend_api/cli.py
- template/{% if has_backend %}backend{% endif %}/tests/unit/test_cli.py
| "Bash(pnpm list *)", | ||
| "Bash(pnpm test-unit *)", | ||
| "Bash(pnpm test-e2e *)", | ||
| "Bash(pnpm test*)", |
There was a problem hiding this comment.
Keep the pnpm allow-list explicit.
Bash(pnpm test*) auto-approves any current or future script whose name starts with test, so a later script addition can silently widen the agent's privileges. Please enumerate only the specific test scripts that are safe to run unattended.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/settings/permissions/bash.jsonc at line 79, Replace the wildcard
permission "Bash(pnpm test*)" with explicit allowed commands by removing the
wildcard and enumerating each safe test script (e.g., "Bash(pnpm test)",
"Bash(pnpm test:unit)", "Bash(pnpm test:integration)" — list the actual script
names used in package.json), ensuring you delete "Bash(pnpm test*)" and add one
entry per approved script so only those exact commands are auto-approved.
| [[tool.uv.index]] | ||
| name = "pypi" | ||
| url = "https://pypi.org/simple/" | ||
| exclude-newer = "1 day" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
uv exclude-newer configuration syntax format
💡 Result:
To exclude newer package versions in uv via configuration (pyproject.toml), use the exclude-newer setting under [tool.uv]. Example: [tool.uv] exclude-newer = "5 days" This limits candidate packages to those uploaded prior to the cutoff date/time, where the value accepts RFC 3339 timestamps (e.g. 2006-12-02T02:07:43Z), local dates (e.g. 2006-12-02), “friendly” durations (e.g. 1 week, 30 days, 24 hours), or ISO 8601 durations (e.g. P7D, PT24H, P30D) [1][2]. Alternative: per-package override - Global cutoff: exclude-newer - Per-package override: exclude-newer-package The docs show that exclude-newer-package accepts package-specific values and includes an example of disabling the global rule for a package using false, e.g. exclude-newer-package = { pytest = false } [3]. CLI syntax note (equivalent concept): uv also provides --exclude-newer / --exclude-newer-package with the same accepted value formats, and per-package values are passed as PACKAGE=DATE [2]. So the “configuration syntax format” you’re looking for is the TOML key/value format in pyproject.toml, not a special nested structure for exclude-newer itself (it’s a single string) [1].
Citations:
- 1: https://docs.astral.sh/uv/reference/settings/
- 2: https://docs.astral.sh/uv/reference/cli/
- 3: Allow relative
exclude-newervalues like "one week ago" astral-sh/uv#14992
🌐 Web query:
uv [[tool.uv.index]] exclude-newer configuration
💡 Result:
In uv, you can configure the exclude-newer setting on a per-index basis within your pyproject.toml or uv.toml file under the [[tool.uv.index]] section [1][2][3]. This feature allows you to define a specific cutoff date or duration for a particular package index, overriding the global exclude-newer setting [1][2][3]. To configure this, add an exclude-newer key to the desired index definition [3]: [[tool.uv.index]] name = "internal" url = "https://internal.example.com/simple" exclude-newer = "7 days" You can also disable the exclude-newer cutoff for a specific index by setting it to false, which is useful for registries that do not provide the necessary upload-time metadata [3][4]: [[tool.uv.index]] name = "internal" url = "https://internal.example.com/simple" exclude-newer = false Regarding configuration priority, package-specific exclude-newer-package overrides take the highest priority, followed by index-specific exclude-newer settings, and finally the global exclude-newer configuration [2][3].
Citations:
- 1: astral-sh/uv@39b83c3
- 2: Add
exclude-newerto[[tool.uv.index]]astral-sh/uv#18839 - 3: https://docs.astral.sh/uv/concepts/indexes/
- 4: exclude-newer should have an extra flag to allow it to skip over packages that don't have publish date information. astral-sh/uv#12449
Revisit the exclude-newer = "1 day" tradeoff (syntax and placement are valid)
exclude-newer = "1 day" is a valid “friendly” duration and can be set per index under [[tool.uv.index]], so the TOML/config approach is correct for pyproject.toml:21-24. The main remaining risk is that this will ignore any packages uploaded within the last 24 hours for that index, which can delay adoption of urgent security fixes/releases.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pyproject.toml` around lines 21 - 24, The current per-index setting
exclude-newer = "1 day" under [[tool.uv.index]] risks skipping packages released
within the last 24 hours (delaying urgent fixes); revisit this tradeoff and
either remove or shorten the duration (e.g., to a few minutes) or make it
configurable globally so critical security or hotfix releases are not ignored;
update the pyproject.toml entry for [[tool.uv.index]] by changing or removing
the exclude-newer value and document the chosen rationale in project
configuration notes.
| def __init__(self, args: tuple[str, ...]) -> None: | ||
| super().__init__(args) | ||
| self._stop_event = threading.Event() | ||
| self._worker: threading.Thread | None = None | ||
|
|
There was a problem hiding this comment.
service debug still gets parsed as an app argument.
When the process is launched as ... service debug, Line 67 rebuilds service_argv from sys.argv[2:], so parser.parse_args() sees ["debug"] and exits before the app starts. Persist the already-split runtime args on the service instance and read them here instead of re-slicing the process-global argv.
Also applies to: 61-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@template/`{% if has_backend %}backend{% endif
%}/src/backend_api/entrypoint/{% if install_as_windows_service
%}win_service.py{% endif %} around lines 48 - 52, The service is re-slicing
process-global sys.argv when constructing service_argv so "service debug" yields
["debug"] passed to parser.parse_args; instead persist the already-split runtime
args passed into the Service at construction by storing args on the instance
(e.g. self._runtime_args) in __init__ and change the code that builds
service_argv / calls parser.parse_args (the block that currently slices sys.argv
and sets service_argv before parser.parse_args) to use self._runtime_args rather
than sys.argv[2:], and ensure the thread worker/stop logic still uses the saved
tuple type.
| if self._worker is not None: | ||
| self._worker.join(timeout=30) | ||
| if self._worker.is_alive(): | ||
| servicemanager.LogErrorMsg(f"{APP_NAME}: worker did not stop within 30s; SCM will force-terminate") |
There was a problem hiding this comment.
The 30-second stop timeout never releases SvcDoRun().
Line 57 only times out the control-thread join. The main service thread is still blocked on Line 99's unbounded self._worker.join(), so a stuck worker leaves the service hung during stop instead of letting the timeout path take effect. SvcDoRun() needs bounded waits or a second signal so the timeout branch can actually exit.
Also applies to: 97-99
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@template/`{% if has_backend %}backend{% endif
%}/src/backend_api/entrypoint/{% if install_as_windows_service
%}win_service.py{% endif %} around lines 56 - 59, SvcDoRun currently calls
self._worker.join() unbounded (and also does a single join(timeout=30) on the
control thread), so if the worker blocks the service never returns to let SCM
force-stop; change the stop/wait logic to use bounded waits and polling: when
stopping, signal the worker to stop, then repeatedly call
self._worker.join(timeout=1) (or loop checking self._worker.is_alive() with
short timeout sleeps) up to the total desired timeout (e.g., 30s) so the method
can return when the timeout expires; update both the control-thread join and the
main worker join sites (references: SvcDoRun, self._worker.join,
self._worker.is_alive) to use this bounded/polled join approach or use a second
event to wake the main thread so the timeout branch can exit.
| # Remove crash dump before CI artifact upload so passing tests don't litter the upload. | ||
| if crash_dump_path.exists(): | ||
| crash_dump_path.unlink() |
There was a problem hiding this comment.
Don’t delete crash dumps in fixture teardown; it hides failure evidence.
This cleanup runs even when tests fail, so CI post-failure diagnostics can lose the file you intend to inspect.
Suggested fix
- # Remove crash dump before CI artifact upload so passing tests don't litter the upload.
- if crash_dump_path.exists():
- crash_dump_path.unlink()
_uninstall_service()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Remove crash dump before CI artifact upload so passing tests don't litter the upload. | |
| if crash_dump_path.exists(): | |
| crash_dump_path.unlink() | |
| _uninstall_service() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@template/`{% if has_backend %}backend{% endif %}/tests/{% if
install_as_windows_service %}windows_service{% endif
%}/test_service_lifecycle.py around lines 168 - 170, The teardown currently
unconditionally deletes crash_dump_path (crash_dump_path.exists() /
crash_dump_path.unlink()), which removes failure evidence; change the fixture
finalizer to only remove the crash dump when the test passed (e.g., check the
pytest outcome via request.node.rep_call.passed in the fixture finalizer) or
remove the unlink call entirely so CI can preserve dumps for failed runs—locate
the fixture finalizer where crash_dump_path.unlink() is called and wrap the
unlink in a conditional that only runs on successful test completion.
| result = _run_sc("query", APP_NAME, check=True) | ||
|
|
||
| output = result.stdout.decode() | ||
| assert "STOPPED" in output, f"service not in STOPPED state; sc query output:\n{output}" | ||
| assert _ERROR_SERVICE_SPECIFIC_ERROR in output, ( | ||
| f"expected WIN32_EXIT_CODE {_ERROR_SERVICE_SPECIFIC_ERROR} (ERROR_SERVICE_SPECIFIC_ERROR) after worker crash;\n" | ||
| f"sc query output:\n{output}" | ||
| ) |
There was a problem hiding this comment.
Poll SCM state before asserting STOPPED/1066 to avoid flaky failures.
sc query is asynchronous to service teardown; a single read can catch STOP_PENDING/transient output even when crash handling is correct.
Suggested stabilization
- result = _run_sc("query", APP_NAME, check=True)
-
- output = result.stdout.decode()
- assert "STOPPED" in output, f"service not in STOPPED state; sc query output:\n{output}"
+ deadline = time.monotonic() + _STOP_TIMEOUT
+ output = ""
+ while time.monotonic() < deadline:
+ result = _run_sc("query", APP_NAME, check=True)
+ output = result.stdout.decode()
+ if "STOPPED" in output:
+ break
+ time.sleep(0.5)
+ else:
+ pytest.fail(f"service not in STOPPED state within {_STOP_TIMEOUT}s; sc query output:\n{output}")
+
assert _ERROR_SERVICE_SPECIFIC_ERROR in output, (
f"expected WIN32_EXIT_CODE {_ERROR_SERVICE_SPECIFIC_ERROR} (ERROR_SERVICE_SPECIFIC_ERROR) after worker crash;\n"
f"sc query output:\n{output}"
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@template/`{% if has_backend %}backend{% endif %}/tests/{% if
install_as_windows_service %}windows_service{% endif
%}/test_service_lifecycle.py around lines 187 - 194, The test reads sc query
once which can hit a transient STOP_PENDING state; change the assertions in
test_service_lifecycle.py to poll _run_sc("query", APP_NAME, check=True) in a
short retry loop (e.g., sleep between attempts, stop after a small timeout or
max attempts) until the output contains "STOPPED" (or timeout/fail), then assert
that _ERROR_SERVICE_SPECIFIC_ERROR appears in the final output; use a local
max_wait_seconds/max_attempts variable and short sleep to avoid flakes while
keeping the existing symbols (_run_sc, APP_NAME, _ERROR_SERVICE_SPECIFIC_ERROR)
and semantics.
2 participants
Why is this change necessary?
Need to be able to run as a service, so automatically restarts when computer reboots
How does this change address the issue?
Breaks apart the
cli.pyfile (and its associated tests) into separate modules for easier in/exclusion of Windows Service functionality via copier template.Adds some more explicit starting and stopping handling of uvicorn itself when app is launched from the entrypoint (needed so Windows Service manager can send the app signals).
If cli receives a Windows Service command (e.g.
service installservice start...) then it will use the pywin32 functionality to create a Windows service. the executable can still be launched standalone with its regular CLI args.service installcan accept the other CLI args after a--What side effects does this change have?
N/A
How is this change tested?
Unit and E2E Smoke Tests of service functionality in CI. In downstream repos, two different executable repos and one that doesn't do that (to confirm no spurious things being pulled in)
Other
Pulled in a few upstream copier template changes
Summary by CodeRabbit
New Features
Chores