Skip to content

Conversation

@sugh01
Copy link
Collaborator

@sugh01 sugh01 commented Nov 28, 2025

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 28, 2025

🤖 Claude Code Review

Status: Complete

No critical issues found. The refactoring successfully centralizes container management and eliminates substantial boilerplate code.

Minor Observation:
The ContainerManager.Cleanup() method may be called twice in test scenarios (once via defer td.Stop(t) and once via t.Cleanup() safety mechanism). While testcontainers typically handles double-cleanup gracefully, making Cleanup() explicitly idempotent by clearing cleanupFunc and aerospikeClient after cleanup would be more defensive.

Positive Changes:

  • ✅ Container cleanup properly registered with t.Cleanup() to prevent resource leaks
  • ✅ PostgreSQL initialization includes retry logic and connection validation
  • ✅ Reduces ~10-15 lines of boilerplate per test
  • ✅ Consistent container setup across all tests
  • ✅ Clear documentation in teranode-test-guide.md

containerManager, err = containers.NewContainerManager(containers.UTXOStoreType(opts.UTXOStoreType))
require.NoError(t, err, "Failed to create container manager")

utxoStoreURL, err := containerManager.Initialize(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resource leak: If daemon initialization fails after this point, the container won't be cleaned up because td.Stop() is never called. Register cleanup with t.Cleanup() immediately after container initialization to prevent leaks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Fixed: Cleanup is now registered with t.Cleanup() immediately after container initialization (line 332-336).


cm.postgresContainer = postgresC

connStr, err := postgresC.ConnectionString(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential resource leak: If ConnectionString() (line 157) or url.Parse() (line 185) fails after the container is successfully created, the PostgreSQL container will not be terminated. Consider assigning cm.cleanupFunc immediately after line 155 (before operations that might fail) to ensure the container is cleaned up on all error paths.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still an issue in current code. The cleanupFunc should be moved to line 156 (right after cm.postgresContainer assignment) to ensure cleanup happens on all error paths, including the ConnectionString() failure at line 158.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in current code: The cleanupFunc is now correctly set at line 158, immediately after the container assignment (line 155), which ensures cleanup happens even if ConnectionString() or url.Parse() fails. The issue is resolved.

@liam liam self-assigned this Dec 1, 2025
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 1, 2025

@liam liam assigned sugh01 and unassigned liam Dec 1, 2025
@liam liam self-requested a review December 1, 2025 10:54
@sugh01 sugh01 merged commit b42963a into bsv-blockchain:main Dec 1, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants