Skip to content

Add disposal method to SerializedStateManager for snapshot refresh timer cleanup#25200

Closed
RishhiB wants to merge 1 commit intomicrosoft:mainfrom
RishhiB:add-serializedstatemanager-disposal-rishhib
Closed

Add disposal method to SerializedStateManager for snapshot refresh timer cleanup#25200
RishhiB wants to merge 1 commit intomicrosoft:mainfrom
RishhiB:add-serializedstatemanager-disposal-rishhib

Conversation

@RishhiB
Copy link
Contributor

@RishhiB RishhiB commented Aug 12, 2025

SerializedStateManager creates a 24-hour snapshot refresh timer (refreshTimer) but provides no mechanism to dispose of it. This could potentially cause Node.js processes to hang in scenarios where the timer outlives the container lifecycle.

Root Cause

The SerializedStateManager class instantiates a Timer object with a 24-hour timeout (line 153: 60 * 60 * 24 * 1000) for periodic snapshot refreshing. While this timer is started and restarted throughout the container lifecycle, there is no corresponding disposal method to clear it when the container is disposed.

Solution

  1. Added dispose() method to SerializedStateManager to clear the refresh timer
  2. Enhanced container disposal by calling serializedStateManager.dispose() in Container.disposeCore()
  3. Added documentation explaining the purpose and importance of the disposal

AB#46301

…mer cleanup

- Added dispose() method to SerializedStateManager to clear the refresh timer
- Called serializedStateManager.dispose() in Container.disposeCore() for complete cleanup
- Prevents Node.js processes from hanging due to long-running snapshot refresh timers
@github-actions github-actions bot added area: loader Loader related issues base: main PRs targeted against main branch labels Aug 12, 2025
@RishhiB RishhiB marked this pull request as ready for review August 12, 2025 20:57
Copilot AI review requested due to automatic review settings August 12, 2025 20:57
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds proper disposal functionality to the SerializedStateManager to prevent Node.js processes from hanging due to undisposed timers. The change addresses a resource cleanup issue where a 24-hour snapshot refresh timer was not being properly disposed of when containers are destroyed.

  • Added a dispose() method to SerializedStateManager that clears the refresh timer
  • Integrated the disposal call into the container's cleanup lifecycle via Container.disposeCore()

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
packages/loader/container-loader/src/serializedStateManager.ts Adds dispose method to clear the refresh timer
packages/loader/container-loader/src/container.ts Calls serializedStateManager.dispose() during container disposal

* This prevents Node.js processes from hanging due to the refresh timer.
*/
public dispose(): void {
this.refreshTimer.clear();
Copy link

Copilot AI Aug 12, 2025

Choose a reason for hiding this comment

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

The dispose method should check if refreshTimer exists before calling clear() to avoid potential runtime errors if dispose() is called multiple times or if the timer was never initialized.

Suggested change
this.refreshTimer.clear();
if (this.refreshTimer) {
this.refreshTimer.clear();
}

Copilot uses AI. Check for mistakes.
@microsoft-github-policy-service
Copy link
Contributor

This PR has been automatically marked as stale because it has had no activity for 60 days. It will be closed if no further activity occurs within 8 days of this comment. Thank you for your contributions to Fluid Framework!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: loader Loader related issues base: main PRs targeted against main branch status: stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants