Skip to content

Conversation

@KJ7LNW
Copy link
Contributor

@KJ7LNW KJ7LNW commented Jul 2, 2025

Dependencies

Note to Reviewer

The commits tell a clean story, it will be easier to understand what is happening here by looking at each commit individually under "Commits" than by looking at all of the files that were changed.

This PR introduces safeReadJson, a new utility to read JSON files safely and efficiently, reducing memory pressure when loading large JSON files by streaming them instead of double-buffering via JSON.parse.

Key Changes

  • safeReadJson Utility:

    • A new function in src/utils/safeReadJson.ts that uses stream-json to parse files without buffering them entirely into memory.
    • Implements file locking via proper-lockfile to prevent race conditions during reads, sharing the lock mechanism with safeWriteJson.
    • Supports extracting specific data using JSON paths.
  • Codebase Adoption:

    • Replaced the pattern of JSON.parse(fs.readFile(...)) with safeReadJson across the codebase to improve performance and consistency.
  • Refactoring:

    • The file locking logic in safeWriteJson.ts has been extracted into a shared _acquireLock function, now used by both safeReadJson and safeWriteJson.
  • Testing:

    • Added comprehensive unit tests for the new safeReadJson utility.
    • Updated existing tests to mock the new utility.

How to Test

  1. Run the application and verify that all JSON file reading operations work as expected.
  2. Check memory usage when working with large task files (should be reduced).
  3. Run the test suite to ensure all functionality remains intact.

Get in Touch

Discord: KJ7LNW


Important

Introduces safeReadJson for efficient JSON reading, refactors codebase to use it, and adds comprehensive tests.

  • Behavior:
    • Introduces safeReadJson in src/utils/safeReadJson.ts for efficient JSON file reading using streaming and file locking.
    • Replaces JSON.parse(fs.readFile(...)) with safeReadJson in modelCache.ts, modelEndpointCache.ts, and importExport.ts.
  • Refactoring:
    • Extracts file locking logic into _acquireLock shared by safeReadJson and safeWriteJson.
  • Testing:
    • Adds unit tests for safeReadJson in safeReadJson.spec.ts.
    • Updates existing tests to mock safeReadJson in importExport.spec.ts and cache-manager.spec.ts.
  • Misc:
    • Adds rule in .roo/rules/use-safeReadJson.md to enforce safeReadJson usage.

This description was created by Ellipsis for bb27ee94901f66f2492dc1327f0ca351c0ca1e5b. You can customize this summary. It will automatically update as commits are pushed.

@KJ7LNW KJ7LNW requested review from cte, jr and mrubens as code owners July 2, 2025 04:37
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. enhancement New feature or request labels Jul 2, 2025
@KJ7LNW KJ7LNW marked this pull request as draft July 2, 2025 04:39
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Jul 2, 2025
@daniel-lxs daniel-lxs moved this from Triage to PR [Draft / In Progress] in Roo Code Roadmap Jul 2, 2025
@hannesrudolph hannesrudolph added PR - Draft / In Progress and removed Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. labels Jul 2, 2025
@KJ7LNW KJ7LNW force-pushed the stream-json-reads branch from 92bdbaf to e74f87e Compare July 4, 2025 02:04
@KJ7LNW KJ7LNW self-assigned this Jul 4, 2025
@KJ7LNW KJ7LNW marked this pull request as ready for review July 4, 2025 02:07
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Jul 4, 2025
@KJ7LNW KJ7LNW force-pushed the stream-json-reads branch from e74f87e to d7fbbbb Compare July 4, 2025 02:16
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Jul 4, 2025

@daniel-lxs please move this into review

@KJ7LNW KJ7LNW force-pushed the stream-json-reads branch from d7fbbbb to ee3831b Compare July 5, 2025 02:08
@daniel-lxs daniel-lxs moved this from PR [Draft / In Progress] to PR [Needs Prelim Review] in Roo Code Roadmap Jul 7, 2025
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Jul 8, 2025

stand by on merging, I have a small change for this, moving back to draft

@KJ7LNW KJ7LNW marked this pull request as draft July 8, 2025 01:44
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Jul 12, 2025

rebased on origin/main, ready for review

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Needs Review] in Roo Code Roadmap Jul 15, 2025
Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

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

Thank you @KJ7LNW!

I left a couple of suggestions, let me know what you think!

@daniel-lxs daniel-lxs moved this from PR [Needs Review] to PR [Changes Requested] in Roo Code Roadmap Jul 15, 2025
@KJ7LNW KJ7LNW force-pushed the stream-json-reads branch from 053c02f to 0fe6cda Compare July 16, 2025 20:55
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:XXL This PR changes 1000+ lines, ignoring generated files. labels Jul 16, 2025
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Jul 16, 2025

@daniel-lxs I resolved your suggested changes and rebased on v3.23.12

Copy link
Member

@daniel-lxs daniel-lxs left a comment

Choose a reason for hiding this comment

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

Thank you @KJ7LNW, looks good to me!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Jul 16, 2025
@daniel-lxs daniel-lxs moved this from PR [Changes Requested] to PR [Needs Review] in Roo Code Roadmap Jul 16, 2025
Eric Wheeler added 4 commits July 18, 2025 13:47
Implement safeReadJson function to complement the existing safeWriteJson
functionality:

- Uses stream-json for efficient processing of large JSON files
- Supports both full object reading and selective path extraction
- Provides file locking to prevent concurrent access
- Includes comprehensive error handling
- Adds complete test coverage
- Passthrough all exceptions

This enables efficient and safe JSON reading operations throughout the
codebase.

Signed-off-by: Eric Wheeler <[email protected]>
Replace manual file reading and JSON parsing with the safer safeReadJson utility
across multiple files in the codebase. This change:

- Provides atomic file access with proper locking to prevent race conditions
- Streams file contents efficiently for better memory usage
- Improves error handling consistency
- Reduces code duplication

Fixes: #5331
Signed-off-by: Eric Wheeler <[email protected]>
Updated test files to properly mock and use safeReadJson/safeWriteJson:
- Added proper imports for safeReadJson from safeWriteJson module
- Updated mock implementations to mock both functions correctly
- Replaced direct fs operations with calls to safe functions
- Updated assertions to match the new behavior

This fixes all failing tests after the conversion to safeReadJson.

Signed-off-by: Eric Wheeler <[email protected]>
@KJ7LNW KJ7LNW force-pushed the stream-json-reads branch from 0fe6cda to b0ec3f4 Compare July 18, 2025 20:48
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Jul 18, 2025

rebased on v3.23.14

@daniel-lxs
Copy link
Member

Thanks for the contribution. At this stage, the impact of these changes isn’t clear and we’re focusing on higher-priority items. Closing for now, but we can reconsider in the future if priorities shift.

@daniel-lxs daniel-lxs closed this Aug 18, 2025
@github-project-automation github-project-automation bot moved this from PR [Needs Review] to Done in Roo Code Roadmap Aug 18, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 18, 2025
hannesrudolph pushed a commit that referenced this pull request Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request lgtm This PR has been approved by a maintainer PR - Needs Review size:XL This PR changes 500-999 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

fix: High memory usage and potential race conditions when reading large JSON files

3 participants