feat(handlers): implement circuit breaker pattern for DbHandler [OMN-780] by jonahgabriel · Pull Request #159 · OmniNode-ai/omnibase_infra

jonahgabriel · 2026-01-16T15:18:26Z

Summary

Implements circuit breaker pattern for HandlerDb to provide connection resilience
Uses existing MixinAsyncCircuitBreaker following established codebase patterns
Only infrastructure failures (connection errors, timeouts) trip the circuit - application bugs (syntax errors, missing tables/columns) do not

Changes

Component	Change
Class inheritance	Added `MixinAsyncCircuitBreaker`
Initialization	Circuit breaker initialized after pool creation (threshold=5, reset_timeout=30s)
`_execute_query()`	Check → Execute → Reset/Record pattern
`_execute_statement()`	Check → Execute → Reset/Record pattern
`describe()`	Includes circuit breaker state in response
`shutdown()`	Resets circuit breaker
`ModelDbDescribeResponse`	Added `circuit_breaker` field

Error Handling Design

Error Type	Trips Circuit	Reason
`PostgresConnectionError`	✅ Yes	Infrastructure issue - DB unavailable
`QueryCanceledError`	✅ Yes	Infrastructure issue - timeout/overload
`PostgresSyntaxError`	❌ No	Application bug - bad SQL
`UndefinedTableError`	❌ No	Application bug - schema mismatch
`UndefinedColumnError`	❌ No	Application bug - schema mismatch

Test plan

Linear

Closes OMN-780

Summary by CodeRabbit

New Features
- Database handler now includes a circuit breaker with lifecycle management and transient vs. permanent error classification for PostgreSQL operations.
- Circuit-breaker state (state, failure counts, thresholds) is exposed in database diagnostics/describe output for observability.
Tests
- Added comprehensive unit tests covering SQL error classification and circuit-breaker behavior across varied PostgreSQL error scenarios.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

…780] Add MixinAsyncCircuitBreaker to HandlerDb for connection resilience. The circuit breaker protects against transient infrastructure failures (connection errors, timeouts) while allowing application-level errors (syntax errors, missing tables/columns) to pass through without tripping the circuit. Changes: - Add MixinAsyncCircuitBreaker to class inheritance - Initialize circuit breaker after pool creation (threshold=5, reset=30s) - Wrap _execute_query() and _execute_statement() with CB pattern - Update describe() to include circuit breaker state - Update shutdown() to reset circuit breaker - Add circuit_breaker field to ModelDbDescribeResponse

linear · 2026-01-16T15:18:29Z

OMN-780

coderabbitai · 2026-01-16T15:18:35Z

📝 Walkthrough

Walkthrough

Integrates a circuit breaker into HandlerDb, adds PostgreSQL error classification for transient vs permanent failures, wires breaker lifecycle into initialization/shutdown and query execution, and exposes breaker state in the DB describe response. Tests for classification and breaker behavior were added.

Changes

Cohort / File(s)	Summary
Handler — circuit breaker & error classification `src/omnibase_infra/handlers/handler_db.py`	HandlerDb now inherits `MixinAsyncCircuitBreaker`; adds `_circuit_breaker_initialized`, lifecycle init/shutdown wiring, `_is_transient_error` with SQLSTATE classification sets, circuit-breaker pre-checks, post-success resets, and circuit-aware error handling across `_execute_query`, `_execute_statement`, and related paths.
Model — describe response `src/omnibase_infra/handlers/models/model_db_describe_response.py`	Adds optional `circuit_breaker: dict[str, JsonType]
Tests — classification & breaker behavior `tests/unit/handlers/test_handler_db.py`	Adds `TestHandlerDbTransientErrorClassification` and `TestHandlerDbCircuitBreakerErrorClassification` suites covering SQLSTATE classification, asyncpg exception mappings, and circuit-breaker trip/no-trip scenarios; updates `__all__`.

Sequence Diagram

sequenceDiagram
    participant Client as Client
    participant Handler as HandlerDb
    participant CB as CircuitBreaker
    participant Pool as ConnectionPool
    participant PG as AsyncPG

    Client->>Handler: execute_query(...)
    Handler->>CB: check_allow_request()
    alt Circuit Open
        CB-->>Handler: raise CircuitOpen
        Handler-->>Client: CircuitOpen error
    else Circuit Closed/Half-Open
        CB-->>Handler: allow
        Handler->>Pool: acquire_connection()
        Pool-->>Handler: connection
        Handler->>PG: execute SQL
        alt Success
            PG-->>Handler: result
            Handler->>CB: record_success / reset
            Handler-->>Client: result
        else Error (transient)
            PG-->>Handler: error (sqlstate ∈ transient set)
            Handler->>CB: record_failure()
            Handler-->>Client: error
        else Error (permanent)
            PG-->>Handler: error (sqlstate ∉ transient set)
            Handler-->>Client: error
        end
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 A breaker hums beneath the code,
Guarding queries down the road,
When transient storms begin to brew,
I mark them, count them—then renew,
Resilient hops and sturdy cheer! 🎛️

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

claude · 2026-01-16T15:20:37Z

Code Review - PR #159: Circuit Breaker Pattern for HandlerDb

Summary

This PR implements the circuit breaker pattern for HandlerDb using the existing MixinAsyncCircuitBreaker. The implementation is solid and follows established codebase patterns. The error handling logic intelligently distinguishes between infrastructure failures (which trip the circuit) and application bugs (which don't).

✅ Strengths

1. Correct Circuit Breaker Integration

Properly extends MixinAsyncCircuitBreaker following the mixin pattern
Circuit breaker initialization happens after pool creation succeeds (line 294-298)
Uses appropriate configuration: threshold=5, reset_timeout=30.0s, transport_type=DATABASE
Properly tracks initialization state with _circuit_breaker_initialized flag

2. Smart Error Classification

The distinction between infrastructure vs application errors is well-designed:

Error Type	Trips Circuit	Reasoning
`PostgresConnectionError`	✅ Yes	Infrastructure - database unavailable
`QueryCanceledError`	✅ Yes	Infrastructure - timeout/overload
`PostgresSyntaxError`	❌ No	Application bug - bad SQL
`UndefinedTableError`	❌ No	Application bug - schema mismatch
`UndefinedColumnError`	❌ No	Application bug - schema mismatch

This is exactly correct - the circuit breaker should only open for transient infrastructure failures, not permanent application bugs.

3. Proper Lock Usage

All circuit breaker operations correctly use async with self._circuit_breaker_lock:

Line 510-514: Check circuit before operation
Line 520-522: Reset on success
Line 528-533: Record failure on timeout
Line 541-547: Record failure on connection error

4. Clean Shutdown Handling

Properly resets circuit breaker state in shutdown() (lines 356-359)

5. Good Observability

Circuit breaker state exposed via describe() method (line 774-775, 782)
New circuit_breaker field in ModelDbDescribeResponse with proper documentation

⚠️ Issues Found

1. CRITICAL: Missing Circuit Check in `_execute_statement` ❌

In _execute_statement (lines 625-659), the circuit breaker check happens before the operation, but failure recording happens inside a catch block that catches all PostgresError types and only filters for connection/timeout errors.

Problem:

except asyncpg.PostgresError as e:
    # Record circuit failure ONLY for connection/timeout errors
    if self._circuit_breaker_initialized and isinstance(
        e, (asyncpg.PostgresConnectionError, asyncpg.QueryCanceledError)
    ):
        async with self._circuit_breaker_lock:
            await self._record_circuit_failure(...)
    raise self._map_postgres_error(e, ctx) from e

This catch-all approach is inconsistent with _execute_query (lines 562-599) which has explicit, separate catch blocks for each error type.

Why This Matters:

Maintainability: If someone adds a new PostgresError subclass, it will be silently caught here
Consistency: The two methods should use the same error handling pattern
Clarity: Explicit is better than implicit (Python Zen)

Recommendation:
Refactor _execute_statement to use explicit catch blocks like _execute_query:

try:
    async with self._pool.acquire() as conn:
        result = await conn.execute(sql, *parameters)
    
    # Reset circuit breaker on success
    if self._circuit_breaker_initialized:
        async with self._circuit_breaker_lock:
            await self._reset_circuit_breaker()
    
    row_count = self._parse_row_count(result)
    return self._build_response([], row_count, correlation_id, input_envelope_id)

except asyncpg.QueryCanceledError as e:
    # Infrastructure failure - record
    if self._circuit_breaker_initialized:
        async with self._circuit_breaker_lock:
            await self._record_circuit_failure("db.execute", correlation_id)
    raise InfraTimeoutError(
        f"Query timed out after {self._timeout}s",
        context=ctx,
        timeout_seconds=self._timeout,
    ) from e

except asyncpg.PostgresConnectionError as e:
    # Infrastructure failure - record
    if self._circuit_breaker_initialized:
        async with self._circuit_breaker_lock:
            await self._record_circuit_failure("db.execute", correlation_id)
    raise InfraConnectionError(
        "Database connection lost during execute", context=ctx
    ) from e

except asyncpg.PostgresSyntaxError as e:
    # Application error - do NOT trip circuit
    raise RuntimeHostError(f"SQL syntax error: {e.message}", context=ctx) from e

except asyncpg.UndefinedTableError as e:
    # Application error - do NOT trip circuit
    raise RuntimeHostError(f"Table not found: {e.message}", context=ctx) from e

except asyncpg.UndefinedColumnError as e:
    # Application error - do NOT trip circuit
    raise RuntimeHostError(f"Column not found: {e.message}", context=ctx) from e

except asyncpg.PostgresError as e:
    # Generic PostgreSQL error - do NOT trip circuit
    raise RuntimeHostError(
        f"Database error: {type(e).__name__}", context=ctx
    ) from e

This makes the error classification explicit and maintainable.

2. Missing Success Reset in `_execute_statement` ⚠️

In _execute_query, the circuit breaker is reset after acquiring the connection but before building the response (lines 551-554):

async with self._pool.acquire() as conn:
    rows = await conn.fetch(sql, *parameters)

# Reset circuit breaker on success
if self._circuit_breaker_initialized:
    async with self._circuit_breaker_lock:
        await self._reset_circuit_breaker()

return self._build_response(...)

However, in _execute_statement, the reset happens after the try block (lines 636-642) but only for the successful path. If an exception occurs, the reset never happens (which is correct), but the code structure is less clear.

Recommendation:
Move the success reset to immediately after the connection operation completes, matching _execute_query's pattern for consistency.

3. MINOR: Duplicate Comments 📝

Lines 578-579 in _execute_query:

except asyncpg.PostgresSyntaxError as e:
    # Application error - do NOT trip circuit
except asyncpg.UndefinedTableError as e:
    # Application error - do NOT trip circuit
except asyncpg.UndefinedColumnError as e:
    # Application error - do NOT trip circuit

These comments are helpful but repetitive. Consider consolidating:

# Application errors - do NOT trip circuit (not infrastructure issues)
except asyncpg.PostgresSyntaxError as e:
    raise RuntimeHostError(f"SQL syntax error: {e.message}", context=ctx) from e
except asyncpg.UndefinedTableError as e:
    raise RuntimeHostError(f"Table not found: {e.message}", context=ctx) from e
except asyncpg.UndefinedColumnError as e:
    raise RuntimeHostError(f"Column not found: {e.message}", context=ctx) from e

Or use a single comment above the group.

4. MINOR: Docstring Could Be More Specific 📖

The circuit breaker docstring at lines 174-183 says:

"Only connection-related errors (PostgresConnectionError, QueryCanceledError) trip the circuit"

This is accurate but could be more specific about why:

Circuit Breaker:
    This handler uses MixinAsyncCircuitBreaker for connection resilience.
    The circuit breaker protects against cascading failures when PostgreSQL
    becomes unavailable or overloaded.
    
    Only **infrastructure failures** trip the circuit:
    - PostgresConnectionError: Database unreachable (network/DNS/auth)
    - QueryCanceledError: Database overloaded (timeout)
    
    **Application bugs do NOT trip the circuit** (syntax errors, missing 
    tables/columns, constraint violations) - these are permanent errors
    that will never self-heal via circuit breaker retry logic.

🧪 Test Coverage

✅ What's Covered

Existing 65 unit tests pass
Type checking (mypy) passes
Lint checking (ruff) passes
Any type validator passes

⚠️ What's Missing

No tests for circuit breaker behavior! The integration tests in test_db_handler_integration.py don't cover:

Circuit opens after threshold failures
- Simulate 5 connection errors → verify circuit opens
Circuit blocks requests when open
- Verify InfraUnavailableError raised when circuit is open
Circuit transitions to half-open after timeout
- Wait 30s → verify next request attempts operation
Circuit resets on success
- Fail 3 times → succeed once → verify failure count resets
Application errors don't trip circuit
- Syntax error → verify circuit remains closed
Circuit breaker state in describe()
- Verify circuit_breaker field populated correctly

Recommendation:
Add a new test class TestHandlerDbCircuitBreaker to test_db_handler_integration.py covering these scenarios. You can mock connection failures using unittest.mock to avoid waiting for real timeouts.

🔒 Security

✅ No Issues

No exposure of sensitive data (DSN properly sanitized)
Circuit breaker state doesn't leak credentials
Correlation IDs properly propagated

📊 Performance

✅ Minimal Overhead

Lock acquisition only happens on circuit breaker operations (fast)
No performance degradation for happy path
Circuit breaker state checks are O(1)

⚠️ Potential Concern

Reset timeout of 30s might be too aggressive for production.

If PostgreSQL is temporarily unavailable (e.g., brief network partition, rolling restart), 30s might cause unnecessary circuit opens. Consider:

Development/Testing: 30s is fine
Production: 60s or configurable via environment variable

Recommendation:
Make reset timeout configurable:

reset_timeout = parse_env_float(
    "POSTGRES_CIRCUIT_BREAKER_RESET_TIMEOUT", 
    default=30.0
)

📝 Documentation

✅ Good

Docstring explains circuit breaker purpose
PR description has clear error classification table
ModelDbDescribeResponse field documented

⚠️ Could Improve

Add ADR (Architecture Decision Record) explaining why circuit breaker is needed for database handlers
Update docs/patterns/circuit_breaker_implementation.md with database-specific guidance
Document the error classification rationale

🎯 Overall Assessment

Category	Rating	Notes
Correctness	⚠️ 7/10	Missing explicit error handling in `_execute_statement`
Code Quality	✅ 9/10	Clean, follows patterns, minor inconsistencies
Test Coverage	⚠️ 5/10	No circuit breaker-specific tests
Security	✅ 10/10	No issues
Performance	✅ 9/10	Minimal overhead, configurable timeout needed
Documentation	✅ 8/10	Good inline docs, could add ADR

✅ Approval Recommendation

Approve with minor changes requested.

Required Changes:

✅ Refactor _execute_statement error handling to use explicit catch blocks (consistency with _execute_query)

Recommended Changes (not blocking):

Add circuit breaker integration tests
Make reset timeout configurable
Add ADR for database circuit breaker pattern

🚀 Next Steps

After this PR merges:

Add circuit breaker tests in follow-up PR
Monitor circuit breaker metrics in production
Consider making threshold/timeout configurable if needed
Update handbook with database resilience patterns

Great work overall! The circuit breaker pattern is exactly what's needed for database resilience. The error classification logic is particularly well thought out. Just needs consistency improvements in error handling.

cc @jonahgabriel

…ment [OMN-780] Address PR review feedback: - Replace catch-all PostgresError handling with explicit catch blocks - Match _execute_query pattern for consistency and maintainability - Infrastructure errors (QueryCanceledError, PostgresConnectionError) trip circuit - Application errors (PostgresSyntaxError, UndefinedTable/Column) do not trip circuit

claude · 2026-01-16T15:50:18Z

PR Review: Circuit Breaker Implementation for HandlerDb

Overview

This PR implements circuit breaker resilience for HandlerDb using the established MixinAsyncCircuitBreaker pattern. The implementation follows ONEX architectural principles and provides connection-level resilience for database operations.

✅ Strengths

1. Correct Circuit Breaker Pattern Usage

Properly extends MixinAsyncCircuitBreaker as required by CLAUDE.md
Circuit breaker initialized after pool creation in initialize() (line 294-300) - this is correct to avoid tripping circuit during startup
Correct check → execute → reset/record pattern in both _execute_query() and _execute_statement()
Properly uses async with self._circuit_breaker_lock for all circuit breaker operations

2. Intelligent Error Classification

The error handling design correctly distinguishes infrastructure vs application errors:

Error Type	Trips Circuit	Reasoning
`PostgresConnectionError`	✅ Yes	Infrastructure - DB unavailable
`QueryCanceledError`	✅ Yes	Infrastructure - timeout/overload
`PostgresSyntaxError`	❌ No	Application bug - bad SQL
`UndefinedTableError`	❌ No	Application bug - schema mismatch
`UndefinedColumnError`	❌ No	Application bug - schema mismatch

This prevents circuit breaker from masking application bugs - excellent design decision.

3. Proper State Management

Circuit breaker state tracked via _circuit_breaker_initialized flag
State reset in shutdown() (lines 356-359)
State exposed via describe() for observability (line 802)

4. Clean Integration

Model updated (ModelDbDescribeResponse) to include circuit breaker state
No breaking changes to existing API surface
All 65 existing tests pass (per PR description)

🔍 Issues & Concerns

CRITICAL: Missing Test Coverage

The PR states "All 65 existing unit tests pass" but does not add new tests for circuit breaker behavior. This is a significant gap.

Required test coverage:

✅ Circuit opens after threshold failures (connection errors)
✅ Circuit resets after timeout
✅ Circuit resets on successful operation
✅ InfraUnavailableError raised when circuit open
✅ Application errors (syntax, missing table) do NOT trip circuit
✅ Circuit state visible in describe() response
✅ Circuit reset in shutdown()

Recommendation: Add a dedicated test class TestHandlerDbCircuitBreaker in tests/unit/handlers/test_handler_db.py covering these scenarios. Reference existing circuit breaker tests in tests/unit/mixins/test_mixin_async_circuit_breaker.py for patterns.

MEDIUM: Lock Acquisition Ordering

Location: Lines 540-546, 627-633

The current pattern acquires the lock separately for check and record:

# Check circuit (lock acquired)
if self._circuit_breaker_initialized:
    async with self._circuit_breaker_lock:
        await self._check_circuit_breaker(...)

# Execute query
async with self._pool.acquire() as conn:
    rows = await conn.fetch(sql, *parameters)

# Reset circuit (lock acquired separately)
if self._circuit_breaker_initialized:
    async with self._circuit_breaker_lock:
        await self._reset_circuit_breaker()

Issue: The lock is released between operations, allowing race conditions if multiple coroutines execute queries concurrently. A failure in one coroutine during the gap between "check" and "record" could result in inconsistent state.

However: This pattern is consistent with other uses in the codebase (e.g., kafka_event_bus.py), so this is more of an architectural observation than a blocker.

Recommendation: Consider documenting this behavior in mixin_async_circuit_breaker.py or keeping as-is for consistency with existing implementations.

MEDIUM: Shutdown Race Condition

Location: Lines 356-359

async def shutdown(self) -> None:
    """Close database connection pool and release resources."""
    # Reset circuit breaker state
    if self._circuit_breaker_initialized:
        async with self._circuit_breaker_lock:
            await self._reset_circuit_breaker()
        self._circuit_breaker_initialized = False

    if self._pool is not None:
        await self._pool.close()
        self._pool = None
        self._initialized = False

Issue: If a query is in-flight during shutdown, the following sequence could occur:

Thread A: Checks _circuit_breaker_initialized (True)
Thread B: Calls shutdown(), sets _circuit_breaker_initialized = False
Thread A: Attempts to record failure, but _circuit_breaker_initialized is now False

Impact: Low - likely only affects graceful shutdown scenarios.

Recommendation: Set _circuit_breaker_initialized = False before resetting state, or add a "shutting down" flag that prevents new operations.

LOW: Generic PostgresError Handling

Location: Lines 595-599 (in _execute_query), similar in _execute_statement

except asyncpg.PostgresError as e:
    # Generic PostgreSQL error - do NOT trip circuit (application bug likely)
    raise RuntimeHostError(
        f"Database error: {type(e).__name__}", context=ctx
    ) from e

Issue: The catch-all PostgresError handler assumes all unhandled errors are application bugs. However, some infrastructure-related errors might fall through:

asyncpg.CannotConnectNowError - DB starting up
asyncpg.TooManyConnectionsError - Connection pool exhausted
asyncpg.AdminShutdownError - DB shutting down

These should arguably trip the circuit breaker.

Recommendation: Add explicit handlers for infrastructure-related PostgresError subclasses that should trip the circuit. See asyncpg error hierarchy.

LOW: Configuration Not Exposed

Location: Lines 294-300

self._init_circuit_breaker(
    threshold=5,
    reset_timeout=30.0,
    service_name="db_handler",
    transport_type=EnumInfraTransportType.DATABASE,
)

Issue: Circuit breaker configuration is hardcoded. Threshold=5 and timeout=30s may not be appropriate for all deployments.

Recommendation:

Add circuit_breaker_threshold and circuit_breaker_reset_timeout to handler config
Parse from environment variables with defaults (similar to _pool_size and _timeout)
Document configuration in handler docstring

LOW: Removed Error Mapping

Location: Line 685

The PR removes the _map_postgres_error() method call in _execute_statement:

# OLD (line 573):
except asyncpg.PostgresError as e:
    raise self._map_postgres_error(e, ctx) from e

# NEW (line 685):
except asyncpg.PostgresError as e:
    raise RuntimeHostError(f"Database error: {type(e).__name__}", context=ctx) from e

Issue: The _map_postgres_error() method likely provided more specific error mapping (e.g., InfraAuthenticationError for auth failures). This was removed in favor of a generic RuntimeHostError.

Impact: Loss of error specificity - harder to diagnose issues.

Recommendation: Restore _map_postgres_error() call or inline its logic into the new error handlers.

📋 Documentation & Style

✅ Good:

Docstring updated with circuit breaker explanation (lines 174-184)
Removed outdated TODO comment (line 174)
Clear inline comments explaining error classification

⚠️ Suggestions:

Add ADR (Architecture Decision Record) documenting why application errors don't trip circuit
Update docs/patterns/circuit_breaker_implementation.md with HandlerDb example

🔒 Security

No security concerns identified. The implementation:

Does not expose DSN in circuit breaker state
Properly sanitizes error messages
Follows existing security patterns

🎯 Performance

Positive Impact:

Circuit breaker prevents resource exhaustion during DB outages
Fast-fail when circuit open reduces latency for dependent operations

Potential Concern:

Lock contention on _circuit_breaker_lock under high concurrency
However, this is mitigated by using asyncio.Lock (coroutine-safe, not blocking threads)

📊 Compliance with CLAUDE.md

Rule	Status	Notes
Use `MixinAsyncCircuitBreaker`	✅ Pass	Correctly extends mixin
Lock usage pattern	✅ Pass	Properly uses `async with self._circuit_breaker_lock`
Error hierarchy	⚠️ Partial	Should use infrastructure-specific errors for connection issues
Zero `Any` types	✅ Pass	No `Any` types introduced
Strong typing	✅ Pass	All parameters properly typed
Test coverage	❌ Fail	No new tests for circuit breaker behavior

🚦 Verdict

Recommendation: APPROVE WITH CHANGES REQUESTED

Blockers (must fix before merge):

Add circuit breaker test coverage - at minimum, test that circuit opens on connection errors and does NOT open on application errors

Strongly Recommended (high value, low effort):

Make circuit breaker config (threshold, timeout) configurable
Add explicit handlers for infrastructure-related PostgresError subclasses
Restore _map_postgres_error() logic for specific error types

Nice-to-Have (can be follow-up PRs):

Document shutdown race condition mitigation strategy
Add ADR for error classification design
Benchmark lock contention under high load

🎉 Overall Assessment

This is a solid implementation that correctly applies the circuit breaker pattern following ONEX conventions. The error classification design is particularly well thought out. The main gap is test coverage - please add tests before merging.

Great work on improving database resilience! 🚀

Reviewed by: Claude Sonnet 4.5 (ONEX Infrastructure Review Agent)
Review Date: 2026-01-16

…ions [OMN-780] Add specific exception handlers for ForeignKeyViolationError and NotNullViolationError with descriptive error messages. This improves error classification for debugging and fixes CI test assertions that expect "foreign key" and "not null" phrases in error messages.

claude · 2026-01-16T16:22:06Z

PR Review: Circuit Breaker Pattern for HandlerDb

Summary

This PR implements the circuit breaker pattern for HandlerDb following established codebase patterns. The implementation is solid overall, with excellent error classification that distinguishes infrastructure failures from application bugs.

✅ Strengths

1. Correct Error Classification

The implementation properly distinguishes between infrastructure failures (which trip the circuit) and application errors (which don't):

Error Type	Trips Circuit	Reasoning
`PostgresConnectionError`	✅ Yes	Infrastructure - DB unavailable
`QueryCanceledError`	✅ Yes	Infrastructure - timeout/overload
`PostgresSyntaxError`	❌ No	Application bug - bad SQL
`UndefinedTableError`	❌ No	Application bug - schema mismatch
`UndefinedColumnError`	❌ No	Application bug - schema mismatch
`ForeignKeyViolationError`	❌ No	Application bug - constraint violation
`NotNullViolationError`	❌ No	Application bug - constraint violation

This design prevents the circuit from opening due to code bugs, which is exactly the right behavior.

2. Consistent Pattern Application

Both _execute_query() and _execute_statement() follow the same pattern:

Check circuit before operation
Execute operation
Reset circuit on success
Record failure only for infrastructure errors

3. Proper Initialization & Cleanup

Circuit breaker initialized after pool creation succeeds (handler_db.py:289-299)
Proper cleanup in shutdown() with flag reset (handler_db.py:356-360)
State tracking via _circuit_breaker_initialized flag prevents premature usage

4. Observability Enhancement

The describe() method now includes circuit breaker state (handler_db.py:819-833), enabling operational monitoring.

⚠️ Issues & Recommendations

1. CRITICAL: Lock Ordering Violation Risk

Location: handler_db.py:548-555, handler_db.py:646-653

The connection acquisition happens inside the try block but before the circuit reset:

try:
    async with self._pool.acquire() as conn:  # Lock acquired
        rows = await conn.fetch(sql, *parameters)  # Query executes
    
    # Reset circuit breaker on success
    if self._circuit_breaker_initialized:
        async with self._circuit_breaker_lock:  # Lock order: pool → circuit
            await self._reset_circuit_breaker()

Problem: If the circuit breaker check at line 542 acquires _circuit_breaker_lock first, then the pool connection second, but the reset acquires them in reverse order (pool releases first, then circuit lock), this creates a potential for lock ordering issues.

Risk Level: Medium - While asyncpg's connection acquisition uses async context managers (not traditional locks), the pattern is fragile and could cause issues if pool behavior changes.

Recommended Fix: Acquire circuit lock after connection context exits:

try:
    async with self._pool.acquire() as conn:
        rows = await conn.fetch(sql, *parameters)
    # Connection released here - no lock held
    
    # Now safe to acquire circuit lock
    if self._circuit_breaker_initialized:
        async with self._circuit_breaker_lock:
            await self._reset_circuit_breaker()

The current code actually already does this correctly - the connection is released before acquiring the circuit lock. However, this subtle correctness deserves a comment to prevent future refactoring mistakes.

Suggested Action: Add a comment clarifying the lock ordering:

# Connection released - no locks held before acquiring circuit lock
if self._circuit_breaker_initialized:
    async with self._circuit_breaker_lock:
        await self._reset_circuit_breaker()

2. Test Coverage Gap

No circuit breaker-specific tests were added for HandlerDb.

I searched the test suite and found:

tests/unit/handlers/test_handler_db.py - No circuit breaker tests
No integration tests for circuit breaker behavior in DB handler

Recommended Tests:

Unit Tests:
- Circuit opens after threshold connection failures
- Circuit does NOT open for syntax errors
- Circuit does NOT open for constraint violations
- Circuit resets after successful queries
- describe() includes circuit state
- shutdown() resets circuit
Integration Tests:
- Verify InfraUnavailableError raised when circuit is OPEN
- Verify HALF_OPEN state allows one test request
- Verify circuit closes after successful recovery

3. Minor: Redundant Generic PostgresError Catch

Location: handler_db.py:606-610, handler_db.py:702-706

except asyncpg.PostgresError as e:
    # Generic PostgreSQL error - do NOT trip circuit (application bug likely)
    raise RuntimeHostError(
        f"Database error: {type(e).__name__}", context=ctx
    ) from e

Issue: This catch-all at the end handles ALL remaining PostgresError subclasses. While the comment says "application bug likely," some PostgresError subclasses might be infrastructure-related (e.g., AdminShutdownError, CrashShutdownError, DiskFullError).

Risk: Low - These errors are rare in normal operation, but the classification assumption may not always hold.

Recommendation: Either:

Add explicit handlers for known infrastructure-related PostgresError subclasses
Update comment to clarify this is a catch-all with best-effort classification
Consider logging these as warnings since classification is uncertain

Suggested Fix:

except asyncpg.PostgresError as e:
    # Catch-all for unclassified PostgreSQL errors
    # NOTE: Does not trip circuit - assumes application bug, but some 
    # infrastructure errors (DiskFullError, AdminShutdown) may reach here
    logger.warning(
        "Unclassified PostgreSQL error: %s",
        type(e).__name__,
        extra={"correlation_id": correlation_id},
    )
    raise RuntimeHostError(
        f"Database error: {type(e).__name__}", context=ctx
    ) from e

4. Documentation: Circuit Breaker Configuration

The docstring mentions circuit breaker initialization but doesn't document the threshold/timeout values or their rationale.

Current: handler_db.py:174-184 (good overview)
Missing: Why threshold=5 and reset_timeout=30s?

Recommended Addition to docstring:

Circuit Breaker Configuration:
    - Failure Threshold: 5 consecutive failures
    - Reset Timeout: 30 seconds
    - Rationale: Database connections are critical infrastructure with
      expected recovery time < 30s for transient network issues

📋 Minor Observations

1. Consistent Code Comments

Excellent inline comments explaining the "do NOT trip circuit" reasoning for each application error. This greatly aids maintainability.

2. Proper Error Context Propagation

All errors maintain the ModelInfraErrorContext with correlation IDs, supporting distributed tracing.

3. Model Field Addition

ModelDbDescribeResponse.circuit_breaker field properly typed as dict[str, object] | None with clear documentation.

🎯 Verdict

Recommendation: ✅ Approve with suggestions

This is a high-quality implementation that follows ONEX patterns correctly. The error classification logic is sound, and the circuit breaker integration is well-executed.

Before Merge:

✅ Critical: Verify lock ordering (current code is correct, but add clarifying comment)
⚠️ Important: Add test coverage for circuit breaker behavior
📝 Nice-to-have: Enhance documentation with configuration rationale
🔍 Consider: Review generic PostgresError handling for edge cases

Estimated Effort: ~2-4 hours for comprehensive test coverage

📚 References

CLAUDE.md: Circuit Breaker Implementation (line 324)
docs/patterns/circuit_breaker_implementation.md
docs/patterns/dispatcher_resilience.md
Reference Implementation: src/omnibase_infra/mixins/mixin_async_circuit_breaker.py

Great work on maintaining the established patterns! 🚀

…t breaker [OMN-780] Add SQLSTATE-based error classification to distinguish transient errors (connection failures, timeouts, resource exhaustion) from permanent errors (constraint violations, syntax errors). Only transient errors now trip the circuit breaker, preventing unnecessary service degradation from application bugs like FK or NOT NULL violations.

claude · 2026-01-16T16:49:40Z

PR Review: Circuit Breaker Implementation for HandlerDb [OMN-780]

✅ Overall Assessment

This is a well-implemented, production-ready circuit breaker integration that follows ONEX patterns correctly. The intelligent SQLSTATE-based error classification is particularly impressive and demonstrates deep understanding of PostgreSQL failure modes.

🎯 Strengths

1. Intelligent Error Classification

The _is_transient_error() implementation using PostgreSQL SQLSTATE codes is excellent:

✅ Clear separation between infrastructure failures (08, 53, 57, 58) and application bugs (22, 23, 28, 42)
✅ Comprehensive documentation explaining the rationale
✅ Conservative fallback strategy (unknown errors default to permanent)
✅ Fallback to exception type checking when SQLSTATE unavailable

Reference: src/omnibase_infra/handlers/handler_db.py:537-634

2. Correct Circuit Breaker Pattern

✅ Follows established MixinAsyncCircuitBreaker pattern from other handlers (Vault, Consul)
✅ Circuit breaker initialized after pool creation succeeds (line 317-322)
✅ Proper lock usage: async with self._circuit_breaker_lock in all circuit breaker operations
✅ Check → Execute → Reset/Record pattern consistently applied

3. Comprehensive Test Coverage

The test suite is thorough with 445 new test lines:

✅ TestHandlerDbTransientErrorClassification: 20+ tests covering all SQLSTATE classes
✅ TestHandlerDbCircuitBreakerErrorClassification: Integration tests verifying circuit behavior
✅ Edge cases: No SQLSTATE, unknown classes, specific exception types

Reference: tests/unit/handlers/test_handler_db.py:1468-1913

4. Security & Best Practices

✅ No sensitive information exposed in error messages
✅ Proper error context with correlation IDs
✅ Circuit state exposed in describe() for observability
✅ Circuit breaker reset in shutdown() for clean teardown

🔍 Code Quality Observations

Excellent Design Decisions

Granular Exception Handling (lines 686-744, 793-851)
- Explicit catch blocks for PostgresSyntaxError, UndefinedTableError, UndefinedColumnError, ForeignKeyViolationError, NotNullViolationError
- Each exception clearly annotated with # Application error - do NOT trip circuit
- Final PostgresError catch uses _is_transient_error() for unknown cases
Documentation Quality
- Updated class docstring explains circuit breaker states (lines 197-208)
- _is_transient_error() has extensive docstring with examples (lines 537-572)
- SQLSTATE class codes documented at module level (lines 138-159)
Consistency
- Both _execute_query() and _execute_statement() follow identical error handling patterns
- Circuit breaker initialization flag prevents operations before initialization

⚠️ Potential Issues & Recommendations

1. Race Condition Risk in Connection Acquire (Minor)

Location: Lines 672-673, 781-782

async with self._pool.acquire() as conn:
    rows = await conn.fetch(sql, *parameters)

# Reset circuit breaker on success (AFTER context manager exits)
if self._circuit_breaker_initialized:
    async with self._circuit_breaker_lock:
        await self._reset_circuit_breaker()

Issue: The circuit breaker reset happens after the connection is returned to the pool. If the connection is unhealthy and __aexit__ raises an exception, the circuit won't record the failure.

Recommendation: Consider wrapping the entire acquire context in try/except:

try:
    async with self._pool.acquire() as conn:
        rows = await conn.fetch(sql, *parameters)
    # Reset ONLY if acquire + fetch both succeed
    if self._circuit_breaker_initialized:
        async with self._circuit_breaker_lock:
            await self._reset_circuit_breaker()
except asyncpg.PostgresConnectionError as e:
    # Record failure for connection errors
    ...

However, this is very low risk in practice since asyncpg typically raises during acquire() or fetch(), not during context manager exit.

2. Missing `UniqueViolationError` Handler (Minor)

Location: Lines 710-728

All other constraint violations have explicit handlers (FK, NOT NULL, Check), but UniqueViolationError is missing. It falls through to the generic PostgresError handler which will correctly classify it as permanent (SQLSTATE 23505), but for consistency:

Recommendation: Add explicit handler for symmetry:

except asyncpg.UniqueViolationError as e:
    # Application error - do NOT trip circuit
    raise RuntimeHostError(
        f"Unique constraint violation: {e.message}", context=ctx
    ) from e

This isn't strictly necessary but improves readability and matches the PR's intent to make constraint violations explicit.

3. Circuit Breaker Threshold Justification (Documentation)

Location: Line 317

self._init_circuit_breaker(
    threshold=5,
    reset_timeout=30.0,
    ...
)

Question: Why threshold=5 and timeout=30s?

Recommendation: Add inline comment explaining the rationale:

# Circuit breaker configured for database resilience:
# - threshold=5: Allow brief connection blips without opening circuit
# - reset_timeout=30s: Short recovery window for transient DB issues
self._init_circuit_breaker(
    threshold=5,
    reset_timeout=30.0,
    ...
)

Compare to other handlers:

Vault: threshold=5, reset=60s (docs/patterns/circuit_breaker_implementation.md)
Recommended Kafka: threshold=3, reset=20s (docs/patterns/dispatcher_resilience.md:70-75)

The database threshold of 5 seems reasonable but the 30s reset is more aggressive than Vault's 60s. This is likely intentional (databases recover faster than Vault), but documenting the reasoning helps future maintainers.

🧪 Test Coverage Assessment

Strengths

✅ All 4 transient SQLSTATE classes tested (08, 53, 57, 58)
✅ All 4 permanent SQLSTATE classes tested (22, 23, 28, 42)
✅ Edge cases: no SQLSTATE, unknown classes
✅ Integration tests verify circuit breaker failure counting

Missing Coverage (Optional Enhancements)

Circuit breaker state transitions: Tests verify failure counting but don't verify CLOSED → OPEN → HALF_OPEN transitions
Concurrent operations: Multiple queries with circuit breaker (though test_mixin_async_circuit_breaker_race_conditions.py exists for mixin-level testing)
describe() circuit state: Test that circuit_breaker field in response contains expected state

These are nice-to-haves not blockers - the existing 445 test lines provide excellent coverage.

🔐 Security Considerations

✅ No security concerns identified

DSN remains properly sanitized
Error messages don't expose sensitive data
Circuit breaker state doesn't leak credentials
SQLSTATE logging is safe (no query content or parameters)

📊 Performance Considerations

Positive

✅ Circuit breaker adds minimal overhead (lock acquisition + counter checks)
✅ SQLSTATE classification is O(1) (frozenset lookups)
✅ Explicit exception handlers avoid isinstance() chains

Neutral

Circuit breaker lock is held briefly (no I/O operations while locked)
Error classification happens after failure, not on happy path

Verdict: Negligible performance impact, excellent failure handling.

📝 ONEX Compliance

✅ Fully compliant with ONEX infrastructure patterns:

Requirement	Status	Evidence
No `Any` types	✅ Pass	No violations in diff
Container injection	✅ N/A	Handler doesn't use container (MVP pattern)
Circuit breaker mixin	✅ Pass	Correctly extends `MixinAsyncCircuitBreaker`
Error hierarchy	✅ Pass	Uses `InfraConnectionError`, `InfraTimeoutError`, `RuntimeHostError`
Error context	✅ Pass	All errors include `ModelInfraErrorContext`
Lock safety	✅ Pass	All circuit methods use `async with self._circuit_breaker_lock`
Declarative nodes	✅ N/A	This is a handler, not a node

🎯 Final Recommendations

Must Fix Before Merge

None - this PR is ready to merge as-is.

Nice-to-Have Improvements

Add UniqueViolationError explicit handler for consistency
Document circuit breaker threshold/timeout rationale with inline comment
Consider adding integration test for circuit state transitions (CLOSED → OPEN → HALF_OPEN)

Questions for Author

Was the 30s reset timeout intentionally more aggressive than Vault's 60s?
Have you considered the connection pool exhaustion scenario (e.g., all 5 connections stuck in query timeout)?

✅ Approval

Recommendation: APPROVE ✅

This PR demonstrates:

Deep understanding of PostgreSQL failure modes
Correct application of ONEX circuit breaker patterns
Excellent test coverage
Production-ready error handling

The SQLSTATE-based classification is a significant improvement over naive exception type checking. The minor suggestions above are optional refinements, not blockers.

Great work! 🚀

📚 References

CLAUDE.md Circuit Breaker Pattern: Lines 533-548
MixinAsyncCircuitBreaker: src/omnibase_infra/mixins/mixin_async_circuit_breaker.py
Dispatcher Resilience: docs/patterns/dispatcher_resilience.md
PostgreSQL SQLSTATE Codes: https://www.postgresql.org/docs/current/errcodes-appendix.html

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/omnibase_infra/handlers/handler_db.py`:
- Around line 138-159: The code currently logs full asyncpg error strings
(exposing message/detail/internal_query) in the DB exception handlers; update
those handlers to stop logging str(error) and instead log only non-sensitive
metadata: the exception class name, the error.code or error.sqlstate (if
present), and whether that sqlstate's class is in
_TRANSIENT_SQLSTATE_CLASSES/_PERMANENT_SQLSTATE_CLASSES; remove any inclusion of
error.message, error.detail, error.context, error.internal_query or parameter
values and replace with a short generic message (e.g., "Postgres error
encountered: code=<code>, class=<class>, transient=<bool>") in the
functions/methods that currently call processLogger/errorLogger with str(error)
(the DB exception handlers referenced alongside _TRANSIENT_SQLSTATE_CLASSES and
_PERMANENT_SQLSTATE_CLASSES).

🧹 Nitpick comments (3)

src/omnibase_infra/handlers/handler_db.py (2)
210-218: Consider DI container injection in HandlerDb.__init__.

If this handler is treated as a service, align the constructor with the ModelONEXContainer DI pattern so dependencies are explicit and consistent.

As per coding guidelines, services should accept ModelONEXContainer via __init__.

964-978: Use JsonType for the circuit breaker state.

circuit_breaker is a JSON-compatible payload; aligning the local type with JsonType keeps it consistent with repo typing standards.
♻️ Suggested change
-from omnibase_core.models.dispatch import ModelHandlerOutput
+from omnibase_core.models.dispatch import ModelHandlerOutput
+from omnibase_core.types import JsonType
@@
-        cb_state: dict[str, object] | None = None
+        cb_state: JsonType | None = None
As per coding guidelines, JSON-compatible values should use JsonType.
src/omnibase_infra/handlers/models/model_db_describe_response.py (1)
28-29: Type circuit_breaker as JsonType.

This field represents JSON state; using the shared alias avoids overly generic dicts and keeps consistency.
♻️ Suggested change
-from pydantic import BaseModel, ConfigDict, Field
+from pydantic import BaseModel, ConfigDict, Field
+from omnibase_core.types import JsonType
@@
-    circuit_breaker: dict[str, object] | None = Field(
+    circuit_breaker: JsonType | None = Field(
         default=None,
         description="Circuit breaker state information (state, failures, threshold, etc.)",
     )
As per coding guidelines, JSON-compatible values should use JsonType.

Also applies to: 75-78

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Lite

📥 Commits

Reviewing files that changed from the base of the PR and between fafe200 and 6c02070.

📒 Files selected for processing (3)

src/omnibase_infra/handlers/handler_db.py
src/omnibase_infra/handlers/models/model_db_describe_response.py
tests/unit/handlers/test_handler_db.py

🧰 Additional context used

📓 Path-based instructions (3)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: Never use Any type - use object for generic payloads in function parameters and return types
Use X | None (PEP 604) instead of Optional[X] for nullable types
All services must use ModelONEXContainer for dependency injection via __init__(self, container: ModelONEXContainer)
Use @allow_any decorator with documented reason as exemption mechanism for Any type violations
Use JsonType from omnibase_core.types as the canonical type alias for JSON-compatible values
Use ModelEventEnvelope[object] for generic dispatcher interfaces and object for generic payloads
Infrastructure error handling must use OnexError base class and never expose passwords, API keys, PII, or connection strings with credentials in error messages
Use InfraConnectionError, InfraTimeoutError, InfraAuthenticationError, InfraUnavailableError for transport failures with proper ModelInfraErrorContext
Correlation IDs must be propagated from incoming requests, auto-generated with uuid4() if missing, and included in all error contexts
External service adapters must implement MixinAsyncCircuitBreaker with appropriate threshold and reset_timeout configuration
Protocol resolution must use duck typing via protocols, never use isinstance checks

Files:

src/omnibase_infra/handlers/models/model_db_describe_response.py
tests/unit/handlers/test_handler_db.py
src/omnibase_infra/handlers/handler_db.py

**/model_*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/model_*.py: File naming convention: Models must use model_<name>.py with class name Model<Name>
Each model file must contain exactly one Model* class
Pydantic workaround for Any type must include # NOTE: comment documenting the reason when technically required
Use SerializeAsAny type wrapper for Pydantic fields containing complex nested models to preserve subclass fields during serialization

Files:

src/omnibase_infra/handlers/models/model_db_describe_response.py

**/handler_*.py

📄 CodeRabbit inference engine (CLAUDE.md)

Handlers must NOT have direct event bus access - only orchestrators may have bus parameters and publish events

Files:

src/omnibase_infra/handlers/handler_db.py

🧠 Learnings (6)

📓 Common learnings

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/metadata_stamping/database/**/*.py : Database layer MUST use connection pooling (10-50 connections), prepared statements, and circuit breaker pattern for resilience. Monitor pool exhaustion at >90% utilization.

📚 Learning: 2025-11-30T21:55:10.298Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/metadata_stamping/database/**/*.py : Database layer MUST use connection pooling (10-50 connections), prepared statements, and circuit breaker pattern for resilience. Monitor pool exhaustion at >90% utilization.

Applied to files:

src/omnibase_infra/handlers/handler_db.py

📚 Learning: 2025-11-30T21:55:10.298Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/nodes/**/*.py : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented

Applied to files:

src/omnibase_infra/handlers/handler_db.py

📚 Learning: 2025-11-24T16:32:55.606Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Node communication must use event-driven patterns through `ModelEventEnvelope` from `omnibase_core.models.events.model_event_envelope`

Applied to files:

src/omnibase_infra/handlers/handler_db.py

📚 Learning: 2025-12-06T22:21:32.649Z

Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern

Applied to files:

src/omnibase_infra/handlers/handler_db.py

📚 Learning: 2025-11-30T21:55:10.298Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/metadata_stamping/database/**/*.py : All input validation MUST prevent SQL injection using prepared statements and parameterized queries. Use asyncpg for PostgreSQL operations.

Applied to files:

src/omnibase_infra/handlers/handler_db.py

🧬 Code graph analysis (3)

src/omnibase_infra/handlers/models/model_db_describe_response.py (2)

src/omnibase_infra/event_bus/kafka_event_bus.py (1)

default (415-431)

src/omnibase_infra/event_bus/models/config/model_kafka_event_bus_config.py (1)

default (567-592)

tests/unit/handlers/test_handler_db.py (2)

src/omnibase_infra/handlers/handler_db.py (2)

_is_transient_error (537-635)

shutdown (377-389)

src/omnibase_infra/errors/error_infra.py (2)

InfraConnectionError (232-339)

RuntimeHostError (82-151)

src/omnibase_infra/handlers/handler_db.py (1)

src/omnibase_infra/mixins/mixin_async_circuit_breaker.py (5)

_init_circuit_breaker (187-269)

_reset_circuit_breaker (506-581)

_check_circuit_breaker (315-412)

_record_circuit_failure (414-504)

_get_circuit_breaker_state (583-652)

🔇 Additional comments (4)

src/omnibase_infra/handlers/handler_db.py (3)

97-97: Circuit breaker mixin + docs look good.

Clear inheritance and docstring updates make the CB behavior easy to discover.

Also applies to: 162-208

316-324: CB lifecycle wiring looks solid.

Initializing after pool creation and resetting during shutdown keeps breaker state aligned with handler lifecycle.

Also applies to: 379-384

663-744: Check → execute → reset/record flow is consistent.

The pre-check and post-success reset plus transient failure recording are applied uniformly to both query and execute paths.

Also applies to: 772-852

tests/unit/handlers/test_handler_db.py (1)

1468-1907: Nice coverage for transient/permanent classification and CB behavior.

The new suites exercise SQLSTATE class handling and circuit-breaker effects, and the exports stay in sync.

Also applies to: 1911-1925

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

Resolve merge conflict in handler_db.py by combining: - Circuit breaker failure recording from feature branch - ModelTimeoutErrorContext usage from main branch

…A_MAX_UNIONS [OMN-1181] Merge main branch changes into PR #158: - Resolve 9 merge conflicts across source and test files - Update INFRA_MAX_UNIONS: 81 → 83 (+2 for EnumPolicyType | str validators) - Fix exemption pattern for error_message_type_registry.py (was registry_message_type.py) - Accept main's file renames (event_bus_*, handler_*, registry_*, validator_*, etc.) - Accept main's test refactoring for renamed modules - Add missing imports to registry_policy.py for semver cache methods Key changes from main: - OMN-1305: isinstance union exclusion from threshold - OMN-1358: Type alias replacements - File naming standardization (service_*, handler_*, validator_* prefixes) - MessageTypeRegistryError added to errors module

Security: - Remove error_message from log context to prevent sensitive data exposure (SQL fragments, data values could contain PII) Bug fixes: - Add explicit UniqueViolationError handlers in _execute_query and _execute_statement methods (was falling through to generic handler) Type improvements: - Use JsonType for circuit_breaker field in ModelDbDescribeResponse instead of dict[str, object] per ONEX typing conventions

claude · 2026-01-16T18:01:38Z

Pull Request Review: Circuit Breaker Pattern for HandlerDb

Summary

This PR implements circuit breaker resilience for the database handler following established ONEX patterns. The implementation correctly uses MixinAsyncCircuitBreaker with intelligent error classification to distinguish infrastructure failures from application bugs.

✅ Strengths

1. Excellent Error Classification Design

The intelligent error classification based on PostgreSQL SQLSTATE codes is well-designed:

Transient errors (Classes 08, 53, 57, 58) correctly trip the circuit breaker
Permanent errors (Classes 22, 23, 28, 42) do NOT affect circuit state
Conservative fallback (unknown errors default to permanent) prevents false circuit trips

The _is_transient_error() method implementation is clear and well-documented with inline comments explaining each SQLSTATE class.

2. Comprehensive Test Coverage

The test suite is exemplary with 445 new lines covering:

SQLSTATE class-based classification (Classes 08, 22, 23, 28, 42, 53, 57, 58)
Specific exception types (FK violations, NOT NULL, syntax errors, etc.)
Edge cases (missing sqlstate, unknown classes)
Circuit breaker integration (verifying failure counts increment correctly)

3. Proper Mixin Integration

The circuit breaker initialization follows ONEX patterns correctly:

Circuit breaker initialized AFTER pool creation succeeds (line 318-323)
Proper locking with async with self._circuit_breaker_lock:
Circuit breaker reset on shutdown (lines 380-384)
State exposed in describe() for observability (line 995)

4. Documentation Quality

Clear inline documentation:

SQLSTATE classes explained with official PostgreSQL doc link (lines 139-158)
Circuit breaker states documented in class docstring (lines 198-208)
Security notes preserved (DSN handling, production safety)

🔍 Issues & Concerns

1. Critical: Potential Race Condition in Circuit Reset ⚠️

Location: handler_db.py:665-677 and 785-797

The circuit breaker reset happens AFTER releasing the connection, which could cause a race condition:

# Current implementation
async with self._pool.acquire() as conn:
    rows = await conn.fetch(sql, *parameters)

# Reset AFTER connection released
if self._circuit_breaker_initialized:
    async with self._circuit_breaker_lock:
        await self._reset_circuit_breaker()

return self._build_response(...)

Problem: If another thread checks the circuit breaker state between connection release and reset, it might see stale failure counts.

Recommendation: Reset the circuit breaker IMMEDIATELY after successful operation, before releasing the connection context:

async with self._pool.acquire() as conn:
    rows = await conn.fetch(sql, *parameters)
    
    # Reset BEFORE leaving connection context
    if self._circuit_breaker_initialized:
        async with self._circuit_breaker_lock:
            await self._reset_circuit_breaker()

return self._build_response([dict(row) for row in rows], ...)

This ensures atomicity between success detection and circuit reset.

2. Design Question: Transaction Rollback (Class 40) Classification

Location: handler_db.py:153-154

The comment states Class 40 (Transaction Rollback - deadlocks, serialization failures) is classified as permanent because it "indicates application-level retry needed, not infrastructure issue."

Consideration: Deadlocks and serialization failures are often transient in nature:

Database is healthy but concurrent transactions conflicted
Retry at application level typically succeeds
High deadlock rates could indicate database contention (infrastructure-adjacent)

Questions:

Should Class 40 errors trip the circuit breaker after a threshold (e.g., sustained deadlock storm)?
Is the current classification intentional to force application-level handling?

This may be working as designed, but worth documenting the rationale more explicitly if Class 40 should remain permanent.

3. Minor: Duplicate Error Handling Code

Location: Multiple catch blocks in _execute_query and _execute_statement

The same error handling logic is duplicated across both methods:

Lines 688-724 (_execute_query)
Lines 806-858 (_execute_statement)

Recommendation: Consider extracting to a shared method:

async def _handle_postgres_error(
    self,
    error: Exception,
    operation: str,
    ctx: ModelInfraErrorContext,
    correlation_id: UUID,
) -> None:
    """Handle PostgreSQL error with circuit breaker logic."""
    # ... shared error handling logic

This reduces duplication and ensures consistent error handling. However, this is a minor refactoring suggestion and not blocking.

4. Security: Circuit Breaker State Exposure

Location: handler_db.py:995 and model_db_describe_response.py:76-79

The describe() method now exposes circuit breaker state including failure counts:

circuit_breaker: dict[str, JsonType] | None = Field(
    default=None,
    description="Circuit breaker state information (state, failures, threshold, etc.)",
)

Consideration: Does exposing failure counts reveal information that could be exploited?

An attacker could observe failure counts approaching threshold
Could be used to time attacks when circuit is about to open

Recommendation: Document whether describe() is exposed via external APIs or restricted to internal monitoring. If exposed externally, consider sanitizing failure count details.

📊 Code Quality Assessment

Aspect	Rating	Notes
Adherence to ONEX Patterns	✅ Excellent	Follows CLAUDE.md patterns precisely
Test Coverage	✅ Excellent	445 lines of comprehensive tests
Type Safety	✅ Excellent	No `Any` types, proper type annotations
Error Handling	✅ Good	Intelligent classification, minor duplication
Documentation	✅ Excellent	Clear inline comments and docstrings
Concurrency Safety	⚠️ Needs Review	Potential race condition in circuit reset
Performance	✅ Good	Minimal overhead, SQLSTATE lookup is O(1)

🔐 Security Review

Strengths:

DSN sanitization maintained (never logged/exposed)
No SQL injection risks introduced
Proper error context without credential leakage

Concerns:

Circuit breaker state exposure (see Issue feat: Complete infrastructure containers operational with Docker secrets #4 above)
Consider rate limiting on describe() if externally accessible

🚀 Performance Considerations

Strengths:

SQLSTATE classification uses frozenset lookups (O(1))
Circuit breaker check is fast (async lock + state check)
No blocking operations in error handling path

Potential Impact:

Extra lock acquisition per query (~microseconds overhead)
Negligible compared to database I/O time

📋 Recommended Actions

Before Merge:

[REQUIRED] Fix race condition in circuit breaker reset (Issue feat: PostgreSQL Adapter with Comprehensive Tests and Structured Logging #1)
[REQUIRED] Clarify/document Class 40 (Transaction Rollback) classification rationale (Issue Add Claude Code GitHub Workflow #2)
[RECOMMENDED] Document security posture of describe() endpoint (Issue feat: Complete infrastructure containers operational with Docker secrets #4)

Optional Refactoring (Post-Merge):

Extract duplicate error handling to shared method (Issue feat: RedPanda Event Bus Integration with Fail-Fast Infrastructure #3)

Verdict

Status: ✅ Approve with Minor Changes

This is a well-crafted implementation that follows ONEX patterns correctly. The intelligent error classification is particularly strong. The race condition in circuit reset should be addressed before merge, but the fix is straightforward and low-risk.

Estimated Risk: Low (once race condition fixed)
Confidence Level: High

References

CLAUDE.md Circuit Breaker Pattern ✅
CLAUDE.md Error Recovery Patterns ✅
docs/patterns/circuit_breaker_implementation.md ✅
docs/patterns/mixin_dependencies.md ✅

Great work on this implementation! 🎉

- Fix race condition in circuit breaker reset by moving it inside connection context (prevents stale failure counts between coroutines) - Expand Class 40 (Transaction Rollback) documentation with design rationale explaining why deadlocks are classified as permanent - Add security documentation for circuit breaker state exposure in describe() method with recommendations for external API exposure

claude · 2026-01-16T18:38:37Z

Pull Request Review - Circuit Breaker Pattern for DbHandler

Overall Assessment

This PR implements a well-designed circuit breaker pattern for HandlerDb that follows ONEX conventions and demonstrates thoughtful error classification. The implementation is production-ready with comprehensive test coverage.

Strengths

1. Excellent Error Classification Design

SQLSTATE-based classification intelligently distinguishes transient vs permanent errors
Transient errors (Classes 08, 53, 57, 58) correctly trip the circuit
Permanent errors (Classes 22, 23, 28, 42) appropriately bypass circuit breaker
Deadlock handling (Class 40) classified as permanent with detailed rationale

2. Comprehensive Documentation

Inline comments explain _is_transient_error() logic clearly
SQLSTATE classes documented with PostgreSQL docs references
Security considerations for circuit breaker state exposure documented

3. Race Condition Fix

Circuit breaker reset correctly performed inside connection context
Prevents stale failure counts between coroutines

4. Extensive Test Coverage

445 lines of new tests
Covers all SQLSTATE classes and edge cases
Comprehensive circuit breaker integration scenarios

5. Security Best Practices

Removed error_message from log context to prevent PII exposure
Maintains DSN sanitization patterns
Documents security considerations for circuit breaker state

6. Follows ONEX Conventions

Uses MixinAsyncCircuitBreaker correctly
Proper JsonType usage
Explicit error handlers for all constraint violations

Minor Observations

1. Circuit Breaker Threshold Configuration

Currently hardcoded (threshold=5, reset_timeout=30.0)
Consider making configurable via env vars in future iteration

2. Error Message Repetition

Duplicated error handling in _execute_query() and _execute_statement()
Acceptable for MVP, could refactor later

3. Unknown SQLSTATE Classes

Defaults to permanent (conservative approach)
Consider adding metrics for unknown classes in production

4. Circuit Breaker State Exposure

describe() exposes failure counts
Consider sanitized parameter for external APIs

Performance Considerations

Lock Contention

Circuit breaker acquires lock on every query
Acceptable for MVP (pool size 5)
Lock held briefly (state checks only, not I/O)
Future: Consider lock-free atomic counters for 10k+ QPS

Circuit Reset Pattern

Resets on every success (standard pattern)
Two lock acquisitions per query when healthy
Alternative: Only reset in HALF_OPEN state (future optimization)

Code Quality

Type Safety: No Any types, proper JsonType usage
Error Handling: Explicit catches, intelligent classification
Testing: Comprehensive SQLSTATE coverage, edge cases tested

Recommendations

Must Address Before Merge: None - ready to merge

Consider for Future PRs:

Configurable circuit breaker thresholds via env vars
Metrics for unknown SQLSTATE classes
Sanitized describe() for public APIs
Lock-free optimization if profiling shows contention

Security Review

No sensitive data in error messages
DSN credentials never exposed
PII exposure prevented in logging
Circuit breaker state exposure documented
No new attack surface

Compliance with CLAUDE.md

Strong typing (no Any)
Container injection via MixinAsyncCircuitBreaker
Proper error hierarchy usage
Circuit breaker pattern correctly implemented
Error sanitization applied

Final Verdict

APPROVE - Merge Recommended

Excellent implementation demonstrating:

Deep understanding of circuit breaker patterns
Thoughtful PostgreSQL error classification
Strong documentation and test coverage
Attention to concurrency safety and security

Production-ready for MVP workloads. Minor optimization opportunities exist but are not blockers.

Great work on the deadlock classification rationale!

Checklist

Code quality: Excellent
Bugs: None identified
Performance: Acceptable for MVP
Security: All concerns addressed
Test coverage: Comprehensive (445 lines)

Reviewed following ONEX conventions per CLAUDE.md

coderabbitai

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@src/omnibase_infra/handlers/handler_db.py`:
- Around line 858-872: Replace the constraint-violation error messages that
interpolate sensitive database values from the asyncpg exceptions in the except
handlers (asyncpg.ForeignKeyViolationError, asyncpg.NotNullViolationError,
asyncpg.UniqueViolationError) by removing e.message from the RuntimeHostError
message strings in handler_db.py; instead use a generic, non-sensitive message
like "Foreign key constraint violation", "Not null constraint violation",
"Unique constraint violation" (preserving context=ctx and raising from e) so the
handlers (RuntimeHostError) still indicate the error type without exposing
sensitive data.
- Around line 738-752: The exception handlers for
asyncpg.ForeignKeyViolationError, asyncpg.NotNullViolationError, and
asyncpg.UniqueViolationError currently include e.message which may leak
sensitive values; update each RuntimeHostError raised in these blocks (the
handlers referencing RuntimeHostError in handler_db.py) to use only the safe
error prefix (e.g., the corresponding entry from _POSTGRES_ERROR_PREFIXES or a
fixed descriptive string like "Foreign key constraint violation") instead of
including e.message, matching how the generic PostgresError handler is
sanitized.
- Around line 139-173: The _PERMANENT_SQLSTATE_CLASSES set is missing the "40"
transaction-rollback class referenced in the design notes; update the
_PERMANENT_SQLSTATE_CLASSES frozenset (symbol name: _PERMANENT_SQLSTATE_CLASSES)
to include "40" so it becomes {"22","23","28","40","42"}, ensuring Class 40
errors are treated as permanent (and hit the DEBUG path rather than the
unknown-class WARNING).

🧹 Nitpick comments (1)

src/omnibase_infra/handlers/handler_db.py (1)

770-888: Consider extracting shared error handling logic.

_execute_query and _execute_statement have nearly identical circuit breaker integration and exception handling (~80 lines duplicated). Consider extracting a shared helper or using a decorator pattern to reduce maintenance burden.

This is optional for this PR but would improve maintainability for future changes.

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Lite

📥 Commits

Reviewing files that changed from the base of the PR and between 6c02070 and d55c04d.

📒 Files selected for processing (2)

src/omnibase_infra/handlers/handler_db.py
src/omnibase_infra/handlers/models/model_db_describe_response.py

🚧 Files skipped from review as they are similar to previous changes (1)

src/omnibase_infra/handlers/models/model_db_describe_response.py

🧰 Additional context used

📓 Path-based instructions (2)

**/*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*.py: ALL coding tasks MUST use sub-agents (agent-commit, agent-testing, agent-contract-validator, agent-onex-coordinator, agent-workflow-coordinator) - NO direct coding allowed
NEVER use run_in_background: true for Task tool - use single message with multiple Task tool calls for parallel execution
ALL changes are breaking changes with NO backwards compatibility - remove old patterns immediately, do not leave deprecated code, and do not provide migration guides
NEVER create versioned directories like v1_0_0/ or v2/ - version through contract.yaml fields only
NEVER use Any type in function parameters, return types, type aliases, or Pydantic Field() annotations without # NOTE: comment and documented justification
Use object instead of Any for generic payloads in function parameters and return types
Pydantic models: one model per file, use PEP 604 unions (X | None not Optional[X]), all data structures must be proper Pydantic models
Use nullable type syntax X | None (PEP 604) instead of Optional[X] for all type annotations
Use ModelEventEnvelope[object] for generic event handling in dispatchers and handlers - do not use Any
Import and use JsonType from omnibase_core.types (or omnibase_infra.models.types re-export) for generic JSON-compatible values
File naming: adapter_.py, dispatcher_.py, error/ directory, model_.py, node.py, plugin_.py, service_.py, etc. per naming table
Class naming: Adapter, Dispatcher, Error, Model, Node, Plugin, Service, etc. per naming table
Standalone registries should be named registry_<purpose>.py with class name Registry<Purpose>
Result models may override __bool__() to enable idiomatic conditional checks - always document in docstring Warning section
ALL services MUST use ModelONEXContainer for dependency injection - pass container to init and call super().init(container)
Import node archetypes from omnib...

Files:

src/omnibase_infra/handlers/handler_db.py

**/*handler*.py

📄 CodeRabbit inference engine (CLAUDE.md)

**/*handler*.py: Handlers MUST NOT have direct event bus access - no _bus, _event_bus, or _publisher attributes, no publish() methods, protocol compliance with ProtocolHandler
Handlers using node introspection (MixinNodeIntrospection) must prefix internal/sensitive methods with _ and use generic parameter names

Files:

src/omnibase_infra/handlers/handler_db.py

🧠 Learnings (7)

📓 Common learnings

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/metadata_stamping/database/**/*.py : Database layer MUST use connection pooling (10-50 connections), prepared statements, and circuit breaker pattern for resilience. Monitor pool exhaustion at >90% utilization.

📚 Learning: 2025-11-30T21:55:10.298Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/metadata_stamping/database/**/*.py : Database layer MUST use connection pooling (10-50 connections), prepared statements, and circuit breaker pattern for resilience. Monitor pool exhaustion at >90% utilization.

Applied to files:

src/omnibase_infra/handlers/handler_db.py

📚 Learning: 2026-01-16T17:34:10.902Z

Learnt from: CR
Repo: OmniNode-ai/omnibase_infra PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-16T17:34:10.902Z
Learning: Applies to **/*adapter*.py : External service adapters MUST implement MixinAsyncCircuitBreaker for resilience - use _init_circuit_breaker() with threshold, reset_timeout, service_name, transport_type

Applied to files:

src/omnibase_infra/handlers/handler_db.py

📚 Learning: 2025-11-30T21:55:10.298Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/metadata_stamping/database/**/*.py : All input validation MUST prevent SQL injection using prepared statements and parameterized queries. Use asyncpg for PostgreSQL operations.

Applied to files:

src/omnibase_infra/handlers/handler_db.py

📚 Learning: 2025-11-30T21:55:10.298Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-30T21:55:10.298Z
Learning: Applies to src/omninode_bridge/nodes/**/*.py : Import mixins from omnibase_core.mixins.* and use Mixin* naming pattern (e.g., MixinHealthCheck, MixinMetrics, MixinEventBus) - never use local custom mixins unless experimental and documented

Applied to files:

src/omnibase_infra/handlers/handler_db.py

📚 Learning: 2025-11-24T16:32:55.606Z

Learnt from: CR
Repo: OmniNode-ai/omninode_bridge PR: 0
File: .cursor/rules/node_standards.mdc:0-0
Timestamp: 2025-11-24T16:32:55.606Z
Learning: Node communication must use event-driven patterns through `ModelEventEnvelope` from `omnibase_core.models.events.model_event_envelope`

Applied to files:

src/omnibase_infra/handlers/handler_db.py

📚 Learning: 2025-12-06T22:21:32.649Z

Learnt from: CR
Repo: OmniNode-ai/omniagent PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-06T22:21:32.649Z
Learning: Applies to nodes/**/*.py : Use `omnibase_infra` handlers for OmniIntelligence queries via HttpRestAdapter envelope pattern

Applied to files:

src/omnibase_infra/handlers/handler_db.py

🧬 Code graph analysis (1)

src/omnibase_infra/handlers/handler_db.py (1)

src/omnibase_infra/mixins/mixin_async_circuit_breaker.py (5)

MixinAsyncCircuitBreaker (115-652)

_reset_circuit_breaker (506-581)

_check_circuit_breaker (315-412)

_record_circuit_failure (414-504)

_get_circuit_breaker_state (583-652)

🔇 Additional comments (8)

src/omnibase_infra/handlers/handler_db.py (8)

98-98: LGTM!

Import follows the Mixin* naming pattern and sources infrastructure mixins from the appropriate package.

176-222: LGTM!

Class inheritance correctly integrates both mixins, and the circuit breaker documentation clearly describes the state machine and error classification policy. Based on learnings, database handlers should use circuit breaker pattern for resilience.

224-231: LGTM!

The _circuit_breaker_initialized flag correctly tracks lazy initialization state, ensuring circuit breaker methods aren't called before _init_circuit_breaker().

329-337: LGTM!

Circuit breaker initialization follows the mixin contract with appropriate parameters. Initializing after pool creation ensures the circuit breaker only activates when the handler is fully functional. Based on learnings, this follows the required pattern for external service adapters.

551-647: LGTM!

The transient error classification logic is well-designed:

SQLSTATE-based classification with type-based fallback

Conservative default to permanent prevents over-tripping

Logging only includes safe metadata (error_type, sqlstate) without sensitive error messages

This correctly addresses the security concern from past reviews about not exposing sensitive error details.

675-698: LGTM!

The circuit breaker integration follows the correct pattern:

Pre-check before acquiring connection prevents wasting pool resources

Reset inside connection context (per PR notes) avoids race conditions between coroutines

Lock is correctly acquired for both check and reset operations

The brief window between check and execute is acceptable—circuit breakers are heuristic protections, not strict guarantees.

1002-1035: LGTM!

The security consideration documentation is thorough and provides actionable guidance for external API exposure. The circuit breaker state retrieval correctly checks initialization status before accessing mixin methods.

675-681: No issue found. All circuit breaker mixin methods (_check_circuit_breaker, _record_circuit_failure, _reset_circuit_breaker) are properly declared as async def in MixinAsyncCircuitBreaker. The await expressions in handler_db.py are correct.

Likely an incorrect or invalid review comment.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

coderabbitai · 2026-01-16T18:38:56Z

+# PostgreSQL SQLSTATE class codes for error classification
+# See: https://www.postgresql.org/docs/current/errcodes-appendix.html
+#
+# TRANSIENT errors (should trip circuit breaker):
+#   - Class 08: Connection Exception (database unreachable, connection lost)
+#   - Class 53: Insufficient Resources (out of memory, disk full, too many connections)
+#   - Class 57: Operator Intervention (admin shutdown, crash recovery, cannot connect now)
+#   - Class 58: System Error (I/O error, undefined file, duplicate file)
+#
+# PERMANENT errors (should NOT trip circuit breaker):
+#   - Class 23: Integrity Constraint Violation (FK, NOT NULL, unique, check)
+#   - Class 42: Syntax Error or Access Rule Violation (bad SQL, undefined table/column)
+#   - Class 28: Invalid Authorization Specification (bad credentials)
+#   - Class 22: Data Exception (division by zero, string data truncation)
+#   - Class 40: Transaction Rollback (serialization failure, deadlock detected)
+#
+#     DESIGN DECISION: Classified as PERMANENT despite deadlocks being retry-able.
+#
+#     Rationale:
+#     1. Deadlocks indicate transaction contention, not infrastructure failure
+#     2. The database is healthy - it correctly detected and resolved the deadlock
+#     3. Retrying at application level (with backoff) typically succeeds
+#     4. Tripping the circuit would block ALL queries, not just the conflicting ones
+#     5. High deadlock rates indicate application design issues (lock ordering,
+#        transaction scope) that should be fixed in code, not masked by circuit breaker
+#
+#     Note: If sustained deadlock storms occur, this is a symptom of application
+#     issues or schema contention that monitoring/alerting should surface, but
+#     the circuit breaker is not the right mitigation tool.
+#
+# The key insight: transient errors indicate the DATABASE INFRASTRUCTURE is unhealthy,
+# while permanent errors indicate the QUERY/APPLICATION is invalid. The circuit breaker
+# protects against infrastructure failures, not application bugs.
+_TRANSIENT_SQLSTATE_CLASSES: frozenset[str] = frozenset({"08", "53", "57", "58"})
+_PERMANENT_SQLSTATE_CLASSES: frozenset[str] = frozenset({"22", "23", "28", "42"})


⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n src/omnibase_infra/handlers/handler_db.py | sed -n '139,200p'

Repository: OmniNode-ai/omnibase_infra

Length of output: 4148

🏁 Script executed:

# Check the _is_transient_error method to understand unknown class handling rg -n "_is_transient_error" src/omnibase_infra/handlers/handler_db.py -A 15

Repository: OmniNode-ai/omnibase_infra

Length of output: 3748

🏁 Script executed:

# Verify the full structure of SQLSTATE constants and their usage rg -n "_TRANSIENT_SQLSTATE_CLASSES|_PERMANENT_SQLSTATE_CLASSES" src/omnibase_infra/handlers/handler_db.py -B 2 -A 2

Repository: OmniNode-ai/omnibase_infra

Length of output: 891

🏁 Script executed:

cat -n src/omnibase_infra/handlers/handler_db.py | sed -n '587,640p'

Repository: OmniNode-ai/omnibase_infra

Length of output: 2835

Add Class 40 to _PERMANENT_SQLSTATE_CLASSES.

Class 40 (Transaction Rollback / deadlocks) is documented as PERMANENT in the design decision (lines 153-167) but missing from _PERMANENT_SQLSTATE_CLASSES (line 173). This causes Class 40 errors to fall through to the "unknown class" path at line 637, generating unnecessary WARNING logs instead of the intended DEBUG log. Add "40" to the set:

_PERMANENT_SQLSTATE_CLASSES: frozenset[str] = frozenset({"22", "23", "28", "40", "42"})

🤖 Prompt for AI Agents

In `@src/omnibase_infra/handlers/handler_db.py` around lines 139 - 173, The _PERMANENT_SQLSTATE_CLASSES set is missing the "40" transaction-rollback class referenced in the design notes; update the _PERMANENT_SQLSTATE_CLASSES frozenset (symbol name: _PERMANENT_SQLSTATE_CLASSES) to include "40" so it becomes {"22","23","28","40","42"}, ensuring Class 40 errors are treated as permanent (and hit the DEBUG path rather than the unknown-class WARNING).

coderabbitai · 2026-01-16T18:38:57Z

+        except asyncpg.ForeignKeyViolationError as e:
+            # Application error - do NOT trip circuit
+            raise RuntimeHostError(
+                f"Foreign key constraint violation: {e.message}", context=ctx
+            ) from e
+        except asyncpg.NotNullViolationError as e:
+            # Application error - do NOT trip circuit
+            raise RuntimeHostError(
+                f"Not null constraint violation: {e.message}", context=ctx
+            ) from e
+        except asyncpg.UniqueViolationError as e:
+            # Application error - do NOT trip circuit
+            raise RuntimeHostError(
+                f"Unique constraint violation: {e.message}", context=ctx
+            ) from e


⚠️ Potential issue | 🟠 Major

Constraint violation messages may expose sensitive data.

The e.message for constraint violations (FK, NOT NULL, unique) can include the conflicting value, which may contain sensitive user data. For example, a unique violation on an email column would expose the email address.

Consider using the error prefix from _POSTGRES_ERROR_PREFIXES without the message, similar to the generic PostgresError handler at line 767:

🛡️ Suggested change

except asyncpg.ForeignKeyViolationError as e: # Application error - do NOT trip circuit raise RuntimeHostError( - f"Foreign key constraint violation: {e.message}", context=ctx + "Foreign key constraint violation", context=ctx ) from e except asyncpg.NotNullViolationError as e: # Application error - do NOT trip circuit raise RuntimeHostError( - f"Not null constraint violation: {e.message}", context=ctx + "Not null constraint violation", context=ctx ) from e except asyncpg.UniqueViolationError as e: # Application error - do NOT trip circuit raise RuntimeHostError( - f"Unique constraint violation: {e.message}", context=ctx + "Unique constraint violation", context=ctx ) from e

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

except asyncpg.ForeignKeyViolationError as e:

# Application error - do NOT trip circuit

raise RuntimeHostError(

f"Foreign key constraint violation: {e.message}", context=ctx

) from e

except asyncpg.NotNullViolationError as e:

# Application error - do NOT trip circuit

raise RuntimeHostError(

f"Not null constraint violation: {e.message}", context=ctx

) from e

except asyncpg.UniqueViolationError as e:

# Application error - do NOT trip circuit

raise RuntimeHostError(

f"Unique constraint violation: {e.message}", context=ctx

) from e

except asyncpg.ForeignKeyViolationError as e:

# Application error - do NOT trip circuit

raise RuntimeHostError(

"Foreign key constraint violation", context=ctx

) from e

except asyncpg.NotNullViolationError as e:

# Application error - do NOT trip circuit

raise RuntimeHostError(

"Not null constraint violation", context=ctx

) from e

except asyncpg.UniqueViolationError as e:

# Application error - do NOT trip circuit

raise RuntimeHostError(

"Unique constraint violation", context=ctx

) from e

🤖 Prompt for AI Agents

In `@src/omnibase_infra/handlers/handler_db.py` around lines 738 - 752, The exception handlers for asyncpg.ForeignKeyViolationError, asyncpg.NotNullViolationError, and asyncpg.UniqueViolationError currently include e.message which may leak sensitive values; update each RuntimeHostError raised in these blocks (the handlers referencing RuntimeHostError in handler_db.py) to use only the safe error prefix (e.g., the corresponding entry from _POSTGRES_ERROR_PREFIXES or a fixed descriptive string like "Foreign key constraint violation") instead of including e.message, matching how the generic PostgresError handler is sanitized.

coderabbitai · 2026-01-16T18:38:57Z

+        except asyncpg.ForeignKeyViolationError as e:
+            # Application error - do NOT trip circuit
+            raise RuntimeHostError(
+                f"Foreign key constraint violation: {e.message}", context=ctx
+            ) from e
+        except asyncpg.NotNullViolationError as e:
+            # Application error - do NOT trip circuit
+            raise RuntimeHostError(
+                f"Not null constraint violation: {e.message}", context=ctx
+            ) from e
+        except asyncpg.UniqueViolationError as e:
+            # Application error - do NOT trip circuit
+            raise RuntimeHostError(
+                f"Unique constraint violation: {e.message}", context=ctx
+            ) from e


⚠️ Potential issue | 🟠 Major

Same sensitive data concern in constraint violation messages.

Same issue as in _execute_query — constraint violation messages at lines 861, 866, 871 may expose sensitive values. Apply the same fix to remove e.message from these error strings.

🤖 Prompt for AI Agents

In `@src/omnibase_infra/handlers/handler_db.py` around lines 858 - 872, Replace the constraint-violation error messages that interpolate sensitive database values from the asyncpg exceptions in the except handlers (asyncpg.ForeignKeyViolationError, asyncpg.NotNullViolationError, asyncpg.UniqueViolationError) by removing e.message from the RuntimeHostError message strings in handler_db.py; instead use a generic, non-sensitive message like "Foreign key constraint violation", "Not null constraint violation", "Unique constraint violation" (preserving context=ctx and raising from e) so the handlers (RuntimeHostError) still indicate the error type without exposing sensitive data.

coderabbitai Bot reviewed Jan 16, 2026

View reviewed changes

Merge branch 'main' into jonah/omn-780-circuit-breaker-dbhandler

cd60430

Resolve merge conflict in handler_db.py by combining: - Circuit breaker failure recording from feature branch - ModelTimeoutErrorContext usage from main branch

coderabbitai Bot reviewed Jan 16, 2026

View reviewed changes

jonahgabriel merged commit 6615596 into main Jan 16, 2026
10 checks passed

jonahgabriel deleted the jonah/omn-780-circuit-breaker-dbhandler branch January 16, 2026 19:03

coderabbitai Bot mentioned this pull request Mar 5, 2026

feat(runtime): per-handler shutdown timeouts (OMN-882) #613

Merged

5 of 6 tasks

Conversation

jonahgabriel commented Jan 16, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Error Handling Design

Test plan

Linear

Summary by CodeRabbit

Uh oh!

linear Bot commented Jan 16, 2026

Uh oh!

coderabbitai Bot commented Jan 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram

Estimated code review effort

Poem

Uh oh!

claude Bot commented Jan 16, 2026

Code Review - PR #159: Circuit Breaker Pattern for HandlerDb

Summary

✅ Strengths

1. Correct Circuit Breaker Integration

2. Smart Error Classification

3. Proper Lock Usage

4. Clean Shutdown Handling

5. Good Observability

⚠️ Issues Found

1. CRITICAL: Missing Circuit Check in _execute_statement ❌

2. Missing Success Reset in _execute_statement ⚠️

3. MINOR: Duplicate Comments 📝

4. MINOR: Docstring Could Be More Specific 📖

🧪 Test Coverage

✅ What's Covered

⚠️ What's Missing

🔒 Security

✅ No Issues

📊 Performance

✅ Minimal Overhead

⚠️ Potential Concern

📝 Documentation

✅ Good

⚠️ Could Improve

🎯 Overall Assessment

✅ Approval Recommendation

Required Changes:

Recommended Changes (not blocking):

🚀 Next Steps

Uh oh!

claude Bot commented Jan 16, 2026

PR Review: Circuit Breaker Implementation for HandlerDb

Overview

✅ Strengths

1. Correct Circuit Breaker Pattern Usage

2. Intelligent Error Classification

3. Proper State Management

4. Clean Integration

🔍 Issues & Concerns

CRITICAL: Missing Test Coverage

MEDIUM: Lock Acquisition Ordering

MEDIUM: Shutdown Race Condition

LOW: Generic PostgresError Handling

LOW: Configuration Not Exposed

LOW: Removed Error Mapping

📋 Documentation & Style

✅ Good:

⚠️ Suggestions:

🔒 Security

🎯 Performance

📊 Compliance with CLAUDE.md

🚦 Verdict

Blockers (must fix before merge):

Strongly Recommended (high value, low effort):

Nice-to-Have (can be follow-up PRs):

🎉 Overall Assessment

Uh oh!

claude Bot commented Jan 16, 2026

PR Review: Circuit Breaker Pattern for HandlerDb

jonahgabriel commented Jan 16, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jan 16, 2026 •

edited

Loading

1. CRITICAL: Missing Circuit Check in `_execute_statement` ❌

2. Missing Success Reset in `_execute_statement` ⚠️

2. Missing `UniqueViolationError` Handler (Minor)