Skip to content

Conversation

@aejeong-context
Copy link

@aejeong-context aejeong-context commented Jan 17, 2026

Fixes #41495

What was changed

  • Wrapped InputStream creation in try-with-resources to ensure proper cleanup
  • Fixed multiple occurrences where StreamUtils.copyToString() left streams open

Why

StreamUtils.copyToString() does not close the provided InputStream.
Leaving these streams open can lead to resource leaks and file descriptor exhaustion.

Testing

  • Existing tests passed
  • Verified affected code paths compile and function as expected

Summary by CodeRabbit

  • Refactor
    • Improved resource management across server and git subsystems by adopting guarded, auto-closing IO patterns and added null-checks when loading templates and scripts to reduce potential resource leaks and make reads more robust.
  • Tests
    • Updated tests to use the safer resource-handling approach, improving reliability of schema, import, and related test executions.

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

…source leaks

StreamUtils.copyToString() does not close the InputStream after reading,
which can lead to resource leaks and file descriptor exhaustion.

This change wraps all StreamUtils.copyToString() calls with try-with-resources
to ensure InputStreams are properly closed.

Fixes appsmithorg#41495
@aejeong-context aejeong-context requested review from a team, abhvsn and sharat87 as code owners January 17, 2026 03:28
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 17, 2026

Walkthrough

Replaced unsafe StreamUtils.copyToString(...) usages with try-with-resources InputStream blocks across production and test files to ensure streams are closed after reading resources; no public APIs or functional behavior changed.

Changes

Cohort / File(s) Summary
Server: migrations & plugins
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/DatabaseChangelog2.java, app/server/appsmith-server/src/main/java/com/appsmith/server/plugins/base/PluginServiceCEImpl.java, app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/CreateDBTablePageSolutionCEImpl.java
Replaced direct StreamUtils.copyToString(...) calls with try-with-resources InputStream blocks; added InputStream imports where needed. Behavior and APIs unchanged; resource management improved.
Server: tests
app/server/appsmith-server/src/test/java/com/appsmith/server/git/ServerSchemaMigrationEnforcerTest.java, app/server/appsmith-server/src/test/java/com/appsmith/server/imports/internal/ImportServiceTests.java, app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/PartialImportServiceTest.java
Updated test helpers to use try-with-resources when reading resources and added java.io.InputStream imports; semantics unchanged, streams are closed properly.
Git utilities
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java
initializeReadme now loads template inside try-with-resources, null-checks the resource and assigns content to a local variable before doing replacements and writing output; fixes potential resource leak and adds explicit missing-resource handling.
Git: Bash service
app/server/appsmith-git/src/main/java/com/appsmith/git/service/BashService.java
callFunctionUnBounded refactored to read script content within a try-with-resources InputStream block with null-check inside; script construction/execution unchanged, stream closed reliably.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

Streams once left open to roam,
Now folded safely back at home.
Try-with-resources sings its song,
File handles short no longer long.
Small change, fewer leaks — code strong. 🛠️

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and concisely describes the main change: closing InputStreams after StreamUtils.copyToString calls to fix resource leaks.
Description check ✅ Passed The description covers the main changes, rationale, and testing, but lacks the required issue link format and Cypress test results section from the template.
Linked Issues check ✅ Passed All code changes properly implement try-with-resources blocks to wrap InputStream usage across 8 files, directly addressing the resource leak fix requirements from issue #41495.
Out of Scope Changes check ✅ Passed All changes are focused solely on wrapping InputStreams in try-with-resources blocks; no unrelated modifications or scope creep detected across the 8 affected files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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

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

@aejeong-context
Copy link
Author

aejeong-context commented Jan 20, 2026

@sharat87 Hi, just a gentle ping to check if this PR is ready to be merged.
Let me know if you’d like me to address the docstring coverage warning.

Copy link
Collaborator

@subrata71 subrata71 left a comment

Choose a reason for hiding this comment

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

Thanks for fixing these resource leaks. There may other unclosed InputStream in PluginServiceCEImpl.java . Could you please check and apply the same try-with-resources pattern if needed.

InputStream obtained from getInputStream() or getResourceAsStream() must be
explicitly closed to prevent resource leaks and file descriptor exhaustion.

This change wraps all InputStream usages with try-with-resources
to ensure they are properly closed.

Fixes appsmithorg#41495
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java`:
- Around line 821-831: The code calls
classLoader.getResourceAsStream(gitServiceConfig.getReadmeTemplatePath())
without checking for null which can cause an NPE at IOUtils.copy; update the
FileUtilsCEImpl logic to check the InputStream for null (similar to the pattern
used in BashService.java) and handle the missing resource (throw a clear
exception or use a fallback) before calling IOUtils.copy; also remove the
redundant .toString() when replacing EDIT_MODE_URL_TEMPLATE and
VIEW_MODE_URL_TEMPLATE on the already-String variable data to simplify the
replacement chain.

Ensure an exception is thrown with a clear message if the README template is not found, preventing potential null pointer issues.
@subrata71
Copy link
Collaborator

@aejeong-context Some quality checks are failing specially spotless check. Could you please fix that by running the following command from /app/server directory?

mvn spotless:apply

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Resource leak: InputStream not closed after StreamUtils.copyToString() calls

2 participants