Skip to content

fix(dev): use globalThis for singleton state to prevent HMR memory leaks#4869

Merged
waleedlatif1 merged 3 commits into
stagingfrom
fix/hmr-singleton-leak
Jun 3, 2026
Merged

fix(dev): use globalThis for singleton state to prevent HMR memory leaks#4869
waleedlatif1 merged 3 commits into
stagingfrom
fix/hmr-singleton-leak

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Redis client, ping interval, MCP pub/sub channels, MCP connection manager, task pub/sub, tool confirmation channel, and execution cancellation channel were all instantiated at module scope
  • Next.js HMR re-evaluates module code on every file save, creating new instances while old ones are retained in memory via setInterval/setTimeout closures — causing unbounded memory growth in dev (observed: 68GB)
  • Moved all singleton state to globalThis so it survives HMR re-evaluations and is only created once per process

Type of Change

  • Bug fix

Testing

Tested manually

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

@vercel
Copy link
Copy Markdown

vercel Bot commented Jun 3, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jun 3, 2026 8:31pm

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented Jun 3, 2026

PR Summary

Low Risk
Refactors where singletons are stored without changing connection semantics; production behavior should match prior module-scope caching, with minor risk if global keys collide across tests without reset.

Overview
Fixes unbounded dev memory growth when Next.js HMR re-runs modules on every save: module-scope singletons (Redis client, PING setInterval, pub/sub channels, MCP connection manager, rate-limit adapter cache) were recreated while old timers/clients stayed alive.

Redis (redis.ts): client, ping health-check interval, failure counters, and reconnect listeners now live on globalThis._redisState instead of module-level lets.

Pub/sub & long-lived services: copilot tool confirmation, task status, execution cancellation, MCP tools/workflow channels, and McpConnectionManager (non-test) are lazily created once per process on globalThis.

Rate limiting: cached storage adapter and the onRedisReconnect registration flag are stored on globalThis so HMR does not stack duplicate reconnect listeners or adapters.

Runtime behavior in production is unchanged; the intent is one instance per Node process, including across HMR reloads in development.

Reviewed by Cursor Bugbot for commit 138f146. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 3, 2026

Greptile Summary

This PR addresses an unbounded memory growth bug in development (observed at 68GB) caused by Next.js HMR re-evaluating module code on every file save and creating new singleton instances while old ones were held alive by setInterval/setTimeout closures. The fix migrates all affected singletons — Redis client/ping state, MCP pub/sub channels, MCP connection manager, task pub/sub, tool confirmation channel, and execution cancellation channel — from module-scoped variables to globalThis, so they survive HMR re-evaluations and are only created once per Node.js process.

  • Pattern is applied correctly across all seven files: channels that can legitimately be null use the 'key' in g sentinel to avoid treating null as "not yet initialized", while channels that are always non-null after creation use the simpler falsy check.
  • The reconnectListeners accumulation bug flagged in the previous review thread has been resolved by also moving _rlCachedAdapter and _rlReconnectListenerRegistered to globalThis in factory.ts.
  • resetForTesting in redis.ts correctly mutates the state object in-place rather than replacing it on globalThis.

Confidence Score: 5/5

Safe to merge — the change is purely a re-homing of singleton state from module scope to globalThis, with no behavioral difference in production.

All seven files apply the globalThis singleton pattern correctly and consistently. No logic changes were made to the actual Redis/pub-sub behavior, and the reconnect-listener accumulation bug from the previous review thread is confirmed resolved.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/lib/core/config/redis.ts Consolidated four module-level variables plus the reconnect listener array into a single globalThis._redisState object; resetForTesting correctly mutates in-place.
apps/sim/lib/core/rate-limiter/storage/factory.ts Moved cachedAdapter and reconnectListenerRegistered to globalThis, fixing stale-closure accumulation in reconnectListeners raised in the previous review thread.
apps/sim/lib/mcp/pubsub.ts Both MCP pub/sub channels moved to globalThis using 'in' guard (correct, since the value can be null on client side).
apps/sim/lib/mcp/connection-manager.ts Manager singleton moved to globalThis._mcpConnectionManager with correct 'in' guard (value can be null in test environments).
apps/sim/lib/copilot/tasks.ts Task pub/sub channel moved to globalThis with 'in' guard to correctly distinguish null (client side) from uninitialized.
apps/sim/lib/copilot/persistence/tool-confirm/index.ts Tool confirmation channel moved to globalThis with falsy guard (correct — channel is always a non-null object after creation).
apps/sim/lib/execution/cancellation.ts Execution cancellation channel changed from eager module-level singleton to a globalThis-backed lazy singleton; falsy guard is correct since the channel is always non-null once created.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Next.js HMR — module re-evaluated] --> B{Key already on globalThis?}
    B -- No: first process start --> C[Create singleton]
    C --> D[Store on globalThis._key]
    D --> E[Assign to module-scoped const]
    B -- Yes: subsequent HMR reload --> F[Skip creation]
    F --> G[Read existing value from globalThis._key]
    G --> E
    E --> H[Export / use singleton]
Loading

Reviews (3): Last reviewed commit: "fix(types): resolve McpConnectionManager..." | Re-trigger Greptile

Comment thread apps/sim/lib/core/config/redis.ts
@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 54d4446. Configure here.

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

@cursor review

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit 138f146. Configure here.

@waleedlatif1 waleedlatif1 merged commit 85942a5 into staging Jun 3, 2026
14 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/hmr-singleton-leak branch June 3, 2026 20:58
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.

1 participant