multi: fix harness race condition and CI fixes

[bhandras]

No description provided.

[gemini-code-assist]

Summary of Changes

Hello @bhandras, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the stability and reliability of the test harness by resolving critical race conditions related to Docker network management. By refining the network creation and cleanup processes, it ensures more robust and isolated test environments, particularly when running tests concurrently. The changes aim to prevent intermittent test failures caused by network state inconsistencies.

Highlights

• Race Condition Fixes: The pull request addresses race conditions in the test harness's Docker network management. The automatic pruning of stale networks during setup has been removed to prevent conflicts in parallel test execution, with each harness now responsible for its own network cleanup. A new verification step ensures Docker networks are fully accessible after creation, mitigating premature success reports.
• Network Prefix Update: The Docker network prefix used for isolation in the harness has been updated from "ark-itest-" to "ark-harness-" for improved consistency and clarity.
• Pruning Function Refinement: The pruneStaleHarnessNetworks function has been renamed to PruneStaleHarnessNetworks and made exported. Its documentation now clarifies that it is no longer automatically invoked during tests, serving instead as a utility for manual cleanup of empty stale Docker networks.

Ignored Files

• Ignored by pattern: .github/workflows/** (1)
  • .github/workflows/main.yml

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request addresses race conditions in the test harness and includes related CI fixes. The changes are well-reasoned, such as removing automatic network pruning to avoid conflicts in parallel tests and adding a verification step for network readiness. The network prefix rename and exporting the pruning function with better comments also improve clarity and usability. I have one suggestion to further enhance the robustness of the network readiness check.

[gemini-code-assist]

While verifying the network is a great addition to prevent race conditions, a single check might not be sufficient if the network isn't ready immediately. Using h.pool.Retry would make this check more robust by polling until the network is available or a timeout is reached. This is a common pattern for handling eventual consistency in test environments and is used elsewhere in the harness.

	require.NoError(h.T, h.pool.Retry(func() error {
		_, err := h.pool.Client.NetworkInfo(h.network.Network.ID)
		return err
	}), "failed to verify network exists")

[bhandras]

done

[claude]

Code Review

I've reviewed this PR against the repository's coding standards from CLAUDE.md and docs/development_guidelines.md. Here's my detailed feedback:

✅ Positive Observations

1. Excellent Problem Identification and Fix
The core issue is well-identified and properly addressed. Removing the automatic pruneStaleHarnessNetworks() call eliminates the race condition where parallel tests could delete each other's networks. The solution is clean and effective.

2. Good Documentation Practices

• The inline comment at harness/harness.go:443-450 thoroughly explains WHY the pruning was removed and provides actionable guidance for manual cleanup
• The function comment for PruneStaleHarnessNetworks() properly documents its purpose and includes a NOTE about why it's no longer called automatically
• Comments explain the intent (race condition prevention) rather than just stating the obvious

3. Network Verification Enhancement
Adding NetworkInfo() call after network creation (harness/harness.go:462-464) is a smart defensive measure that catches Docker readiness issues early.

4. CI Improvements
The artifact retention on test failure and permission fixes will significantly improve debugging capabilities.

---

🔧 Required Changes

1. Function Comment Formatting Issue (harness/harness.go:436-437)

The current function comment doesn't match the actual behavior after your changes:

// setupDockerEnvironment initializes the Docker pool, prunes stale networks,
// and creates an isolated network for this test run.

This comment still mentions "prunes stale networks" but that's no longer happening. Should be:

// setupDockerEnvironment initializes the Docker pool and creates an isolated
// Docker network for this test run. Each harness instance creates its own
// network and is responsible for cleaning it up during shutdown.

2. Function Comment Must Start with Function Name (harness/harness.go:696-697)

Per the coding guidelines, function comments must begin with the function name:

Current:

// PruneStaleHarnessNetworks performs best-effort cleanup of stale harness
// Docker networks that are empty.

Should wrap to avoid the split:

// PruneStaleHarnessNetworks performs best-effort cleanup of empty stale
// harness Docker networks.

This keeps "PruneStaleHarnessNetworks" as the clear subject and improves readability.

---

💡 Suggestions for Improvement

1. Network Prefix Change Justification

The network prefix changed from ark-itest- to ark-harness- (harness/harness.go:59). While this makes sense for consistency, it's worth noting:

• Any existing stale networks with the old prefix won't be matched by the new PruneStaleHarnessNetworks() function
• Consider documenting this in the commit message or adding a one-time cleanup step

2. Comment Formatting Consistency (harness/harness.go:702)

The comment style is inconsistent:

// Best-effort cleanup of empty harness networks.

This duplicates the function comment. Either expand it with implementation details or remove it entirely. If keeping it, make it more substantive:

// List all Docker networks and remove any with our prefix that have no
// active containers attached.

3. Test Coverage Consideration

The guidelines emphasize "near 90% test coverage where practical." Consider adding a unit test that:

• Verifies NetworkInfo() is called after network creation
• Tests the behavior when NetworkInfo() fails
• Validates that each harness instance creates unique networks

4. CI Workflow Comments

The commented-out cleanup step (.github/workflows/main.yml:47-49) is good for documentation, but consider if the comment should explain WHY it's disabled (performance/necessity) rather than just HOW to re-enable it.

---

🔍 Additional Observations

1. Error Handling
The use of require.NoError() is appropriate for test harness code where failures should immediately halt test execution.

2. Commit Message Quality
Your commit messages follow the repository's conventions well:

• Present tense ✓
• Proper prefixes (harness:, github:) ✓
• Subject ≤50 characters ✓
• Detailed body explaining WHY ✓

3. No Security Concerns
The changes are purely related to test infrastructure and don't introduce security vulnerabilities.

4. Performance Impact
Adding the NetworkInfo() verification adds minimal overhead (single API call) and is worth the reliability improvement.

---

📝 Summary

Required: Fix the two function comments to match coding standards and actual behavior.

Recommended: Consider adding test coverage and documenting the network prefix migration.

Overall Assessment: This is a solid fix for a tricky race condition. The code quality is good, with just minor documentation improvements needed to fully align with the repository's strict standards. The CI improvements are valuable additions that will help with debugging.

[claude]

Code Review

I've reviewed PR #8 and overall the changes look good. The race condition fix is well-reasoned and the CI improvements will help with debugging.

Strengths

1. Race Condition Fix (harness/harness.go:443-450) - The removal of automatic pruneStaleHarnessNetworks() is the right approach. Parallel tests were racing when one harness tried to clean up networks while another was creating them. The explanatory comment clearly documents why this was removed.

2. Network Verification (harness/harness.go:459-464) - Adding NetworkInfo() verification after creation catches Docker race conditions where network creation succeeds but isn't immediately accessible. Comment explains the WHY, following the project's literate programming style.

3. Function Visibility (harness/harness.go:696-701) - Converting pruneStaleHarnessNetworks() to PruneStaleHarnessNetworks() with proper documentation makes it available for manual cleanup.

4. CI Artifact Handling (.github/workflows/main.yml:194-217) - The artifact upload improvements are excellent: permission fixes prevent failures, matrix-aware naming avoids collisions, only uploads on failure (saves space), and if-no-files-found:ignore prevents false failures.

5. Network Naming Change (harness/harness.go:59) - ark-itest- to ark-harness- is more accurate.

Potential Issues & Suggestions

1. Stale Network Accumulation - While removing automatic cleanup fixes the race, stale networks will accumulate over time, especially if tests are interrupted. Consider adding one-time cleanup at test suite beginning with grace period filter, or documenting cleanup in README/CLAUDE.md.

2. Artifact Permission Fix Timing - The permission fix runs with if:always() but artifact upload only runs on if:failure(). Consider changing to if:failure() to match and avoid unnecessary work.

3. Error Log Level - Current code looks good, uses require.NoError() which fails the test without logging at error level, per CLAUDE.md guidelines.

Security & Performance

• No security issues identified (test infrastructure only)
• Positive: artifact uploads only on failure saves CI bandwidth/storage
• Removing unnecessary network pruning reduces Docker API calls
• Network verification adds one extra NetworkInfo() call per test, but negligible

Test Coverage

Changes are to test infrastructure. The proof will be in running CI - if parallel test runs complete without network errors, the fix is validated.

Alignment with CLAUDE.md

• Comment style: Comments explain WHY and HOW, not just WHAT
• Error handling: Proper use of require.NoError
• Function comments: PruneStaleHarnessNetworks() has proper godoc
• Commit messages: All follow guidelines (harness: prefix, present tense, explains WHY)

Recommendation

APPROVE with minor suggestions. The PR successfully addresses the race condition and improves CI debugging. The suggestions above are optional improvements, not blockers.

Before merging:

1. Verify make lint passes
2. Run make unit-race multiple times in parallel to verify race is fixed
3. Consider implementing stale network cleanup strategy for long-term maintenance

Great work identifying and fixing this race condition! The explanatory comments will be very helpful for future maintainers.

[claude]

Pull Request Review

This PR addresses race conditions in parallel test execution and improves CI artifact handling. Overall, the changes are well-reasoned and improve test reliability. Below are my findings:

✅ Positive Aspects

1. Network Verification Enhancement (harness/harness.go:462-465)

   • The addition of NetworkInfo verification after network creation is excellent defensive programming
   • Helps catch race conditions where Docker reports success but network isn't ready
   • Appropriate use of retry logic via h.pool.Retry()

2. Race Condition Fix (harness/harness.go:443-450)

   • Removing automatic pruneStaleHarnessNetworks() is the right solution
   • The detailed comment explains the race condition clearly:
     • Test A creates network
     • Test B prunes it (0 containers)
     • Test A fails when trying to attach containers
   • Function preserved as PruneStaleHarnessNetworks() for manual cleanup

3. Network Prefix Update (harness/harness.go:59)

   • Changed from ark-itest- to ark-harness- for clarity
   • Minor but improves semantic accuracy

4. CI Artifact Improvements (.github/workflows/main.yml:195-218)

   • Permission fixes ensure artifacts can be uploaded from sudo-run tests
   • Conditional upload only on failure saves storage costs
   • Proper sanitization of matrix values for artifact names
   • 5-day retention is reasonable for debugging

5. Space Cleanup Optimization (.github/workflows/main.yml:47-49)

   • Disabling cleanup in static-checks job saves ~30 seconds
   • Appropriate since that job doesn't need extra space

🔍 Code Quality Observations

Following Repository Conventions

• ✅ Comments properly explain why and how, not just what
• ✅ Function comments start with function name (PruneStaleHarnessNetworks, setupDockerEnvironment)
• ✅ Code properly organized into logical stanzas with explanatory comments
• ✅ Changes are atomic and well-scoped per commit

Commit Message Quality

All four commits follow the project's standards:

• ✅ Present tense in subject lines
• ✅ Package prefixes (harness:, github:)
• ✅ Detailed explanations in commit bodies
• ✅ Each commit is independently buildable

🤔 Minor Observations

1. Consistency in CI Jobs (.github/workflows/main.yml:89-90)

   • The lint job still uses cleanup-space while static-checks has it commented
   • Consider: Should lint also disable cleanup? It depends on whether lint needs the space
   • This is fine as-is if lint genuinely needs the cleanup

2. Network Verification Timeout (harness/harness.go:462)

   • Uses default pool.Retry() timeout (likely 30s based on defaultTimeout)
   • Consider: A shorter timeout might be sufficient since network creation should be fast
   • However, CI environments under load may benefit from the longer timeout
   • Recommendation: This is fine as-is, but could add explicit timeout if issues arise

3. Error Message Clarity (harness/harness.go:465)

   • Error message "failed to verify network exists" could include network ID/name
   • Minor improvement for debugging: "failed to verify network %s exists", h.network.Network.ID

4. Test Coverage

   • The existing harness_test.go with TestHarnessMultiNode effectively tests parallel scenarios
   • The ParallelN helper with semaphore is excellent for controlled parallelism

🔒 Security Considerations

• ✅ No security concerns
• Permission fixes (sudo chown) are appropriate for CI environment
• Artifact retention limited to 5 days

🚀 Performance Considerations

• ✅ Disabling space cleanup saves CI time
• ✅ Removing automatic pruning eliminates network deletion/recreation overhead
• Network verification adds negligible overhead (<1s typically)

📝 Suggestions (Optional Enhancements)

1. Enhanced Network Verification Error Message:

require.NoError(h.T, h.pool.Retry(func() error {
    _, err := h.pool.Client.NetworkInfo(h.network.Network.ID)
    if err != nil {
        return fmt.Errorf("network %s not accessible: %w", 
            h.network.Network.Name, err)
    }
    return nil
}), "failed to verify network exists")

2. Document Manual Cleanup in project docs or README:

   • Since automatic pruning is removed, document how to clean up stale networks
   • Command: docker network prune or call PruneStaleHarnessNetworks()

3. CI Artifact Path Specificity:

   • Current pattern **/test-artifacts/ is good
   • Consider if any other logs should be captured (e.g., **/harness.log)

✅ Approval Recommendation

This PR is ready to merge. The changes directly address the race condition issue, improve CI reliability, and follow the project's code standards excellently. The commit history is clean, explanations are thorough, and the implementation is sound.

The minor suggestions above are optional enhancements that don't block merging.

---

Review Summary:

• Code Quality: ⭐⭐⭐⭐⭐
• Testing: ⭐⭐⭐⭐⭐ (existing tests cover the scenarios)
• Documentation: ⭐⭐⭐⭐⭐ (excellent inline comments)
• Adherence to Guidelines: ⭐⭐⭐⭐⭐

Status: ✅ Approved