docker-agent found that streamImageTo returned a plain error (channel
intact) when the announce write itself failed -- for the success
announce, and for the error announce on a platform-parse/ImageSave
failure. In both cases the provider never received anything to read
and was left blocked on ReadString, while the caller, seeing a plain
error, never closed stdin either -- a deadlock.
Both paths now return streamAbortedError when the announce write
fails, matching the existing mid-stream/close failure handling so the
caller's errors.As check closes stdin and unblocks the provider.
Pinned by two new regression tests using a writer that always errors,
verified by reverting the fix locally and confirming both fail with a
plain error instead of streamAbortedError.
Signed-off-by: Nicolas De Loof <[email protected]>
Three fixes from glours' review:
- ensureProviderImages now skips a provider service declaring neither
image: nor build: (legal for a pure resource provider, e.g. one
backing a remote Amazon RDS instance). Without the guard, such a
service reached pull with a fabricated "<project>-<service>"
reference (GetImageNameOrDefault) under source=registry
(providerImageSource treats Build == nil as registry-authoritative)
-- a name that was never real. Already reachable through existing
e2e fixtures (TestDependsOnMultipleProviders' provider1/provider2),
only a no-op today because the demo pull() short-circuits on an
unset env var, not because of any guard here.
- localImageFacts now resolves --digest through the manifest-aware
localContentDigest instead of the raw ImageInspect ID. Under
BuildKit provenance attestations, the raw ID covers the attestation
manifest too and changes on every build even when the runnable image
content didn't (#13636), which would make the provider re-sync on
every up for no reason -- defeating docs/extension.md's own
"identity test" contract for --digest. Threads service.Platform
through from runProviderPull so the resolved manifest matches what
the provider will actually run.
- streamImageTo now distinguishes a failure before the success
announce (invalid platform, ImageSave error -- a clean, well-formed
error line, channel intact) from one after it (the chunked body
truncated, channel unresynchronizable) via a new streamAbortedError
type. Only the latter closes stdin; the provider protocol supports
several sequential get-image requests per pull invocation, and
closing stdin on a clean pre-stream error was killing any other
independent request in the same invocation for no reason.
Each fix is pinned by a regression test, verified by reverting the
fix locally and confirming the new test catches it.
Signed-off-by: Nicolas De Loof <[email protected]>
A service declaring only build: (no image:) answered get-service-config
with an empty "image" field: service.Image is the YAML-declared value,
and compose-go never fills it in for this case (every other caller of
GetImageNameOrDefault in this package exists precisely because of that).
The provider only ever sees this response, never the model compose
builds internally, so an sbx-style provider building its own runtime
image rejected the service outright with "defines no image".
The response now carries the resolved name (api.GetImageNameOrDefault)
instead - the same one the image phase already built and tagged by the
time a provider asks. The build directive itself is cleared before
marshaling: a provider has no builder to run it against, only an image
identity to run.
Pinned by a regression test using the existing fake-provider harness,
verified by ablation to fail on either half of the fix reverted.
Signed-off-by: Nicolas De Loof <[email protected]>
extension.md gains the Image distribution section: invocation contract
(--image/--digest/--created/--source/--policy), the get-image request
and its chunked image-stream answer (zero-chunk terminated, stdin
exclusivity and drain-before-next-answer contract), the two-branch
decision guidance (digest as identity, created as ordering fallback,
reproducible builds caveat), and the support-by-presence metadata
convention is now stated as the general rule. The example provider
implements pull end to end — request, stock chunked reader, hash
recorded at PROVIDER_PULL_MARKER — and the e2e scenario locks the
whole path: up builds the service image and streams it to the provider
under the local verdict, compose pull re-invokes it under the
freshness contract.
Signed-off-by: Nicolas De Loof <[email protected]>
startService errored with 'no container to start' whenever the
project-wide container listing came back empty, before the per-service
filtering that lets provider services pass through. A project whose
services are all provider-backed legitimately reaches the start phase
with zero containers. The empty-list error is now softened for
provider services only — a relay container deployed for published
endpoints still goes through the regular start path.
Signed-off-by: Nicolas De Loof <[email protected]>
ensureImagesExists and docker compose pull now give provider-backed
services their turn: compose invokes the provider's pull command with
the image identity and the state of the local daemon cache
(--image/--digest/--created), plus two verdicts compose alone can
compute so providers never re-implement that arbitration:
- --source: the authority for this invocation — local (the daemon's
image is the desired state: build-only service, pull_policy build,
or an image the current run just built) or registry (resolve the
reference upstream — including the CI workflow where build is only
the recipe used to publish the image consumers pull);
- --policy: missing on the up path (a usable version suffices) or
always on compose pull (ensure freshness of the authority).
digest/created describe the cache, they are not instructions: the
provider persists them as the bookkeeping keys of what it ingested —
digest as identity test, created as the ordering fallback for backends
that cannot preserve digests. Providers without the metadata block are
skipped: behavior unchanged.
Providers are independent, so their pulls run without fail-fast
cancellation — the first failure no longer kills its siblings mid-run
and mangles their reports into context-canceled noise; every provider
runs to completion and every failure is reported, prefixed by service.
On the pull path the provider phase runs unconditionally — skipping it
because an unrelated service failed to pull would silently leave
provider runtimes stale — and its error is a per-service pull error,
following their exact regime: reported with them, suppressed by
IgnoreFailures. A fatal errgroup error is reported joined with the
accumulated per-service context instead of dropping it. The provider
runs under the root context: the errgroup context is canceled once
Wait returns and would kill the provider process on the spot.
Signed-off-by: Nicolas De Loof <[email protected]>
Providers opt into image distribution by declaring a pull block in
their metadata — the presence of the command block is the declaration
of support, generalizing the stop precedent. During its pull command a
provider can request the service image from the local daemon with a
get-image message (optionally platform-narrowed); compose answers on
stdin with one image-stream JSON line then the tar as HTTP/1.1 chunked
data (RFC 9112): length-prefixed blocks need no in-band delimiter in
binary data, the zero-length chunk marks a COMPLETE transfer, and any
stock chunked reader consumes it. There is deliberately no trailer nor
final CRLF after the zero chunk — a stock reader stops there without
consuming further bytes, so the next stdin answer starts clean for a
provider requesting several images. An export failure is announced
in-band through the error field so the provider is never left waiting;
a failure after the announce closes the answer channel so the
truncation is observable as EOF mid-chunk instead of a stream nobody
will finish. The stream is exclusive on stdin for its whole duration —
the framing invariant behind the lock.
get-image is scoped to the pull command: emitted during up, down or
stop it is a protocol error rather than served, so an image export can
never stall another lifecycle command.
Message dispatch moves out of executePlugin into handlePluginMessage,
which also keeps the function under the complexity threshold.
Signed-off-by: Nicolas De Loof <[email protected]>
Binding the announced gateway alone restricts by destination address, not
by the interface traffic arrives on, so another bridge can still reach it
by IP. A provider that pins the socket to the relay-link bridge's own
interface (SO_BINDTODEVICE on Linux) keeps the endpoint relay-only.
Signed-off-by: Nicolas De Loof <[email protected]>
Unify the up and down paths on one removal helper that tolerates both
NotFound and Conflict and emits the same progress events as removeNetwork,
build one down op per relay-link network gated on the shared limiter (so
the empty-ops warning is reachable again), and share the label filter.
Reword the isolation claim: the relay-link network is joined by nothing
else, but its gateway is a host address that local containers can still
reach by IP; only a provider binding exclusively that address keeps the
endpoint off loopback and LAN-facing addresses.
Signed-off-by: Nicolas De Loof <[email protected]>
get-relay-info answers pointed a locally-bound provider at one of the
dependents' own networks: shared infrastructure every other service on
it could also reach, and no more isolated than any project bridge.
Provision a dedicated, internal:true bridge network scoped to that one
provider service instead, created only when the provider itself sends
get-relay-info -- the provider's own declaration that it binds locally,
never for a provider backing a remote resource. The relay container
joins it alongside the dependents' networks it already joins; it is
torn down with the relay on endpoint changes and on down, including
the no-compose-file down path.
The network's lifecycle is hardened against the races a shared,
project-scoped resource runs into:
- a NetworkCreate conflict on the deterministic name re-checks by
label (not just by name) before adopting the existing network, so a
genuine name clash with an unrelated, user-declared network fails
loudly instead of being silently adopted into the relay's isolation
boundary;
- a NotFound gateway lookup after losing that conflict falls back to
an unresolved answer instead of propagating the error;
- removeRelayLinkNetwork and ensureRelayLinkNetworksDown both tolerate
a NetworkRemove conflict from the daemon's asynchronous endpoint
disconnect, which can race the relay container's own removal;
- ensureRelayLinkNetworksDown accumulates failures with errors.Join
across every relay-link network of a project instead of aborting on
the first one, and skips (warn-only) a network that still has active
endpoints, mirroring removeNetwork's existing tolerance for regular
project networks.
docs/extension.md documents that the "desktop" network name in the
get-relay-info answer (under Docker Desktop) is an opaque placeholder,
never a real Docker network a provider can inspect or connect to.
Signed-off-by: Nicolas De Loof <[email protected]>
compose-go's loader now silently renames both deprecated
x-initialSync and x-develop extensions to their official
initial_sync and develop attributes before compose ever sees them
as extensions, so the legacy lookups and deprecation warnings for
both can no longer be reached. That's why the e2e test asserting on
the x-initialSync warning broke under the bump.
Simplify both lookups to rely on the promoted fields directly, and
rewrite the e2e test to check the legacy attribute still triggers a
real initial sync instead of asserting on an unreachable message.
Signed-off-by: Guillaume Lours <[email protected]>
down's outer ops dispatch bounded only how many ops ran concurrently,
separate from the semaphore already shared by image removal -- a
single network plus --rmi could spend up to 2x the configured
budget. Network and volume removal now share that same semaphore.
up's post-attach log streaming called ContainerInspect directly and
passed no limiter to doLogContainer, unlike `compose logs --follow`'s
equivalent path, so it never shared --parallel's budget either.
The plan executor also held a node's slot for its whole dependency
wait instead of just its own work, and internal/sync/tar.go's file
sync used a hardcoded cap of 16 instead of --parallel.
Signed-off-by: Guillaume Lours <[email protected]>
Co-Authored-By: Claude Sonnet 5 <[email protected]>
`docker compose cp` fanned out to every matched container through a
bare unbounded errgroup, ignoring --parallel entirely -- the same
class of gap already closed everywhere else in this package. Routed
it through the existing forEachContainerConcurrent helper instead of
a bespoke fan-out.
inspectWithSlot and doLogContainer both release a semaphore slot
early via an explicit call rather than a defer, so a panic between
acquire and that release leaked the slot forever. Guarded both with
a panic-safe release, scoped tightly in doLogContainer so a later
panic in the same function can't trigger a double release.
Signed-off-by: Guillaume Lours <[email protected]>
Co-Authored-By: Claude Sonnet 5 <[email protected]>
down and stop rebuilt their per-container limiter on every service
visit, so independent services could each spend a full budget
concurrently -- the same leak already fixed on restart. start's
InDependencyOrder call had no concurrency option, leaving every
`up`'s container-start phase fully unbounded. wait.go goes back to
unbounded on purpose: ContainerWait blocks per container
indefinitely, so capping it would serialize waits meant to run
concurrently.
Also extracts the shared per-container fan-out into one helper reused
by restart/down/stop, and dedupes the concurrency-guard test fixtures.
Signed-off-by: Guillaume Lours <[email protected]>
Logs --follow reused the bounded errgroup for both the indefinite log
streams and the monitor goroutine, so once maxConcurrency streams
opened the monitor never started and further services never showed
logs. restart gave each service its own budget, so ready services
could multiply maxConcurrency instead of sharing it.
Bound both with a semaphore that gates only the connect/restart call,
never the indefinite work that follows, sharing one instance across
restart's services. Adds regression tests, plus one guarding the
topological node order a bounded errgroup now relies on in executor.
Signed-off-by: Guillaume Lours <[email protected]>
errgroup.SetLimit(0) means "allow zero goroutines", not "unlimited",
and maxConcurrency's Go zero-value is 0 — only NewComposeService sets
it to -1 explicitly. Any composeService{} literal built without it
(common in tests) silently deadlocked at every call site that called
SetLimit unconditionally.
Separately, --parallel/COMPOSE_PARALLEL_LIMIT was only ever wired
into pull, push, and the dependency-graph traversal, despite the docs
promising a generic bound on "concurrent engine calls". Every other
bulk operation (kill, pause, down, the up/create plan executor, logs,
ps, top, wait, restart, remove, images, model pulls, watch) launched
one goroutine per container/image/DAG-node with no cap at all.
Fix both via a shared newLimitedErrgroup helper applied at every
fan-out site, threading maxConcurrency through forEachContainerConcurrent
and ImagePruner, which had no access to composeService. Add a
regression test for the zero-value case.
service_containers.go's waitDependencies is intentionally left
unguarded: it's a per-dependency ticker poll, not a burst of engine
calls.
Signed-off-by: Guillaume Lours <[email protected]>
Wait has no real container to observe in dry-run: getContainers
falls back to the live daemon and finds nothing, so the command
failed with a misleading "no containers for project" error instead
of signaling upfront that the operation doesn't apply under
simulation.
Signed-off-by: Guillaume Lours <[email protected]>
DryRunClient fakes container state as permanently running/healthy,
which never satisfies the exited-state check that
service_completed_successfully waits for. With up -d passing no
default timeout, the wait loop polled forever and dry-run never
returned, even though no container was ever really started.
Signed-off-by: Guillaume Lours <[email protected]>
The start flow hand-rolled its ContainerList without the ConfigHashLabel
presence filter every other listing applies (getDefaultFilters) — the
actual source of pre_start hook runners leaking into startService and
waitDependencies, patched downstream by isNotHookContainer. Routing
through getContainers fixes the leak at the source, like
getContainersByService already does; the isNotHookContainer guards stay
as defense-in-depth.
The dry-run client keyed its ContainerList interception on the calling
function: caller "start" answered from the faked-creations cache, while
caller "getContainers" fell back to the real daemon once the cache was
seeded — so routing start through getContainers made dry-run-created
containers invisible to the start phase ("service has no container to
start", TestUpDryRunMissingImage). The getContainers case now keeps
answering from the seeded cache, where faked creations land.
Signed-off-by: Nicolas De Loof <[email protected]>
Five more findings landed on the previous commit (three from a fresh
copilot-pull-request-reviewer pass, two from docker-agent):
- **Duplicate hook containers** (observed_state.go): a runner created
before the ConfigHashLabel exclusion existed (or by an older compose
version) still carries that label, so it matches getContainers' own
filters and is already present in `raw` -- appending getHookContainers'
result unconditionally classified it twice, scheduling two removals for
one ID. Deduplicated by container ID before classification. Locked in by
TestCollectObservedState_LegacyHookRunnerNotDuplicated.
- **Hook runners misjudged as a satisfied dependency** (service_containers.go):
waitDependencies filtered a dependency's containers the same way
startService's own replica filter used to (isService + isNotOneOff, no
hook exclusion) -- a dependency's retained/exited runner could be read as
the dependency itself, either masking a genuinely missing dependency or
reporting an unhealthy one that's really just a diagnostic leftover.
Added isNotHookContainer there too.
- **per_replica bypassing the stale-runner purge** (reconcile.go): the
early return added for #14259 skipped the purge loop entirely, so
runners from a prior per_replica: false generation would never be
cleaned up once a hook switched to per_replica: true. Reordered: the
purge always runs when there's a service to reconcile; only the create
loop is skipped for per_replica. Locked in by
TestReconcileContainers_StaleHookRunnersPurgedEvenWhenPerReplica.
- **keptRunning ignores stops already planned elsewhere** (reconcile.go):
a replica reported running in the observed-state snapshot but already
scheduled for a stop by planRecreateNetworks or planStopDependents
(network recreate, a restart: true dependency cascade) would not
actually be running by the time the start phase evaluates pre_start --
but keptRunning didn't know that, so no runner got planned and start
later failed with "no hook runner container found". Only counts a
replica as kept when stoppedByPlan has no entry for it. Locked in by
TestReconcileContainers_HookRunnerPlannedWhenCascadeStopsRunningReplica.
- **Success-removal uses the cancellable ctx** (pre_start.go): inconsistent
with the cancellation-cleanup path in the same file, which already uses
context.WithoutCancel to guarantee removal survives a Ctrl-C landing in
the narrow window right after a hook's success. Matched the pattern.
Signed-off-by: Nicolas De Loof <[email protected]>
A GitHub Copilot PR review (docker/compose#14221#pullrequestreview-5300897216)
found three real issues in the reconciliation-plan-based hook runners, all
confirmed by direct reproduction against a live daemon before being fixed:
- **Hook runners excluded from discovery, stale runners persist.**
collectObservedState's single getContainers call requires ConfigHashLabel
(getDefaultFilters), which hook runners deliberately never carry -- so they
never reached the classification loop that populates HookContainers, and
planPreStartHookRunners never saw a stale runner to purge. Reproduced: a
failed hook run followed by a second `up` failed with "container name ...
already in use" instead of recovering. Fixed by listing hook containers
separately, by project+hook label alone (new getHookContainers), and
merging them in before classification. Locked in by a new e2e scenario,
TestPreStartHookRetryAfterFailureSucceeds, that reproduces the exact
failure-then-retry sequence.
- **Prepared hook runners misclassified as service replicas.** start's
containers.filter(isService(name), isNotOneOff) has no reason to exclude a
hook runner: it carries the same project/service labels as a real replica
and no one-off label either. Added isNotHookContainer to the filter chain
in startService, so a hook runner is never counted as (or started as) a
surviving replica.
- **Per-replica hooks create runners before unsupported configuration is
rejected.** per_replica is not yet supported (#14259): runPreStart
validates every hook up front and rejects the whole service before
executing any of them, but planPreStartHookRunners had no equivalent
check, so create/up would plan (and leave behind) runners for a service
whose hooks could never run. Mirrored the same up-front, all-or-nothing
validation in planPreStartHookRunners.
Signed-off-by: Nicolas De Loof <[email protected]>
pre_start hook containers become first-class resources of the
reconciliation plan. The reconciler plans one OpCreateHookContainer per
declared hook — gated on the same predicate the start phase uses to run
the hooks: a replica to start and no running replica SURVIVING the plan
(a recreated replica is not running at start time, so its runners are
planned too). Runner creation depends on every replica node (the
executor resolves the VolumesFrom target against a final live view),
on the infrastructure, and on the purge of previously observed runners:
names are deterministic (<project>-<service>-pre_start-<i>) so repeated
plans converge on the same container instead of accumulating anonymous
ones, and a new HookIndexLabel ties each runner to its hook.
runPreStart becomes pure execution: it looks up the created-state
runner for each hook by label and never creates one. A declared hook
without a prepared runner is an actionable error naming the
reconciliation command (docker compose up <service>) — the accepted
consequence is that stop-then-start of a hooked service errors, since
runners are consumed on success (locked by e2e). Success removal,
failure retention and cancellation cleanup are unchanged.
Runners stay invisible to the start/ps container listings — they carry
no ConfigHashLabel, which getDefaultFilters requires — and down keeps
removing them by hook label.
Rebased onto main, which diverged substantially since this branch was
opened (the unrelated container-spec-layering refactor, #14093,
restructured compose-go's ServiceConfig/PreStartHook types this PR also
touches). Reconciling pre_start.go/pre_start_test.go against that
refactor surfaced a real bug: createPreStartContainer builds the hook's
config via the shared getCreateConfigs, which unconditionally stamps a
ConfigHashLabel — silently contradicting this same commit's own
"invisible to ps/start" guarantee above, since that label is exactly
what getDefaultFilters requires. Fixed by deleting the label after
getCreateConfigs returns, with a test asserting its absence (an
independent local review, via the GitHub Copilot CLI, and a
fresh-context review agent both converged on catching this during
rebase verification). Also restored two security/label tests that only
existed on one side of the rebase (SecuritySensitiveFieldsHonored,
HookLabelsMerged), ported to call createPreStartContainer directly.
Also addresses glours' three review comments on this PR: hook-container
create operations for a service no longer chain serially (each depends
on the same shared containerNodes+infraDeps+purges set, letting the
executor create them in parallel), the corresponding plan-string test
assertion, and a clearer missing-runner error message hinting this
isn't necessarily a planning bug.
Signed-off-by: Nicolas De Loof <[email protected]>
pre_start hooks run in ephemeral containers. When a run fails, the
runner is deliberately retained for post-mortem inspection, and the next
run purges it — but that purge lived entirely inside the imperative
primitive, invisible to the reconciliation plan. It is now a plan
operation: when pre_start is going to run again (hooks declared, no
replica running at observation — the imperative gating), the plan emits
one best-effort RemoveContainer per stale runner, dropping its anonymous
volumes, exactly the warn-only semantics of the imperative purge that
remains in place as backstop.
Observed state learns to tell hook containers apart: they carry no
container-number label and previously classified as a service replica
numbered 0. They now land in a dedicated HookContainers bucket the
reconciler plans purges from.
The imperative primitive is also split into its lifecycle-free execution
piece (execPreStartHook: start, wait, log streaming, retain-on-failure)
and the create/remove pieces around it, recomposed identically in
runPreStart — locked by the existing characterization tests. This
prepares moving hook-container creation and post-success removal into
the plan once the start phase lands (#14200).
Signed-off-by: Nicolas De Loof <[email protected]>
removeTaggedImagesOp switched to a plain errgroup.Group so one
image's failure can't cancel the others still in flight, but
Wait() only ever surfaces the first error returned to it,
silently dropping any other concurrent failures. Aggregate every
failure instead, the same way removeImages already separates
tolerated outcomes (still-in-use, already-gone) from real errors
before joining them.
Also give a tagged-image listing failure its own "Tagged images"
label instead of the generic Compose-wide one, matching the
dedicated label the dangling-image path already has.
Signed-off-by: Guillaume Lours <[email protected]>
removeTaggedImagesOp shared an errgroup.WithContext-derived ctx across
every image removal goroutine. A genuine failure on one image canceled
that ctx, so every image still queued behind it failed with "context
canceled" instead of ever being attempted -- a regression from the
independent per-image ops it replaced.
Also extracts the Conflict/NotFound tolerance rule and the
orphaned-image check into single shared helpers instead of leaving two
independently maintained copies to drift apart, and drops a return
bucket nothing outside its own test consumed.
Signed-off-by: Guillaume Lours <[email protected]>
When a dangling-image batch had both a genuine removal error and an
image tolerated as still in use, with nothing actually removed, the
error path reported only the genuine failure and silently dropped the
still-in-use count — the same information loss the "Removed" label
was already fixed to avoid, just on its error-reporting sibling
branch. Both facts now show up together regardless of which path the
batch's outcome takes.
Signed-off-by: Guillaume Lours <[email protected]>
A dangling image still in use failed the whole op, while a tagged
image or volume in the same state only gets a swallowed warning via
removeResource — same daemon condition, inconsistent outcome. Dangling
images now get the same tolerance. Also stop claiming "Removed" when
a removal failure means nothing actually was, bound the tagged-image
fan-out the same way the dangling one already is, and share the
listing+filter logic between the down and watch paths instead of
duplicating it.
Signed-off-by: Guillaume Lours <[email protected]>
Found during review of #14219: `ensureImagesDown` still listed tagged
images to prune synchronously and returned the error immediately,
before `down` had scheduled the already-built network/volume cleanup
ops onto its errgroup — the exact same hazard just fixed for dangling
images, on a different code path. The listing is now deferred to when
its own op runs, so a failure surfaces as a visible error without
blocking sibling ops.
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
`down --rmi` always printed "Dangling images ... Removed" even when
there was nothing to remove, on every repeat `down` (#14219). The
dangling-images removal now: stays silent when there's nothing
eligible, same as removeNetwork's silent skip; shows a visible error
instead of the misleading label when the listing itself fails, without
aborting the rest of `down`; and keeps the "Removed" status visible
alongside the real daemon error when some images fail to remove for a
genuine reason (not a benign already-gone race).
Co-Authored-By: Claude Sonnet 5 <[email protected]>
Signed-off-by: Guillaume Lours <[email protected]>
A service migrated from regular replicas to a provider left its old
containers standing: not converged (a provider service has no replicas
to converge), not orphaned (the service is still in the model). The
first up after the migration then failed creating the relay, because
the leftover replica still held the service's canonical container name
the relay takes over. And even without the name conflict (replicas
2..n of a scaled service), stale replicas kept the service's network
alias, competing with the relay on DNS and serving outdated traffic.
The reconciler now plans a stop+remove for every observed non-relay
container of a provider service, and the RunProvider node depends on
those removals: the canonical name is free by the time
ensureServiceRelay creates the relay. The relay itself (RelayLabel) is
the provider service's legitimate container and is left alone —
converged by ensureServiceRelay, not by the plan.
The e2e scenario migrates a deployed service to a provider and locks
the replacement (Recreated, relay reachable at the compose-native
address); the Scenario DSL gains FromFile — a local mirror of
FromRemote — to switch the model between steps.
Signed-off-by: Nicolas De Loof <[email protected]>
Address glours review on #14258: TestExecutePlugin_GetRelayInfo and
TestExecutePlugin_GetRelayInfoDesktop only exercised the happy path.
Add TestExecutePlugin_GetRelayInfoUnresolvedGateway to cover the
best-effort contract relay.go omitempty is built around: a
NetworkInspect failure, an IPv6-only gateway, and no gateway at all
must each still list the network with Gateway left empty.
Signed-off-by: Nicolas De Loof <[email protected]>
A provider running its service LOCALLY has to pick the address its
published endpoint binds, and on a standalone Linux engine no address
is both relay-reachable and off the LAN (#14257):
host.docker.internal resolves there to the bridge gateway, which a
loopback-only listener cannot accept, while the wildcard exposes the
port on every host interface. The bundled example provider worked
around it by binding 0.0.0.0.
The new get-relay-info message closes that gap, with compose owning
the platform knowledge so the provider just binds what is announced.
Compose answers with one JSON line listing the networks the relay
would join — the dependents' networks, exactly as selected for the
relay deployment — each with the address a locally-run endpoint should
bind to be reachable from the relay: the network's engine-assigned
gateway on a standalone engine (an address the host owns on that
bridge, reachable from the relay but not from the LAN), and 127.0.0.1
under Docker Desktop — the networks live inside the VM there, and the
host's own loopback is, factually, where a host process is reached
through the Desktop proxy. Resolution is lazy (Desktop detection and
network inspects run only when a provider asks — a remote-resource
provider never does) and best-effort: a missing gateway means "bind
elsewhere".
Compose also announces the message types it accepts in the
COMPOSE_PROVIDER_MESSAGES environment variable of the provider
process: an unknown message stays a fatal protocol error by design
(a provider REQUIRING an unsupported message must fail loudly), and
the announcement is how a provider adapts instead of failing. The
example provider binds the first announced gateway, falling back to
the wildcard only when none is announced; through it,
TestProviderPublishEndpoint exercises the gateway path on Linux CI
and the loopback path on Docker Desktop.
Signed-off-by: Nicolas De Loof <[email protected]>
The plan learns the start vocabulary — inert until a caller opts in
(ReconcileOptions.Scope, zero value keeps today's create-only plans
byte-identical):
- OpWaitCondition, one node per (awaited service, condition),
deduplicated across dependents like networkNodes deduplicates
networks; required:false marks the shared node best-effort, one
required dependent upgrades it. service_started needs no node — a
plain DAG edge to the dependency's chain end expresses it. Health is
deliberately re-observed at execution time: the plan encodes what to
wait for, never a stale observation.
- OpRunPreStart, emitted at plan time only when no replica was running
at observation — the imperative gating — targeting the
lowest-numbered replica.
- OpRunPostStart per container, after its start.
- replica chains: inject+start+post_start of replica n+1 depends on
the end of replica n's chain, today's sequential start order made
visible in golden plans; startChainEnds points at the chain end so a
service_started dependent waits for the whole service, matching
InDependencyOrder semantics.
- scope Start plans starting observed exited/created containers
without converging them (the future compose start); scope
CreateStart appends the start phase to the create plan, start nodes
resolving their target from the create node that materializes the
replica (CreateNodeID, the mechanism OpRenameContainer already
uses).
Lifecycle parity with the imperative engine is load-bearing and
golden-locked:
- dependency conditions are evaluated even when nothing has to start
(waitDependencies runs for every visited service before looking at
what to start), so an up with everything running still fails on an
unhealthy required dependency;
- an exceptional-state replica takes NO start-phase node: its bare
create-phase restart leaves it running when the start phase looks, so
the imperative engine neither re-starts nor injects — and it gates
pre_start like any running replica;
- startChainEnds carries the end-of-visit node set (waits included when
nothing started), so a service_started dependent begins only once the
dependency's whole visit completed, matching InDependencyOrder;
- under scope Start, a scale>0 service with no container at all fails
the plan with startService's exact error.
The "service:<name>:<number>" resource-ID format is built and parsed in
one place (serviceReplicaID/serviceReplicaPrefix/startGroupID), and the
replica sort deliberately carries the plan's determinism over the
unordered containerNodes iteration.
Golden tests only; no executor support yet and no caller passes the
scope. Epic #14081, Lot 1 — reconciler (first item).
Signed-off-by: Nicolas De Loof <[email protected]>
The exact pull-failure wording is daemon-backend dependent (classic
graphdriver: "pull access denied for ..."; containerd image store:
"...: not found"), and OutputMatches only checks stdout while the
daemon's progress line is written to stderr. Assert the image name
appears on stderr instead, which holds regardless of backend or
wording.
Signed-off-by: Ricardo Branco <[email protected]>
wantAbs was just wantSource under a different name, left over from an
earlier revision. Use wantSource directly.
(glours review on #14234)
Signed-off-by: Nicolas De Loof <[email protected]>
BindMountSource kept iterating after locating the bind mount at the
expected destination, so a hypothetical second bind entry at the same
target would trip a spurious mismatch even though the first entry
already confirmed the source. Break out of the loop once a match is
found, per docker-agent review feedback on #14234.
Signed-off-by: Nicolas De Loof <[email protected]>
.Mounts also lists named volumes and tmpfs entries at the same
Destination; matching on Destination alone let those shadow the
intended bind mount, either failing with a confusing volume Source or
silently passing against an empty tmpfs Source. Filter to Type ==
"bind" before comparing, as the function's name and doc promise.
Reported by docker-agent.
Signed-off-by: Nicolas De Loof <[email protected]>
filepath.Abs resolves a relative wantSource against the test process
cwd, not any project directory -- silently comparing against the
wrong base for a future caller passing a relative path, even though
the doc comment frames this as a general check. Reject a relative
wantSource explicitly instead.
(docker-agent review on #14234)
Signed-off-by: Nicolas De Loof <[email protected]>
TestOciRemoteProjectDirectory publishes a project whose only service
mounts a relative volume, then runs `up` on the oci:// artifact with an
explicit --project-directory pointing elsewhere. The relative volume
must resolve against that directory.
It currently fails against the pinned compose-go: LoadConfigFiles
defaults the working dir to the downloaded artifact's own cache
directory whenever a remote resource loader is involved, silently
overriding the explicit --project-directory the same way it would a
mere default -- docker/compose#14224. On Docker Desktop this surfaces
as a hard failure (the cache directory isn't a shared mount); on Linux
it silently mounts the wrong directory, matching the original report.
New BindMountSource check (pkg/e2e/checks.go) pins a service's bind
mount source to an exact expected path, for tests that need to verify
which working directory a relative volume path resolved against.
Needs the compose-go fix (github.com/ndeloof/compose-go@a626c70,
branch 14224-working-dir) to pass -- not yet reflected in go.mod here.
Signed-off-by: Nicolas De Loof <[email protected]>
glours' review on this PR (pullrequestreview-5277692679): stopLateStarter
only ever got armed inside stopOnFirstExit's own closure, so it caught a
service racing the on-exit cascade's sweep but not one racing
gracefulTeardown's (Ctrl+C/SIGTERM) identical one-shot stopApplication
sweep -- same root cause, same file, just the other trigger. Worse for
that path: Ctrl+C can land at t~=0, before monitor.Start even starts and
while s.start() (its own uncancelable context) is still climbing the
dependency graph, so most services may still be created, not running.
The only recovery today is a second manual Ctrl+C.
u.isTerminated already flags "termination is underway" and was already
set by gracefulTeardown (just not consulted for late-starter catching,
and set after stopApplication rather than before it). Extract the
late-starter watch out of stopOnFirstExit into its own listener,
stopLateStarters, armed on u.isTerminated regardless of which path set
it, and register it unconditionally (Ctrl+C works regardless of the
on-exit policy, unlike stopOnFirstExit's own listener). stopOnFirstExit
now sets isTerminated itself before its sweep, symmetric with
gracefulTeardown (also reordered to set it before stopApplication, not
after).
isLateStarter is extracted as a pure predicate, following the existing
shouldFollowStartEvent precedent, and unit-tested (TestIsLateStarter):
the previous fix (ca867482a) shipped only against a flaky test hitting
the on-exit case by chance, with no dedicated regression test at all.
Signed-off-by: Nicolas De Loof <[email protected]>
TestUpExitCodeFromContainerKilled hung for its full 2-minute budget on
CI, and its event transcript shows why: 'Aborting on container exit'
sweeps the application while the start phase — which runs on a
deliberately uncancelable context for SIGTERM management — was still
starting services; test-1 came up AFTER its stop and stayed up, so the
monitor never drained and up never returned.
The abort listener now watches the events stream past its trigger: any
container started after the sweep is a late starter from that race, and
gets stopped as it appears. Event-driven, so there is no listing window
to miss; idempotent stops make duplicates harmless. The exit code
semantics are preserved: the late starter's own 143 flows through
captureExitCodeFrom exactly as when the sweep wins the race.
TestUpExitCodeFrom* pass 5/5 locally.
Signed-off-by: Nicolas De Loof <[email protected]>
TestWatch/debian flakes on loaded runners during 'writing to a file
until Compose watch is up and running': that bootstrap loop ran under
poll.WaitOn's default 10s budget, which must absorb image pull/build,
container start and watcher initialization. Only the bootstrap gets the
2-minute budget — every later step keeps the sharp default so a real
sync regression still fails fast.
Signed-off-by: Nicolas De Loof <[email protected]>
TestAttachRestart kept failing residually after the re-attach anchoring
fix, and its new daemon-view dump told exactly why: the daemon holds
all three 'world' lines while compose printed two. The monitor returns
on the final die event — an events-channel fact — and up canceled the
global context on the spot, killing the re-attach log streams with the
last run's line still in flight: the exit notice outran the output that
preceded it.
Re-attach streams are now counted in a WaitGroup, and the monitor
wrapper waits for them to reach their natural EOF — guaranteed once the
containers exited — before canceling, under a bound that only protects
against a wedged daemon and is skipped entirely when the context is
already down (Ctrl-C). AttachRestart passes 5/5 locally with this
drain; it failed within 2 CI attempts without it.
Signed-off-by: Nicolas De Loof <[email protected]>
Wait listed running containers only, so a service that finished between
up and the listing — a fast run, or a service long done by the time the
user types the command — failed with 'no containers for project' in a
few milliseconds instead of returning its recorded exit code. This is
also the root cause of the TestWaitAndDrop flake: its 'faster' service
sleeps 2 seconds, less than the harness latency between the two steps.
The condition wait observes (container no longer running) already holds
for such a target: fall back to a full listing only when no container
is running, and let ContainerWait return the recorded status
immediately. Scoping the fallback to the previously-erroring path keeps
every other semantics intact — in particular a stale exited one-off can
never short-circuit a wait that has live containers to observe.
Exit-code propagation verified end to end: wait on an already-failed
service returns its code (7), not an error.
Signed-off-by: Nicolas De Loof <[email protected]>
Review finding on runEndTracker.Observe: recording nothing for an exit
event without a timestamp is the deliberate choice — the anchor is
evaluated by the daemon against its own log clock, so substituting the
local clock would introduce real skew mis-anchoring to paper over a
hypothetical daemon quirk, while dropping only degrades that container
to the pre-tracker fallback.
Signed-off-by: Nicolas De Loof <[email protected]>
The FinishedAt anchor did not cure the third-run loss on CI (still 2
worlds for 3 exit notices after a full minute, oldstable runner). The
remaining suspects are on both sides of the API: a line the daemon
never captured (copier torn down before a millisecond-lived run's
output) or a line compose still fails to relay. On timeout the test
now dumps `docker logs` for the container — ground truth that
discriminates the two on the next CI occurrence.
Signed-off-by: Nicolas De Loof <[email protected]>
The CI hardening in the previous commit turned TestAttachRestart's
flake into a reliable detector, and what it detected is a real loss:
re-attaching with since=StartedAt drops a fast run's first lines
forever, because the daemon starts copying stdout before it records
StartedAt.
Anchoring on the inspected FinishedAt fixes the common case but leaves
a narrower race CI still caught: when the new run itself finishes
before compose reacts to its start event, the inspected FinishedAt is
already the NEW run's own end, and the log window drops everything the
run printed — two worlds for three exit notices, the third never
arriving no matter how long you wait.
Both re-attach sites (attached up, logs --follow) therefore anchor on
the session's own record of the container's previous exit
(runEndTracker): the monitor delivers events in order, so the anchor
captured synchronously at start-event time is necessarily the previous
run's end — nanosecond-precise, immune to how fast the new run dies.
The inspected FinishedAt remains the fallback for a container the
session never saw exit, and a fresh container keeps no lower bound. A
unit test pins the ordering contract, including the fast-run sequence
CI caught.
Signed-off-by: Nicolas De Loof <[email protected]>
Three tests failed 9 CI runs across 5 branches this week, all on
asynchronous-observation races, none reproducible locally:
- RequireServiceState asserted on a single `compose ps` snapshot; the
daemon reports state transitions asynchronously from everything else
a test observes (TestUpDependenciesNotStopped saw 'created' while
the container's logs were already flowing). It now polls until the
state converges (15s bound).
- TestAttachRestart counted restart log lines in a snapshot taken as
soon as the third exit notice appeared; exit notices come from the
events monitor while log lines come from the re-attached logs
stream — two channels with no ordering between them. The count is
now awaited like the exit notices already were; a genuinely lost
line still fails, by timeout.
- TestUpExitCodeFromContainerKilled ran a full up+abort cycle under a
60s ceiling, once exceeded on a loaded oldstable runner; raised to
120s.
Signed-off-by: Nicolas De Loof <[email protected]>
The plan documented in both PRs had #14215 land first, then #14093's
compose-go bump delete the transient TestServiceHashContinuity. Merged
in the opposite order, main's pkg/compose tests no longer even compile:
richServiceFixture sets fields the container-spec layering turned into
promoted embeds (struct literals require go1.27 for that), and once
compilable two tests fail by construction.
- richServiceFixture is built by assignment;
- TestServiceHashContinuity is deleted, exactly as its own comment
prescribes: the layered compose-go is the reorder it existed to
outlive, its proof duty (pinned == historical bytes) is done, and
TestHashGoldenValues carries the continuity contract alone — it
PASSES on the layered compose-go, confirming the pinning preserved
every historical hash through the layering;
- pull_refresh_after, newly exposed at the service root, is appended
to serviceHashKeyOrder: the fallback already emitted it in that
exact position, so no hash moves — listing it only freezes the
layout and satisfies the coverage walker.
Signed-off-by: Nicolas De Loof <[email protected]>
The trimming of hash-excluded fields becomes trimServiceHashFields,
shared between ServiceHash and the continuity test so the exclusion set
cannot silently drift; pinRootKeyOrder reuses the package's sortedKeys
helper; the key-order coverage walker now follows encoding/json
promotion rules for non-struct embeds and unexported fields.
Golden hashes unchanged.
Signed-off-by: Nicolas De Loof <[email protected]>
Nicolas De Loof