fix(notifications): stop Sencho version notifications from silently skipping (#594)

* fix(notifications): stop Sencho version notifications from silently skipping Three independent defects combined to make version-update notifications silently fail even while Fleet overview correctly surfaced an update button: - The in-memory 6-hour cooldown was advanced before the network fetch, so a single transient failure at boot could lock the check for the rest of the container lifetime. Moved the cooldown update inside the success branch so failures retry on the next eval cycle. - MonitorService called the raw version fetch directly, bypassing the CacheService wrapper (TTL, inflight dedup, stale-on-error) that Fleet uses, so the two paths could diverge. Unified both on a shared getLatestVersion() helper in utils/version-check.ts. - The dedup key could carry stale state from a previous build and never self-clear. It now self-heals when the running version reaches the previously-notified version, so future releases re-fire as expected. Added diagnostic logs gated on debug mode for each skip branch, plus three regression tests covering cooldown-on-failure, cooldown-on-success, and dedup self-heal. * docs(notifications): drop legacy-upgrade framing from alerts troubleshooting Sencho has not shipped publicly, so troubleshooting entries written in 'this used to happen but now does Y' mode reference a past that does not exist for any reader. Rewrote the version-notification, image-update, and crash-alert troubleshooting entries to describe current behavior positively without referring to prior builds, upgrade paths, or legacy fixes. * chore(security): accept CVE-2026-33810 in bundled Docker CLI 29.4.0 Trivy now flags CVE-2026-33810 (Go stdlib crypto/x509 DNS constraint bypass, fixed in Go 1.26.2) in the Docker CLI static binary we ship. Docker CLI 29.4.0 is the latest upstream release and still links Go 1.26.1; no newer static binary exists yet. Same exposure profile as the already-accepted CVE-2026-32280: the Docker CLI and compose plugin only validate certificates from well-known registry CAs and the local Docker socket, not from attacker-controlled CAs with crafted DNS name constraints. Revisit on the next Docker CLI release that rebuilds against Go 1.26.2 or later.
2026-08-31 12:48:10 +00:00 · 2026-04-14 16:34:28 -04:00
parent f8a7abc07b
commit 6b8c369745
6 changed files with 207 additions and 85 deletions
@@ -15,6 +15,7 @@ const { mockGetGlobalSettings, mockGetNodes, mockGetStackAlerts, mockAddContaine
  mockCurrentLoad, mockMem, mockFsSize,
  mockExecAsync,
  mockFetchLatestSenchoVersion,
+  mockGetLatestVersion,
  mockGetSenchoVersion,
 } = vi.hoisted(() => ({
  mockGetGlobalSettings: vi.fn().mockReturnValue({}),
@@ -37,6 +38,7 @@ const { mockGetGlobalSettings, mockGetNodes, mockGetStackAlerts, mockAddContaine
  mockFsSize: vi.fn().mockResolvedValue([{ mount: '/', use: 30 }]),
  mockExecAsync: vi.fn().mockResolvedValue({ stdout: '' }),
  mockFetchLatestSenchoVersion: vi.fn().mockRejectedValue(new Error('not configured')),
+  mockGetLatestVersion: vi.fn().mockResolvedValue(null),
  mockGetSenchoVersion: vi.fn().mockReturnValue(null),
 }));

@@ -70,6 +72,7 @@ vi.mock('../services/DockerController', () => ({

 vi.mock('../utils/version-check', () => ({
  fetchLatestSenchoVersion: (...args: unknown[]) => mockFetchLatestSenchoVersion(...args),
+  getLatestVersion: (...args: unknown[]) => mockGetLatestVersion(...args),
 }));

 vi.mock('../services/CapabilityRegistry', async () => {
@@ -549,13 +552,25 @@ describe('MonitorService - restart_count metric', () => {
 // ── Sencho version update check ───────────────────────────────────────

 describe('MonitorService - Sencho version check', () => {
-  it('dispatches notification when newer version available', async () => {
-    mockGetSenchoVersion.mockReturnValue('0.45.0');
-    mockFetchLatestSenchoVersion.mockResolvedValue('0.46.0');
-    mockGetSystemState.mockReturnValue(null); // No previous notification
+  /** Stateful system_state backing for tests that need getSystemState to
+   *  reflect setSystemState writes within the same evaluation. */
+  function wireStatefulSystemState(seed: Record<string, string> = {}) {
+    const store: Record<string, string> = { ...seed };
+    mockGetSystemState.mockImplementation((key: string) => store[key] ?? null);
+    mockSetSystemState.mockImplementation((key: string, value: string) => { store[key] = value; });
+    return store;
+  }
+
+  beforeEach(() => {
    mockGetGlobalSettings.mockReturnValue({});
    mockGetNodes.mockReturnValue([]);
    mockGetStackAlerts.mockReturnValue([]);
+  });
+
+  it('dispatches notification when newer version available', async () => {
+    mockGetSenchoVersion.mockReturnValue('0.45.0');
+    mockGetLatestVersion.mockResolvedValue('0.46.0');
+    mockGetSystemState.mockReturnValue(null); // No previous notification

    const svc = MonitorService.getInstance();
    // Reset the version check timer so it runs immediately
@@ -570,11 +585,9 @@ describe('MonitorService - Sencho version check', () => {

  it('does not re-notify for the same version', async () => {
    mockGetSenchoVersion.mockReturnValue('0.45.0');
-    mockFetchLatestSenchoVersion.mockResolvedValue('0.46.0');
-    mockGetSystemState.mockReturnValue('0.46.0'); // Already notified for this version
-    mockGetGlobalSettings.mockReturnValue({});
-    mockGetNodes.mockReturnValue([]);
-    mockGetStackAlerts.mockReturnValue([]);
+    mockGetLatestVersion.mockResolvedValue('0.46.0');
+    // Running version < last notified, so self-heal does NOT clear the key.
+    mockGetSystemState.mockReturnValue('0.46.0');

    const svc = MonitorService.getInstance();
    (svc as any).lastVersionCheckAt = 0;
@@ -585,10 +598,7 @@ describe('MonitorService - Sencho version check', () => {

  it('handles version check failure gracefully', async () => {
    mockGetSenchoVersion.mockReturnValue('0.45.0');
-    mockFetchLatestSenchoVersion.mockRejectedValue(new Error('Network down'));
-    mockGetGlobalSettings.mockReturnValue({});
-    mockGetNodes.mockReturnValue([]);
-    mockGetStackAlerts.mockReturnValue([]);
+    mockGetLatestVersion.mockResolvedValue(null); // CacheService failed + no stale

    const svc = MonitorService.getInstance();
    (svc as any).lastVersionCheckAt = 0;
@@ -600,29 +610,23 @@ describe('MonitorService - Sencho version check', () => {

  it('respects the 6-hour cooldown interval', async () => {
    mockGetSenchoVersion.mockReturnValue('0.45.0');
-    mockFetchLatestSenchoVersion.mockResolvedValue('0.46.0');
+    mockGetLatestVersion.mockResolvedValue('0.46.0');
    mockGetSystemState.mockReturnValue(null);
-    mockGetGlobalSettings.mockReturnValue({});
-    mockGetNodes.mockReturnValue([]);
-    mockGetStackAlerts.mockReturnValue([]);

    const svc = MonitorService.getInstance();
    // Simulate the check ran 1 hour ago (within 6-hour window)
    (svc as any).lastVersionCheckAt = Date.now() - 1 * 60 * 60 * 1000;
    await (svc as any).evaluate();

-    // fetchLatestSenchoVersion should not have been called since we're within cooldown
-    expect(mockFetchLatestSenchoVersion).not.toHaveBeenCalled();
+    // getLatestVersion should not have been called since we're within cooldown
+    expect(mockGetLatestVersion).not.toHaveBeenCalled();
  });

  it('skips version check when getSenchoVersion returns null', async () => {
    // Simulates the production-Docker scenario that previously leaked "0.0.0"
    mockGetSenchoVersion.mockReturnValue(null);
-    mockFetchLatestSenchoVersion.mockResolvedValue('0.46.0');
+    mockGetLatestVersion.mockResolvedValue('0.46.0');
    mockGetSystemState.mockReturnValue(null);
-    mockGetGlobalSettings.mockReturnValue({});
-    mockGetNodes.mockReturnValue([]);
-    mockGetStackAlerts.mockReturnValue([]);

    const svc = MonitorService.getInstance();
    (svc as any).lastVersionCheckAt = 0;
@@ -630,5 +634,64 @@ describe('MonitorService - Sencho version check', () => {

    expect(mockDispatchAlert).not.toHaveBeenCalledWith('info', expect.stringContaining('0.46.0'));
    expect(mockSetSystemState).not.toHaveBeenCalledWith('last_sencho_update_notified_version', expect.anything());
+    // Should not have even attempted the lookup.
+    expect(mockGetLatestVersion).not.toHaveBeenCalled();
+  });
+
+  // ── Regression coverage for PR: cooldown leak + dedup self-heal ───────
+
+  it('does NOT advance cooldown when getLatestVersion returns null (retries next cycle)', async () => {
+    mockGetSenchoVersion.mockReturnValue('0.45.0');
+    mockGetLatestVersion.mockResolvedValue(null);
+    mockGetSystemState.mockReturnValue(null);
+
+    const svc = MonitorService.getInstance();
+    (svc as any).lastVersionCheckAt = 0;
+
+    await (svc as any).evaluate();
+    await (svc as any).evaluate();
+
+    // Both evals should attempt the lookup since failures do not lock cooldown.
+    expect(mockGetLatestVersion).toHaveBeenCalledTimes(2);
+    expect((svc as any).lastVersionCheckAt).toBe(0);
+  });
+
+  it('DOES advance cooldown on a successful lookup (prevents re-fetch inside window)', async () => {
+    mockGetSenchoVersion.mockReturnValue('0.45.0');
+    mockGetLatestVersion.mockResolvedValue('0.46.0');
+    mockGetSystemState.mockReturnValue(null);
+
+    const svc = MonitorService.getInstance();
+    (svc as any).lastVersionCheckAt = 0;
+
+    await (svc as any).evaluate();
+    const firstCooldown = (svc as any).lastVersionCheckAt;
+    expect(firstCooldown).toBeGreaterThan(0);
+
+    // Second eval immediately after: cooldown gate should block it.
+    mockGetLatestVersion.mockClear();
+    await (svc as any).evaluate();
+
+    expect(mockGetLatestVersion).not.toHaveBeenCalled();
+    // Exactly one dispatch across both evals.
+    const availabilityDispatches = mockDispatchAlert.mock.calls.filter(
+      (args: unknown[]) => typeof args[1] === 'string' && args[1].includes('available'),
+    );
+    expect(availabilityDispatches).toHaveLength(1);
+  });
+
+  it('self-heals dedup after user upgrades to the previously-notified version', async () => {
+    // Prior notification stored "0.46.0" back when the user was on 0.45.0.
+    // User has now upgraded to 0.46.0; a new release (0.47.0) just dropped.
+    const store = wireStatefulSystemState({ last_sencho_update_notified_version: '0.46.0' });
+    mockGetSenchoVersion.mockReturnValue('0.46.0');
+    mockGetLatestVersion.mockResolvedValue('0.47.0');
+
+    const svc = MonitorService.getInstance();
+    (svc as any).lastVersionCheckAt = 0;
+    await (svc as any).evaluate();
+
+    expect(mockDispatchAlert).toHaveBeenCalledWith('info', expect.stringContaining('0.47.0'));
+    expect(store.last_sencho_update_notified_version).toBe('0.47.0');
  });
 });