refactor(t096): arch + reliability hardening sweep (15 verified fixes)

CONTEXT.md

@@ -61,7 +61,7 @@ The server-side, authoritative Slack capture modality (ADR-0011). The web server
 _Avoid_: scraper, poller, backfill.
 
 **Workspace Registry**:
-The server-side persistence (`slack-workspaces.json`) mapping each Slack `teamId` → `{ url, name, enterpriseId, lastSeen }`, populated the first time a workspace tab is seen live (ADR-0011). Persisting `enterpriseId` lets a cold start resolve each workspace's **Grid Group** without live creds. It drives two things: re-extraction targets for stale creds, and the **Parked Tab** keep-alive loop, which ensures exactly one tab per registered workspace exists on the remote browser (recreated via `/json/new` if closed or after a browser restart) so creds self-refresh and the hijack stays armed. Distinct from ADR-0010 **Workspaces** (multi-CDP-host UI), though both stamp entries with a workspace key.
+The server-side persistence (`slack-workspaces.json`) mapping each Slack `teamId` → `{ url, name, enterpriseId, lastSeen }`, populated the first time a workspace tab is seen live (ADR-0011). Persisting `enterpriseId` lets a cold start resolve each workspace's **Grid Group** without live creds. It drives two things: re-extraction targets for stale creds, and the **Parked Tab** keep-alive loop. The keeper ensures at least one Slack tab is live so creds self-refresh and the hijack stays armed — but it **defers to a Pin** (t098): a registered workspace whose URL is pinned is owned by its Pin and the keeper never spawns an anonymous duplicate for it (closing its tab no longer resurrects a stray). When no Slack tab is live at all, a single cred lifeline opens one tab, preferring a pinned URL, so shared creds remain available to the sweep. Per-workspace anonymous tabs are kept only for unpinned workspaces. Distinct from ADR-0010 **Workspaces** (multi-CDP-host UI), though both stamp entries with a workspace key.
 _Avoid_: account list, team store, tenant table.
 
 **Grid Group**:
@@ -108,7 +108,7 @@ _Avoid_: native tab, page view, webview tab.
 - **Viewport Transform** maps canvas coordinates to **Remote Page** coordinates for both drawing **Screencast Frames** and hit-testing **Input Forwarding**.
 - **Adaptive Viewport** (when enabled) resizes the **Remote Page** to the canvas so **Screencast Frames** fill it without letterbox bars.
 - A **Notification Side-Channel** attaches to a background **Tab**'s target and uses a **Notification Adapter** to run **Notification Capture** — independent of the **Active Tab**'s screencast socket. Clicking the result activates the owning Tab and, if the entry carries an `activate` intent, the activation registry maps it to a **Remote Page** deep-open intention.
-- For Slack, the **Slack Content Sweep** is the authoritative **Notification Capture** writer; the in-page hijack provides only an instant foreground toast. The sweep reads creds from a live **Tab**, persists workspaces in the **Workspace Registry**, keeps a **Parked Tab** alive per registered workspace, and respects the **Channel Exclude** list.
+- For Slack, the **Slack Content Sweep** is the authoritative **Notification Capture** writer; the in-page hijack provides only an instant foreground toast. The sweep reads creds from a live **Tab**, persists workspaces in the **Workspace Registry**, uses the **Parked Tab** keeper (pin-deferred since t098 — a pinned workspace is owned by its Pin), and respects the **Channel Exclude** list.
 - A **Local Tab** renders a real local web page (in-DOM `<webview>`) alongside CDP Tabs — it does not use **Screencast Frames** or **Input Forwarding**; it gets direct device access instead.
 
 ## Example dialogue

core/notifications-sidechain.js

@@ -32,6 +32,14 @@ const { tsCmp } = require("./slack-sweep")
 
 const NOTIFY_BINDING = "__cdpNotify"
 const DEFAULT_CAP = 200
+// An awaitable side-channel CDP call (cred extraction) gets a timeout so a stalled socket
+// frees its promise instead of hanging forever (t096, P4).
+const CDP_CALL_TIMEOUT_MS = 10_000
+// A side-channel whose target is still live but never reaches OPEN (hung CONNECTING — no open,
+// no close, no error) is reaped after this and re-attached on the next reconcile (t096, P3).
+// Reconcile runs every ~5s and a healthy local CDP socket opens in well under a second, so a
+// still-non-OPEN socket past this threshold is genuinely stuck.
+const SIDECHANNEL_STALE_MS = 15_000
 
 // Notification Adapters identify notification-capable sites by URL hostname. Each
 // names its capture script (loaded via the injected `readInject`) and the icon to
@@ -97,6 +105,8 @@ function createNotificationCenter(deps) {
   // since the sweep can't cover them.
   const sweepDisabledTeams = new Set()
   const log = deps.log || (() => {})
+  const now = deps.now || (() => Date.now())
+  const WS_OPEN = WebSocketCtor && WebSocketCtor.OPEN != null ? WebSocketCtor.OPEN : 1
 
   function recordCreds(team, cookie) {
     const prev = credsByTeam.get(team.teamId) || {}
@@ -187,18 +197,33 @@ function createNotificationCenter(deps) {
     const adapter = adapterFor(target.url)
     if (!adapter || !target.webSocketDebuggerUrl) return
     const ws = new WebSocketCtor(target.webSocketDebuggerUrl)
+    ws.__attachedAt = now()
     sideChannels.set(target.id, ws)
     let cmdId = 1
     // Fire-and-forget CDP send (capture-script injection doesn't need the reply).
     const cdp = (method, params) =>
       ws.send(JSON.stringify({ id: cmdId++, method, params: params || {} }))
     // Awaitable CDP call — cred extraction needs the evaluate/getCookies results, so it
-    // correlates the reply by command id through `pending`.
+    // correlates the reply by command id through `pending`. A timeout rejects a call whose
+    // reply never arrives (stalled socket), and `drop` rejects any in-flight call on
+    // close/error — so a dead socket never leaves a promise hanging (t096, P4).
     const pending = new Map()
     const cdpCall = (method, params) =>
-      new Promise((resolve) => {
+      new Promise((resolve, reject) => {
         const id = cmdId++
-        pending.set(id, resolve)
+        const timer = setTimeout(() => {
+          if (pending.delete(id)) reject(new Error(`cdp ${method} timed out`))
+        }, CDP_CALL_TIMEOUT_MS)
+        pending.set(id, {
+          resolve: (v) => {
+            clearTimeout(timer)
+            resolve(v)
+          },
+          reject: (e) => {
+            clearTimeout(timer)
+            reject(e)
+          },
+        })
         ws.send(JSON.stringify({ id, method, params: params || {} }))
       })
     ws.on("open", () => {
@@ -217,7 +242,7 @@ function createNotificationCenter(deps) {
       try {
         const msg = JSON.parse(data.toString())
         if (msg.id != null && pending.has(msg.id)) {
-          pending.get(msg.id)(msg)
+          pending.get(msg.id).resolve(msg)
           pending.delete(msg.id)
           return
         }
@@ -228,6 +253,9 @@ function createNotificationCenter(deps) {
     })
     const drop = () => {
       if (sideChannels.get(target.id) === ws) sideChannels.delete(target.id)
+      // Free any in-flight awaitable call so a stalled socket can't leak its promise.
+      for (const p of pending.values()) p.reject(new Error("side-channel closed"))
+      pending.clear()
     }
     ws.on("close", drop)
     ws.on("error", drop)
@@ -274,7 +302,14 @@ function createNotificationCenter(deps) {
     const matched = list.filter((t) => t.type === "page" && adapterFor(t.url))
     const liveIds = new Set(matched.map((t) => t.id))
     for (const [id, ws] of sideChannels) {
-      if (!liveIds.has(id)) {
+      // Reap a vanished/changed target's socket, OR a socket on a still-live target that is
+      // stuck below OPEN past the stale threshold (hung CONNECTING — no open/close/error fires,
+      // so it would otherwise sit unreaped and unre-attached; t096, P3).
+      const stale =
+        liveIds.has(id) &&
+        ws.readyState !== WS_OPEN &&
+        now() - (ws.__attachedAt || 0) > SIDECHANNEL_STALE_MS
+      if (!liveIds.has(id) || stale) {
         try {
           ws.close()
         } catch {}

core/notifications-sidechain.test.ts

@@ -672,3 +672,63 @@ describe("load + close", () => {
     expect(FakeWs.instances.every((w) => w.closed)).toBe(true)
   })
 })
+
+// A socket that never reaches OPEN — models a hung CONNECTING side-channel.
+class HungWs extends FakeWs {
+  readyState = 0
+}
+
+describe("reconcile — reap hung side-channel (t096, P3)", () => {
+  it("reaps a non-OPEN socket on a still-live target past the stale window and re-attaches", async () => {
+    const { center, setNow } = makeCenter({ WebSocketCtor: HungWs as any })
+    setNow(1_000)
+    await center.reconcile([teamsTarget()])
+    expect(FakeWs.instances).toHaveLength(1)
+    const hung = FakeWs.instances[0]
+
+    setNow(1_000 + 15_000 + 1) // past SIDECHANNEL_STALE_MS
+    await center.reconcile([teamsTarget()])
+
+    expect(hung.closed).toBe(true)
+    expect(FakeWs.instances).toHaveLength(2) // re-attached
+  })
+
+  it("does not reap a freshly-attached non-OPEN socket within the stale window", async () => {
+    const { center, setNow } = makeCenter({ WebSocketCtor: HungWs as any })
+    setNow(1_000)
+    await center.reconcile([teamsTarget()])
+
+    setNow(1_000 + 5_000) // before the stale threshold
+    await center.reconcile([teamsTarget()])
+
+    expect(FakeWs.instances).toHaveLength(1)
+    expect(FakeWs.instances[0].closed).toBe(false)
+  })
+
+  it("does not reap an OPEN socket on a live target", async () => {
+    const { center, setNow } = makeCenter() // default FakeWs is OPEN (readyState 1)
+    setNow(1_000)
+    await center.reconcile([teamsTarget()])
+    setNow(1_000 + 60_000)
+    await center.reconcile([teamsTarget()])
+    expect(FakeWs.instances).toHaveLength(1)
+    expect(FakeWs.instances[0].closed).toBe(false)
+  })
+})
+
+describe("side-channel cdpCall reject-on-close (t096, P4)", () => {
+  it("rejects an in-flight cred extraction when the socket closes — no creds, no hang", async () => {
+    const onCreds = vi.fn()
+    const { center } = makeCenter({ onCreds })
+    await center.reconcile([slackTarget()])
+    const ws = FakeWs.instances[0]
+    ws.open() // fires extractSlackCreds → sends the localConfig read, awaits its reply
+
+    ws.close() // reply never comes — drop() must reject the pending call
+    await Promise.resolve()
+    await Promise.resolve()
+
+    expect(center.listCreds()).toEqual([])
+    expect(onCreds).not.toHaveBeenCalled()
+  })
+})

core/paint-ack-pacer.js

@@ -0,0 +1,28 @@
+// Adaptive paint-ack watchdog window (t096, P2).
+//
+// The stranded-paint watchdog (web/server.mjs) frees the one-in-flight slot and re-acks the
+// remote if a supporting client never acks a painted frame. A FIXED window trips early on a
+// device that legitimately paints slower than it — degrading that device to eager self-ack and
+// re-introducing the stale-frame backlog the paint-ack gate exists to prevent. This tracks an
+// EWMA of observed paint-ack latency (markSent → client ack) and sizes the window to a multiple
+// of it, never below a floor and never above a cap: a fast link keeps the tight floor, a
+// genuinely-slow device gets the slack it needs, and a pathological sample can't run away.
+//
+// Pure: no timers, no clock — the server measures the latency and owns the setTimeout. Tested
+// by paint-ack-pacer.test.ts.
+
+function createPaintAckPacer({ floorMs = 1000, factor = 3, capMs = 5000, alpha = 0.3 } = {}) {
+  let ewma = null
+  return {
+    record(latencyMs) {
+      if (!(latencyMs >= 0)) return // ignore negative / NaN
+      ewma = ewma === null ? latencyMs : alpha * latencyMs + (1 - alpha) * ewma
+    },
+    windowMs() {
+      if (ewma === null) return floorMs
+      return Math.max(floorMs, Math.min(capMs, Math.round(factor * ewma)))
+    },
+  }
+}
+
+module.exports = { createPaintAckPacer }

core/paint-ack-pacer.test.ts

@@ -0,0 +1,42 @@
+import { describe, expect, it } from "vitest"
+// CommonJS shared core (ADR-0008): adaptive paint-ack watchdog window.
+import { createPaintAckPacer } from "./paint-ack-pacer"
+
+describe("createPaintAckPacer", () => {
+  it("returns the floor before any sample", () => {
+    expect(createPaintAckPacer().windowMs()).toBe(1000)
+  })
+
+  it("stays at the floor for fast paints", () => {
+    const p = createPaintAckPacer()
+    p.record(50)
+    expect(p.windowMs()).toBe(1000) // 3 * 50 < floor
+  })
+
+  it("grows to a multiple of the EWMA for slow paints", () => {
+    const p = createPaintAckPacer({ alpha: 1 }) // alpha 1 → EWMA tracks the last sample
+    p.record(500)
+    expect(p.windowMs()).toBe(1500) // 3 * 500
+  })
+
+  it("caps the window for pathologically slow paints", () => {
+    const p = createPaintAckPacer({ alpha: 1 })
+    p.record(10_000)
+    expect(p.windowMs()).toBe(5000)
+  })
+
+  it("smooths samples via the EWMA", () => {
+    const p = createPaintAckPacer({ alpha: 0.5, floorMs: 0 })
+    p.record(1000) // EWMA = 1000
+    p.record(2000) // EWMA = 0.5*2000 + 0.5*1000 = 1500
+    expect(p.windowMs()).toBe(4500) // 3 * 1500
+  })
+
+  it("ignores negative / NaN latencies", () => {
+    const p = createPaintAckPacer({ alpha: 1 })
+    p.record(600) // window 1800
+    p.record(-5)
+    p.record(Number.NaN)
+    expect(p.windowMs()).toBe(1800)
+  })
+})

core/slack-workspaces.js

@@ -44,15 +44,37 @@ function liveTeamIds(targets) {
 }
 
 // Which registered workspaces need a parked tab created: those with no live tab and not
-// created within the cooldown. `createdAt` maps teamId → last create timestamp. Pure.
-function planParkedTabs(registry, live, createdAt, now) {
+// created within the cooldown. `createdAt` maps teamId → last create timestamp.
+//
+// `pinUrlByTeam` (t098) maps a pinned workspace's teamId → its pin URL. A pinned workspace
+// is considered OWNED BY ITS PIN: the keeper never spawns an anonymous duplicate for it
+// (closing its tab no longer resurrects a stray). Capture is unaffected because one live
+// Slack tab refreshes creds for ALL workspaces and the sweep polls each over the web API
+// regardless of which tab is live. So per-workspace tabs aren't needed — only one live tab
+// is. When NO Slack tab is live and nothing else would open one, a single cred lifeline
+// plan keeps one alive, preferring a pinned URL (so it adopts into the pin on next reload).
+// Omitting `pinUrlByTeam` preserves the prior per-workspace behavior. Pure.
+function planParkedTabs(registry, live, createdAt, now, pinUrlByTeam = {}) {
+  const offCooldown = (teamId) => {
+    const last = createdAt[teamId]
+    return !(last && now - last < CREATE_COOLDOWN_MS)
+  }
   const plans = []
   for (const teamId of Object.keys(registry)) {
     if (live.has(teamId)) continue
-    const last = createdAt[teamId]
-    if (last && now - last < CREATE_COOLDOWN_MS) continue
+    if (pinUrlByTeam[teamId]) continue // pin owns it — don't reopen
+    if (!offCooldown(teamId)) continue
     plans.push({ teamId, url: registry[teamId].url })
   }
+  // Cred lifeline: nothing live and nothing else planned → keep exactly one Slack tab alive
+  // via a pinned workspace, so shared creds keep refreshing. Cooldown-gated; one is enough.
+  if (live.size === 0 && plans.length === 0) {
+    for (const teamId of Object.keys(pinUrlByTeam)) {
+      if (!offCooldown(teamId)) continue
+      plans.push({ teamId, url: pinUrlByTeam[teamId] })
+      break
+    }
+  }
   return plans
 }
 

core/slack-workspaces.test.ts

@@ -83,4 +83,61 @@ describe("planParkedTabs — recreate registered workspaces with no live tab", (
     const plans = planParkedTabs(reg, new Set(["T1"]), { T2: 4000 }, 40000)
     expect(plans).toEqual([{ teamId: "T2", url: CLIENT("T2") }])
   })
+
+  it("omitting pinUrlByTeam is byte-identical to the prior behavior", () => {
+    expect(planParkedTabs(reg, new Set(["T1"]), {}, 5000)).toEqual([
+      { teamId: "T2", url: CLIENT("T2") },
+    ])
+  })
+})
+
+describe("planParkedTabs — defers to a pinned workspace (t098)", () => {
+  const reg = {
+    T1: { teamId: "T1", url: CLIENT("T1"), name: "A", lastSeen: 1 },
+    T2: { teamId: "T2", url: CLIENT("T2"), name: "B", lastSeen: 1 },
+  }
+  const PIN = (team: string) => `https://app.slack.com/client/${team}/CPIN`
+
+  it("skips a registered workspace that has a pin — the pin owns it", () => {
+    // T1 live, T2 not live but pinned → no reopen for T2.
+    const plans = planParkedTabs(reg, new Set(["T1"]), {}, 5000, { T2: PIN("T2") })
+    expect(plans).toEqual([])
+  })
+
+  it("still plans an unpinned workspace alongside a pinned one", () => {
+    // T1 pinned (skip), T2 unpinned + not live → only T2 reopens.
+    const plans = planParkedTabs(reg, new Set(), {}, 5000, { T1: PIN("T1") })
+    expect(plans).toEqual([{ teamId: "T2", url: CLIENT("T2") }])
+  })
+
+  it("cred lifeline: opens one pinned workspace (at the pin URL) when nothing is live and nothing else is planned", () => {
+    // Both pinned, neither live → no normal plan, but the lifeline opens exactly one at its pin URL.
+    const plans = planParkedTabs(reg, new Set(), {}, 5000, { T1: PIN("T1"), T2: PIN("T2") })
+    expect(plans).toHaveLength(1)
+    expect(plans[0]).toEqual({ teamId: "T1", url: PIN("T1") })
+  })
+
+  it("no lifeline when a Slack tab is already live", () => {
+    const plans = planParkedTabs(reg, new Set(["T1"]), {}, 5000, { T1: PIN("T1"), T2: PIN("T2") })
+    expect(plans).toEqual([])
+  })
+
+  it("no lifeline when an unpinned plan already keeps a tab alive", () => {
+    // T1 pinned, T2 unpinned + not live → T2's normal plan covers cred-refresh; no extra lifeline.
+    const plans = planParkedTabs(reg, new Set(), {}, 5000, { T1: PIN("T1") })
+    expect(plans).toEqual([{ teamId: "T2", url: CLIENT("T2") }])
+  })
+
+  it("cred lifeline respects the create cooldown", () => {
+    // Only T1 registered + pinned, not live, but just created at t=4000 (cooldown) → no lifeline.
+    const oneReg = { T1: reg.T1 }
+    const plans = planParkedTabs(oneReg, new Set(), { T1: 4000 }, 5000, { T1: PIN("T1") })
+    expect(plans).toEqual([])
+  })
+
+  it("cred lifeline bootstraps from a pin even when the workspace is not yet registered", () => {
+    // Fresh start: empty registry, no live tab, a pin exists → open it to seed creds.
+    const plans = planParkedTabs({}, new Set(), {}, 5000, { T9: PIN("T9") })
+    expect(plans).toEqual([{ teamId: "T9", url: PIN("T9") }])
+  })
 })