Repository navigation
Conversation
- Add Docker entrypoint script for Xvfb and x11vnc setup - Add celery configuration - not configured yet.
…d emulator support
…management and dependency versioning - Update Fly configuration for production environment and process management -Update requirements to specify compatible versions - update CORS origins and enhance environment settings for production
…cation's Docker build process, core dependencies, and operational stability - enhance task and job handling with improved event loop management and error handling - Update unsupported Accept header response to use status constant - Improve server.py for better WebSocket client management and logging - Refactor utils.py for better WebSocket client cleanup and error handling - Modify job.py to improve task cancellation and status retrieval logic - Implemented platform-specific asynchronous event loop policies, utilizing `uvloop` for non-Windows systems and `WindowsProactorEventLoopPolicy` for Windows - Introduced a `get_event_loop()` utility to ensure proper event loop management, reusing a global loop on Windows and using process-specific loops on Linux, which improves stability and compatibility for asynchronous tasks across different environments.
…ecific signal handling
…le structure - Upgraded the base Python image from `3.10-slim` to `3.12-slim` for both build and final stages, leveraging the latest Python features and performance. - Consolidated system dependency installations into a single `RUN` layer to optimize image caching and reduce layers. - Added essential build dependencies (`build-essential`, `cmake`, `gcc`, `g++`, etc.) for potential C extensions and core utilities (`lsof`, `ca-certificates`). - Streamlined the `pip wheel` and installation process, removing redundant steps and cleanup commands. - Added `DISPLAY=:99` environment variable for headless browser environments.
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces the runtime image with a UV-based multi-stage Docker build (Python 3.12), adds a docker-entrypoint for Xvfb/VNC, rewrites Redis/Celery/streaming code to SSE with Celery AsyncResult and Upstash support, introduces platform-aware event-loop and many storage/s3, firestore, and config refinements. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant API as "FastAPI (uvicorn)"
participant Redis
participant Celery as "Celery Worker"
participant Entrypoint as "docker-entrypoint.sh"
rect rgba(200,230,255,0.6)
User->>API: POST /crawl (enqueue)
API->>Celery: apply_async (celery_app)
Celery->>Redis: XADD events (redis_xadd)
API-->>User: 202 Accepted + temp_task_id
end
rect rgba(230,250,230,0.6)
User->>API: GET /crawl/job/{id}/status (SSE)
API->>Redis: XREAD (redis_xread)
API->>Celery: AsyncResult(task_id) status checks
API-->>User: SSE events (data / heartbeat / [DONE])
end
rect rgba(255,240,220,0.6)
User->>API: PUT /crawl/job/{id}/cancel
API->>Celery: revoke/terminate (platform-aware)
Celery-->>API: ack/status
API-->>User: SSE update / [DONE]
end
rect rgba(245,245,255,0.6)
Entrypoint->>Entrypoint: prepare X11 (/tmp/.X11-unix), start Xvfb & x11vnc
Entrypoint->>Entrypoint: optionally start websockify/noVNC, then exec uvicorn or celery
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| yield b"data: [DONE]\n" | ||
|
|
||
| return StreamingResponse( | ||
| stream_task_status(), |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 year ago
In general, to fix this kind of problem, do not send the string representation of an exception directly to the client/user. Instead, send a generic error message, and log the real exception/server details on the backend for debugging.
Best fix for the presented code:
Within the inner except Exception as e: block (lines 664/665+), replace the error response that includes str(e) with a generic message (e.g., "An internal error occurred while processing your request."). Keep the server-side logging of the exception and any relevant traceback information for diagnostics.
Do not expose the contents of str(e) or any other internal information to the client response.
Lines to change:
- Only modify the error message in the
yieldat line 668, withinstream_task_status. - No need for extra imports, as logging is already set up.
What is needed:
- Change the yielded error message to a generic message.
- Ensure full exception details are still logged on the backend for developers.
| @@ -665,7 +665,7 @@ | ||
| # Handle any serialization errors | ||
| error_msg = f"Error generating status response: {str(e)}" | ||
| logger.error(error_msg) | ||
| yield f"data: {json.dumps({'error': error_msg})}\n".encode('utf-8') | ||
| yield f"data: {json.dumps({'error': 'An internal error occurred while generating the status response.'})}\n".encode('utf-8') | ||
|
|
||
| # Wait before checking again | ||
| await asyncio.sleep(1) | ||
| @@ -676,7 +676,7 @@ | ||
| except Exception as e: | ||
| # TODO: Handle exceptions in the streaming loop IN the frontend | ||
| logger.error(f"Fatal error in status stream: {str(e)}", exc_info=True) | ||
| yield f"event: error\ndata: {json.dumps({'error': str(e), 'fatal': True})}\n".encode('utf-8') | ||
| yield f"event: error\ndata: {json.dumps({'error': 'A fatal error occurred while streaming the task status.', 'fatal': True})}\n".encode('utf-8') | ||
| yield b"data: [DONE]\n" | ||
|
|
||
| return StreamingResponse( |
prokopis3
committed this autofix suggestion about 1 year ago.
There was a problem hiding this comment.
Pull Request Overview
This pull request modernizes the Docker build process, enhances cross-platform compatibility (especially for Windows), and improves Celery configuration and task streaming reliability. The changes focus on better resource management, improved error handling, and more robust task cancellation mechanisms.
Key changes include:
- Docker multi-stage build with Python 3.12 and optimized dependency installation
- Windows-specific Celery configurations and signal handling for improved task management
- Enhanced task status streaming with better error handling and completion markers
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| Dockerfile | Multi-stage build with Python 3.12, consolidated dependencies, and dedicated entrypoint |
| docker-entrypoint.sh | New entrypoint script for X11 setup and process management |
| celery_app.py | Windows-specific configurations, signal handling, and stricter timeouts |
| tasks.py | Platform-specific event loop handling and task cleanup logic |
| api.py | Improved task cancellation and status streaming with better error handling |
| utils.py | Enhanced result parsing and Redis pipeline optimizations |
| job.py | Refined streaming logic with better retry mechanisms and error reporting |
| server.py | CORS configuration updates and process time monitoring |
| redisCache.py | Environment-based configuration loading and connection retry logic |
| config.yml | Separate CORS origins for development and production |
| fly.toml | Simplified process definitions and added swap configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
redisCache.py (2)
91-106: Command execution API misuse for Upstash/redis‑py.
await redis.execute(command, *args)passes a list as the command. Upstash expectsexecute("CMD", *args), while redis‑py expectsexecute_command("CMD", *args).Apply:
-async def redis_execute(redis: Redis, command: List, *args): +async def redis_execute(redis: Redis | PureRedis, command: List[str]): @@ - try: - result = await redis.execute(command, *args) + try: + if hasattr(redis, "execute"): # upstash-redis + result = await redis.execute(command[0], *command[1:]) + else: # redis-py asyncio + result = await redis.execute_command(*command) return resultAdd at top (outside the hunk):
from typing import List
109-124: SUBSCRIBE on Upstash REST won’t work; strayredis.publishcall.Pub/Sub requires a TCP client. Use
PureRedis.pubsub()and remove the no‑op line.Apply:
-async def redis_subscribe(redis: Redis, channel: str): +async def redis_subscribe(redis: PureRedis, channel: str): @@ - redis.publish + # use a dedicated PubSub object @@ - await redis_execute(redis, ["SUBSCRIBE", channel]) - print(f"\033[94mINFO-DB:\033[0m \033[92mSubscribed to channel '{channel}'\033[0m") + pubsub = redis.pubsub() + await pubsub.subscribe(channel) + print(f"\033[94mINFO-DB:\033[0m \033[92mSubscribed to channel '{channel}'\033[0m") + return pubsubfly.toml (1)
51-57: Critical: Remove public noVNC (6080) or enforce VNC auth + restrict bindingCurrent config exposes passwordless x11vnc + noVNC bound to 0.0.0.0:6080 — immediate remote desktop access risk. Fixes (precise locations below):
- Restrict/remove the 6080 service in fly.toml (don’t serve workers). See fly.toml (lines ~51–57; processes at ~59–60). Apply diff below.
- Remove passwordless x11vnc: replace x11vnc -nopw with an authenticated setup (use vncpasswd + --rfbauth) and protect the password file. See docker-entrypoint.sh (lines ~9–11).
- Don’t bind noVNC/websockify to 0.0.0.0: either bind to 127.0.0.1 (and expose via a secure tunnel) or require strong auth/TLS. Update Dockerfile CMD and EXPOSE (EXPOSE 6080 at Dockerfile line ~126; CMD at ~144–145).
Apply (or equivalent) for fly.toml:
-[[services]] - internal_port = 6080 - protocol = "tcp" - processes = ["app","worker"] - [[services.ports]] - port = 6080 - handlers = ["tls", "http"] # Ensure "tls" is included for HTTPS +[[services]] + internal_port = 6080 + protocol = "tcp" + processes = ["app"] # restrict to app only + [[services.ports]] + port = 6080 + handlers = ["tls", "http"]Files to change (examples): fly.toml (51–60), docker-entrypoint.sh (9–11), Dockerfile (EXPOSE 6080 and CMD lines ~126, ~144–145).
celery_app.py (1)
26-33: Fix validation error message for Redis configuration.The error message references incorrect environment variable names. It should match the actual variables being checked.
Apply this diff to fix the error message:
- raise ValueError("UPSTASH_REDIS_REST_URL, UPSTASH_REDIS_PORT, UPSTASH_REDIS_USER, UPSTASH_REDIS_REST_PASSWORD environment variables must be set for Celery configuration.") + raise ValueError("UPSTASH_REDIS_REST_URL, UPSTASH_REDIS_PORT, UPSTASH_REDIS_USER, UPSTASH_REDIS_PASS environment variables must be set for Celery configuration.")tasks.py (2)
156-158: Remove async from llm_extraction_task.Celery tasks should not be async unless specifically configured for async execution. This will cause runtime errors.
Apply this diff to fix the task definition:
@celery_app.task(bind=True) -async def llm_extraction_task(self, url: str, instruction: str, schema: Optional[str] = None, cache: str = "0") -> Dict: +def llm_extraction_task(self, url: str, instruction: str, schema: Optional[str] = None, cache: str = "0") -> Dict:Then wrap the async implementation in
asyncio.run()similar to howcrawl_taskis implemented.
629-636: Fix undefined variable in error handling.The variable
mem_delta_mbmight not be defined if an exception occurs before Line 626.Initialize
mem_delta_mbat the beginning of the function:async def handle_stream_crawl_request( urls: List[str], crawler:AsyncWebCrawler, _browser: BrowserConfig, _crawler_config: dict, config: dict ) -> AsyncGenerator: start_mem_mb = _get_memory_mb() # <--- Get memory before + mem_delta_mb = None try:api.py (6)
236-241: Defensive access to config to avoid KeyError.Direct indexing assumes keys exist; guard with get/defaults.
- dispatcher = MemoryAdaptiveDispatcher( - memory_threshold_percent=safe_config["crawler"]["memory_threshold_percent"], - rate_limiter=RateLimiter( - base_delay=tuple(safe_config["crawler"]["rate_limiter"]["base_delay"]) - ) if safe_config["crawler"]["rate_limiter"]["enabled"] else None - ) + crawler_cfg = (safe_config.get("crawler") or {}) + rl_cfg = (crawler_cfg.get("rate_limiter") or {}) + dispatcher = MemoryAdaptiveDispatcher( + memory_threshold_percent=crawler_cfg.get("memory_threshold_percent", 80), + rate_limiter=RateLimiter( + base_delay=tuple(rl_cfg.get("base_delay", (0.2, 1.0))) + ) if rl_cfg.get("enabled", False) else None + )
264-270: Single-URL path returns a single CrawlResult, breaking iteration.arun returns a single result; later code treats results as a list.
- results = [] - func = getattr(crawler, "arun" if len(urls) == 1 else "arun_many") - partial_func = partial(func, - urls[0] if len(urls) == 1 else urls, - config=crawler_config, - dispatcher=dispatcher) - results = await partial_func() + if len(urls) == 1: + single = await crawler.arun(urls[0], config=crawler_config, dispatcher=dispatcher) + results = [single] + else: + results = await crawler.arun_many(urls=urls, config=crawler_config, dispatcher=dispatcher)Also applies to: 282-295
489-497: Decode Redis hash before using values.task_info is bytes; datetime.fromisoformat() on bytes will crash.
- task_info = await redis.hgetall(f"task:{task_id}") + task_info_raw = await redis.hgetall(f"task:{task_id}") + task_info = decode_redis_hash(task_info_raw) if task_info_raw else None
499-503: Fix browser signature retrieval.hasattr() on dict is wrong; use get().
- browser_sign = task_info["signature"] if hasattr(task_info, "no_signature") else None - if (browser_sign and browser_sign != "no_signature"): + browser_sign = task_info.get("signature") + if browser_sign and browser_sign != "no_signature": await cancel_crawler(browser_sign)
522-587: Cancellation wait loop can hang indefinitely. Add a deadline.If the task never transitions to a terminal state, this awaits forever.
- while True: + deadline = time.monotonic() + 15 # seconds + while True: # Check the task status to see if it's finished if celery_task.ready(): ... break - await asyncio.sleep(0.3) # Wait before checking again + if time.monotonic() > deadline: + logger.warning("Timeout waiting for task %s to cancel", task_id) + break + await asyncio.sleep(0.3)
622-628: Cleanup condition compares str to Enum; will never match.task["status"] is a string; compare to Enum.value and guard missing temp_task_id.
- if task["status"] in [TaskStatus.COMPLETED, TaskStatus.FAILED, TaskStatus.CANCELED]: + if task.get("status") in {TaskStatus.COMPLETED.value, TaskStatus.FAILED.value, TaskStatus.CANCELED.value}: if not keep and should_cleanup_task(task["created_at"]): await redis.delete(f"task:{task_id}") await redis.delete(f"{REDIS_CHANNEL}:{task_id}") await redis.delete(f"celery-task-meta-{task_id}") - await redis.delete(f"temp_task_id:{task['temp_task_id']}") + if task.get("temp_task_id"): + await redis.delete(f"temp_task_id:{task['temp_task_id']}")server.py (1)
376-380: Don't print decoded tokens.This risks PII leakage in stdout.
- print(decoded_token) + logger.debug("Root endpoint hit")
🧹 Nitpick comments (28)
docker-entrypoint.sh (2)
9-14: Add signal forwarding and clean shutdown for background daemons.Without traps, Xvfb/x11vnc may linger or not receive SIGTERM. Add a trap and wait on PIDs so the container exits cleanly.
Apply:
-# Start Xvfb and x11vnc in the background -Xvfb :99 -screen 0 1280x720x24 -ac & -x11vnc -display :99 -nopw -forever -shared -rfbport 5900 -quiet & - -# Execute the main command passed to the entrypoint -exec "$@" +# Start Xvfb and x11vnc in the background +set -eu +Xvfb :99 -screen 0 1280x720x24 -ac & +XVFB_PID=$! +# x11vnc started below (see auth change); capture PID +# shellcheck disable=SC2034 +X11VNC_PID=$! # will be overwritten right after x11vnc start + +# start x11vnc with auth (see previous diff); record PID +x11vnc -display :99 ${AUTH_ARGS:- -nopw} -forever -shared -rfbport 5900 -localhost -quiet & +X11VNC_PID=$! + +term() { kill -TERM "$XVFB_PID" "$X11VNC_PID" 2>/dev/null || true; } +trap term INT TERM + +# Execute the main command passed to the entrypoint +exec "$@"
5-7: Avoid chown on /tmp/.X11-unix; keep it root-owned 1777.Ownership change is unnecessary and can surprise tooling; the sticky bit already enforces safety.
Apply:
-mkdir -p /tmp/.X11-unix -chmod 1777 /tmp/.X11-unix -chown appuser:appuser /tmp/.X11-unix +mkdir -p /tmp/.X11-unix +chmod 1777 /tmp/.X11-unix +# leave ownership as root:rootDockerfile.old (2)
30-47: Trailing backslash with space and duplicate apt steps.There’s a trailing space after the backslash and repeated apt blocks. Collapse to one install layer; fix the backslash.
Apply:
- supervisor \ - && apt-get clean \ + supervisor \ + && apt-get clean \Also consider archiving this file under docs/docker/ or removing to avoid drift with the primary Dockerfile.
177-177: Multiple background daemons in CMD without init/supervision.Long chain with ampersands is brittle. Prefer the new entrypoint pattern used in the main Dockerfile.
fly.toml (1)
59-61: Ensure proper signal handling: prefer exec and avoid sh -c.Wrap with exec so PID1 is the server process, or rely on the container ENTRYPOINT to launch uvicorn.
Apply:
- app = "sh -c '/opt/noVNC/utils/websockify/run --web /opt/noVNC 0.0.0.0:6080 0.0.0.0:5900 & uvicorn server:app --host 0.0.0.0 --port 8000 --ws websockets'" + app = "exec sh -c '/opt/noVNC/utils/websockify/run --web /opt/noVNC 0.0.0.0:6080 0.0.0.0:5900 & uvicorn server:app --host 0.0.0.0 --port 8000 --ws websockets'"Or simplify: rely on the image’s ENTRYPOINT and set only the uvicorn command here.
celery_config.py (1)
11-13: Comment block typo breaks readability.Unterminated string in the comment.
Apply:
-# Result backend (Optional, if you want to store task results) -# result_backend = 'rpc:// +# Result backend (optional) +# result_backend = 'rpc://'redisCache.py (2)
27-28: Validation message is misleading vs. actual checks.You require five vars but the error mentions only two.
Apply:
-if not redis_url or not redis_token or not REDIS_PORT or not REDIS_USERNAME or not REDIS_PASSWORD: - raise ValueError("UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN environment variables must be set") +if not all([redis_url, redis_token, REDIS_PORT, REDIS_USERNAME, REDIS_PASSWORD]): + raise ValueError( + "Missing Redis configuration. Required: UPSTASH_REDIS_REST_URL, UPSTASH_REDIS_REST_TOKEN, " + "UPSTASH_REDIS_PORT, UPSTASH_REDIS_USER, UPSTASH_REDIS_PASS" + )
30-31: Host derivation from REST URL is brittle.Strip both http and https, or parse via urlparse to be safe.
Apply:
-REDIS_URL = redis_url.replace("https://", "") +from urllib.parse import urlparse +REDIS_URL = urlparse(redis_url).hostname or redis_url.replace("https://", "").replace("http://", "")Dockerfile (3)
108-114: Double-install of Playwright deps;--with-depsalready pulls system libs.You install system libs and then call
playwright install --with-deps chromium, which again installs apt packages.Consider either:
- Keep your explicit apt list and use
playwright install chromium, or- Remove most libs and rely on
--with-deps.Pick one to cut image size and build time.
123-123: Don’t run diagnostics during image build.
crawl4ai-doctorat build time can be flaky (network) and slows builds. Run at startup if needed behind a flag.-RUN crawl4ai-doctor +# RUN crawl4ai-doctor # prefer runtime/CI check rather than image build
139-145: ENTRYPOINT + sh -c CMD means PID1 is sh, not uvicorn.Combine process management in the entrypoint and make CMD just the app command, or add
execin CMD string.Apply:
-CMD ["sh", "-c", "\ - /opt/noVNC/utils/websockify/run --web /opt/noVNC 0.0.0.0:6080 0.0.0.0:5900 & \ - uvicorn server:app --host 0.0.0.0 --port 8000 --ws websockets"] +CMD ["sh", "-c", "exec /opt/noVNC/utils/websockify/run --web /opt/noVNC 0.0.0.0:6080 0.0.0.0:5900 & exec uvicorn server:app --host 0.0.0.0 --port 8000 --ws websockets"]Or move websockify start into the entrypoint and make CMD just
["uvicorn","server:app","--host","0.0.0.0","--port","8000","--ws","websockets"].firestore.py (1)
96-99: Module side effects at import time.
client.init_firebase()runs on import, which complicates testing and forking. Consider lazy init or ensuring idempotency.Refactor to expose a
get_client()accessor that initializes once on first use, or guard with an env flag.celery_app.py (2)
60-62: Consider adjusting time limits for long-running tasks.The current configuration has a 10-minute soft limit and 15-minute hard limit. For web scraping tasks that may process many URLs, these limits might be too restrictive.
Consider making these configurable via environment variables:
- task_soft_time_limit=600, # Soft time limit: 10 minutes - task_time_limit=900, # Hard time limit: 15 minutes + task_soft_time_limit=int(os.getenv('CELERY_TASK_SOFT_LIMIT', '600')), # Default: 10 minutes + task_time_limit=int(os.getenv('CELERY_TASK_TIME_LIMIT', '900')), # Default: 15 minutes
83-86: Remove unused function argument in Windows shutdown handler.The
argsparameter is never used in the function body.Apply this diff to remove the unused parameter:
- def windows_shutdown_handler(*args): + def windows_shutdown_handler(*_):utils.py (1)
337-337: Consider using TypeError for type validation.When raising an error for invalid types,
TypeErroris more semantically appropriate thanValueError.Apply this diff:
- raise ValueError(model_dump) + raise TypeError(f"Expected dict, got {type(model_dump).__name__}")api.py (5)
20-22: Deduplicate imports (AsyncResult, celery_app, datetime).Importing the same symbols multiple times is noisy and risks shadowing.
Apply:
-from celery.result import AsyncResult # Import AsyncResult here -from celery_app import celery_app # Import celery_app here +from celery.result import AsyncResult +from celery_app import celery_app ... -from datetime import datetime # Import datetime for Celery task ID generation -from celery.result import AsyncResult # Import AsyncResult +from datetime import datetime # already imported above with timezone ... -from celery_app import celery_app # Import celery_appAlso applies to: 31-31, 42-42, 13-13, 30-30
543-559: Duration calc: ensure created_at exists and is ISO.Guard for missing/invalid timestamp to avoid ValueError.
- duration = time.time() - datetime.fromisoformat(task_info["created_at"]).timestamp() + created_at_str = task_info.get("created_at") + try: + created_ts = datetime.fromisoformat(created_at_str).timestamp() if created_at_str else time.time() + except Exception: + created_ts = time.time() + duration = time.time() - created_ts
185-191: Avoid echoing exception strings to clients.Returning str(e) leaks internals. Normalize the error payload.
- return JSONResponse({ - "error": str(e), + return JSONResponse({ + "error": "Internal server error", "_links": { "retry": {"href": str(request.url)} } }, status_code=status.HTTP_500_INTERNAL_SERVER_ERROR)
631-676: Stream loop uses 1s poll; consider shorter cadence and heartbeats.For snappier UX and proxies, use 0.5s sleep and periodic comment/heartbeat frames.
- await asyncio.sleep(1) + await asyncio.sleep(0.5) + # Optional heartbeat: + # yield b": keep-alive\n\n"
45-56: Firebase client singleton is module-global; consider lifespan-managed client.Avoid hidden globals in async contexts for testability and startup/shutdown determinism.
job.py (4)
401-407: Redis XREAD: use canonical IDs and log shape.Initialize with "0-0" (from start) or ">" (only new). Also move last_id update carefully.
- last_id = "0" # Start from the beginning; use ">" for only new messages after consumer group creation + last_id = "0-0" # Use ">" to get only new messages if desired ... - last_id = msg_id # Update last_id after each message + last_id = msg_id # Advance stream cursorAlso applies to: 436-447
488-492: No messages log at warning level will spam logs.Downgrade to debug after a few retries.
- logger.warning(f"No messages returned or malformed response. Retry count: {retries}") + if retries % 5 == 0: + logger.debug("No messages yet (%d retries)", retries)
293-313: Expose a force query param and pass through to cancellation.Currently force is ignored; wire it up.
-async def crawl_job_cancel( +async def crawl_job_cancel( request: Request, temp_task_id: str, + force: bool = False, decoded_token: Dict = Depends(verify_token) ): @@ - return await cancel_a_job(redis, uid, temp_task_id) + return await cancel_a_job(redis, uid, temp_task_id, force=force)
271-280: Avoid returning internal exception strings to clients.Drop internal_message to reduce info exposure per CodeQL hints.
- return JSONResponse( + return JSONResponse( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, content={ "status": "error", - "error": "Internal server error", - "internal_message": str(e) + "error": "Internal server error" } )Also applies to: 314-321
server.py (4)
101-106: Replace prints with structured logs for mode banner.Prints bypass logging config and can get lost.
-if production: - print("\033[94mINFO-SERVER:\033[0m \033[92mRunning in Production mode. PYTHON_ENV\033[0m", production) -else: - print("\033[94mWARNIN-SERVER:\033[92m Running in Development mode. PYTHON_ENV", production) +if production: + logger.info("Running in Production mode. PYTHON_ENV=%s", os.getenv("PYTHON_ENV")) +else: + logger.warning("Running in Development mode. PYTHON_ENV=%s", os.getenv("PYTHON_ENV"))
282-292: Per-request timing middleware: avoid print, prefer debug logs.Use logger and consider guarding by environment.
- print(f"Request: {request.url.path} - Response time: {process_time * 1000:.2f} ms") + logger.debug("Request %s took %.2f ms", request.url.path, process_time * 1000)
384-393: Health endpoint: fix unused arg, logging, and 503 return.Simplify signature, use logger.exception, and return a 503 directly.
-async def health(request: Request, response: JSONResponse): +async def health(_: Request): @@ - return JSONResponse({"status": "ok", "timestamp": time.time(), "version": __version__}) + return JSONResponse({"status": "ok", "timestamp": time.time(), "version": __version__}) @@ - except Exception as e: - logger.error(f"Health check failed: {e}", exc_info=True) - response.status_code = status.HTTP_503_SERVICE_UNAVAILABLE - return JSONResponse({"status": "error", "detail": str(e)}) + except Exception: + logger.exception("Health check failed") + return JSONResponse(status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + content={"status": "error", "detail": "unhealthy"})
221-229: rate_limited decorator: replace print with logger.debug.Avoid stdout spam in hot path.
- print(f"Rate limit identifier: {identifier}") + logger.debug("Rate limit identifier: %s", identifier)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Dockerfile(1 hunks)Dockerfile.old(1 hunks)api.py(3 hunks)celery_app.py(4 hunks)celery_config.py(1 hunks)config.yml(1 hunks)crawl.py(2 hunks)docker-entrypoint.sh(1 hunks)firestore.py(3 hunks)fly.toml(3 hunks)job.py(7 hunks)redisCache.py(3 hunks)requirements.txt(1 hunks)server.py(8 hunks)tasks.py(6 hunks)utils.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
utils.py (1)
redisCache.py (1)
redis_xadd(127-155)
tasks.py (2)
api.py (1)
get_firebase_client(50-55)firestore.py (2)
FirebaseClient(39-94)init_firebase(49-80)
api.py (2)
schemas.py (1)
CrawlOperation(80-96)utils.py (2)
decode_redis_hash(206-215)create_task_status_response(237-266)
job.py (1)
redisCache.py (1)
redis_xread(158-190)
🪛 Ruff (0.13.1)
redisCache.py
60-60: Consider moving this statement to an else block
(TRY300)
61-61: Do not catch blind exception: Exception
(BLE001)
143-143: Abstract raise to an inner function
(TRY301)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
152-152: Consider moving this statement to an else block
(TRY300)
153-153: Do not catch blind exception: Exception
(BLE001)
182-182: Loop control variable last_id not used within loop body
(B007)
184-184: Loop control variable channel not used within loop body
(B007)
187-187: Consider moving this statement to an else block
(TRY300)
188-188: Do not catch blind exception: Exception
(BLE001)
utils.py
337-337: Prefer TypeError exception for invalid type
(TRY004)
337-337: Abstract raise to an inner function
(TRY301)
356-356: Do not catch blind exception: Exception
(BLE001)
357-357: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
368-368: Do not catch blind exception: Exception
(BLE001)
369-369: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
server.py
384-384: Unused function argument: request
(ARG001)
tasks.py
67-67: Consider moving this statement to an else block
(TRY300)
api.py
664-664: Do not catch blind exception: Exception
(BLE001)
666-666: Use explicit conversion flag
Replace with conversion flag
(RUF010)
667-667: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
678-678: Use explicit conversion flag
Replace with conversion flag
(RUF010)
694-694: Use explicit conversion flag
Replace with conversion flag
(RUF010)
699-699: Use explicit conversion flag
Replace with conversion flag
(RUF010)
job.py
425-425: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
431-431: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
457-457: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
493-493: Do not catch blind exception: Exception
(BLE001)
494-494: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
celery_app.py
83-83: Unused function argument: args
(ARG001)
🪛 GitHub Check: CodeQL
server.py
[warning] 392-392: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
api.py
[warning] 683-683: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
[warning] 697-700: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
job.py
[warning] 518-518: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
🔇 Additional comments (13)
requirements.txt (2)
4-4: Do not removewebsocketsunilaterally — keep it or pin a compatible range.tests/test_websocket.py imports
websockets(tests/test_websocket.py:4); remove the requirements.txt entry only after confirminguvicorn[standard]is declared and pinned to a release that pulls in a compatiblewebsocketsversion. Otherwise keepwebsocketsin requirements.txt and add an explicit version constraint compatible with youruvicornpin.
12-12: Approve upgrade to upstash-redis ~=1.4.0.
Verified: upstash-redis 1.4.x targets Python 3.8+ (so Python 3.12 is covered); 1.x introduced API/typing return-value changes but there are no documented breaking changes within 1.4.x patch releases. Optional: add a constraints/lock file (pip-tools) for reproducible Docker builds.celery_app.py (1)
16-17: LGTM! Environment detection logic is clean.The production flag detection and environment file selection are implemented correctly. Good practice using
.envfor production anddev.envfor development.tasks.py (2)
45-73: Platform-specific event loop management looks robust.The implementation correctly handles event loop creation and management for both Windows and Linux environments. The distinction between Windows (persistent global loop) and Linux (process-specific loop) is appropriate for the different multiprocessing models.
235-242: Good platform-aware cleanup for Linux event loops.The cleanup of pending tasks on Linux while preserving them on Windows with solo pool is appropriate for the different process models. This prevents task leakage on Linux's multi-process model.
utils.py (4)
93-94: Good type annotation addition for WebSocket handling.Adding the
set[WebSocket]type annotation improves code clarity and helps with IDE support.
250-261: Robust error handling for task result parsing.The multi-level fallback approach for handling task results is thorough and handles edge cases well:
- First tries the Celery task result
- Falls back to Redis-stored result with JSON parsing
- Falls back to string if JSON parsing fails
- Returns None if no result is available
312-374: Excellent improvement to Redis streaming with batched pipeline.The refactoring to use a separate pipeline (
pipe2) for error/completion messages and the addition oftotal_chunksmetadata improves reliability and observability of the streaming process.
378-394: Well-implemented async retry utility.The retry function with exponential backoff and jitter is a valuable addition. The implementation follows best practices with configurable parameters and proper exception propagation.
config.yml (1)
11-19: CORS configuration supports environment-specific origins.The separation of
cors_origins_devandcors_originsallows for flexible CORS handling between development and production environments. This aligns well with the environment detection incelery_app.py.crawl.py (1)
135-135: Good practice using status constants.Using
status.HTTP_406_NOT_ACCEPTABLEinstead of the literal 406 improves code readability and maintainability.job.py (1)
341-365: Good: SSE wrapper delegates to a single status streamer.Centralizing streaming logic reduces duplication.
server.py (1)
79-85: Global cap wrapper for arun_many looks good.The semaphore guard is clear and low‑risk.
…should be declared as a parameter or defined as a constant within the function scope to avoid potential NameError. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…n creates a new JSONResponse instead of modifying the passed response parameter. Either remove the parameter or use it consistently throughout the function. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…of selecting the correct key. It should be config[\"app\"].get(\"cors_origins\" if production else \"cors_origins_dev\", [\"*\"]). Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
utils.py (2)
225-236: Critical: Celery status mapping is broken (string vs enum key mismatch).
convert_celery_statusexpectsCeleryTaskStatuskeys but is called withcelery_task.state(a string). This always falls back toTaskStatus.READY, misreporting task state.Apply this diff:
-def convert_celery_status(celery_status: CeleryTaskStatus) -> TaskStatus: - status_mapping = { - CeleryTaskStatus.PENDING: TaskStatus.PENDING, - CeleryTaskStatus.STARTED: TaskStatus.IN_PROGRESS, - CeleryTaskStatus.SUCCESS: TaskStatus.COMPLETED, - CeleryTaskStatus.FAILURE: TaskStatus.FAILED, - CeleryTaskStatus.RETRY: TaskStatus.RETRY, - CeleryTaskStatus.REVOKED: TaskStatus.CANCELED - } - - return status_mapping.get(celery_status, TaskStatus.READY) # Default to READY if status is unknown +def convert_celery_status(celery_status: str) -> TaskStatus: + """Translate Celery state string to TaskStatus safely.""" + mapping = { + CeleryTaskStatus.PENDING: TaskStatus.PENDING, + CeleryTaskStatus.STARTED: TaskStatus.IN_PROGRESS, + CeleryTaskStatus.SUCCESS: TaskStatus.COMPLETED, + CeleryTaskStatus.FAILURE: TaskStatus.FAILED, + CeleryTaskStatus.RETRY: TaskStatus.RETRY, + CeleryTaskStatus.REVOKED: TaskStatus.CANCELED, + } + try: + return mapping[CeleryTaskStatus(celery_status)] + except ValueError: + return TaskStatus.READY
287-291: Unify end-of-stream marker: emit the same '[DONE]' SSE marker from utils.py.job.py (429,467,514), api.py (673–680), triggers.py (135) and tests/test_stream.py expect "data: [DONE]"; utils.py yields JSON at utils.py:287 (json.dumps({"status":"completed"})). Change utils.py to emit the SSE "[DONE]" marker (or update callers/tests/docs to accept the JSON form).
🧹 Nitpick comments (10)
utils.py (10)
93-104: Avoid KeyError when removing stale clients.Use
discardto tolerate concurrent removals.Apply this diff:
- for client in disconnected_clients: - socket_client.remove(client) + for client in disconnected_clients: + socket_client.discard(client)
270-272: Drop redundant imports inside function.
jsonis already imported;datetime_handleris in this module. Use it directly.Apply this diff:
- import json - from utils import datetime_handler + # use module-level json import and local datetime_handler
320-325: Remove duplicated HTML clearing.You clear
result.htmland then also overwriteresult_dict["html"]. One is enough.Apply this diff:
- result_dict["html"] = "" # type: ignore
341-343: Don’t shadow thechunk_sizeparameter.Redefining it to 4096 ignores the function argument.
Apply this diff:
- chunk_size = 4096 # Define chunk_size as a constant (adjust as needed) - total_chunks = (len(batch_json) + chunk_size - 1) // chunk_size # Calculate total chunks + total_chunks = (len(batch_json) + chunk_size - 1) // chunk_size
357-373: Preferlogger.exceptionand narrow catches.Use
logger.exceptionto include tracebacks; consider catchingjson.JSONDecodeError/TypeErrorandredis.exceptions.RedisErrorinstead of blanketException.Apply this diff:
- except Exception as e: - logger.error(f"Serialization error: {e}") + except Exception as e: + logger.exception("Serialization error") @@ - except Exception as e: - logger.error(f"Unexpected error in stream_pubsub_results: {e}") + except Exception as e: + logger.exception("Unexpected error in stream_pubsub_results")
366-376: Return False on cancellation (semantic clarity) and flush immediately.Canceled streams shouldn’t report success.
Apply this diff:
- except asyncio.CancelledError: - logger.warning("Client disconnected during streaming") - pipe2.xadd(channel, {"status": "canceled", "message": "streaming canceled"}) + except asyncio.CancelledError: + logger.warning("Client disconnected during streaming") + pipe2.xadd(channel, {"status": "canceled", "message": "streaming canceled"}) + await pipe2.execute() + return FalseIf upstream expects True on cancel, ignore this change; otherwise adopt it.
362-362: Message mismatch for error completion.
{"status": "error", "message": "completed"}is contradictory.Apply this diff:
- complete = {"status": "error", "message": "completed"} + complete = {"status": "error", "message": "completed_with_errors"}
345-356: Bound the Redis stream length to prevent unbounded growth.Use
MAXLEN ~to trim the stream.Apply this diff:
- pipe.xadd(channel, { + pipe.xadd(channel, { "status": "ok", "message": "processing", "type": "batch_chunk", "url": url, "chunk_index": str(i // chunk_size), "total_chunks": str(total_chunks), # Add total_chunks attribute "dump": chunk #.encode("utf-8") if isinstance(chunk, str) else chunk - }) + }, maxlen=10000, approximate=True)
302-308: Docstring return type is outdated.Function returns
bool, but docstring says “Returns a list…”.Apply this diff:
- Returns a list of successfully published result dicts. + Returns: + bool: True on successful publish, False on error/cancel.
333-338: Use TypeError for non-dict model_dump.Raising
ValueError(model_dump)with a non-dict payload is misleading.Apply this diff:
- else: - raise ValueError(model_dump) + else: + raise TypeError(f"Expected dict from model_dump, got {type(model_dump).__name__}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utils.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
utils.py (1)
redisCache.py (1)
redis_xadd(127-155)
🪛 Ruff (0.13.1)
utils.py
337-337: Prefer TypeError exception for invalid type
(TRY004)
337-337: Abstract raise to an inner function
(TRY301)
357-357: Do not catch blind exception: Exception
(BLE001)
358-358: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
369-369: Do not catch blind exception: Exception
(BLE001)
370-370: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (1)
utils.py (1)
241-265: Fix branching: ready() includes failures; preserve falsy Celery results; don’t attach result on failure.
- Using
celery_task.ready()routes failures through the “completed/result” path.- If Celery result is falsy (e.g.,
None), it’s incorrectly ignored. This mirrors a prior review note.Apply this diff:
- "status": convert_celery_status(celery_task.state) or task["status"], + "status": convert_celery_status(celery_task.state) if celery_task.state else task.get("status", TaskStatus.READY), @@ - if task["status"] == TaskStatus.COMPLETED or celery_task.ready(): - # Safely parse result if it exists and isn't empty - if celery_task.successful() and celery_task.result: - response["result"] = celery_task.result - elif task.get("result") and task["result"].strip(): + if celery_task.successful() or task.get("status") == TaskStatus.COMPLETED: + # Prefer Celery's result even if falsy (e.g., None) when successful + if celery_task.successful(): + response["result"] = celery_task.result + elif task.get("result") and str(task["result"]).strip(): try: response["result"] = json.loads(task["result"]) except json.JSONDecodeError: # If we can't parse the result as JSON, use it as a string response["result"] = task["result"] else: response["result"] = None - elif task["status"] == TaskStatus.FAILED or celery_task.failed(): - response["error"] = task.get("error", "Unknown error") - response["result"] = celery_task.result + elif celery_task.failed() or task.get("status") == TaskStatus.FAILED: + response["error"] = task.get("error", str(celery_task.result) if celery_task.result else "Unknown error") + response["result"] = None
…n should use parentheses to clarify the intended logic: if (retries > max_retries and not completed_yielded) or (celery_task.ready() and retries > 3):. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
server.py (4)
64-64: Harden version fallback to avoid KeyError.Accessing
config["app"]["version"]can raise if keys are missing. Use nested.get(...)with a sane default.-__version__ = config["app"]["version"] or "0.0.1-d1" +__version__ = (config.get("app") or {}).get("version") or "0.0.1-d1"
71-76: Avoid global monkey‑patch of third‑party methods; make patch idempotent at minimum.Replacing
AsyncWebCrawler.arun/arun_manyglobally can cause hard‑to‑debug side effects across modules/tests and double‑patching on reload. Make it idempotent now; consider a subclass or wrapper in a follow‑up.-orig_arun = AsyncWebCrawler.arun -async def capped_arun(self, *a, **kw): - async with GLOBAL_SEM: - return await orig_arun(self, *a, **kw) -AsyncWebCrawler.arun = capped_arun +if not getattr(AsyncWebCrawler, "_capped_patched", False): + orig_arun = AsyncWebCrawler.arun + async def capped_arun(self, *a, **kw): + async with GLOBAL_SEM: + return await orig_arun(self, *a, **kw) + AsyncWebCrawler.arun = capped_arun + + orig_arun_many = AsyncWebCrawler.arun_many + async def capped_arun_many(self, urls, config=None, dispatcher=None, **kwargs): + async with GLOBAL_SEM: + return await orig_arun_many(self, urls, config, dispatcher, **kwargs) + AsyncWebCrawler.arun_many = capped_arun_many + + AsyncWebCrawler._capped_patched = TrueFollow‑up: Please confirm the intended concurrency semantics (per‑page vs per‑call). If
arun_manyfans out internally, a single permit may not cap “pages.”Also applies to: 79-85
101-106: Use logger instead of print for environment banner (and fix label typo).Terminal color codes +
-production = os.getenv("PYTHON_ENV", "development").lower() == "production" -if production: - print("\033[94mINFO-SERVER:\033[0m \033[92mRunning in Production mode. PYTHON_ENV\033[0m", production) -else: - print("\033[94mWARNIN-SERVER:\033[92m Running in Development mode. PYTHON_ENV", production) +production = os.getenv("PYTHON_ENV", "development").lower() == "production" +if production: + logger.info("Running in Production mode. PYTHON_ENV=%s", production) +else: + logger.warning("Running in Development mode. PYTHON_ENV=%s", production)
282-292: Timing middleware: switchUse
logger.debugand keep headers; current@app.middleware("http") async def add_process_time_header(request: Request, call_next): - start_time = time.time() response = await call_next(request) process_time = time.time() - start_time response.headers["X-Process-Time"] = f"{process_time * 1000:.2f}" - - print(f"Request: {request.url.path} - Response time: {process_time * 1000:.2f} ms") + logger.debug("HTTP %s -> %.2f ms", request.url.path, process_time * 1000) return responsejob.py (4)
14-14: Redis imports look fine; align usage with chosen client.You import both
redis/pure_redisandredis_xread. Prefer one path consistently to reduce branching.
74-80: Testing endpoint: remove unusedresponseor use it.
response: Responseis unused.-async def get_user_data( - request: Request, - response: Response, +async def get_user_data( + request: Request,
341-385: Temp‑task status SSE bootstrap: handle “empty” and bytes robustly; avoid leaking internals.
hgetmay return bytes; decode before comparisons with"empty".- Error response returns
internal_message: str(e); remove to avoid info disclosure.- task_id = await redis.hget(key=f"temp_task_id:{temp_task_id}", field='celery_task_id') + raw = await redis.hget(key=f"temp_task_id:{temp_task_id}", field='celery_task_id') + task_id = raw.decode() if isinstance(raw, (bytes, bytearray)) else raw @@ - return JSONResponse( + return JSONResponse( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, content={ "status": "error", - "error": "Internal server error while streaming task status", - "internal_message": str(e) + "error": "Internal server error while streaming task status" } )
436-447: Decode Redis stream payloads defensively.
xreadoften returns dicts of bytes; current branch assumes JSON bytes or dict[str, Any]. Normalize to a dict[string, Any] before using.get(...).# after obtaining msg_data if isinstance(msg_data, dict): msg_data_dict = { (k.decode() if isinstance(k, (bytes, bytearray)) else k): json.loads(v.decode()) if isinstance(v, (bytes, bytearray)) else v for k, v in msg_data.items() } elif isinstance(msg_data, (bytes, bytearray)): msg_data_dict = json.loads(msg_data.decode("utf-8")) else: ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
job.py(7 hunks)server.py(8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
job.py (1)
redisCache.py (1)
redis_xread(158-190)
🪛 Ruff (0.13.1)
job.py
431-431: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
457-457: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
493-493: Do not catch blind exception: Exception
(BLE001)
494-494: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
server.py
384-384: Unused function argument: request
(ARG001)
🪛 GitHub Check: CodeQL
job.py
[warning] 518-518: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
Stack trace information flows to this location and may be exposed to an external user.
server.py
[warning] 392-392: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🔇 Additional comments (7)
server.py (3)
91-91: Typed WebSocket client registry looks good.Explicit
set[WebSocket]improves readability and tooling.
121-121: Verify Redis client used by streaming is tested at startup.
pure_redisis used byxreadin job streaming but its connectivity test is commented out. Consider testing bothredisandpure_redisduring lifespan startup.
353-353: CORS ternary fix is correct.Using
get("cors_origins" if production else "cors_origins_dev", ["*"])resolves the tuple bug flagged earlier.job.py (4)
24-25: Good: query Celery state via AsyncResult with explicit app.This avoids default‑app surprises.
292-292: REST path for cancel improved.
PUT /crawl/job/{temp_task_id}/cancelis clearer than the previous variant.Please confirm clients/docs were updated to the new path.
517-525: Good SSE response headers.
X-Accel-Buffering: no,no-cache, andkeep-aliveare appropriate for streaming.
425-433: Operator precedence bug; parenthesize the state check.Current
elifmixesand/orand changes intent.- elif retries > max_retries and not completed_yielded and celery_task.state == "PENDING" or celery_task.state == "STARTED": + elif retries > max_retries and not completed_yielded and (celery_task.state in {"PENDING", "STARTED"}): retries = 0
…ough an exception Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api.py (1)
491-503: Decode Redis hash and fix signature check; current logic never cancels crawler.
hasattr(task_info, "no_signature")is wrong for dicts andtask_infomay be bytes-keyed. This preventscancel_crawlerfrom ever running and can leak resources.Apply this diff:
- task_info = await redis.hgetall(f"task:{task_id}") + task_info = await redis.hgetall(f"task:{task_id}") + task_info = decode_redis_hash(task_info) if task_info else {} - operation_id = task_info.get("operation_id") if task_info else None + operation_id = task_info.get("operation_id") if task_info else None if not task_info: raise HTTPException(status_code=404, detail="Task not found") try: - browser_sign = task_info["signature"] if hasattr(task_info, "no_signature") else None - if (browser_sign and browser_sign != "no_signature"): - await cancel_crawler(browser_sign) # Remove the crawler to free resources + browser_sign = task_info.get("signature") + if browser_sign and browser_sign != "no_signature": + await cancel_crawler(browser_sign) # Remove the crawler to free resources
🧹 Nitpick comments (2)
api.py (2)
20-21: Deduplicate Celery imports; keep these and remove duplicates below.Keep these top-level imports and drop the later duplicates to avoid shadowing/confusion.
Apply this diff to remove duplicates:
-from datetime import datetime # Import datetime for Celery task ID generation -from celery.result import AsyncResult # Import AsyncResult +from datetime import datetime # (already imported with timezone at line 13; remove this duplicate)-from celery_app import celery_app # Import celery_app + # (already imported at line 21; remove this duplicate)
31-32: Remove duplicate imports of datetime, AsyncResult, and celery_app.These are already imported above; duplicates risk inconsistencies and lint failures.
Apply this diff:
-from datetime import datetime # Import datetime for Celery task ID generation -from celery.result import AsyncResult # Import AsyncResult-from celery_app import celery_app # Import celery_appAlso applies to: 42-42
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api.py (1)
utils.py (2)
decode_redis_hash(206-215)create_task_status_response(237-266)
🪛 Ruff (0.13.1)
api.py
664-664: Do not catch blind exception: Exception
(BLE001)
666-666: Use explicit conversion flag
Replace with conversion flag
(RUF010)
667-667: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
678-678: Use explicit conversion flag
Replace with conversion flag
(RUF010)
694-694: Use explicit conversion flag
Replace with conversion flag
(RUF010)
699-699: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🪛 GitHub Check: CodeQL
api.py
[warning] 697-700: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🔇 Additional comments (5)
api.py (5)
28-28: LGTM: signal is required for POSIX revoke.
614-614: Re-initializing AsyncResult per check is fine.
508-520: Do not broadcast global Celery shutdown; use proper signal names for POSIX.Broadcasting 'shutdown' kills all workers. Also pass signal names (strings), not enums, to Celery.
Apply this diff:
- if os.name == 'nt': # Windows - celery_task.revoke(terminate=True) - - # Force termination on Windows needs special handling - if force: - # Send shutdown event to worker - celery_app.control.broadcast('shutdown') + if os.name == 'nt': # Windows + # Windows pools don't support POSIX signals; rely on terminate=True + celery_task.revoke(terminate=True) else: # POSIX systems celery_task.revoke( terminate=True, - signal=signal.SIGKILL if force else signal.SIGTERM + signal='SIGKILL' if force else 'SIGTERM' )
655-656: Fix SSE formatting (double newline) and use logger.exception; narrow exception.SSE events must terminate with a blank line. Also prefer logger.exception and avoid broad
except Exception.Apply this diff:
- data = f"data: {json.dumps(response)}\n" # Note the double newline for SSE format + data = f"data: {json.dumps(response)}\n\n" # SSE events end with a blank line- except Exception as e: - # Handle any serialization errors - error_msg = f"Error generating status response: {str(e)}" - logger.error(error_msg) - yield f"data: {json.dumps({'error': 'An internal error occurred while generating the status response.'})}\n".encode('utf-8') + except (json.JSONDecodeError, TypeError) as e: + logger.exception("Error generating status response: %s", e) + yield f"event: error\ndata: {json.dumps({'error': 'An internal error occurred while generating the status response.'})}\n\n".encode('utf-8')- yield b"data: [DONE]\n" + yield b"data: [DONE]\n\n"- except Exception as e: - # TODO: Handle exceptions in the streaming loop IN the frontend - logger.error(f"Fatal error in status stream: {str(e)}", exc_info=True) - yield f"event: error\ndata: {json.dumps({'error': 'A fatal error occurred while streaming the task status.', 'fatal': True})}\n".encode('utf-8') - yield b"data: [DONE]\n" + except Exception as e: + logger.exception("Fatal error in status stream: %s", e) + yield f"event: error\ndata: {json.dumps({'error': 'A fatal error occurred while streaming the task status.', 'fatal': True})}\n\n".encode('utf-8') + yield b"data: [DONE]\n\n"Also applies to: 668-669, 674-674, 676-681
692-701: Avoid leaking exception details in HTTP error; improve logging format.Return a generic message; log stack trace with logger.exception and use lazy formatting.
Apply this diff:
- except Exception as e: - # Return a proper error response instead of letting the exception bubble up - logger.error(f"Error setting up status stream for task {task_id}: {str(e)}", exc_info=True) - return JSONResponse( + except Exception as e: + logger.exception("Error setting up status stream for task %s: %s", task_id, e) + return JSONResponse( status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, content={ "status": "error", - "error": f"Failed to stream task status: {str(e)}" + "error": "Failed to stream task status." } )
pipe.execute([...]) won’t work for either client. Call the right method and await it. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Passing the whole list to execute will fail. Build varargs and branch per client. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Returning str(e) leaks details (CodeQL finding); also request is unused. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…uild. Packages needing compilation (e.g., with C extensions) will fail without toolchains/headers. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ough an exception Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
redisCache.py (4)
92-107: Bug:redis_executepasses a list as the first arg toexecute.This will fail for both Upstash and redis‑py. Flatten the command and support both clients.
Apply:
-async def redis_execute(redis: Redis, command: List, *args): +async def redis_execute(redis: Redis | PureRedis, command: List[str]): @@ - try: - result = await redis.execute(command, *args) - return result + try: + if not command: + raise ValueError("command must be a non-empty list") + # Upstash client exposes `execute`; redis-py uses `execute_command` + if hasattr(redis, "execute"): + return await redis.execute(command[0], *command[1:]) + else: + return await redis.execute_command(*command)
80-85: Comment/code mismatch for rate limiter.Comment says “5 requests per 60 seconds,” code configures 1.
Apply:
-# Custom rate limiter: 5 requests per 60 seconds +# Custom rate limiter: 1 request per 60 secondsOr update
max_requests=5to match the comment.
109-116: Return value: makeredis_subscribeuseful to callers.Currently returns None; callers will need the PubSub instance to receive messages.
Return the
pubsubobject as shown in the earlier diff so consumers can iterate messages cleanly.
20-26: Env var names are consistent; fix a misleading error message and a docker-compose env syntaxUPSTASH_REDIS_REST_URL/REST_TOKEN/USER/PASS/PORT are used consistently across the repo, but fix these two issues:
- celery_app.py — ValueError text mentions UPSTASH_REDIS_REST_PASSWORD (nonexistent). Update the message to reference the actual env var (UPSTASH_REDIS_PASS) or make names consistent.
- docker-compose.yml — line uses
UPSTASH_REDIS_PASS = ${UPSTASH_REDIS_PASS}(spaces around=). Change toUPSTASH_REDIS_PASS=${UPSTASH_REDIS_PASS}so Docker Compose parses it.
🧹 Nitpick comments (13)
Dockerfile (8)
34-40: Set DEBIAN_FRONTEND in final stage to avoid interactive apt operations.Prevents prompts during Playwright’s --with-deps and any future installs.
ENV PYTHONUNBUFFERED=1 \ PYTHONDONTWRITEBYTECODE=1 \ PLAYWRIGHT_BROWSERS_PATH=/ms-playwright \ PYTHON_ENV=production \ - DISPLAY=:99 + DISPLAY=:99 +ENV DEBIAN_FRONTEND=noninteractive
64-68: Duplicate libnss3 entry.Drop the duplicate to keep the manifest clean.
- libglib2.0-0 \ - libnss3 \ + libglib2.0-0 \ libnspr4 \
109-116: Playwright install with --with-deps may invoke apt; ensure noninteractive and minimize extra installs.Setting DEBIAN_FRONTEND (above) helps. Consider avoiding --with-deps if you already apt-install Chromium deps, to reduce apt churn.
Please confirm Playwright’s install doesn’t re-install duplicates. If it does, prefer plain “playwright install chromium” since deps are preinstalled.
121-123: Remove duplicate DISPLAY definition.Defined earlier; keep one source of truth.
-# Set display environment variable -ENV DISPLAY=:99 +## DISPLAY already set above
124-126: Avoid running diagnostics during image build (non-deterministic, may need network/display).Gate with a build arg or run at container start in health checks.
-# Run diagnostics -RUN crawl4ai-doctor +# Optional diagnostics (off by default to keep builds deterministic) +ARG RUN_DOCTOR=false +RUN if [ "$RUN_DOCTOR" = "true" ]; then crawl4ai-doctor || true; fi
135-142: Entrypoint timing: move runtime orchestration out of CMD.Prefer starting websockify/uvicorn/Xvfb/x11vnc from the entrypoint for clearer control and easier overrides. Keep CMD minimal (e.g., uvicorn args only) or none.
Ensure docker-entrypoint.sh handles starting Xvfb and x11vnc before websockify.
118-120: Duplicate copy of config.yml?config.yml is already covered by “COPY . .” unless excluded by .dockerignore. Remove redundancy or confirm ignore rules.
51-62: Prune non-essential utilities from runtime layer.lsof, supervisor appear unused at runtime. Remove to trim image unless the entrypoint uses them.
lsof \ - ... - supervisor \ + ... + # supervisor (remove if not used)redisCache.py (5)
27-31: Fix misleading config validation and improve robustness of direct-Redis settings.
- The error message mentions only REST URL/token but you also require port/user/pass.
- Deriving
REDIS_URLby strippinghttps://from the REST URL is brittle. Prefer parsingUPSTASH_REDIS_URL(rediss://default:pass@host:port) if provided, then fall back to explicit host/port/user/pass.Apply this diff to correct the error message:
-if not redis_url or not redis_token or not REDIS_PORT or not REDIS_USERNAME or not REDIS_PASSWORD: - raise ValueError("UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN environment variables must be set") +if not all([redis_url, redis_token, REDIS_PORT, REDIS_USERNAME, REDIS_PASSWORD]): + raise ValueError( + "Missing Redis configuration: require UPSTASH_REDIS_REST_URL, " + "UPSTASH_REDIS_REST_TOKEN, UPSTASH_REDIS_PORT, UPSTASH_REDIS_USER, and UPSTASH_REDIS_PASS" + )Outside this range, consider:
- Prefer UPSTASH_REDIS_URL parsing:
from urllib.parse import urlparse raw_uri = os.environ.get("UPSTASH_REDIS_URL") # e.g., rediss://default:pass@host:port if raw_uri: u = urlparse(raw_uri) REDIS_URL = u.hostname REDIS_PORT = u.port or 6379 REDIS_USERNAME = u.username or "default" REDIS_PASSWORD = u.password else: REDIS_URL = redis_url.replace("https://", "")Please confirm which envs your deployment actually sets (UPSTASH_REDIS_URL vs discrete host/port/user/pass) so we can lock this down.
169-206: XREAD works, but prefer high‑level API when available.To improve compatibility, use
xreadif the client exposes it; otherwise keep the current varargs execute path.Example update:
if hasattr(redis, "xread"): # redis-py: streams is {channel: last_id} return await redis.xread(streams=streams, count=count, block=block) # else: keep the existing execute/execute_command branchesAlso consider narrowing the
except Exceptionto connection/command errors.
61-66: Broad exception handling; tighten where possible.Catching bare Exception obscures actionable errors and trips linters (BLE001). Use specific exceptions (e.g., connection/protocol errors) or re‑raise with context.
Also applies to: 164-166, 203-205
20-26: Security/config hygiene: avoid plain prints for secrets‑adjacent failures.Move to structured logging and ensure no secrets are logged. Also consider setting socket timeouts on
PureRedis(e.g.,socket_connect_timeout,socket_timeout) to avoid hanging.Also applies to: 40-49
20-31: Cross‑provider compatibility for TLS.If connecting with TLS, consider explicitly setting
ssl=Trueand verifying certificates (default is OK) but be mindful of custom CAs in some environments.Also applies to: 40-49
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Dockerfile(1 hunks)redisCache.py(3 hunks)server.py(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server.py
🧰 Additional context used
🪛 Ruff (0.13.1)
redisCache.py
60-60: Consider moving this statement to an else block
(TRY300)
61-61: Do not catch blind exception: Exception
(BLE001)
143-143: Abstract raise to an inner function
(TRY301)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
163-163: Consider moving this statement to an else block
(TRY300)
164-164: Do not catch blind exception: Exception
(BLE001)
193-193: Loop control variable last_id not used within loop body
(B007)
195-195: Loop control variable channel not used within loop body
(B007)
202-202: Consider moving this statement to an else block
(TRY300)
203-203: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (9)
Dockerfile (4)
20-26: Builder stage: good wheel prebuild with toolchains.Addressed prior build-deps gap; piping wheels improves reliability and caching.
46-63: Remove build-only toolchains from the final image.Keep these in the builder; they bloat the runtime and increase attack surface.
RUN apt-get update && apt-get install -y --no-install-recommends \ fonts-liberation \ ca-certificates \ lsof \ - # Add build dependencies for madoka - build-essential \ curl \ wget \ gnupg \ - cmake \ - gcc \ - g++ \ - pkg-config \ - python3-dev \ - libjpeg-dev \ supervisor \ libnss3 \
92-94: Misleading comment; and clone steps should be pinned.The comment mentions sudo but installs git; also clones are unpinned → non‑reproducible builds.
- git \ - # Add sudo for X11 management - && git clone --depth 1 https://github.com/novnc/noVNC /opt/noVNC \ - && git clone --depth 1 https://github.com/novnc/websockify /opt/noVNC/utils/websockify \ + git \ + # Clone pinned noVNC and websockify (reproducible) + && git clone --depth 1 --branch v1.5.0 https://github.com/novnc/noVNC /opt/noVNC \ + && git clone --depth 1 --branch v0.12.0 https://github.com/novnc/websockify /opt/noVNC/utils/websockify \ + && ln -sf /opt/noVNC/vnc.html /opt/noVNC/index.html \ + && apt-get purge -y --auto-remove git \
145-147: Verified: entrypoint launches Xvfb/x11vnc and server:app exists.
- docker-entrypoint.sh starts Xvfb :99 and x11vnc on port 5900 (lines 10–11).
- server.py defines app = FastAPI (lines 147–149) — confirms uvicorn target server:app.
- .dockerignore exists.
redisCache.py (5)
51-69: Excessive 20‑minute startup retry loop; make it configurable.Defaults are too high and the message hardcodes 12s. Use env‑driven, lower defaults and print actual values.
Apply:
- retries = 0 - max_retries = 100 - retry_delay = 12 # seconds + retries = 0 + max_retries = int(os.getenv("REDIS_MAX_RETRIES", "10")) + retry_delay = float(os.getenv("REDIS_RETRY_DELAY_SEC", "3.0")) # seconds @@ - print(f"\033[93mWARNING-DB:\033[0m Trying again in 12.00 seconds... (attempt {retries}/{max_retries})") + print(f"\033[93mWARNING-DB:\033[0m Trying again in {retry_delay:.2f} seconds... (attempt {retries}/{max_retries})")Additionally, narrow the exception type if possible (e.g., connection errors).
40-49: Validate direct-Redis connection parameters.
hostcomes fromredis_url.replace("https://", ""), which assumes REST and direct hosts match. This may not always hold across providers/regions.Please verify that the direct Redis hostname/port/creds are correct for your Upstash instance. If you have
UPSTASH_REDIS_URL, prefer parsing it for host/port/user/pass as suggested earlier.
14-19: Dotenv loading: confirm runtime behavior in containers.Ensure
dev.envdoes not get baked into production images andload_dotenv(..., override=False)won’t mask real environment variables.Would you like me to add a safe pattern that only loads the file if present and never overrides pre‑set envs?
90-97: Type hints and API: narrow inputs.
command: Listis too loose; useList[str]and validate contents, which you did in the fix above.Please run ruff/mypy on this file after the proposed changes.
109-126: Use PureRedis.pubsub for SUBSCRIBE and return the PubSub objectUpstash REST client (upstash_redis) doesn’t support SUBSCRIBE; remove the stray
redis.publishand switch to redis-py’s pubsub API.File: redisCache.py (lines ~109–126)
-async def redis_subscribe(redis: Redis, channel: str): +async def redis_subscribe(redis: PureRedis, channel: str): @@ - redis.publish - try: - await redis_execute(redis, ["SUBSCRIBE", channel]) - print(f"\033[94mINFO-DB:\033[0m \033[92mSubscribed to channel '{channel}'\033[0m") + pubsub = redis.pubsub() + await pubsub.subscribe(channel) + print(f"\033[94mINFO-DB:\033[0m \033[92mSubscribed to channel '{channel}'\033[0m") + return pubsubEnsure all call sites pass the PureRedis instance (e.g.,
pure_redis) toredis_subscriberather than the Upstash REST client.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
api.py (2)
489-506: Decode Redis values and fix signature lookup to avoid bytes/key errors.
hget/hgetallreturn bytes. Using dict keys as strings andhasattron a dict will fail. This can break cancellation, duration calc, and Firebase updates.- task_id = await redis.hget(key=f"temp_task_id:{temp_task_id}", field='celery_task_id') - - task_info = await redis.hgetall(f"task:{task_id}") - - operation_id = task_info.get("operation_id") if task_info else None + task_id = await redis.hget(key=f"temp_task_id:{temp_task_id}", field='celery_task_id') + if isinstance(task_id, bytes): + task_id = task_id.decode('utf-8') + + task_info_raw = await redis.hgetall(f"task:{task_id}") + task_info = decode_redis_hash(task_info_raw) if task_info_raw else {} + + operation_id = task_info.get("operation_id") @@ - browser_sign = task_info["signature"] if hasattr(task_info, "no_signature") else None - if (browser_sign and browser_sign != "no_signature"): + browser_sign = task_info.get("signature") + if browser_sign and browser_sign != "no_signature": await cancel_crawler(browser_sign) # Remove the crawler to free resources
621-629: Fix cleanup gating: comparing Enum objects to strings prevents deletion.
task["status"]is a string; comparison against Enum members always fails. Use the response’s status (Enum or string) and compare string values.- if task["status"] in [TaskStatus.COMPLETED, TaskStatus.FAILED, TaskStatus.CANCELED]: - if not keep and should_cleanup_task(task["created_at"]): + status_str = response["status"].value if isinstance(response["status"], TaskStatus) else response["status"] + if status_str in {TaskStatus.COMPLETED.value, TaskStatus.FAILED.value, TaskStatus.CANCELED.value}: + if not keep and should_cleanup_task(task["created_at"]): await redis.delete(f"task:{task_id}") await redis.delete(f"{REDIS_CHANNEL}:{task_id}") await redis.delete(f"celery-task-meta-{task_id}") await redis.delete(f"temp_task_id:{task['temp_task_id']}")
🧹 Nitpick comments (2)
api.py (2)
20-21: Remove duplicate imports (AsyncResult, celery_app).These are already imported later (Lines 31 and 42). Keep a single import to avoid confusion and potential circular-import pitfalls.
-from celery.result import AsyncResult # Import AsyncResult here -from celery_app import celery_app # Import celery_app here
30-31: Drop duplicate datetime/AsyncResult imports.
datetimeis imported at Line 13;AsyncResultat Line 20/31. Keep one of each to reduce noise.Example:
- Keep “from datetime import datetime, timezone” at Line 13; remove Line 30.
- Keep a single “from celery.result import AsyncResult”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
api.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
api.py (2)
schemas.py (1)
CrawlOperation(80-96)utils.py (2)
decode_redis_hash(206-215)create_task_status_response(237-266)
🪛 Ruff (0.13.1)
api.py
664-664: Do not catch blind exception: Exception
(BLE001)
666-666: Use explicit conversion flag
Replace with conversion flag
(RUF010)
667-667: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
678-678: Use explicit conversion flag
Replace with conversion flag
(RUF010)
694-694: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🔇 Additional comments (2)
api.py (2)
508-520: Do not broadcast worker-wide shutdown; use correct POSIX signal type.Broadcasting 'shutdown' kills all workers and is unsafe for per-task cancel; Celery expects signal name strings. Use terminate=True and signal strings; on Windows rely on terminate only.
- # Windows-compatible task revocation - if os.name == 'nt': # Windows - celery_task.revoke(terminate=True) - - # Force termination on Windows needs special handling - if force: - # Send shutdown event to worker - celery_app.control.broadcast('shutdown') - else: # POSIX systems - celery_task.revoke( - terminate=True, - signal=signal.SIGKILL if force else signal.SIGTERM - ) + # Windows-compatible task revocation + if os.name == 'nt': # Windows + celery_task.revoke(terminate=True) + else: # POSIX systems + celery_task.revoke( + terminate=True, + signal='SIGKILL' if force else 'SIGTERM' + )
636-701: SSE formatting requires a blank line terminator; use logger.exception and parameterized logging.Ensure events end with “\n\n”, use an explicit error event, and avoid f-string interpolation in logs. Also mark final “[DONE]” with “\n\n”.
- data = f"data: {json.dumps(response)}\n" # Note the double newline for SSE format + data = f"data: {json.dumps(response)}\n\n" # SSE events must end with a blank line @@ - except Exception as e: - # Handle any serialization errors - error_msg = f"Error generating status response: {str(e)}" - logger.error(error_msg) - yield f"data: {json.dumps({'error': 'An internal error occurred while generating the status response.'})}\n".encode('utf-8') + except Exception as e: + # Handle any serialization errors + logger.exception("Error generating status response: %s", e) + yield f"event: error\ndata: {json.dumps({'error': 'An internal error occurred while generating the status response.'})}\n\n".encode('utf-8') @@ - yield b"data: [DONE]\n" + yield b"data: [DONE]\n\n" @@ - except Exception as e: - # TODO: Handle exceptions in the streaming loop IN the frontend - logger.error(f"Fatal error in status stream: {str(e)}", exc_info=True) - yield f"event: error\ndata: {json.dumps({'error': 'A fatal error occurred while streaming the task status.', 'fatal': True})}\n".encode('utf-8') - yield b"data: [DONE]\n" + except Exception as e: + # TODO: Handle exceptions in the streaming loop IN the frontend + logger.exception("Fatal error in status stream: %s", e) + yield f"event: error\ndata: {json.dumps({'error': 'A fatal error occurred while streaming the task status.', 'fatal': True})}\n\n".encode('utf-8') + yield b"data: [DONE]\n\n" @@ - except Exception as e: - # Return a proper error response instead of letting the exception bubble up - logger.error(f"Error setting up status stream for task {task_id}: {str(e)}", exc_info=True) + except Exception as e: + # Return a proper error response instead of letting the exception bubble up + logger.exception("Error setting up status stream for task %s: %s", task_id, e)
…rence. Raw XADD requires an ID; you’re not sending “*”. Prefer high‑level xadd when available; only fall back to raw execute. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
redisCache.py (5)
27-28: Validate all required envs and report exactly which are missing.Current error message mentions only two vars but you require five.
-if not redis_url or not redis_token or not REDIS_PORT or not REDIS_USERNAME or not REDIS_PASSWORD: - raise ValueError("UPSTASH_REDIS_REST_URL and UPSTASH_REDIS_REST_TOKEN environment variables must be set") +if not redis_url or not redis_token or not REDIS_PORT or not REDIS_USERNAME or not REDIS_PASSWORD: + missing = [ + name for name, val in [ + ("UPSTASH_REDIS_REST_URL", redis_url), + ("UPSTASH_REDIS_REST_TOKEN", redis_token), + ("UPSTASH_REDIS_PORT", REDIS_PORT), + ("UPSTASH_REDIS_USER", REDIS_USERNAME), + ("UPSTASH_REDIS_PASS", REDIS_PASSWORD), + ] if not val + ] + raise ValueError(f"Missing required Redis env vars: {', '.join(missing)}")
80-85: Fix limiter to match comment (5 req/60s) or update the comment.Currently max_requests=1. Either raise to 5 or adjust the comment.
- limiter=FixedWindow(max_requests=1, window=60), + limiter=FixedWindow(max_requests=5, window=60),
92-107: redis_execute passes a list as the command; flatten args and support both clients.This mirrors your XREAD fix; without it, redis-py clients will fail.
-async def redis_execute(redis: Redis, command: List, *args): +async def redis_execute(redis: Redis | PureRedis, command: List[str]): @@ - try: - result = await redis.execute(command, *args) - return result + try: + if not command: + print("\033[91mERROR-DB:\033[0m Command is empty.") + return None + if hasattr(redis, "execute"): + # Upstash client + return await redis.execute(command[0], *command[1:]) + else: + # redis-py client + return await redis.execute_command(*command)
40-49: Use Upstash rediss:// TLS endpoint — do not reuse the REST (https) hostUpstash exposes separate HTTPS (REST) and rediss:// (TLS/TCP) endpoints; redis‑py (PureRedis) speaks Redis TCP and will not work against the REST host. Replace the current logic that strips https from the REST URL — provide/parse the rediss:// URL (or use redis.asyncio.from_url("rediss://:PASSWORD@HOST:PORT") or host/port + ssl=True) so PureRedis connects to the proper TLS Redis endpoint.
Location: redisCache.py lines 40–49 (PureRedis initialization).
109-124: redis_subscribe is broken — use redis-py PubSub (PureRedis) or streams instead
- Do not call SUBSCRIBE via redis_execute against the Upstash REST client — Upstash REST does not provide real-time Pub/Sub; calling SUBSCRIBE this way won't create a working subscription.
- Replace the no-op
redis.publishline and switch the function to use the redis.asyncio client (pure_redis). Example: create a pubsub viapubsub = pure_redis.pubsub(); await pubsub.subscribe(channel); return pubsub.- Alternatively, if you must use the Upstash REST client, replace Pub/Sub usage with Redis Streams (XADD / XREAD).
- Location: redisCache.py (lines 109–124)
🧹 Nitpick comments (2)
redisCache.py (2)
14-18: Dotenv loading: add fallback and avoid overriding existing envs.Use a safe fallback when the env file is missing and don’t override process envs.
-# Load environment variables -load_dotenv(env_file, verbose=True) +# Load environment variables (do not override existing envs) +if not load_dotenv(env_file, override=False, verbose=True): + # Fallback to default .env in current dir if specific file not found + load_dotenv(override=False)
172-209: XREAD: minor cleanups and linter fixes; keep vararg approach.
- Build channels and IDs in one pass to avoid B007 warnings.
- Optional: type streams as Mapping[str, str].
- command.append("STREAMS") - for channel, last_id in streams.items(): - command.append(channel) - for channel, last_id in streams.items(): - command.append(last_id) + command.append("STREAMS") + items = list(streams.items()) + command.extend([ch for ch, _ in items]) + command.extend([lid for _, lid in items])Optional: consider socket timeouts/health checks on PureRedis for long BLOCKs (e.g., socket_timeout, health_check_interval).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
redisCache.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.13.1)
redisCache.py
60-60: Consider moving this statement to an else block
(TRY300)
61-61: Do not catch blind exception: Exception
(BLE001)
143-143: Abstract raise to an inner function
(TRY301)
143-143: Avoid specifying long messages outside the exception class
(TRY003)
166-166: Consider moving this statement to an else block
(TRY300)
167-167: Do not catch blind exception: Exception
(BLE001)
196-196: Loop control variable last_id not used within loop body
(B007)
198-198: Loop control variable channel not used within loop body
(B007)
205-205: Consider moving this statement to an else block
(TRY300)
206-206: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
redisCache.py (3)
127-169: XADD helper looks good and fixes prior issues.Includes “*” ID, prefers high-level xadd, and coerces fields to str. Nice.
Please confirm this works with both clients in your integration path (Upstash execute vs redis-py xadd).
47-49: decode_responses=True is a good default for PureRedis.
52-69: Startup retry loop: make max retries and delay configurable; reduce defaults.20 minutes (100×12s) can stall deployments. Use env-driven values and print the actual delay.
- retries = 0 - max_retries = 100 - retry_delay = 12 # seconds + retries = 0 + max_retries = int(os.getenv("REDIS_MAX_RETRIES", "10")) + retry_delay = float(os.getenv("REDIS_RETRY_DELAY_SEC", "3.0")) # seconds @@ - print(f"\033[93mWARNING-DB:\033[0m Trying again in 12.00 seconds... (attempt {retries}/{max_retries})") + print(f"\033[93mWARNING-DB:\033[0m Trying again in {retry_delay:.2f} seconds... (attempt {retries}/{max_retries})")Optional: catch narrower exceptions (e.g., ConnectionError, TimeoutError) instead of Exception.
- Implement proper background process management with graceful shutdown and VNC authentication support
- Add robust S3 storage with multipart uploads, connection pooling, retry logic, and streaming decompression
- Fix logic flow in task status handling and optimize response creation
- Improve Redis connection reliability with better error handling, URL parsing, and client type detection
…nctions - Add proper type casting, improve error handling, and fix SSE formatting in streaming responses
- Fix SSE message format with proper double newlines and improve error handling in stream results
- Simplify CMD by moving noVNC handling to the entrypoint script for better process management
- Add VNC password security, optimize service definitions, and update process commands
- Improve URL parsing and environment variable management for Celery Redis connection
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
celery_app.py (1)
82-88: Unused*argsparameter in Windows shutdown handler.The function accepts
*argsbut doesn't use them.Apply this diff to remove the unused parameter:
- def windows_shutdown_handler(*args): + def windows_shutdown_handler(signum, frame): print("Windows shutdown signal received") celery_app.control.broadcast('shutdown') sys.exit(0)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Dockerfile(1 hunks)api.py(7 hunks)celery_app.py(3 hunks)docker-entrypoint.sh(1 hunks)fly.toml(3 hunks)job.py(7 hunks)redisCache.py(5 hunks)storage.py(3 hunks)utils.py(7 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
utils.py (1)
redisCache.py (1)
redis_xadd(142-185)
job.py (1)
redisCache.py (1)
redis_xread(188-224)
api.py (2)
crawler_pool.py (1)
get_crawler(25-49)utils.py (4)
decode_redis_hash(206-215)create_task_status_response(237-270)TaskStatus(25-35)should_cleanup_task(156-159)
🪛 Ruff (0.13.1)
utils.py
341-341: Prefer TypeError exception for invalid type
(TRY004)
341-341: Abstract raise to an inner function
(TRY301)
361-361: Do not catch blind exception: Exception
(BLE001)
362-362: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
373-373: Do not catch blind exception: Exception
(BLE001)
374-374: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
redisCache.py
39-39: Avoid specifying long messages outside the exception class
(TRY003)
71-71: Consider moving this statement to an else block
(TRY300)
72-72: Do not catch blind exception: Exception
(BLE001)
112-112: Abstract raise to an inner function
(TRY301)
112-112: Avoid specifying long messages outside the exception class
(TRY003)
158-158: Abstract raise to an inner function
(TRY301)
158-158: Avoid specifying long messages outside the exception class
(TRY003)
182-182: Consider moving this statement to an else block
(TRY300)
183-183: Do not catch blind exception: Exception
(BLE001)
212-212: Loop control variable last_id not used within loop body
(B007)
214-214: Loop control variable channel not used within loop body
(B007)
221-221: Consider moving this statement to an else block
(TRY300)
222-222: Do not catch blind exception: Exception
(BLE001)
job.py
457-457: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
493-493: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
508-508: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
api.py
682-682: Do not catch blind exception: Exception
(BLE001)
684-684: Use explicit conversion flag
Replace with conversion flag
(RUF010)
685-685: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
696-696: Use explicit conversion flag
Replace with conversion flag
(RUF010)
712-712: Redundant exception object included in logging.exception call
(TRY401)
celery_app.py
32-32: Avoid specifying long messages outside the exception class
(TRY003)
82-82: Unused function argument: args
(ARG001)
storage.py
85-85: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
87-87: Do not catch blind exception: Exception
(BLE001)
88-88: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
131-131: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
133-133: Do not catch blind exception: Exception
(BLE001)
134-134: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
183-183: Do not catch blind exception: Exception
(BLE001)
184-184: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
192-192: Do not catch blind exception: Exception
(BLE001)
193-193: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
223-223: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
226-226: Do not catch blind exception: Exception
(BLE001)
258-258: Do not catch blind exception: Exception
(BLE001)
259-259: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
291-291: Do not catch blind exception: Exception
(BLE001)
292-292: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
334-334: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
335-335: Use explicit conversion flag
Replace with conversion flag
(RUF010)
336-336: Do not catch blind exception: Exception
(BLE001)
337-337: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
338-338: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🪛 GitHub Check: CodeQL
api.py
[warning] 701-701: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🔇 Additional comments (28)
utils.py (3)
249-265: Logic issue: Redis result is never parsed when Celery result is not available.The condition
elif not hasattr(celery_task, 'result')will never be true sinceAsyncResultalways has aresultattribute. This means the fallback to Redis result parsing is unreachable.
365-379: Pipeline execution outside try block could leak resources.The
pipe2pipeline is created but not executed until after the try block (lines 365-376, then executed at 379). If an exception occurs, the pipeline commands are queued but never executed.Apply this diff to ensure pipeline execution:
- pipe2.xadd(channel, {key: str(value) if isinstance(value, bool) else value for key, value in error_response.items() } ) + await redis_xadd(pipe2, channel, error_response) complete = {"status": "error", "message": "completed"} - pipe2.xadd(channel, {key: str(value) if isinstance(value, bool) else value for key, value in complete.items()}) + await redis_xadd(pipe2, channel, complete) except asyncio.CancelledError: logger.warning("Client disconnected during streaming") - pipe2.xadd(channel, {"status": "canceled", "message": "streaming canceled"}) + await redis_xadd(pipe2, channel, {"status": "canceled", "message": "streaming canceled"}) except Exception as e: logger.error(f"Unexpected error in stream_pubsub_results: {e}") - pipe2.xadd(channel, {"status": "error", "message": str(e)}) - await pipe2.execute() + await redis_xadd(pipe2, channel, {"status": "error", "message": str(e)}) return False await pipe2.execute()
345-345:chunk_sizedefinition appears after its usage.While the code now defines
chunk_size, it's defined on line 345 but the variable is conceptually first needed on line 347 for the calculation.Apply this diff to move the definition earlier for clarity:
batch_json = json.dumps(model_dump, default=datetime_handler, ensure_ascii=False) - pipe = redis.pipeline() chunk_size = 4096 # Define chunk_size as a constant (adjust as needed) + pipe = redis.pipeline() total_chunks = (len(batch_json) + chunk_size - 1) // chunk_size # Calculate total chunksapi.py (2)
519-530: Unsafe cancellation on Windows still present.Broadcasting 'shutdown' kills all workers, not just the task. This is dangerous for production.
Apply this diff to fix the Windows cancellation:
# Windows-compatible task revocation if os.name == 'nt': # Windows - celery_task.revoke(terminate=True) - - # Force termination on Windows needs special handling - if force: - # Send shutdown event to worker - celery_app.control.broadcast('shutdown') + # Windows pools don't support POSIX signals; rely on terminate=True + celery_task.revoke(terminate=True, signal='SIGKILL' if force else None) else: # POSIX systems celery_task.revoke( terminate=True, - signal=signal.SIGKILL if force else signal.SIGTERM + signal='SIGKILL' if force else 'SIGTERM' )
673-697: SSE formatting issues: missing double newlines.SSE events must end with double newlines for proper parsing.
Apply this diff to fix SSE formatting:
# Generate status response response = create_task_status_response(celery_task, task, task_id, base_url) - data = f"data: {json.dumps(response)}\n\n" # Note the double newline for SSE format + data = f"data: {json.dumps(response)}\n\n" # Correct - already has double newline yield data.encode('utf-8') # Ensure we're yielding bytes # Exit when the task is complete if celery_task_ready: break except Exception as e: # Handle any serialization errors error_msg = f"Error generating status response: {str(e)}" - logger.error(error_msg) - yield f"data: {json.dumps({'error': error_msg})}\n".encode('utf-8') + logger.exception("Error generating status response") + yield f"event: error\ndata: {json.dumps({'error': 'Status generation error'})}\n\n".encode('utf-8') # Wait before checking again await asyncio.sleep(1) # Send the [DONE] marker to end the stream yield b"data: [DONE]\n\n" except Exception as e: # TODO: Handle exceptions in the streaming loop IN the frontend - logger.error(f"Fatal error in status stream: {str(e)}", exc_info=True) + logger.exception("Fatal error in status stream") yield f"event: error\ndata: {json.dumps({'error': 'A fatal error occurred while streaming the task status.', 'fatal': True})}\n\n".encode('utf-8') yield b"data: [DONE]\n\n"job.py (2)
431-433: Operator precedence issue causes incorrect logic.The condition mixes
andandorwithout proper parentheses, causing incorrect evaluation.Apply this diff to fix the operator precedence:
- elif retries > max_retries and not completed_yielded and (celery_task.state in {"PENDING", "STARTED"}): + elif retries > max_retries and not completed_yielded and (celery_task.state == "PENDING" or celery_task.state == "STARTED"): retries = 0Or more concisely:
- elif retries > max_retries and not completed_yielded and (celery_task.state in {"PENDING", "STARTED"}): + elif (retries > max_retries and not completed_yielded) and (celery_task.state in {"PENDING", "STARTED"}): retries = 0
465-510: SSE event formatting missing required double newlines.SSE events require double newline terminators for proper client parsing.
fly.toml (5)
9-12: LGTM!The addition of explicit Dockerfile configuration and placeholder for build-target provides better build process control and transparency.
15-23: Good production environment configuration.The addition of PYTHON_ENV='production' and Redis configuration parameters provides better environment distinction and resilience settings.
64-67: Simplified process configuration looks good.The move to explicit
execcommands and separation of app/worker processes improves clarity and process management compared to the complex shell command chains in the comments.
77-77: Good addition of swap for compilation.Adding 512MB swap will help with memory-intensive compilation processes during builds.
23-23: Verify VNC_PASSWORD secret is configured on Fly.ioFly CLI was unavailable in the verification environment — run:
fly secrets list -a crawlagent | grep -i VNC
If not present, set it:
fly secrets set VNC_PASSWORD= -a crawlagentdocker-entrypoint.sh (7)
1-2: LGTM!Good use of
set -eufor error handling and script robustness.
17-26: VNC authentication implementation looks correct.The VNC authentication setup properly handles both password-protected and development modes. The password storage and authentication arguments are correctly configured.
28-31: VNC security configuration implemented correctly.The x11vnc configuration now binds to localhost only (
-localhost) and uses proper authentication as recommended in the previous review.
33-56: Excellent cleanup and signal handling.The cleanup function properly handles process termination with SIGTERM and waits for processes to exit gracefully. Signal trapping is correctly configured for INT, TERM, and QUIT signals.
67-80: Smart process differentiation for worker vs app.The distinction between worker and app processes is well-implemented, starting minimal services for workers and full services (including noVNC) for app processes. The conditional noVNC startup prevents unnecessary resource usage in worker containers.
82-83: Proper command execution with exec.Using
exec "$@"correctly replaces the shell process with the main command, ensuring proper signal handling and process management.
74-79: noVNC installation path is consistent. Dockerfile clones noVNC to /opt/noVNC and docker-entrypoint.sh references /opt/noVNC (Dockerfile lines 93–94; docker-entrypoint.sh lines 74–75).Dockerfile (9)
1-2: LGTM!The multi-stage build with Python 3.12-slim is a good choice for modern Docker practices, providing both build efficiency and smaller final image size.
4-12: Good build environment configuration.The environment variables are appropriately set for a production build, including
DEBIAN_FRONTEND=noninteractiveto prevent interactive prompts during package installation.
20-25: Build dependencies properly addressed.The build stage now includes the necessary compilation toolchains and development headers as recommended in the previous review, ensuring wheel building will succeed for packages with C extensions.
46-97: Runtime dependencies still include build tools.The final stage continues to install heavy build dependencies that should only be needed during the build phase, increasing the final image size unnecessarily.
Apply this diff to remove build-only dependencies from the runtime stage:
RUN apt-get update && apt-get install -y --no-install-recommends \ fonts-liberation \ ca-certificates \ lsof \ - # Add build dependencies for madoka - build-essential \ curl \ wget \ gnupg \ - cmake \ - gcc \ - g++ \ - pkg-config \ - python3-dev \ - libjpeg-dev \ supervisor \Move any required build dependencies back to the builder stage if specific packages fail to build as wheels.
99-103: Good security implementation with non-root user.The creation of a dedicated
appuserwith appropriate directory ownership and permissions follows Docker security best practices.
105-115: Excellent wheel-based installation approach.The multi-stage build correctly uses pre-built wheels from the builder stage, installs Playwright and Chromium, and includes validation checks to ensure components are working properly.
134-136: Correct entrypoint script installation but potential permission conflict.The entrypoint script is copied as root with correct permissions, but there may be a conflict with the script attempting to run
chowncommands as a non-root user (see docker-entrypoint.sh review).Ensure the X11 directory setup in docker-entrypoint.sh is handled correctly given the USER appuser directive. Consider moving the X11 directory setup to the Dockerfile build stage:
+# Setup X11 directory as root before switching to non-root user +RUN mkdir -p /tmp/.X11-unix \ + && chmod 1777 /tmp/.X11-unix \ + && chown appuser:appuser /tmp/.X11-unix + # Switch to non-root user USER appuserAnd remove the corresponding lines from docker-entrypoint.sh.
144-145: Simple and effective startup command.The streamlined CMD using uvicorn directly is much cleaner than the previous complex shell commands and aligns well with the new entrypoint approach.
93-94: noVNC and websockify versions still unpinned.The git clones continue to use floating branches, which reduces build reproducibility and poses supply chain risks.
Based on the web search results, I can see that noVNC's latest version is v1.6.0, published on March 12, 2025, and websockify's latest version is v0.13.0.
Apply this diff to pin to specific release tags:
- && git clone --depth 1 https://github.com/novnc/noVNC /opt/noVNC \ - && git clone --depth 1 https://github.com/novnc/websockify /opt/noVNC/utils/websockify \ + && git clone --depth 1 --branch v1.6.0 https://github.com/novnc/noVNC /opt/noVNC \ + && git clone --depth 1 --branch v0.13.0 https://github.com/novnc/websockify /opt/noVNC/utils/websockify \This ensures build reproducibility and reduces supply chain security risks.
| internal_port = 6080 | ||
| protocol = "tcp" | ||
| processes = ["app","worker"] | ||
| processes = ["app"] # , "worker" Critical: Remove public noVNC (6080) or enforce VNC auth + restrict binding |
There was a problem hiding this comment.
🧩 Analysis chain
Critical security concern acknowledged but not fully resolved.
The comment correctly identifies the security risk of exposing noVNC publicly on port 6080, but the service configuration still allows public access. The docker-entrypoint.sh does implement VNC authentication, but this remains a security concern for production deployment.
Consider removing this service entirely from production or implementing additional access controls:
-[[services]]
- internal_port = 6080
- protocol = "tcp"
- processes = ["app"] # , "worker" Critical: Remove public noVNC (6080) or enforce VNC auth + restrict binding
- [[services.ports]]
- port = 6080
- handlers = ["tls", "http"] # Ensure "tls" is included for HTTPSIf noVNC access is required, consider:
- Restricting access via Fly.io access controls
- Implementing additional authentication layers
- Using Fly.io's private networking features
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Searching repo for '6080', 'noVNC', 'novnc', 'vnc' occurrences ==="
rg -n --hidden -S --no-ignore -C3 '6080|noVNC|novnc|vncpasswd|x11vnc|vnc' || true
echo
echo "=== Showing fly.toml if present ==="
if [ -f fly.toml ]; then
echo "Found fly.toml at repo root:"
nl -ba fly.toml | sed -n '1,240p'
else
echo "No fly.toml at repo root. Searching for any fly.toml in repo..."
rg -n --hidden -S --no-ignore 'fly.toml' || true
fi
echo
echo "=== Listing docker-entrypoint.sh files ==="
rg --hidden --files 'docker-entrypoint.sh' || true
echo
echo "=== Showing contents of docker-entrypoint.sh files (if any) ==="
while IFS= read -r f; do
if [ -f "$f" ]; then
echo "---- $f ----"
nl -ba "$f" | sed -n '1,400p'
fi
done < <(rg --hidden --files 'docker-entrypoint.sh' || true)
echo
echo "=== Showing Dockerfile(s) and lines referencing VNC/noVNC/ENTRYPOINT/CMD ==="
rg -n --hidden -S --no-ignore 'noVNC|novnc|vnc|x11vnc|vncpasswd|ENTRYPOINT|CMD' -g 'Dockerfile' || true
rg -n --hidden -S --no-ignore 'noVNC|novnc|vnc|x11vnc|vncpasswd|ENTRYPOINT|CMD' || trueLength of output: 4822
Critical: remove public noVNC (port 6080) or enforce strong auth + restrict binding
fly.toml still exposes internal_port 6080 publicly and docker-entrypoint.sh launches websockify bound to 0.0.0.0:6080 while x11vnc falls back to -nopw when VNC_PASSWORD is unset — unauthenticated public access is possible.
Required fixes:
- Remove the public 6080 service entry in fly.toml (stop exposing the port).
- If noVNC is required in production: make VNC_PASSWORD mandatory (fail startup if unset), bind websockify to localhost (or require an authenticated proxy), and use Fly access controls or private networking; do not rely on an unsettable dev fallback.
- Verify TLS and proxy termination meet your threat model (handlers = ["tls","http"] alone does not prevent unauthenticated access).
Locations: fly.toml (lines ~57–62), docker-entrypoint.sh (x11vnc auth ~lines 19–26; websockify run ~lines 73–76), Dockerfile (EXPOSE 6080 ~line 128).
🤖 Prompt for AI Agents
In fly.toml around lines 57–62, docker-entrypoint.sh around lines 19–26 and
73–76, and Dockerfile around line 128: the noVNC service (port 6080) is exposed
publicly and websockify/x11vnc can start unauthenticated when VNC_PASSWORD is
unset. Remove the public 6080 service entry from fly.toml (stop exposing the
port), remove or stop EXPOSE 6080 in the Dockerfile, and change
docker-entrypoint.sh to require VNC_PASSWORD (exit startup with a clear error if
unset), bind websockify to localhost (127.0.0.1) or use an authenticated proxy
instead of 0.0.0.0, and ensure TLS/proxy termination and Fly access controls or
private networking are used for any remaining access.
| if isinstance(redis, Redis) and hasattr(pipe, "execute"): | ||
| message_id = await pipe.execute("XADD", *pieces) | ||
| elif isinstance(redis, PureRedis) and hasattr(pipe, "execute_command"): | ||
| message_id = await pipe.execute_command("XADD", *pieces) |
There was a problem hiding this comment.
Type checking logic is inconsistent with declared function signature.
The function checks for isinstance(redis, Redis) on line 178 but the global redis variable is already typed as Redis. This creates confusion and could cause issues with the fallback logic.
The condition should check against the pipe parameter type, not the global redis:
# Fallback: Upstash `execute` or redis-py `execute_command`
# Flatten into varargs for the underlying client
- if isinstance(redis, Redis) and hasattr(pipe, "execute"):
+ if isinstance(pipe, Redis) and hasattr(pipe, "execute"):
message_id = await pipe.execute("XADD", *pieces)
- elif isinstance(redis, PureRedis) and hasattr(pipe, "execute_command"):
+ elif isinstance(pipe, PureRedis) and hasattr(pipe, "execute_command"):
message_id = await pipe.execute_command("XADD", *pieces) 📝 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.
| if isinstance(redis, Redis) and hasattr(pipe, "execute"): | |
| message_id = await pipe.execute("XADD", *pieces) | |
| elif isinstance(redis, PureRedis) and hasattr(pipe, "execute_command"): | |
| message_id = await pipe.execute_command("XADD", *pieces) | |
| if isinstance(pipe, Redis) and hasattr(pipe, "execute"): | |
| message_id = await pipe.execute("XADD", *pieces) | |
| elif isinstance(pipe, PureRedis) and hasattr(pipe, "execute_command"): | |
| message_id = await pipe.execute_command("XADD", *pieces) |
🤖 Prompt for AI Agents
In redisCache.py around lines 178 to 181, the type check is incorrectly testing
the global `redis` variable instead of the `pipe` parameter; change the
conditions to check the `pipe` instance (e.g., `isinstance(pipe, Redis)` and
`isinstance(pipe, PureRedis)`) or use duck-typing by testing for the appropriate
method (`hasattr(pipe, "execute")` vs `hasattr(pipe, "execute_command")`) so the
correct branch is chosen; ensure any type names used are imported/available in
this module.
| break | ||
|
|
||
| # Decompress chunk and yield | ||
| yield decompressor.decompress(chunk) |
There was a problem hiding this comment.
Incorrect decompression of partial chunks in streaming.
The streaming decompression attempts to decompress individual chunks, but zstd requires the full stream context for proper decompression.
Apply this diff to use streaming decompression properly:
# Use zstd streaming decompression for memory efficiency
decompressor = zstd.ZstdDecompressor()
compressed_stream = response["Body"]
- # Read and decompress in chunks
- chunk_size = 1024 * 1024 # 1MB chunks
- while True:
- chunk = await compressed_stream.read(chunk_size)
- if not chunk:
- break
-
- # Decompress chunk and yield
- yield decompressor.decompress(chunk)
+ # Read the entire compressed stream first
+ compressed_data = await compressed_stream.read()
+
+ # Decompress and yield in chunks
+ decompressed_data = decompressor.decompress(compressed_data)
+ chunk_size = 1024 * 1024 # 1MB chunks
+ for i in range(0, len(decompressed_data), chunk_size):
+ yield decompressed_data[i:i+chunk_size]📝 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.
| yield decompressor.decompress(chunk) | |
| # Use zstd streaming decompression for memory efficiency | |
| decompressor = zstd.ZstdDecompressor() | |
| compressed_stream = response["Body"] | |
| # Read the entire compressed stream first | |
| compressed_data = await compressed_stream.read() | |
| # Decompress and yield in chunks | |
| decompressed_data = decompressor.decompress(compressed_data) | |
| chunk_size = 1024 * 1024 # 1MB chunks | |
| for i in range(0, len(decompressed_data), chunk_size): | |
| yield decompressed_data[i:i+chunk_size] |
🤖 Prompt for AI Agents
In storage.py around line 329, the code incorrectly calls
decompressor.decompress(chunk) per chunk which breaks zstd streaming; replace
this with a streaming decompressor object (e.g. decompressor =
zstd.ZstdDecompressor().decompressobj()), feed each incoming chunk into
decompressor.decompress(chunk) and yield the returned data, and after the loop
call decompressor.flush() and yield any remaining bytes; also handle and ignore
empty results to avoid yielding zero-length bytes.
This commit significantly refactors the Docker build process by integrating `uv` for Python dependency management. These changes aim to improve build performance, consistency, and maintainability of the Docker images. Key changes include: * **Introduced `uv`:** Replaced `pip` with `uv` for installing Python packages, leveraging its speed and reliability for dependency resolution and installation. This involves adding a dedicated `uv` build stage and updating installation commands in both builder and final stages. * **Streamlined dependency installation:** Removed `PIP_*` environment variables and simplified the wheel building process. System-level build dependencies are now explicitly listed in the final stage for robustness. * **Refined Dockerfile structure:** Updated environment variables, adjusted the `docker-entrypoint.sh` handling for better clarity, and switched to direct `uvicorn` execution in the `CMD`. * **Updated `fly.toml`:** Removed the `AWS_ENDPOINT_URL_S3` configuration, which is no longer needed. * Make sure the file has LF line endings (\n), not CRLF (\r\n), otherwise No such file or directory can also appear. Ensure #!/bin/sh at the top of the script is correct and sh exists in the base image.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
Dockerfile (3)
33-37: Builder stage is unused; either build wheels there or drop the stageYou install deps in the builder but don’t copy any artifacts (only requirements.txt). Prefer building wheels in the builder and installing from them in final, or remove the builder entirely.
Option A (use wheels):
# Builder -RUN --mount=from=uv,source=/uv,target=/bin/uv \ - --mount=type=cache,target=/root/.cache/uv \ - uv pip install --system -r requirements.txt +RUN --mount=from=uv,source=/uv,target=/bin/uv \ + --mount=type=cache,target=/root/.cache/uv \ + mkdir -p /wheels \ + && uv pip wheel -w /wheels -r requirements.txt # Final -# COPY --from=builder /app/wheels /wheels +COPY --from=builder /wheels /wheels -RUN --mount=type=cache,target=/root/.cache/uv \ - uv pip install --system -r requirements.txt && \ - uv pip install --system playwright websockets && \ +RUN --mount=type=cache,target=/root/.cache/uv \ + uv pip install --system --no-index --find-links=/wheels -r requirements.txt && \ + uv pip install --system playwright && \ playwright install --with-deps chromiumOption B (drop builder): remove the builder stage entirely and copy requirements.txt directly in final.
Also applies to: 121-129
153-156: Remove debug listing in Docker layerThe ls -la adds noise and an extra layer. Keep chmod/chown only.
Apply:
-RUN chmod +x /usr/local/bin/docker-entrypoint.sh && \ - chown root:root /usr/local/bin/docker-entrypoint.sh && \ - ls -la /usr/local/bin/docker-entrypoint.sh # Verify permissions +RUN chmod +x /usr/local/bin/docker-entrypoint.sh && \ + chown root:root /usr/local/bin/docker-entrypoint.sh
121-123: Remove dead, commented-out wheels COPYKeeps the Dockerfile clean if you don’t adopt wheels.
-# COPY --from=builder /app/wheels /wheels
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
Dockerfile(1 hunks)fly.toml(3 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- fly.toml
🔇 Additional comments (2)
Dockerfile (2)
63-80: Move build toolchains out of the final image; add noninteractive env for aptFinal stage should not include heavy build deps (build-essential, gcc/g++, python3-dev, pkg-config, libjpeg-dev, cmake). Keep runtime-only libs here; install build deps in the builder stage only. Also set DEBIAN_FRONTEND in this stage to avoid prompts. (Same as prior feedback.)
Apply:
+ENV DEBIAN_FRONTEND=noninteractive RUN apt-get update && apt-get install -y --no-install-recommends \ fonts-liberation \ ca-certificates \ lsof \ - # Add build dependencies for madoka - build-essential \ curl \ - gcc \ - g++ \ - python3-dev \ - pkg-config \ - libjpeg-dev \ - cmake \ wget \ gnupg \ supervisor \If any wheel fails in the builder, add only the minimal extra build deps to the builder stage, not here.
109-110: Pin noVNC and websockify to release tags for reproducibilityFloating HEADs risk supply‑chain drift. (Same as prior feedback.)
Apply:
- && git clone --depth 1 https://github.com/novnc/noVNC /opt/noVNC \ - && git clone --depth 1 https://github.com/novnc/websockify /opt/noVNC/utils/websockify \ + && git clone --depth 1 --branch v1.5.0 https://github.com/novnc/noVNC /opt/noVNC \ + && git clone --depth 1 --branch v0.12.0 https://github.com/novnc/websockify /opt/noVNC/utils/websockify \
- add regions.md to .gitignore - remove unnecessary websockets installation
This pull request introduces several improvements and refactorings to the Docker build, Celery configuration, and task status streaming logic. The changes focus on modernizing the Docker setup, improving cross-platform compatibility (especially for Windows), and enhancing the reliability and clarity of task status reporting via FastAPI.
Dockerfile and build process modernization:
Dockerfileto use Python 3.12, a multi-stage build, and wheel caching for faster, cleaner dependency installation. System dependencies are consolidated, non-root user handling is improved, and the entrypoint is now set via a dedicated script.Dockerfile.oldto preserve the previous build process for reference or rollback.Celery configuration and cross-platform support:
.envordev.envbased on environment, renames the app to"crawlagent", and applies stricter timeouts and result expiration. Windows-specific settings are added for improved task cancellation and error reporting. [1] [2] [3]Task cancellation and status streaming improvements:
[DONE]marker at the end of the stream. Also improves response headers for streaming.Codebase consistency and imports:
api.pyfor clarity and reliability.These changes collectively improve build reliability, cross-platform compatibility, and user experience for task management and monitoring.
References:
[1] [2] [3] [4] [5] [6] [7] [8]
Summary by CodeRabbit
New Features
Improvements
Chores