fix(gateway): unbreak daemon after onboard — remove duplicate block + guard push attrs#1484
Conversation
…ock + guard push attrs Two root causes for 'praisonai gateway status' showing the launchd daemon running while port 8765 refuses connections after curl | bash onboarding in v4.6.22: 1. server.py had 84 lines of duplicated/malformed code appended to start_with_config(), causing IndentationError at module import time. The launchd service crash-looped (KeepAlive=true) and filled bot-stderr.log with ~68MB of tracebacks. Removed the duplicated tail so the method ends correctly at stop_channels(). 2. /health references self._push_enabled / _channel_mgr / _presence_mgr / _redis_pubsub, but PR #1483 moved push infra out of the core gateway. Once the daemon stayed up, the first health probe raised AttributeError: 'WebSocketGateway' object has no attribute '_push_enabled', producing HTTP 500. Guard every push attribute access with getattr so health reports cleanly when push is disabled. Verified: fresh launchd kickstart, curl http://127.0.0.1:8765/health returns {"status": "healthy", ...}; praisonai gateway status reports 'Gateway Status: healthy'.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe gateway server module was refactored to avoid attribute errors and eliminate duplicated shutdown scaffolding. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@copilot Do a thorough review of this PR. Read ALL existing reviewer comments above from Qodo, Coderabbit, and Gemini first — incorporate their findings. Review areas:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of the health endpoint in server.py by implementing defensive attribute access using getattr and removes a significant amount of redundant code. A review comment suggests extending this defensive approach to the internal _client attribute of the Redis pub-sub manager to maintain consistency and prevent potential attribute errors.
| push_status["online_clients"] = presence_mgr.get_online_count() | ||
| redis_pubsub = getattr(self, "_redis_pubsub", None) | ||
| if redis_pubsub is not None: | ||
| push_status["redis_connected"] = redis_pubsub._client is not None |
There was a problem hiding this comment.
To be truly defensive as noted in the comment on line 963, it is safer to use getattr when accessing the private _client attribute of redis_pubsub. Since these attributes are dynamically injected by a wrapper and are considered "stale" in the core gateway, their internal structure might not be guaranteed. Accessing a private attribute directly could lead to an AttributeError if the implementation of the injected object changes or if it differs from the expected RedisPubSubAdapter type.
| push_status["redis_connected"] = redis_pubsub._client is not None | |
| push_status["redis_connected"] = getattr(redis_pubsub, "_client", None) is not None |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/praisonai/praisonai/gateway/server.py (1)
963-977: Defensivegetattrguards look correct and fix the reported AttributeError.The block correctly short-circuits when
_push_enabledis absent or falsy, and each sub-manager is fetched with a safe default before use. This matches the stated fix for the daemon returning HTTP 500 on/healthafter push wrappers were extracted in#1483, and preserves the prior shape ofresult["push"]when push infra is actually wired in.One minor note (pre-existing, not introduced here): line 974 still reaches into
redis_pubsub._client, a private attribute of the pubsub object. If a future refactor of that class renames_client,/healthwill regress again the same way it did here. Consider exposing a small public accessor (e.g.is_connected()) on the pubsub wrapper in a follow-up so the gateway doesn't depend on its internals.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/praisonai/praisonai/gateway/server.py` around lines 963 - 977, The health-check currently inspects redis_pubsub._client (alongside _push_enabled, _channel_mgr.list_channels() and _presence_mgr.get_online_count()) which works now but relies on a private attribute; update the code to call a public accessor on the pubsub wrapper instead: add/use a method like is_connected() on the pubsub class and replace the check redis_pubsub._client is not None with redis_pubsub.is_connected() (or, if is_connected() doesn’t exist yet, add it to the pubsub wrapper to return connection status), keeping the rest of the logic that builds result["push"] unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/praisonai/praisonai/gateway/server.py`:
- Around line 963-977: The health-check currently inspects redis_pubsub._client
(alongside _push_enabled, _channel_mgr.list_channels() and
_presence_mgr.get_online_count()) which works now but relies on a private
attribute; update the code to call a public accessor on the pubsub wrapper
instead: add/use a method like is_connected() on the pubsub class and replace
the check redis_pubsub._client is not None with redis_pubsub.is_connected() (or,
if is_connected() doesn’t exist yet, add it to the pubsub wrapper to return
connection status), keeping the rest of the logic that builds result["push"]
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a5e9c8a5-8bc7-46d6-ab81-22847c1a3098
📒 Files selected for processing (1)
src/praisonai/praisonai/gateway/server.py
|
@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 |
|
Claude finished @MervinPraison's task —— View job Final Architecture ReviewPhase 1: Architecture Review per AGENTS.md
Phase 2: Fix Valid Issues from Reviewers
Phase 3: Final Verdict
Architecture Review Results✅ Protocol-Driven Core Compliance: The gateway module uses defensive attribute checking with ✅ Backward Compatibility: The fix maintains backward compatibility by preserving the same health response structure when push infrastructure is present. ✅ Performance Impact: No performance regressions - the defensive Issue Analysis & ResolutionThe PR correctly fixes both identified issues:
Additional Fix Applied:
Files Modified
Final Verdict: ✅ APPROVEDThe PR successfully resolves the daemon crash loop and HTTP 500 health endpoint issues. The additional defensive guard ensures robustness against future Redis adapter implementation changes. All architectural guidelines from AGENTS.md are followed. Ready for merge - minimal, focused fix with proper defensive programming patterns. |
Apply defensive getattr() to redis_pubsub._client to maintain consistency with other push attribute guards and prevent future AttributeError if the Redis adapter implementation changes. Addresses feedback from CodeRabbit and Gemini reviewers. Co-authored-by: Mervin Praison <MervinPraison@users.noreply.github.com>
There was a problem hiding this comment.
MervinPraison has reached the 50-review limit for trial accounts. To continue receiving code reviews, upgrade your plan.
Problem
After a fresh
curl -fsSL https://praison.ai/install.sh | bashon v4.6.22, the onboarding wizard completes successfully and installs the launchd daemon, but:Root causes (two)
1.
IndentationErrorcrash loop inpraisonai/gateway/server.pystart_with_config()had its last 28 lines duplicated four times with broken indentation (84 spurious lines, 1809-1892). Python refused to import the module; launchd'sKeepAlive=truecrash-looped the daemon, producing a 68 MBbot-stderr.login minutes. Port 8765 never opened, hence 'Connection refused'.2.
AttributeError: _push_enabled→ HTTP 500Once the daemon stayed up,
/healthstill failed:PR #1483 moved
PushClientand transports out of the core gateway into the wrapper, but left stale references (_push_enabled,_channel_mgr,_presence_mgr,_redis_pubsub) inhealth().Fix (minimal, 1 file)
start_with_config()ends cleanly atawait self.stop_channels().health()withgetattr(self, "_attr", default)so core gateways without push report healthy.Verification
Diff:
1 file changed, 11 insertions(+), 92 deletions(-)Summary by CodeRabbit