fix: resolve all Cell Identity banner and cert issues

Four bugs fixed:

1. Banner delay (up to 5 s): DraftConfigContext now exposes isDirty as
   reactive useState so App.jsx re-renders immediately when any section
   marks itself dirty, instead of waiting for the next checkPending() poll.

2. Banner re-triggers after Apply (race): For non-'*' container restarts
   (e.g., cell_name → DNS restart) the background thread took ~300 ms to
   clear _pending_restart. A concurrent checkPending() poll could see
   needs_restart=True and overwrite the frontend's optimistic clear.
   Fix: set needs_restart=False and applying=True synchronously before
   spawning the thread.

3. Apply showed banner during applyPending() when hasDirty()==false:
   setApplyStatus('saving') was skipped for the auto-save-then-apply
   path, leaving applyStatus=null while applyPending() ran and the
   banner stayed visible. Always set 'saving' before applyPending().

4. Cert status always 'unknown' in pic_ngo mode: _check_cert_via_ssl
   connected to cell-caddy:443 but sent SNI='cell-caddy'. Caddy finds no
   matching cert and returns nothing. Fix: pass the effective public
   domain (e.g. pic1.pic.ngo) as SNI so Caddy returns the right cert.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

api/caddy_manager.py

@@ -634,7 +634,16 @@ class CaddyManager(BaseServiceManager):
         else:
             caddy_host = os.environ.get('CADDY_CERT_HOST', 'cell-caddy')
             caddy_port = int(os.environ.get('CADDY_HTTPS_PORT', '443'))
-            result = self._check_cert_via_ssl(caddy_host, caddy_port)
+            # Use the effective domain as TLS SNI so Caddy serves the right
+            # certificate.  Without this, Caddy receives SNI='cell-caddy' which
+            # matches no cert and the handshake returns nothing.
+            sni = None
+            if self.config_manager:
+                try:
+                    sni = self.config_manager.get_effective_domain()
+                except Exception:
+                    pass
+            result = self._check_cert_via_ssl(caddy_host, caddy_port, sni=sni)
             status = result if result is not None else {
                 'status': 'unknown', 'expiry': None, 'days_remaining': None
             }
@@ -649,14 +658,18 @@ class CaddyManager(BaseServiceManager):
         return status
 
     @staticmethod
-    def _check_cert_via_ssl(hostname: str, port: int = 443) -> Optional[Dict[str, Any]]:
+    def _check_cert_via_ssl(hostname: str, port: int = 443, sni: str = None) -> Optional[Dict[str, Any]]:
-        """Open an SSL connection and return cert expiry info, or None on failure."""
+        """Open an SSL connection and return cert expiry info, or None on failure.
+
+        Connect to hostname:port but present sni (if given) as the TLS server
+        name so Caddy returns the right certificate for the public domain.
+        """
         ctx = _ssl.create_default_context()
         ctx.check_hostname = False
         ctx.verify_mode = _ssl.CERT_NONE
         try:
             with _socket.create_connection((hostname, port), timeout=5) as raw:
-                with ctx.wrap_socket(raw, server_hostname=hostname) as tls:
+                with ctx.wrap_socket(raw, server_hostname=sni or hostname) as tls:
                     der = tls.getpeercert(binary_form=True)
             if not der:
                 return None

api/routes/config.py

@@ -792,6 +792,12 @@ def apply_pending_config():
                     + (' (network_recreate)' if needs_network_recreate else '')
                 )
         else:
+            # Clear needs_restart immediately so frontend polls don't see stale
+            # state while the container restart runs in the background thread.
+            config_manager.configs['_pending_restart']['needs_restart'] = False
+            config_manager.configs['_pending_restart']['applying'] = True
+            config_manager._save_all_configs()
+
             def _do_apply():
                 import time as _time
                 import subprocess as _subprocess

tests/test_caddy_manager.py

@@ -412,6 +412,45 @@ class TestRefreshCertStatus(unittest.TestCase):
         # Should have been persisted to identity
         mgr.config_manager.set_identity_field.assert_called_with('tls', expected)
 
+    def test_refresh_cert_status_uses_effective_domain_as_sni(self):
+        """refresh_cert_status passes the effective domain as SNI, not the container hostname.
+
+        Without this, Caddy receives SNI='cell-caddy' which matches no certificate
+        and the SSL handshake returns nothing, leaving cert status as 'unknown'.
+        """
+        mgr = _mgr(identity={'cell_name': 'pic1', 'domain_mode': 'pic_ngo'})
+        mgr.config_manager.get_effective_domain.return_value = 'pic1.pic.ngo'
+        expected = {'status': 'valid', 'expiry': '2026-12-01T00:00:00+00:00', 'days_remaining': 179}
+        with patch.object(CaddyManager, '_check_cert_via_ssl', return_value=expected) as mock_ssl:
+            mgr.refresh_cert_status()
+        # The SNI keyword argument must be the effective domain, not the container name.
+        call_kwargs = mock_ssl.call_args
+        sni_passed = call_kwargs.kwargs.get('sni') or (
+            call_kwargs.args[2] if len(call_kwargs.args) > 2 else None
+        )
+        self.assertEqual(sni_passed, 'pic1.pic.ngo',
+                         f'Expected SNI=pic1.pic.ngo but got {sni_passed!r}')
+
+    def test_check_cert_via_ssl_passes_sni_to_wrap_socket(self):
+        """_check_cert_via_ssl uses sni parameter as server_hostname in SSL handshake."""
+        der = self._make_der_cert(60)
+        mock_tls = MagicMock()
+        mock_tls.__enter__ = MagicMock(return_value=mock_tls)
+        mock_tls.__exit__ = MagicMock(return_value=False)
+        mock_tls.getpeercert.return_value = der
+        mock_raw = MagicMock()
+        mock_raw.__enter__ = MagicMock(return_value=mock_raw)
+        mock_raw.__exit__ = MagicMock(return_value=False)
+        with patch('caddy_manager._socket.create_connection', return_value=mock_raw) as mock_conn:
+            with patch('caddy_manager._ssl.create_default_context') as mock_ctx:
+                mock_ctx.return_value.wrap_socket.return_value = mock_tls
+                CaddyManager._check_cert_via_ssl('cell-caddy', 443, sni='pic1.pic.ngo')
+        # TCP connects to container hostname, SSL handshake uses the public domain
+        mock_conn.assert_called_with(('cell-caddy', 443), timeout=5)
+        mock_ctx.return_value.wrap_socket.assert_called_with(
+            mock_raw, server_hostname='pic1.pic.ngo'
+        )
+
     def test_refresh_cert_status_ssl_failure_returns_unknown(self):
         """When SSL check returns None, status is 'unknown'."""
         mgr = _mgr(identity={'cell_name': 'alpha', 'domain_mode': 'pic_ngo'})

tests/test_config_apply.py

@@ -179,6 +179,45 @@ class TestConfigApplyRoute(unittest.TestCase):
         self.assertIn('-d', cmd)
         self.assertIn('dns', cmd)
 
+    # ── Race-condition fix: needs_restart cleared synchronously ────────────
+    # For non-'*' container restarts the background thread takes ~300 ms.
+    # The frontend polls /api/config/pending every 5 s; if needs_restart is
+    # still True when that poll fires, the banner re-appears after Apply.
+    # Fix: set needs_restart=False and applying=True before spawning the thread.
+
+    @patch('threading.Thread')
+    @patch('docker.from_env')
+    def test_specific_containers_clears_needs_restart_synchronously(
+            self, mock_docker, mock_thread):
+        """needs_restart must be False as soon as apply returns, not after thread."""
+        mock_docker.side_effect = Exception('no docker in test')
+        mock_thread.return_value = MagicMock()  # thread is mocked — never runs
+        _set_pending_restart(['cell_name changed to pic2'], ['dns'])
+
+        self.client.post('/api/config/apply')
+
+        pending = config_manager.configs.get('_pending_restart', {})
+        self.assertFalse(pending.get('needs_restart', True),
+                         'needs_restart must be False immediately after apply for non-* restarts')
+        self.assertTrue(pending.get('applying', False),
+                        'applying must be True while the background thread runs')
+
+    @patch('threading.Thread')
+    @patch('docker.from_env')
+    def test_wildcard_containers_sets_applying_but_not_clears_needs_restart(
+            self, mock_docker, mock_thread):
+        """For '*' restarts the helper container clears the flag; API must not."""
+        mock_docker.side_effect = Exception('no docker in test')
+        mock_thread.return_value = MagicMock()
+        _set_pending_restart(['ip_range changed'], ['*'])
+
+        self.client.post('/api/config/apply')
+
+        pending = config_manager.configs.get('_pending_restart', {})
+        # Wildcard restart: API sets applying=True but leaves needs_restart=True
+        # so the helper container can clear it on success.
+        self.assertTrue(pending.get('applying', False))
+
     # ── Exception in route body returns 500 ───────────────────────────────
 
     @patch('app.config_manager')

webui/src/App.jsx

@@ -188,18 +188,17 @@ function AppCore() {
   const [applyStatus, setApplyStatus] = useState(null); // null | 'saving' | 'restarting' | 'done' | 'timeout' | 'error'
   const [applyError, setApplyError] = useState('');
 
-  const { flushAll, hasDirty, clearAllDirty } = useDraftConfig();
+  const { flushAll, hasDirty, isDirty, clearAllDirty } = useDraftConfig();
 
   const handleApply = useCallback(async () => {
     setApplyError('');
-    if (hasDirty()) {
     setApplyStatus('saving');
     try {
-        await flushAll();
+      if (hasDirty()) await flushAll();
     } catch {
       // flush errors are shown via Settings toasts; continue with apply
     }
-    }
+    clearAllDirty();
     try {
       await cellAPI.applyPending();
     } catch (err) {
@@ -228,7 +227,7 @@ function AppCore() {
     setApplyStatus('timeout');
     setApplyError('Containers may still be starting — check docker logs if services are unavailable');
     setTimeout(() => setApplyStatus(null), 8000);
-  }, [flushAll, hasDirty]);
+  }, [flushAll, hasDirty, clearAllDirty]);
 
   const handleCancel = useCallback(async () => {
     clearAllDirty();
@@ -328,7 +327,7 @@ function AppCore() {
                       </div>
                     )}
 
-                    {isOnline && (pending.needs_restart || hasDirty()) && !applyStatus && (
+                    {isOnline && (pending.needs_restart || isDirty) && !applyStatus && (
                       <PendingRestartBanner pending={pending} onApply={handleApply} onCancel={handleCancel} />
                     )}
 

webui/src/__tests__/Settings.IdentitySave.test.jsx

@@ -51,7 +51,9 @@ vi.mock('../contexts/DraftConfigContext', () => ({
     registerFlusher: vi.fn(() => vi.fn()),
     setDirty: (...a) => mockSetDirty(...a),
     hasDirty: vi.fn(() => false),
+    isDirty: false,
     flushAll: vi.fn(),
+    clearAllDirty: vi.fn(),
   }),
 }));
 
@@ -230,3 +232,53 @@ describe('Cell Identity — Accept path (saveIdentity called by flusher)', () =>
     expect(mockUpdateConfig.mock.calls[0][0]).toMatchObject({ cell_name: 'pic2' });
   });
 });
+
+describe('Cell Identity — DraftConfig dirty state is set synchronously on change', () => {
+  /**
+   * Verifies that draftConfig.setDirty is called in the same synchronous
+   * event handler as the identity state change, not in a deferred effect.
+   * This is what allows the banner to appear immediately (the isDirty reactive
+   * state in DraftConfigContext re-renders App.jsx without waiting for a poll).
+   */
+  beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); });
+  afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); });
+
+  it('setDirty("identity", true) is called before any timer fires after cell_name change', async () => {
+    await renderSettings();
+    mockSetDirty.mockClear();
+
+    fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } });
+
+    // setDirty must be called synchronously within the event, not in a timer
+    expect(mockSetDirty).toHaveBeenCalledWith('identity', true);
+    expect(mockSetDirty).toHaveBeenCalledTimes(1);
+  });
+
+  it('setDirty("identity", true) is called before any timer fires after ip_range change', async () => {
+    await renderSettings();
+    mockSetDirty.mockClear();
+
+    fireEvent.change(screen.getByDisplayValue('172.20.0.0/16'), { target: { value: '10.0.0.0/8' } });
+
+    expect(mockSetDirty).toHaveBeenCalledWith('identity', true);
+    expect(mockSetDirty).toHaveBeenCalledTimes(1);
+  });
+
+  it('setDirty("identity", false) is called after saveIdentity completes', async () => {
+    mockGetConfig
+      .mockResolvedValueOnce({ data: makeCfg() })
+      .mockResolvedValue({ data: makeCfg({ ip_range: '10.0.0.0/8' }) });
+    await renderSettings();
+    mockSetDirty.mockClear();
+
+    fireEvent.change(screen.getByDisplayValue('172.20.0.0/16'), { target: { value: '10.0.0.0/8' } });
+
+    // auto-save fires after 800 ms
+    await act(async () => { vi.advanceTimersByTime(900); });
+    await act(async () => { await Promise.resolve(); });
+
+    const calls = mockSetDirty.mock.calls;
+    expect(calls.some(([k, v]) => k === 'identity' && v === true)).toBe(true);
+    expect(calls.some(([k, v]) => k === 'identity' && v === false)).toBe(true);
+  });
+});

webui/src/contexts/DraftConfigContext.jsx

@@ -1,4 +1,4 @@
-import { createContext, useContext, useRef, useCallback } from 'react';
+import { createContext, useContext, useRef, useCallback, useState } from 'react';
 
 const DraftConfigContext = createContext(null);
 
@@ -11,9 +11,12 @@ export function DraftConfigProvider({ children }) {
   }, []);
 
   const hasDirtyRef = useRef({});  // key → boolean
+  // isDirty is a reactive mirror of hasDirtyRef so consumers re-render immediately.
+  const [isDirty, setIsDirty] = useState(false);
 
-  const setDirty = useCallback((key, isDirty) => {
+  const setDirty = useCallback((key, dirty) => {
-    hasDirtyRef.current[key] = isDirty;
+    hasDirtyRef.current[key] = dirty;
+    setIsDirty(Object.values(hasDirtyRef.current).some(Boolean));
   }, []);
 
   const hasDirty = useCallback(() => {
@@ -27,10 +30,11 @@ export function DraftConfigProvider({ children }) {
 
   const clearAllDirty = useCallback(() => {
     hasDirtyRef.current = {};
+    setIsDirty(false);
   }, []);
 
   return (
-    <DraftConfigContext.Provider value={{ registerFlusher, setDirty, hasDirty, flushAll, clearAllDirty }}>
+    <DraftConfigContext.Provider value={{ registerFlusher, setDirty, hasDirty, isDirty, flushAll, clearAllDirty }}>
       {children}
     </DraftConfigContext.Provider>
   );