fix: preserve terminated-instance describe-visibility window in GC reaper (#363)

* fix: preserve terminated-instance describe-visibility window in GC reaper

The TerminatedTeardownReaper purged a terminated record the moment it drove
the instance's pending NAT/OVN teardown to done. Normal terminations always
arrive NAT/OVN-pending, so the first 2m GC sweep completed and purged them
immediately, removing the instance from DescribeInstances before clients could
observe state=terminated — an AWS-compat divergence that flaked e2e-single
TestRunInstancesMultiCount.

Stamp TerminatedAt on transition and purge a completed record only once it is
older than a 15m visibility window; fresher records are left to the terminated
bucket's 1h TTL, keeping the instance describable as terminated. The ADR-0003
§3 purge-on-completion invariants still hold (their seeded records predate the
window). Closes mulga-siv-328.

* test(e2e): poll intra-SG east-west ping until L2 converges

TestSameSGComms fired a single un-retried 'ping -c 3 -W 2' immediately after
the source instance's SSH became reachable, but the ICMP target is the
destination's east-west data plane. Two instances boot in parallel and the
destination guest NIC / OVN port flows come up several seconds after the EC2
running transition the launch waits on. When the probe wins that race the
source kernel reports 'Destination Host Unreachable' (ARP unresolved), not an
ACL drop, and the test flaked.

Replace the one-shot ping with pingConverged, a bounded poll that retries
until 0% packet loss or 45s. A genuine self-ingress/datapath break still
stays at 100% loss for the full window and fails, so coverage is preserved.

spinifex/vm/gc_invariants_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"testing"
+	"time"
 
 	"github.com/aws/aws-sdk-go/service/ec2"
 	"github.com/stretchr/testify/assert"
@@ -56,6 +57,30 @@ func TestRLC5_GCRetriesPendingTeardownToDoneThenPurges(t *testing.T) {
 	_ = m
 }
 
+// TestRLC5_GCRetainsRecentlyCompletedForVisibility enforces the describe-
+// visibility window: a record the GC drives to done while still inside the
+// window must be retained (not purged) so the just-terminated instance stays
+// describable as `terminated`; the 1h bucket TTL bounds visibility. Mirrors a
+// normal termination, which arrives NAT/OVN-pending and completes on the first
+// sweep, seconds after terminate.
+func TestRLC5_GCRetainsRecentlyCompletedForVisibility(t *testing.T) {
+	_, _, store, reaper, v := seedTerminated(t, map[string]string{
+		TeardownNAT: string(TeardownPending),
+		TeardownOVN: string(TeardownPending),
+	})
+	v.TerminatedAt = time.Now()
+	require.NoError(t, store.WriteTerminatedInstance(v.ID, v))
+
+	reaped, err := reaper.Sweep(context.Background())
+	require.NoError(t, err)
+
+	assert.Equal(t, 0, reaped, "a record completed inside the visibility window must not be purged")
+	assert.True(t, v.TeardownComplete(), "teardown is still driven to done")
+	remaining, err := store.ListTerminatedInstances()
+	require.NoError(t, err)
+	require.Len(t, remaining, 1, "the terminated instance must stay describable inside the window")
+}
+
 // TestRLC5_GCNeverPurgesIncompleteRecord enforces ADR-0003 §4 (no-orphan
 // completeness / never abandon): a dependent that keeps failing must leave the
 // terminated record present and marked, not purged, so a later sweep retries it.

spinifex/vm/shutdown.go

@@ -234,6 +234,12 @@ func (m *Manager) finalizeTerminated(instance *VM) error {
 		return fmt.Errorf("transition to terminated: %w", err)
 	}
 
+	// Stamp the termination time so the GC backstop can preserve a
+	// describe-visibility window before reclaiming the record early.
+	if instance.TerminatedAt.IsZero() {
+		instance.TerminatedAt = time.Now()
+	}
+
 	if m.deps.StateStore != nil {
 		if err := m.deps.StateStore.WriteTerminatedInstance(instance.ID, instance); err != nil {
 			slog.Error("Failed to write terminated instance to KV, keeping in local state for retry",

spinifex/vm/teardown_reaper.go

@@ -4,15 +4,24 @@ import (
 	"context"
 	"fmt"
 	"log/slog"
+	"time"
 )
 
+// terminatedVisibilityWindow is how long a freshly terminated instance stays
+// describable before the GC backstop may reclaim its record early. It exceeds
+// the e2e terminate-then-poll budget and stays under the bucket's 1h TTL, which
+// bounds visibility regardless. Records completed within the window are left to
+// the TTL (AWS keeps terminated instances describable ~1h); only records already
+// older than the window when the GC finishes their teardown are purged inline.
+const terminatedVisibilityWindow = 15 * time.Minute
+
 // TerminatedTeardownReaper completes interrupted instance teardown (ADR-0003
 // §1/§3). It scans the terminated KV bucket for records this node owns whose
 // per-dependent teardown is not all `done`, re-drives each outstanding
 // dependent through the now-idempotent cleaner, and purges the terminated
-// record once every dependent reaches `done`. A node-down mid-cascade leaves
-// dependents `pending`/`failed`; this reaper finishes them rather than
-// abandoning them.
+// record once every dependent reaches `done` and its describe-visibility window
+// has elapsed. A node-down mid-cascade leaves dependents `pending`/`failed`;
+// this reaper finishes them rather than abandoning them.
 type TerminatedTeardownReaper struct {
 	m *Manager
 }
@@ -29,8 +38,9 @@ func (r *TerminatedTeardownReaper) Class() string      { return "instance-teardo
 func (r *TerminatedTeardownReaper) Scope() ReaperScope { return ScopeNodeLocal }
 
 // Sweep finishes outstanding teardown for this node's terminated instances.
-// Records already complete on arrival are left to the bucket's TTL (preserving
-// describe-visibility); only records this sweep brings to completion are purged.
+// A completed record is purged only once it is older than the visibility window;
+// fresher ones (and any already complete on arrival) are left to the bucket's 1h
+// TTL so a just-terminated instance stays describable, matching AWS semantics.
 func (r *TerminatedTeardownReaper) Sweep(context.Context) (int, error) {
 	if r.m.deps.StateStore == nil {
 		return 0, nil
@@ -51,12 +61,15 @@ func (r *TerminatedTeardownReaper) Sweep(context.Context) (int, error) {
 
 		r.retryOutstanding(v)
 
-		if v.TeardownComplete() {
+		if v.TeardownComplete() && time.Since(v.TerminatedAt) >= terminatedVisibilityWindow {
 			if r.purge(v) {
 				reaped++
 			}
 			continue
 		}
+		// Either still incomplete (retry next sweep), or complete but inside the
+		// describe-visibility window: persist progress and leave the record to
+		// the 1h bucket TTL so the instance stays describable as terminated.
 		if err := r.m.deps.StateStore.WriteTerminatedInstance(v.ID, v); err != nil {
 			slog.Warn("vm/gc: failed to persist advanced teardown marks",
 				"instanceId", v.ID, "err", err)

spinifex/vm/vm.go

@@ -134,6 +134,12 @@ type VM struct {
 	// pending|failed). Stamped by terminateCleanup; the GC backstop retries
 	// pending/failed and purges the record once all dependents are done.
 	Teardown map[string]string `json:"teardown,omitempty"`
+
+	// TerminatedAt is when the instance entered StateTerminated. The GC backstop
+	// keeps a completed terminated record describable until this is older than
+	// the visibility window, after which it may reclaim it early; the bucket's
+	// 1h TTL bounds visibility regardless.
+	TerminatedAt time.Time `json:"terminated_at,omitzero"`
 }
 
 // ResetNodeLocalState zeroes node-specific fields after deserializing a VM

tests/e2e/single/baseline_test.go

@@ -8,6 +8,7 @@ import (
 	"net"
 	"os/exec"
 	"strconv"
+	"strings"
 	"time"
 
 	"testing"
@@ -83,6 +84,29 @@ func instancePrivateIP(t *testing.T, fix *Fixture, id string) string {
 	return ip
 }
 
+// pingConverged probes dst over ICMP from tgt, retrying until the east-west
+// datapath converges (0% loss) or timeout elapses. L2 between two freshly
+// launched instances has a warm-up: ARP cannot resolve until the destination
+// guest NIC is up and OVN has programmed its port flows, which lags the EC2
+// "running" transition the launch waits on. A single un-retried ping races that
+// window and reports a false "Destination Host Unreachable". Returns the last
+// ping output and whether it ever converged.
+func pingConverged(tgt harness.SSHTarget, dst string, timeout time.Duration) (string, bool) {
+	deadline := time.Now().Add(timeout)
+	var out string
+	for {
+		var err error
+		out, err = sshCapture(tgt, fmt.Sprintf("ping -c 3 -W 2 %s", dst))
+		if err == nil && strings.Contains(out, "0% packet loss") {
+			return out, true
+		}
+		if time.Now().After(deadline) {
+			return out, false
+		}
+		time.Sleep(2 * time.Second)
+	}
+}
+
 // sshCapture runs cmd over SSH and returns combined output + error without
 // fataling, so callers can assert on the exit status (e.g. ping result).
 func sshCapture(tgt harness.SSHTarget, cmd string) (string, error) {
@@ -317,10 +341,9 @@ func runSameSGComms(t *testing.T, fix *Fixture) {
 
 	tgt := harness.SSHTarget{User: "ec2-user", Host: host, Port: port, KeyPath: keyPath}
 	harness.Step(t, "ping %s (%s) from %s via default-SG self-ingress", dstID, dstPriv, srcID)
-	out, err := sshCapture(tgt, fmt.Sprintf("ping -c 3 -W 2 %s", dstPriv))
-	require.NoErrorf(t, err,
-		"intra-default-SG ping %s -> %s failed; self-ingress not enforced on the datapath\n%s",
+	out, converged := pingConverged(tgt, dstPriv, 45*time.Second)
+	require.Truef(t, converged,
+		"intra-default-SG east-west %s -> %s never reached 0%% loss within 45s; "+
+			"ARP/L2 datapath unreachable across the default-SG self-ingress\n%s",
 		srcID, dstID, out)
-	assert.Containsf(t, out, "0% packet loss",
-		"intra-default-SG ping had loss; self-ingress datapath degraded\n%s", out)
 }