From 13ed366002f67ec96df8e4422c32e02e975e4685 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Mon, 31 Aug 2026 19:51:06 +0000 Subject: [PATCH 1/3] Prevent test network lock convoys --- .../test_network_config_lock_test.go | 48 ++++++ lib/instances/test_network_config_test.go | 138 +++++++++++------- skills/test-agent/agents/test-agent/NOTES.md | 32 ++++ 3 files changed, 164 insertions(+), 54 deletions(-) create mode 100644 lib/instances/test_network_config_lock_test.go diff --git a/lib/instances/test_network_config_lock_test.go b/lib/instances/test_network_config_lock_test.go new file mode 100644 index 000000000..9c404a572 --- /dev/null +++ b/lib/instances/test_network_config_lock_test.go @@ -0,0 +1,48 @@ +package instances + +import ( + "path/filepath" + "syscall" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestTryWithTestFileLock(t *testing.T) { + lockPath := filepath.Join(t.TempDir(), "test.lock") + + called := false + acquired, err := tryWithTestFileLock(lockPath, func() error { + called = true + return nil + }) + require.NoError(t, err) + require.True(t, acquired) + 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 + acquired, err = tryWithTestFileLock(lockPath, func() error { + called = true + return nil + }) + require.NoError(t, err) + require.False(t, acquired) + require.False(t, called) +} + +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) +} diff --git a/lib/instances/test_network_config_test.go b/lib/instances/test_network_config_test.go index 0df71cb89..c7049994d 100644 --- a/lib/instances/test_network_config_test.go +++ b/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,9 +187,9 @@ func allocateTestNetworkLease(testName string, seq uint32) (*testNetworkLease, e cfg: cfg, release: func() { releaseOnce.Do(func() { - _ = withTestSubnetLock(func() error { - cleanupTestNetworkArtifacts(bridgeName, allocatedSubnet) + cleanupTestNetworkArtifacts(bridgeName, allocatedSubnet) + _ = withTestSubnetLock(func() error { leases, err := loadSubnetLeases() if err != nil { return nil @@ -215,9 +205,24 @@ func allocateTestNetworkLease(testName string, seq uint32) (*testNetworkLease, e }, nil } +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 +236,25 @@ func withTestSubnetLock(fn func() error) error { return fn() } -func openTestSubnetLockFile(lockPath string) (*os.File, error) { +func tryWithTestFileLock(lockPath string, fn func() error) (bool, error) { + lockFile, err := openTestLockFile(lockPath) + if err != nil { + return false, 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 false, nil + } + return false, fmt.Errorf("acquire lock: %w", err) + } + defer syscall.Flock(int(lockFile.Fd()), syscall.LOCK_UN) + + return true, 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 +384,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 +411,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 +435,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 +578,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 { + delArgs, ruleComment, ok := parseIPTablesAppendRule(table, line) + if !ok || ruleComment != comment { continue } - ruleNum, convErr := strconv.Atoi(fields[0]) - if convErr != nil { - 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 diff --git a/skills/test-agent/agents/test-agent/NOTES.md b/skills/test-agent/agents/test-agent/NOTES.md index bc1ab914f..d8c7a80ff 100644 --- a/skills/test-agent/agents/test-agent/NOTES.md +++ b/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. From 334ad51bb0c68586526c9cfcbb1bcac547bd3f04 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Mon, 31 Aug 2026 20:27:59 +0000 Subject: [PATCH 2/3] Simplify test network cleanup helpers --- .../test_network_config_lock_test.go | 6 ++---- lib/instances/test_network_config_test.go | 19 ++++++++----------- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/lib/instances/test_network_config_lock_test.go b/lib/instances/test_network_config_lock_test.go index 9c404a572..f651d72b7 100644 --- a/lib/instances/test_network_config_lock_test.go +++ b/lib/instances/test_network_config_lock_test.go @@ -12,12 +12,11 @@ func TestTryWithTestFileLock(t *testing.T) { lockPath := filepath.Join(t.TempDir(), "test.lock") called := false - acquired, err := tryWithTestFileLock(lockPath, func() error { + err := tryWithTestFileLock(lockPath, func() error { called = true return nil }) require.NoError(t, err) - require.True(t, acquired) require.True(t, called) heldLock, err := openTestLockFile(lockPath) @@ -27,12 +26,11 @@ func TestTryWithTestFileLock(t *testing.T) { defer syscall.Flock(int(heldLock.Fd()), syscall.LOCK_UN) called = false - acquired, err = tryWithTestFileLock(lockPath, func() error { + err = tryWithTestFileLock(lockPath, func() error { called = true return nil }) require.NoError(t, err) - require.False(t, acquired) require.False(t, called) } diff --git a/lib/instances/test_network_config_test.go b/lib/instances/test_network_config_test.go index c7049994d..8e5b66aa9 100644 --- a/lib/instances/test_network_config_test.go +++ b/lib/instances/test_network_config_test.go @@ -192,13 +192,10 @@ func allocateTestNetworkLease(testName string, seq uint32) (*testNetworkLease, e _ = withTestSubnetLock(func() error { leases, err := loadSubnetLeases() if err != nil { - return nil + return err } delete(leases, allocatedSubnet) - if err := saveSubnetLeases(leases); err != nil { - return nil - } - return nil + return saveSubnetLeases(leases) }) }) }, @@ -208,7 +205,7 @@ func allocateTestNetworkLease(testName string, seq uint32) (*testNetworkLease, e func cleanupStaleTestNetworks(routes []hostRoute) { testNetworkGuardCleanupOnce.Do(func() { lockPath := filepath.Join(os.TempDir(), "hypeman-test-network-cleanup.lock") - _, err := tryWithTestFileLock(lockPath, func() error { + 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 @@ -236,22 +233,22 @@ func withTestSubnetLock(fn func() error) error { return fn() } -func tryWithTestFileLock(lockPath string, fn func() error) (bool, error) { +func tryWithTestFileLock(lockPath string, fn func() error) error { lockFile, err := openTestLockFile(lockPath) if err != nil { - return false, fmt.Errorf("open lock file: %w", err) + 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 false, nil + return nil } - return false, fmt.Errorf("acquire lock: %w", err) + return fmt.Errorf("acquire lock: %w", err) } defer syscall.Flock(int(lockFile.Fd()), syscall.LOCK_UN) - return true, fn() + return fn() } func openTestLockFile(lockPath string) (*os.File, error) { From 44e1f8a766e1d7e7589c299fa561fe69c26576b5 Mon Sep 17 00:00:00 2001 From: yummybomb <19238148+yummybomb@users.noreply.github.com> Date: Mon, 31 Aug 2026 20:54:57 +0000 Subject: [PATCH 3/3] Prevent release from deleting reused leases --- .../test_network_config_lock_test.go | 35 +++++++++++++++++++ lib/instances/test_network_config_test.go | 25 +++++++------ 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/lib/instances/test_network_config_lock_test.go b/lib/instances/test_network_config_lock_test.go index f651d72b7..b7c881bbd 100644 --- a/lib/instances/test_network_config_lock_test.go +++ b/lib/instances/test_network_config_lock_test.go @@ -1,6 +1,7 @@ package instances import ( + "os" "path/filepath" "syscall" "testing" @@ -34,6 +35,40 @@ func TestTryWithTestFileLock(t *testing.T) { 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) diff --git a/lib/instances/test_network_config_test.go b/lib/instances/test_network_config_test.go index 8e5b66aa9..fcdfb1704 100644 --- a/lib/instances/test_network_config_test.go +++ b/lib/instances/test_network_config_test.go @@ -187,21 +187,26 @@ func allocateTestNetworkLease(testName string, seq uint32) (*testNetworkLease, e cfg: cfg, release: func() { releaseOnce.Do(func() { - cleanupTestNetworkArtifacts(bridgeName, allocatedSubnet) - - _ = withTestSubnetLock(func() error { - leases, err := loadSubnetLeases() - if err != nil { - return err - } - delete(leases, allocatedSubnet) - return saveSubnetLeases(leases) - }) + 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")