From 255f9e2576a3e1e9931c46a540b356fdcf6c3394 Mon Sep 17 00:00:00 2001 From: Dmitrii Date: Wed, 22 Apr 2026 13:59:52 -0400 Subject: [PATCH] fix: port changes now correctly queue pending restart for all services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs fixed: 1. calendar_manager and wireguard_manager (port-only) called _restart_container immediately in apply_config, bypassing the pending restart banner and restarting the container before the docker port binding in .env was updated — leaving the service broken until the banner was applied manually. apply_config now only updates the config file (radicale.conf / wg0.conf); the docker compose restart happens via the banner as intended. 2. Port change detection in update_config used `if old_val is not None` to guard against triggering on unchanged values. When a service's port was never explicitly saved (first time), old_val was None, so the pending restart was never queued. Fix: fall back to PORT_DEFAULTS[key] so the comparison is always against the effective current value. Add TestPortChangeDetection (5 tests) covering first-save and multi-service accumulation cases. Co-Authored-By: Claude Sonnet 4.6 --- api/app.py | 10 +++--- api/calendar_manager.py | 6 ++-- api/wireguard_manager.py | 10 ++++-- tests/test_pending_restart.py | 62 +++++++++++++++++++++++++++++++++++ 4 files changed, 79 insertions(+), 9 deletions(-) diff --git a/api/app.py b/api/app.py index 7ffba4c..9e74f3c 100644 --- a/api/app.py +++ b/api/app.py @@ -538,11 +538,13 @@ def update_config(): port_changed_containers = set() port_change_messages = [] + import ip_utils as _ip_utils_pcd for (svc_key, field), (_env_key, containers) in _PORT_CHANGE_MAP.items(): if svc_key in data and field in data[svc_key]: - old_val = old_svc_configs.get(svc_key, {}).get(field) + default_val = _ip_utils_pcd.PORT_DEFAULTS.get(_env_key) + old_val = old_svc_configs.get(svc_key, {}).get(field, default_val) new_val = data[svc_key][field] - if old_val is not None and old_val != new_val: + if old_val != new_val: port_changed_containers.update(containers) port_change_messages.append( f'{svc_key} {field}: {old_val} → {new_val}' @@ -550,9 +552,9 @@ def update_config(): # wireguard_port in identity also drives WG_PORT env var; sync to service config if 'wireguard_port' in identity_updates: - old_wg = old_identity.get('wireguard_port') + old_wg = old_identity.get('wireguard_port', _ip_utils_pcd.PORT_DEFAULTS.get('wg_port', 51820)) new_wg = identity_updates['wireguard_port'] - if old_wg is not None and old_wg != new_wg: + if old_wg != new_wg: # Sync to wireguard service config and update wg0.conf _wg_svc = config_manager.configs.get('wireguard', {}) _wg_svc['port'] = new_wg diff --git a/api/calendar_manager.py b/api/calendar_manager.py index 60951b6..ee6dd2c 100644 --- a/api/calendar_manager.py +++ b/api/calendar_manager.py @@ -478,7 +478,7 @@ class CalendarManager(BaseServiceManager): f.write(config_content) def apply_config(self, config: Dict[str, Any]) -> Dict[str, Any]: - """Update radicale config port and restart cell-radicale.""" + """Update radicale config file. Port changes go through pending restart (docker binding).""" restarted = [] warnings = [] if 'port' not in config: @@ -494,8 +494,8 @@ class CalendarManager(BaseServiceManager): ] with open(radicale_conf, 'w') as f: f.writelines(lines) - self._restart_container('cell-radicale') - restarted.append('cell-radicale') + # No immediate restart — docker port binding must be updated first. + # The pending restart banner will run docker compose up with updated .env. except Exception as e: warnings.append(f"radicale config update failed: {e}") return {'restarted': restarted, 'warnings': warnings} diff --git a/api/wireguard_manager.py b/api/wireguard_manager.py index 141fdff..4590dac 100644 --- a/api/wireguard_manager.py +++ b/api/wireguard_manager.py @@ -225,21 +225,27 @@ class WireGuardManager(BaseServiceManager): return result changed = False + port_only_change = True if 'port' in config and config['port']: lines = _set_iface_field(lines, 'ListenPort', config['port']) changed = True if 'address' in config and config['address']: lines = _set_iface_field(lines, 'Address', config['address']) changed = True + port_only_change = False if 'private_key' in config and config['private_key']: lines = _set_iface_field(lines, 'PrivateKey', config['private_key']) changed = True + port_only_change = False if changed: with open(cf, 'w') as f: f.writelines(lines) - self._restart_container('cell-wireguard') - restarted.append('cell-wireguard') + # Port-only changes: docker binding must be updated first via pending restart. + # Non-port changes (address, private_key) can restart immediately. + if not port_only_change: + self._restart_container('cell-wireguard') + restarted.append('cell-wireguard') except Exception as e: warnings.append(f"wg0.conf update failed: {e}") logger.error(f"apply_config error: {e}") diff --git a/tests/test_pending_restart.py b/tests/test_pending_restart.py index cab4cf1..e981c9d 100644 --- a/tests/test_pending_restart.py +++ b/tests/test_pending_restart.py @@ -209,5 +209,67 @@ class TestCancelPendingEndpoint(unittest.TestCase): self.assertEqual(data['changes'], []) +class TestPortChangeDetection(unittest.TestCase): + """Test that port changes always trigger pending restart, even on first save.""" + + def setUp(self): + app.config['TESTING'] = True + self.client = app.test_client() + _clear_pending_restart() + # Remove any stored service configs so we start clean + for key in ('calendar', 'files', 'wireguard', 'network', 'email'): + config_manager.configs.pop(key, None) + + def tearDown(self): + _clear_pending_restart() + for key in ('calendar', 'files', 'wireguard', 'network', 'email'): + config_manager.configs.pop(key, None) + + def _put_config(self, payload): + return self.client.put('/api/config', + data=json.dumps(payload), + content_type='application/json') + + def test_calendar_port_first_save_marks_pending(self): + """First-time calendar port save should still queue pending restart.""" + r = self._put_config({'calendar': {'port': 5233}}) + self.assertEqual(r.status_code, 200) + p = config_manager.configs.get('_pending_restart', {}) + self.assertTrue(p.get('needs_restart'), 'pending restart not set on first calendar port save') + self.assertIn('radicale', p.get('containers', [])) + + def test_files_port_first_save_marks_pending(self): + """First-time files (webdav) port save should queue pending restart.""" + r = self._put_config({'files': {'port': 8181}}) + self.assertEqual(r.status_code, 200) + p = config_manager.configs.get('_pending_restart', {}) + self.assertTrue(p.get('needs_restart')) + self.assertIn('webdav', p.get('containers', [])) + + def test_files_manager_port_first_save_marks_pending(self): + r = self._put_config({'files': {'manager_port': 9090}}) + self.assertEqual(r.status_code, 200) + p = config_manager.configs.get('_pending_restart', {}) + self.assertTrue(p.get('needs_restart')) + self.assertIn('filegator', p.get('containers', [])) + + def test_multiple_service_port_changes_accumulate_containers(self): + """Saving two services should accumulate both containers in pending.""" + self._put_config({'calendar': {'port': 5233}}) + self._put_config({'files': {'port': 8181}}) + p = config_manager.configs.get('_pending_restart', {}) + self.assertTrue(p.get('needs_restart')) + containers = p.get('containers', []) + self.assertIn('radicale', containers) + self.assertIn('webdav', containers) + + def test_same_port_as_default_no_pending(self): + """Saving the default port value should NOT trigger pending restart.""" + r = self._put_config({'calendar': {'port': 5232}}) # 5232 is default + self.assertEqual(r.status_code, 200) + p = config_manager.configs.get('_pending_restart', {}) + self.assertFalse(p.get('needs_restart', False)) + + if __name__ == '__main__': unittest.main()