fix: update FastAPI command for Windows compatibility #507
Conversation
WalkthroughStart-up logic for the FastAPI/sync microservice was revised to use OS-specific commands (Windows uses uvicorn with host 127.0.0.1:8001 and --reload), introduce host/port variables, log the exact command/PID/URL, remove a debug print, preserve non-blocking subprocess.Popen startup, and ensure original CWD is restored on success or error. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Microservice as microservice.py
participant OS as subprocess / shell
participant FastAPI as Service
Caller->>Microservice: request start (dev/frozen/fastapi)
Microservice->>Microservice: resolve paths, venv (if dev), host/port
Microservice->>Microservice: pick OS-specific command (uvicorn on Windows)
Microservice->>OS: Popen(cmd..., cwd=target_dir, ...)
Microservice->>Microservice: logger.info(command), logger.info(PID/URL)
OS-->>FastAPI: process starts (non-blocking)
Microservice->>Microservice: restore original CWD (success/error)
Microservice-->>Caller: return process handle / info
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
✨ Finishing touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (2)
backend/app/utils/microservice.py (2)
242-242: Log the args list instead of a space-joined string.Safer and clearer for paths with spaces; also avoids unnecessary string building.
- logger.info(f"Executing command: {' '.join(cmd)}") + logger.info("Executing command: %s", cmd)
245-245: Optional: avoid chdir by using Popen(cwd=...).Keeps the function side-effect free and removes the need to restore cwd.
- process = subprocess.Popen(cmd) + process = subprocess.Popen(cmd, cwd=str(service_path))Additionally remove the
os.chdir(service_path)and the subsequent restore. For example:# Outside the selected range; illustrative: original_cwd = os.getcwd() # remove os.chdir(service_path) # remove ... os.chdir(original_cwd) # remove
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/utils/microservice.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tauri Tests
🔇 Additional comments (1)
backend/app/utils/microservice.py (1)
235-237: No action required — uvicorn present in sync-microservice requirements.uvicorn==0.35.0 and fastapi==0.116.1 are declared in sync-microservice/requirements.txt (also present in backend/requirements.txt).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/app/utils/microservice.py (3)
257-261: Add --host and reuse port var for non-Windows for parity and least exposure.Explicitly bind to 127.0.0.1 (as in Windows branch) and reuse the host/port vars.
- cmd = [str(python_executable), "-m", "fastapi", "dev", "--port", "8001"] + cmd = [ + str(python_executable), + "-m", + "fastapi", + "dev", + "--host", + host, + "--port", + port, + ]
82-88: Same stdout/stderr pipe issue in frozen mode.Inherit stdio or silence; don’t leave pipes unread.
- process = subprocess.Popen( - [str(sync_executable)], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE, - text=True, - cwd=str(sync_dir), # Set working directory to sync service directory - ) + process = subprocess.Popen([str(sync_executable)], cwd=str(sync_dir))
264-266: Don’t pipe stdout/stderr without consumers; risk of deadlock/blocking.The child can block when pipe buffers fill. Inherit parent stdio (default) or use DEVNULL.
- process = subprocess.Popen( - cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, text=True - ) + process = subprocess.Popen(cmd, cwd=str(service_path))
🧹 Nitpick comments (4)
backend/app/utils/microservice.py (4)
235-237: Avoid process-wide chdir; pass cwd to Popen instead.Reduces side effects and race potential in multi-threaded contexts.
- original_cwd = os.getcwd() - os.chdir(service_path) + # Launch with cwd set in Popen to avoid global chdir @@ - # Restore original working directory - - os.chdir(original_cwd) + # cwd provided via Popen; no restoration needed @@ - # Restore original working directory in case of error - - try: - os.chdir(original_cwd) - except Exception: - pass + # No global cwd change to revertAlso applies to: 268-271, 280-284
272-274: Log dynamic URL using host/port variables.Keeps logs consistent if config changes.
- logger.info("Service should be available at http://localhost:8001") + logger.info(f"Service should be available at http://{host}:{port}")
262-266: Optional: verify service readiness before returning True.Consider a short retry loop to check the port is open/alive; avoids false “started” signals if the child crashes immediately.
Example (outside this hunk):
import socket, time def _wait_for_port(host: str, port: int, timeout_s: int = 10) -> bool: deadline = time.time() + timeout_s while time.time() < deadline: with socket.socket() as s: s.settimeout(0.5) if s.connect_ex((host, port)) == 0: return True time.sleep(0.2) return FalseThen call
_wait_for_port(host, int(port))and log success/failure.Also applies to: 272-275
136-141: Proceeding after failed dependency install may just defer failure.If requirements installation fails, consider surfacing failure early or at least warn with actionable hint (e.g., “ensure fastapi[standard] or uvicorn is installed”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app/utils/microservice.py(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tauri Tests
🔇 Additional comments (3)
backend/app/utils/microservice.py (3)
242-256: Windows path: Good switch to uvicorn with localhost and reload.This addresses the Windows incompatibility and aligns dev ergonomics.
182-190: Venv Python resolution looks correct across OSes.
139-141: Verified: FastAPI (>=0.100) and uvicorn present in requirementsrequirements file /home/jailuser/git/sync-microservice/requirements.txt contains fastapi==0.116.1, fastapi-cli==0.0.8, fastapi-cloud-cli==0.1.5, uvicorn==0.35.0, watchfiles==1.1.0 — no changes required.
|
Thank you @Hemil36! |
2 participants
Fixes : #506
This pull request updates the logic for starting the FastAPI development server in the
_start_fastapi_servicefunction to support both Windows and non-Windows platforms. The most important change is the introduction of platform-specific commands to ensure compatibility.Platform compatibility improvements:
_start_fastapi_serviceinbackend/app/utils/microservice.pyto use a different command for Windows—runninguvicorndirectly instead offastapi dev—to properly start the FastAPI app across operating systems.Summary by CodeRabbit
Bug Fixes
Chores