fix(P2): peer add rollback, helper failure recovery, manager extraction (A2/A3/A5)
A3 — Peer add atomicity: track firewall_applied flag and call
clear_peer_rules() during rollback so partial peer-add failures
don't leave stale iptables rules behind. Added test.
A2 — Pending config flag: instead of clearing before spawning the
helper container (fire-and-forget), set applying=True and let the
helper clear it on success by writing to cell_config.json via a
mounted /app/data volume. On API restart after a failed apply,
_recover_pending_apply() resets the applying flag so the UI shows
pending changes and the user can retry. GET /api/config/pending now
includes the applying field.
A5 (foundation) — Extract all manager instantiation into managers.py.
app.py re-exports every name so existing test patches (patch('app.X'))
continue to work unchanged. 1021 unit tests pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -65,18 +65,22 @@ class TestConfigApplyRoute(unittest.TestCase):
|
||||
data = json.loads(r.data)
|
||||
self.assertTrue(data.get('restart_in_progress'))
|
||||
|
||||
# ── Pending state cleared after apply ──────────────────────────────────
|
||||
# ── Pending state marked "applying" after apply (not immediately cleared) ─
|
||||
|
||||
@patch('threading.Thread')
|
||||
@patch('docker.from_env')
|
||||
def test_apply_clears_pending_state(self, mock_docker, mock_thread):
|
||||
def test_apply_sets_applying_flag(self, mock_docker, mock_thread):
|
||||
mock_docker.side_effect = Exception('no docker in test')
|
||||
# Don't actually start the thread so we don't need subprocess
|
||||
mock_thread.return_value = MagicMock()
|
||||
_set_pending_restart(['config changed'], ['*'])
|
||||
self.client.post('/api/config/apply')
|
||||
pending = config_manager.configs.get('_pending_restart', {})
|
||||
self.assertFalse(pending.get('needs_restart', False))
|
||||
# The route now marks needs_restart=True + applying=True instead of clearing
|
||||
# immediately. The helper container clears the flag on success; if the helper
|
||||
# fails, needs_restart stays set so the UI continues showing pending changes.
|
||||
self.assertTrue(pending.get('needs_restart', False))
|
||||
self.assertTrue(pending.get('applying', False))
|
||||
|
||||
# ── needs_network_recreate=True → helper script includes 'down' ────────
|
||||
|
||||
|
||||
@@ -370,3 +370,50 @@ def test_delete_nonexistent_peer_returns_gracefully(admin_client, mock_peer_regi
|
||||
r = _delete_peer(admin_client, 'nobody')
|
||||
# Route must not 500 when the peer simply doesn't exist
|
||||
assert r.status_code in (200, 404)
|
||||
|
||||
|
||||
# ── POST /api/peers — firewall rollback (A3) ──────────────────────────────────
|
||||
|
||||
def test_create_peer_rolls_back_firewall_on_dns_failure(
|
||||
auth_mgr, mock_email_mgr, mock_calendar_mgr,
|
||||
mock_file_mgr, mock_wg_mgr, mock_peer_registry):
|
||||
"""If apply_all_dns_rules raises after firewall rules were applied, the peer
|
||||
add must call clear_peer_rules to undo the firewall state (A3 fix)."""
|
||||
app.config['TESTING'] = True
|
||||
app.config['SECRET_KEY'] = 'test-secret'
|
||||
|
||||
mock_fw = MagicMock()
|
||||
mock_fw.apply_peer_rules.return_value = True
|
||||
mock_fw.apply_all_dns_rules.side_effect = RuntimeError('CoreDNS unreachable')
|
||||
|
||||
patches = [
|
||||
patch('app.auth_manager', auth_mgr),
|
||||
patch('app.email_manager', mock_email_mgr),
|
||||
patch('app.calendar_manager', mock_calendar_mgr),
|
||||
patch('app.file_manager', mock_file_mgr),
|
||||
patch('app.wireguard_manager', mock_wg_mgr),
|
||||
patch('app.peer_registry', mock_peer_registry),
|
||||
patch('app.firewall_manager', mock_fw),
|
||||
]
|
||||
try:
|
||||
import auth_routes
|
||||
patches.append(patch.object(auth_routes, 'auth_manager', auth_mgr, create=True))
|
||||
except (ImportError, AttributeError):
|
||||
pass
|
||||
|
||||
started = [p.start() for p in patches]
|
||||
try:
|
||||
with app.test_client() as client:
|
||||
r = _login(client)
|
||||
assert r.status_code == 200
|
||||
resp = _post_peer(client)
|
||||
assert resp.status_code == 500, (
|
||||
f'expected 500 on DNS failure but got {resp.status_code}'
|
||||
)
|
||||
# Firewall rules must be cleared as part of rollback
|
||||
mock_fw.clear_peer_rules.assert_called_once()
|
||||
# Registry entry must also be rolled back
|
||||
mock_peer_registry.remove_peer.assert_called_once()
|
||||
finally:
|
||||
for p in patches:
|
||||
p.stop()
|
||||
|
||||
Reference in New Issue
Block a user