Skip to content

fix(linter): fix Arc<ModuleRecord> memory leak and lifecycle issues#14049

Merged
Boshen merged 4 commits intomainfrom
fix-arc-memory-leaks
Sep 24, 2025
Merged

fix(linter): fix Arc<ModuleRecord> memory leak and lifecycle issues#14049
Boshen merged 4 commits intomainfrom
fix-arc-memory-leaks

Conversation

@Boshen
Copy link
Member

@Boshen Boshen commented Sep 23, 2025 •
edited
Loading

Summary

Fixes #10627

This PR fixes two critical issues with Arc<ModuleRecord> management in the linter's runtime:

  1. Memory leak: Arc<ModuleRecord> instances weren't being dropped after program exits
  2. Premature deallocation: ModuleRecords were disappearing during parallel linting when called from the language server
  3. Also includes fix: use Weak references for CachedPath to enable proper drop oxc-resolver#727

Changes

1. Use Weak references to break circular dependencies

  • Changed loaded_modules field from Arc<ModuleRecord> to Weak<ModuleRecord>
  • Added get_loaded_module() helper method for safe weak reference upgrades
  • Updated all import rules to use the helper method

2. Fix ModuleRecord lifecycle management

  • Made modules_by_path and encountered_paths fields of the Runtime struct (with Mutex wrapping)
  • Removed premature clearing from inside resolve_modules that was causing ModuleRecords to disappear
  • Added clearing after rayon::scope completes in all run methods (run, run_source, run_test_source)

Technical Details

The root cause was that modules_by_path was being cleared while parallel linting tasks spawned by rayon::scope were still running. This caused:

  • Weak references in loaded_modules to become invalid
  • Import plugin rules to fail when trying to access loaded modules
  • The issue was particularly visible when running from the language server

The fix ensures that:

  • ModuleRecords remain alive during the entire linting process
  • Memory is still properly cleaned up after each linting session
  • No circular references can cause memory leaks

Test Plan

🤖 Generated with Claude Code

@Boshen Boshen requested a review from camc314 as a code owner September 23, 2025 14:28
Copilot AI review requested due to automatic review settings September 23, 2025 14:28
@graphite-app
Copy link
Contributor

graphite-app bot commented Sep 23, 2025

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

@github-actions github-actions bot added A-linter Area - Linter C-bug Category - Bug labels Sep 23, 2025
Copilot AI reviewed Sep 23, 2025
View reviewed changes
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 fixes memory leaks and lifecycle issues with Arc<ModuleRecord> management in the linter's runtime by switching to weak references and properly managing the module cache lifetime.

  • Changed loaded_modules from Arc<ModuleRecord> to Weak<ModuleRecord> to break circular dependencies
  • Moved module cache (modules_by_path and encountered_paths) to Runtime struct fields with proper lifecycle management
  • Added module cache clearing after linting completes to prevent memory leaks

Reviewed Changes

Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.

File Description
crates/oxc_linter/src/service/runtime.rs Major refactoring to use Mutex-wrapped module caches as struct fields, switch to weak references, and add proper cleanup
crates/oxc_linter/src/module_record.rs Changed loaded_modules to use Weak<ModuleRecord> and added helper method for safe upgrades
crates/oxc_linter/src/module_graph_visitor.rs Updated to handle weak references with proper upgrade checks
Import rule files Updated to use new get_loaded_module() helper method instead of direct weak reference access

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@Boshen Boshen requested a review from Sysix September 23, 2025 14:34
@codspeed-hq
Copy link

codspeed-hq bot commented Sep 23, 2025 •
edited
Loading

CodSpeed Instrumentation Performance Report

Merging #14049 will not alter performance

Comparing fix-arc-memory-leaks (b85c07d) with main (1b3f437)

Summary

✅ 37 untouched

graphite-app[bot]
graphite-app bot reviewed Sep 23, 2025
View reviewed changes
@Boshen Boshen force-pushed the fix-arc-memory-leaks branch from 81c02c0 to 0e76665 Compare September 23, 2025 14:48
graphite-app[bot]
graphite-app bot reviewed Sep 23, 2025
View reviewed changes
@Boshen Boshen marked this pull request as draft September 23, 2025 14:58
@github-actions github-actions bot added the A-editor Area - Editor and Language Server label Sep 23, 2025
@Boshen Boshen assigned Boshen and unassigned Sysix Sep 23, 2025
@Sysix
Copy link
Member

Sysix commented Sep 23, 2025

Tested with heaptrack and memory stats. After 1000x iteration with the example code in #12100
I get on main:

Current physical memory usage: 256819200

heaptrack stats:
        allocations:            51469914
        leaked allocations:     1787
        temporary allocations:  1848137

In this PR:

Current physical memory usage: 215678976

heaptrack stats:
        allocations:            51459931
        leaked allocations:     654
        temporary allocations:  1861597

Looks like there are more problems with the server :(

@Boshen Boshen force-pushed the fix-arc-memory-leaks branch from 529b7c7 to 8e458f4 Compare September 24, 2025 05:39
@Boshen Boshen marked this pull request as ready for review September 24, 2025 06:43
@Boshen
Copy link
Member Author

Boshen commented Sep 24, 2025

I tested through variables methods:

  • Run the main logic in a loop and observe no raising in memory - both cli and language server
  • Used dhat to observe memory at exit, there are no memory leaks.

@Boshen Boshen merged commit 2186b28 into main Sep 24, 2025
27 checks passed
@Boshen Boshen deleted the fix-arc-memory-leaks branch September 24, 2025 07:53
@Boshen Boshen mentioned this pull request Sep 24, 2025
Merged
theoludwig added a commit to theoludwig/eslint-config-conventions that referenced this pull request Sep 25, 2025
@theoludwig
fix(oxlint): enable back import rules
a13aead 
Ref: oxc-project/oxc#14049
@Sysix Sysix mentioned this pull request Sep 29, 2025
Merged
graphite-app bot pushed a commit that referenced this pull request Sep 29, 2025
@Sysix
refactor(language_server): remove unused fixture files (#14246)
7fe930c 
After #14049, the fixtures files are not needed anymore.
They were only files which simplified the bug search
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

Copilot code review Copilot Copilot left review comments

@camc314 camc314 Awaiting requested review from camc314 camc314 is a code owner

@Sysix Sysix Awaiting requested review from Sysix

Assignees

@Boshen Boshen

Labels

A-editor Area - Editor and Language Server A-linter Area - Linter C-bug Category - Bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

linter: memory leak with import plugin

3 participants

@Boshen @Sysix