fix(machine): the hot attach takes turns too, and a transient port-group conflict stops disarming the firewall capability

internal/core/machine/incus.go

@@ -741,13 +741,46 @@ func (d *Incus) Attach(ctx context.Context, name string, att Attachment) error {
 		if att.Address != "" {
 			args = append(args, "ipv4.address="+att.Address)
 		}
-		if _, err := d.run(ctx, args...); err != nil {
+		// The hot half of a membership: the machine is running, so this add
+		// plugs an OVN port and the daemon resolves the network's ACL
+		// references inside it — the same window #577 closed on the two start
+		// doors and on attachExtra, on this call site the packs' Join drives.
+		// An isolation detach landing inside (a peering acceptance empties the
+		// foreign list mid-apply, IsolateNetwork unsets and deletes the rule
+		// set) kills the add with `Cannot find security ACL ID for "iso-…"`.
+		// Measured raw on the station (2026-08-28, Incus 7.2, OVN): a detach
+		// fired 25–200 ms into a ~400 ms device add killed 11 adds of 14 with
+		// exactly that error. Same lock as the detach, so the two take turns.
+		// The attach-per-machine lock above orders nothing here: the detach
+		// never takes it. Lock order is attach-lock then network-lock,
+		// nowhere the reverse. TestAHotAttachAndAnIsolationDetachTakeTurns
+		// fails without this.
+		//
+		// `unlock`, not `release`: attachExtra above holds this lock around
+		// the same command, and the falsify harness rewrites a fragment that
+		// must match exactly once.
+		unlock := d.networkLock(att.Network)
+		_, err := d.run(ctx, args...)
+		unlock()
+		if err != nil {
 			return fmt.Errorf("attach %s to network %s: %w", name, att.Network, err)
 		}
 	case att.Address != "" && devices.own[device]["ipv4.address"] != att.Address:
 		// Re-attached at a different address: the reservation must follow, or
 		// the bridge keeps handing the machine the address of a previous life.
-		if _, err := d.run(ctx, "config", "device", "set", name, device, "ipv4.address="+att.Address); err != nil {
+		//
+		// Under the network's lock too, and that is measured, not symmetry:
+		// ipv4.address is not among the keys an OVN NIC updates in place, so
+		// the daemon removes and re-adds the device — a ~10 s operation on
+		// this station that re-plugs the OVN port and resolves the network's
+		// ACL references exactly as the add above does. A detach fired 50 ms
+		// or later into it killed 6 moves of 8 with the same `Cannot find
+		// security ACL ID`. TestAnAddressMoveAndAnIsolationDetachTakeTurns
+		// fails without this.
+		unlock := d.networkLock(att.Network)
+		_, err := d.run(ctx, "config", "device", "set", name, device, "ipv4.address="+att.Address)
+		unlock()
+		if err != nil {
 			return fmt.Errorf("move %s to %s on network %s: %w", name, att.Address, att.Network, err)
 		}
 	}

internal/core/machine/incus_apply_conflict_test.go

@@ -129,3 +129,134 @@ func TestANonTransientFailureIsNotRetried(t *testing.T) {
 type textError struct{ s string }
 
 func (e *textError) Error() string { return e.s }
+
+// portGroupCollision is the wording measured in the wild on 2026-08-28
+// (serve-after.log of the full outscale chain): EnsureFirewall's PUT losing
+// the OVSDB port-group creation to a concurrent NIC edit — the same conflict
+// TestApplyFirewallRidesOutADuplicatePortGroupRace records for the device
+// side, on the neighbouring door.
+const portGroupCollision = `incus query: Error: Failed ensuring ACL is configured in OVN: ` +
+	`Failed creating port group "incus_acl4448_net4613" for security ACL "osc-x" and network "fnt-a" setup: ` +
+	`constraint violation: Transaction causes multiple rows in "Port_Group" table ` +
+	`to have identical values (incus_acl4448_net4613) for index on column "name".`
+
+// The write-side sibling of the device edit's retry: replacing a rule set
+// that is in use makes the daemon re-ensure it in OVN, and that ensure loses
+// the same port-group collision. Observed twice in one run, nine seconds
+// apart, both inserts colliding with a row that predated them both — so the
+// daemon's own existence check can stay stale for seconds under load, no
+// ordering of this process's calls could have prevented the second one, and
+// asking again is the remedy, exactly as for the device edit.
+func TestEnsureFirewallRidesOutADuplicatePortGroupRace(t *testing.T) {
+	var mu sync.Mutex
+	puts := 0
+	runner := func(_ context.Context, args ...string) ([]byte, error) {
+		key := strings.Join(args, " ")
+		switch {
+		case key == "network acl show osc-x":
+			return []byte("name: osc-x\n"), nil
+		case strings.HasPrefix(key, "query -X PUT "):
+			mu.Lock()
+			defer mu.Unlock()
+			puts++
+			if puts == 1 {
+				return nil, &textError{portGroupCollision}
+			}
+			return nil, nil
+		}
+		return nil, nil
+	}
+
+	d := NewIncusOVN()
+	d.runner = runner
+	d.busyPoll = time.Millisecond
+
+	if err := d.EnsureFirewall(context.Background(), FirewallSpec{
+		Name:  "osc-x",
+		Rules: []FirewallRule{{Direction: "ingress", Action: "allow", Protocol: "tcp", PortFrom: 22, PortTo: 22}},
+	}); err != nil {
+		t.Fatalf("the write gave up on a transient conflict: %v", err)
+	}
+	mu.Lock()
+	defer mu.Unlock()
+	if puts != 2 {
+		t.Fatalf("expected the PUT to be asked again through the conflict, got %d attempt(s)", puts)
+	}
+	if !d.Capabilities().Firewall {
+		t.Fatal("a conflict the retry rode out still withdrew the firewall capability")
+	}
+}
+
+// Two EnsureFirewall of one absent name — the two machines of a group applied
+// concurrently both ensuring the permissive set, or two rule writes of one
+// group — both pass the existence check, and the daemon answers the loser
+// "The network ACL already exists" (measured on this station, 2026-08-28).
+// The object the loser wanted is there: the rules must still be written, and
+// the capability must stand — before this, the loser's error flowed through
+// firewallRefused and one lost create disarmed capabilities.firewall for the
+// rest of the process.
+func TestALostCreateRaceIsNotARefusal(t *testing.T) {
+	var mu sync.Mutex
+	var put string
+	runner := func(_ context.Context, args ...string) ([]byte, error) {
+		key := strings.Join(args, " ")
+		switch {
+		case key == "network acl show opn-fnt":
+			return nil, &textError{"incus network: Error: Network ACL not found"}
+		case key == "network acl create opn-fnt":
+			return nil, &textError{"incus network: Error: The network ACL already exists"}
+		case strings.HasPrefix(key, "query -X PUT "):
+			mu.Lock()
+			put = key
+			mu.Unlock()
+			return nil, nil
+		}
+		return nil, nil
+	}
+
+	d := NewIncusOVN()
+	d.runner = runner
+	d.busyPoll = time.Millisecond
+
+	if err := d.EnsureFirewall(context.Background(), FirewallSpec{Name: "opn-fnt"}); err != nil {
+		t.Fatalf("a create the winner had already done was reported as a failure: %v", err)
+	}
+	mu.Lock()
+	defer mu.Unlock()
+	if !strings.Contains(put, "/1.0/network-acls/opn-fnt") {
+		t.Fatalf("the rules were never written after the lost create: %q", put)
+	}
+	if !d.Capabilities().Firewall {
+		t.Fatal("a lost create race withdrew the firewall capability")
+	}
+}
+
+// A conflict that outlives the retry budget is an error — returned, logged by
+// the caller — but it is not the host refusing the rule set, and it must not
+// disarm the capability: one timing blip used to flip capabilities.firewall
+// for the rest of the process, and every suite keyed on the capability then
+// skipped its enforcement assertions — the instrument muted by the very noise
+// it existed to see through. A real refusal still withdraws it
+// (TestAFirewallWriteTheHostRefusesWithdrawsTheCapability).
+func TestASpentConflictBudgetDoesNotWithdrawTheCapability(t *testing.T) {
+	runner := func(_ context.Context, args ...string) ([]byte, error) {
+		key := strings.Join(args, " ")
+		if strings.HasPrefix(key, "query -X PUT ") {
+			return nil, &textError{portGroupCollision}
+		}
+		return nil, nil
+	}
+
+	d := NewIncusOVN()
+	d.runner = runner
+	d.busyPoll = time.Millisecond
+	d.busyBudget = 5 * time.Millisecond
+
+	err := d.EnsureFirewall(context.Background(), FirewallSpec{Name: "osc-x"})
+	if err == nil {
+		t.Fatal("a write that never landed was reported written")
+	}
+	if !d.Capabilities().Firewall {
+		t.Fatal("a transient conflict outliving the budget withdrew the firewall capability")
+	}
+}

internal/core/machine/incus_attach_queue_test.go

@@ -24,6 +24,14 @@ import (
 // agent takes a beat to answer. Serialised, the pair costs two beats; per
 // machine, it costs one. The assertion is on the total, because that is the
 // property a stack feels: a slow machine must not tax its neighbours.
+//
+// Two different *networks* too, and that is a decision, not a convenience.
+// Since the hot attach takes its network's lock around the device add (the
+// turn-taking with the isolation detach, measured in
+// incus_start_race_test.go), two adds on one network do take turns — the same
+// bounded, plug-length wait #577 accepted for two starts on one network. What
+// this test refuses is the defect it was measured against: a lock one
+// machine's wait makes every other machine pay, whatever network it is on.
 func TestTwoMachinesAttachWithoutQueueing(t *testing.T) {
 	const beat = 300 * time.Millisecond
 
@@ -53,14 +61,15 @@ func TestTwoMachinesAttachWithoutQueueing(t *testing.T) {
 
 	var wg sync.WaitGroup
 	start := time.Now()
-	for _, name := range []string{"feint-scw-one", "feint-scw-two"} {
+	for i, name := range []string{"feint-scw-one", "feint-scw-two"} {
+		network := []string{"fnt-net-a", "fnt-net-b"}[i]
 		wg.Add(1)
-		go func(machine string) {
+		go func(machine, network string) {
 			defer wg.Done()
 			// The error is not the subject: a stub runtime cannot complete an
 			// attachment. What is asserted is how long the pair took.
-			_ = d.Attach(context.Background(), machine, Attachment{Network: "fnt-net"})
-		}(name)
+			_ = d.Attach(context.Background(), machine, Attachment{Network: network})
+		}(name, network)
 	}
 	wg.Wait()
 	elapsed := time.Since(start)

internal/core/machine/incus_firewall.go

@@ -100,10 +100,19 @@ func (d *Incus) EnsureFirewall(ctx context.Context, spec FirewallSpec) error {
 
 	if _, err := d.run(ctx, "network", "acl", "show", spec.Name); err != nil {
 		if !isNotFound(err) {
-			return d.firewallRefused(fmt.Errorf("inspect firewall %s: %w", spec.Name, err))
+			return d.firewallWriteFailed(fmt.Errorf("inspect firewall %s: %w", spec.Name, err))
 		}
-		if _, err := d.run(ctx, "network", "acl", "create", spec.Name); err != nil {
-			return d.firewallRefused(fmt.Errorf("create firewall %s: %w", spec.Name, err))
+		// A lost create race is a success: two EnsureFirewall of one name —
+		// the two machines of a group applied concurrently both ensuring the
+		// permissive set, or two rule writes of one group — both pass the
+		// show above, and the daemon answers the loser "The network ACL
+		// already exists" (measured on this station, 2026-08-28). The object
+		// the loser wanted is there; refusing would fail the caller's rules
+		// and, worse, withdraw capabilities.firewall over a race the winner
+		// already resolved. TestALostCreateRaceIsNotARefusal fails without
+		// the tolerance.
+		if _, err := d.runUntilFree(ctx, "network", "acl", "create", spec.Name); err != nil && !isAlreadyExists(err) {
+			return d.firewallWriteFailed(fmt.Errorf("create firewall %s: %w", spec.Name, err))
 		}
 	}
 
@@ -140,13 +149,56 @@ func (d *Incus) EnsureFirewall(ctx context.Context, spec FirewallSpec) error {
 	if err != nil {
 		return fmt.Errorf("encode firewall %s: %w", spec.Name, err)
 	}
-	if _, err := d.run(ctx, "query", "-X", "PUT", "--data", string(encoded),
+	// runUntilFree, not run: replacing a rule set that is in use makes the
+	// daemon re-ensure it in OVN, and that ensure loses the same OVSDB
+	// port-group collision ApplyFirewall's device edits ride out. Measured in
+	// the wild on 2026-08-28 (serve-after.log, functional outscale chain):
+	// `write firewall osc-…: Failed creating port group "incus_acl4448_net4613"
+	// … constraint violation: Transaction causes multiple rows in "Port_Group"
+	// table`, twice in one run, nine seconds apart, both inserts colliding
+	// with the same row that predated them both — so the daemon's own
+	// existence check can read stale for seconds under load, and no ordering
+	// of this process's calls could have prevented the second failure. The
+	// remedy is asking again, which is #522's settled answer for exactly this
+	// wording; #577's refusal to retry (`Cannot find security ACL ID`, a
+	// *deleted* rule set no retry can resurrect) names a different cause and
+	// stands. TestEnsureFirewallRidesOutADuplicatePortGroupRace fails without
+	// the retry.
+	if _, err := d.runUntilFree(ctx, "query", "-X", "PUT", "--data", string(encoded),
 		"/1.0/network-acls/"+spec.Name); err != nil {
-		return d.firewallRefused(fmt.Errorf("write firewall %s: %w", spec.Name, err))
+		return d.firewallWriteFailed(fmt.Errorf("write firewall %s: %w", spec.Name, err))
 	}
 	return nil
 }
 
+// isAlreadyExists recognises the daemon refusing to create an object that is
+// already there — "The network ACL already exists" is the wording Incus 7.2
+// answers the loser of two concurrent creates with.
+func isAlreadyExists(err error) bool {
+	return err != nil && strings.Contains(strings.ToLower(err.Error()), "already exists")
+}
+
+// firewallWriteFailed reports a failed firewall write, withdrawing the
+// firewall capability only when the host actually refused the rule set.
+//
+// The distinction is measured, not cautious. On 2026-08-28 a transient OVSDB
+// port-group collision failed one EnsureFirewall PUT and the process answered
+// with `capabilities.firewall is now false` for the rest of its life — every
+// suite keyed on the capability then skips its enforcement assertions, so one
+// timing blip mutes the very instrument that would notice a real refusal.
+// A conflict that outlives the retry budget is still an error, returned and
+// logged by the caller; what it is not is the host saying these rules cannot
+// be enforced, which is the one thing firewallRefused exists to record (#454,
+// #181). TestASpentConflictBudgetDoesNotWithdrawTheCapability fails without
+// the distinction; TestAFirewallWriteTheHostRefusesWithdrawsTheCapability
+// holds the refusing half.
+func (d *Incus) firewallWriteFailed(err error) error {
+	if isTransientConflict(err) {
+		return err
+	}
+	return d.firewallRefused(err)
+}
+
 // firewallRefused records that the host refused a rule set this driver had
 // already accepted, and returns the error unchanged so no caller has to know.
 //