feat(provider): get-service-config provider request - #14175
Conversation
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The control-channel implementation is well-structured. The synchronous request/response loop is simple and correct for the typical case. One finding in the example code, and one lower-confidence note below.
Lower-confidence findings (not posted inline)
- [low] pkg/compose/plugins.go:208 — Potential deadlock when large service config response exceeds OS pipe buffer (confidence: weak 52/100). The synchronous
responses.Encode()write to the provider's stdin happens in the same goroutine that reads from stdout. If the marshalledtypes.ServiceConfigever exceeds the OS pipe buffer (~64 KB on Linux), and the provider is blocked waiting for this response before writing more output, neither end makes progress. Typical configs are well under 1 KB so this is unlikely in practice, but a goroutine for the write would eliminate the risk entirely.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
cc1654d to
a6f92eb
Compare
279831f to
3b98f63
Compare
3b98f63 to
9250d4a
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium-severity correctness issue found in the example code added by this PR.
9250d4a to
500f4e0
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two findings in the newly introduced get-service-config protocol code:
- docs/examples/provider.go:108 — error path emits hand-rolled JSON that breaks on special characters in the error string, contradicting the comment on the very next line.
- pkg/compose/plugins.go:183 — synchronous write to the provider's stdin pipe inside the stdout-reading loop can deadlock when the serialized
ServiceConfigJSON exceeds the OS pipe buffer (~64 KB).
glours
left a comment
There was a problem hiding this comment.
nit: this hand-rolls what json.Decoder already does — bufio.Reader.ReadBytes('\n') + json.Unmarshal assumes each message is exactly one newline-terminated line, which happens to hold today but is a more fragile assumption than just decoding from the stream directly.
docs/examples/provider.go, added by this same PR, already uses the simpler json.NewDecoder(os.Stdin) / .Decode(&config) pattern, worth mirroring it here for consistency (and it lets you drop the bufio import too).
500f4e0 to
1dcf08b
Compare
|
/review |
|
👋 Heads up: The |
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
Two medium/low-confidence correctness findings in the new get-service-config control-channel handler. The core issue is that goroutines dispatched to write the config reply to stdin are not tracked by a sync.WaitGroup, so they can write to a closed pipe when the read loop exits — silently dropping the reply to the provider.
1dcf08b to
e1ce900
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One medium-severity finding introduced by this PR.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
One CONFIRMED medium-severity race condition found in the new get-service-config control channel implementation.
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The get-service-config protocol implementation is well-structured. The concurrency design in plugins.go correctly handles the bidirectional stdin/stdout channel:
- Goroutine coordination:
answers.WaitGroup+stdinMumutex properly serialize writes to the provider's stdin pipe, preventing interleaving and buffering deadlocks. - Defer ordering: The two-branch defer (close-then-wait on error paths, wait-then-close on clean exit) is the right design — it prevents hanging when the provider is still alive and reading, while ensuring in-flight writes complete on normal exit.
- Closure safety:
payloadis declared locally in theGetServiceConfigTypecase block beforeanswers.Add(1), so each goroutine captures its own copy — no shared-variable capture issue. processExitedaccess: Read and written exclusively in the main goroutine (the defer runs in the same goroutine aftercmd.Wait()) — no data race.- Decoder reuse: The example provider correctly creates
json.NewDecoder(os.Stdin)once and reuses it across requests, avoiding the read-ahead pitfall. - JSON safety: Config values written to setenv messages go through
json.Marshalrather than string interpolation — no injection risk.
No bugs introduced by this PR.
Providers could not see the definition of the service they manage:
options had to be duplicated between the compose file and the provider,
or the provider had to re-resolve the model on its own.
A provider may now emit {"type": "get-service-config"} on stdout;
compose answers on the provider's stdin with one JSON line holding the
resolved canonical configuration of the provider's own service,
straight from the in-memory model. The message can be repeated; each
occurrence is answered with one line. Detection is by construction: a
compose that predates the message aborts on it and never writes to
stdin, so the provider treats EOF as 'unsupported, upgrade compose'.
The example provider demonstrates the round trip, backed by an e2e
scenario; unit tests drive executePlugin against a helper-process
provider and cover the injection.
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
e1ce900 to
6a4b684
Compare
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The get-service-config control-channel implementation is well-structured. The goroutine-based stdin write pattern correctly avoids deadlocks (large configs won't block the stdout read loop), the LIFO defer ordering is sound for both normal and error exit paths, and the processExited flag correctly gates the two cleanup strategies. The omitempty addition on JsonMessage.Message is safe: Compose only decodes JsonMessage (reads provider stdout) and never encodes it, so omitting the empty field from the test helper's output has no impact on real providers. The provider example correctly reuses a single json.Decoder for stdin across multiple requests, avoiding read-ahead data loss.
Providers could not see the definition of the service they manage: options had to be duplicated between the compose file and the provider, or re-resolved outside the running process.
A provider may now emit
{"type": "get-service-config"}on stdout; compose answers on the provider's stdin with one JSON line holding the resolved canonical configuration of the provider's own service, from the in-memory model. A compose that predates the message aborts on it and never writes to stdin, so a provider treats EOF as "unsupported, upgrade compose".Scoped down from the original version:
addhostand the links-style variables convention are superseded by the relay approach (#14193).🤖 Generated with Claude Code