diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a81003e6..692a49b59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -78,6 +78,19 @@ The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- **`LLMTraceRepository.save()` premature commit breaks UnitOfWork transactions** (#7505): + Replaced the unconditional `session.commit()` in `LLMTraceRepository.save()` with a + dual-path implementation that respects the UnitOfWork (UoW) pattern. When an external + session is provided (UoW mode), the method now calls only `session.flush()`, leaving + transaction control to the caller. When no session is provided (standalone mode), the + method creates its own session, flushes, commits, and closes it to ensure durable + persistence. This eliminates three data-integrity violations: premature commit of outer + UoW transactions, loss of rollback capability for subsequent failures, and a mismatch + between the class docstring ("Callers are responsible for commit") and the implementation. + Input validation for the `trace` argument was also added. Two new BDD scenarios verify + the session contract: `Repository save() calls flush not commit` and `LLM trace rolled + back when UnitOfWork transaction rolls back`. + - **git_tools._get_base_env() TOCTOU Race Condition** (#7619): Fixed a Time-Of-Check-To-Time-Of-Use race condition in `git_tools._get_base_env()` where two concurrent threads could both observe `_BASE_ENV is None`, both diff --git a/CONTRIBUTORS.md b/CONTRIBUTORS.md index 1ce843171..233e467bc 100644 --- a/CONTRIBUTORS.md +++ b/CONTRIBUTORS.md @@ -29,3 +29,4 @@ Below are some of the specific details of various contributions. * HAL 9000 has contributed the git worktree TOCTOU race condition fix (PR #8178 / issue #7507): replaced the unsafe mkdtemp() + rmdir() pattern with a parent-directory approach to eliminate the race window in concurrent git worktree operations. * HAL 9000 has contributed the git_tools TOCTOU race condition fix (PR #8255 / issue #7619): eliminated the Time-Of-Check-To-Time-Of-Use race in `_get_base_env()` by adding double-checked locking with a module-level `threading.Lock`, preventing concurrent threads from writing conflicting environment snapshots. * HAL 9000 has contributed comprehensive milestone documentation for v3.6.0 (Advanced Concepts & Deferred Features) and v3.7.0 (TUI Implementation) (PR #9903): split into sub-documents covering context strategies, LLM backends, resource types, A2A rename, container tool execution, scope chain resolution, cost/safety budgets, E2E workflow tests, code review examples, plugin architecture, TUI layout, persona system, reference/command input, session management, configuration, and TuiMaterializer integration. +* HAL 9000 has contributed the LLMTraceRepository data-integrity fix (PR #8185 / issue #7505): replaced the unconditional `session.commit()` in `LLMTraceRepository.save()` with a dual-path implementation that respects the UnitOfWork pattern — flushing only when an external session is provided, and flushing + committing + closing when operating standalone. This eliminates premature transaction commits, loss of rollback capability, and a docstring/implementation mismatch. diff --git a/features/steps/llm_trace_steps.py b/features/steps/llm_trace_steps.py index eed9f756d..67e24da83 100644 --- a/features/steps/llm_trace_steps.py +++ b/features/steps/llm_trace_steps.py @@ -809,6 +809,9 @@ class _BrokenSession: def rollback(self) -> None: pass + def close(self) -> None: + pass + def query(self, *_args: Any, **_kwargs: Any) -> Any: raise SQLAlchemyDatabaseError("mock", {}, Exception("broken")) @@ -985,8 +988,10 @@ def step_save_with_spy(context: Context) -> None: object.__setattr__(real_session, "flush", spy_flush) object.__setattr__(real_session, "commit", spy_commit) + # Pass the session explicitly to test the UoW path: save() must flush + # but must NOT commit (the caller owns the transaction boundary). repo = LLMTraceRepository(session_factory=lambda: real_session) - repo.save(context.trace) + repo.save(context.trace, session=real_session) # Commit so the data is visible for subsequent queries object.__setattr__(real_session, "commit", original_commit) real_session.commit() @@ -1030,7 +1035,9 @@ def step_save_in_uow_rollback(context: Context) -> None: session = context.uow_session_factory() repo = LLMTraceRepository(session_factory=lambda: session) try: - repo.save(context.trace) + # Pass the session explicitly to use UoW mode: save() flushes but + # does NOT commit, so the caller's rollback can undo the change. + repo.save(context.trace, session=session) # Simulate a subsequent failure that triggers rollback raise RuntimeError("Simulated failure after save") except RuntimeError: diff --git a/src/cleveragents/infrastructure/database/llm_trace_repository.py b/src/cleveragents/infrastructure/database/llm_trace_repository.py index 70fcfa1ae..a2adc0671 100644 --- a/src/cleveragents/infrastructure/database/llm_trace_repository.py +++ b/src/cleveragents/infrastructure/database/llm_trace_repository.py @@ -31,6 +31,17 @@ class LLMTraceRepository: Uses the session-factory pattern: each public method obtains a session from the factory. Callers are responsible for commit. + + When ``save()`` is called with an explicit ``session`` argument the + repository operates in *UnitOfWork mode*: it flushes the change into + the caller's transaction but does **not** commit or close the session. + The caller (or the enclosing ``UnitOfWork``) is responsible for the + final commit. + + When ``save()`` is called without an explicit ``session`` argument the + repository operates in *standalone mode*: it creates its own session + from the factory, flushes, commits, and closes the session so that the + trace is durably persisted even outside a ``UnitOfWork``. """ def __init__( @@ -46,16 +57,26 @@ class LLMTraceRepository: return self._sf() @database_retry - def save(self, trace: LLMTrace) -> None: + def save(self, trace: LLMTrace, session: Session | None = None) -> None: """Persist a single ``LLMTrace`` row. Args: - trace: The trace to persist. + trace: The trace to persist. Must not be ``None``. + session: Optional external SQLAlchemy session. When provided + the repository flushes into the caller's transaction and + does **not** commit or close the session (UnitOfWork mode). + When omitted the repository creates its own session, commits, + and closes it (standalone mode). Raises: + ValueError: If ``trace`` is ``None``. DatabaseError: On unrecoverable persistence failure. """ - session = self._session() + if trace is None: + raise ValueError("trace must not be None") + + own_session = session is None + s: Session = self._session() if own_session else session try: model = LLMTraceModel( trace_id=trace.trace_id, @@ -77,11 +98,16 @@ class LLMTraceRepository: error=trace.error, timestamp=trace.timestamp.isoformat(), ) - session.add(model) - session.flush() + s.add(model) + s.flush() + if own_session: + s.commit() except (SQLAlchemyDatabaseError, OperationalError) as exc: - session.rollback() + s.rollback() raise DatabaseError(f"Failed to save LLM trace: {exc}") from exc + finally: + if own_session: + s.close() @database_retry def get(self, trace_id: str) -> LLMTrace | None: