diff --git a/app/__init__.py b/app/__init__.py index 3a6a505..3c018a0 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -3,8 +3,12 @@ import atexit import os import threading +import time +import traceback -from flask import Flask +from flask import Flask, g, jsonify, request +from flask.wrappers import Response +from werkzeug.exceptions import HTTPException from app.config import get_package_version, get_settings @@ -12,6 +16,17 @@ _cleanup_thread: threading.Thread | None = None _cleanup_stop_event = threading.Event() +# Background log-sync thread control +_log_sync_thread: threading.Thread | None = None +_log_sync_stop_event = threading.Event() +# Set to wake the sync worker immediately (e.g. right after an error is logged). +_log_sync_trigger_event = threading.Event() +# How often to sync logs to S3 when nothing has triggered an early sync (seconds). +_LOG_SYNC_INTERVAL = 300 +# Request paths to exclude from automatic per-request logging (noisy / would +# create feedback loops with the logging pipeline itself). +_REQUEST_LOG_SKIP_PREFIXES = ("/api/logs", "/api/upload/progress") + def _sse_cleanup_worker() -> None: """Background worker that periodically cleans up stale SSE queues.""" @@ -52,6 +67,128 @@ def _stop_sse_cleanup() -> None: _cleanup_thread.join(timeout=2.0) +def _run_log_sync() -> None: + """Sync log files to S3 once, if a bucket is configured. + + Failures are logged as WARNING (never ERROR) so that a persistently failing + sync cannot re-trigger itself through the error hook and hot-loop. + """ + from app.services import s3_service + from app.services.log_service import get_log_service + + settings = get_settings() + log = get_log_service() + + if not settings.s3_bucket: + return # Nothing to sync to yet; stay quiet. + + try: + client = s3_service.create_s3_client(settings.aws_profile, settings.aws_region) + log.sync_logs_to_s3(client, settings.s3_bucket) + except Exception as e: + log.warning( + "sync", + "log_sync_failed", + f"Automatic log sync to S3 failed: {e}", + {"error": str(e)}, + ) + + +def _log_sync_worker() -> None: + """Background worker: sync logs to S3 periodically and on demand. + + Wakes every ``_LOG_SYNC_INTERVAL`` seconds, or immediately whenever + ``_log_sync_trigger_event`` is set (e.g. just after an error is logged). + """ + while not _log_sync_stop_event.is_set(): + # Wait for either the interval to elapse or an explicit trigger. + _log_sync_trigger_event.wait(timeout=_LOG_SYNC_INTERVAL) + if _log_sync_stop_event.is_set(): + break + _log_sync_trigger_event.clear() + _run_log_sync() + + +def _start_log_sync() -> None: + """Start the background log-sync thread and register the error trigger.""" + global _log_sync_thread + + from app.services.log_service import get_log_service + + # Logging an ERROR wakes the sync worker for a near-immediate upload. + get_log_service().set_error_callback(_log_sync_trigger_event.set) + + if _log_sync_thread is None: + _log_sync_thread = threading.Thread(target=_log_sync_worker, daemon=True, name="LogSync") + _log_sync_thread.start() + + +def _stop_log_sync() -> None: + """Stop the background log-sync thread (flushing one final sync).""" + _log_sync_stop_event.set() + _log_sync_trigger_event.set() # Wake it so it can observe the stop flag. + if _log_sync_thread: + _log_sync_thread.join(timeout=2.0) + + +def _register_logging_hooks(app: Flask) -> None: + """Wire up automatic per-request logging and a global error handler. + + - Every ``/api/*`` request is logged (method, path, status, duration) so user + actions are captured without per-route boilerplate. + - Any uncaught exception is logged at ERROR level with a traceback, which also + triggers a near-immediate S3 sync via the log service's error hook. + """ + from app.services.log_service import get_log_service + + def _should_log(path: str) -> bool: + return path.startswith("/api/") and not path.startswith(_REQUEST_LOG_SKIP_PREFIXES) + + @app.before_request + def _stamp_request_start() -> None: + g._request_start = time.monotonic() + + @app.after_request + def _log_request(response: Response) -> Response: + if _should_log(request.path): + start = getattr(g, "_request_start", None) + duration_ms = round((time.monotonic() - start) * 1000, 1) if start else None + level = "WARNING" if response.status_code >= 400 else "INFO" + get_log_service().log( + level, + "request", + "http_request", + f"{request.method} {request.path} -> {response.status_code}", + { + "method": request.method, + "path": request.path, + "status": response.status_code, + "duration_ms": duration_ms, + "remote_addr": request.remote_addr, + }, + ) + return response + + @app.errorhandler(Exception) + def _handle_uncaught(error: Exception) -> Response | tuple[Response, int] | HTTPException: + # Let normal HTTP errors (404, 405, explicit aborts, ...) keep their + # default handling; only true crashes are logged as ERROR. + if isinstance(error, HTTPException): + return error + get_log_service().error( + "error", + "unhandled_exception", + f"Unhandled {type(error).__name__} on {request.method} {request.path}: {error}", + { + "exception_type": type(error).__name__, + "method": request.method, + "path": request.path, + "traceback": traceback.format_exc(), + }, + ) + return jsonify({"error": "Internal server error"}), 500 + + def create_app() -> Flask: """Create and configure the Flask application.""" app = Flask(__name__) @@ -86,11 +223,18 @@ def inject_display_name() -> dict[str, str]: app.register_blueprint(delete_bp, url_prefix="/api/delete") app.register_blueprint(large_folder_upload_bp, url_prefix="/api/large-folder-upload") + # Automatic request/error logging hooks + _register_logging_hooks(app) + # Start background SSE cleanup thread _start_sse_cleanup() + # Start background log-sync thread (periodic + error-triggered S3 upload) + _start_log_sync() + # Register cleanup on shutdown atexit.register(_stop_sse_cleanup) + atexit.register(_stop_log_sync) # Log application startup from app.services.log_service import get_log_service diff --git a/app/routes/logs.py b/app/routes/logs.py index 394783f..8d2c2d1 100644 --- a/app/routes/logs.py +++ b/app/routes/logs.py @@ -91,6 +91,39 @@ def get_upload_stats() -> tuple[Response, int]: return jsonify(stats), 200 +@logs_bp.route("/client", methods=["POST"]) +def log_client_event() -> tuple[Response, int]: + """Record a client-side (browser) event, typically a JS error. + + Frontend global error hooks POST here so browser failures land in the same + JSONL pipeline (and S3 sync) as backend logs. + + JSON body: + level: INFO/WARNING/ERROR (default ERROR) + event: machine-readable event name (default client_error) + message: human-readable message + metadata: optional dict of extra context (url, stack, userAgent, ...) + """ + data = request.get_json(silent=True) or {} + + level = str(data.get("level", "ERROR")).upper() + if level not in ("INFO", "WARNING", "ERROR"): + level = "ERROR" + event = str(data.get("event", "client_error")) + message = str(data.get("message", "")) or "Client-side event" + + metadata = data.get("metadata") + if not isinstance(metadata, dict): + metadata = {} + # Always capture where it came from so logs are attributable. + metadata.setdefault("user_agent", request.headers.get("User-Agent", "")) + metadata.setdefault("remote_addr", request.remote_addr) + + log = get_log_service() + log.log(level, "client", event, message, metadata) + return jsonify({"success": True}), 200 + + @logs_bp.route("/sync", methods=["POST"]) def sync_logs() -> tuple[Response, int]: """Trigger S3 sync of log files. diff --git a/app/services/log_service.py b/app/services/log_service.py index 854be2c..1777853 100644 --- a/app/services/log_service.py +++ b/app/services/log_service.py @@ -9,6 +9,7 @@ import json import re import threading +from collections.abc import Callable from datetime import UTC, datetime from pathlib import Path from typing import Any @@ -22,6 +23,16 @@ class LogService: def __init__(self) -> None: """Initialize the log service.""" self._write_lock = threading.Lock() + self._error_callback: Callable[[], None] | None = None + + def set_error_callback(self, callback: Callable[[], None] | None) -> None: + """Register a callback fired (best-effort) after every ERROR-level log. + + Used to trigger an immediate S3 log sync when something goes wrong. + The callback must be cheap and non-blocking (e.g. set a threading.Event); + it runs on the thread that logged the error. + """ + self._error_callback = callback def _get_log_dir(self) -> Path: """Get the configured log directory, creating it if needed.""" @@ -102,6 +113,14 @@ def log( with open(log_file, "a", encoding="utf-8") as f: f.write(line + "\n") + # Fire the error hook outside the write lock so a slow/failing callback + # can never block logging. Never let it raise back into the caller. + if entry["level"] == "ERROR" and self._error_callback is not None: + try: + self._error_callback() + except Exception: + pass + def info( self, category: str, @@ -568,7 +587,7 @@ def sync_logs_to_s3( self, s3_client: Any, bucket: str, - prefix: str = "logs/", + prefix: str = "app_logs/", ) -> dict[str, Any]: """Upload new/changed log files (JSONL + CSV) to S3. diff --git a/frontend/src/main.tsx b/frontend/src/main.tsx index 8ec974e..d796907 100644 --- a/frontend/src/main.tsx +++ b/frontend/src/main.tsx @@ -3,6 +3,10 @@ import { createRoot } from 'react-dom/client'; import { BrowserRouter } from 'react-router-dom'; import App from './App.tsx'; import './index.css'; +import { installGlobalErrorReporting } from './utils/errorReporter.ts'; + +// Capture uncaught browser errors / promise rejections and ship them to S3. +installGlobalErrorReporting(); // biome-ignore lint/style/noNonNullAssertion: root element is guaranteed to exist in index.html createRoot(document.getElementById('root')!).render( diff --git a/frontend/src/utils/errorReporter.ts b/frontend/src/utils/errorReporter.ts new file mode 100644 index 0000000..fa3ee45 --- /dev/null +++ b/frontend/src/utils/errorReporter.ts @@ -0,0 +1,89 @@ +/** + * Global browser error reporting. + * + * Captures uncaught errors and unhandled promise rejections and ships them to + * the backend (`POST /api/logs/client`), where they join the same JSONL log + * pipeline and S3 sync as server-side errors. + */ + +interface ClientLogPayload { + level?: 'INFO' | 'WARNING' | 'ERROR'; + event?: string; + message: string; + metadata?: Record; +} + +let installed = false; + +/** Best-effort POST; never throws and never reports its own failures. */ +function report(payload: ClientLogPayload): void { + try { + void fetch('/api/logs/client', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(payload), + keepalive: true, // survive page unload / navigation + }).catch(() => { + /* swallow: reporting must never cascade into more errors */ + }); + } catch { + /* swallow */ + } +} + +function baseMetadata(): Record { + return { + url: window.location.href, + user_agent: navigator.userAgent, + }; +} + +/** Install global `error` and `unhandledrejection` listeners. Idempotent. */ +export function installGlobalErrorReporting(): void { + if (installed) return; + installed = true; + + window.addEventListener('error', (e: ErrorEvent) => { + report({ + level: 'ERROR', + event: 'uncaught_error', + message: e.message || 'Uncaught error', + metadata: { + ...baseMetadata(), + source: e.filename, + line: e.lineno, + column: e.colno, + stack: e.error instanceof Error ? e.error.stack : undefined, + }, + }); + }); + + window.addEventListener('unhandledrejection', (e: PromiseRejectionEvent) => { + const reason = e.reason; + const message = + reason instanceof Error + ? reason.message + : typeof reason === 'string' + ? reason + : 'Unhandled promise rejection'; + report({ + level: 'ERROR', + event: 'unhandled_rejection', + message, + metadata: { + ...baseMetadata(), + stack: reason instanceof Error ? reason.stack : undefined, + }, + }); + }); +} + +/** Manually report a client-side error (e.g. from a React error boundary). */ +export function reportClientError(message: string, metadata?: Record): void { + report({ + level: 'ERROR', + event: 'client_error', + message, + metadata: { ...baseMetadata(), ...metadata }, + }); +} diff --git a/tests/conftest.py b/tests/conftest.py index e3896aa..29a0f14 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -45,12 +45,23 @@ def app() -> Generator[Flask, None, None]: # Create the app (settings will be loaded from default) _ = SETTINGS_FILE # Reference to avoid unused import warning + # Redirect logs to a temp dir so the automatic request/error logging hooks + # don't write into the repo's real logs/ directory during tests. + from app.config import get_settings + + settings = get_settings() + log_temp_dir = tempfile.mkdtemp() + original_log_dir = settings._settings.get("log_directory") + settings._settings["log_directory"] = log_temp_dir + app = create_app() app.config["TESTING"] = True yield app # Cleanup + if original_log_dir is not None: + settings._settings["log_directory"] = original_log_dir os.unlink(temp_settings) if dist_created: os.unlink(index_path) diff --git a/tests/test_log_service.py b/tests/test_log_service.py index 132f856..45d6f04 100644 --- a/tests/test_log_service.py +++ b/tests/test_log_service.py @@ -340,6 +340,22 @@ def test_sync_uses_relative_hive_paths( assert "/month=" in s3_key assert "/day=" in s3_key + def test_sync_default_prefix_is_app_logs( + self, log_service: LogService, _mock_settings: Any + ) -> None: + """Test that logs land under the app_logs/ prefix on S3 by default.""" + mock_client = MagicMock() + + with _mock_settings: + log_service.info("app", "test", "Entry") + log_service.sync_logs_to_s3(mock_client, "test-bucket") + + calls = mock_client.upload_file.call_args_list + assert len(calls) >= 1 + for call in calls: + s3_key = call[0][2] + assert s3_key.startswith("app_logs/") + class TestHivePartitioning: """Tests for hive-partitioned directory structure.""" diff --git a/tests/test_routes.py b/tests/test_routes.py index da87d12..35f58b4 100644 --- a/tests/test_routes.py +++ b/tests/test_routes.py @@ -3,6 +3,7 @@ import json from unittest.mock import MagicMock, patch +from flask import Flask from flask.testing import FlaskClient @@ -281,6 +282,87 @@ def test_sync_success(self, mock_s3: MagicMock, client: FlaskClient) -> None: assert "synced" in data +class TestAutomaticLogging: + """Tests for the automatic request-logging hook and global error handler.""" + + def test_api_request_is_logged(self, client: FlaskClient) -> None: + """Every /api/* request is recorded as a 'request' log entry.""" + from app.services.log_service import get_log_service + + client.get("/api/settings/version") + + entries = get_log_service().read_log_entries(category="request", limit=50)["entries"] + assert any( + e["event"] == "http_request" and "/api/settings/version" in e["message"] + for e in entries + ) + + def test_log_endpoints_are_not_logged(self, client: FlaskClient) -> None: + """Requests to /api/logs/* are skipped to avoid feedback loops.""" + from app.services.log_service import get_log_service + + client.get("/api/logs/stats") + + entries = get_log_service().read_log_entries(category="request", limit=100)["entries"] + assert not any("/api/logs/stats" in e["message"] for e in entries) + + def test_uncaught_exception_is_logged_and_returns_500( + self, app: Flask, client: FlaskClient + ) -> None: + """An uncaught exception is logged at ERROR level and returns a 500 JSON body.""" + from app.services.log_service import get_log_service + + @app.route("/api/_test_boom") + def _boom() -> str: + raise ValueError("kaboom for testing") + + response = client.get("/api/_test_boom") + assert response.status_code == 500 + assert json.loads(response.data)["error"] == "Internal server error" + + entries = get_log_service().read_log_entries(level="ERROR", limit=50)["entries"] + crash = next((e for e in entries if e["event"] == "unhandled_exception"), None) + assert crash is not None + assert crash["metadata"]["exception_type"] == "ValueError" + assert "traceback" in crash["metadata"] + + def test_404_is_not_logged_as_crash(self, client: FlaskClient) -> None: + """HTTP errors keep default handling and are not logged as unhandled crashes.""" + from app.services.log_service import get_log_service + + response = client.get("/api/upload/does-not-exist") + assert response.status_code == 404 + + entries = get_log_service().read_log_entries(level="ERROR", limit=50)["entries"] + assert not any(e["event"] == "unhandled_exception" for e in entries) + + def test_client_log_endpoint_records_browser_error(self, client: FlaskClient) -> None: + """POST /api/logs/client stores a browser-side error in the 'client' category.""" + from app.services.log_service import get_log_service + + response = client.post( + "/api/logs/client", + data=json.dumps( + { + "level": "ERROR", + "event": "uncaught_error", + "message": "TypeError: x is undefined", + "metadata": {"url": "http://localhost/files", "stack": "at foo()"}, + } + ), + content_type="application/json", + ) + assert response.status_code == 200 + assert json.loads(response.data)["success"] is True + + entries = get_log_service().read_log_entries(category="client", limit=50)["entries"] + entry = next((e for e in entries if e["event"] == "uncaught_error"), None) + assert entry is not None + assert entry["message"] == "TypeError: x is undefined" + assert entry["metadata"]["url"] == "http://localhost/files" + assert "user_agent" in entry["metadata"] + + class TestProgressSSE: """Tests for Server-Sent Events progress endpoint."""