Cleanup service - move preserve parent and utxo cleanup from blockass…

[freemans13]

…embly to cleanup service

[github-actions]

🤖 Claude Code Review

Status: Complete

Current Review:

Found 2 issues:

1. Goroutine leak in notification handler (services/cleanup/Server.go:113-126)

   • Notification subscription goroutine does not respect context cancellation

2. Unused blobStore parameter (services/cleanup/Server.go:63)

   • Parameter accepted but never used, passed as nil from daemon

---

Architecture Summary:
This PR successfully extracts cleanup logic from BlockAssembler to a dedicated Cleanup service. The new service polls BlockAssembly state and coordinates preserve-parent and DAH cleanup operations sequentially, with proper safety checks to prevent cleanup during reorgs.

[freemans13]

🤖 Claude Code Review

Status: Complete

Current Review:

This is a well-architected refactoring that extracts cleanup logic from BlockAssembler into a dedicated Cleanup service. The design is solid with proper separation of concerns, comprehensive error handling, and good coordination patterns.

Strengths:

• Clean extraction of cleanup responsibilities following microservices architecture
• Proper coordination with block persister via BlockPersisted notifications to avoid data loss
• Sequential processing with deduplication prevents flooding during block catchup
• Three-phase cleanup (preserve parents → process expired → DAH) with abort-on-failure for safety
• Comprehensive Prometheus metrics for observability
• Health check implementation with dependency validation

Minor observations:

• protobuf RPC method naming: HealthGRPC instead of Health - this appears intentional but differs from other services
• The protobuf Go package path references bitcoin-sv/teranode (should be bsv-blockchain/teranode) but this is generated code

Overall, this refactoring improves maintainability and follows teranode's microservices patterns correctly.

1. ✅ RPC Method Naming: HealthGRPC vs Health
   This is correct and intentional. This follows the established pattern across all teranode services:
   Health(ctx context.Context, checkLiveness bool) - Internal method used by health check framework
   HealthGRPC(ctx context.Context, *EmptyMessage) - gRPC endpoint that wraps the internal method
   Pattern confirmed in:
   services/alert/Server.go
   services/blockassembly/Server.go
   services/blockchain/Server.go
   services/blockvalidation/Server.go
   services/propagation/Server.go
   services/subtreevalidation/Server.go
   The cleanup service follows this exact pattern.
2. ✅ Protobuf Package Path
   Already correct. The .proto file uses:
   option go_package = "github.com/bsv-blockchain/teranode/services/cleanup/cleanup_api";
   The generated .pb.go files correctly reference bsv-blockchain/teranode. There are no references to the old bitcoin-sv organization name in the cleanup service code or generated protobuf files.
   Summary
   Both observations from GitHub are non-issues:
   The HealthGRPC naming is the correct established pattern
   The protobuf package paths are already correct (bsv-blockchain/teranode)
   No changes needed! The cleanup service follows all teranode conventions correctly. ✅

[github-actions]

Status: Still present

The notification subscription goroutine does not respect context cancellation. If the service stops before the subscription channel closes, this goroutine will leak.

Analysis: The issue remains at lines 113-126 in Server.go. The goroutine needs a select statement to handle both ctx.Done() and the subscription channel to prevent leaking when the service stops.

[github-actions]

Status: Still present

The blobStore parameter is accepted in the New() constructor but never used anywhere in the service implementation. Since it is passed as nil from daemon_services.go, this parameter should be removed to keep the API clean.

Evidence:

• Stored in Server struct (line 43)
• Accepted by constructor (line 63)
• Passed as nil by caller in daemon_services.go (line 1132)
• Never referenced or used anywhere in the cleanup service

[icellan]

Why is this in a new config package and not the settings package?

[icellan]

Use GetBlockAssemblyClient()

[icellan]

Should be called worker.go (snake case, small letters, not include the package name)

https://github.com/bsv-blockchain/teranode/blob/5c7609ec21e7ac9966ed057db71a737de6afdc7f/docs/references/codingConventions.md

[icellan]

metrics should be registered in the initPrometheusMetrics function. Otherwise the metrics will be created on every single service that is run, regardless of whether the cleanup service is working or not. This creates a lot of noise and extra data/processing for the metrics server. We should follow the pattern as in the other services.