Conversation
Co-authored-by: ormidales <46538211+ormidales@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent multiple concurrent cleanup loops from accumulating by ensuring the periodic cleanupStaleTrackedRequests interval is de-duplicated across background script re-executions.
Changes:
- Clear any previously registered cleanup interval before creating a new one.
- Store the interval ID on
globalThisto allow reuse across script re-execution within the same global context.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
background.js
Outdated
| if (globalThis.cleanupIntervalId) { | ||
| clearInterval(globalThis.cleanupIntervalId); | ||
| } | ||
| globalThis.cleanupIntervalId = setInterval(cleanupStaleTrackedRequests, REQUEST_TRACK_TTL_MS); |
There was a problem hiding this comment.
globalThis.cleanupIntervalId is a very generic global property name; if more background scripts (or future dependencies) add their own interval tracking, this risks collisions and accidentally clearing the wrong interval. Consider namespacing the key (e.g., a project-specific prefix) and/or guarding the clearInterval call with a type check (number/object) to ensure you only clear your own interval.
| if (globalThis.cleanupIntervalId) { | |
| clearInterval(globalThis.cleanupIntervalId); | |
| } | |
| globalThis.cleanupIntervalId = setInterval(cleanupStaleTrackedRequests, REQUEST_TRACK_TTL_MS); | |
| const CLEANUP_INTERVAL_KEY = "__acceptLangExt_cleanupIntervalId"; | |
| const existingCleanupIntervalId = globalThis[CLEANUP_INTERVAL_KEY]; | |
| if (typeof existingCleanupIntervalId === "number" || typeof existingCleanupIntervalId === "object") { | |
| clearInterval(existingCleanupIntervalId); | |
| } | |
| globalThis[CLEANUP_INTERVAL_KEY] = setInterval(cleanupStaleTrackedRequests, REQUEST_TRACK_TTL_MS); |
background.js
Outdated
| if (globalThis.cleanupIntervalId) { | ||
| clearInterval(globalThis.cleanupIntervalId); | ||
| } | ||
| globalThis.cleanupIntervalId = setInterval(cleanupStaleTrackedRequests, REQUEST_TRACK_TTL_MS); |
There was a problem hiding this comment.
PR description references a MV3 Service Worker lifecycle, but this repo’s manifest.json is manifest_version: 2 with a classic background page (background.scripts). In MV2, background reloads typically destroy the old JS context (and its interval), so this change may not address the reported reproduction scenario; please confirm the issue occurs in the current MV2 runtime or update the PR description/linked issue context accordingly.
|
@copilot apply changes based on the comments in this thread |
…rInterval Co-authored-by: ormidales <46538211+ormidales@users.noreply.github.com>
Done in 874cebb. The |
Each time the background script reloads without a full context reset, a new
setIntervalloop was spawned without clearing the previous one — silently multiplying cleanup executions and leaking memory/CPU over time.Change
setIntervalregistration inbackground.jsby storing its ID onglobalThisunder a namespaced key and clearing any prior interval before creating a new one:__acceptLangExt_cleanupIntervalId) to avoid collisions with other scripts or future dependencies.typeoftype guard (numberorobject) ensures only our own interval is ever cleared, protecting against accidental interference with unrelated globals.globalThispersists across script re-executions within the same global context, ensuring exactly one cleanup loop is active regardless of how many times the script reloads.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.