Skip to content

Fix #1472: bound the mailbox escalation-toast dedupe set - #1484

Open
mohidmakhdoomi wants to merge 10 commits into
mainfrom
builder/bugfix-1472
Open

Fix #1472: bound the mailbox escalation-toast dedupe set#1484
mohidmakhdoomi wants to merge 10 commits into
mainfrom
builder/bugfix-1472

Conversation

@mohidmakhdoomi

Copy link
Copy Markdown
Collaborator

Summary

The VSCode mailbox escalation toast deduped "already shown" escalations with a seen Set that only ever grew — one permanent entry per escalated mailbox row for the lifetime of the extension host. It is now bounded, evicting on the mailbox row leaving the escalated set.

Fixes #1472

Root Cause

apps/vscode/src/notifications/mailbox-escalation-toast.tsconst seen = new Set<string>() is activation-scoped, and seen.add(payload.mailboxId) was its only mutation: no delete, no clear, anywhere in the module. Add-only container + extension-host lifetime = unbounded growth. Negligible per entry, but it never drains.

Fix

Evict on the mailbox row leaving the escalated set — the same signal that would let a legitimate re-escalation re-notify — mirroring how activateGateToasts prunes a builder that leaves the blocked set:

  • The toast now takes the OverviewCache (already in scope at the single call site in extension.ts) and subscribes to onDidChange. OverviewData carries no per-row mailbox ids, only the workspace-level mailboxEscalated flag, so false — nothing held in this workspace is escalated — is the finest-grained "left the escalated set" signal available client-side. At that point every id in the set has left it, so the set is dropped whole. A null/absent cache read says nothing about the escalated set and never prunes.
  • MAX_SEEN = 500 with oldest-first eviction (a Set iterates in insertion order) backstops the pathological window that stays escalated all day and so never observes that false.

No protocol or server change; the module and its call site are the whole surface.

Two notes checked against the server rather than assumed:

  • db/mailbox.ts sets escalated = 1 once, guarded by escalated = 0, and never resets it; terminal rows are pruned. So the same mailboxId cannot legitimately re-escalate — eviction here is purely about memory, never about suppressing a notification.
  • The prune cannot fire on a stale snapshot: OverviewCache.refresh() is last-write-wins by sequence, and the escalation SSE event itself triggers a refresh whose request starts after Tower flagged the row, so an older in-flight mailboxEscalated: false response can never commit after it.

Test Plan

  • Regression test added — 4 new cases in src/__tests__/mailbox-escalation-toast.test.ts: an id re-toasts after de-escalation; the cap evicts oldest-first while the workspace never de-escalates; no prune while still escalated; no prune on an empty cache. Verified failing without the fix (2 failed / 10 passed with the eviction neutered) and passing with it (12/12).
  • Build passes — pnpm check-types clean; porch build check green.
  • All tests pass — 72 files / 850 tests in the vscode unit suite. pnpm lint reports one pre-existing warning in the unrelated src/commands/tunnel.ts.

🤖 Generated with Claude Code

mohidmakhdoomi and others added 7 commits August 17, 2026 19:03
The `seen` Set in `activateMailboxEscalationToasts` was add-only and
activation-scoped: one permanent entry per escalated mailbox row for the
lifetime of the extension host.

Evict on the row leaving the escalated set — the same signal that would let
a legitimate re-escalation re-notify, mirroring how `activateGateToasts`
prunes a builder that leaves the blocked set. The overview exposes no
per-row mailbox ids, only the workspace-level `mailboxEscalated` flag, so
`false` (nothing in this workspace is escalated) is the finest-grained
signal available client-side: at that point every id in the set has left it,
and the set is dropped whole. A MAX_SEEN=500 oldest-first cap backstops a
window that stays escalated all day and never observes that `false`.

Regression tests pin both paths (verified failing without the fix): an id
re-toasts after de-escalation, and the cap evicts oldest-first while the
workspace never de-escalates. Also covered: no prune while still escalated,
and no prune on an empty cache.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
CMAP non-blocking finding (claude lane): Tower also reports
`mailboxEscalated: false` when it cannot read the mailbox, so the doc
comment's "no held row is escalated" was marginally stronger than the
server guarantees. Say so, and why it is harmless — a row escalates
exactly once server-side and there is no SSE replay, so an early prune
leaves no second event to dedupe against.

Also commits the builder thread with the CMAP verdicts and the disposition
of each non-blocking note.

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

Copy link
Copy Markdown
Collaborator Author

Architect integration review (risk tier: Low — isolated VSCode module)

Verdict: APPROVE (pending maintainer review — we are not maintainers; maintainer merges.)

Verified directly against the diff:

  • Root cause is real and the fix is complete. Pre-PR seen Set at mailbox-escalation-toast.ts:30 had zero delete/clear paths; this PR adds both the semantic eviction (clear on mailboxEscalated: false via OverviewCache.onDidChange) and a MAX_SEEN=500 oldest-first backstop for a continuously-escalated window.
  • Eviction key matches the tracking issue's guidance (Tracking: mailbox-first send follow-ups — gate correctness, interrupt semantics, attribution, visibility (post-1313) #1483 workstream D): the row leaving the escalated set is the same signal that would legitimize a re-notification. Mirrors the gate-toast.ts pruning precedent, so the two toast modules now age their dedupe state the same way.
  • Server-side one-shot escalation semantics (escalated=1 guarded by escalated=0, never reset) means whole-set clearing on the workspace-level flag cannot cause duplicate toasts — verified the builder's analysis against db/mailbox.ts.
  • Stale-snapshot race is closed by OverviewCache last-write-wins sequencing; the no-data guard correctly treats an empty cache as no signal.
  • One-argument signature change, one call site (extension.ts:1522); four targeted regression tests cover both eviction paths, the still-escalated case, and the empty-cache guard.
  • Interlock with Serializer convergence: route mailbox write edge through submitToSession (serialize gated deliveries vs interrupt/escape) #1365 (workstream B): that issue concerns write-path locking, not escalation semantics — no coupling risk to this eviction key.

Builder CMAP: gemini/codex/claude all APPROVE. No findings to address.

Parked for maintainer approval + merge.

mohidmakhdoomi and others added 3 commits August 17, 2026 19:24
The pr gate was approved and porch reports protocol complete. Porch's
final merge task is deliberately skipped: a maintainer merges this PR,
not the builder. GitHub concurs — REVIEW_REQUIRED / BLOCKED.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VSCode: bound the mailbox escalation-toast seen Set (dedupe by mailboxId with eviction)

1 participant