fix(vpcd): make handleDeleteNAT idempotent on 'already absent' (mulga-siv-107)

Instance termination fans out through multiple cleanup paths
(instanceCleanerAdapter.ReleasePublicIP AND
VPCServiceImpl.DeleteNetworkInterface), both of which publish
vpc.delete-nat for the same (vpc, logical_ip). The publish is
fire-and-forget (utils.PublishNATEvent uses PublishEvent for
vpc.delete-nat, not RequestEvent), so callers never branch on the
response. The second publisher then triggered a spurious
'ERROR vpcd: failed to delete dnat_and_snat rule ... not found' log
line ms after the first delete succeeded — confusing triage signal
with no actionable consequence.

Add ErrNATNotFound sentinel to the OVN client interface and wrap the
not-found return paths in both LiveOVNClient and MockOVNClient.
handleDeleteNAT matches errors.Is(err, ErrNATNotFound) and treats
that as success (logs INFO 'already absent (idempotent)').

Existing TestTopologyHandler_DeleteNAT_NotFound asserted the opposite
contract; the comment claimed EIP disassociation needed the error
signal, but DisassociateAddress also fire-and-forgets vpc.delete-nat,
so the contract never had a real consumer. Test renamed to
_Idempotent with assertSuccess.

Other failure modes (router missing, OVN unreachable, transact error)
still surface as ERROR + reply with the error string.

Refs: mulga-siv-107

spinifex/services/vpcd/mock_ovn.go

@@ -452,7 +452,7 @@ func (m *MockOVNClient) DeleteNAT(_ context.Context, routerName string, natType,
 		}
 	}
 	if foundUUID == "" {
-		return fmt.Errorf("NAT %s %s not found", natType, logicalIP)
+		return fmt.Errorf("NAT %s %s: %w", natType, logicalIP, ErrNATNotFound)
 	}
 	// Remove from router's NAT list
 	for i, uuid := range lr.NAT {
@@ -479,7 +479,7 @@ func (m *MockOVNClient) DeleteNATByExternalIP(_ context.Context, routerName stri
 		}
 	}
 	if len(foundUUIDs) == 0 {
-		return fmt.Errorf("NAT %s external_ip=%s not found", natType, externalIP)
+		return fmt.Errorf("NAT %s external_ip=%s: %w", natType, externalIP, ErrNATNotFound)
 	}
 	for _, foundUUID := range foundUUIDs {
 		for i, uuid := range lr.NAT {

spinifex/services/vpcd/ovn.go

@@ -2,6 +2,7 @@ package vpcd
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"log/slog"
 	"slices"
@@ -12,6 +13,12 @@ import (
 	"github.com/ovn-kubernetes/libovsdb/ovsdb"
 )
 
+// ErrNATNotFound is returned by DeleteNAT / DeleteNATByExternalIP when the
+// rule is already absent. Callers that want idempotent semantics (e.g. the
+// vpc.delete-nat NATS handler, which races with vm-cleanup paths that emit
+// the same delete) match it with errors.Is and treat as success.
+var ErrNATNotFound = errors.New("NAT not found")
+
 // transactOps executes a set of OVSDB operations as a single transaction,
 // checking both the RPC error and individual operation results.
 func (c *LiveOVNClient) transactOps(ctx context.Context, ops []ovsdb.Operation) error {
@@ -841,7 +848,7 @@ func (c *LiveOVNClient) DeleteNAT(ctx context.Context, routerName string, natTyp
 		return fmt.Errorf("find NAT: %w", err)
 	}
 	if len(nats) == 0 {
-		return fmt.Errorf("NAT %s %s not found", natType, logicalIP)
+		return fmt.Errorf("NAT %s %s: %w", natType, logicalIP, ErrNATNotFound)
 	}
 
 	nat := &nats[0]
@@ -883,7 +890,7 @@ func (c *LiveOVNClient) DeleteNATByExternalIP(ctx context.Context, routerName st
 		return fmt.Errorf("find NAT by external IP: %w", err)
 	}
 	if len(nats) == 0 {
-		return fmt.Errorf("NAT %s external_ip=%s not found", natType, externalIP)
+		return fmt.Errorf("NAT %s external_ip=%s: %w", natType, externalIP, ErrNATNotFound)
 	}
 
 	lr, err := c.GetLogicalRouter(ctx, routerName)

spinifex/services/vpcd/topology.go

@@ -3,6 +3,7 @@ package vpcd
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"net"
@@ -1380,6 +1381,17 @@ func (h *TopologyHandler) handleDeleteNAT(msg *nats.Msg) {
 	routerName := "vpc-" + evt.VpcId
 
 	if err := h.ovn.DeleteNAT(ctx, routerName, "dnat_and_snat", evt.LogicalIP); err != nil {
+		// vpc.delete-nat is published from multiple cleanup paths (per-VM
+		// ReleasePublicIP, ENI delete, VPC delete sweep), so a duplicate
+		// delete for the same logical_ip is expected. Treat "already gone"
+		// as success — the caller's intent ("ensure this NAT is removed")
+		// is satisfied whether we did the delete or someone else did.
+		if errors.Is(err, ErrNATNotFound) {
+			slog.Info("vpcd: dnat_and_snat rule already absent (idempotent)",
+				"router", routerName, "logical_ip", evt.LogicalIP)
+			respond(msg, nil)
+			return
+		}
 		slog.Error("vpcd: failed to delete dnat_and_snat rule", "router", routerName, "logicalIP", evt.LogicalIP, "err", err)
 		respond(msg, err)
 		return

spinifex/services/vpcd/topology_test.go

@@ -4015,11 +4015,17 @@ func TestTopologyHandler_DeleteNAT_Success(t *testing.T) {
 	assert.Empty(t, lr.NAT, "NAT rule must be removed")
 }
 
-// TestTopologyHandler_DeleteNAT_NotFound: an idempotent delete-by-LogicalIP
-// for a NAT that doesn't exist must return a clear error, not silently
-// succeed. Otherwise EIP disassociation can't tell whether it actually
-// disabled the NAT or hit a stale fixture.
-func TestTopologyHandler_DeleteNAT_NotFound(t *testing.T) {
+// TestTopologyHandler_DeleteNAT_NotFound_Idempotent: instance termination
+// fans out through multiple cleanup paths (ReleasePublicIP via
+// instanceCleanerAdapter AND DeleteNetworkInterface in the VPC service), and
+// both publish vpc.delete-nat for the same (vpc, logical_ip). The publish is
+// fire-and-forget (utils.PublishNATEvent for vpc.delete-nat), so callers
+// never branch on the response — a hard error on second delete just
+// pollutes triage logs without surfacing any actionable failure. Treat
+// "rule already absent" as success so the second publisher doesn't emit a
+// spurious ERROR. The handler still returns an error for other failure
+// modes (router missing, OVN unreachable, transact failure).
+func TestTopologyHandler_DeleteNAT_NotFound_Idempotent(t *testing.T) {
 	_, nc := startTestNATS(t)
 	mock := NewMockOVNClient()
 	_ = mock.Connect(context.Background())
@@ -4040,5 +4046,5 @@ func TestTopologyHandler_DeleteNAT_NotFound(t *testing.T) {
 	data, _ := json.Marshal(delEvt)
 	resp, err := nc.Request(TopicDeleteNAT, data, 5_000_000_000)
 	require.NoError(t, err)
-	assertFailure(t, resp, "delete-nat for non-existent rule must fail")
+	assertSuccess(t, resp, "delete-nat for non-existent rule must be idempotent (mulga-siv-107)")
 }