mirror of
https://github.com/jhd3197/ServerKit.git
synced 2026-10-28 15:47:50 +09:00
fix(api): a crash the route caught must still reach /monitoring/errors
Plan 76 milestone B, drift/bug row 4. 43 API handlers answered HTTP 500 from
their own `except Exception`, so the global 500 handler never ran — and that
handler was the ONLY caller of error_log_service.record_error(source='backend').
Every one of those crashes was absent from /monitoring/errors, and the page read
empty precisely because someone had "handled" them.
35 of the 43 also returned str(exc) to the caller. Two returned the full
traceback (templates.py, which also print()ed it), and several logged with a
locally-minted uuid4 'ref' that correlated with nothing but a log line.
The recording side of the 500 handler moves to app/error_reporting.py, so it is
importable rather than reachable only by not-catching:
record_unexpected(exc) -> logs, rolls the session back, records, returns
the request id
unexpected_response(exc) -> the above plus the documented crash body
The 500 handler now calls the same pair, so there is one recorder rather than
one recorder and 43 routes that quietly opted out of it.
Caller-visible change on those endpoints: the body is now the documented
{'error': 'Internal server error', 'status': 500, 'code': 'internal_error',
'request_id': ...} instead of the exception text. That is the point — an
unexpected exception's text is written for whoever reads the log and routinely
carries connection strings, absolute paths, and SQL. The request id is the part
a caller can act on, and it matches the row in the error log. No frontend code
read the 'ref' or 'trace' keys this drops (checked).
Two sites keep their own wording and call record_unexpected instead: sso.py's
login failures should not read as a generic crash on the login page.
docker.py's bulk endpoint keeps its per-item `results` — the caller's half-done
work survives, only the exception text is dropped.
Ratchet: tests/unreported_crash_census.py + its test, held at **0**, not a
legacy baseline to shrink. It counts blanket handlers that answer 500 without
calling either reporter, and is scoped to app/api — a service catching an
exception and returning a value is making a domain decision, not declaring an
HTTP crash. Verified non-vacuous: an injected handler fails it, and it passes
again on revert.
Measured and deliberately still open: 56 API handlers swallow a crash and
answer 200. A failure reported as success is worse, but converging it is
milestone C's response-envelope work, not this row.
Gate: tests/test_unexpected_error_reporting.py (11, including the two that pin
why the old shape lost the crash), test_unreported_crash_ratchet.py (9), and
the 66 test files covering every touched module: 1105 passed, 7 skipped, 1
pre-existing Windows-only failure (test_fleet_doctor's socket.gaierror).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
1 parent
39443c1e35
commit
a7d5a0b919
21 files changed
+687
-208
No files matched your search
+8
-51
@@ -474,59 +474,16 @@ def create_app(config_name=None):
|
||||
def internal_error(e):
|
||||
original = getattr(e, 'original_exception', None)
|
||||
exc = original or e
|
||||
request_id = get_request_id(create=True)
|
||||
app.logger.exception(
|
||||
'Unhandled exception on %s %s [request_id=%s]',
|
||||
request.method, request.path, request_id,
|
||||
exc_info=exc
|
||||
)
|
||||
# Roll the failing request's session back BEFORE recording. The 500
|
||||
# handler shares the app-context-scoped db.session with the view that
|
||||
# just crashed, so without this a crash caused by a DB error leaves the
|
||||
# session in a failed transaction and record_error's very first query
|
||||
# raises PendingRollbackError — swallowed by its own contract, which
|
||||
# means database-caused 500s never reach the error log at all. It also
|
||||
# stops record_error's commit() from flushing any ORM work the crashed
|
||||
# view had left pending. Flask-SQLAlchemy's teardown rolls this session
|
||||
# back moments later regardless, so a clean request is unaffected.
|
||||
try:
|
||||
db.session.rollback()
|
||||
except Exception: # noqa: BLE001 - must never change the 500 response
|
||||
pass
|
||||
# Record into the centralized error log. error_log_service never raises
|
||||
# by contract, but belt-and-braces: a recording failure must never
|
||||
# change this response.
|
||||
try:
|
||||
import traceback as _traceback
|
||||
from flask_jwt_extended import get_jwt_identity
|
||||
from app.services import error_log_service
|
||||
user_id = None
|
||||
try:
|
||||
user_id = get_jwt_identity()
|
||||
except Exception: # noqa: BLE001 - anonymous crashes still count
|
||||
pass
|
||||
error_log_service.record_error(
|
||||
source='backend',
|
||||
exception_type=type(exc).__name__,
|
||||
message=str(exc),
|
||||
traceback=''.join(
|
||||
_traceback.format_exception(type(exc), exc, exc.__traceback__)
|
||||
),
|
||||
endpoint=request.path,
|
||||
method=request.method,
|
||||
user_id=user_id,
|
||||
context={'request_id': request_id},
|
||||
)
|
||||
except Exception: # noqa: BLE001
|
||||
pass
|
||||
# Logging, session rollback, and the error-log write live in
|
||||
# app/error_reporting.py so a route that catches Exception itself can
|
||||
# still put its crash on the record. Before that split this was the
|
||||
# only path to /monitoring/errors, so every locally-handled 500 was
|
||||
# invisible there.
|
||||
from app.error_reporting import record_unexpected, unexpected_error_body
|
||||
|
||||
# JSON either way: unlike a 404, there is no sensible SPA fallback for a
|
||||
# crash, and an HTML page tells the caller nothing it can act on.
|
||||
return {
|
||||
'error': 'Internal server error',
|
||||
'status': 500,
|
||||
'code': 'internal_error',
|
||||
'request_id': request_id,
|
||||
}, 500
|
||||
return unexpected_error_body(record_unexpected(exc)), 500
|
||||
|
||||
# Framework-generated HTTP errors (abort(), 405 from the router, 413 from
|
||||
# MAX_CONTENT_LENGTH) rendered Werkzeug's HTML page even under /api/, so an
|
||||
|
||||
@@ -35,6 +35,7 @@ from app.middleware.rbac import admin_required, get_current_user
|
||||
from app.models.ai import AiConversation, AiMessage, AiPendingAction
|
||||
from app.services import ai_service
|
||||
from app.services.ai_tool_registry import ai_tool_registry
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
ai_bp = Blueprint('ai', __name__)
|
||||
@@ -296,9 +297,8 @@ def chat():
|
||||
# gate=None: write tools refuse (no interactive confirmation in this mode).
|
||||
conv = ai_service.build_conversation(row, user, mode, page_context, gate=None)
|
||||
reply = conv.ask(safe_message)
|
||||
except Exception as exc:
|
||||
logger.exception("AI /chat failed")
|
||||
return jsonify({'error': f'AI request failed: {exc}'}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
_persist_assistant_message(row, reply, tool_calls=[], usage=conv.usage)
|
||||
ai_service.persist_conversation(row, conv, page_context=page_context)
|
||||
|
||||
@@ -6,6 +6,7 @@ Mounted at /api/v1/bandwidth (registered in app/__init__.py).
|
||||
"""
|
||||
from flask import Blueprint, jsonify, request
|
||||
from app.middleware.rbac import get_current_user
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
from ..middleware.rbac import admin_required, viewer_required
|
||||
from ..models import User, Application
|
||||
@@ -24,8 +25,8 @@ def get_apps_bandwidth():
|
||||
data = BandwidthService.overview(days=30)
|
||||
# JSON object keys must be strings.
|
||||
return jsonify({'apps': {str(k): v for k, v in data.items()}})
|
||||
except Exception as exc: # noqa: BLE001 — surface as a clean JSON error
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@bandwidth_bp.route('/apps/<int:app_id>', methods=['GET'])
|
||||
@@ -46,8 +47,8 @@ def get_app_bandwidth(app_id):
|
||||
'series': series,
|
||||
'month_bytes': BandwidthService.monthly_total(app_id),
|
||||
})
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@bandwidth_bp.route('/aggregate', methods=['POST'])
|
||||
@@ -60,5 +61,5 @@ def run_aggregate():
|
||||
return jsonify(result)
|
||||
except ValueError:
|
||||
return jsonify({'error': 'day must be YYYY-MM-DD'}), 400
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
@@ -4,6 +4,7 @@ from app.models import User, Application
|
||||
from app.services.docker_service import DockerService
|
||||
from app.middleware.rbac import admin_required
|
||||
from app import db, paths
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
docker_bp = Blueprint('docker', __name__)
|
||||
|
||||
@@ -605,12 +606,12 @@ def cleanup_all_apps():
|
||||
# Commit database changes
|
||||
try:
|
||||
db.session.commit()
|
||||
except Exception as e:
|
||||
db.session.rollback()
|
||||
return jsonify({
|
||||
'error': f'Database commit failed: {str(e)}',
|
||||
'results': results
|
||||
}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
# The per-item results are the caller's half-done work and must
|
||||
# survive; only the exception text is dropped.
|
||||
body, status = unexpected_response(exc)
|
||||
body['results'] = results
|
||||
return body, status
|
||||
|
||||
return jsonify({
|
||||
'success': True,
|
||||
|
||||
+13
-12
@@ -13,6 +13,7 @@ Contract (the CLI codes against this too):
|
||||
{'items': [{kind, type?, id?, name?}]}
|
||||
"""
|
||||
from flask import Blueprint, jsonify, request
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
from ..middleware.rbac import admin_required
|
||||
from ..services.doctor_service import DoctorService
|
||||
@@ -31,8 +32,8 @@ def get_drift_report():
|
||||
"""Last stored drift report (null when no sweep has run yet)."""
|
||||
try:
|
||||
return jsonify({'report': DriftService.get_last_report()})
|
||||
except Exception as exc: # noqa: BLE001 — surface as a clean JSON error
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@doctor_bp.route('/drift/check', methods=['POST'])
|
||||
@@ -43,8 +44,8 @@ def run_drift_check():
|
||||
from app.jobs.service import JobService
|
||||
job = JobService.enqueue(DRIFT_JOB_KIND, payload={}, max_attempts=1)
|
||||
return jsonify({'job_id': job.id}), 202
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@doctor_bp.route('/drift/<check_type>/<resource_id>/repair', methods=['POST'])
|
||||
@@ -59,8 +60,8 @@ def repair_drift(check_type, resource_id):
|
||||
result = DriftService.repair(check_type, rid)
|
||||
status = 200 if result.get('success') else 400
|
||||
return jsonify(result), status
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
@@ -74,8 +75,8 @@ def get_doctor_report():
|
||||
"""Last stored doctor report (null when the doctor has never run)."""
|
||||
try:
|
||||
return jsonify({'report': DoctorService.get_last_report()})
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@doctor_bp.route('/run', methods=['POST'])
|
||||
@@ -86,8 +87,8 @@ def run_doctor():
|
||||
try:
|
||||
report = DoctorService.run()
|
||||
return jsonify({'report': report})
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@doctor_bp.route('/repair', methods=['POST'])
|
||||
@@ -100,5 +101,5 @@ def repair_items():
|
||||
return jsonify({'error': "Body must carry a non-empty 'items' list."}), 400
|
||||
try:
|
||||
return jsonify({'results': DoctorService.repair(items)})
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
@@ -1,6 +1,7 @@
|
||||
"""File Manager API endpoints for browsing, editing, and managing files."""
|
||||
|
||||
from flask import Blueprint, request, jsonify, send_file
|
||||
from app.error_reporting import unexpected_response
|
||||
from ..middleware.rbac import permission_required
|
||||
from ..services.file_service import FileService
|
||||
from ..services.storage_provider_service import StorageProviderService
|
||||
@@ -353,8 +354,8 @@ def download_file():
|
||||
as_attachment=True,
|
||||
download_name=os.path.basename(path)
|
||||
)
|
||||
except Exception as e:
|
||||
return jsonify({'error': str(e)}), 500
|
||||
except Exception as e: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(e)
|
||||
|
||||
|
||||
@files_bp.route('/upload', methods=['POST'])
|
||||
@@ -414,8 +415,8 @@ def upload_file():
|
||||
|
||||
except PermissionError:
|
||||
return jsonify({'error': 'Permission denied'}), 403
|
||||
except Exception as e:
|
||||
return jsonify({'error': str(e)}), 500
|
||||
except Exception as e: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(e)
|
||||
|
||||
|
||||
# ── S3 / object-storage browser (reuses the configured backup storage creds) ──
|
||||
|
||||
@@ -20,6 +20,7 @@ layer only authenticates, shapes the batch, and reports refusals as results
|
||||
rather than errors.
|
||||
"""
|
||||
from flask import Blueprint, jsonify, request
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
from ..middleware.rbac import admin_required, get_current_user
|
||||
from ..services.fleet_doctor_service import FLEET_DOCTOR_JOB_KIND, FleetDoctorService
|
||||
@@ -44,8 +45,8 @@ def get_fleet_report():
|
||||
"""
|
||||
try:
|
||||
return jsonify({'report': FleetDoctorService.fleet_report()})
|
||||
except Exception as exc: # noqa: BLE001 — surface as a clean JSON error
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@fleet_doctor_bp.route('/run', methods=['POST'])
|
||||
@@ -56,8 +57,8 @@ def run_fleet_sweep():
|
||||
from app.jobs.service import JobService
|
||||
job = JobService.enqueue(FLEET_DOCTOR_JOB_KIND, payload={}, max_attempts=1)
|
||||
return jsonify({'job_id': job.id}), 202
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@fleet_doctor_bp.route('/repair', methods=['POST'])
|
||||
@@ -88,5 +89,5 @@ def repair_fleet_items():
|
||||
continue
|
||||
results.append(FleetRepairService.repair(item, user_id=user_id))
|
||||
return jsonify({'results': results})
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
@@ -11,6 +11,7 @@ from flask_jwt_extended import jwt_required
|
||||
from app.services.audit_service import AuditService
|
||||
from app.models.audit_log import AuditLog
|
||||
from app.middleware.rbac import get_current_user
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
marketplace_bp = Blueprint('marketplace', __name__)
|
||||
|
||||
@@ -92,8 +93,5 @@ def install_registry(slug):
|
||||
return jsonify(plugin.to_dict()), 201
|
||||
except ValueError as e:
|
||||
return jsonify({'error': str(e)}), 400
|
||||
except Exception:
|
||||
import logging, uuid
|
||||
ref = uuid.uuid4().hex[:8]
|
||||
logging.getLogger(__name__).exception('Registry install failed (ref=%s)', ref)
|
||||
return jsonify({'error': 'Installation failed. Check server logs.', 'ref': ref}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
@@ -12,6 +12,7 @@ from app.services.settings_service import SettingsService
|
||||
from app.services.bounce_service import BounceService
|
||||
from app.notifications.service import NotificationBusService
|
||||
from app.notifications.providers import EmailProviderService, SUPPORTED as EMAIL_PROVIDERS
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
# Header carrying the provider's HMAC-SHA256 of the raw request body, formatted
|
||||
# as ``sha256=<hexdigest>`` (GitHub-style). The shared secret is
|
||||
@@ -227,9 +228,8 @@ def update_user_preferences():
|
||||
try:
|
||||
db.session.commit()
|
||||
return jsonify({'success': True, 'preferences': prefs.to_dict()}), 200
|
||||
except Exception as e:
|
||||
db.session.rollback()
|
||||
return jsonify({'error': str(e)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc) # rolls the session back itself
|
||||
|
||||
|
||||
@notifications_bp.route('/preferences/test', methods=['POST'])
|
||||
|
||||
+33
-69
@@ -11,6 +11,7 @@ from flask_jwt_extended import jwt_required
|
||||
from app.services.audit_service import AuditService
|
||||
from app.models.audit_log import AuditLog
|
||||
from app.middleware.rbac import get_current_user
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
plugins_bp = Blueprint('plugins', __name__)
|
||||
|
||||
@@ -93,20 +94,13 @@ def install_plugin():
|
||||
return jsonify(plugin.to_dict()), 201
|
||||
except ValueError as e:
|
||||
return jsonify({'error': str(e)}), 400
|
||||
except Exception:
|
||||
# Log full traceback server-side; details go to the InstalledPlugin
|
||||
# row's error_message field. Don't echo raw exceptions to the
|
||||
# client — they can leak filesystem paths, library versions, or
|
||||
# SQL fragments.
|
||||
import logging, uuid
|
||||
ref = uuid.uuid4().hex[:8]
|
||||
logging.getLogger(__name__).exception(
|
||||
'Plugin install failed (ref=%s)', ref
|
||||
)
|
||||
return jsonify({
|
||||
'error': 'Installation failed. Check server logs.',
|
||||
'ref': ref,
|
||||
}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
# Not echoing the raw exception is right — it leaks filesystem
|
||||
# paths, library versions, and SQL fragments. The door does that and
|
||||
# also puts the crash in the error log behind /monitoring/errors,
|
||||
# under the request id the caller is handed (the local uuid4 'ref'
|
||||
# correlated with nothing but a log line).
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@plugins_bp.route('/preview', methods=['POST'])
|
||||
@@ -137,14 +131,8 @@ def preview_plugin():
|
||||
# GitHubResolveError is a ValueError subclass — its message is already
|
||||
# user-actionable (rate limit / 404 / private / no plugin.json).
|
||||
return jsonify({'error': str(e)}), 400
|
||||
except Exception:
|
||||
import logging, uuid
|
||||
ref = uuid.uuid4().hex[:8]
|
||||
logging.getLogger(__name__).exception('Plugin preview failed (ref=%s)', ref)
|
||||
return jsonify({
|
||||
'error': 'Preview failed. Check server logs.',
|
||||
'ref': ref,
|
||||
}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@plugins_bp.route('/install-local', methods=['POST'])
|
||||
@@ -182,20 +170,13 @@ def install_plugin_local():
|
||||
return jsonify(plugin.to_dict()), 201
|
||||
except ValueError as e:
|
||||
return jsonify({'error': str(e)}), 400
|
||||
except Exception:
|
||||
# Log full traceback server-side; details go to the InstalledPlugin
|
||||
# row's error_message field. Don't echo raw exceptions to the
|
||||
# client — they can leak filesystem paths, library versions, or
|
||||
# SQL fragments.
|
||||
import logging, uuid
|
||||
ref = uuid.uuid4().hex[:8]
|
||||
logging.getLogger(__name__).exception(
|
||||
'Plugin install failed (ref=%s)', ref
|
||||
)
|
||||
return jsonify({
|
||||
'error': 'Installation failed. Check server logs.',
|
||||
'ref': ref,
|
||||
}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
# Not echoing the raw exception is right — it leaks filesystem
|
||||
# paths, library versions, and SQL fragments. The door does that and
|
||||
# also puts the crash in the error log behind /monitoring/errors,
|
||||
# under the request id the caller is handed (the local uuid4 'ref'
|
||||
# correlated with nothing but a log line).
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@plugins_bp.route('/install-upload', methods=['POST'])
|
||||
@@ -240,20 +221,13 @@ def install_plugin_upload():
|
||||
return jsonify(plugin.to_dict()), 201
|
||||
except ValueError as e:
|
||||
return jsonify({'error': str(e)}), 400
|
||||
except Exception:
|
||||
# Log full traceback server-side; details go to the InstalledPlugin
|
||||
# row's error_message field. Don't echo raw exceptions to the
|
||||
# client — they can leak filesystem paths, library versions, or
|
||||
# SQL fragments.
|
||||
import logging, uuid
|
||||
ref = uuid.uuid4().hex[:8]
|
||||
logging.getLogger(__name__).exception(
|
||||
'Plugin install failed (ref=%s)', ref
|
||||
)
|
||||
return jsonify({
|
||||
'error': 'Installation failed. Check server logs.',
|
||||
'ref': ref,
|
||||
}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
# Not echoing the raw exception is right — it leaks filesystem
|
||||
# paths, library versions, and SQL fragments. The door does that and
|
||||
# also puts the crash in the error log behind /monitoring/errors,
|
||||
# under the request id the caller is handed (the local uuid4 'ref'
|
||||
# correlated with nothing but a log line).
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@plugins_bp.route('/<int:plugin_id>', methods=['DELETE'])
|
||||
@@ -385,20 +359,13 @@ def install_builtin(slug):
|
||||
return jsonify(plugin.to_dict()), 201
|
||||
except ValueError as e:
|
||||
return jsonify({'error': str(e)}), 400
|
||||
except Exception:
|
||||
# Log full traceback server-side; details go to the InstalledPlugin
|
||||
# row's error_message field. Don't echo raw exceptions to the
|
||||
# client — they can leak filesystem paths, library versions, or
|
||||
# SQL fragments.
|
||||
import logging, uuid
|
||||
ref = uuid.uuid4().hex[:8]
|
||||
logging.getLogger(__name__).exception(
|
||||
'Plugin install failed (ref=%s)', ref
|
||||
)
|
||||
return jsonify({
|
||||
'error': 'Installation failed. Check server logs.',
|
||||
'ref': ref,
|
||||
}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
# Not echoing the raw exception is right — it leaks filesystem
|
||||
# paths, library versions, and SQL fragments. The door does that and
|
||||
# also puts the crash in the error log behind /monitoring/errors,
|
||||
# under the request id the caller is handed (the local uuid4 'ref'
|
||||
# correlated with nothing but a log line).
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@plugins_bp.route('/<slug>/assets/<path:asset_path>', methods=['GET'])
|
||||
@@ -727,11 +694,8 @@ def update_plugin_route(plugin_id):
|
||||
return jsonify(plugin.to_dict())
|
||||
except ValueError as e:
|
||||
return jsonify({'error': str(e)}), 400
|
||||
except Exception:
|
||||
import logging, uuid
|
||||
ref = uuid.uuid4().hex[:8]
|
||||
logging.getLogger(__name__).exception('Plugin update failed (ref=%s)', ref)
|
||||
return jsonify({'error': 'Update failed. Check server logs.', 'ref': ref}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@plugins_bp.route('/contributions', methods=['GET'])
|
||||
|
||||
@@ -7,6 +7,7 @@ from app.services.security_service import SecurityService
|
||||
from app.services.image_scanner_service import ImageScannerService
|
||||
from app.services.malware_scan_service import MalwareScanService
|
||||
from app.services.yara_scan_service import YaraScanService
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
security_bp = Blueprint('security', __name__)
|
||||
|
||||
@@ -638,8 +639,8 @@ def fim_baseline(scope):
|
||||
result = FileIntegrityService.baseline(scope, options=options)
|
||||
except FileIntegrityScopeError as exc:
|
||||
return jsonify({'error': str(exc)}), 400
|
||||
except Exception as exc:
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
return jsonify(result), 200
|
||||
|
||||
|
||||
@@ -654,8 +655,8 @@ def fim_check(scope):
|
||||
result = FileIntegrityService.check(scope)
|
||||
except FileIntegrityScopeError as exc:
|
||||
return jsonify({'error': str(exc)}), 400
|
||||
except Exception as exc:
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
return jsonify(result), 200
|
||||
|
||||
|
||||
@@ -670,8 +671,8 @@ def fim_accept(scope):
|
||||
result = FileIntegrityService.accept(scope)
|
||||
except FileIntegrityScopeError as exc:
|
||||
return jsonify({'error': str(exc)}), 400
|
||||
except Exception as exc:
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
return jsonify(result), 200
|
||||
|
||||
|
||||
|
||||
@@ -13,6 +13,7 @@ ref}``). The doctor report carries the same items as its ``setup.*`` section, so
|
||||
this endpoint and Monitoring → Doctor stay in lockstep.
|
||||
"""
|
||||
from flask import Blueprint, jsonify, request
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
from ..middleware.rbac import admin_required, auth_required, get_current_user
|
||||
from ..services.setup_health_service import SetupHealthService
|
||||
@@ -33,8 +34,8 @@ def get_setup_health():
|
||||
summary. Cheap — DB/settings probes only, no host calls."""
|
||||
try:
|
||||
return jsonify(SetupHealthService.evaluate(scope='panel'))
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@setup_health_bp.route('/account', methods=['GET'])
|
||||
@@ -47,8 +48,8 @@ def get_account_security():
|
||||
if not user:
|
||||
return jsonify({'error': 'User not found'}), 404
|
||||
return jsonify(SetupHealthService.evaluate(scope='user', user=user))
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
@@ -83,8 +84,8 @@ def snooze_item():
|
||||
return err
|
||||
try:
|
||||
return jsonify(SetupHealthService.snooze(key, days=data.get('days', 30), user=user))
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@setup_health_bp.route('/snooze', methods=['DELETE'])
|
||||
@@ -100,8 +101,8 @@ def unsnooze_item():
|
||||
return err
|
||||
try:
|
||||
return jsonify(SetupHealthService.unsnooze(key, user=user))
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
# --------------------------------------------------------------------------- #
|
||||
@@ -115,8 +116,8 @@ def dns_backfill_preview():
|
||||
"""Dry-run: the managed hosts a DNS backfill would ensure records for."""
|
||||
try:
|
||||
return jsonify(SetupReconcileService.dns_backfill_preview())
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@setup_health_bp.route('/reconcile/dns/apply', methods=['POST'])
|
||||
@@ -127,8 +128,8 @@ def dns_backfill_apply():
|
||||
from app.jobs.service import JobService
|
||||
job = JobService.enqueue(DNS_BACKFILL_JOB_KIND, payload={}, max_attempts=1)
|
||||
return jsonify({'job_id': job.id}), 202
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@setup_health_bp.route('/reconcile/url-fix/preview', methods=['POST'])
|
||||
@@ -138,8 +139,8 @@ def url_fix_preview():
|
||||
stale IP, with the per-site swap they'd get."""
|
||||
try:
|
||||
return jsonify(SetupReconcileService.url_fix_preview())
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@setup_health_bp.route('/reconcile/url-fix/apply', methods=['POST'])
|
||||
@@ -150,5 +151,5 @@ def url_fix_apply():
|
||||
from app.jobs.service import JobService
|
||||
job = JobService.enqueue(URL_FIX_JOB_KIND, payload={}, max_attempts=1)
|
||||
return jsonify({'job_id': job.id}), 202
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
@@ -3,6 +3,7 @@
|
||||
Mounted at /api/v1/speedtest (registered in app/__init__.py).
|
||||
"""
|
||||
from flask import Blueprint, jsonify
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
from ..middleware.rbac import admin_required, viewer_required
|
||||
from ..services.speed_test_service import SPEEDTEST_JOB_KIND, SpeedTestService
|
||||
@@ -17,8 +18,8 @@ def get_speed_test():
|
||||
"""Last stored result + status of any in-flight speed test job."""
|
||||
try:
|
||||
return jsonify(SpeedTestService.get_status())
|
||||
except Exception as exc: # noqa: BLE001 — surface as a clean JSON error
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@speedtest_bp.route('/run', methods=['POST'])
|
||||
@@ -31,5 +32,5 @@ def run_speed_test():
|
||||
from app.jobs.service import JobService
|
||||
job = JobService.enqueue(SPEEDTEST_JOB_KIND, payload={}, max_attempts=1)
|
||||
return jsonify({'job_id': job.id}), 202
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return jsonify({'error': str(exc)}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
@@ -11,6 +11,7 @@ from app.services import sso_service
|
||||
from app.services.settings_service import SettingsService
|
||||
from app.services.audit_service import AuditService
|
||||
from app.middleware.rbac import admin_required, get_current_user
|
||||
from app.error_reporting import record_unexpected, unexpected_response
|
||||
|
||||
sso_bp = Blueprint('sso', __name__)
|
||||
|
||||
@@ -70,11 +71,12 @@ def callback(provider):
|
||||
details={'provider': provider, 'error': str(e)},
|
||||
)
|
||||
return jsonify({'error': str(e)}), 403
|
||||
except Exception as e:
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
AuditService.log(
|
||||
action=AuditLog.ACTION_SSO_LOGIN_FAILED,
|
||||
details={'provider': provider, 'error': str(e)},
|
||||
details={'provider': provider, 'error': str(exc)},
|
||||
)
|
||||
record_unexpected(exc)
|
||||
return jsonify({'error': 'SSO authentication failed'}), 500
|
||||
|
||||
return _complete_sso_login(user, provider, is_new)
|
||||
@@ -95,7 +97,8 @@ def saml_callback():
|
||||
user, is_new = sso_service.find_or_create_user('saml', profile)
|
||||
except ValueError as e:
|
||||
return jsonify({'error': str(e)}), 403
|
||||
except Exception as e:
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
record_unexpected(exc)
|
||||
return jsonify({'error': 'SAML authentication failed'}), 500
|
||||
|
||||
return _complete_sso_login(user, 'saml', is_new)
|
||||
@@ -124,8 +127,8 @@ def saml_metadata():
|
||||
metadata = auth.get_settings().get_sp_metadata()
|
||||
from flask import Response
|
||||
return Response(metadata, mimetype='application/xml')
|
||||
except Exception as e:
|
||||
return jsonify({'error': str(e)}), 500
|
||||
except Exception as e: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(e)
|
||||
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@@ -11,6 +11,7 @@ from flask import Blueprint, jsonify, send_file
|
||||
|
||||
from app.middleware.rbac import admin_required
|
||||
from app.services import support_bundle_service
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
support_bundle_bp = Blueprint('support_bundle', __name__)
|
||||
|
||||
@@ -21,8 +22,8 @@ def build_support_bundle():
|
||||
"""Build a diagnostic support bundle and return it as a zip download."""
|
||||
try:
|
||||
path = support_bundle_service.build()
|
||||
except Exception as exc: # noqa: BLE001 - surface as a JSON error, not a 500 page
|
||||
return jsonify({'error': f'Failed to build support bundle: {exc}'}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
if not os.path.isfile(path):
|
||||
return jsonify({'error': 'Support bundle was not created'}), 500
|
||||
|
||||
@@ -19,6 +19,7 @@ from app.services.template_service import TemplateService
|
||||
from app.services.repository_manifest_service import RepositoryManifestService
|
||||
from app.services.resource_grant_service import ResourceGrantService
|
||||
from app.utils.slug import validate_app_name
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
templates_bp = Blueprint('templates', __name__)
|
||||
|
||||
@@ -348,11 +349,10 @@ def install_template(template_id):
|
||||
status = result.get('job', {}).get('status')
|
||||
return jsonify(result), 201 if status == 'succeeded' else 202
|
||||
|
||||
except Exception as e:
|
||||
import traceback
|
||||
error_trace = traceback.format_exc()
|
||||
print(f"Template install error: {error_trace}")
|
||||
return jsonify({'error': str(e), 'trace': error_trace}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
# Was returning the full traceback to the caller and print()ing it.
|
||||
# The door logs it and files it under a request id instead.
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@templates_bp.route('/<template_id>/capacity', methods=['GET'])
|
||||
@@ -446,11 +446,8 @@ def validate_installation():
|
||||
|
||||
return jsonify({'valid': True, 'capacity': capacity}), 200
|
||||
|
||||
except Exception as e:
|
||||
import traceback
|
||||
error_trace = traceback.format_exc()
|
||||
print(f"Validate install error: {error_trace}")
|
||||
return jsonify({'error': str(e), 'trace': error_trace}), 500
|
||||
except Exception as exc: # noqa: BLE001 - reported, not swallowed
|
||||
return unexpected_response(exc)
|
||||
|
||||
|
||||
@templates_bp.route('/test-db-connection', methods=['POST'])
|
||||
|
||||
@@ -0,0 +1,111 @@
|
||||
"""One place that turns an *unexpected* exception into a recorded, correlated 500.
|
||||
|
||||
The global ``@app.errorhandler(500)`` logs the crash, records it into the
|
||||
centralized error log behind ``/monitoring/errors``, and answers with the
|
||||
documented JSON body carrying a request id. A route that catches ``Exception``
|
||||
itself never reaches that handler, so its crash is logged nowhere and is
|
||||
invisible on the errors page — the failure looks handled precisely because
|
||||
someone handled it.
|
||||
|
||||
That is why this module exists rather than only the handler: the recording side
|
||||
is importable, so a route that must keep answering JSON in-band can still put
|
||||
its crash on the record.
|
||||
|
||||
Use it only for genuinely unexpected failures. An expected, caller-actionable
|
||||
failure should raise one of ``app.exceptions``' typed errors instead — those
|
||||
are not crashes and must not fill the error log.
|
||||
|
||||
from app.error_reporting import unexpected_response
|
||||
|
||||
try:
|
||||
return jsonify(SomeService.run())
|
||||
except Exception as exc: # noqa: BLE001
|
||||
return unexpected_response(exc)
|
||||
|
||||
Prefer deleting the ``try``/``except`` entirely where nothing in it does real
|
||||
cleanup: letting the exception reach the global handler produces the same
|
||||
response through the same recorder, with one less thing to keep in step.
|
||||
"""
|
||||
|
||||
import traceback as _traceback
|
||||
|
||||
from flask import current_app, request
|
||||
|
||||
from app.middleware.request_id import get_request_id
|
||||
|
||||
|
||||
def record_unexpected(exc, *, request_id=None):
|
||||
"""Log `exc` and record it into the centralized error log. Never raises.
|
||||
|
||||
Returns the request id the crash was recorded under so a caller can hand it
|
||||
to the client — an error the user can quote is worth far more than the
|
||||
exception text this replaces.
|
||||
"""
|
||||
request_id = request_id or get_request_id(create=True)
|
||||
|
||||
try:
|
||||
current_app.logger.exception(
|
||||
'Unhandled exception on %s %s [request_id=%s]',
|
||||
request.method, request.path, request_id, exc_info=exc,
|
||||
)
|
||||
except Exception: # noqa: BLE001 - reporting must never mask the crash
|
||||
pass
|
||||
|
||||
# Roll back BEFORE recording. This shares the app-context-scoped session
|
||||
# with the code that just crashed, so a DB-caused failure leaves it in a
|
||||
# failed transaction and record_error's first query would raise
|
||||
# PendingRollbackError — swallowed by its own contract, which is how
|
||||
# database-caused crashes silently never reach the error log.
|
||||
try:
|
||||
from app import db
|
||||
db.session.rollback()
|
||||
except Exception: # noqa: BLE001
|
||||
pass
|
||||
|
||||
try:
|
||||
from app.services import error_log_service
|
||||
from app.middleware.rbac import get_current_user
|
||||
|
||||
user = None
|
||||
try:
|
||||
user = get_current_user()
|
||||
except Exception: # noqa: BLE001 - anonymous crashes still count
|
||||
pass
|
||||
|
||||
error_log_service.record_error(
|
||||
source='backend',
|
||||
exception_type=type(exc).__name__,
|
||||
message=str(exc),
|
||||
traceback=''.join(
|
||||
_traceback.format_exception(type(exc), exc, exc.__traceback__)
|
||||
),
|
||||
endpoint=getattr(request, 'path', None),
|
||||
method=getattr(request, 'method', None),
|
||||
user_id=user.id if user else None,
|
||||
context={'request_id': request_id},
|
||||
)
|
||||
except Exception: # noqa: BLE001
|
||||
pass
|
||||
|
||||
return request_id
|
||||
|
||||
|
||||
def unexpected_error_body(request_id):
|
||||
"""The documented crash body — identical to the global 500 handler's."""
|
||||
return {
|
||||
'error': 'Internal server error',
|
||||
'status': 500,
|
||||
'code': 'internal_error',
|
||||
'request_id': request_id,
|
||||
}
|
||||
|
||||
|
||||
def unexpected_response(exc):
|
||||
"""Record `exc` and return the standard ``(body, 500)`` for a crash.
|
||||
|
||||
The body deliberately does NOT carry ``str(exc)``. An unexpected exception's
|
||||
text is written for whoever reads the log, not for the caller: it routinely
|
||||
contains connection strings, absolute paths, and SQL. The request id is the
|
||||
part the caller can act on.
|
||||
"""
|
||||
return unexpected_error_body(record_unexpected(exc)), 500
|
||||
@@ -0,0 +1 @@
|
||||
0
|
||||
@@ -0,0 +1,164 @@
|
||||
"""A crash a route handled itself must still reach /monitoring/errors (plan 76, B).
|
||||
|
||||
The global 500 handler is what logs a crash and writes it to the centralized
|
||||
error log. A route that catches ``Exception`` itself answers before that
|
||||
handler runs, so the crash is recorded nowhere — and the errors page reads
|
||||
empty precisely because someone "handled" it. 43 API handlers did this while
|
||||
also returning ``str(exc)`` to the caller.
|
||||
|
||||
These tests pin both halves of the contract: the crash is recorded, and the
|
||||
caller gets a correlatable request id instead of the exception text.
|
||||
|
||||
No blueprint is registered here on purpose. The ``app`` fixture is per-test but
|
||||
the application object underneath it is built once per session, so a route
|
||||
added in a test stays mounted for every later one. The door is a plain
|
||||
function; a request context is all it needs.
|
||||
"""
|
||||
|
||||
import pytest
|
||||
from flask import jsonify
|
||||
|
||||
from app.error_reporting import (
|
||||
record_unexpected,
|
||||
unexpected_error_body,
|
||||
unexpected_response,
|
||||
)
|
||||
|
||||
|
||||
def _boom(message='secret-connstring-in-here'):
|
||||
"""A raised-and-caught exception, so it carries a real traceback."""
|
||||
try:
|
||||
raise RuntimeError(message)
|
||||
except RuntimeError as exc:
|
||||
return exc
|
||||
|
||||
|
||||
def _errors_for(endpoint):
|
||||
from app.models.error_log import ErrorLog
|
||||
return ErrorLog.query.filter_by(endpoint=endpoint).all()
|
||||
|
||||
|
||||
class TestTheOldShapeLosesTheCrash:
|
||||
"""Why this milestone exists — asserted, not assumed."""
|
||||
|
||||
def test_catching_the_exception_yourself_records_nothing(self, app):
|
||||
with app.test_request_context('/__probe/handled-badly'):
|
||||
try:
|
||||
raise RuntimeError('secret-connstring-in-here')
|
||||
except Exception as exc: # noqa: BLE001 - the shape being replaced
|
||||
body, status = jsonify({'error': str(exc)}), 500
|
||||
|
||||
assert status == 500
|
||||
assert body.get_json()['error'] == 'secret-connstring-in-here'
|
||||
assert _errors_for('/__probe/handled-badly') == [], (
|
||||
'the locally-handled crash was expected to bypass the error log; '
|
||||
'if it now records, the bypass is closed and this test should '
|
||||
'become the positive assertion'
|
||||
)
|
||||
|
||||
|
||||
class TestTheDoorRecordsAndRedacts:
|
||||
|
||||
def test_the_crash_reaches_the_error_log(self, app):
|
||||
with app.test_request_context('/__probe/recorded'):
|
||||
unexpected_response(_boom())
|
||||
|
||||
rows = _errors_for('/__probe/recorded')
|
||||
assert len(rows) == 1, 'the swallowed crash did not reach the error log'
|
||||
assert rows[0].exception_type == 'RuntimeError'
|
||||
assert 'secret-connstring-in-here' in rows[0].message
|
||||
|
||||
def test_the_traceback_is_kept_for_whoever_reads_the_log(self, app):
|
||||
with app.test_request_context('/__probe/traceback'):
|
||||
unexpected_response(_boom())
|
||||
|
||||
row = _errors_for('/__probe/traceback')[0]
|
||||
assert 'RuntimeError' in (row.traceback or '')
|
||||
|
||||
def test_the_caller_gets_a_request_id_not_the_exception_text(self, app):
|
||||
with app.test_request_context('/__probe/redacted'):
|
||||
body, status = unexpected_response(_boom())
|
||||
|
||||
assert status == 500
|
||||
assert body['error'] == 'Internal server error'
|
||||
assert body['code'] == 'internal_error'
|
||||
assert body['request_id']
|
||||
assert 'secret-connstring' not in str(body)
|
||||
|
||||
def test_the_recorded_request_id_matches_the_one_returned(self, app):
|
||||
"""An id the caller can quote is the point; a different id in the log
|
||||
makes it useless for support."""
|
||||
with app.test_request_context('/__probe/correlated'):
|
||||
body, _ = unexpected_response(_boom())
|
||||
|
||||
row = _errors_for('/__probe/correlated')[0]
|
||||
assert (row.get_context() or {}).get('request_id') == body['request_id']
|
||||
|
||||
def test_the_endpoint_and_method_are_recorded(self, app):
|
||||
with app.test_request_context('/__probe/where', method='POST'):
|
||||
unexpected_response(_boom())
|
||||
|
||||
row = _errors_for('/__probe/where')[0]
|
||||
assert row.method == 'POST'
|
||||
|
||||
def test_repeated_crashes_dedupe_rather_than_flood(self, app):
|
||||
"""error_log_service dedupes by fingerprint; routing through the door
|
||||
must not defeat that and turn a hot loop into a million rows."""
|
||||
for _ in range(3):
|
||||
with app.test_request_context('/__probe/deduped'):
|
||||
unexpected_response(_boom())
|
||||
|
||||
rows = _errors_for('/__probe/deduped')
|
||||
assert len(rows) == 1
|
||||
assert rows[0].count == 3
|
||||
|
||||
|
||||
class TestReportingNeverMasksTheCrash:
|
||||
"""`record_error` is bulletproof by contract; the door must be too, or a
|
||||
failure to *report* a crash becomes a second, louder crash."""
|
||||
|
||||
def test_a_broken_recorder_does_not_raise(self, app, monkeypatch):
|
||||
from app.services import error_log_service
|
||||
|
||||
def _explode(*_a, **_kw):
|
||||
raise RuntimeError('error log is down')
|
||||
|
||||
monkeypatch.setattr(error_log_service, 'record_error', _explode)
|
||||
|
||||
with app.test_request_context('/__probe/recorder-down'):
|
||||
body, status = unexpected_response(_boom())
|
||||
|
||||
assert status == 500
|
||||
assert body['request_id']
|
||||
|
||||
def test_it_survives_a_session_already_in_a_failed_transaction(self, app):
|
||||
"""The case the rollback exists for: a DB-caused crash leaves the shared
|
||||
session unusable, and record_error's first query would raise."""
|
||||
from app import db
|
||||
|
||||
with app.test_request_context('/__probe/failed-txn'):
|
||||
try:
|
||||
db.session.execute(db.text('SELECT * FROM table_that_is_not_there'))
|
||||
except Exception: # noqa: BLE001
|
||||
pass
|
||||
body, status = unexpected_response(_boom())
|
||||
|
||||
assert status == 500
|
||||
assert len(_errors_for('/__probe/failed-txn')) == 1
|
||||
|
||||
|
||||
def test_the_global_handler_and_the_door_answer_identically(app, client):
|
||||
"""Two doors that answer differently are two contracts again."""
|
||||
with app.test_request_context('/__probe/shape'):
|
||||
body, _ = unexpected_response(_boom())
|
||||
|
||||
assert body == unexpected_error_body(body['request_id'])
|
||||
|
||||
|
||||
def test_record_unexpected_returns_the_id_it_recorded_under(app):
|
||||
with app.test_request_context('/__probe/returns-id'):
|
||||
request_id = record_unexpected(_boom())
|
||||
|
||||
assert request_id
|
||||
recorded = _errors_for('/__probe/returns-id')[0].get_context() or {}
|
||||
assert recorded.get('request_id') == request_id
|
||||
@@ -0,0 +1,119 @@
|
||||
"""The unreported-crash ratchet (plan 76, milestone B).
|
||||
|
||||
Second-wave rule 1: every door gets a ratchet before its migration starts,
|
||||
because a migration without one regrows. This is milestone B's.
|
||||
|
||||
It holds at zero the number of API handlers that tell a caller "this crashed"
|
||||
(HTTP 500) without the crash reaching the centralized error log. The failure it
|
||||
prevents is silent: the endpoint still answers, the operator's /monitoring/errors
|
||||
page just never learns the crash happened.
|
||||
"""
|
||||
|
||||
from unreported_crash_census import census, count_file, read_ceiling, total
|
||||
|
||||
|
||||
def test_the_count_is_at_or_below_the_ceiling():
|
||||
found = census()
|
||||
count = total(found)
|
||||
ceiling = read_ceiling()
|
||||
worst = sorted(found, key=lambda p: -len(found[p]))[:5]
|
||||
assert count <= ceiling, (
|
||||
f'{count} API handler(s) answer 500 without reporting the crash, '
|
||||
f'ceiling is {ceiling}.\n'
|
||||
f'Return app.error_reporting.unexpected_response(exc) instead of a '
|
||||
f'hand-shaped 500 — or call record_unexpected(exc) if the route must '
|
||||
f'keep its own caller-facing wording.\n'
|
||||
f'Best of all, delete the try/except and let the global 500 handler '
|
||||
f'answer.\n'
|
||||
f'Files: ' + ', '.join(f'{p} ({len(found[p])})' for p in worst)
|
||||
)
|
||||
|
||||
|
||||
def test_the_ceiling_stays_at_zero():
|
||||
"""This one is not a countdown — it started at 43 and closed in one pass.
|
||||
|
||||
A raised ceiling here means somebody reintroduced an invisible crash, which
|
||||
is exactly the regression the ratchet exists to catch.
|
||||
"""
|
||||
assert read_ceiling() == 0, (
|
||||
'the unreported-crash ceiling is not 0; this ratchet is not a legacy '
|
||||
'baseline to shrink but an invariant to hold'
|
||||
)
|
||||
|
||||
|
||||
class TestTheCensusCountsTheRightThings:
|
||||
"""A miscounting ratchet is worse than none: it reads green either way."""
|
||||
|
||||
def _count(self, tmp_path, source):
|
||||
path = tmp_path / 'sample.py'
|
||||
path.write_text(source, encoding='utf-8')
|
||||
return len(count_file(str(path)))
|
||||
|
||||
def test_counts_a_hand_shaped_500(self, tmp_path):
|
||||
assert self._count(tmp_path, (
|
||||
'def view():\n'
|
||||
' try:\n'
|
||||
' return work()\n'
|
||||
' except Exception as exc:\n'
|
||||
" return jsonify({'error': str(exc)}), 500\n"
|
||||
)) == 1
|
||||
|
||||
def test_counts_a_bare_except(self, tmp_path):
|
||||
assert self._count(tmp_path, (
|
||||
'def view():\n'
|
||||
' try:\n'
|
||||
' return work()\n'
|
||||
' except:\n'
|
||||
" return jsonify({'error': 'boom'}), 500\n"
|
||||
)) == 1
|
||||
|
||||
def test_does_not_count_one_that_goes_through_the_door(self, tmp_path):
|
||||
assert self._count(tmp_path, (
|
||||
'def view():\n'
|
||||
' try:\n'
|
||||
' return work()\n'
|
||||
' except Exception as exc:\n'
|
||||
' return unexpected_response(exc)\n'
|
||||
)) == 0
|
||||
|
||||
def test_does_not_count_one_that_records_and_keeps_its_wording(self, tmp_path):
|
||||
"""sso.py's shape: an auth failure should not read as a generic crash."""
|
||||
assert self._count(tmp_path, (
|
||||
'def view():\n'
|
||||
' try:\n'
|
||||
' return work()\n'
|
||||
' except Exception as exc:\n'
|
||||
' record_unexpected(exc)\n'
|
||||
" return jsonify({'error': 'SSO authentication failed'}), 500\n"
|
||||
)) == 0
|
||||
|
||||
def test_does_not_count_one_that_re_raises(self, tmp_path):
|
||||
"""Re-raising reaches the global handler, which reports it."""
|
||||
assert self._count(tmp_path, (
|
||||
'def view():\n'
|
||||
' try:\n'
|
||||
' return work()\n'
|
||||
' except Exception:\n'
|
||||
' cleanup()\n'
|
||||
' raise\n'
|
||||
)) == 0
|
||||
|
||||
def test_does_not_count_a_narrow_except(self, tmp_path):
|
||||
"""Catching a specific error is a domain decision, not a swallowed crash."""
|
||||
assert self._count(tmp_path, (
|
||||
'def view():\n'
|
||||
' try:\n'
|
||||
' return work()\n'
|
||||
' except ValueError as exc:\n'
|
||||
" return jsonify({'error': str(exc)}), 500\n"
|
||||
)) == 0
|
||||
|
||||
def test_does_not_count_a_non_500_answer(self, tmp_path):
|
||||
"""A 400/404 is mapping an expected failure, not declaring a crash."""
|
||||
assert self._count(tmp_path, (
|
||||
'def view():\n'
|
||||
' try:\n'
|
||||
' return work()\n'
|
||||
' except Exception as exc:\n'
|
||||
" return jsonify({'error': str(exc)}), 400\n"
|
||||
)) == 0
|
||||
@@ -0,0 +1,156 @@
|
||||
"""Count API handlers that answer 500 without reporting the crash (plan 76, B).
|
||||
|
||||
The global 500 handler is the only thing that logs a crash and writes it to the
|
||||
centralized error log behind ``/monitoring/errors``. A route that catches
|
||||
``Exception`` itself answers before that handler runs, so its crash is recorded
|
||||
nowhere — the errors page is empty precisely because someone "handled" it.
|
||||
|
||||
A handler is counted when all three hold:
|
||||
|
||||
* it catches ``Exception``/``BaseException``/bare
|
||||
* it returns a response instead of re-raising (so the global handler is out)
|
||||
* that response carries HTTP 500 (it is telling the caller "this crashed")
|
||||
|
||||
and it does NOT go through ``app.error_reporting`` — either
|
||||
``unexpected_response(exc)`` for the standard body, or ``record_unexpected(exc)``
|
||||
where the route must keep its own caller-facing wording (``sso.py`` does: an
|
||||
auth failure should not read as a generic crash on the login page).
|
||||
|
||||
Scope is ``app/api`` on purpose. A service catching an exception and returning a
|
||||
value is making a domain decision, not answering an HTTP request; forcing those
|
||||
through an HTTP-shaped door would be the wrong convergence. The HTTP boundary is
|
||||
where "this is a crash" is decided.
|
||||
|
||||
NOT counted, and deliberately still open (see the plan's milestone C): the
|
||||
handlers that swallow a crash and answer **200**. Those are a bigger problem —
|
||||
a failure reported as success — but converging them is the response-envelope
|
||||
work, not this row.
|
||||
|
||||
Usage (from ``backend/``)::
|
||||
|
||||
python tests/unreported_crash_census.py # print the count + report
|
||||
python tests/unreported_crash_census.py --update # write the count as the ceiling
|
||||
"""
|
||||
|
||||
import ast
|
||||
import os
|
||||
import sys
|
||||
|
||||
HERE = os.path.dirname(os.path.abspath(__file__))
|
||||
BACKEND = os.path.dirname(HERE)
|
||||
API_DIR = os.path.join(BACKEND, 'app', 'api')
|
||||
CEILING_FILE = os.path.join(HERE, 'UNREPORTED_CRASH_CEILING')
|
||||
|
||||
REPORTERS = {'unexpected_response', 'record_unexpected'}
|
||||
|
||||
|
||||
def _rel(path):
|
||||
return os.path.relpath(path, BACKEND).replace(os.sep, '/')
|
||||
|
||||
|
||||
def _is_blanket(handler):
|
||||
t = handler.type
|
||||
if t is None:
|
||||
return True
|
||||
if isinstance(t, ast.Name) and t.id in ('Exception', 'BaseException'):
|
||||
return True
|
||||
if isinstance(t, ast.Tuple):
|
||||
return any(isinstance(e, ast.Name) and e.id in ('Exception', 'BaseException')
|
||||
for e in t.elts)
|
||||
return False
|
||||
|
||||
|
||||
def _reports(handler):
|
||||
for n in ast.walk(handler):
|
||||
if isinstance(n, ast.Call):
|
||||
fn = n.func
|
||||
name = getattr(fn, 'attr', None) or getattr(fn, 'id', None)
|
||||
if name in REPORTERS:
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def _answers_500(handler):
|
||||
"""Returns a response carrying 500, and does not re-raise."""
|
||||
if any(isinstance(n, ast.Raise) for n in ast.walk(handler)):
|
||||
return False
|
||||
for n in ast.walk(handler):
|
||||
if isinstance(n, ast.Return) and isinstance(n.value, ast.Tuple) \
|
||||
and len(n.value.elts) == 2:
|
||||
status = n.value.elts[1]
|
||||
if isinstance(status, ast.Constant) and status.value == 500:
|
||||
return True
|
||||
return False
|
||||
|
||||
|
||||
def count_file(path):
|
||||
"""Return [(lineno, function_name)] of unreported crash answers in one file."""
|
||||
try:
|
||||
with open(path, encoding='utf-8') as f:
|
||||
tree = ast.parse(f.read())
|
||||
except (SyntaxError, UnicodeDecodeError):
|
||||
return []
|
||||
|
||||
owner = {}
|
||||
for node in ast.walk(tree):
|
||||
if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
|
||||
for child in ast.walk(node):
|
||||
if isinstance(child, ast.ExceptHandler):
|
||||
owner.setdefault(id(child), node.name)
|
||||
|
||||
found = []
|
||||
for node in ast.walk(tree):
|
||||
if not isinstance(node, ast.ExceptHandler):
|
||||
continue
|
||||
if not _is_blanket(node) or not _answers_500(node) or _reports(node):
|
||||
continue
|
||||
found.append((node.lineno, owner.get(id(node), '<module>')))
|
||||
return sorted(found)
|
||||
|
||||
|
||||
def census():
|
||||
found = {}
|
||||
if not os.path.isdir(API_DIR):
|
||||
return found
|
||||
for name in sorted(os.listdir(API_DIR)):
|
||||
if not name.endswith('.py'):
|
||||
continue
|
||||
path = os.path.join(API_DIR, name)
|
||||
hits = count_file(path)
|
||||
if hits:
|
||||
found[_rel(path)] = hits
|
||||
return found
|
||||
|
||||
|
||||
def total(found=None):
|
||||
if found is None:
|
||||
found = census()
|
||||
return sum(len(v) for v in found.values())
|
||||
|
||||
|
||||
def read_ceiling():
|
||||
with open(CEILING_FILE, encoding='utf-8') as f:
|
||||
return int(f.read().strip())
|
||||
|
||||
|
||||
def _write_ceiling(value):
|
||||
with open(CEILING_FILE, 'w', encoding='utf-8') as f:
|
||||
f.write('%d\n' % value)
|
||||
|
||||
|
||||
if __name__ == '__main__':
|
||||
found = census()
|
||||
count = total(found)
|
||||
for rel in sorted(found, key=lambda p: (-len(found[p]), p)):
|
||||
hits = found[rel]
|
||||
print('%-45s %d (%s)' % (
|
||||
rel, len(hits), ', '.join('L%d %s()' % h for h in hits[:5])))
|
||||
print('\ntotal: %d' % count)
|
||||
if '--update' in sys.argv:
|
||||
_write_ceiling(count)
|
||||
print('ceiling updated to %d' % count)
|
||||
else:
|
||||
try:
|
||||
print('ceiling: %d' % read_ceiling())
|
||||
except FileNotFoundError:
|
||||
print('no ceiling file yet; run with --update')
|
||||
Reference in new issue
Block a user