test(effect): add comprehensive effect idempotency and retry behavior tests [OMN-954] by jonahgabriel · Pull Request #78 · OmniNode-ai/omnibase_infra

jonahgabriel · 2025-12-22T00:41:45Z

Summary

Implements G4 acceptance criteria tests for effect idempotency, circuit breaker, backoff policy, retry exhaustion, and partial failure scenarios.

✅ Duplicate intent safe
✅ Circuit breaker verified
✅ Backoff policy validated
✅ Retry exhaustion tested
✅ Partial failure scenarios tested

Test Coverage (51 tests)

Test File	Coverage
`test_effect_idempotency.py`	Duplicate intent safety, natural key deduplication, domain isolation
`test_effect_circuit_breaker.py`	State transitions (CLOSED→OPEN→HALF_OPEN→CLOSED), per-backend isolation
`test_effect_retry_backoff.py`	Exponential backoff timing, retry exhaustion, non-retryable errors
`test_effect_partial_failure.py`	Consul/Postgres partial failure combinations, error aggregation

New Components

Models (src/omnibase_infra/nodes/effects/models/):

ModelBackendResult - Individual backend operation outcome
ModelRegistryRequest - Dual-backend registration request
ModelRegistryResponse - Response with partial failure support (status: success | partial | failed)

Effect Node (src/omnibase_infra/nodes/effects/):

RegistryEffect - Effect node for dual-backend registration execution
ProtocolConsulClient - Protocol for Consul service registration
ProtocolPostgresAdapter - Protocol for PostgreSQL registration persistence

Test plan

All 51 new tests pass locally
Pre-commit hooks pass (architecture, patterns, linting)
Type checking passes (mypy)

Linear

Closes OMN-954

Summary by CodeRabbit

Release Notes

New Features
- Introduced Registry Effect Node for dual-backend node registration against Consul and PostgreSQL with partial failure handling and targeted retries.
- Added idempotency tracking with configurable in-memory cache featuring LRU eviction and TTL-based expiration.
Documentation
- Added comprehensive Registry Effect Node documentation including architecture, performance characteristics, configuration options, and usage examples.
Tests
- Added integration tests validating end-to-end registration flows, partial failures, and idempotency behavior.
- Added unit tests for effect operations, circuit breaker integration, retry logic, and idempotency store behavior.
- Added performance tests for high-volume registration, concurrent load, cache stress, and latency distribution.

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

… tests [OMN-954] Implement G4 acceptance criteria tests for effect idempotency, circuit breaker, backoff policy, retry exhaustion, and partial failure scenarios. Test Coverage (51 tests): - test_effect_idempotency.py: Duplicate intent safety, natural key deduplication - test_effect_circuit_breaker.py: State transitions, per-backend isolation - test_effect_retry_backoff.py: Exponential backoff, retry exhaustion - test_effect_partial_failure.py: Consul/Postgres partial failure handling New Models: - ModelBackendResult: Individual backend operation outcome - ModelRegistryRequest: Dual-backend registration request - ModelRegistryResponse: Response with partial failure support New Effect: - RegistryEffect: Effect node for dual-backend registration execution - ProtocolConsulClient: Protocol for Consul service registration - ProtocolPostgresAdapter: Protocol for PostgreSQL registration persistence

linear · 2025-12-22T00:41:47Z

OMN-954

coderabbitai · 2025-12-22T00:41:50Z

Warning

Rate limit exceeded

@jonahgabriel has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 5 minutes and 30 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 74152c0 and ba08620.

📒 Files selected for processing (17)

src/omnibase_infra/nodes/effects/contract.yaml
src/omnibase_infra/nodes/effects/models/__init__.py
src/omnibase_infra/nodes/effects/models/model_registry_request.py
src/omnibase_infra/nodes/effects/protocol_effect_idempotency_store.py
src/omnibase_infra/nodes/effects/registry_effect.py
src/omnibase_infra/nodes/effects/store_effect_idempotency_inmemory.py
tests/integration/registration/effect/conftest.py
tests/integration/registration/effect/test_doubles.py
tests/integration/registration/effect/test_registry_effect_integration.py
tests/performance/registration/effect/conftest.py
tests/performance/registration/effect/test_idempotency_store_performance.py
tests/unit/nodes/effects/__init__.py
tests/unit/nodes/effects/test_sanitize_backend_error.py
tests/unit/registration/effect/conftest.py
tests/unit/registration/effect/test_effect_circuit_breaker.py
tests/unit/registration/effect/test_effect_idempotency.py
tests/unit/registration/effect/test_effect_partial_failure.py

Walkthrough

Introduces the NodeRegistryEffect, an async effect node coordinating dual-backend (Consul and PostgreSQL) node registration. Features idempotency tracking via pluggable stores, partial failure handling, and targeted retries. Includes comprehensive data models, protocol interfaces, core implementation, and extensive test coverage.

Changes

Cohort / File(s)	Summary
Core Data Models `src/omnibase_infra/nodes/effects/models/model_backend_result.py` `src/omnibase_infra/nodes/effects/models/model_effect_idempotency_config.py` `src/omnibase_infra/nodes/effects/models/model_registry_request.py` `src/omnibase_infra/nodes/effects/models/model_registry_response.py`	New Pydantic models for effect I/O: ModelBackendResult (operation outcomes), ModelEffectIdempotencyConfig (cache/TTL settings), ModelRegistryRequest (registration input), ModelRegistryResponse (with status, per-backend results, and helper methods).
Protocol Interfaces `src/omnibase_infra/nodes/effects/protocol_consul_client.py` `src/omnibase_infra/nodes/effects/protocol_postgres_adapter.py` `src/omnibase_infra/nodes/effects/protocol_effect_idempotency_store.py`	New protocol abstractions for Consul client (register_service), PostgreSQL adapter (upsert), and idempotency store (mark_completed, is_completed, get_completed_backends, clear operations).
Core Implementation `src/omnibase_infra/nodes/effects/registry_effect.py` `src/omnibase_infra/nodes/effects/store_effect_idempotency_inmemory.py`	NodeRegistryEffect async node coordinating dual-backend registration with idempotency checks and error handling; InMemoryEffectIdempotencyStore with LRU eviction, TTL expiration, and async-safe operations.
Public API & Configuration `src/omnibase_infra/nodes/effects/__init__.py` `src/omnibase_infra/nodes/effects/models/__init__.py` `src/omnibase_infra/nodes/__init__.py` `src/omnibase_infra/nodes/effects/contract.yaml`	Package initializers exporting models and node; contract YAML defining NodeRegistryEffect version and type (EFFECT_GENERIC).
Documentation `src/omnibase_infra/nodes/effects/README.md`	Comprehensive documentation of Registry Effect Node: architecture, memory characteristics, performance, configuration, usage examples, thread-safety, and persistent backend recommendations.
Integration Test Infrastructure `tests/integration/registration/__init__.py` `tests/integration/registration/effect/__init__.py` `tests/integration/registration/effect/conftest.py` `tests/integration/registration/effect/test_doubles.py`	Integration test package with conftest providing fresh fixtures (consul_client, postgres_adapter, idempotency_store, registry_effect, sample_request, request_factory); test doubles (StubConsulClient, StubPostgresAdapter) with configurable failure modes and call tracking.
Integration Tests `tests/integration/registration/effect/test_registry_effect_integration.py`	Comprehensive end-to-end test suite: full success, partial failures (Consul/PostgreSQL), complete failure, exception handling, retry success after partial failure, idempotency verification, async behavior, and concurrent registration validation.
Unit Test Infrastructure `tests/unit/registration/__init__.py` `tests/unit/registration/effect/__init__.py` `tests/unit/registration/effect/conftest.py`	Unit test package with fixtures for idempotency store, mocked Consul/PostgreSQL backends, sample models, and factory helpers.
Unit Tests: Behavior & Correctness `tests/unit/registration/effect/test_effect_circuit_breaker.py` `tests/unit/registration/effect/test_effect_idempotency.py` `tests/unit/registration/effect/test_effect_idempotency_store.py` `tests/unit/registration/effect/test_effect_partial_failure.py` `tests/unit/registration/effect/test_effect_retry_backoff.py`	Circuit breaker lifecycle, idempotency logic with natural keys, cache operations (LRU/TTL), partial failure scenarios and error aggregation, and retry backoff with exponential delays.
Performance Tests `tests/performance/__init__.py` `tests/performance/registration/__init__.py` `tests/performance/registration/effect/__init__.py` `tests/performance/registration/effect/conftest.py`	Performance test package infrastructure with fixtures for default/small/large cache stores, mock clients, and sample model factories.
Performance Tests: Benchmarks `tests/performance/registration/effect/test_effect_performance.py` `tests/performance/registration/effect/test_idempotency_store_performance.py`	High-volume sequential, concurrent load (100–500 ops), LRU eviction stress, memory bounds, latency distribution (p50/p95/p99), and throughput tests; idempotency store LRU/TTL/concurrency and mixed workload benchmarks with performance metrics.

Sequence Diagram

sequenceDiagram
    participant Client
    participant NodeRegistryEffect
    participant IdempotencyStore
    participant ConsulClient
    participant PostgresAdapter
    participant Response

    Client->>NodeRegistryEffect: register_node(ModelRegistryRequest)
    
    Note over NodeRegistryEffect: Extract correlation_id
    
    NodeRegistryEffect->>IdempotencyStore: is_completed(correlation_id, "consul")
    alt Consul already completed
        IdempotencyStore-->>NodeRegistryEffect: true
        Note over NodeRegistryEffect: Skip Consul
    else Consul not completed
        IdempotencyStore-->>NodeRegistryEffect: false
        NodeRegistryEffect->>ConsulClient: register_service(...)
        Note over ConsulClient: Execute registration
        ConsulClient-->>NodeRegistryEffect: ModelBackendResult (success/fail)
        alt Consul succeeded
            NodeRegistryEffect->>IdempotencyStore: mark_completed(correlation_id, "consul")
        end
    end
    
    NodeRegistryEffect->>IdempotencyStore: is_completed(correlation_id, "postgres")
    alt PostgreSQL already completed
        IdempotencyStore-->>NodeRegistryEffect: true
        Note over NodeRegistryEffect: Skip PostgreSQL
    else PostgreSQL not completed
        IdempotencyStore-->>NodeRegistryEffect: false
        NodeRegistryEffect->>PostgresAdapter: upsert(...)
        Note over PostgresAdapter: Execute registration
        PostgresAdapter-->>NodeRegistryEffect: ModelBackendResult (success/fail)
        alt PostgreSQL succeeded
            NodeRegistryEffect->>IdempotencyStore: mark_completed(correlation_id, "postgres")
        end
    end
    
    Note over NodeRegistryEffect: Determine overall status<br/>(success / partial / failed)
    
    NodeRegistryEffect->>Response: ModelRegistryResponse<br/>(consul_result, postgres_result,<br/>status, error_summary)
    Response-->>Client: Return response

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

NodeRegistryEffect logic (registry_effect.py): Async dual-backend coordination with exception handling, idempotency checks, and status determination requires careful verification of control flow and error scenarios.
InMemoryEffectIdempotencyStore (store_effect_idempotency_inmemory.py): LRU eviction logic, TTL-based expiration with time handling, and concurrent async access with locking patterns need thorough scrutiny.
ModelRegistryResponse helpers (model_registry_response.py): Status derivation logic from backend results and aggregation of errors must be validated.
Test coverage breadth: Extensive test files across integration, unit, and performance suites with complex mock orchestration and concurrent scenarios; particular attention to retry logic tests, partial failure edge cases, and idempotency verification.
Protocol consistency: Verify that all protocol implementations (stubs and mocks in tests) correctly match their abstract contracts.

Poem

🐰 A node hops 'tween two distant lands,
Consul and SQL, holding steady hands—
When one path stumbles, retry takes its stand,
Idempotent and certain, perfectly planned! 🎯

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

claude · 2025-12-22T00:43:53Z

PR Review: Effect Idempotency and Retry Behavior Tests

Overall Assessment

This PR implements comprehensive test coverage for G4 acceptance criteria (effect idempotency, circuit breaker, backoff, retry exhaustion, and partial failure scenarios). The implementation is well-structured with strong adherence to ONEX patterns, but there are several critical architectural issues that must be addressed.

🚨 Critical Issues

1. ONEX Naming Convention Violation - Node Class

Issue: RegistryEffect violates ONEX node naming convention

Location: src/omnibase_infra/nodes/effects/registry_effect.py:64

CLAUDE.md Rule:

Node | node.py | Node<Name><Type> | NodePostgresAdapterEffect

Current:

class RegistryEffect:  # WRONG

Required:

class NodeRegistryEffect:  # CORRECT

Impact: This breaks ONEX architectural consistency. All node classes MUST follow the Node<Name><Type> pattern to enable:

Automated discovery and introspection
Type-safe node resolution
Contract validation
Registry patterns

Fix: Rename RegistryEffect → NodeRegistryEffect in:

registry_effect.py (class definition)
__init__.py exports
All test imports
Documentation references

2. Missing ONEX Base Class Inheritance

Issue: RegistryEffect does not extend NodeEffect from omnibase_core

CLAUDE.md Rule:

Architecture Rule: omnibase_infra extends base archetypes from omnibase_core. Never define new node archetypes in infra - they belong in core.

Current:

class RegistryEffect:  # Missing base class
    def __init__(self, consul_client: ProtocolConsulClient, ...):
        ...

Required:

from omnibase_core.nodes import NodeEffect, ModelEffectInput, ModelEffectOutput

class NodeRegistryEffect(NodeEffect):
    def __init__(self, container: ModelONEXContainer):
        super().__init__(container)
        # Resolve dependencies via container

Impact: Without base class inheritance:

Node does not follow ONEX Effect archetype contract
Missing standardized lifecycle methods (execute, health checks)
Cannot be registered in ONEX runtime
Breaks polymorphic node handling

3. Container-Based Dependency Injection Violation

Issue: Constructor uses direct dependency injection instead of ModelONEXContainer

CLAUDE.md Rule:

Container Injection - def __init__(self, container: ModelONEXContainer)

Current:

def __init__(
    self,
    consul_client: ProtocolConsulClient,
    postgres_adapter: ProtocolPostgresAdapter,
) -> None:

Required:

def __init__(self, container: ModelONEXContainer) -> None:
    super().__init__(container)
    self._consul_client = container.resolve(ProtocolConsulClient)
    self._postgres_adapter = container.resolve(ProtocolPostgresAdapter)

Impact:

Violates ONEX infrastructure DI pattern
Hard to test with real containers
Cannot use infrastructure service wiring
Breaks container-based configuration

Reference: docs/patterns/container_dependency_injection.md

4. Memory Leak Risk - Unbounded Cache

Issue: _completed_backends dict grows unbounded

Location: registry_effect.py:101

self._completed_backends: dict[UUID, set[str]] = {}  # Never cleared\!

Problem: Every register_node call adds an entry keyed by correlation_id. In production:

Long-running process → thousands of correlation IDs → memory leak
No TTL or eviction policy
No max size limit

Impact:

Production memory exhaustion
Performance degradation over time
Potential OOM crashes

Recommended Fix:

from cachetools import TTLCache

self._completed_backends: TTLCache = TTLCache(
    maxsize=10000,  # Limit entries
    ttl=3600,       # 1 hour TTL
)

Or use a proper idempotency store pattern with external storage (Redis, Postgres).

5. Idempotency Design Flaw

Issue: In-memory idempotency state does not survive restarts

Location: registry_effect.py:100-101

Problem: _completed_backends is instance state, not persisted:

Node restarts → state lost
Retry after restart → duplicate backend operations
Violates idempotency guarantees

CLAUDE.md Principle:

Idempotency persists across effect restarts

Solution: Use InMemoryIdempotencyStore or PostgresIdempotencyStore:

from omnibase_infra.idempotency import ProtocolIdempotencyStore

def __init__(self, container: ModelONEXContainer):
    super().__init__(container)
    self._idempotency_store = container.resolve(ProtocolIdempotencyStore)

async def register_node(self, request: ModelRegistryRequest):
    # Check if already processed
    if await self._idempotency_store.check_and_record(request.correlation_id):
        # Return cached result or skip
        return self._get_cached_result(request.correlation_id)
    
    # Execute registration...

⚠️ High Priority Issues

6. Error Handling - str(e) May Expose Secrets

Location: registry_effect.py:223, 280

except Exception as e:
    return ModelBackendResult(
        success=False,
        error=str(e),  # ⚠️ May contain credentials\!
        ...
    )

CLAUDE.md Rule:

Error messages MUST be sanitized before inclusion. Never include:

Credentials, connection strings, or secrets

Problem: str(e) from connection exceptions often contains:

Connection strings with passwords
API keys in URLs
Internal hostnames

Fix:

except InfraConnectionError as e:
    error = e.model.message  # Already sanitized
except Exception as e:
    # Sanitize generic exceptions
    error = f"{type(e).__name__}: Connection failed"

7. Missing Correlation ID Propagation

Issue: Backend operations don't receive correlation_id

Location: registry_effect.py:191-196, 247-253

result = await self._consul_client.register_service(
    service_id=service_id,
    service_name=service_name,
    tags=request.tags,
    health_check=request.health_check_config,
    # ❌ Missing: correlation_id=request.correlation_id
)

CLAUDE.md Rule:

Always propagate: Pass correlation_id from incoming requests to error context

Impact: Breaks distributed tracing chain - cannot correlate backend operations with requests

Fix: Update protocols to accept correlation_id:

class ProtocolConsulClient(Protocol):
    async def register_service(
        self,
        service_id: str,
        service_name: str,
        tags: list[str],
        correlation_id: UUID,  # Add this
        health_check: dict[str, str] | None = None,
    ) -> dict[str, bool | str]:
        ...

8. Hardcoded Backend IDs

Location: registry_effect.py:205, 262

backend_id="consul",  # Hardcoded string
backend_id="postgres", # Hardcoded string

Recommendation: Use enum for type safety:

from enum import Enum

class EnumBackendType(str, Enum):
    CONSUL = "consul"
    POSTGRES = "postgres"

backend_id=EnumBackendType.CONSUL,

✅ Strengths

Excellent Test Coverage

51 tests covering all G4 acceptance criteria
Circuit breaker state machine transitions thoroughly tested
Per-backend isolation validated
Correlation ID propagation verified
Edge cases covered (threshold=1, zero timeout, concurrent ops)

Strong Model Design

ModelBackendResult - Clean separation of backend results
ModelRegistryResponse - Excellent partial failure handling
Immutable models (frozen=True) - Thread-safe by design
Helper methods (is_partial_failure, get_failed_backends) - Great DX

Good Documentation

Comprehensive module docstrings
Clear architecture explanations
Test case organization by acceptance criteria
Inline comments explain design decisions

ONEX Pattern Compliance

✅ No Any types - uses object for generic payloads
✅ Strong typing throughout
✅ Pydantic models with proper validation
✅ Protocol-based abstractions
✅ Error context with correlation IDs
✅ One model per file convention

📊 Test Quality Analysis

Circuit Breaker Tests (test_effect_circuit_breaker.py)

Strengths:

Complete state machine coverage (CLOSED → OPEN → HALF_OPEN → CLOSED)
Per-backend isolation validated
Thread safety tested with 50 concurrent operations
Edge cases: threshold=1, zero timeout

Suggestions:

Add test for circuit state persistence across restarts
Test circuit behavior under high load (1000+ concurrent ops)

Idempotency Tests (test_effect_idempotency.py)

Concern: Tests mock idempotency but implementation doesn't actually use the store pattern. Tests pass but don't validate real behavior.

Partial Failure Tests (test_effect_partial_failure.py)

Excellent: Tests all combinations (Consul success + Postgres fail, etc.)

🔧 Recommendations

Immediate (Before Merge)

Rename RegistryEffect → NodeRegistryEffect (CRITICAL)
Extend NodeEffect base class (CRITICAL)
Use ModelONEXContainer for DI (CRITICAL)
Fix unbounded cache - add TTL or size limit
Sanitize str(e) in error handling
Propagate correlation_id to backend calls

Follow-Up (Next PR)

Implement persistent idempotency store
Add contract.yaml for node definition
Add integration tests with real container wiring
Add performance tests (high volume, concurrent requests)
Document memory/performance characteristics

🎯 Verdict

APPROVAL WITH REQUIRED CHANGES ✋

This PR represents excellent test coverage and solid engineering, but has critical architectural violations that prevent merging as-is:

Node naming convention (MUST fix)
Missing base class inheritance (MUST fix)
Container DI violation (MUST fix)
Memory leak risk (SHOULD fix)
Idempotency persistence issue (SHOULD fix)

Estimated effort to fix: 4-6 hours

Once the critical issues are addressed, this will be a strong addition to the ONEX infrastructure layer.

📚 References

CLAUDE.md: Node naming conventions (line 96)
CLAUDE.md: Container-based DI (lines 123-130)
CLAUDE.md: Error sanitization (lines 450-480)
docs/patterns/container_dependency_injection.md
docs/patterns/error_handling_patterns.md
omnibase_core.nodes - NodeEffect base class

…MN-954] Security: - Replace str(e) with sanitized error messages to prevent secret exposure Naming Convention: - Rename RegistryEffect → NodeRegistryEffect per ONEX standards New Components: - Add contract.yaml for ONEX node definition - Add ProtocolEffectIdempotencyStore for pluggable backends - Add StoreEffectIdempotencyInMemory with LRU/TTL cache - Add ModelEffectIdempotencyConfig for cache configuration Testing: - Add 16 integration tests with container wiring - Add 25 performance tests (high volume, concurrent) - Add 19 unit tests for idempotency store Documentation: - Add README.md with memory/performance characteristics - Update docstrings with O notation and throughput benchmarks

claude · 2025-12-22T01:29:32Z

PR Review: Effect Idempotency and Retry Behavior Tests [OMN-954]

Summary

This PR implements comprehensive testing for effect-level idempotency, circuit breaker integration, retry/backoff policies, and partial failure handling. The implementation is solid and production-ready with excellent test coverage (51 tests), thorough documentation, and proper adherence to ONEX conventions.

✅ Strengths

Excellent Documentation - README.md is exceptionally detailed with memory characteristics, performance benchmarks, configuration guidance, and production considerations
Strong Type Safety - Zero Any types, proper use of X | None (PEP 604), all models follow ONEX naming conventions
Comprehensive Test Coverage - 51 tests across unit, integration, and performance categories
Security-Conscious Error Handling - Sanitized error messages, proper correlation ID tracking
Memory-Bounded Design - LRU eviction strategy with configurable limits and clear documentation

🔍 Issues & Recommendations

Should Fix Before Merge

Error Message Sanitization (registry_effect.py:302,362) - Using str() on backend result errors could expose sensitive information. Consider more defensive sanitization.
Missing Contract Fields (contract.yaml) - Contract is missing required fields per ONEX schema: input_model, output_model, dependencies. Add these to align with full ONEX contract pattern.
Idempotency Store Protocol - Custom ProtocolEffectIdempotencyStore instead of existing ProtocolIdempotencyStore. Document rationale for separate protocol.
Circuit Breaker Integration - NodeRegistryEffect does NOT implement MixinAsyncCircuitBreaker. Verify design decision (node-level vs backend-level resilience) and document clearly.

Nice to Have

Rename registry_effect.py to node.py per ONEX conventions (or document exception)
Remove @pytest.mark.unit from performance tests (semantically incorrect)
Standardize all export position across modules

📋 ONEX Guidelines Compliance

✅ No Any types used
✅ Proper Pydantic models (one per file)
✅ PEP 604 union syntax (X | None)
✅ Sanitized error messages
✅ Correlation ID tracking
✅ Protocol-based duck typing
✅ Bounded memory with LRU eviction
✅ Comprehensive test coverage

🧪 Test Quality

Unit Tests: Well-structured, comprehensive fixtures, proper mocks, edge cases covered
Integration Tests: Container wiring used properly, realistic scenarios
Performance Tests: Latency distribution analysis, throughput benchmarks, properly marked for CI exclusion

🚀 Performance

Documented performance is excellent:

Idempotency checks: O(1) amortized
Throughput: >5,000 ops/sec single-worker, >10,000 concurrent
Memory: Linear scaling (100K entries = ~10MB)

🏆 Overall Assessment

Verdict: ✅ APPROVE with minor recommendations

This PR demonstrates excellent engineering with thorough documentation, comprehensive test coverage, production-ready design, and security-conscious implementation. The issues identified are minor and can be addressed in follow-up commits or accepted as-is with documentation.

Recommended Action: Merge after addressing contract.yaml fields and documenting the idempotency protocol design decision.

Related: OMN-954 | Reviewed: 2025-12-22 | By: Claude Code (ONEX Architecture Review)

coderabbitai

Actionable comments posted: 4

🧹 Nitpick comments (17)

src/omnibase_infra/nodes/effects/protocol_effect_idempotency_store.py (1)
98-130: Consider documenting exception behavior for all protocol methods.

Only mark_completed (Line 93-94) documents potential exceptions. For consistency and clarity, consider documenting exception behavior for is_completed, get_completed_backends, and clear methods, especially since implementations may encounter store unavailability or lock acquisition failures.
Example documentation pattern
 async def is_completed(self, correlation_id: UUID, backend: str) -> bool:
     """Check if a backend is completed for a correlation ID.

     Args:
         correlation_id: Unique identifier for the operation.
         backend: Backend identifier to check.

     Returns:
         True if the backend is completed, False otherwise.
+
+    Raises:
+        RuntimeError: If the store is unavailable.
     """
     ...
src/omnibase_infra/nodes/effects/README.md (1)
1-350: Well-structured and comprehensive documentation.

This README provides excellent coverage of the NodeRegistryEffect including architecture diagrams, memory characteristics, performance benchmarks, configuration guidance, production considerations, and usage examples. The warnings about in-memory store limitations in multi-instance deployments are particularly valuable.

One minor issue: the basic usage example on line 240 uses uuid4() but the import statement on line 217 only shows from unittest.mock import AsyncMock. Consider adding from uuid import uuid4 to the import section of that example for completeness.
🔎 Suggested fix for the example import
 ```python
+from uuid import uuid4
 from unittest.mock import AsyncMock

 from omnibase_infra.nodes.effects import NodeRegistryEffect
src/omnibase_infra/nodes/effects/models/model_registry_request.py (1)
76-79: Consider using Literal type for node_type validation.

The docstring specifies valid values as effect, compute, reducer, orchestrator. Using a Literal type would provide validation at the Pydantic level and better IDE support.
🔎 Optional: Add type constraint for node_type
+from typing import Literal
+
+NodeType = Literal["effect", "compute", "reducer", "orchestrator"]
+
 class ModelRegistryRequest(BaseModel):
     # ...
-    node_type: str = Field(
+    node_type: NodeType = Field(
         ...,
         description="Type of ONEX node (effect, compute, reducer, orchestrator)",
     )
Alternatively, if extensibility is needed, keep str but add a validator for known types.
src/omnibase_infra/nodes/effects/protocol_postgres_adapter.py (1)
26-46: Consider using TypedDict for stronger return type safety.

The return type dict[str, bool | str] is flexible but doesn't enforce the expected structure ({"success": bool, "error"?: str}). A TypedDict would provide better type checking and documentation.
🔎 Optional: Define structured return type
from typing import TypedDict, Protocol
from uuid import UUID


class UpsertResult(TypedDict, total=False):
    success: bool  # required
    error: str     # optional


class ProtocolPostgresAdapter(Protocol):
    async def upsert(
        self,
        node_id: UUID,
        node_type: str,
        node_version: str,
        endpoints: dict[str, str],
        metadata: dict[str, str],
    ) -> UpsertResult:
        ...
This makes the expected return structure explicit while maintaining backward compatibility with existing implementations.
tests/unit/registration/effect/test_effect_idempotency_store.py (1)
430-434: Consider using a more specific exception type.

Using pytest.raises(Exception) is overly broad. Pydantic frozen models raise ValidationError when attempting mutation.
🔎 Proposed fix
+    from pydantic import ValidationError
+
     def test_config_is_frozen(self) -> None:
         """Test that config is immutable."""
         config = ModelEffectIdempotencyConfig()
-        with pytest.raises(Exception):  # Pydantic raises ValidationError
+        with pytest.raises(ValidationError):
             config.max_cache_size = 5000  # type: ignore[misc]
tests/integration/registration/effect/conftest.py (1)
153-160: The __all__ export list is unnecessary in conftest.py.

Pytest automatically discovers fixtures in conftest.py files without requiring explicit exports. The __all__ definition has no effect on fixture availability and can be safely removed.
🔎 Proposed fix
     return _create_request
-
-
-__all__ = [
-    "consul_client",
-    "postgres_adapter",
-    "idempotency_store",
-    "registry_effect",
-    "sample_request",
-    "request_factory",
-]
tests/performance/registration/effect/test_idempotency_store_performance.py (1)

334-358: Workload mix test creates coroutines correctly but could be clearer.

The workload dictionary creates coroutine objects that are awaited later. This works but may be confusing since coroutines are created before store.clear_all() resets the store.

Consider restructuring to create coroutines after the store reset for each workload iteration to ensure clean state, or add a comment clarifying that the coroutines capture the store reference and execute after reset.

src/omnibase_infra/nodes/effects/registry_effect.py (1)

275-276: retries field is always 0 - retry logic not implemented at this level.

The retries field in ModelBackendResult is set to 0 in all cases. This is intentional since retry logic is expected to be handled by the caller or circuit breaker, but it may be worth documenting this explicitly or removing the unused variable.

The retries = 0 assignment on line 276/338 and its usage in ModelBackendResult could be removed if retry counting is handled elsewhere, or the docstring could clarify that retry tracking is delegated to a higher layer.
tests/performance/registration/effect/conftest.py (1)
22-22: Unused TYPE_CHECKING import.

The TYPE_CHECKING constant is imported but never used in this file. Remove it to keep imports clean.
🔎 Proposed fix
 from datetime import UTC, datetime
-from typing import TYPE_CHECKING
 from unittest.mock import AsyncMock, MagicMock
 from uuid import UUID, uuid4
tests/unit/registration/effect/test_effect_idempotency.py (2)
64-71: Fixture duplicates the one in conftest.py.

This inmemory_idempotency_store fixture is defined both here and in tests/unit/registration/effect/conftest.py. The conftest fixture will be automatically available, so this local definition is redundant and could lead to confusion.
🔎 Proposed fix

Remove lines 64-71 since the fixture is already defined in conftest.py:
-@pytest.fixture
-def inmemory_idempotency_store() -> InMemoryIdempotencyStore:
-    """Create an InMemoryIdempotencyStore for testing.
-
-    Returns:
-        A fresh InMemoryIdempotencyStore instance.
-    """
-    return InMemoryIdempotencyStore()
-
-
74-99: Mock fixtures duplicate those in conftest.py.

The mock_consul_client and mock_postgres_client fixtures here are similar to those in the conftest. Consider using the shared fixtures from conftest.py to reduce duplication. However, note that the conftest version uses synchronous MagicMock for register/deregister while this version uses AsyncMock - verify which is correct for your use case.
src/omnibase_infra/nodes/effects/store_effect_idempotency_inmemory.py (1)

314-343: Internal async methods don't actually await.

_maybe_cleanup_expired, _cleanup_expired_entries, and _evict_lru_if_needed are declared async but don't contain any await expressions. They could be regular sync methods. However, this is a minor style point and doesn't affect correctness - keeping them async maintains consistency if future I/O operations are added.

tests/unit/registration/effect/test_effect_retry_backoff.py (1)

329-333: Access circuit breaker state via public methods instead of internal attributes.

The test directly accesses private attributes _circuit_breaker_initialized, _circuit_breaker_failures, and _circuit_breaker_open (8 occurrences). ConsulHandler exposes circuit breaker status through the public health_check() method, which returns circuit_breaker_state and circuit_breaker_failure_count. Other handlers in the codebase follow the pattern of providing a public get_circuit_state() method for tests to safely access circuit breaker state with proper lock protection. Consider either adding a public state accessor method to ConsulHandler (consistent with other handlers) or refactoring these assertions to use health_check().
tests/unit/registration/effect/test_effect_circuit_breaker.py (1)
49-49: Remove unused TypeVar.

T is declared but never used in this module.
🔎 Proposed fix
-T = TypeVar("T")
-
-
 class MockConsulBackend:
tests/performance/registration/effect/test_effect_performance.py (1)

503-509: Consider adding a public method for cache memory estimation.

Accessing store._cache directly breaks encapsulation. While acceptable in tests, a public get_cache_memory_bytes() method on InMemoryEffectIdempotencyStore would be cleaner and more maintainable.
tests/integration/registration/effect/test_registry_effect_integration.py (2)
601-602: Remove unused fixture or use it.

request_factory is suppressed as unused but the comment says it's "available for debugging." Either remove from function signature or use it to create the test requests instead of manual ModelRegistryRequest construction.
🔎 Option 1: Remove unused fixture
     async def test_different_correlation_ids_independent(
         self,
         registry_effect: NodeRegistryEffect,
         consul_client: StubConsulClient,
         postgres_adapter: StubPostgresAdapter,
-        request_factory: Callable[..., ModelRegistryRequest],
     ) -> None:
         """Test that different correlation IDs are processed independently.

         Verifies that idempotency is keyed by correlation_id, not node_id.
         """
-        # Suppress unused fixture warning - fixture is available for debugging
-        _ = request_factory
-
         # Arrange - Same node_id but different correlation_ids
         base_node_id = uuid4()
712-712: Move import to module level.

import asyncio inside the test method is unconventional. Move it to the module-level imports for consistency.
🔎 Proposed fix

At module level (after line 36):
 import pytest
+import asyncio
In the test method:
-        import asyncio
-
         effect = NodeRegistryEffect(

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Lite

📥 Commits

Reviewing files that changed from the base of the PR and between 67c8edf and 74152c0.

📒 Files selected for processing (33)

src/omnibase_infra/nodes/__init__.py
src/omnibase_infra/nodes/effects/README.md
src/omnibase_infra/nodes/effects/__init__.py
src/omnibase_infra/nodes/effects/contract.yaml
src/omnibase_infra/nodes/effects/models/__init__.py
src/omnibase_infra/nodes/effects/models/model_backend_result.py
src/omnibase_infra/nodes/effects/models/model_effect_idempotency_config.py
src/omnibase_infra/nodes/effects/models/model_registry_request.py
src/omnibase_infra/nodes/effects/models/model_registry_response.py
src/omnibase_infra/nodes/effects/protocol_consul_client.py
src/omnibase_infra/nodes/effects/protocol_effect_idempotency_store.py
src/omnibase_infra/nodes/effects/protocol_postgres_adapter.py
src/omnibase_infra/nodes/effects/registry_effect.py
src/omnibase_infra/nodes/effects/store_effect_idempotency_inmemory.py
tests/integration/registration/__init__.py
tests/integration/registration/effect/__init__.py
tests/integration/registration/effect/conftest.py
tests/integration/registration/effect/test_doubles.py
tests/integration/registration/effect/test_registry_effect_integration.py
tests/performance/__init__.py
tests/performance/registration/__init__.py
tests/performance/registration/effect/__init__.py
tests/performance/registration/effect/conftest.py
tests/performance/registration/effect/test_effect_performance.py
tests/performance/registration/effect/test_idempotency_store_performance.py
tests/unit/registration/__init__.py
tests/unit/registration/effect/__init__.py
tests/unit/registration/effect/conftest.py
tests/unit/registration/effect/test_effect_circuit_breaker.py
tests/unit/registration/effect/test_effect_idempotency.py
tests/unit/registration/effect/test_effect_idempotency_store.py
tests/unit/registration/effect/test_effect_partial_failure.py
tests/unit/registration/effect/test_effect_retry_backoff.py

🧰 Additional context used

📓 Path-based instructions (8)

**/contract.yaml