Skip to content

fix(services): Improve status code consistency - #1090

Open
vprashrex wants to merge 21 commits into
mainfrom
fix/834-status-code-consistency
Open

vprashrex wants to merge 21 commits into
mainfrom
fix/834-status-code-consistency

Conversation

@vprashrex

@vprashrex vprashrex commented Jul 29, 2026 •

Copy link
Copy Markdown
Collaborator

Issue

Closes #834

Summary

Status codes now mean the same thing across endpoints, every error is reported once with the route and tenant on it, and a Celery task stays in the same trace as the request that started it.

Error responses

request fails
  IntegrityError       -> db_error_handler          23505 → 409, 23503 → 422, else 500
                                                      client: short message; constraint name → logs + Sentry tag only
  HTTPException        -> http_exception_handler    registered on Starlette's base class, so 404/405 also use APIResponse
  any other Exception  -> generic_exception_handler  500, no exception text in the body, one Sentry event (logger.error only)
every handler          -> tags http.route / http.method / http.status_code / error.class, transaction "METHOD /route"

Status code fixes

Where Before After
Credential encrypt / decrypt failure ValueError → 500 502, "Retry shortly."
Delete credentials that don't exist 500 404
Missing provider / credential in request ValueError → 500 400
Duplicate credential / config / version per-call 400 or 500 409 from the central DB handler
Delete an already deleted collection 400 409
Unreadable document format (doc transform) 400 422

Observability

 api request
   llm.start_job
     apply_async
-      (manual inject/extract)  → worker spans reparented to the request, Sentry split them into separate transactions
+      run/run_llm_job          → CeleryInstrumentor parents it; one trace end to end
 celery task_prerun
+  tags celery.task_name, tenant.project_id, tenant.org_id (strings) from task kwargs
 log levels
-  4xx from the client side (LLM job HTTPException, send_callback failures) logged as error
+  logged as warning; 5xx stay error
 sentry
-  profiling, Redis tracing (Redis is still used; only its spans are gone)
+  gen_ai.input.messages / gen_ai.output.messages masked like the other GenAI content keys

Docs updated: docs/wiki/cross-cutting/exceptions.md, docs/wiki/cross-cutting/observability.md, the observability and LLM-call architecture docs, .claude/conventions/celery.md.

Evidence

  • Before: a duplicate credential returned 400 or 500 depending on the call site; a crypto failure returned 500 with the exception text; a bot hitting an unknown path got FastAPI's {"detail": "Not Found"}; each LLM job showed up in Sentry as two transactions (request, worker) and callback 401s were error-level alerts.
    After: checked live against a personal Sentry project:
    • one trace http.server → llm.start_job → apply_async → run_llm_job → chat → provider.execute
    • tenant tags present as strings on API and worker spans
    • no Redis spans, no profiles
    • bot 404 returns the APIResponse shape
    • callback and config errors arrive as warnings
    • a 500 carries the transaction and route
  • Tests: test_exception_handlers.py (DB handler 409/422/500, generic handler captures once, 404/405 shape), credentials, config, version, onboarding, security, collection and tracing tests updated or added. CI checks (3.12) passes.

Merge Danger

Door: two-way

Revert restores the old handlers and status codes; there are no migrations and no data changes.

Blast Radius: API clients

  • Clients that branch on status codes see new values: 409 instead of 400/500 for duplicates, 404 instead of 500 for missing credentials, 422 instead of 400 for unreadable documents, 502 instead of 500 for credential crypto failures.
  • Sentry: Redis spans and profiles stop; LLM jobs now appear as one transaction instead of two, so per-transaction counts drop.
  • codecov/patch is below target on this PR; everything else in CI passes.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026 •

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are limited based on label configuration.

🏷️ Required labels (at least one): 1
  • ready-for-review

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

Run configuration

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot changed the title Refactor error handling across services to use domain-specific exceptions refactor(services): Improve error handling structure Jul 29, 2026
@vprashrex
vprashrex changed the base branch from feat/claude-skill-plan-creator to main July 29, 2026 04:25
@github-actions

github-actions Bot commented Jul 29, 2026 •

Copy link
Copy Markdown

OpenAPI changes   ⚪ No API surface changes

Note

This PR does not modify the API contract.

main ↔ 65c04273 · generated by oasdiff

…ions

- Updated error logging to include exception information for better traceability.
- Introduced `ConflictError`, `UpstreamError`, and `ServiceUnavailableError` for more granular HTTP status codes.
- Changed HTTP status codes in various tests and service methods to align with new error handling conventions.
- Added tests to ensure correct behavior when handling exceptions, including cases for missing resources, state conflicts, and upstream errors.
- Updated documentation to reflect changes in exception handling and status code mappings.
@vprashrex
vprashrex force-pushed the fix/834-status-code-consistency branch from 2898d88 to 10ed0a4 Compare July 29, 2026 04:33
@codecov

codecov Bot commented Jul 29, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.77064% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
backend/app/core/telemetry/llm.py 85.71% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

…s codes

- Replaced custom exceptions (InvalidPayloadError, ServiceUnavailableError, etc.) with HTTPException across various services and CRUD operations to standardize error responses.
- Updated the exception handling logic in evaluations, guardrails, LLM jobs, STT evaluations, and onboarding to raise HTTPException with appropriate status codes.
- Removed references to custom exceptions in tests and replaced them with HTTPException assertions.
- Added a new exception taxonomy in app/core/exceptions.py to categorize domain-specific errors.
- Updated documentation to reflect changes in exception handling and status code consistency.
@vprashrex vprashrex changed the title refactor(services): Improve error handling structure fix(services): Improve status code consistency Jul 29, 2026
@vprashrex vprashrex linked an issue Jul 31, 2026 that may be closed by this pull request
@vprashrex vprashrex self-assigned this Jul 31, 2026
@vprashrex
vprashrex marked this pull request as draft September 29, 2026 05:11
@vprashrex
vprashrex marked this pull request as ready for review October 19, 2026 04:45
- Updated tests to propagate IntegrityError instead of raising HTTPException for unique constraint violations in config and version creation.
- Introduced make_integrity_error utility to simulate IntegrityError with SQLSTATE and constraint details.
- Enhanced test coverage for handling foreign key violations and ensuring proper logging behavior for duplicate run names in evaluation runs.
- Adjusted exception handling in credentials management to correctly propagate unique constraint violations and other integrity errors.
- Improved documentation on exception handling and observability practices related to error logging and status code consistency.
@vprashrex
vprashrex removed the request for review from AkhileshNegi October 23, 2026 02:03
@vprashrex
vprashrex marked this pull request as draft October 23, 2026 02:03
@vprashrex
vprashrex marked this pull request as ready for review October 26, 2026 04:18
@vprashrex
vprashrex requested review from AkhileshNegi and Ayush8923 and removed request for Ayush8923 October 26, 2026 05:28
Comment on lines +199 to +200
# Fixed detail: str(exc) here leaked provider bodies and DB text to callers.
# logger.error alone reports it; adding capture_exception makes a second event.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you take a look at this? why we need this?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Observability: Improve status codes consistency

2 participants