Clean up multipart tempfile on segment upload failure paths - #19455
Draft
deepthi912 wants to merge 1 commit into
Draft
Clean up multipart tempfile on segment upload failure paths#19455deepthi912 wants to merge 1 commit into
deepthi912 wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19455 +/- ##
============================================
+ Coverage 67.64% 67.65% +0.01%
Complexity 1430 1430
============================================
Files 3488 3488
Lines 224396 224400 +4
Branches 35422 35425 +3
============================================
+ Hits 151788 151818 +30
+ Misses 60577 60561 -16
+ Partials 12031 12021 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
deepthi912
force-pushed
the
fix-multipart-tempfile-leak-uploadSegment
branch
from
September 4, 2026 07:12
87dc5d4 to
2f436c1
Compare
Adds multiPart.cleanup() at the outer finally of four controller REST endpoints that receive FormDataMultiPart so that Jersey/mimepull temp files (MIME*.tmp on the controller's java.io.tmpdir) do not leak on failure paths. Endpoints fixed: - PinotSegmentUploadDownloadRestletResource.uploadSegment (case SEGMENT/METADATA guard-clause throws, case URI with unexpected body, default case) - PinotSegmentUploadDownloadRestletResource.uploadReingestedSegment (any post-try exception) - PinotIngestionRestletResource.ingestFromFile (any exception in the ingestData delegation) - LLCSegmentCompletionHandlers.extractSegmentMetadataFromForm (segmentCommitEndWithMetadata's only cleanup path) Ownership centralization: - Removed the inner multiPart.cleanup() from createSegmentFileFromMultipart (called by uploadSegment and uploadReingestedSegment) and from FileIngestionHelper.copyMultipartToLocal (called by ingestFromFile via ingestData). The outer REST endpoint now owns the multipart lifecycle. Comments added documenting this. Not covered: - JVM death or IOException from mimepull during body spool - no finally can run for those; needs a separate reaper. - uploadSegments/uploadReingestedSegment pre-try validation throws (missing tableName, wrong upload type, etc.) - protocol errors from malformed clients; kept out of scope for this cleanup fix.
deepthi912
force-pushed
the
fix-multipart-tempfile-leak-uploadSegment
branch
from
September 4, 2026 07:28
2f436c1 to
42c225c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
multiPart.cleanup()to the outerfinallyofuploadSegmentanduploadReingestedSegmentso Jersey/mimepull tempfiles (MIME*.tmpunder the controller'sjava.io.tmpdir) don't leak when the handler exits before reachingcreateSegmentFileFromMultipart.Leak paths fixed
uploadSegment: guard-clause throws for missingDOWNLOAD_URI(case SEGMENT and case METADATA), unexpected body in case URI, default caseuploadReingestedSegment: pre-try guard throws (wrongUPLOAD_TYPE/ missingDOWNLOAD_URI/COPY_SEGMENT_TO_DEEP_STORE != true) — moved inside the existingtryso the newfinallycovers themThe existing
multiPart.cleanup()insidecreateSegmentFileFromMultipartis kept for defense in depth.MIMEPart.close()is idempotent, so double-invocation on the success path is safe.Not covered
Leaks that no per-handler
finallycan catch — JVM death,IOExceptionfrom mimepull during body spool, client disconnect — need a separate startup reaper for staleMIME*.tmp. Out of scope for this PR.