Fix V1 callbacks (#11654)

Co-authored-by: openhands <openhands@all-hands.dev>

tests/unit/app_server/test_sql_event_callback_service.py

@@ -372,3 +372,284 @@ class TestSQLEventCallbackService:
         assert len(result.items) == 2
         assert result.items[0].id == callback2.id
         assert result.items[1].id == callback1.id
+
+    async def test_save_event_callback_new(
+        self,
+        service: SQLEventCallbackService,
+        sample_callback: EventCallback,
+    ):
+        """Test saving a new event callback (insert scenario)."""
+        # Save the callback
+        original_updated_at = sample_callback.updated_at
+        saved_callback = await service.save_event_callback(sample_callback)
+
+        # Verify the returned callback
+        assert saved_callback.id == sample_callback.id
+        assert saved_callback.conversation_id == sample_callback.conversation_id
+        assert saved_callback.processor == sample_callback.processor
+        assert saved_callback.event_kind == sample_callback.event_kind
+        assert saved_callback.status == sample_callback.status
+
+        # Verify updated_at was changed (handle timezone differences)
+        # Convert both to UTC for comparison if needed
+        original_utc = (
+            original_updated_at.replace(tzinfo=timezone.utc)
+            if original_updated_at.tzinfo is None
+            else original_updated_at
+        )
+        saved_utc = (
+            saved_callback.updated_at.replace(tzinfo=timezone.utc)
+            if saved_callback.updated_at.tzinfo is None
+            else saved_callback.updated_at
+        )
+        assert saved_utc >= original_utc
+
+        # Commit the transaction to persist changes
+        await service.db_session.commit()
+
+        # Verify the callback can be retrieved
+        retrieved_callback = await service.get_event_callback(sample_callback.id)
+        assert retrieved_callback is not None
+        assert retrieved_callback.id == sample_callback.id
+        assert retrieved_callback.conversation_id == sample_callback.conversation_id
+        assert retrieved_callback.event_kind == sample_callback.event_kind
+
+    async def test_save_event_callback_update_existing(
+        self,
+        service: SQLEventCallbackService,
+        sample_request: CreateEventCallbackRequest,
+    ):
+        """Test saving an existing event callback (update scenario)."""
+        # First create a callback through the service
+        created_callback = await service.create_event_callback(sample_request)
+        original_updated_at = created_callback.updated_at
+
+        # Modify the callback
+        created_callback.event_kind = 'ObservationEvent'
+        from openhands.app_server.event_callback.event_callback_models import (
+            EventCallbackStatus,
+        )
+
+        created_callback.status = EventCallbackStatus.DISABLED
+
+        # Save the modified callback
+        saved_callback = await service.save_event_callback(created_callback)
+
+        # Verify the returned callback has the modifications
+        assert saved_callback.id == created_callback.id
+        assert saved_callback.event_kind == 'ObservationEvent'
+        assert saved_callback.status == EventCallbackStatus.DISABLED
+
+        # Verify updated_at was changed (handle timezone differences)
+        original_utc = (
+            original_updated_at.replace(tzinfo=timezone.utc)
+            if original_updated_at.tzinfo is None
+            else original_updated_at
+        )
+        saved_utc = (
+            saved_callback.updated_at.replace(tzinfo=timezone.utc)
+            if saved_callback.updated_at.tzinfo is None
+            else saved_callback.updated_at
+        )
+        assert saved_utc >= original_utc
+
+        # Commit the transaction to persist changes
+        await service.db_session.commit()
+
+        # Verify the changes were persisted
+        retrieved_callback = await service.get_event_callback(created_callback.id)
+        assert retrieved_callback is not None
+        assert retrieved_callback.event_kind == 'ObservationEvent'
+        assert retrieved_callback.status == EventCallbackStatus.DISABLED
+
+    async def test_save_event_callback_timestamp_update(
+        self,
+        service: SQLEventCallbackService,
+        sample_callback: EventCallback,
+    ):
+        """Test that save_event_callback properly updates the timestamp."""
+        # Record the original timestamp
+        original_updated_at = sample_callback.updated_at
+
+        # Wait a small amount to ensure timestamp difference
+        import asyncio
+
+        await asyncio.sleep(0.01)
+
+        # Save the callback
+        saved_callback = await service.save_event_callback(sample_callback)
+
+        # Verify updated_at was changed and is more recent (handle timezone differences)
+        original_utc = (
+            original_updated_at.replace(tzinfo=timezone.utc)
+            if original_updated_at.tzinfo is None
+            else original_updated_at
+        )
+        saved_utc = (
+            saved_callback.updated_at.replace(tzinfo=timezone.utc)
+            if saved_callback.updated_at.tzinfo is None
+            else saved_callback.updated_at
+        )
+        assert saved_utc >= original_utc
+        assert isinstance(saved_callback.updated_at, datetime)
+
+        # Verify the timestamp is recent (within last minute)
+        now = datetime.now(timezone.utc)
+        time_diff = now - saved_utc
+        assert time_diff.total_seconds() < 60
+
+    async def test_save_event_callback_with_null_values(
+        self,
+        service: SQLEventCallbackService,
+        sample_processor: EventCallbackProcessor,
+    ):
+        """Test saving a callback with null conversation_id and event_kind."""
+        # Create a callback with null values
+        callback = EventCallback(
+            conversation_id=None,
+            processor=sample_processor,
+            event_kind=None,
+        )
+
+        # Save the callback
+        saved_callback = await service.save_event_callback(callback)
+
+        # Verify the callback was saved correctly
+        assert saved_callback.id == callback.id
+        assert saved_callback.conversation_id is None
+        assert saved_callback.event_kind is None
+        assert saved_callback.processor == sample_processor
+
+        # Commit and verify persistence
+        await service.db_session.commit()
+        retrieved_callback = await service.get_event_callback(callback.id)
+        assert retrieved_callback is not None
+        assert retrieved_callback.conversation_id is None
+        assert retrieved_callback.event_kind is None
+
+    async def test_save_event_callback_preserves_created_at(
+        self,
+        service: SQLEventCallbackService,
+        sample_request: CreateEventCallbackRequest,
+    ):
+        """Test that save_event_callback preserves the original created_at timestamp."""
+        # Create a callback through the service
+        created_callback = await service.create_event_callback(sample_request)
+        original_created_at = created_callback.created_at
+
+        # Wait a small amount to ensure timestamp difference
+        import asyncio
+
+        await asyncio.sleep(0.01)
+
+        # Save the callback again
+        saved_callback = await service.save_event_callback(created_callback)
+
+        # Verify created_at was preserved but updated_at was changed
+        assert saved_callback.created_at == original_created_at
+        # Handle timezone differences for comparison
+        created_utc = (
+            original_created_at.replace(tzinfo=timezone.utc)
+            if original_created_at.tzinfo is None
+            else original_created_at
+        )
+        updated_utc = (
+            saved_callback.updated_at.replace(tzinfo=timezone.utc)
+            if saved_callback.updated_at.tzinfo is None
+            else saved_callback.updated_at
+        )
+        assert updated_utc >= created_utc
+
+    async def test_save_event_callback_different_statuses(
+        self,
+        service: SQLEventCallbackService,
+        sample_processor: EventCallbackProcessor,
+    ):
+        """Test saving callbacks with different status values."""
+        from openhands.app_server.event_callback.event_callback_models import (
+            EventCallbackStatus,
+        )
+
+        # Test each status
+        statuses = [
+            EventCallbackStatus.ACTIVE,
+            EventCallbackStatus.DISABLED,
+            EventCallbackStatus.COMPLETED,
+            EventCallbackStatus.ERROR,
+        ]
+
+        for status in statuses:
+            callback = EventCallback(
+                conversation_id=uuid4(),
+                processor=sample_processor,
+                event_kind='ActionEvent',
+                status=status,
+            )
+
+            # Save the callback
+            saved_callback = await service.save_event_callback(callback)
+
+            # Verify the status was preserved
+            assert saved_callback.status == status
+
+            # Commit and verify persistence
+            await service.db_session.commit()
+            retrieved_callback = await service.get_event_callback(callback.id)
+            assert retrieved_callback is not None
+            assert retrieved_callback.status == status
+
+    async def test_save_event_callback_returns_same_object(
+        self,
+        service: SQLEventCallbackService,
+        sample_callback: EventCallback,
+    ):
+        """Test that save_event_callback returns the same object instance."""
+        # Save the callback
+        saved_callback = await service.save_event_callback(sample_callback)
+
+        # Verify it's the same object (identity check)
+        assert saved_callback is sample_callback
+
+        # But verify the updated_at was modified on the original object
+        assert sample_callback.updated_at == saved_callback.updated_at
+
+    async def test_save_event_callback_multiple_saves(
+        self,
+        service: SQLEventCallbackService,
+        sample_callback: EventCallback,
+    ):
+        """Test saving the same callback multiple times."""
+        # Save the callback multiple times
+        first_save = await service.save_event_callback(sample_callback)
+        first_updated_at = first_save.updated_at
+
+        # Wait a small amount to ensure timestamp difference
+        import asyncio
+
+        await asyncio.sleep(0.01)
+
+        second_save = await service.save_event_callback(sample_callback)
+        second_updated_at = second_save.updated_at
+
+        # Verify timestamps are different (handle timezone differences)
+        first_utc = (
+            first_updated_at.replace(tzinfo=timezone.utc)
+            if first_updated_at.tzinfo is None
+            else first_updated_at
+        )
+        second_utc = (
+            second_updated_at.replace(tzinfo=timezone.utc)
+            if second_updated_at.tzinfo is None
+            else second_updated_at
+        )
+        assert second_utc >= first_utc
+
+        # Verify it's still the same callback
+        assert first_save.id == second_save.id
+        assert first_save is second_save  # Same object instance
+
+        # Commit and verify only one record exists
+        await service.db_session.commit()
+        retrieved_callback = await service.get_event_callback(sample_callback.id)
+        assert retrieved_callback is not None
+        assert retrieved_callback.id == sample_callback.id

tests/unit/experiments/test_experiment_manager.py

@@ -169,6 +169,7 @@ class TestExperimentManagerIntegration:
         # The service requires a lot of deps, but for this test we won't exercise them.
         app_conversation_info_service = Mock()
         app_conversation_start_task_service = Mock()
+        event_callback_service = Mock()
         sandbox_service = Mock()
         sandbox_spec_service = Mock()
         jwt_service = Mock()
@@ -179,6 +180,7 @@ class TestExperimentManagerIntegration:
             user_context=user_context,
             app_conversation_info_service=app_conversation_info_service,
             app_conversation_start_task_service=app_conversation_start_task_service,
+            event_callback_service=event_callback_service,
             sandbox_service=sandbox_service,
             sandbox_spec_service=sandbox_spec_service,
             jwt_service=jwt_service,