Skip to content

Fix effect cleanup race condition and unmanaged timers in synthutils#7457

Open
sahu-virendra-1908 wants to merge 3 commits into
sugarlabs:masterfrom
sahu-virendra-1908:fix-synth-epoch-cleanup
Open

Fix effect cleanup race condition and unmanaged timers in synthutils#7457
sahu-virendra-1908 wants to merge 3 commits into
sugarlabs:masterfrom
sahu-virendra-1908:fix-synth-epoch-cleanup

Conversation

@sahu-virendra-1908

Copy link
Copy Markdown
Contributor

What this fixes

Prevents unmanaged effect-cleanup callbacks from executing against disposed synth instances after playback is stopped.

The current implementation of _performNotes() creates per-note Tone.js effect nodes such as:

Tone.Vibrato
Tone.Tremolo
Tone.Phaser
Tone.Chorus
Tone.Distortion

and schedules their cleanup using a raw:

setTimeout(...)

callback.

Because these timers are not managed by Music Blocks' timer infrastructure, they remain active after playback is stopped and instruments are disposed.

When the delayed cleanup eventually executes, it may attempt to:

synth.toDestination()

or perform other operations on synth instances that have already been disposed.

This creates stale cleanup activity, can interfere with subsequent playback sessions, and leaves dynamically created effect nodes outside centralized cleanup handling.

Related Issue: [https://github.com//issues/7400]


Root cause

Current cleanup logic:

setTimeout(
    () => {
        effectsToDispose.forEach(effect => effect.dispose());
        temp_filters.forEach(filter => filter.dispose());

        const remaining = (_effectsInFlight.get(synth) || 1) - 1;
        _effectsInFlight.set(synth, remaining);

        if (
            remaining === 0 &&
            synth &&
            typeof synth.toDestination === "function"
        ) {
            synth.toDestination();
        }
    },
    beatValue * 1000 + 500
);

The cleanup callback captures references to:

synth
effectsToDispose
temp_filters

and executes long after note playback has completed.

If playback is stopped before the timeout fires:

disposeAllInstruments()

disposes the synths, but the pending cleanup callback still executes later.

This can result in cleanup operations running against already-disposed audio objects and creates a gap between timer management and instrument lifecycle management.


What I changed

Replaced the raw cleanup timer with a managed guarded timer:

this._timerManager.setGuardedTimeout(...)

and added instrument epoch validation:

const capturedEpoch = this._instrumentEpoch;

if (this._instrumentEpoch !== capturedEpoch) {
    ...
    return;
}

The cleanup callback now verifies that the instrument generation is still valid before attempting synth reconnection logic.

If instruments have already been disposed:

disposeAllInstruments()

the callback safely disposes temporary effect/filter nodes and exits without interacting with the disposed synth.


Added pending effect tracking

Registered dynamically created effect nodes for centralized cleanup:

effectsToDispose.push(vibrato);
this._trackPendingEffect(vibrato);

This allows temporary effect nodes to participate in global cleanup and prevents them from being left outside the disposal lifecycle.


Result

  • Effect cleanup now follows the managed timer infrastructure
  • Cleanup callbacks no longer reconnect disposed synth instances
  • Temporary effect/filter nodes are safely disposed after instrument teardown
  • Playback stop/start cycles are more stable
  • Reduces risk of stale audio-node cleanup affecting subsequent sessions
  • Keeps effect lifecycle aligned with instrument lifecycle management

Testing

Verified with:

  • Vibrato-enabled timbre playback
  • Repeated Play → Stop → Play cycles
  • Early Stop before note completion
  • Effect-enabled compositions with multiple simultaneous notes

Results:

  • Effect cleanup executes correctly
  • No cleanup interaction occurs with disposed synth instances
  • Playback continues normally after repeated stop/start cycles
  • No regressions observed in effect playback behavior

PR Category

  • Bug Fix
  • Feature
  • Performance
  • Tests
  • Documentation

Checklist

  • I have tested these changes locally and they work as expected.
  • I have added/updated tests that prove the effectiveness of these changes.
  • I have updated the documentation to reflect these changes, if applicable.
  • I have followed the project's coding style guidelines.
  • I have run npm run lint and npx prettier --check . with no errors.
  • I have addressed the code review feedback from the previous submission, if applicable.
  • I have enabled "Allow edits by maintainers" (required for auto rebase; this only affects the PR branch, not your fork).

@github-actions github-actions Bot added bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed area/javascript Changes to JS source files labels Jun 1, 2026
@sahu-virendra-1908

Copy link
Copy Markdown
Contributor Author

@sum2it @omsuneri @walterbender @ssz2605 kindly review this PR. Happy to make any improvements or changes if required.

@github-actions

github-actions Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

🧪 Jest Test Results

✅ All Jest tests passed! This PR is ready to merge.

Coverage: Statements: 48.03% | Branches: 39.54% | Functions: 52.75% | Lines: 48.43%
Master Coverage: Statements: 48.23% | Branches: 39.77% | Functions: 52.95% | Lines: 48.62%

@ssz2605 ssz2605 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I noticed this PR includes blocks.js changes (cycle detection and iterative _calculateDragGroup() traversal) in addition to the synth cleanup work. These seem unrelated to the issue described in the PR. Would it make sense to split those changes into a separate PR so the synth cleanup fix can be reviewed independently?

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

Labels

area/javascript Changes to JS source files bug fix Fixes a bug or incorrect behavior size/M Medium: 50-249 lines changed

Projects

Development

Successfully merging this pull request may close these issues.

2 participants