Repository navigation
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
ExecuteHandler buffered command output without limit, so a command with large output could OOM-kill PicoD and lose the sandbox session. Keep at most --max-output-size bytes (default 10 MB) per stream, discard the rest without breaking the pipe, and report truncation via stdout_truncated/stderr_truncated and a note in stderr. Signed-off-by: GuTS805 <alok020505@gmail.com>
810c684 to
e84606c
Compare
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #494 +/- ##
==========================================
+ Coverage 59.98% 61.91% +1.92%
==========================================
Files 36 35 -1
Lines 3589 3684 +95
==========================================
+ Hits 2153 2281 +128
+ Misses 1216 1178 -38
- Partials 220 225 +5
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:
|
Signed-off-by: GuTS805 <alok020505@gmail.com>
|
Has this occurred in actual usage? The reproduction demonstrates the risk, but I’m not yet convinced we need a configurable output limit and API changes at this stage. |
|
Not from a user report, but I reproduced it on the e2e kind setup (
If the flag and API changes are too much for now, I can reduce it to a fixed limit plus a stderr note only. Let me know. |
|
This seems like a common challenge for code execution services. Could we look at how other projects handle large command output before settling on an approach? |
|
well I checked a few (Piston, LangChain shell tool, kubelet logs). Most of them cap the output per stream and set a truncated flag, which is what this PR does. Streaming is the other option, but it's a bigger change and already listed as a future enhancement in the PicoD proposal. |
What type of PR is this?
/kind bug
What this PR does / why we need it:
POST /api/executebuffered stdout/stderr in unboundedbytes.Buffers. A command with large output (catof a big file, a print loop,yes) could grow PicoD's memory until the sandbox container was OOM-killed, losing the session's workspace and state.This PR:
cappedBufferwriter that keeps the first N bytes per stream and discards the rest.Writealways reports the full length, so the command keeps running instead of dying on a broken pipe.--max-output-sizeflag (default 10 MB per stream;0or negative means the default).stdout_truncated/stderr_truncatedtoExecuteResponse. Both areomitempty, so untruncated responses are byte-for-byte unchanged. A short note is also appended tostderr.execute_command_resultreturns both flags and logs a warning on truncation. The LangChain integration now passes the realtruncatedvalue instead of a hardcodedFalse.docs/design/picod-proposal.md.Measured on Linux with 300 MB of command output:
Sys)Which issue(s) this PR fixes:
Fixes #493
Special notes for your reviewer:
cappedBufferdeliberately does not embedbytes.Buffer. os/exec copies output withio.Copy, which would use an embeddedReadFromand bypass the limit.TestCappedBuffer_IOCopyRespectsLimitcovers this.outputLimit,appendTruncationNotes), soExecuteHandlerstays within the gocyclo limit of 15.execute.goas fix(picod): stop child processes when execute times out #473 (process-group kill on timeout). Whichever merges second needs a small rebase aroundcmd.Stdout/cmd.Stderr.cappedBuffertests.go test ./pkg/picod/passes on Linux,golangci-lintis clean, andruffplus SDK unit tests pass.Does this PR introduce a user-facing change?: