From 02770bce7d0bbfa8a32b6c1efb0095f964f18a32 Mon Sep 17 00:00:00 2001 From: Azamat Date: Fri, 3 Apr 2026 01:55:12 +0300 Subject: [PATCH] fix sandbox replace trace identity --- tasks.md | 38 ++++++++++++++++++++++- test/test_sandbox_usecase.py | 59 +++++++++++++++++++++++++++++++++++- usecase/sandbox.py | 30 +++++++++++++++--- 3 files changed, 120 insertions(+), 7 deletions(-) diff --git a/tasks.md b/tasks.md index e01dcde..4b9e7e5 100644 --- a/tasks.md +++ b/tasks.md @@ -281,9 +281,45 @@ ### M23. Boundary review для sandbox observability - Субагент: `code-reviewer` -- Статус: pending +- Статус: in_progress - Зависимости: `M22` - Commit required: no - Scope: проверить, что observability изменения закрывают issue #11 и FR-034 без нарушения clean architecture - Файлы: весь измененный код после `M19`-`M22` - Критерии приемки: inner layers не импортируют OTel; Docker-specific tracing остается в `adapter/docker/`; current-state и duration metrics достаточно покрывают sandbox lifecycle; замечания сведены к minor или отсутствуют + +## Follow-up после M23 boundary review + +### M24. Исправить replace trace identity в CreateSandbox + +- Субагент: `feature-developer` +- Статус: completed +- Зависимости: `M23` +- Commit required: yes +- Commit message: `fix sandbox replace trace identity` +- Scope: устранить смешение old/new sandbox identifiers в replace path usecase tracing +- Файлы: `usecase/sandbox.py`, при необходимости точечные тесты в `test/*` +- Решение: сохранять старые и новые sandbox identifiers в отдельных span attrs или child spans так, чтобы replace success и replace failure оставались однозначно трассируемыми +- Критерии приемки: replace path не перетирает previous/new identifiers; при replace failure span остается консистентным и отражает обе стороны lifecycle + +### M25. Добрать failure-path observability regression tests + +- Субагент: `test-engineer` +- Статус: pending +- Зависимости: `M24` +- Commit required: yes +- Commit message: `add sandbox observability failure tests` +- Scope: покрыть tests для replace-failure trace, cleanup error metrics/spans и Docker stop observability +- Файлы: `test/test_sandbox_usecase.py`, `test/test_docker_runtime.py`, при необходимости другие focused tests в `test/*` +- Решение: использовать presence-based assertions и проверять ключевые span/metric contracts без brittle exact-order checks +- Критерии приемки: есть тест на replace failure tracing; есть тест на `sandbox.cleanup.error.total`; есть тесты на Docker stop observability для success/error/not_found или эквивалентного набора outcome paths + +### M26. Повторный boundary review для sandbox observability + +- Субагент: `code-reviewer` +- Статус: pending +- Зависимости: `M25` +- Commit required: no +- Scope: подтвердить, что follow-up fixes закрыли M23 замечания без новых boundary нарушений +- Файлы: весь измененный код после `M24`-`M25` +- Критерии приемки: нет замечаний по replace tracing identity и missing failure-path observability coverage; clean architecture по-прежнему соблюдена diff --git a/test/test_sandbox_usecase.py b/test/test_sandbox_usecase.py index 403eb0f..92c7937 100644 --- a/test/test_sandbox_usecase.py +++ b/test/test_sandbox_usecase.py @@ -9,7 +9,7 @@ from adapter.observability.noop import NoopMetrics, NoopTracer from domain.sandbox import SandboxSession, SandboxStatus from repository.sandbox_lock import ProcessLocalSandboxLifecycleLocker from repository.sandbox_session import InMemorySandboxSessionRepository -from usecase.interface import AttrValue, Attrs +from usecase.interface import Attrs, AttrValue from usecase.sandbox import CleanupExpiredSandboxes, CreateSandbox, CreateSandboxCommand CHAT_ID = UUID('11111111-1111-1111-1111-111111111111') @@ -486,6 +486,10 @@ def test_create_sandbox_replace_records_observability_and_final_active_count( 'usecase.create_sandbox', {'chat.id': str(CHAT_ID)}, { + 'sandbox.previous_session.id': str(SESSION_OLD_ID), + 'sandbox.previous_container.id': 'container-old', + 'sandbox.new_session.id': str(SESSION_NEW_ID), + 'sandbox.new_container.id': f'container-{SESSION_NEW_ID}', 'session.id': str(SESSION_NEW_ID), 'container.id': f'container-{SESSION_NEW_ID}', 'sandbox.result': 'replaced', @@ -649,6 +653,59 @@ def test_create_sandbox_error_records_observability(monkeypatch) -> None: assert excinfo.value in span.errors +def test_create_sandbox_replace_stop_failure_preserves_separate_identities( + monkeypatch, +) -> None: + now = datetime(2026, 4, 2, 12, 0, tzinfo=UTC) + expired_session = SandboxSession( + session_id=SESSION_OLD_ID, + chat_id=CHAT_ID, + container_id='container-old', + status=SandboxStatus.RUNNING, + created_at=now - timedelta(minutes=10), + expires_at=now, + ) + repository = InMemorySandboxSessionRepository() + repository.save(expired_session) + metrics = RecordingMetrics() + tracer = RecordingTracer() + usecase = CreateSandbox( + repository=repository, + locker=FakeLocker(), + runtime=FailingStopRuntime('container-old'), + clock=FakeClock(now), + logger=FakeLogger(), + metrics=metrics, + tracer=tracer, + ttl=timedelta(minutes=5), + ) + monkeypatch.setattr('usecase.sandbox._new_session_id', lambda: SESSION_NEW_ID) + + with pytest.raises(RuntimeError, match='stop_failed') as excinfo: + usecase.execute(CreateSandboxCommand(chat_id=CHAT_ID)) + + _assert_increment_metric_present( + metrics, + 'sandbox.create.total', + attrs={'result': 'error'}, + ) + span = _find_span( + tracer, + 'usecase.create_sandbox', + {'chat.id': str(CHAT_ID)}, + { + 'sandbox.previous_session.id': str(SESSION_OLD_ID), + 'sandbox.previous_container.id': 'container-old', + 'sandbox.new_session.id': str(SESSION_NEW_ID), + 'sandbox.result': 'error', + }, + ) + assert 'sandbox.new_container.id' not in span.attrs + assert 'session.id' not in span.attrs + assert 'container.id' not in span.attrs + assert excinfo.value in span.errors + + def test_create_sandbox_serializes_duplicate_concurrent_create_for_chat_id( monkeypatch, ) -> None: diff --git a/usecase/sandbox.py b/usecase/sandbox.py index 2bdb369..0a3412f 100644 --- a/usecase/sandbox.py +++ b/usecase/sandbox.py @@ -67,10 +67,22 @@ class CreateSandbox: return session result = 'created' + new_session_id: UUID | None = None if session is not None: result = 'replaced' - span.set_attribute('session.id', str(session.session_id)) - span.set_attribute('container.id', session.container_id) + new_session_id = _new_session_id() + span.set_attribute( + 'sandbox.previous_session.id', + str(session.session_id), + ) + span.set_attribute( + 'sandbox.previous_container.id', + session.container_id, + ) + span.set_attribute( + 'sandbox.new_session.id', + str(new_session_id), + ) self._logger.info( 'sandbox_replaced', attrs=_sandbox_attrs(session), @@ -81,16 +93,24 @@ class CreateSandbox: created_at = self._clock.now() expires_at = created_at + self._ttl - session_id = _new_session_id() - span.set_attribute('session.id', str(session_id)) + if new_session_id is None: + new_session_id = _new_session_id() + span.set_attribute('session.id', str(new_session_id)) new_session = self._runtime.create( - session_id=session_id, + session_id=new_session_id, chat_id=chat_id, created_at=created_at, expires_at=expires_at, ) + if result == 'replaced': + span.set_attribute( + 'sandbox.new_container.id', + new_session.container_id, + ) self._repository.save(new_session) _set_active_count(self._metrics, self._repository) + if result == 'replaced': + span.set_attribute('session.id', str(new_session.session_id)) span.set_attribute('container.id', new_session.container_id) span.set_attribute('sandbox.result', result) self._metrics.increment(