Skip to content

fix: log unexpected errors server-side, return generic message to client (#991)#1057

Merged
psschwei merged 6 commits into
generative-computing:mainfrom
SAY-5:say5-log-unhandled-errors
May 12, 2026
Merged

fix: log unexpected errors server-side, return generic message to client (#991)#1057
psschwei merged 6 commits into
generative-computing:mainfrom
SAY-5:say5-log-unhandled-errors

Conversation

@SAY-5
Copy link
Copy Markdown
Contributor

@SAY-5 SAY-5 commented May 11, 2026

Misc PR

Type of PR

  • Bug Fix
  • New Feature
  • Documentation
  • Other

Description

The catch-all except Exception in cli/serve/app.py had two problems:

  1. No server-side log trail. Operators saw a 500 response with no stack trace in the application log, the exact case where a visible traceback matters.
  2. Exception message leaked verbatim to clients. f"Internal server error: {e!s}" returned arbitrary str(e) content (file paths, internal module names, schema fragments) across the OpenAI-compatible API boundary.

Replaces the handler with logger.exception("Unhandled error in chat-completion handler") (full traceback, named logger) plus a generic client message. The explicit except ValueError 400-mapping above it stays untouched, that path is intentional validation feedback.

Testing

  • Tests added to the respective file if code was changed
  • New code has 100% coverage if code as added
  • Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

  • AI coding assistants used

@SAY-5 SAY-5 requested a review from a team as a code owner May 11, 2026 09:36
@SAY-5 SAY-5 requested review from akihikokuroda and psschwei May 11, 2026 09:36
@github-actions github-actions Bot added the bug Something isn't working label May 11, 2026
@akihikokuroda akihikokuroda changed the title fix(serve): log unexpected errors server-side, return generic message to client (#991) fix: log unexpected errors server-side, return generic message to client (#991) May 11, 2026
@github-actions
Copy link
Copy Markdown
Contributor

The PR description has been updated. Please fill out the template for your PR to be reviewed.

@akihikokuroda
Copy link
Copy Markdown
Member

Thanks for your contribution. I updated the title and descriptions to make the CI works.

Comment thread cli/serve/app.py Outdated
import uuid
from typing import Any, Literal, cast

logger = logging.getLogger(__name__)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe this line being here is what's causing the CI failure. Can you move this down to after all the imports?

@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 11, 2026

Good catch, I'll move that import below the others and push.

@SAY-5 SAY-5 force-pushed the say5-log-unhandled-errors branch from 4c3905d to 64997cb Compare May 11, 2026 17:35
@psschwei
Copy link
Copy Markdown
Member

@SAY-5 looks like there's a test error

@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 11, 2026

Good catch. The two failing assertions were checking that the raw exception text leaks into the client response, which is exactly what this PR removes. I flipped both checks to assert the opposite (raw exception text must NOT appear) and kept the generic message check. Pushed a50ea10 + 42f1988.

@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 11, 2026

The logger = logging.getLogger(__name__) line is already after all imports (line 46, after the .utils import). The actual CI failure was the test assertions still expecting the raw exception text to appear in the response body, which I fixed in 42f1988 by updating both assertions to check that the raw text does NOT leak.

Comment thread cli/serve/app.py Outdated
import asyncio
import importlib.util
import inspect
import logging
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We probably should use MelleaLogger instead of the python one

Switch the serve handler's logger from the stdlib logging.getLogger to MelleaLogger.get_logger so unhandled errors surface through the shared mellea logging configuration.

Signed-off-by: SAY-5 <say.apm35@gmail.com>
@SAY-5
Copy link
Copy Markdown
Contributor Author

SAY-5 commented May 11, 2026

Switched to MelleaLogger.get_logger in d2080c9. The 19 serve tests still pass locally.

Copy link
Copy Markdown
Member

@psschwei psschwei left a comment

Choose a reason for hiding this comment

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

Thanks @SAY-5 !

@psschwei psschwei added this pull request to the merge queue May 12, 2026
Merged via the queue into generative-computing:main with commit e2e1b5d May 12, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

m serve: generic except-Exception handler masks errors and leaks message to client

3 participants