fix(server): close WriteStreams on createTempFiles failure#40765
fix(server): close WriteStreams on createTempFiles failure#40765SebTardif wants to merge 1 commit into
Conversation
|
I don't think createTempFiles should exist. For same machine scenario (local or remote), existing file path should be used. Otherwise, files should be streamed to the location determined by the server, not client. |
|
Thank you for the feedback. I understand the concern about the Looking at the current implementation:
Could you clarify the preferred approach for the remote case? Options I see:
I'm happy to implement the proper architectural solution once the approach is clarified. The fd leak fix in this PR addresses a real issue in the current code, but I understand if you want to redesign this entirely rather than patch the existing approach. |
|
Let's not block this on the redesign. |
Test results for "MCP"2 failed 7055 passed, 1068 skipped Merge workflow run. |
Test results for "tests 1"5 flaky41715 passed, 850 skipped Merge workflow run. |
Summary
Promise.allin try-catch to clean up WriteStreams if any file creation failsmkdirorcreateWriteStreamfails mid-way through creating multiple temp filesIntroduced in #31165 (2022-03-18). Related: #40754 (path validation), #40764 (WritableStreamDispatcher._onDispose).