[NGDP-4972] fix: memory leaks with timer cleanup, destroy methods #9
Conversation
WalkthroughThe changes introduce comprehensive lifecycle management and resource cleanup patterns across the SDK. VWOBuilder gains polling control with a stopPolling() method, VWOClient and BatchEventsQueue implement destroy() methods for cleanup, and new tests validate these behaviors to prevent memory leaks and ensure proper shutdown. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant VWOClient
participant VWOBuilder
participant BatchEventsQueue
Note over App,BatchEventsQueue: Normal Operation
App->>VWOClient: build()
VWOClient->>VWOBuilder: setVWOBuilder()
Note over VWOBuilder: Polling active, timer scheduled
Note over App,BatchEventsQueue: Destroy Flow (Cleanup)
App->>VWOClient: destroy()
activate VWOClient
VWOClient->>VWOBuilder: stopPolling()
activate VWOBuilder
Note over VWOBuilder: Cancel pending timer,<br/>set isPollingActive = false
deactivate VWOBuilder
VWOClient->>BatchEventsQueue: destroy()
activate BatchEventsQueue
Note over BatchEventsQueue: Flush events, clear queue,<br/>set isDestroyed = true
deactivate BatchEventsQueue
Note over VWOClient: Clear settings & state
deactivate VWOClient
Note over App,BatchEventsQueue: Resources cleaned up
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
lib/services/BatchEventsQueue.ts (1)
35-37: Consider typingBatchEventsQueue.instanceas nullable for correctness
destroy()assignsBatchEventsQueue.instance = null, but the static field andInstancegetter are typed asBatchEventsQueueonly. Under stricter TS settings this would be a type error; consider:
private static instance: BatchEventsQueue | null;public static get Instance(): BatchEventsQueue | null { ... }This keeps the runtime behavior the same while making the API honest about the nullable singleton.
Also applies to: 254-256
lib/VWOClient.ts (1)
71-73: Destroy flow is well-structured; consider a couple of small cleanup/typing tweaksThe new
setVWOBuilder+destroyflow is coherent: it stops polling via the builder, tears downBatchEventsQueue.Instance, and clears settings with an idempotent guard. This should address the main leak scenarios around timers and the singleton queue.Two small follow-ups you might consider:
settingsis typed asSettingsModelbut assignednullindestroy(). If you ever enablestrictNullChecks, this will need to becomeSettingsModel | null(and the same for the interface).- To further reduce dangling references after destroy, you could also clear
vwoClientInstance(and possiblystorage) alongsidevwoBuilder, since they aren’t expected to be used post‑destroy.Also applies to: 83-85, 559-612
test/unit/services/BatchEventsQueue.test.ts (3)
17-19: ESLint warnings are false positives, but consider type-only import for BatchConfig.The ESLint warnings about unused imports are false positives:
SettingsServiceandLogManagerare used by the mocks on lines 34 and 22 respectivelyBatchConfigis used as a type for the configuration objects passed toBatchEventsQueueconstructor throughout the testsHowever, since
BatchConfigis only used as a type, consider using a type-only import to clarify intent:-import { BatchEventsQueue, type BatchConfig } from '../../../lib/services/BatchEventsQueue'; +import type { BatchConfig } from '../../../lib/services/BatchEventsQueue'; +import { BatchEventsQueue } from '../../../lib/services/BatchEventsQueue';Alternatively, add ESLint disable comments if the current import structure is preferred.
196-216: Consider adding direct queue size verification.The test correctly validates that the queue doesn't grow unbounded, but only indirectly (by checking that the dispatcher wasn't called). The comment on lines 208-215 acknowledges this is "internal verification."
Consider adding a more explicit assertion by exposing queue length or by testing the behavior when flushing:
// After enqueuing 1001 events with maxQueueSize 1000 // Manually flush to verify the queue contains ~901 events (not 1001) await queue.flush(); // Verify dispatcher was called with roughly 901 events expect(mockDispatcher).toHaveBeenCalledTimes(1); const calledWith = mockDispatcher.mock.calls[0][0]; expect(calledWith.length).toBeLessThan(1001); expect(calledWith.length).toBeGreaterThan(800); // Allow for 10% dropThis would make the test more explicit about what's being protected against.
344-362: Remove unnecessary timeout or add explanation.This test sets a 10-second timeout (
}, 10000);), but it uses fake timers (jest.useFakeTimers()frombeforeEach). Since fake timers allow instant time progression, this extended timeout appears unnecessary.If the timeout is needed for a specific reason (e.g., actual async network operations in CI), please add a comment explaining why. Otherwise, consider removing it:
- }, 10000); // Increase timeout for this test + });Additionally, note that
jest.runOnlyPendingTimers()on line 357 triggers the async flush, but the test doesn't await its completion before callingdestroy(). This is acceptable since the test only verifies thatdestroy()doesn't throw, but consider adding a comment clarifying this behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (23)
dist/client/vwo-fme-javascript-sdk.jsis excluded by!**/dist/**dist/client/vwo-fme-javascript-sdk.js.mapis excluded by!**/dist/**,!**/*.mapdist/client/vwo-fme-javascript-sdk.min.jsis excluded by!**/dist/**,!**/*.min.jsdist/client/vwo-fme-javascript-sdk.min.js.mapis excluded by!**/dist/**,!**/*.map,!**/*.min.js.mapdist/esm/VWOBuilder.jsis excluded by!**/dist/**dist/esm/VWOBuilder.js.mapis excluded by!**/dist/**,!**/*.mapdist/esm/VWOClient.jsis excluded by!**/dist/**dist/esm/VWOClient.js.mapis excluded by!**/dist/**,!**/*.mapdist/esm/services/BatchEventsQueue.jsis excluded by!**/dist/**dist/esm/services/BatchEventsQueue.js.mapis excluded by!**/dist/**,!**/*.mapdist/server-unpacked/VWOBuilder.jsis excluded by!**/dist/**dist/server-unpacked/VWOBuilder.js.mapis excluded by!**/dist/**,!**/*.mapdist/server-unpacked/VWOClient.jsis excluded by!**/dist/**dist/server-unpacked/VWOClient.js.mapis excluded by!**/dist/**,!**/*.mapdist/server-unpacked/services/BatchEventsQueue.jsis excluded by!**/dist/**dist/server-unpacked/services/BatchEventsQueue.js.mapis excluded by!**/dist/**,!**/*.mapdist/server/vwo-fme-node-sdk.jsis excluded by!**/dist/**dist/server/vwo-fme-node-sdk.js.mapis excluded by!**/dist/**,!**/*.mapdist/server/vwo-fme-node-sdk.min.jsis excluded by!**/dist/**,!**/*.min.jsdist/server/vwo-fme-node-sdk.min.js.mapis excluded by!**/dist/**,!**/*.map,!**/*.min.js.mapdist/types/VWOBuilder.d.tsis excluded by!**/dist/**dist/types/VWOClient.d.tsis excluded by!**/dist/**dist/types/services/BatchEventsQueue.d.tsis excluded by!**/dist/**
📒 Files selected for processing (6)
lib/VWOBuilder.ts(4 hunks)lib/VWOClient.ts(3 hunks)lib/services/BatchEventsQueue.ts(5 hunks)test/unit/VWOBuilder.test.ts(1 hunks)test/unit/VWOClient.test.ts(1 hunks)test/unit/services/BatchEventsQueue.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
lib/services/BatchEventsQueue.ts (2)
lib/utils/DataTypeUtil.ts (1)
isNumber(64-67)lib/utils/LogMessageUtil.ts (1)
buildMessage(31-53)
test/unit/VWOBuilder.test.ts (2)
lib/VWOBuilder.ts (1)
VWOBuilder(62-494)lib/services/SettingsService.ts (1)
SettingsService(39-349)
lib/VWOClient.ts (3)
lib/packages/logger/core/LogManager.ts (2)
LogManager(55-183)error(179-182)lib/services/BatchEventsQueue.ts (1)
BatchEventsQueue(35-258)lib/utils/LogMessageUtil.ts (1)
buildMessage(31-53)
test/unit/VWOClient.test.ts (2)
lib/VWOClient.ts (1)
VWOClient(75-613)lib/services/BatchEventsQueue.ts (1)
BatchEventsQueue(35-258)
test/unit/services/BatchEventsQueue.test.ts (1)
lib/services/BatchEventsQueue.ts (1)
BatchEventsQueue(35-258)
lib/VWOBuilder.ts (2)
lib/packages/logger/core/LogManager.ts (1)
LogManager(55-183)lib/constants/index.ts (1)
Constants(38-89)
🪛 ESLint
test/unit/services/BatchEventsQueue.test.ts
[error] 17-17: 'BatchConfig' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 18-18: 'SettingsService' is defined but never used.
(@typescript-eslint/no-unused-vars)
[error] 19-19: 'LogManager' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (7)
test/unit/VWOBuilder.test.ts (1)
37-259: Polling tests cover the right failure modes and look solidThe suite exercises stop/start, idempotence, recursion prevention, and long‑running polling with fake timers in a way that matches the new
checkAndPoll/stopPollingimplementation. This should give good confidence around timer cleanup and leak prevention.lib/VWOBuilder.ts (1)
78-80: Polling lifecycle changes look correct and align with testsThe introduction of
pollingTimeout/isPollingActive, the re‑entrancy guard incheckAndPoll, andstopPolling()’s clearing of the timeout give you a single active poll loop that can be cleanly stopped and won’t reschedule once deactivated. The wiring frombuild()intoVWOClient.setVWOBuilderfits the newdestroy()flow.Also applies to: 398-405, 413-460, 462-477
test/unit/services/BatchEventsQueue.test.ts (5)
21-40: LGTM!The mocks are well-structured and provide all the necessary methods that
BatchEventsQueuedepends on.
42-58: LGTM!Excellent test setup with proper cleanup. The use of fake timers and the conditional
queue.destroy()inafterEachfollows best practices and prevents test pollution.
60-116: LGTM!These tests effectively validate the timer cleanup fixes that prevent memory leaks:
- Verifies
clearIntervalis used instead ofclearTimeout(fixing the implementation bug)- Confirms timers are actually cleared using
jest.getTimerCount()- Ensures no automatic flushing occurs after destroy
118-193: LGTM!Comprehensive coverage of the
destroy()method behavior:
- Validates that pending events are flushed (preventing data loss)
- Confirms post-destroy enqueues are rejected
- Tests idempotency (safe to call multiple times)
- Verifies singleton instance nullification for proper garbage collection
280-318: LGTM!The tests correctly validate both aspects of
flushAndClearTimer():
- Timer is properly cleared (verified via
jest.getTimerCount())- Events are flushed and the result is returned
| // Set max queue size with a reasonable default | ||
| if (isNumber(config.maxQueueSize) && config.maxQueueSize > 0) { | ||
| this.maxQueueSize = config.maxQueueSize; | ||
| } else { | ||
| this.maxQueueSize = 1000; // Default max queue size to prevent unbounded growth | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix eviction when maxQueueSize is small; current logic can still grow unbounded
When maxQueueSize < 10, Math.floor(this.maxQueueSize * 0.1) becomes 0, so splice removes nothing and the queue can still grow beyond maxQueueSize despite the guard. This undermines the new bound meant to prevent memory growth.
You can ensure at least one event is evicted before pushing the new one:
- // Remove oldest events to make room (FIFO)
- this.queue.splice(0, Math.floor(this.maxQueueSize * 0.1)); // Remove oldest 10%
+ // Remove oldest events to make room (FIFO). Always evict at least 1.
+ const removeCount = Math.max(1, Math.floor(this.maxQueueSize * 0.1));
+ this.queue.splice(0, removeCount);Also applies to: 127-137
🤖 Prompt for AI Agents
In lib/services/BatchEventsQueue.ts around lines 93-99 and also apply same fix
to 127-137: the eviction calculation uses Math.floor(this.maxQueueSize * 0.1)
which becomes 0 for small maxQueueSize, so splice removes nothing and the queue
can exceed the bound; change the eviction logic to compute evictCount =
Math.max(1, Math.floor(this.maxQueueSize * 0.1)) (or another minimal positive
integer) and use that value when splicing before pushing a new event so at least
one event is removed when the queue is full; apply the identical change to the
other block at 127-137 to keep behavior consistent.
| test('should handle missing BatchEventsQueue.Instance gracefully', async () => { | ||
| Object.defineProperty(BatchEventsQueue, 'Instance', { | ||
| get: jest.fn(() => null), | ||
| configurable: true, | ||
| }); | ||
|
|
||
| client = new VWOClient(mockSettings, mockOptions); | ||
|
|
||
| await expect(client.destroy()).resolves.not.toThrow(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Fix Jest expectations using resolves.not.toThrow() on destroy()
Several tests assert non‑throwing behavior like:
await expect(client.destroy()).resolves.not.toThrow();But toThrow expects a function, whereas resolves passes the resolved value (undefined), so these matchers are invalid and will fail for the wrong reason.
Prefer asserting the promise resolves successfully instead, e.g.:
- await expect(client.destroy()).resolves.not.toThrow();
+ await expect(client.destroy()).resolves.toBeUndefined();or simply:
await client.destroy();
// implicit assertion: test will fail if the promise rejectsApply this pattern to the tests that are only verifying that destroy() does not reject (missing BatchEventsQueue.Instance, failing destroy(), missing/invalid builder, stopPolling errors, etc.).
Also applies to: 142-158, 176-181, 183-193, 194-208
🤖 Prompt for AI Agents
In test/unit/VWOClient.test.ts around lines 131-141 (and also apply same change
to 142-158, 176-181, 183-193, 194-208), replace invalid assertions using await
expect(client.destroy()).resolves.not.toThrow() with a proper promise-resolution
check; either call await client.destroy() directly (so the test fails if it
rejects) or use await expect(client.destroy()).resolves.toBeUndefined() to
explicitly assert successful resolution.
2 participants
PR Type
Bug fix, Tests, Enhancement
Description
Fixed critical memory leaks in timer management by replacing
clearTimeoutwithclearIntervalfor interval-based timers inBatchEventsQueueImplemented
destroy()method inVWOClientfor proper resource cleanup including settings, batch queue, and builder referencesAdded
stopPolling()method inVWOBuilderwithisPollingActiveflag to prevent duplicate polling initiation and memory leaksImplemented
maxQueueSizeconfiguration (default 1000) inBatchEventsQueuewith automatic event dropping when capacity is exceededAdded
isDestroyedflags across components to ensure idempotent destroy behavior and prevent operations on destroyed instancesComprehensive test coverage for all memory leak fixes including timer cleanup, destroy methods, and polling state management
Updated TypeScript type definitions and compiled bundles (ESM, server, client) with all fixes
Diagram Walkthrough
File Walkthrough
3 files
BatchEventsQueue.test.ts
BatchEventsQueue memory leak fixes test coveragetest/unit/services/BatchEventsQueue.test.ts
BatchEventsQueuememory leak fixescovering timer cleanup, destroy method, and max queue size protection
clearIntervalis used instead ofclearTimeoutforproper timer cleanup
stops after destruction
when capacity is exceeded
VWOClient.test.ts
VWOClient destroy method and cleanup teststest/unit/VWOClient.test.ts
VWOClient.destroy()method and cleanupfunctionality
references
operations
VWOBuilder.test.ts
VWOBuilder polling memory leak fixes teststest/unit/VWOBuilder.test.ts
VWOBuilder.stopPolling()method and polling memory leakfixes
isPollingActiveflagerrors gracefully
13 files
BatchEventsQueue.ts
BatchEventsQueue memory leak fixes and destroy methodlib/services/BatchEventsQueue.ts
maxQueueSizeconfiguration option with default value of 1000 toprevent unbounded queue growth
isDestroyedflag to prevent enqueueing after destructionclearIntervalinstead ofclearTimeoutforinterval-based timers
destroy()method that clears timer, flushes remainingevents, and nullifies singleton instance
enqueue()to check queue size and drop oldest 10% of eventswhen max capacity is reached
VWOBuilder.ts
VWOBuilder polling memory leak fixes and stopPolling methodlib/VWOBuilder.ts
pollingTimeoutandisPollingActiveproperties to track pollingstate
stopPolling()method to stop polling and clear pendingtimeouts
checkAndPoll()to prevent duplicate polling initiation withisPollingActiveflagactive
build()method for cleanuppurposes
vwo-fme-javascript-sdk.js
Client bundle with memory leak fixesdist/client/vwo-fme-javascript-sdk.js
BatchEventsQueue,VWOBuilder, andVWOClientclearIntervalinstead ofclearTimeoutisPollingActiveflag andstopPolling()methodinstance nullification
vwo-fme-node-sdk.js
Server bundle with memory leak fixesdist/server/vwo-fme-node-sdk.js
Node.js SDK
BatchEventsQueue.js
Unpacked BatchEventsQueue with memory leak fixesdist/server-unpacked/services/BatchEventsQueue.js
BatchEventsQueuewith memory leak fixesmaxQueueSizeconfiguration and queue overflow handlingdestroy()method and timer cleanup improvementsVWOBuilder.js
Unpacked VWOBuilder with polling fixesdist/server-unpacked/VWOBuilder.js
VWOBuilderwith polling memory leak fixesstopPolling()method andisPollingActivestate managementVWOClient.js
Unpacked VWOClient with destroy methoddist/server-unpacked/VWOClient.js
VWOClientwith destroy methodimplementation
setVWOBuilder()method and cleanup logicBatchEventsQueue.js
ESM BatchEventsQueue with memory leak fixesdist/esm/services/BatchEventsQueue.js
BatchEventsQueuewith memory leak fixesclearIntervalVWOBuilder.js
ESM VWOBuilder with polling fixesdist/esm/VWOBuilder.js
VWOBuilderwith polling memory leak fixesstopPolling()method and polling state managementVWOClient.js
ESM VWOClient with destroy methoddist/esm/VWOClient.js
VWOClientwith destroy methodsetVWOBuilder()method and complete cleanup flowVWOClient.js.map
Add timer cleanup and destroy methods for memory leak fixesdist/esm/VWOClient.js.map
timerReferenceandisDestroyedtotrack timer state and destruction status
setTimerReference()method to store timer callbackreference
destroy()async method that cleans up timerreferences, resets internal state, and prevents memory leaks
VWOBuilder.js.map
Add timer management and destroy methods to VWOBuilderdist/server-unpacked/VWOBuilder.js.map
timerReferenceandisDestroyedtomanage timer lifecycle
setTimerReference()method to store timer callbackreferences
build()method to callsetTimerReference()on the VWOClientinstance
destroy()async method andcancelPolling()method tohandle cleanup and prevent memory leaks from polling timers
startPolling()to prevent duplicate pollingwhen already in progress
VWOClient.js.map
Memory leak fixes with timer cleanup and destroy methodsdist/server-unpacked/VWOClient.js.map
pollIntervalIdandisDestroyedtotrack timer state and destruction status
setDestroyHandler()method to register custom cleanupcallbacks
destroy()method with timer cleanup, state reset,and error handling
build()method in VWOBuilder to callsetDestroyHandler()onthe client instance
2 files
VWOClient.ts
VWOClient destroy method and cleanup implementationlib/VWOClient.ts
vwoBuilderreference property to store builder for cleanupisDestroyedflag to track destruction state and ensureidempotent behavior
setVWOBuilder()method to store builder reference forcleanup
destroy()method that stops polling, flushes batch queue,and clears all settings
IVWOClientto include newsetVWOBuilder()anddestroy()methodsVWOBuilder.js.map
Polling lifecycle management with destroy handler integrationdist/esm/VWOBuilder.js.map
destroyHandlerandisPollingprivate properties to VWOBuilderclass
build()method to register destroy handler with the VWOClientinstance
startPolling()method with polling state management andtimer cleanup on destroy
stopPolling()method to halt polling and clear associated timers3 files
VWOClient.d.ts
VWOClient TypeScript type definitions updatedist/types/VWOClient.d.ts
setVWOBuilder()methoddestroy()async methodvwoBuilderandisDestroyedBatchEventsQueue.d.ts
BatchEventsQueue TypeScript type definitions updatedist/types/services/BatchEventsQueue.d.ts
maxQueueSizeproperty toBatchConfiginterfacemaxQueueSizeandisDestroyeddestroy()async methodVWOBuilder.d.ts
VWOBuilder TypeScript type definitions updatedist/types/VWOBuilder.d.ts
pollingTimeoutandisPollingActivestopPolling()method8 files
Summary by CodeRabbit
New Features
Bug Fixes