compose: retained named volumes and strict reconciliation - #59
Merged
Conversation
Add top-level named volumes to hypeman compose, backed by the existing Hypeman volume APIs, and make reconciliation strict and non-destructive by default: - Compose files declare top-level volumes with size_gb (optional explicit name) and attach them to services via shorthand (volume:/abs/path[:ro|rw]) or mapping mount declarations. - Volumes are created before instances and tagged with compose ownership. Planned mounts carry the volume name so rendered hashes stay stable; names resolve to server IDs at apply time. - Volumes are retained across instance replacement and compose down. Deleting retained data requires the explicit destructive option compose down --volumes, and volume spec changes conflict rather than silently replacing data. - compose up/plan prune owned instances and ingresses that are no longer declared, without touching unmanaged resources. - Compose parsing is strict: unknown fields and duplicate keys fail validation, including inside volume mount mappings. Invalid or ambiguous mount declarations (unknown volume, relative path, duplicate mount path, duplicate volume attachment) fail validation. Existing stateless compose files are unaffected: no volumes means no volume actions and identical plan/up/down behavior. Tests cover shorthand parsing, strict parsing, validation, deterministic rendering, volume plan semantics, and nonce persistence through replacement, failed replacement, and down/up against an in-memory Hypeman API seam.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Prune deletes after creates
- Pruned delete actions are now explicitly prioritized during
Upso owned resources removed from the compose file are deleted before create/replace actions run.
- Pruned delete actions are now explicitly prioritized during
- ✅ Fixed: Conflict skips prune claims
- Ambiguous ingress-rename conflicts now record all candidate ingress IDs as claimed, preventing
pruneActionsfrom emitting contradictory deletes for those same owned ingresses.
- Ambiguous ingress-rename conflicts now record all candidate ingress IDs as claimed, preventing
Or push these changes by commenting:
@cursor push 91d3109abb
Preview (91d3109abb)
diff --git a/lib/compose/compose.go b/lib/compose/compose.go
--- a/lib/compose/compose.go
+++ b/lib/compose/compose.go
@@ -64,12 +64,19 @@ type Action struct {
instanceID string
ingressID string
volumeID string
+ prune bool
+ claimedIDs claimedResourceIDs
instanceInput hypeman.InstanceNewParams
ingressInput hypeman.IngressNewParams
volumeInput hypeman.VolumeNewParams
buildInput *desiredBuild
}
+type claimedResourceIDs struct {
+ instances []string
+ ingresses []string
+}
+
func NewRunner(file string, client hypeman.Client, opts ...option.RequestOption) (*Runner, error) {
spec, err := loadComposeSpec(file)
if err != nil {
@@ -64,12 +64,19 @@ type Action struct {
instanceID string
ingressID string
volumeID string
+ prune bool
+ claimedIDs claimedResourceIDs
instanceInput hypeman.InstanceNewParams
ingressInput hypeman.IngressNewParams
volumeInput hypeman.VolumeNewParams
buildInput *desiredBuild
}
+type claimedResourceIDs struct {
+ instances []string
+ ingresses []string
+}
+
func NewRunner(file string, client hypeman.Client, opts ...option.RequestOption) (*Runner, error) {
spec, err := loadComposeSpec(file)
if err != nil {
diff --git a/lib/compose/compose_test.go b/lib/compose/compose_test.go
--- a/lib/compose/compose_test.go
+++ b/lib/compose/compose_test.go
@@ -476,3 +476,46 @@ func TestPlanIngressActionConflictsWhenRenameCandidateIsAmbiguous(t *testing.T)
assert.Equal(t, "multiple owned ingresses for service have changed names", action.Reason)
assert.Empty(t, action.ingressID)
}
+
+func TestPruneActionsSkipsAmbiguousIngressRenameConflictCandidates(t *testing.T) {
+ desired := desiredIngress{
+ Name: "app-api-public",
+ Service: "api",
+ Hash: "public-hash",
+ Input: hypeman.IngressNewParams{
+ Name: "app-api-public",
+ },
+ }
+ owned := []hypeman.Ingress{
+ {
+ ID: "old-http-id",
+ Name: "app-api-http",
+ Tags: composeTags("app", "api", composeResourceIngress, "http-hash"),
+ },
+ {
+ ID: "old-grpc-id",
+ Name: "app-api-grpc",
+ Tags: composeTags("app", "api", composeResourceIngress, "grpc-hash"),
+ },
+ }
+
+ action := planIngressAction(desired, owned, nil, map[string]struct{}{
+ "app-api-public": {},
+ })
+ require.Equal(t, "conflict", action.Action)
+
+ pruned := pruneActions(nil, owned, []Action{action})
+ assert.Empty(t, pruned)
+}
+
+func TestUpActionOrderRunsPruneDeletesBeforeCreatesAndReplaces(t *testing.T) {
+ actions := []Action{
+ {Action: "create", Type: "ingress", Name: "app-api-public"},
+ {Action: "replace", Type: "instance", Name: "app-api"},
+ {Action: "delete", Type: "instance", Name: "app-cache", prune: true},
+ {Action: "delete", Type: "ingress", Name: "app-api-http", prune: true},
+ {Action: "delete", Type: "volume", Name: "app-data"},
+ }
+
+ assert.Equal(t, []int{2, 3, 0, 1, 4}, upActionOrder(actions))
+}
@@ -476,3 +476,46 @@ func TestPlanIngressActionConflictsWhenRenameCandidateIsAmbiguous(t *testing.T)
assert.Equal(t, "multiple owned ingresses for service have changed names", action.Reason)
assert.Empty(t, action.ingressID)
}
+
+func TestPruneActionsSkipsAmbiguousIngressRenameConflictCandidates(t *testing.T) {
+ desired := desiredIngress{
+ Name: "app-api-public",
+ Service: "api",
+ Hash: "public-hash",
+ Input: hypeman.IngressNewParams{
+ Name: "app-api-public",
+ },
+ }
+ owned := []hypeman.Ingress{
+ {
+ ID: "old-http-id",
+ Name: "app-api-http",
+ Tags: composeTags("app", "api", composeResourceIngress, "http-hash"),
+ },
+ {
+ ID: "old-grpc-id",
+ Name: "app-api-grpc",
+ Tags: composeTags("app", "api", composeResourceIngress, "grpc-hash"),
+ },
+ }
+
+ action := planIngressAction(desired, owned, nil, map[string]struct{}{
+ "app-api-public": {},
+ })
+ require.Equal(t, "conflict", action.Action)
+
+ pruned := pruneActions(nil, owned, []Action{action})
+ assert.Empty(t, pruned)
+}
+
+func TestUpActionOrderRunsPruneDeletesBeforeCreatesAndReplaces(t *testing.T) {
+ actions := []Action{
+ {Action: "create", Type: "ingress", Name: "app-api-public"},
+ {Action: "replace", Type: "instance", Name: "app-api"},
+ {Action: "delete", Type: "instance", Name: "app-cache", prune: true},
+ {Action: "delete", Type: "ingress", Name: "app-api-http", prune: true},
+ {Action: "delete", Type: "volume", Name: "app-data"},
+ }
+
+ assert.Equal(t, []int{2, 3, 0, 1, 4}, upActionOrder(actions))
+}
diff --git a/lib/compose/reconcile.go b/lib/compose/reconcile.go
--- a/lib/compose/reconcile.go
+++ b/lib/compose/reconcile.go
@@ -116,7 +116,7 @@ func (r *Runner) Up(ctx context.Context, opts UpOptions) (Plan, error) {
return result, fmt.Errorf("replace required:\n%s\n\nRun again with --replace to recreate changed resources.", strings.Join(blockers, "\n"))
}
- for i := range result.Actions {
+ for _, i := range upActionOrder(result.Actions) {
action := &result.Actions[i]
switch action.Action {
case "create":
@@ -116,7 +116,7 @@ func (r *Runner) Up(ctx context.Context, opts UpOptions) (Plan, error) {
return result, fmt.Errorf("replace required:\n%s\n\nRun again with --replace to recreate changed resources.", strings.Join(blockers, "\n"))
}
- for i := range result.Actions {
+ for _, i := range upActionOrder(result.Actions) {
action := &result.Actions[i]
switch action.Action {
case "create":
@@ -565,9 +565,15 @@ func pruneActions(ownedInstances []hypeman.Instance, ownedIngresses []hypeman.In
if action.instanceID != "" {
claimedInstances[action.instanceID] = struct{}{}
}
+ for _, id := range action.claimedIDs.instances {
+ claimedInstances[id] = struct{}{}
+ }
if action.ingressID != "" {
claimedIngresses[action.ingressID] = struct{}{}
}
+ for _, id := range action.claimedIDs.ingresses {
+ claimedIngresses[id] = struct{}{}
+ }
}
var pruned []Action
for _, inst := range ownedInstances {
@@ -565,9 +565,15 @@ func pruneActions(ownedInstances []hypeman.Instance, ownedIngresses []hypeman.In
if action.instanceID != "" {
claimedInstances[action.instanceID] = struct{}{}
}
+ for _, id := range action.claimedIDs.instances {
+ claimedInstances[id] = struct{}{}
+ }
if action.ingressID != "" {
claimedIngresses[action.ingressID] = struct{}{}
}
+ for _, id := range action.claimedIDs.ingresses {
+ claimedIngresses[id] = struct{}{}
+ }
}
var pruned []Action
for _, inst := range ownedInstances {
@@ -580,6 +586,7 @@ func pruneActions(ownedInstances []hypeman.Instance, ownedIngresses []hypeman.In
Name: inst.Name,
Service: inst.Tags[composeTagService],
Reason: "no longer declared in compose file",
+ prune: true,
instanceID: inst.ID,
})
}
@@ -580,6 +586,7 @@ func pruneActions(ownedInstances []hypeman.Instance, ownedIngresses []hypeman.In
Name: inst.Name,
Service: inst.Tags[composeTagService],
Reason: "no longer declared in compose file",
+ prune: true,
instanceID: inst.ID,
})
}
@@ -593,6 +600,7 @@ func pruneActions(ownedInstances []hypeman.Instance, ownedIngresses []hypeman.In
Name: ing.Name,
Service: ing.Tags[composeTagService],
Reason: "no longer declared in compose file",
+ prune: true,
ingressID: ing.ID,
})
}
@@ -593,6 +600,7 @@ func pruneActions(ownedInstances []hypeman.Instance, ownedIngresses []hypeman.In
Name: ing.Name,
Service: ing.Tags[composeTagService],
Reason: "no longer declared in compose file",
+ prune: true,
ingressID: ing.ID,
})
}
@@ -645,6 +653,7 @@ func planIngressAction(desired desiredIngress, owned []hypeman.Ingress, all []hy
if len(renameCandidates) > 1 {
action.Action = "conflict"
action.Reason = "multiple owned ingresses for service have changed names"
+ action.claimedIDs.ingresses = ingressIDs(renameCandidates)
return action
}
for _, ing := range all {
@@ -645,6 +653,7 @@ func planIngressAction(desired desiredIngress, owned []hypeman.Ingress, all []hy
if len(renameCandidates) > 1 {
action.Action = "conflict"
action.Reason = "multiple owned ingresses for service have changed names"
+ action.claimedIDs.ingresses = ingressIDs(renameCandidates)
return action
}
for _, ing := range all {
@@ -671,6 +680,14 @@ func desiredIngressNamesByService(ingresses []desiredIngress) map[string]map[str
return names
}
+func ingressIDs(ingresses []hypeman.Ingress) []string {
+ ids := make([]string, 0, len(ingresses))
+ for _, ingress := range ingresses {
+ ids = append(ids, ingress.ID)
+ }
+ return ids
+}
+
func (r *Runner) listComposeInstances(ctx context.Context) ([]hypeman.Instance, error) {
instances, err := r.client.Instances.List(ctx, hypeman.InstanceListParams{
Tags: map[string]string{composeTagName: r.spec.Name},
@@ -671,6 +680,14 @@ func desiredIngressNamesByService(ingresses []desiredIngress) map[string]map[str
return names
}
+func ingressIDs(ingresses []hypeman.Ingress) []string {
+ ids := make([]string, 0, len(ingresses))
+ for _, ingress := range ingresses {
+ ids = append(ids, ingress.ID)
+ }
+ return ids
+}
+
func (r *Runner) listComposeInstances(ctx context.Context) ([]hypeman.Instance, error) {
instances, err := r.client.Instances.List(ctx, hypeman.InstanceListParams{
Tags: map[string]string{composeTagName: r.spec.Name},
@@ -751,6 +768,22 @@ func conflictBlockers(actions []Action) []string {
return blockers
}
+func upActionOrder(actions []Action) []int {
+ order := make([]int, 0, len(actions))
+ for i := range actions {
+ if actions[i].Action == "delete" && actions[i].prune {
+ order = append(order, i)
+ }
+ }
+ for i := range actions {
+ if actions[i].Action == "delete" && actions[i].prune {
+ continue
+ }
+ order = append(order, i)
+ }
+ return order
+}
+
func summarizeComposeActions(actions []Action) Summary {
var summary Summary
for _, action := range actions {
@@ -751,6 +768,22 @@ func conflictBlockers(actions []Action) []string {
return blockers
}
+func upActionOrder(actions []Action) []int {
+ order := make([]int, 0, len(actions))
+ for i := range actions {
+ if actions[i].Action == "delete" && actions[i].prune {
+ order = append(order, i)
+ }
+ }
+ for i := range actions {
+ if actions[i].Action == "delete" && actions[i].prune {
+ continue
+ }
+ order = append(order, i)
+ }
+ return order
+}
+
func summarizeComposeActions(actions []Action) Summary {
var summary Summary
for _, action := range actions {You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 820fcc5. Configure here.
rgarcia
commented
Aug 3, 2026
rgarcia
left a comment
rgarcia
left a comment
Contributor
Author
There was a problem hiding this comment.
Independent review — round 1
Strong implementation overall: retained-volume semantics (create-before-instance, name→ID resolution at apply time, immutability-as-conflict, retention across down/--replace), strict YAML parsing, and the fake-API test suite covering nonce persistence, replace-failure recovery, destructive down --volumes, and prune scoping all check out. CI is green and I reproduced go test ./lib/compose/... locally. Docs match behavior.
Two findings before I can sign off — both overlap with Cursor Bugbot's findings on this head, which I verified independently:
Important
- Prune deletes run after creates, which can wedge
upon key moves (lib/compose/reconcile.go,Plan/Up). Prune actions are appended last, so when a uniquely-constrained key (most plausibly an ingress hostname) moves from a removed service to a new/renamed one, the create for the new ingress executes while the pruned owned ingress still holds the hostname. If the server enforces uniqueness,uperrors, and re-running hits the same deterministic order — the user must manually delete orcompose downto recover. Consider applying prune deletes before creates/replaces (or at least before ingress creates), mirroring howapplyReplacealready deletes before creating.
Minor
- Conflict paths without resource IDs cause contradictory plan output.
planIngressAction's multiple-rename-candidatesconflictdoesn't setingressID, sopruneActionsalso emitsdelete: no longer declared in compose filefor the same owned ingresses. Conflicts blockUp, so nothing is actually deleted, butcompose planshows both a conflict and deletes for resources in an ambiguous state. Suppressing prunes when the plan contains conflicts (or claiming rename candidates) would make plan output coherent. - (nit)
resolveInstanceVolumeIDsre-lists compose volumes per instance create; a single lookup shared across the apply pass would avoid N+1 list calls. Not blocking. - (nit)
planVolumeAction's "name exists without compose ownership" reason also fires for volumes owned by a different compose project (any non-matchinghypeman.compose.name); conflicting is correct, the reason string is slightly misleading.
Verdict for this round: changes requested on finding 1 (and Bugbot's unresolved findings). Everything else is in good shape — once ordering is addressed this should be a quick re-approve.
Review round 1 fixes: - Plan prune deletes before instance/ingress creates/replaces so a pruned owned resource frees its unique keys (names, ingress hostnames) before a new resource reuses them; previously a hostname moving from a removed service to a new one could deterministically wedge up until manual intervention. Mirrors applyReplace's delete-then-create. - The ambiguous ingress rename conflict now claims its rename candidates, so plan no longer shows both a conflict and prune deletes for the same owned ingresses. - resolveInstanceVolumeIDs shares one compose-volume list across the apply pass instead of re-listing per instance create. - planVolumeAction reports when a conflicting volume is owned by a different compose project instead of claiming missing ownership. Regression tests: hostname move across services no longer wedges up (fake enforces hostname uniqueness), conflicted rename candidates are not planned for deletion, and the volume lookup is listed once per apply pass.
Contributor
Author
|
Round 1 findings addressed in ac4827a:
|
rgarcia
commented
Aug 3, 2026
rgarcia
left a comment
rgarcia
left a comment
Contributor
Author
There was a problem hiding this comment.
Independent review — round 2
Reviewed head ac4827a. All round-1 findings are addressed and I verified each independently:
- Prune ordering (important, round 1 #1) — fixed.
Plannow emits prune deletes before instance/ingress creates/replaces, andUpapplies actions in plan order, so plan output matches execution order (cleaner than reordering only at apply time).TestComposeUpPrunesBeforeCreatesSoMovedHostnameDoesNotWedgeproves the moved-hostname case against a fake that enforces hostname uniqueness, including delete-before-create request ordering, plan ordering, and idempotent re-run. - Conflict prune claims (round 1 #2) — fixed via
claimedIngressIDson ambiguous-rename conflicts;pruneActionshonors them. Covered by unit + end-to-end tests, which also confirmUprefuses with conflicts and touches nothing. - N+1 volume list (nit) — fixed with a per-apply-pass
volumeIDsByNamecache, reset at the start of eachUp; volumes are still created before instance resolves. Test asserts request counts. - Misleading conflict reason (nit) — a volume owned by a different compose project now reports
name is owned by a different compose project "<name>".
Verified locally on ac4827a: go build ./..., go vet, and full go test ./... pass. CI (lint, semgrep) is green on the head, Cursor Bugbot's check on this head completed with no new findings, and both of its round-1 threads are resolved.
Review satisfied — no blocking or important findings. (Formal approval omitted since this is my own PR per GitHub self-approval rules; the workflow verdict is authoritative.)
sjmiller609
approved these changes
Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds retained named volumes to
hypeman compose, backed by the existing Hypeman volume APIs, and makes reconciliation strict, predictable, and non-destructive by default.Retained volumes
volumes:declarations (size_gb, optional explicitname) and per-service mounts, in shorthand (data:/var/lib/data[:ro|rw]) or mapping form.hypeman.compose.name/resource/hash; no service tag since volumes can be shared).--replace) andcompose down—downreports them asskip: retained.hypeman compose down --volumes, with clear plan output (delete ... --volumes destroys retained data).size_gb) plans a conflict that blocksup, instead of silently replacing the volume and losing data.up --replacerecreates the instance on the same volume.Strict reconciliation
compose up/plannow prune owned instances/ingresses that are no longer declared in the file (delete: no longer declared in compose file). Resources without compose ownership tags are never touched. Undeclared owned volumes are reportedskip: retained, never auto-deleted.Backward compatibility
Existing stateless compose files are unaffected: with no
volumes:declared there are no volume actions and plan/up/down behave as before (the only intentional behavioral change is thatupprunes owned resources removed from the file, which is the strict-reconciliation feature itself).Runner.Downgains aDownOptionsstruct in place of the bareverbose bool.Acceptance criteria mapping
compose downby defaultdown --volumes) with clear plan outputcompose upplans pruning for removed owned instances/ingresses; unmanaged resources untouchedhttptestfake implementing the volumes/instances/ingresses/images endpoints compose uses)Tests
go test ./...— full suite green. New coverage inlib/compose/volumes_test.go:down --volumesdestroys data and down is idempotent; spec change conflicts without touching data; pruning removes only owned resourcesRisks
compose upnow deletes owned instances/ingresses removed from the file (intended strict behavior; called out above). Retained volumes are never auto-pruned.${env:...}/${file:...}references (the:in the interpolation syntax is ambiguous with the mount separator); the mapping form supports interpolation. Mount parsing errors point at the line.Review request
Please review with Cursor Bugbot (cursor bugbot review requested on this PR), especially around the plan/apply ordering, the name→ID volume resolution at apply time, and the prune claim-tracking in
pruneActions.Note
Medium Risk
Changes data lifecycle (retained volumes and explicit
--volumesdeletion) and makescompose updelete owned resources dropped from the file; mistakes in compose files or pruning order could cause unexpected deletes or hostname conflicts.Overview
Adds retained named volumes to
hypeman compose: top-levelvolumes(size_gb, optionalname), service mounts (shorthandvol:/path[:ro|rw]or mapping), create-before-instance apply order, compose ownership tags (no per-service tag on volumes), and name→ID resolution at instance create so hashes stay stable. Volumes survivedown,--replace, and pruning;compose down --volumesis the only supported way to destroy their data. Declared volume changes after create surface as conflicts (not replace).Tightens reconciliation: strict YAML (
KnownFields, duplicate keys, mount validation),up/planprune compose-owned instances and ingresses removed from the file (unmanaged resources untouched), with prune deletes ordered before creates so hostnames/names can be reused. Ambiguous ingress rename conflicts claim candidate IDs so prune does not delete them.Runner.DowntakesDownOptions(includingVolumes); CLI wires--volumes.Docs and a large
volumes_test.gosuite cover persistence, failed replace recovery, and apply ordering.Reviewed by Cursor Bugbot for commit ac4827a. Bugbot is set up for automated code reviews on this repo. Configure here.