fix(core): clean socket handlers causing memory leaks

[amhsirak]

ref: #898

Summary by CodeRabbit

• Bug Fixes
  • Improved resource cleanup and management to ensure proper release of resources during workflow execution.
  • Enhanced handling of popup windows and listener removal to prevent resource leaks.
  • Better error handling across various workflow execution paths with improved logging.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

Walkthrough

Adds comprehensive resource cleanup mechanisms to the Interpreter class. Introduces a new cleanup() public method that stops workflows, clears ad-blocker resources, and resets internal state. Integrates cleanup routines across multiple control paths (abort, errors, page closure) and augments the runLoop with a persistent popup listener that routes popup pages back into the workflow via the concurrency manager.

Changes

Cohort / File(s)

Summary

Cleanup & Resource Management maxun-core/src/interpret.ts

Introduces new newPage variable with finally-block cleanup; defines cleanup function to remove popup listeners; calls cleanup across early-return paths (abort, circuit breaker, page closed, load-state errors); augments runLoop with persistent popup listener routing via concurrency manager; adds public cleanup(): Promise<void> method to stop workflows, clear ad-blocker resources, clear accumulated data structures, and reset state flags.

Sequence Diagram

sequenceDiagram
    participant Interpreter as Interpreter
    participant PopupListener as Popup Listener
    participant ConcurrencyMgr as Concurrency Manager
    participant Workflow as Workflow Loop

    Workflow->>PopupListener: Register listener (runLoop)
    activate PopupListener
    
    Note over PopupListener: Waits for popup pages
    PopupListener->>ConcurrencyMgr: Route popup page back
    ConcurrencyMgr->>Workflow: Enqueue popup page
    Workflow->>Workflow: Process popup in workflow
    
    Interpreter->>Interpreter: Error/Abort/Page Close
    Interpreter->>Interpreter: Call cleanup()
    Interpreter->>Workflow: Stop running workflow
    Interpreter->>PopupListener: Remove listener
    deactivate PopupListener
    Interpreter->>Interpreter: Clear resources (ad-blocker, data)
    Interpreter->>Interpreter: Reset state flags

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Areas requiring extra attention:
  • Error handling and logging in the cleanup() method, particularly around ad-blocker resource cleanup and workflow interruption
  • Popup listener registration/deregistration flow in runLoop to ensure no memory leaks or dangling listeners
  • Verify cleanup is called on all early-return paths (abort, circuit breaker, page closed, load-state errors)
  • Ensure newPage finally block properly closes pages in all scenarios, including exception cases

Poem

🐰 When code grows wild, it's time to sweep,
With cleanup routines, promises to keep,
Pop-up listeners now route just right,
Through concurrency's paths, resources take flight,
State flags reset, the slate runs clean—
The tidiest workflow ever seen! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title 'fix(core): clean socket handlers causing memory leaks' is only partially related to the changeset. While the PR addresses resource cleanup and memory management, the actual changes focus on implementing a comprehensive cleanup mechanism for the Interpreter class (closing pages, removing listeners, clearing data structures, resetting state flags), not specifically on 'socket handlers' as the title suggests.	Revise the title to more accurately reflect the main changes, such as 'fix(core): add cleanup mechanism for Interpreter to prevent resource leaks' or similar.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch core-resource-cleanup

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

• Provide your own instructions using the high_level_summary_instructions setting.
• Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
• Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
4. 📊 Contributor Summary — Include a Markdown table showing contributions:
   | Contributor | Lines Added | Lines Removed | Files Changed |
5. ✔️ Additional Notes — Add any extra reviewer context.
   Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

maxun-core/src/interpret.ts (2)

1484-1492: Log errors in the cleanup catch block.

The empty catch block silently swallows any errors that occur during cleanup. While cleanup errors are often non-critical, they should still be logged for debugging purposes.

Apply this diff:

     const cleanup = () => {
       try {
         if (!p.isClosed()) {
           p.removeListener('popup', popupHandler);
         }
       } catch (cleanupError) {
+        this.log('Failed to remove popup listener', Level.WARN);
       }
     };

---

1736-1742: Consider resetting additional state for complete cleanup.

The cleanup method clears data structures and some state flags, but doesn't reset:

• screenshotCounter
• scrapeListCounter
• concurrency (should this be recreated or reset?)

If the Interpreter instance is intended to be reusable after cleanup, these should also be reset to initial values.

Apply this diff if reusability is intended:

       // Clear accumulated data to free memory
       this.cumulativeResults = [];
       this.namedResults = {};
       this.serializableDataByType = { scrapeList: {}, scrapeSchema: {} };
+      
+      // Reset counters
+      this.screenshotCounter = 0;
+      this.scrapeListCounter = 0;
 
       // Reset state
       this.isAborted = false;
       this.initializedWorkflow = null;

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad8df66 and bf1a6f1.

📒 Files selected for processing (1)

• maxun-core/src/interpret.ts (5 hunks)

🔇 Additional comments (3)

maxun-core/src/interpret.ts (3)

463-482: LGTM: Proper resource cleanup in enqueueLinks.

The addition of the finally block ensures that newly created pages are properly closed even when errors occur, preventing memory leaks. The null-check and isClosed() guard are appropriate.

---

1729-1729: The review comment is incorrect. Level.DEBUG is properly defined in the logger module.

The verification shows:

• Level.DEBUG is explicitly defined in maxun-core/src/utils/logger.ts at line 10 with value 95
• It's part of the exported Level enum (lines 5-12)
• It's already used in the logger function signature as a valid parameter type (line 16)
• The import in interpret.ts (line 17) correctly imports Level from the logger module

The code at lines 1729 and 1744 is correct and requires no changes.

---

1726-1733: Based on my verification, I can now provide the rewritten review comment:

---

The PlaywrightBlocker instance does not have cleanup or disposal methods in v1.31.3, so setting this.blocker = null is the correct approach.

The @cliqz/adblocker-playwright v1.31.3 library's public API does not expose any cleanup(), dispose(), or destroy() methods on the PlaywrightBlocker class. The only lifecycle methods available are enableBlockingInPage(page) and disableBlockingInPage(page) for managing blocking at the page level, which are already being called appropriately in the codebase (lines 147 and 157). Dereferencing the instance via null assignment is sufficient.

The try-catch block around the null assignment is unnecessary, as variable assignment does not throw errors.