mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
[chore] converters: typed InvalidIdentifierError from require_uuid + log the orchestrator drop (#25)
require_uuid raised a bare ValueError('UUID value cannot be None'), so a
malformed/None identifier propagated as an opaque error callers either let
500 or broad-catch-and-silently-swallow — the orchestrator reaper call site
wrapped it in a bare except-Exception return with NO log, dropping a bad
task_id_str invisibly. Introduce InvalidIdentifierError(ValueError) and
raise it from require_uuid for both None and unparseable input; it stays a
ValueError subclass so existing except-ValueError / except-Exception callers
are unaffected, but typed so a caller can handle a bad identifier distinctly.
The reaper now catches the typed error, logs at warning, and no-ops — the
drop is visible instead of swallowed.
Tests: None and an unparseable string both raise InvalidIdentifierError; it
subclasses ValueError (back-comat).
This commit is contained in:
@@ -4626,11 +4626,20 @@ class AgentOrchestrator:
|
|||||||
"""
|
"""
|
||||||
from roboco.db.base import get_session_factory
|
from roboco.db.base import get_session_factory
|
||||||
from roboco.services.task import TaskService
|
from roboco.services.task import TaskService
|
||||||
from roboco.utils.converters import require_uuid
|
from roboco.utils.converters import InvalidIdentifierError, require_uuid
|
||||||
|
|
||||||
try:
|
try:
|
||||||
task_id = require_uuid(task_id_str)
|
task_id = require_uuid(task_id_str)
|
||||||
except Exception:
|
except InvalidIdentifierError as exc:
|
||||||
|
# A malformed task_id_str is a bad identifier, not a transient
|
||||||
|
# failure — log it so the drop is visible instead of swallowed,
|
||||||
|
# then no-op (nothing to release). Other exceptions still fall
|
||||||
|
# through to the broad catch below (#25).
|
||||||
|
logger.warning(
|
||||||
|
"stopped agent claim had malformed task id",
|
||||||
|
task_id_str=task_id_str,
|
||||||
|
error=str(exc),
|
||||||
|
)
|
||||||
return
|
return
|
||||||
try:
|
try:
|
||||||
factory = get_session_factory()
|
factory = get_session_factory()
|
||||||
|
|||||||
@@ -8,6 +8,16 @@ from typing import Any
|
|||||||
from uuid import UUID as PythonUUID
|
from uuid import UUID as PythonUUID
|
||||||
|
|
||||||
|
|
||||||
|
class InvalidIdentifierError(ValueError):
|
||||||
|
"""A malformed/None identifier reached UUID coercion.
|
||||||
|
|
||||||
|
A ``ValueError`` subclass so existing ``except ValueError`` callers keep
|
||||||
|
working, but typed so a caller can distinguish a bad identifier from any
|
||||||
|
other exception instead of broad-catching and silently swallowing it
|
||||||
|
(#25).
|
||||||
|
"""
|
||||||
|
|
||||||
|
|
||||||
def require_uuid(value: Any) -> PythonUUID:
|
def require_uuid(value: Any) -> PythonUUID:
|
||||||
"""
|
"""
|
||||||
Convert SQLAlchemy UUID to Python UUID, raising if None.
|
Convert SQLAlchemy UUID to Python UUID, raising if None.
|
||||||
@@ -19,13 +29,19 @@ def require_uuid(value: Any) -> PythonUUID:
|
|||||||
Python UUID
|
Python UUID
|
||||||
|
|
||||||
Raises:
|
Raises:
|
||||||
ValueError: If value is None or cannot be converted
|
InvalidIdentifierError: If value is None or cannot be parsed as a UUID.
|
||||||
|
A ``ValueError`` subclass, so existing ``except ValueError`` /
|
||||||
|
``except Exception`` callers are unaffected, but typed so callers
|
||||||
|
can handle a bad identifier distinctly instead of swallowing it.
|
||||||
"""
|
"""
|
||||||
if value is None:
|
if value is None:
|
||||||
raise ValueError("UUID value cannot be None")
|
raise InvalidIdentifierError("UUID value cannot be None")
|
||||||
if isinstance(value, PythonUUID):
|
if isinstance(value, PythonUUID):
|
||||||
return value
|
return value
|
||||||
return PythonUUID(str(value))
|
try:
|
||||||
|
return PythonUUID(str(value))
|
||||||
|
except (ValueError, AttributeError, TypeError) as exc:
|
||||||
|
raise InvalidIdentifierError(f"invalid UUID identifier: {value!r}") from exc
|
||||||
|
|
||||||
|
|
||||||
def repo_key(git_url: str) -> str:
|
def repo_key(git_url: str) -> str:
|
||||||
|
|||||||
@@ -6,6 +6,7 @@ from uuid import uuid4
|
|||||||
|
|
||||||
import pytest
|
import pytest
|
||||||
from roboco.utils.converters import (
|
from roboco.utils.converters import (
|
||||||
|
InvalidIdentifierError,
|
||||||
require_uuid,
|
require_uuid,
|
||||||
to_python_uuid,
|
to_python_uuid,
|
||||||
to_python_uuid_list,
|
to_python_uuid_list,
|
||||||
@@ -27,6 +28,27 @@ def test_require_uuid_raises_for_none() -> None:
|
|||||||
require_uuid(None)
|
require_uuid(None)
|
||||||
|
|
||||||
|
|
||||||
|
def test_require_uuid_none_raises_typed_identifier_error() -> None:
|
||||||
|
"""#25: a None identifier raises the typed InvalidIdentifierError (a
|
||||||
|
ValueError subclass), not a bare ValueError, so callers can distinguish a
|
||||||
|
bad identifier from any other exception instead of broad-catching."""
|
||||||
|
with pytest.raises(InvalidIdentifierError, match="cannot be None"):
|
||||||
|
require_uuid(None)
|
||||||
|
|
||||||
|
|
||||||
|
def test_require_uuid_unparseable_raises_typed_identifier_error() -> None:
|
||||||
|
"""#25: an unparseable identifier raises InvalidIdentifierError, not a bare
|
||||||
|
ValueError — the dropped-identifier pattern must surface as a typed error."""
|
||||||
|
with pytest.raises(InvalidIdentifierError, match="invalid UUID identifier"):
|
||||||
|
require_uuid("not-a-uuid")
|
||||||
|
|
||||||
|
|
||||||
|
def test_invalid_identifier_error_is_value_error() -> None:
|
||||||
|
"""Back-compat: InvalidIdentifierError subclasses ValueError so existing
|
||||||
|
`except ValueError` callers keep catching it."""
|
||||||
|
assert issubclass(InvalidIdentifierError, ValueError)
|
||||||
|
|
||||||
|
|
||||||
def test_to_python_uuid_returns_none_for_none() -> None:
|
def test_to_python_uuid_returns_none_for_none() -> None:
|
||||||
assert to_python_uuid(None) is None
|
assert to_python_uuid(None) is None
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user