Skip to content

Fix temp file leak in batch metadata segment upload - #19377

Merged
yashmayya merged 1 commit into
apache:masterfrom
shounakmk219:fix-metadata-push-controller-temp-file-leak
Aug 27, 2026
Merged

Fix temp file leak in batch metadata segment upload#19377
yashmayya merged 1 commit into
apache:masterfrom
shounakmk219:fix-metadata-push-controller-temp-file-leak

Conversation

@shounakmk219

Copy link
Copy Markdown
Collaborator

Problem

The batch metadata segment upload path (POST /segments/batchUpload) creates three temporary files under java.io.tmpdir that are never deleted:

File Created in Scope
segmentMetadata-<uuid>/ createSegmentFileFromSegmentMetadataInfo per segment
allSegmentsMetadataTar-<uuid>.tar.gz createSegmentsMetadataInfoMap per request
allSegmentsMetadataDir-<uuid>/ createSegmentsMetadataInfoMap per request

uploadSegments already has a tempFiles list drained by cleanupTempFiles in a finally, but it only ever receives the three per-segment entries (tempEncryptedFile, tempDecryptedFile, tempSegmentDir) which live under the controller's own getFileUploadTempDir() / getUntarredFileTempDir(). The three files above live in java.io.tmpdir instead, so ControllerFilePathProvider.initDir's startup cleanDirectory does not reclaim them either — they survive controller restarts and are only removed when the OS reaps /tmp.

In createSegmentFileFromSegmentMetadataInfo, only the intermediate tar file had a finally; the staging directory it was built from had none.

Per push of N segments this leaves behind 1 tar + 1 directory holding 2N metadata files + N directories holding 2 files each — roughly 3x the metadata volume of every push. metadata.properties scales with column count, so wide tables on a frequent push cadence will fill /tmp on the controller.

Fix

  • createSegmentFileFromSegmentMetadataInfo — open the try before the staging work so the finally covers the staging directory as well as the tar file. This also handles a createCompressedTarFile failure leaving a partial tar behind. Switched the finally from forceDelete to deleteQuietly so a cleanup failure cannot mask the exception that caused it.

  • createSegmentsMetadataInfoMap — takes the caller's tempFiles list and registers both request-scoped files as soon as their paths are computed, so they are cleaned even when the untar or the mapping-file read fails part way through. They are registered rather than deleted locally because the returned SegmentMetadataInfo values hold live File handles into that directory, which the caller reads inside its loop.

  • uploadSegments — moved the createSegmentsMetadataInfoMap call inside the try. Previously a failure there leaked both files and skipped multiPart.cleanup() entirely. ControllerApplicationException extends WebApplicationException, so the catch block rethrows it unchanged; the only behavioral delta is a metric increment of 0 and the cleanup now running.

The equivalent client-side code in SegmentPushUtils.createSegmentsMetadataTarFile already deletes its staging directory in a finally — the controller side had not been given the same treatment.

Testing

  • testCreateSegmentFileFromSegmentMetadataInfo extended with a before/after snapshot of java.io.tmpdir entries matching the two segmentMetadata* prefixes.
  • New testCreateSegmentsMetadataInfoMapRegistersTempFilesForCleanup builds a real uber tar and asserts the map is correct, that its file handles are still readable (cleanup correctly deferred to the caller), that both temp files landed in tempFiles, and that draining the list leaves no residue.

Both assert on a prefix-filtered diff of the temp directory rather than absolute emptiness, so unrelated entries cannot flake them.

Confirmed the tests are not vacuous — with the cleanup removed they fail with expected [] but got [segmentMetadata-3cff66bd-…] and expected [2] but found [0] respectively.

createSegmentsMetadataInfoMap was made package-private @VisibleForTesting to allow the new test; it has no callers outside this class.

Backward compatibility

None affected — no wire format, serialization, or public API change. createSegmentsMetadataInfoMap is an internal static helper.

The batch metadata upload path (`POST /segments/batchUpload`) creates three temporary
files under `java.io.tmpdir` that were never deleted:

- `segmentMetadata-<uuid>/` — the staging dir tarred up by
  `createSegmentFileFromSegmentMetadataInfo`. Only the resulting tar file had a
  `finally`; the source dir had none. One leaked dir per segment per push.
- `allSegmentsMetadataTar-<uuid>.tar.gz` and `allSegmentsMetadataDir-<uuid>/` —
  created by `createSegmentsMetadataInfoMap` and never removed.

`uploadSegments`'s existing `cleanupTempFiles` list only covered the three
per-segment files under the controller's own upload/untar temp dirs. The leaked
files live in `java.io.tmpdir` instead, so `ControllerFilePathProvider.initDir`'s
startup `cleanDirectory` does not reclaim them either — they survive controller
restarts. Each push leaks roughly 3x its metadata volume, which scales with
column count, so wide tables on a frequent push cadence fill up /tmp.

Changes:

- `createSegmentFileFromSegmentMetadataInfo`: open the `try` before the staging
  work so the `finally` covers the staging dir as well as the tar file. This also
  handles a `createCompressedTarFile` failure leaving a partial tar behind. Use
  `deleteQuietly` rather than `forceDelete` so a cleanup failure cannot mask the
  exception that caused it.
- `createSegmentsMetadataInfoMap`: register both request-scoped files with the
  caller's `tempFiles` list as soon as their paths are computed, so they are
  cleaned even when the untar or the mapping-file read fails part way. They are
  registered rather than deleted locally because the returned
  `SegmentMetadataInfo` values hold live `File` handles into that directory.
- `uploadSegments`: move the `createSegmentsMetadataInfoMap` call inside the
  `try`. Previously a failure there leaked both files and skipped
  `multiPart.cleanup()` entirely.

The equivalent client-side code in `SegmentPushUtils.createSegmentsMetadataTarFile`
already cleans up its staging dir in a `finally`; the controller side had not
been given the same treatment.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@shounakmk219 shounakmk219 added the bug Something is not working as expected label Aug 27, 2026
@shounakmk219
shounakmk219 requested a review from yashmayya August 27, 2026 14:10
@codecov-commenter

codecov-commenter commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.51%. Comparing base (a5c2c45) to head (150a94f).

Files with missing lines Patch % Lines
...ces/PinotSegmentUploadDownloadRestletResource.java 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master   #19377      +/-   ##
============================================
- Coverage     67.54%   67.51%   -0.04%     
  Complexity     1430     1430              
============================================
  Files          3486     3486              
  Lines        224044   224047       +3     
  Branches      35353    35353              
============================================
- Hits         151339   151273      -66     
- Misses        60677    60739      +62     
- Partials      12028    12035       +7     
Flag Coverage Δ
integration 100.00% <ø> (ø)
integration1 100.00% <ø> (ø)
integration2 0.00% <ø> (ø)
java-25 67.51% <81.81%> (-0.04%) ⬇️
lane-a 100.00% <ø> (ø)
lane-b 0.00% <ø> (ø)
temurin 67.51% <81.81%> (-0.04%) ⬇️
unittests 67.51% <81.81%> (-0.04%) ⬇️
unittests1 57.60% <ø> (-0.02%) ⬇️
unittests2 39.32% <81.81%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@yashmayya yashmayya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @shounakmk219!

@yashmayya
yashmayya merged commit faad15f into apache:master Aug 27, 2026
12 checks passed
@Jackie-Jiang Jackie-Jiang added the ingestion Related to data ingestion pipeline label Aug 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working as expected ingestion Related to data ingestion pipeline

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants