fix: add gateway restart + consolidate reliability/hooks flags onto Typer surface#3164
fix: add gateway restart + consolidate reliability/hooks flags onto Typer surface#3164praisonai-triage-agent[bot] wants to merge 2 commits into
Conversation
…yper surface (fixes #3161) - Add first-class `gateway restart` (daemon-aware graceful drain + relaunch) - Surface reliability/admission/idle/drain/identity flags on Typer `gateway start` - Add `gateway hooks {add,list,remove}` Typer sub-app reusing GatewayHandler.hooks - Resolve pause/resume/reconnect target from the PID lock when --url omitted - Add daemon restart() for systemd/launchd/windows + restart_daemon dispatcher Co-authored-by: MervinPraison <MervinPraison@users.noreply.github.com>
|
@coderabbitai review |
|
/review |
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
✅ Action performedReview finished.
|
MervinPraison
added
pipeline/blocked:ci
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe gateway CLI now exposes production start settings, a daemon-aware restart command, PID-lock-based channel controls, and ChangesGateway CLI enhancements
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant GatewayCLI
participant Daemon
participant GatewayHandler
Operator->>GatewayCLI: gateway restart
GatewayCLI->>Daemon: check status and restart
alt daemon restart unavailable
GatewayCLI->>GatewayHandler: stop with force=False
GatewayCLI->>GatewayHandler: start gateway
end
GatewayCLI-->>Operator: report restart result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR consolidates gateway operations under the Typer CLI. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "fix: apply drain window to restart, reso..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| result = restart_daemon() | ||
| if result.get("ok"): | ||
| output.print_success(result.get("message", "Service restarted")) | ||
| return | ||
| output.print_warning( | ||
| f"Daemon restart unavailable ({result.get('error', 'unknown')}); " | ||
| "falling back to direct drain + relaunch." | ||
| ) |
There was a problem hiding this comment.
If an installed service fails to restart, this branch falls through to a direct PID stop and foreground start. Systemd or launchd can respawn the stopped managed process, leaving it to race the foreground process for the same PID lock and port or to create duplicate channel consumers.
|
@claude You are the FINAL architecture reviewer. If the branch is under MervinPraison/PraisonAI (not a fork), you are able to make modifications to this branch and push directly. SCOPE: Focus ONLY on Python packages (praisonaiagents, praisonai). Do NOT modify praisonai-rust or praisonai-ts. Read ALL comments above from Gemini, Qodo, CodeRabbit, and Copilot carefully before responding. Phase 1: Review per AGENTS.md
Phase 2: FIX Valid Issues Phase 3: Final Verdict |
MervinPraison
added
pipeline/awaiting-merge-gate
|
Claude finished @MervinPraison's task —— View job Final Architecture Review — PR #3164 ✅Phase 1 — Architecture (AGENTS.md): PASS Phase 2 — Valid issues fixed (pushed to this branch):
Files modified:
Assessed but intentionally NOT changed (avoiding scope creep):
Tests: Verdict: ✅ ApproveBackward-compatible, correctly-layered operator UX with the P1 correctness gaps resolved. No blocking issues remain. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/praisonai-bot/tests/unit/cli/test_gateway_install.py (1)
130-159: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover restart failure and exit-code propagation.
Add cases where
restart_daemon()returnsok=Falseand where directGatewayHandler.start()returns75or78. Assert that the command exits nonzero rather than reporting a successful restart.🤖 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 `@src/praisonai-bot/tests/unit/cli/test_gateway_install.py` around lines 130 - 159, Add failure-path tests for the restart command alongside test_gateway_restart_daemon_aware and test_gateway_restart_direct_when_no_daemon: verify an ok=False result from restart_daemon produces a nonzero CLI exit, and verify direct GatewayHandler.start() return codes 75 and 78 are propagated as nonzero exits rather than reported as success.
🤖 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 `@src/praisonai-bot/praisonai_bot/cli/commands/gateway.py`:
- Around line 204-206: Propagate the drain_timeout value from the gateway
command into the shutdown path before relaunching: update the daemon/direct-stop
contracts and the call sites around restart_daemon() and handler.stop() so both
receive and honor the configured timeout. Preserve the existing
replacement-process configuration while ensuring --drain-timeout controls
draining of the current gateway.
- Around line 147-163: Capture the return value from GatewayHandler.start() in
both gateway command paths and return that exit code from their command
functions instead of discarding it. Preserve the existing handler.start
arguments and ensure documented 0, 75, and 78 results reach the CLI process.
- Around line 242-250: Update the installed-service branch in the gateway
command to report the restart error and exit nonzero when restart_daemon()
returns an unsuccessful result. Remove the direct drain/relaunch fallback from
this path, while preserving direct fallback only when daemon_status indicates no
service is installed.
In `@src/praisonai-bot/praisonai_bot/daemon/systemd.py`:
- Around line 99-108: Bound all daemon restart subprocesses and return a
controlled failure when they exceed the timeout: in
src/praisonai-bot/praisonai_bot/daemon/systemd.py lines 99-108, update the
systemctl invocation in the restart flow and handle subprocess.TimeoutExpired;
in src/praisonai-bot/praisonai_bot/daemon/launchd.py lines 115-129, add timeouts
to the kickstart, unload, and load subprocesses; in
src/praisonai-bot/praisonai_bot/daemon/windows.py lines 208-225, add timeouts to
the query, end, and run subprocesses. Preserve the existing success and other
error responses while providing an appropriate timeout error response.
---
Nitpick comments:
In `@src/praisonai-bot/tests/unit/cli/test_gateway_install.py`:
- Around line 130-159: Add failure-path tests for the restart command alongside
test_gateway_restart_daemon_aware and
test_gateway_restart_direct_when_no_daemon: verify an ok=False result from
restart_daemon produces a nonzero CLI exit, and verify direct
GatewayHandler.start() return codes 75 and 78 are propagated as nonzero exits
rather than reported as success.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 591bab66-a87d-4417-abe5-e9e973f59755
📒 Files selected for processing (6)
src/praisonai-bot/praisonai_bot/cli/commands/gateway.pysrc/praisonai-bot/praisonai_bot/daemon/__init__.pysrc/praisonai-bot/praisonai_bot/daemon/launchd.pysrc/praisonai-bot/praisonai_bot/daemon/systemd.pysrc/praisonai-bot/praisonai_bot/daemon/windows.pysrc/praisonai-bot/tests/unit/cli/test_gateway_install.py
| handler.start( | ||
| host=host, | ||
| port=port, | ||
| agent_file=agents, | ||
| config_file=config, | ||
| openai_api=True if openai_api else None, | ||
| mcp=True if mcp else None, | ||
| drain_timeout=drain_timeout, | ||
| max_concurrent_runs=max_concurrent_runs, | ||
| queue_depth=queue_depth, | ||
| overflow_policy=overflow_policy, | ||
| reliability=reliability, | ||
| identity_store=identity_store, | ||
| scale_to_zero=True if scale_to_zero else None, | ||
| idle_minutes=idle_minutes, | ||
| drain_marker=drain_marker, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Propagate GatewayHandler.start() exit codes.
Both commands discard the documented 0/75/78 result, so fatal configuration or transient startup failures are reported by the CLI as success.
Proposed fix
- handler.start(
+ code = handler.start(
...
)
+ raise typer.Exit(code)Also applies to: 258-264
🤖 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 `@src/praisonai-bot/praisonai_bot/cli/commands/gateway.py` around lines 147 -
163, Capture the return value from GatewayHandler.start() in both gateway
command paths and return that exit code from their command functions instead of
discarding it. Preserve the existing handler.start arguments and ensure
documented 0, 75, and 78 results reach the CLI process.
| drain_timeout: float = typer.Option( | ||
| 10.0, "--drain-timeout", | ||
| help="Seconds to wait for in-flight agent turns to finish before relaunch", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Apply --drain-timeout to the shutdown phase.
The option is never passed to restart_daemon() or handler.stop(); it only configures the replacement process. Consequently, restart --drain-timeout 30 cannot control how long the existing gateway drains. Plumb this value through the daemon/direct-stop contracts before relaunching.
Also applies to: 242-264
🤖 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 `@src/praisonai-bot/praisonai_bot/cli/commands/gateway.py` around lines 204 -
206, Propagate the drain_timeout value from the gateway command into the
shutdown path before relaunching: update the daemon/direct-stop contracts and
the call sites around restart_daemon() and handler.stop() so both receive and
honor the configured timeout. Preserve the existing replacement-process
configuration while ensuring --drain-timeout controls draining of the current
gateway.
| if daemon_status.get("installed"): | ||
| result = restart_daemon() | ||
| if result.get("ok"): | ||
| output.print_success(result.get("message", "Service restarted")) | ||
| return | ||
| output.print_warning( | ||
| f"Daemon restart unavailable ({result.get('error', 'unknown')}); " | ||
| "falling back to direct drain + relaunch." | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not directly relaunch after an installed service restart fails.
At this point the service remains managed and may still be running or automatically relaunch. Starting an additional foreground gateway can create duplicate processes or port conflicts. Report the daemon error and exit nonzero; direct fallback should be limited to cases where no service is installed.
🤖 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 `@src/praisonai-bot/praisonai_bot/cli/commands/gateway.py` around lines 242 -
250, Update the installed-service branch in the gateway command to report the
restart error and exit nonzero when restart_daemon() returns an unsuccessful
result. Remove the direct drain/relaunch fallback from this path, while
preserving direct fallback only when daemon_status indicates no service is
installed.
| try: | ||
| subprocess.run( | ||
| ["systemctl", "--user", "restart", SERVICE_NAME], | ||
| check=True, capture_output=True, | ||
| ) | ||
| return {"ok": True, "message": f"Service restarted: {SERVICE_NAME}"} | ||
| except subprocess.CalledProcessError as e: | ||
| return {"ok": False, "error": f"systemctl error: {e.stderr.decode()[:300] if e.stderr else str(e)}"} | ||
| except FileNotFoundError: | ||
| return {"ok": False, "error": "systemctl not found. Is systemd available?"} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound daemon restart subprocesses.
All platform implementations can leave gateway restart hanging indefinitely when the service manager stalls.
src/praisonai-bot/praisonai_bot/daemon/systemd.py#L99-L108: add a timeout tosystemctland handleTimeoutExpired.src/praisonai-bot/praisonai_bot/daemon/launchd.py#L115-L129: add timeouts to kickstart, unload, and load.src/praisonai-bot/praisonai_bot/daemon/windows.py#L208-L225: add timeouts to query, end, and run.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 99-102: Command coming from incoming request
Context: subprocess.run(
["systemctl", "--user", "restart", SERVICE_NAME],
check=True, capture_output=True,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.21)
[error] 100-100: subprocess call: check for execution of untrusted input
(S603)
[error] 101-101: Starting a process with a partial executable path
(S607)
📍 Affects 3 files
src/praisonai-bot/praisonai_bot/daemon/systemd.py#L99-L108(this comment)src/praisonai-bot/praisonai_bot/daemon/launchd.py#L115-L129src/praisonai-bot/praisonai_bot/daemon/windows.py#L208-L225
🤖 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 `@src/praisonai-bot/praisonai_bot/daemon/systemd.py` around lines 99 - 108,
Bound all daemon restart subprocesses and return a controlled failure when they
exceed the timeout: in src/praisonai-bot/praisonai_bot/daemon/systemd.py lines
99-108, update the systemctl invocation in the restart flow and handle
subprocess.TimeoutExpired; in src/praisonai-bot/praisonai_bot/daemon/launchd.py
lines 115-129, add timeouts to the kickstart, unload, and load subprocesses; in
src/praisonai-bot/praisonai_bot/daemon/windows.py lines 208-225, add timeouts to
the query, end, and run subprocesses. Preserve the existing success and other
error responses while providing an appropriate timeout error response.
…rd Windows restart (#3161) Addresses reviewer (Greptile P1) findings on PR #3164: - restart --drain-timeout now flows into stop()/graceful-stop so the OLD process gets the full drain window instead of a fixed 10s cut-off. - pause/resume/reconnect + URL resolution now honour --host/--port / GATEWAY_PORT so a gateway on a non-default endpoint is located instead of always probing 127.0.0.1:8765. - Windows daemon restart aborts if `schtasks /End` fails, preventing a duplicate/colliding gateway relaunch. - Documented that the direct (non-service) restart fallback cannot recover CLI-only launch flags; service/YAML is the production path. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
MervinPraison
added
pipeline/blocked:stale-final
praisonai-triage-agent
Bot
removed
the
pipeline/blocked:cooldown
1 participant
Fixes #3161
Summary
Completes and consolidates the
praisonai gatewayCLI so operators have one canonical, discoverable surface — no more hidden second parser and no more hand-copied OS restart commands.gateway restart— new first-class command performing a graceful drain + relaunch. Daemon-aware: delegates to the installed service (launchdkickstart -k/systemctl --user restart/schtasks /End+/Run) when present, otherwise drains the running PID and relaunches directly.gateway startflags — the reliability/admission/idle/drain/identity flags that previously lived only on the argparse parser are now visible in Typergateway start --help:--drain-timeout,--max-concurrent-runs,--queue-depth,--overflow-policy,--reliability,--identity-store,--scale-to-zero,--idle-minutes,--drain-marker.Nonestill means "fall back to YAML".gateway hooks {add,list,remove}— now a discoverable Typer sub-app, reusing the existingGatewayHandler.hooks()implementation.pause/resume/reconnect— resolve the running gateway from the PID lock (host+port) when--urlis omitted, instead of forcing a hand-typed WebSocket URL.Layer placement
Pure wrapper (
praisonai-bot) CLI/operator UX. The underlying capabilities (drain, admission, reliability presets, restart-intent exit codes, hooks) already exist in core/runtime andGatewayHandler— this only exposes them on the canonical parser and adds the missingrestartverb. No new core params, no new dependencies, noagent.pychanges.Test plan
restartregistered, daemon-aware path, direct-relaunch fallback,hookssub-app registration + delegation.gateway start --helpexposes all production flags;gateway --helplistsrestartandhooks.praisonaiwrapper-import tests).Generated with Claude Code
Summary by CodeRabbit
New Features
gateway restartwith daemon-aware restart and foreground fallback.gateway hooks add,list, andremovecommands.Tests