Conversation
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR adds exec() to the container API, enabling short-lived process execution inside DO-attached containers with stdin/stdout/stderr streaming, combined output mode, and a buffered output() helper.
Issues (by severity)
- [MEDIUM]
demuxDockerExecOutputsilently swallows DISCONNECTED exceptions without ending stdout/stderr streams, which can leave downstreamReadableStreamconsumers hanging forever. - [MEDIUM]
output()passeskj::maxValuetoreadAllBytes(), meaning a malicious or buggy container process can produce unlimited stdout/stderr and OOM the worker. - [LOW]
ExecProcess::kill()accepts signal 0, which has nokilleffect on POSIX — but the RPC schema usesUInt32while the JS API usesint, creating a mismatch wherekill(0)is rejected on the JS side but would be valid on the capnp side. Consider harmonizing. - [LOW] In
demuxDockerExecOutput,stdout/stderrMaybeowners have already been moved into the catch block's scope when usingtry/catchwith coroutines — theKJ_IF_SOMEin the catch block accesses the same locals from the outer scope. This works because the catch is in the same scope, but is fragile.
This review was generated by an AI assistant and may contain inaccuracies.
|
Review posted successfully on PR #6457. Here's a summary of the findings: 5 inline comments posted as a batch review:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6457 +/- ##
==========================================
- Coverage 70.98% 70.58% -0.40%
==========================================
Files 429 429
Lines 120725 121401 +676
Branches 19008 19104 +96
==========================================
- Hits 85692 85689 -3
- Misses 23740 24417 +677
- Partials 11293 11295 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1b71a48 to
97aaf22
Compare
|
The generated output of |
Merging this PR will improve performance by 25.85%
Performance Changes
Comparing Footnotes
|
gpanders
left a comment
There was a problem hiding this comment.
container-client.c++ implementation looks good to me (with a couple small comments).
|
API LGTM, did not review implementation |
…h stdio We also have to rename to (stdout|stderr|stdin)Writer all capnp variables and methods
Makes
this.ctx.container.exec()available under experimental.The API is designed to be very extensible around ReadableStream/WritableStream, and familiar for the JS crowd.
I believe most of the bulk in container.c++ is defining the schemas.
Local dev:
It just calls the /exec APIs from Docker, but it turned out a not so easy task to keep it dead-simple. The reasoning is that Docker uses a multiplexed stream with streamIds that we have to parse. Not only that, this stream starts when we call
/exec/<id>/start, which needs aUpgrade: tcpheader (why not WebSockets...?). This makes us unable to use the KJ HttpClient, and hijack ourselves the connection by writing the headers and reading them with various KJ utils.