Feat/logservice#132
Open
Fabito97 wants to merge 4 commits into
Open
Conversation
|
Warning Review limit reached
More reviews will be available in 20 minutes and 48 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ 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: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR introduces logservice, a new FastAPI microservice that streams real-time logs from PM2-managed processes via Server-Sent Events. The service validates process existence, handles client disconnections gracefully, and formats output as either raw text or JSON. Deployment is integrated into the existing workflow with environment setup and automated PM2 lifecycle management. ChangesLogservice Log Streaming Feature
Sequence DiagramsequenceDiagram
participant Client
participant FastAPI
participant PM2
participant PM2Logs as pm2 logs subprocess
Client->>FastAPI: GET /logs/{service}?lines=N&output_format=text
FastAPI->>PM2: pm2 describe service
PM2-->>FastAPI: process metadata or error
FastAPI->>PM2Logs: spawn pm2 logs process
loop Stream until disconnect
PM2Logs-->>FastAPI: log line from stdout/stderr
FastAPI->>FastAPI: format as SSE text or JSON
FastAPI-->>Client: SSE data frame
end
Client-xClient: disconnect
FastAPI->>PM2Logs: terminate subprocess
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…to feat/logservice
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/deploy.yml (1)
220-244: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueLogservice is not stopped during rollback.
When the health check fails (Lines 220-244), the rollback deletes
$API_NAMEand$WORKER_NAMEbut not$LOGSERVICE_NAME. This leaves logservice running while API/worker are rolled back, which may cause inconsistent state if logservice depends on a specific version of the other services.♻️ Proposed fix to include logservice in rollback
if [ "$HEALTHY" = "false" ]; then echo "Health check failed. Rolling back..." pm2 delete $API_NAME || true pm2 delete $WORKER_NAME || true + pm2 delete $LOGSERVICE_NAME || true🤖 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 @.github/workflows/deploy.yml around lines 220 - 244, The rollback sequence fails to stop/restart the log service; add pm2 delete $LOGSERVICE_NAME || true alongside the existing pm2 delete $API_NAME and pm2 delete $WORKER_NAME, and then restart the log service after restoring backups using the same pm2 start pattern used for API/WORKER (i.e., a pm2 start invocation that names the process "$LOGSERVICE_NAME" and uses the appropriate start command/--cwd), and include "$LOGSERVICE_NAME" in the pm2 save step and in backup cleanup so the logservice is returned to the previous version as part of the rollback.
🤖 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 @.github/workflows/deploy.yml:
- Around line 178-180: Remove the extra consecutive blank lines in the GitHub
Actions workflow so there are no more than two consecutive empty lines in
deploy.yml; open the workflow file and collapse the three consecutive blank
lines down to at most two (ensure surrounding YAML structure/indentation is
unchanged).
- Around line 175-177: Replace the direct system installation command pip3
install -r "$LOGSERVICE_DIR/requirements.txt" with a user-scoped or isolated
install; either add the --user flag to pip3 install (e.g., pip3 install --user
-r "$LOGSERVICE_DIR/requirements.txt") or create/activate a virtual environment
first and then install into it (ensure the deploy step creates a venv, activates
it, and runs pip3 install -r "$LOGSERVICE_DIR/requirements.txt" inside that
venv) so the workflow no longer requires root or pollutes the system Python
environment.
In `@logservice/main.py`:
- Around line 10-29: The validate_pm2_process function currently passes the
service parameter directly to subprocess calls; add an explicit allowlist check
at the start of validate_pm2_process (and the other call site that uses
asyncio.create_subprocess_exec) to only permit the four known services:
"api-staging", "api-production", "worker-staging", "worker-production"; if
service is not one of these, raise an HTTPException (400 or 404 as appropriate)
before calling subprocess.run or asyncio.create_subprocess_exec to prevent
injection or unexpected names.
- Around line 19-23: The except FileNotFoundError block that raises
HTTPException should capture the original exception and chain it so the
traceback is preserved: change the handler to bind the FileNotFoundError (e.g.,
"except FileNotFoundError as err:") and re-raise the HTTPException using "raise
HTTPException(... ) from err" so the original error is attached to the
HTTPException; update the except block that currently does "except
FileNotFoundError: raise HTTPException(...)" to use this pattern.
- Around line 83-90: The SSE response returned by StreamingResponse (wrapping
pm2_log_stream) needs the X-Accel-Buffering: no header to disable proxy
buffering; update the headers dict in the return of StreamingResponse (the block
that constructs the response for pm2_log_stream) to include "X-Accel-Buffering":
"no" alongside the existing "Cache-Control" and "Connection" entries so
nginx/other reverse proxies won’t buffer or delay SSE events.
- Around line 47-55: The loop currently awaits request.is_disconnected() before
calling process.stdout.readline(), but readline() can block indefinitely and
prevent timely disconnection detection; modify the read loop to race or timeout
the readline so you can periodically re-check request.is_disconnected() — e.g.,
create a readline task (process.stdout.readline()) and use asyncio.wait or
asyncio.wait_for with a short timeout to await either the line or a timeout,
then if the timeout occurs re-check request.is_disconnected() and continue, and
when the readline completes handle the line as before; reference
process.stdout.readline and request.is_disconnected in your changes.
In `@logservice/requirements.txt`:
- Around line 1-2: Pin the packages in logservice/requirements.txt to fixed
versions to ensure reproducible deployments: replace the unpinned entries for
fastapi and uvicorn with explicit versions (e.g., fastapi==0.136.3 and
uvicorn==0.49.0) or at minimum pinned major versions (e.g.,
fastapi>=0.136,<0.137) and consider adding a constraints or hash-locked
requirements file for stronger supply-chain integrity.
In `@logservice/start.sh`:
- Around line 2-3: The start.sh currently hardcodes cd /var/www/logservice which
breaks when LOGSERVICE_DIR is set by the deploy workflow; update start.sh to
derive the working dir dynamically (recommended: use the script's directory via
dirname "$0" or respect an environment variable/first argument like
LOGSERVICE_DIR) and remove the hardcoded path; ensure you check cd's return
status and exit with a non‑zero code on failure (handle per shellcheck) before
running exec uvicorn main:app so the service won't start in the wrong directory.
---
Outside diff comments:
In @.github/workflows/deploy.yml:
- Around line 220-244: The rollback sequence fails to stop/restart the log
service; add pm2 delete $LOGSERVICE_NAME || true alongside the existing pm2
delete $API_NAME and pm2 delete $WORKER_NAME, and then restart the log service
after restoring backups using the same pm2 start pattern used for API/WORKER
(i.e., a pm2 start invocation that names the process "$LOGSERVICE_NAME" and uses
the appropriate start command/--cwd), and include "$LOGSERVICE_NAME" in the pm2
save step and in backup cleanup so the logservice is returned to the previous
version as part of the rollback.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e2f1a96-4fd9-46da-a374-95955e68afff
📒 Files selected for processing (4)
.github/workflows/deploy.ymllogservice/main.pylogservice/requirements.txtlogservice/start.sh
…idation and pin dependency versions
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 PR Checklist
Type of Change
Service(s) Affected
logservice(FastAPI log streaming utility)deploy.yml(App deployment script)Description
This PR introduces the initial implementation of the logservice, a FastAPI‑based utility that exposes PM2 logs via HTTP endpoint and integrates it into the deployment script.
Key additions:
main.py: FastAPI app with/logs/{service}endpoints for streaming logs.requirements.txt: Python dependencies (fastapi,uvicorn).start.sh: Script to launch logservice with Uvicorn.Testing
How was this is to be tested?
pip3 install -r requirements.txt../start.sh.api-staging,api-production,worker-staging, andworker-production.Breaking Changes
Security
Reviewer Notes
$DEPLOY_DIR/logservice) matches repo expectations.Summary by CodeRabbit
Release Notes
New Features
Chores