fix(updater): address Qodo review on tier 4

- UpdatePage: only show "deferred until" subtitle when scheduledFor
  actually matches nextWindowOpensAt. The previous `scheduledFor >
  now + 60s` heuristic misfired during a normal in-window 15-min
  grace period.
- applyPipeline: return the enriched preflight reason (`reason:
  detail`) instead of only `pf.reason`, so /admin/update/apply 409
  bodies and failure-notify emails preserve diagnostics like the
  Node engine mismatch detail.
- updater/index: key the cached nodemailer transport on the full
  set of SMTP options (host + port + secure + auth) so runtime
  changes to port/credentials via reloadSettings() invalidate
  the cache.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: JohnMcLear

admin/src/pages/UpdatePage.tsx

@@ -192,12 +192,17 @@ export const UpdatePage = () => {
               values={{tag: scheduled.targetTag, remaining: fmtRemaining(remainingMs)}}
             />
           </p>
-          {/* Tier 4: when the next fire is gated by a maintenance window, the
-              backend persists `scheduledFor` to the next opening. Surface that
-              so the admin understands why the countdown is long. */}
+          {/* Tier 4: only surface the deferral subtitle when `scheduledFor`
+              was actually snapped forward to the next window opening. The
+              backend keeps `scheduledFor = now + grace` whenever that lands
+              inside the window, so we can't use a fixed time-distance
+              heuristic (a normal 15-min grace would falsely match). Instead,
+              compare against `nextWindowOpensAt` with a small tolerance — the
+              two are computed seconds apart at request time, so an exact-ish
+              match is the only safe signal that the schedule was deferred. */}
           {us.tier === 'autonomous' && us.nextWindowOpensAt
-              && new Date(scheduled.scheduledFor).getTime()
-                 > Date.now() + 60 * 1000 && (
+              && Math.abs(new Date(scheduled.scheduledFor).getTime()
+                          - new Date(us.nextWindowOpensAt).getTime()) < 60 * 1000 && (
             <p className="update-scheduled-deferred">
               <Trans
                 i18nKey="update.page.scheduled.deferred_until"

src/node/updater/applyPipeline.ts

@@ -1,11 +1,11 @@
 import {UpdateState} from './types';
-import {PreflightResult, PreflightReason} from './preflight';
+import {PreflightResult} from './preflight';
 import {ExecutorResult} from './UpdateExecutor';
 import {Drainer, DrainBroadcastKey} from './SessionDrainer';
 
 export type ApplyOutcome =
   | {outcome: 'pending-verification'}
-  | {outcome: 'preflight-failed'; reason: PreflightReason}
+  | {outcome: 'preflight-failed'; reason: string}
   | {outcome: 'cancelled'}
   | {outcome: 'lock-held'}
   | {outcome: 'busy'; status: string}
@@ -99,7 +99,7 @@ export const applyUpdate = async (
         lastResult: {targetTag, fromSha: '', outcome: 'preflight-failed', reason: reasonStr, at},
       });
       deps.appendLog(`[${at}] PREFLIGHT_FAILED ${reasonStr}`);
-      return {outcome: 'preflight-failed', reason: pf.reason};
+      return {outcome: 'preflight-failed', reason: reasonStr};
     }
 
     // Re-load state after preflight: the cancel endpoint can flip execution

src/node/updater/index.ts

@@ -48,11 +48,31 @@ export const getDetectedInstallMethod = () => detectedMethod;
  * set; never imported when mail is disabled (keeps boot costs predictable for
  * installs that don't care about outbound mail).
  *
- * Rebuilt automatically only if the cached host doesn't match current
- * settings — a settings reload mid-process therefore picks up new mail config
- * without requiring a restart.
+ * The cache is keyed on the full set of SMTP options that `buildTransport`
+ * consumes (host, port, secure, auth). `reloadSettings()` can mutate any of
+ * these at runtime, so a host-only key would silently keep using a stale
+ * transport when an operator rotates credentials or moves to a different
+ * port without changing host.
  */
-let transportCache: {host: string; transporter: {sendMail: (m: any) => Promise<any>}} | null = null;
+let transportCache: {key: string; transporter: {sendMail: (m: any) => Promise<any>}} | null = null;
+
+/**
+ * Stable string key derived from the SMTP options `buildTransport` consumes.
+ * Exported as `_internal` so tests can verify that runtime mutations to
+ * `port`/`secure`/`auth` (without a host change) actually invalidate the
+ * cache — a regression caught by Qodo on PR #7753.
+ */
+export const smtpTransportKey = (mail: {
+  host?: string | null;
+  port?: number | string | null;
+  secure?: boolean | null;
+  auth?: unknown;
+}): string => JSON.stringify({
+  host: mail.host ?? null,
+  port: Number(mail.port) || 587,
+  secure: !!mail.secure,
+  auth: mail.auth ?? null,
+});
 
 const buildTransport = async (host: string) => {
   const {default: nodemailer} = await import('nodemailer');
@@ -73,8 +93,9 @@ const sendEmailViaSmtp = async (to: string, subject: string, body: string): Prom
     logger.info(`(would send email) to=${to} subject="${subject}"`);
     return;
   }
-  if (!transportCache || transportCache.host !== host) {
-    transportCache = {host, transporter: await buildTransport(host)};
+  const key = smtpTransportKey(settings.mail);
+  if (!transportCache || transportCache.key !== key) {
+    transportCache = {key, transporter: await buildTransport(host)};
   }
   try {
     await transportCache.transporter.sendMail({

src/tests/backend-new/specs/updater/applyPipeline.test.ts

@@ -84,6 +84,26 @@ describe('applyUpdate (extracted pipeline)', () => {
     expect(final.lastResult?.reason).toBe('low-disk-space');
   });
 
+  it('preserves the preflight detail in the returned reason (HTTP + email use the return value)', async () => {
+    // Regression: applyUpdate built `reasonStr = reason: detail` for state +
+    // logs but returned only `pf.reason`, so /admin/update/apply 409 bodies
+    // and failure-notify emails lost the engine-mismatch detail.
+    const {deps, loadState} = baseDeps();
+    deps.runPreflight = async () => ({
+      ok: false,
+      reason: 'node-engine-mismatch',
+      detail: 'target requires Node >=26.0.0, running 25.0.0',
+    });
+    const r = await applyUpdate({targetTag: 'v2.0.1', deps});
+    expect(r).toEqual({
+      outcome: 'preflight-failed',
+      reason: 'node-engine-mismatch: target requires Node >=26.0.0, running 25.0.0',
+    });
+    const final = loadState();
+    expect(final.lastResult?.reason)
+        .toBe('node-engine-mismatch: target requires Node >=26.0.0, running 25.0.0');
+  });
+
   it('returns cancelled when the post-preflight state check shows state was reset (admin cancelled mid-preflight)', async () => {
     const {deps} = baseDeps();
     // First preflight pass mutates state to 'preflight'. Then the cancel handler

src/tests/backend-new/specs/updater/smtpTransportKey.test.ts

@@ -0,0 +1,41 @@
+import {describe, it, expect} from 'vitest';
+import {smtpTransportKey} from '../../../../node/updater/index';
+
+describe('smtpTransportKey', () => {
+  // Regression for Qodo PR #7753 review: the nodemailer transport cache was
+  // invalidated only on host change. Operators rotating SMTP credentials or
+  // moving to a different port without changing host would keep using the
+  // stale transport after reloadSettings().
+
+  it('differs when port changes', () => {
+    const base = {host: 'smtp.example.com', port: 587, secure: false, auth: null};
+    expect(smtpTransportKey(base))
+        .not.toBe(smtpTransportKey({...base, port: 465}));
+  });
+
+  it('differs when secure flag changes', () => {
+    const base = {host: 'smtp.example.com', port: 587, secure: false, auth: null};
+    expect(smtpTransportKey(base))
+        .not.toBe(smtpTransportKey({...base, secure: true}));
+  });
+
+  it('differs when auth changes', () => {
+    const base = {host: 'smtp.example.com', port: 587, secure: false,
+                  auth: {user: 'a', pass: '1'}};
+    expect(smtpTransportKey(base))
+        .not.toBe(smtpTransportKey({...base, auth: {user: 'a', pass: '2'}}));
+  });
+
+  it('is stable for an unchanged config (cache hit on repeat calls)', () => {
+    const cfg = {host: 'smtp.example.com', port: 587, secure: false,
+                 auth: {user: 'a', pass: '1'}};
+    expect(smtpTransportKey(cfg)).toBe(smtpTransportKey({...cfg}));
+  });
+
+  it('falls back to port 587 when port is unset or non-numeric', () => {
+    expect(smtpTransportKey({host: 'h'}))
+        .toBe(smtpTransportKey({host: 'h', port: 587}));
+    expect(smtpTransportKey({host: 'h', port: 'not-a-number' as any}))
+        .toBe(smtpTransportKey({host: 'h', port: 587}));
+  });
+});