[Improvement-18459][Common] Cap whole-file log download at 47MB to prevent OOM - #18463
[Improvement-18459][Common] Cap whole-file log download at 47MB to prevent OOM#18463xmg333 wants to merge 2 commits into
Conversation
SbloodyS
left a comment
There was a problem hiding this comment.
For a log larger than 47 MB:
LogServiceImpl#getTaskInstanceWholeLogFileBytesreturnsERROR.LogClientDelegate#getWholeLogBytestreats every local error as a reason to callremoteLogClient.getWholeLog(...).RemoteLogClientcallsgetFileContentBytesFromRemote, which now uses the same capped reader and silently returns only the first 47 MB.
With remote logging enabled, the API can therefore return a successfully downloaded but truncated log. With remote logging disabled or unavailable, it may return an empty log or a generic download error instead of the clear size-limit message.
Please distinguish “local log unavailable” from “log exceeds the supported size,” propagate the latter to the API, and make the reader fail explicitly rather than silently truncating. The regression test should cover the complete LogClientDelegate/API path, not only LogServiceImpl.
Additionally, the linked issue expects large logs to remain downloadable through chunked streaming. This PR rejects them entirely.
getFileContentBytesFromLocal reads entire files into memory with no limit, causing OOM when downloading large task logs. This PR adds a chunked RPC (getTaskInstanceLogFileChunk) that reads 8MB at a time via RandomAccessFile, and streams the result to the HTTP response via StreamingResponseBody. Architecture: try chunk -> catch -> fallback whole - New worker: chunked RPC succeeds, any size log is downloadable - Old worker: chunk RPC fails, falls back to legacy getWholeLogBytes - Mid-stream failure (bytes already written): throws IOException (no corruption) - First-chunk failure (nothing written): safe fallback to remote legacy The legacy getTaskInstanceWholeLogFileBytes is unchanged (backward compatible). readFileRange fails explicitly (IOException) on missing files rather than silently returning empty bytes. Co-Authored-By: Claude <noreply@anthropic.com>
2aa5e28 to
f7168cf
Compare
|
Thanks for the review @SbloodyS . I've reworked the approach based on your feedback. New ** chunked streaming log ** is now completed. Could you confirm if this scope is what you had in mind? What's newNew RPC: Fallback logic (the important part)The API server runs a chunked loop with an The core invariant: fallback only happens when Three concrete scenarios:
Legacy path unchanged:
Tests cover all three scenarios in |
…tion tests Address code review feedback: - Split streamLogBytes into checkDownloadLogAuth (sync, before response committed) + streamLogBytes(TaskInstance, OutputStream) — auth failures now return JSON error via @ApiException instead of a fake .log file - Add testStreamWholeLogRpcThrowsFallsBackToRemote: old worker (method-not-found) triggers RPC exception, safely falls back to remote (offset==0) - Add testStreamWholeLogRpcThrowsMidStreamThrows: mid-stream RPC exception throws IOException (offset>0, no corruption) - Fix getBytes() to use StandardCharsets.UTF_8 Co-Authored-By: Claude <noreply@anthropic.com>
Was this PR generated or assisted by AI?
YES. Implementation and tests drafted with assistance from Claude (Anthropic); reviewed by human.
Purpose of the pull request
getFileContentBytesFromLocalread entire files into memory with no size limit. Downloading a large task log caused OOM on the worker.This PR caps the read at 47 MB and returns a clear error for oversized logs.
Why 47 MB, not 64 MB? The
byte[]is JSON-serialized as base64 (~1.33× expansion) before RPC transmission. 47 MB raw → ~63 MB JSON body, staying under the 64 MBmaxFrameSizeinTransporterDecoder. 64 MB raw would produce ~86 MB body and be rejected byTooLongFrameException.close #18459
Brief change log
LogUtils: addMAX_LOG_DOWNLOAD_SIZE = 47 MB;getFileContentBytesFromLocalstops reading once the limit is reached.LogServiceImpl: checks file size before reading; returnsERRORwith a clear message for oversized logs instead of silently truncating.Verify this pull request
This change added tests and can be verified as follows:
LogServiceImplTest: a 48 MB file returnsERRORwith message containing "exceeds maximum download size"../mvnw spotless:checkpasses.Pull Request Notice
Pull Request Notice