Fix #12: roll back mutations when saving fails

tests/test_main.py

@@ -5,19 +5,31 @@ This module contains unit tests for the HostsManagerApp class,
 validating application behavior, navigation, and user interactions.
 """
 
-from unittest.mock import Mock, patch
+from datetime import datetime
+from unittest.mock import AsyncMock, Mock, patch
+
+import pytest
 
 
 from hosts.tui.app import HostsManagerApp
 from hosts.core.models import HostEntry, HostsFile
 from hosts.core.parser import HostsParser
 from hosts.core.config import Config
-from hosts.core.manager import HostsManager
+from hosts.core.dns import DNSResolution, DNSResolutionStatus
 
 
 class TestHostsManagerApp:
     """Test cases for the HostsManagerApp class."""
 
+    @staticmethod
+    def fail_saves(app: HostsManagerApp) -> None:
+        """Configure a TUI app with an isolated failed-persistence boundary."""
+        app.manager.save_hosts_file = Mock(return_value=(False, "Permission denied"))
+        app.table_handler.populate_entries_table = Mock()
+        app.table_handler.move_cursor_to_entry_index = Mock()
+        app.details_handler.update_entry_details = Mock()
+        app.update_status = Mock()
+
     def test_app_initialization(self):
         """Test application initialization."""
         with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
@@ -537,8 +549,7 @@ class TestHostsManagerApp:
             app.update_status = Mock()
 
             serialized_files = []
-            app.manager = Mock(spec=HostsManager)
-            app.manager.enter_edit_mode.return_value = (True, "Edit mode enabled")
+            app.manager.enter_edit_mode = Mock(return_value=(True, "Edit mode enabled"))
 
             def toggle_entry(hosts_file, index):
                 hosts_file.toggle_entry(index)
@@ -548,8 +559,8 @@ class TestHostsManagerApp:
                 serialized_files.append(HostsParser().serialize(hosts_file))
                 return True, "Hosts file saved successfully"
 
-            app.manager.execute_toggle_command.side_effect = toggle_entry
-            app.manager.save_hosts_file.side_effect = save_hosts_file
+            app.manager.execute_toggle_command = Mock(side_effect=toggle_entry)
+            app.manager.save_hosts_file = Mock(side_effect=save_hosts_file)
 
             app.action_sort_by_ip()
             assert app.edit_mode is False
@@ -1138,20 +1149,16 @@ class TestHostsManagerApp:
             entry = HostEntry(ip_address="192.168.1.1", hostnames=["test.local"])
             app.hosts_file.add_entry(entry)
             app.manager.execute_toggle_command(app.hosts_file, 0)
-            app.manager.save_hosts_file = Mock(
-                return_value=(False, "Permission denied")
-            )
-            app.table_handler.populate_entries_table = Mock()
-            app.details_handler.update_entry_details = Mock()
-            app.update_status = Mock()
+            self.fail_saves(app)
 
             app.action_undo()
 
-            assert entry.is_active is False
+            assert app.hosts_file.entries[0].is_active is False
             assert app.manager.can_undo()
             assert not app.manager.can_redo()
-            app.table_handler.populate_entries_table.assert_not_called()
-            app.details_handler.update_entry_details.assert_not_called()
+            app.table_handler.populate_entries_table.assert_called_once()
+            app.table_handler.move_cursor_to_entry_index.assert_called_once_with(0)
+            app.details_handler.update_entry_details.assert_called_once()
             app.update_status.assert_called_once_with(
                 "❌ Undo save failed; previous state restored: Permission denied"
             )
@@ -1190,22 +1197,449 @@ class TestHostsManagerApp:
             app.hosts_file.add_entry(entry)
             app.manager.execute_toggle_command(app.hosts_file, 0)
             app.manager.undo_last_operation(app.hosts_file)
+            self.fail_saves(app)
+
+            app.action_redo()
+
+            assert app.hosts_file.entries[0].is_active is True
+            assert not app.manager.can_undo()
+            assert app.manager.can_redo()
+            app.table_handler.populate_entries_table.assert_called_once()
+            app.table_handler.move_cursor_to_entry_index.assert_called_once_with(0)
+            app.details_handler.update_entry_details.assert_called_once()
+            app.update_status.assert_called_once_with(
+                "❌ Redo save failed; previous state restored: Permission denied"
+            )
+
+    def test_toggle_save_failure_restores_model_selection_and_history(self):
+        """A failed toggle restores all user-visible and undoable state."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            app.hosts_file = HostsFile(
+                entries=[
+                    HostEntry(ip_address="192.0.2.1", hostnames=["one.test"]),
+                    HostEntry(ip_address="192.0.2.2", hostnames=["two.test"]),
+                ]
+            )
+            app.selected_entry_index = 0
+            app.manager.execute_toggle_command(app.hosts_file, 1)
+            app.manager.undo_last_operation(app.hosts_file)
+            self.fail_saves(app)
+
+            app.action_toggle_entry()
+
+            assert app.hosts_file.entries[0].is_active is True
+            assert app.selected_entry_index == 0
+            assert not app.manager.can_undo()
+            assert app.manager.can_redo()
+            app.update_status.assert_called_once_with(
+                "❌ Toggle save failed; previous state restored: Permission denied"
+            )
+
+    def test_move_save_failure_restores_entry_order_and_selection(self):
+        """A failed move leaves the selected Host Entry in its original position."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            app.hosts_file = HostsFile(
+                entries=[
+                    HostEntry(ip_address="192.0.2.1", hostnames=["one.test"]),
+                    HostEntry(ip_address="192.0.2.2", hostnames=["two.test"]),
+                ]
+            )
+            app.selected_entry_index = 1
+            self.fail_saves(app)
+
+            app.action_move_entry_up()
+
+            assert [entry.hostnames[0] for entry in app.hosts_file.entries] == [
+                "one.test",
+                "two.test",
+            ]
+            assert app.selected_entry_index == 1
+            assert not app.manager.can_undo()
+            app.update_status.assert_called_once_with(
+                "❌ Move save failed; previous state restored: Permission denied"
+            )
+
+    def test_add_save_failure_removes_entry_and_restores_history(self):
+        """A failed add restores the prior entries and history."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            existing = HostEntry(ip_address="192.0.2.1", hostnames=["one.test"])
+            added = HostEntry(ip_address="192.0.2.2", hostnames=["two.test"])
+            app.hosts_file = HostsFile(entries=[existing])
+            app.selected_entry_index = 0
+            self.fail_saves(app)
+            app.push_screen = Mock(
+                side_effect=lambda _screen, callback: callback(added)
+            )
+
+            app.action_add_entry()
+
+            assert [entry.hostnames[0] for entry in app.hosts_file.entries] == [
+                "one.test"
+            ]
+            assert app.selected_entry_index == 0
+            assert not app.manager.can_undo()
+            app.update_status.assert_called_once_with(
+                "❌ Add save failed; previous state restored: Permission denied"
+            )
+
+    def test_later_mutation_does_not_persist_failed_add(self):
+        """A later successful save excludes a previously failed addition."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            existing = HostEntry(ip_address="192.0.2.1", hostnames=["one.test"])
+            added = HostEntry(ip_address="192.0.2.2", hostnames=["two.test"])
+            app.hosts_file = HostsFile(entries=[existing])
             app.manager.save_hosts_file = Mock(
                 return_value=(False, "Permission denied")
             )
             app.table_handler.populate_entries_table = Mock()
+            app.table_handler.move_cursor_to_entry_index = Mock()
+            app.table_handler.restore_cursor_position = Mock()
+            app.details_handler.update_entry_details = Mock()
+            app.update_status = Mock()
+            app.set_timer = Mock()
+            app.push_screen = Mock(
+                side_effect=lambda _screen, callback: callback(added)
+            )
+            app.action_add_entry()
+
+            persisted = []
+
+            def save_hosts_file(hosts_file):
+                persisted.append(HostsParser().serialize(hosts_file))
+                return True, "Hosts file saved"
+
+            app.manager.save_hosts_file = Mock(side_effect=save_hosts_file)
+            app.action_toggle_entry()
+
+            assert len(persisted) == 1
+            assert "one.test" in persisted[0]
+            assert "two.test" not in persisted[0]
+
+    def test_save_failure_restores_exact_duplicate_selection(self):
+        """Rollback keeps the second of two identical Host Entries selected."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            app.hosts_file = HostsFile(
+                entries=[
+                    HostEntry(ip_address="192.0.2.1", hostnames=["same.test"]),
+                    HostEntry(ip_address="192.0.2.1", hostnames=["same.test"]),
+                ]
+            )
+            app.selected_entry_index = 1
+            app.manager.save_hosts_file = Mock(
+                return_value=(False, "Permission denied")
+            )
+            table = Mock(row_count=2)
+            app.query_one = Mock(return_value=table)
+            app.table_handler.populate_entries_table = Mock()
             app.details_handler.update_entry_details = Mock()
             app.update_status = Mock()
 
-            app.action_redo()
+            app.action_toggle_entry()
 
-            assert entry.is_active is True
+            assert app.selected_entry_index == 1
+            table.move_cursor.assert_called_once_with(row=1)
+
+    def test_delete_save_failure_restores_entry_and_selection(self):
+        """A failed delete restores the removed Host Entry and selection."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            app.hosts_file = HostsFile(
+                entries=[
+                    HostEntry(ip_address="192.0.2.1", hostnames=["one.test"]),
+                    HostEntry(ip_address="192.0.2.2", hostnames=["two.test"]),
+                ]
+            )
+            app.selected_entry_index = 1
+            self.fail_saves(app)
+            app.push_screen = Mock(side_effect=lambda _screen, callback: callback(True))
+
+            app.action_delete_entry()
+
+            assert [entry.hostnames[0] for entry in app.hosts_file.entries] == [
+                "one.test",
+                "two.test",
+            ]
+            assert app.selected_entry_index == 1
             assert not app.manager.can_undo()
-            assert app.manager.can_redo()
-            app.table_handler.populate_entries_table.assert_not_called()
-            app.details_handler.update_entry_details.assert_not_called()
             app.update_status.assert_called_once_with(
-                "❌ Redo save failed; previous state restored: Permission denied"
+                "❌ Delete save failed; previous state restored: Permission denied"
+            )
+
+    def test_entry_editor_save_failure_restores_all_entry_fields(self):
+        """A failed editor save restores ordinary fields and DNS metadata."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            resolved_at = datetime(2026, 9, 4, 10, 30)
+            app.hosts_file = HostsFile(
+                entries=[
+                    HostEntry(
+                        ip_address="192.0.2.1",
+                        hostnames=["one.test", "alias.test"],
+                        comment="original",
+                        is_active=False,
+                        dns_name="source.test",
+                        resolved_ip="192.0.2.1",
+                        last_resolved=resolved_at,
+                        dns_resolution_status="resolved",
+                    )
+                ]
+            )
+            app.selected_entry_index = 0
+            app.entry_edit_mode = True
+            app.manager.save_hosts_file = Mock(
+                return_value=(False, "Permission denied")
+            )
+            app.table_handler.populate_entries_table = Mock()
+            app.table_handler.move_cursor_to_entry_index = Mock()
+            app.update_status = Mock()
+
+            ip_input = Mock(value="198.51.100.8")
+            hostname_input = Mock(value="changed.test")
+            comment_input = Mock(value="changed")
+            active_checkbox = Mock(value=True)
+            dns_input = Mock(value="")
+            ip_radio = Mock(id="edit-ip-entry-radio", value=True)
+            dns_radio = Mock(id="edit-dns-entry-radio", value=False)
+            radio_set = Mock(pressed_button=ip_radio)
+            widgets = {
+                "#entry-details-display": Mock(),
+                "#entry-edit-form": Mock(),
+                "#ip-input": ip_input,
+                "#hostname-input": hostname_input,
+                "#comment-input": comment_input,
+                "#active-checkbox": active_checkbox,
+                "#dns-name-input": dns_input,
+                "#edit-entry-type-radio": radio_set,
+                "#edit-ip-entry-radio": ip_radio,
+                "#edit-dns-entry-radio": dns_radio,
+                "#edit-ip-section": Mock(),
+                "#edit-dns-section": Mock(),
+            }
+            app.query_one = Mock(side_effect=lambda selector, *_args: widgets[selector])
+            app.set_timer = Mock(side_effect=lambda _delay, callback: callback())
+
+            saved = app.edit_handler.validate_and_save_entry_changes()
+
+            entry = app.hosts_file.entries[0]
+            assert saved is False
+            assert entry.ip_address == "192.0.2.1"
+            assert entry.hostnames == ["one.test", "alias.test"]
+            assert entry.comment == "original"
+            assert entry.is_active is False
+            assert entry.dns_name == "source.test"
+            assert entry.resolved_ip == "192.0.2.1"
+            assert entry.last_resolved == resolved_at
+            assert entry.dns_resolution_status == "resolved"
+            assert ip_input.value == "192.0.2.1"
+            assert hostname_input.value == "one.test, alias.test"
+            assert comment_input.value == "original"
+            assert active_checkbox.value is False
+            assert dns_input.value == "source.test"
+            app.update_status.assert_called_once_with(
+                "❌ Edit save failed; previous state restored: Permission denied"
+            )
+
+    @pytest.mark.asyncio
+    async def test_batch_dns_save_failure_restores_mapping_and_metadata(self):
+        """A failed batch DNS refresh restores every changed DNS field."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            original_time = datetime(2026, 9, 3, 8, 0)
+            resolved_time = datetime(2026, 9, 4, 11, 0)
+            app.hosts_file = HostsFile(
+                entries=[
+                    HostEntry(
+                        ip_address="192.0.2.1",
+                        hostnames=["one.test"],
+                        dns_name="source.test",
+                        resolved_ip="192.0.2.1",
+                        last_resolved=original_time,
+                        dns_resolution_status="match",
+                    )
+                ]
+            )
+            app.selected_entry_index = 0
+            app.dns_service.resolve_entry_async = AsyncMock(
+                return_value=DNSResolution(
+                    hostname="source.test",
+                    resolved_ip="198.51.100.9",
+                    status=DNSResolutionStatus.RESOLVED,
+                    resolved_at=resolved_time,
+                )
+            )
+            self.fail_saves(app)
+            workers = []
+            app.run_worker = Mock(
+                side_effect=lambda worker, **_kwargs: workers.append(worker)
+            )
+
+            app.action_refresh_dns()
+            await workers[0]
+
+            entry = app.hosts_file.entries[0]
+            assert entry.ip_address == "192.0.2.1"
+            assert entry.resolved_ip == "192.0.2.1"
+            assert entry.last_resolved == original_time
+            assert entry.dns_resolution_status == "match"
+            assert app.selected_entry_index == 0
+            app.update_status.assert_any_call(
+                "❌ DNS refresh save failed; previous state restored: Permission denied"
+            )
+
+    @pytest.mark.asyncio
+    async def test_single_dns_save_failure_restores_mapping_and_metadata(self):
+        """A failed selected-entry DNS refresh restores every changed field."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            original_time = datetime(2026, 9, 3, 8, 0)
+            app.hosts_file = HostsFile(
+                entries=[
+                    HostEntry(
+                        ip_address="192.0.2.1",
+                        hostnames=["one.test"],
+                        dns_name="source.test",
+                        resolved_ip="192.0.2.1",
+                        last_resolved=original_time,
+                        dns_resolution_status="match",
+                    )
+                ]
+            )
+            app.dns_service.resolve_entry_async = AsyncMock(
+                return_value=DNSResolution(
+                    hostname="source.test",
+                    resolved_ip="198.51.100.9",
+                    status=DNSResolutionStatus.RESOLVED,
+                    resolved_at=datetime(2026, 9, 4, 11, 0),
+                )
+            )
+            self.fail_saves(app)
+            workers = []
+            app.run_worker = Mock(
+                side_effect=lambda worker, **_kwargs: workers.append(worker)
+            )
+
+            app.action_update_single_dns()
+            await workers[0]
+
+            entry = app.hosts_file.entries[0]
+            assert entry.ip_address == "192.0.2.1"
+            assert entry.resolved_ip == "192.0.2.1"
+            assert entry.last_resolved == original_time
+            assert entry.dns_resolution_status == "match"
+            app.update_status.assert_any_call(
+                "❌ DNS refresh save failed; previous state restored: Permission denied"
+            )
+
+    @pytest.mark.asyncio
+    async def test_single_dns_refresh_updates_selected_duplicate(self):
+        """DNS refresh retains the selected entry identity across its await."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            app.hosts_file = HostsFile(
+                entries=[
+                    HostEntry(
+                        ip_address="192.0.2.1",
+                        hostnames=["same.test"],
+                        dns_name="source.test",
+                    ),
+                    HostEntry(
+                        ip_address="192.0.2.2",
+                        hostnames=["same.test"],
+                        dns_name="source.test",
+                    ),
+                ]
+            )
+            app.selected_entry_index = 1
+            app.dns_service.resolve_entry_async = AsyncMock(
+                return_value=DNSResolution(
+                    hostname="source.test",
+                    resolved_ip="198.51.100.9",
+                    status=DNSResolutionStatus.RESOLVED,
+                    resolved_at=datetime(2026, 9, 4, 11, 0),
+                )
+            )
+            app.manager.save_hosts_file = Mock(return_value=(True, "Saved"))
+            app.table_handler.populate_entries_table = Mock()
+            app.table_handler.restore_cursor_position = Mock()
+            app.details_handler.update_entry_details = Mock()
+            app.update_status = Mock()
+            workers = []
+            app.run_worker = Mock(
+                side_effect=lambda worker, **_kwargs: workers.append(worker)
+            )
+
+            app.action_update_single_dns()
+            await workers[0]
+
+            assert app.hosts_file.entries[0].ip_address == "192.0.2.1"
+            assert app.hosts_file.entries[1].ip_address == "198.51.100.9"
+
+    @pytest.mark.asyncio
+    async def test_new_entry_dns_save_failure_keeps_saved_placeholder(self):
+        """Failed post-add DNS persistence keeps the already-saved DNS placeholder."""
+        with patch("hosts.tui.app.HostsParser"), patch("hosts.tui.app.Config"):
+            app = HostsManagerApp()
+            app.edit_mode = True
+            app.manager.edit_mode = True
+            app.hosts_file = HostsFile()
+            entry = HostEntry(
+                ip_address="0.0.0.0",
+                hostnames=["one.test"],
+                is_active=False,
+                dns_name="source.test",
+            )
+            app.manager.execute_add_command(app.hosts_file, entry)
+            app.dns_service.resolve_entry_async = AsyncMock(
+                return_value=DNSResolution(
+                    hostname="source.test",
+                    resolved_ip="198.51.100.9",
+                    status=DNSResolutionStatus.RESOLVED,
+                    resolved_at=datetime(2026, 9, 4, 11, 0),
+                )
+            )
+            self.fail_saves(app)
+            workers = []
+            app.run_worker = Mock(
+                side_effect=lambda worker, **_kwargs: workers.append(worker)
+            )
+
+            app._resolve_new_dns_entry(entry)
+            await workers[0]
+
+            restored = app.hosts_file.entries[0]
+            assert restored.ip_address == "0.0.0.0"
+            assert restored.resolved_ip is None
+            assert restored.last_resolved is None
+            assert restored.dns_resolution_status is None
+            assert restored.is_active is False
+            assert app.manager.can_undo()
+            app.update_status.assert_any_call(
+                "❌ DNS activation save failed; previous state restored: Permission denied"
             )
 
     def test_main_function(self):

tests/test_manager.py

@@ -556,6 +556,28 @@ class TestHostsManager:
             assert not success
             assert "No sudo permissions" in message
 
+    def test_save_mutation_failure_restores_hosts_file_and_history(self):
+        """The manager owns rollback of model and undo/redo state."""
+        manager = HostsManager()
+        manager.edit_mode = True
+        hosts_file = HostsFile(
+            entries=[HostEntry("192.0.2.1", ["one.test"], is_active=True)]
+        )
+        manager.execute_toggle_command(hosts_file, 0)
+        manager.undo_last_operation(hosts_file)
+        state = manager.capture_mutation_state(hosts_file)
+
+        manager.execute_toggle_command(hosts_file, 0)
+        manager.save_hosts_file = Mock(return_value=(False, "Permission denied"))
+
+        success, message, restored_hosts_file = manager.save_mutation(hosts_file, state)
+
+        assert success is False
+        assert message == "Permission denied"
+        assert restored_hosts_file.entries[0].is_active is True
+        assert not manager.can_undo()
+        assert manager.can_redo()
+
     @patch("subprocess.run")
     def test_restore_backup_success(self, mock_run):
         """Test restoring backup successfully."""