[DONOTMERGE] Workerd interceptOutboundHttp integration#6161
[DONOTMERGE] Workerd interceptOutboundHttp integration#6161
Conversation
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR improves sidecar container recovery by: (1) destroying stale sidecars before creating new ones in ensureSidecarStarted, (2) accepting 304/409 from Docker's start endpoint, and (3) testing idempotency after abort.
Two issues found, ranked by severity:
| JSG_REQUIRE(response.statusCode == 204, Error, | ||
| "Starting network sidecar container failed with: ", response.body); | ||
| JSG_REQUIRE(response.statusCode == 204 || response.statusCode == 304 || response.statusCode == 409, Error, | ||
| "Starting network sidecar container failed with: ", response.statusCode); |
There was a problem hiding this comment.
The error message lost the response body — the old code had response.body, which contains Docker's human-readable error. With only response.statusCode, a failure here produces something like "Starting network sidecar container failed with: 500" which is much harder to debug than the original which would include Docker's error JSON.
| "Starting network sidecar container failed with: ", response.statusCode); | |
| "Starting network sidecar container failed with: ", response.statusCode, " ", response.body); |
| network, kj::str(dockerPath), kj::HttpMethod::POST, kj::mv(endpoint), kj::str("")); | ||
| JSG_REQUIRE(response.statusCode == 204, Error, | ||
| "Starting network sidecar container failed with: ", response.body); | ||
| JSG_REQUIRE(response.statusCode == 204 || response.statusCode == 304 || response.statusCode == 409, Error, |
There was a problem hiding this comment.
Nit: accepting 409 ("container conflict") silently seems risky — Docker returns 409 when the container is paused, restarting, or otherwise in a bad state. Since the line above freshly creates the sidecar, getting 409 here would indicate something genuinely wrong.
If you need 409 for the recovery path (stale container left running), the new destroySidecarContainer call before createSidecarContainer should already handle that. Consider dropping 409, or at minimum adding a comment explaining when 409 is expected.
|
I'm Bonk, and I've done a quick review of your PR. This PR improves sidecar container recovery by destroying stale sidecars before creating new ones in Two issues posted as review comments:
|
1 participant
No description provided.