Prevent test network lock convoys

lib/instances/test_network_config_lock_test.go

@@ -0,0 +1,81 @@
+package instances
+
+import (
+	"os"
+	"path/filepath"
+	"syscall"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestTryWithTestFileLock(t *testing.T) {
+	lockPath := filepath.Join(t.TempDir(), "test.lock")
+
+	called := false
+	err := tryWithTestFileLock(lockPath, func() error {
+		called = true
+		return nil
+	})
+	require.NoError(t, err)
+	require.True(t, called)
+
+	heldLock, err := openTestLockFile(lockPath)
+	require.NoError(t, err)
+	defer heldLock.Close()
+	require.NoError(t, syscall.Flock(int(heldLock.Fd()), syscall.LOCK_EX))
+	defer syscall.Flock(int(heldLock.Fd()), syscall.LOCK_UN)
+
+	called = false
+	err = tryWithTestFileLock(lockPath, func() error {
+		called = true
+		return nil
+	})
+	require.NoError(t, err)
+	require.False(t, called)
+}
+
+func TestReleaseRemovesLeaseBeforeNetworkArtifacts(t *testing.T) {
+	tmpDir := t.TempDir()
+	t.Setenv("TMPDIR", tmpDir)
+
+	const subnet = "10.200.1.0/24"
+	require.NoError(t, saveSubnetLeases(map[string]subnetLease{subnet: {SubnetCIDR: subnet}}))
+
+	binDir := filepath.Join(tmpDir, "bin")
+	require.NoError(t, os.Mkdir(binDir, 0o755))
+	markerPath := filepath.Join(tmpDir, "lease-present-during-cleanup")
+	t.Setenv("HYPEMAN_TEST_RELEASE_MARKER", markerPath)
+	t.Setenv("HYPEMAN_TEST_RELEASE_LEASES", testSubnetLeaseFilePath())
+	t.Setenv("HYPEMAN_TEST_RELEASE_SUBNET", subnet)
+
+	ipScript := `#!/bin/sh
+if [ "$1" = "-4" ] && [ "$2" = "route" ] && [ "$3" = "del" ]; then
+	if grep -Fq "$HYPEMAN_TEST_RELEASE_SUBNET" "$HYPEMAN_TEST_RELEASE_LEASES"; then
+		touch "$HYPEMAN_TEST_RELEASE_MARKER"
+	fi
+fi
+exit 0
+`
+	require.NoError(t, os.WriteFile(filepath.Join(binDir, "ip"), []byte(ipScript), 0o755))
+	require.NoError(t, os.WriteFile(filepath.Join(binDir, "iptables"), []byte("#!/bin/sh\nexit 0\n"), 0o755))
+	t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH"))
+
+	releaseTestNetworkLease("hm1234", subnet)
+
+	require.NoFileExists(t, markerPath)
+	leases, err := loadSubnetLeases()
+	require.NoError(t, err)
+	require.NotContains(t, leases, subnet)
+}
+
+func TestParseIPTablesAppendRule(t *testing.T) {
+	line := `-A FORWARD -i hm1234 -o bond0 -m comment --comment "hypeman-fwd-out-hm1234" -j ACCEPT`
+	args, comment, ok := parseIPTablesAppendRule("filter", line)
+	require.True(t, ok)
+	require.Equal(t, "hypeman-fwd-out-hm1234", comment)
+	require.Equal(t, []string{
+		"-t", "filter", "-D", "FORWARD", "-i", "hm1234", "-o", "bond0",
+		"-m", "comment", "--comment", "hypeman-fwd-out-hm1234", "-j", "ACCEPT",
+	}, args)
+}

lib/instances/test_network_config_test.go

@@ -10,7 +10,6 @@ import (
 	"path/filepath"
 	"regexp"
 	"runtime"
-	"strconv"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -126,25 +125,16 @@ func allocateTestNetworkLease(testName string, seq uint32) (*testNetworkLease, e
 	var bridgeName string
 	var cfg config.NetworkConfig
 
+	if routes, err := listHostRoutes(); err == nil {
+		cleanupStaleTestNetworks(routes)
+	}
+
 	err := withTestSubnetLock(func() error {
 		routes, err := listHostRoutes()
 		if err != nil {
 			return err
 		}
 
-		testNetworkGuardCleanupOnce.Do(func() {
-			cleanupStaleLinkDownRoutes(routes)
-			// Sweep iptables rules for test bridges that no longer exist. Once a
-			// bridge is fully deleted its route is gone too, so linkdown cleanup
-			// above can't catch these — they would otherwise leak forever.
-			sweepOrphanedTestIPTablesRules()
-			// Refresh route snapshot after cleanup so subnet selection sees current state.
-			refreshed, refreshErr := listHostRoutes()
-			if refreshErr == nil {
-				routes = refreshed
-			}
-		})
-
 		leases, err := loadSubnetLeases()
 		if err != nil {
 			return err
@@ -197,27 +187,44 @@ func allocateTestNetworkLease(testName string, seq uint32) (*testNetworkLease, e
 		cfg: cfg,
 		release: func() {
 			releaseOnce.Do(func() {
-				_ = withTestSubnetLock(func() error {
-					cleanupTestNetworkArtifacts(bridgeName, allocatedSubnet)
-
-					leases, err := loadSubnetLeases()
-					if err != nil {
-						return nil
-					}
-					delete(leases, allocatedSubnet)
-					if err := saveSubnetLeases(leases); err != nil {
-						return nil
-					}
-					return nil
-				})
+				releaseTestNetworkLease(bridgeName, allocatedSubnet)
 			})
 		},
 	}, nil
 }
 
+func releaseTestNetworkLease(bridgeName, allocatedSubnet string) {
+	err := withTestSubnetLock(func() error {
+		leases, err := loadSubnetLeases()
+		if err != nil {
+			return err
+		}
+		delete(leases, allocatedSubnet)
+		return saveSubnetLeases(leases)
+	})
+	logTestNetworkErr("release subnet lease", err)
+
+	cleanupTestNetworkArtifacts(bridgeName, allocatedSubnet)
+}
+
+func cleanupStaleTestNetworks(routes []hostRoute) {
+	testNetworkGuardCleanupOnce.Do(func() {
+		lockPath := filepath.Join(os.TempDir(), "hypeman-test-network-cleanup.lock")
+		err := tryWithTestFileLock(lockPath, func() error {
+			cleanupStaleLinkDownRoutes(routes)
+			// Sweep iptables rules for test bridges that no longer exist. Once a
+			// bridge is fully deleted its route is gone too, so linkdown cleanup
+			// above can't catch these — they would otherwise leak forever.
+			sweepOrphanedTestIPTablesRules()
+			return nil
+		})
+		logTestNetworkErr("stale network cleanup", err)
+	})
+}
+
 func withTestSubnetLock(fn func() error) error {
 	lockPath := filepath.Join(os.TempDir(), "hypeman-test-network.lock")
-	lockFile, err := openTestSubnetLockFile(lockPath)
+	lockFile, err := openTestLockFile(lockPath)
 	if err != nil {
 		return fmt.Errorf("open subnet lock file: %w", err)
 	}
@@ -231,7 +238,25 @@ func withTestSubnetLock(fn func() error) error {
 	return fn()
 }
 
-func openTestSubnetLockFile(lockPath string) (*os.File, error) {
+func tryWithTestFileLock(lockPath string, fn func() error) error {
+	lockFile, err := openTestLockFile(lockPath)
+	if err != nil {
+		return fmt.Errorf("open lock file: %w", err)
+	}
+	defer lockFile.Close()
+
+	if err := syscall.Flock(int(lockFile.Fd()), syscall.LOCK_EX|syscall.LOCK_NB); err != nil {
+		if errors.Is(err, syscall.EWOULDBLOCK) {
+			return nil
+		}
+		return fmt.Errorf("acquire lock: %w", err)
+	}
+	defer syscall.Flock(int(lockFile.Fd()), syscall.LOCK_UN)
+
+	return fn()
+}
+
+func openTestLockFile(lockPath string) (*os.File, error) {
 	lockFile, err := os.OpenFile(lockPath, os.O_RDWR, 0)
 	if err == nil {
 		_ = lockFile.Chmod(0o666)
@@ -361,7 +386,7 @@ func cleanupStaleLinkDownRoutes(routes []hostRoute) {
 // captures both the full comment and the referenced bridge name. We deliberately
 // anchor on the "hm" test prefix so we never touch "ha"-prefixed rules from a
 // real (non-test) hypeman process running on the same host.
-var testRuleCommentPattern = regexp.MustCompile(`hypeman-(?:fwd-out|fwd-in|nat)-(hm[0-9a-f]+)`)
+var testRuleCommentPattern = regexp.MustCompile(`^hypeman-(?:fwd-out|fwd-in|nat)-(hm[0-9a-f]+)$`)
 
 // sweepOrphanedTestIPTablesRules removes hypeman test iptables rules whose
 // referenced bridge interface no longer exists. Once a bridge is fully deleted,
@@ -388,17 +413,19 @@ func sweepOrphanedTestRulesInChain(table, chain string) {
 		return
 	}
 
-	// Collect comments whose bridge interface is gone. Cache bridge existence and
-	// dedupe comments so we shell out and delete each orphan only once.
+	// Collect rules whose bridge interface is gone. Cache bridge existence, but
+	// retain duplicate rules so the sweep removes every copy.
 	exists := make(map[string]bool)
-	seen := make(map[string]struct{})
-	var orphanedComments []string
+	var orphanedRules [][]string
 	for _, line := range strings.Split(string(output), "\n") {
-		match := testRuleCommentPattern.FindStringSubmatch(line)
+		delArgs, comment, ok := parseIPTablesAppendRule(table, line)
+		if !ok {
+			continue
+		}
+		match := testRuleCommentPattern.FindStringSubmatch(comment)
 		if match == nil {
 			continue
 		}
-		comment := match[0]
 		bridge := match[1]
 
 		alive, checked := exists[bridge]
@@ -410,18 +437,11 @@ func sweepOrphanedTestRulesInChain(table, chain string) {
 			// Never delete a rule whose bridge interface still exists.
 			continue
 		}
-
-		if _, ok := seen[comment]; ok {
-			continue
-		}
-		seen[comment] = struct{}{}
-		orphanedComments = append(orphanedComments, comment)
+		orphanedRules = append(orphanedRules, delArgs)
 	}
 
-	// Delete by comment, reusing the existing line-number-based deleter which
-	// handles quoting and renumbering correctly.
-	for _, comment := range orphanedComments {
-		deleteIPTablesRulesByComment(table, chain, comment)
+	for _, delArgs := range orphanedRules {
+		deleteIPTablesRuleWithRetry(delArgs)
 	}
 }
 
@@ -560,37 +580,49 @@ func deleteIPTablesRulesByComment(table, chain, comment string) {
 	if table != "" {
 		args = append(args, "-t", table)
 	}
-	args = append(args, "-L", chain, "--line-numbers", "-n")
+	args = append(args, "-S", chain)
 	output, err := newTestIPTablesCommand(args...).Output()
 	if err != nil {
-		logTestNetworkErr(fmt.Sprintf("iptables list %s/%s for comment %q", table, chain, comment), err)
+		logTestNetworkErr(fmt.Sprintf("iptables -S %s/%s for comment %q", table, chain, comment), err)
 		return
 	}
 
-	var ruleNums []int
 	for _, line := range strings.Split(string(output), "\n") {
-		if !strings.Contains(line, comment) {
-			continue
-		}
-		fields := strings.Fields(line)
-		if len(fields) == 0 {
-			continue
-		}
-		ruleNum, convErr := strconv.Atoi(fields[0])
-		if convErr != nil {
+		delArgs, ruleComment, ok := parseIPTablesAppendRule(table, line)
+		if !ok || ruleComment != comment {
 			continue
 		}
-		ruleNums = append(ruleNums, ruleNum)
+		deleteIPTablesRuleWithRetry(delArgs)
+	}
+}
+
+func parseIPTablesAppendRule(table, line string) ([]string, string, bool) {
+	fields := strings.Fields(line)
+	if len(fields) < 2 || fields[0] != "-A" {
+		return nil, "", false
+	}
+	for i := range fields {
+		fields[i] = strings.Trim(fields[i], `"'`)
 	}
 
-	for i := len(ruleNums) - 1; i >= 0; i-- {
-		delArgs := []string{}
-		if table != "" {
-			delArgs = append(delArgs, "-t", table)
+	var comment string
+	for i := 0; i < len(fields)-1; i++ {
+		if fields[i] == "--comment" {
+			comment = fields[i+1]
+			break
 		}
-		delArgs = append(delArgs, "-D", chain, strconv.Itoa(ruleNums[i]))
-		deleteIPTablesRuleWithRetry(delArgs)
 	}
+	if comment == "" {
+		return nil, "", false
+	}
+
+	fields[0] = "-D"
+	args := make([]string, 0, len(fields)+2)
+	if table != "" {
+		args = append(args, "-t", table)
+	}
+	args = append(args, fields...)
+	return args, comment, true
 }
 
 // deleteIPTablesRuleWithRetry runs an iptables `-D` delete, retrying a few times

skills/test-agent/agents/test-agent/NOTES.md

@@ -474,3 +474,35 @@
   - pass on `deft-kernel-dev`, confirmed v49.0 and v51.1 x86_64 binaries report as x86-64 ELF.
 - `go test -count=10 -v -run "^TestMultipleVersions$" -timeout=10m ./lib/vmm`
   - pass on `deft-kernel-dev`, package time `0.741s`
+
+## 2026-08-31 - Shared test-network lock convoy
+
+### Flake signature
+- Main Test workflow run `33411612595`, attempt 1, timed out in `lib/instances` after 20 minutes.
+- The timeout dump showed many integration tests blocked in cleanup at `withTestSubnetLock`.
+- Attempt 2 passed in just under the 20-minute package timeout.
+
+### Root cause
+- Six Linux test suites were running concurrently on the shared host and contending on `/ci/hypeman-test-network.lock`.
+- The lock protected both the subnet lease file and slow host cleanup. Its holder was deleting stale bridges and iptables rules while every other suite waited to allocate or release a lease.
+- The host had roughly 8,500 accumulated hypeman iptables rules, making each line-number-based list/delete cleanup increasingly expensive.
+
+### Fix
+- Restricted the blocking subnet lock to route/lease selection and lease-file updates.
+- Moved stale host cleanup behind a separate non-blocking lock so another suite already performing the sweep never stalls allocation.
+- Changed iptables cleanup from mutable line numbers to exact rule specifications, allowing per-test host cleanup to run outside the subnet lease lock safely.
+- Reused the orphan sweep's existing `iptables -S` output instead of listing the full chain again for every orphan comment.
+
+### Validation
+- Baseline full no-cache runs on `deft-kernel-dev`:
+  - Run 1: `226s` (failed on an unrelated `TestCreateInstanceWithNetwork` tc assertion)
+  - Run 2: `530s` (pass)
+- Targeted lock/parser loop:
+  - `go test -count=20 -run '^(TestTryWithTestFileLock|TestParseIPTablesAppendRule)$' ./lib/instances`
+  - pass locally and on `deft-kernel-dev`
+- One initial post-fix full run failed on the unrelated `TestBuilderPersistentCacheReuse` integration test.
+- Required three consecutive full no-cache runs then passed:
+  - Run 1: `285s`
+  - Run 2: `443s`
+  - Run 3: `369s`
+- Full command used `go test -count=1 -tags containers_image_openpgp -timeout=20m ./...` with the CI prewarm directory, registry mirror, reflink strict mode, and `/ci` scratch path.