Repository navigation
feat: scale and watch rebuild use the unified plan - #14291
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The global wait timeout incorrectly covers all plan operations and changes established timeout semantics.
Review effort: Balanced
Findings: 1
What changed in this PR
Unifies Create and Start planning for detached up, scale, and watch rebuilds.
Changes:
- Adds shared plan preparation and Create+Start execution.
- Migrates scale and watch rebuild to the unified plan.
- Refreshes renamed-container state and adds regression tests.
| File | Description |
|---|---|
pkg/compose/create.go |
Adds plan preparation and shared Create+Start helper. |
pkg/compose/up.go |
Runs detached up through one plan. |
pkg/compose/scale.go |
Migrates scale to unified execution. |
pkg/compose/watch.go |
Narrows and unifies rebuild execution. |
pkg/compose/reconcile.go |
Associates recreate renames with their service. |
pkg/compose/executor_ops.go |
Refreshes renamed-container execution state. |
pkg/compose/executor_start_test.go |
Adds deadline and rename regressions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
dde9b6a to
e107b5a
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
e107b5a to
ffa77f3
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
ffa77f3 to
a398600
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Start nodes can run before all Create nodes succeed, allowing partially started applications after a later create failure.
Review effort: Balanced
Findings: 1
Open (3)
Resolved since last review (1)
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
a398600 to
a3c864e
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
a3c864e to
4cde09d
Compare
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
4cde09d to
f60136c
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
The stacked lifecycle and concurrency changes are extensive, and the new watch rebuild scope lacks targeted regression coverage.
2 open findings
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
Same create() + start() two-snapshot pattern upDetached already replaced for `up -d`, now shared via a new createAndStart helper for the other two callers that had it: - Scale: exact equivalent, same project object was already passed to both calls. - watch's rebuild(): the create() call used the full project (scoped only via CreateOptions.Services/Recreate policy), but start() used a project narrowed to services + their dependents (WithSelectedServices). The narrowed project is now computed once and used for both phases instead. This does shrink the create phase's own scope a little, deliberately: WithSelectedServices only trims the project's Services map (unrelated services move to DisabledServices, not vanish -- orphan detection still recognizes their containers via DisabledServiceNames(), so nothing gets misflagged) -- Networks, Volumes, Configs, Secrets are untouched, so external-resource resolution is unaffected. What does narrow: config-drift checking (RecreateDependencies policy) and the image pull/validate pre-pass no longer consider services outside this rebuild's blast radius. For a file-watch-triggered rebuild of specific services this is arguably more correct than today (a watch rebuild of one service shouldn't be re-validating or drift-checking the rest of the project on every keystroke) -- not a literal byte-for-byte port, a deliberate, narrower scope. The old code also returned a create() failure immediately but logged and swallowed a start() failure; a single combined call can't keep that distinction. Traced as inert either way: the sole caller, rebuildScheduler.run(), discards rebuild()'s return value unconditionally -- only the logged message differs, not control flow. AttachTo: services, also dropped from the old StartOptions, was already dead in this call (only read by start() when Project is nil, which it never was here). Verified with the full e2e suite (TestScale*, TestWatch* in particular) against a locally built binary, plus the existing unit suite and lint. No new failures; the only 3 remaining are the same pre-existing, environment-specific ones from the previous PR in this stack (macOS Docker Desktop restricted-host-mount errors, a model-pull/registry issue). Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
…ce outside the selection still uses it A project narrowed with WithSelectedServices (watch's rebuild, which this PR moves to a narrowed Create phase, and `up`/`scale --no-deps`) keeps every network and volume of the model but moves the services left out to DisabledServices. The reconciler looked for the consumers of a resource in project.Services only, so a drifted network or volume used by a disabled service looked unused: the plan removed it without detaching that service's container, and the removal failed with "in use". For a volume, the user was first asked to confirm a data-destroying recreation that could not succeed. A same-name drifted network or volume that a disabled service still holds through a live container is now kept as is, with a warning, and no prompt is shown. A disabled service without a live container does not block anything, and a rename is unaffected (it never removes the live resource). Also correct the comment in rebuild, which claimed narrowing left the convergence of networks and volumes unaffected. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
fd1f9dd to
3844eac
Compare
watch's rebuild narrows the project to the rebuilt services and their dependents for the create phase too. Nothing locked that contract: a future change of the selection or of the observed state could silently bring back the convergence of the whole project, or report the services left out as orphans. Two tests: the plan of a narrowed project never touches a drifted service outside the selection, nor removes it as an orphan under --remove-orphans; and the observation of a narrowed project keeps the containers of the disabled services out of the orphans, so the "Found orphan containers" warning does not fire on every rebuild. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟢 APPROVE
The unified-plan switchover for scale and watch rebuild is clean. The createAndStart helper correctly mirrors upDetached's ScopeCreateStart path. The heldOutsideSelection guard is structurally sound: networkConsumers/volumeConsumers and r.observed.Containers consistently use service.Name, so the container lookup is always correct. Silently skipping the data-loss prompt for a volume held by a disabled service is deliberate and correct (the removal would fail anyway). The rebuild error-propagation change is an improvement.
One low-confidence observation about the dropped AttachTo field is noted inline — likely benign given the scheduler context, but worth a quick review.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Note: This review covers only the commits pushed since
3844eac8d894c13baf3ddae170b32953cbd191fc. Earlier commits were already reviewed.
Assessment: 🟡 NEEDS ATTENTION
This incremental diff adds two new unit tests. One test has a real coverage gap that would let a reconciler regression slip through undetected.
Lower-confidence findings (not posted inline)
- [low]
pkg/compose/reconcile_test.go:2228 —strings.Contains(resourceID, "api")is an overly broad substring match; a resource incidentally containing "api" in its name would fire a false-positive assertion failure (not verified; unverified low-severity finding) - [low]
pkg/compose/reconcile_test.go:2207 —opts.Recreate = api.RecreateForcemakes thedriftedAPIimage setup irrelevant; the hash-diverged recreation path is not exercised (not verified; unverified low-severity finding)
The narrowed-selection test only checked the selected service itself, so a plan silently dropping its dependent would have passed. Also match the disabled service by its resource prefix rather than a substring, and drop the drifted image setup that force recreate made irrelevant. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
This review covers only the commits since af80ac3d3458.
Assessment: 🟢 APPROVE
The incremental diff is 43 lines, all in pkg/compose/reconcile_test.go, addressing the three issues flagged in previous review cycles:
- Removed misleading drifted-API setup — the stale-image config was irrelevant because
opts.Recreate = api.RecreateForceforces recreation regardless of drift; usingapiSvcdirectly is correct. - Tightened the negative assertion —
strings.HasPrefix(node.Operation.ResourceID, "service:api:")is strictly more precise than the oldstrings.Contains(..., "api")substring check, with no false negatives given the test's observed state (empty Networks/Volumes, api disabled). - Added the missing positive assertion for
worker—recreated["service:worker:1"]closes the gap where a silent reconciler regression droppingworkerfrom the plan would have passed the test undetected.
All three changes are correct and no new issues were introduced.
The rebuild now reports "Failed to recreate and restart service(s) after update" instead of "Application failed to start after update", which TestRebuildOnDotEnvWithExternalNetwork still asserted was absent: the string no longer existed anywhere, so the check could never fail and the test no longer detected a failed rebuild. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
declaredServices duplicated (*types.Project).AllServices(), already in the pinned compose-go: Services plus DisabledServices in a fresh map. Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Incremental review — this review covers only the commits pushed since
35fb07580112. Earlier commits were already reviewed.
Assessment: 🟢 APPROVE
The incremental diff is a clean refactor: replacing the local declaredServices() helper (which explicitly built project.Services ∪ project.DisabledServices) with r.project.AllServices() from the compose-go library. The maps import is correctly removed. Two inline notes are raised about the correctness of AllServices() semantics — both are assessed as unlikely in practice (the naming convention, the usages in config.go/completion.go, and the PR's stated intent all point to the correct union), but flagged since the compose-go source is not directly accessible in this review context.
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
What this PR does, in one sentence
docker compose scaleandwatch's rebuild-on-change step now run their Create and Start phases as a single plan, the same switchover #14290 did forup -d.Context
Both
Scaleandwatch's rebuild step share the same old pattern detachedupused to: acreate()call to converge the project, immediately followed by a separatestart()call that re-lists containers from scratch and runs the older imperative engine to start them — two snapshots of the daemon instead of one.What the PR brings
A new shared helper,
createAndStart, runspreparePlan(..., ScopeCreateStart)+executePlanonce — the same thingupDetacheddoes forup -d, minus--waithandling (neither caller here ever uses it).Scale's conversion is an exact equivalent: it already passed the same project to both calls.watch's rebuild needed more care: it calledcreate()on the full project butstart()on a project narrowed to the rebuilt service(s) and their dependents (WithSelectedServices), so only that subset gets started/health-checked, not the whole app. The narrowed project is now used for both phases. This does shrink the create phase's scope a little, deliberately — verified that narrowing only trims which services get reconciled (drift-checked, pulled/validated), not network/volume/external-resource handling, and not orphan detection (excluded services move toDisabledServices, still recognized as known, never misflagged). For a rebuild scoped to specific services, that narrower blast radius is arguably more correct than re-checking the whole project's drift on every file-watch trigger — not a literal byte-for-byte port of today's behavior, a conscious, narrower one.Narrowing the create phase has one consequence the reconciler now accounts for: a project narrowed with
WithSelectedServiceskeeps every network and volume of the model but moves the services left out toDisabledServices, so a drifted network or volume still used by such a service's live container looked unused, and its removal failed with "in use" (for a volume, after asking the user to confirm a data-destroying recreation that could not succeed). Such a resource is now kept as is, with a warning and no prompt.The narrowed scope is pinned at the two layers it lives in: the plan of a narrowed project never touches a drifted service outside the selection (nor removes it as an orphan), and the observation keeps the disabled services' containers out of the orphans, so the orphan warning does not fire on every rebuild.
Verified against the full e2e suite (
TestScale*,TestWatch*) on a locally built binary, not just unit tests.Follows #14290 (merged). Part of #14081.
🤖 Generated with Claude Code