Not returning on connection close for chainsync, block-fetch and tx-submission protocol

[jkawan]

1. Added changes to not return error while closing the connection for chainsync, block-fetch and tx-submission protocol- made changes in client,server and respective protocol files.
2. Added unit tests to test the change.

Closes #1112

Summary by CodeRabbit

• New Features

  • Centralized connection error handling for more consistent closure semantics.

• Improvements

  • Per-protocol concurrency and explicit state tracking with safe state queries.
  • Protocol contexts propagated for clearer cancellation and lifecycle control.
  • Avoid redundant "done" messages during client shutdown to reduce noise.

• Tests

  • Expanded test coverage across connection handling, blockfetch, chainsync, txsubmission, and protocol lifecycle/configuration behaviors.

[jkawan]

1. Added changes to not return error while closing the connection for chainsync, block-fetch and tx-submission protocol- made changes in client,server and respective protocol files.
2. Added unit tests to test the change.

Closes #1112

[wolf31o2]

@coderabbitai review

[coderabbitai[bot]]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai[bot]]

[!NOTE]

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

📝 Walkthrough

Walkthrough

This PR implements protocol state tracking and conditional error handling to address closing connections when protocols are shut down. It adds IsDone() and GetDoneState() methods to track protocol terminal states, introduces a currentState field to the Protocol struct for state management, and modifies Stop() implementations in blockfetch and chainsync clients to conditionally send final messages only when protocols are active. New test suites validate protocol initialization, configuration, and message-sending behavior across blockfetch, chainsync, and txsubmission protocols. Connection tests are refactored to verify error handling differs when protocols are active versus stopped.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Core logic changes across multiple protocol implementations require careful verification
• New API methods (IsDone(), GetDoneState()) and currentState field tracking need validation for correctness with existing state machine
• Multiple similar test files (blockfetch, chainsync, txsubmission) reduce individual review effort but cumulative coverage is significant
• Conditional logic in Stop() methods affects error propagation semantics and warrants close inspection

Areas requiring extra attention:

• Protocol.IsDone() state detection logic and its correctness against various protocol states
• Interaction between currentState tracking and existing state machine in stateLoop
• Conditional Stop() implementations in blockfetch/client.go and chainsync/client.go — verify state checks prevent message sends appropriately
• Connection error handling behavior with active vs. stopped protocols in connection_test.go refactoring
• Test cleanup delays (100ms sleeps in blockfetch/client_test.go and chainsync/client_test.go) — assess if timing is reliable

Possibly related PRs

• #1233: Modifies protocol/protocol.go with currentState field and state-access methods, directly related to the state tracking infrastructure introduced here.

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	❓ Inconclusive	While most changes are in-scope, minor formatting changes (blank lines in txsubmission/client.go and keepalive/client_test.go) and a 100ms sleep delay in test cleanup appear tangential to the core objective.	Clarify if the sleep delays in blockfetch/client_test.go and chainsync/client_test.go are necessary for the feature or if they should be separated into a distinct refactoring PR.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: preventing error returns on connection close for three specific protocols (chainsync, block-fetch, tx-submission) after proper shutdown.
Linked Issues check	✅ Passed	The PR directly addresses issue #1112 by implementing the required functionality: preventing error returns when protocols are properly shut down before connection close across chainsync, block-fetch, and tx-submission.

✨ Finishing touches

• [ ] 📝 Generate docstrings

🧪 Generate unit tests (beta)

• [ ] Create PR with unit tests
• [ ] Post copyable unit tests in a comment
• [ ] Commit unit tests in branch feat/no-error-return

---

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 90450f286811a969c8f020f1f062f934a3b2ca78 and 26c42c7c9586b0da462e18d44fc730f850ec487b.

📒 Files selected for processing (5)

• connection_test.go (2 hunks)
• protocol/blockfetch/client_test.go (1 hunks)
• protocol/chainsync/client.go (1 hunks)
• protocol/chainsync/client_test.go (1 hunks)
• protocol/protocol.go (3 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• protocol/protocol.go

🧰 Additional context used

🧬 Code graph analysis (1)

protocol/chainsync/client.go (7)

protocol/keepalive/messages.go (1)

• NewMsgDone (94-101)

protocol/localtxmonitor/messages.go (1)

• NewMsgDone (79-86)

protocol/chainsync/messages.go (1)

• NewMsgDone (269-276)

protocol/localstatequery/messages.go (1)

• NewMsgDone (245-252)

protocol/peersharing/messages.go (1)

• NewMsgDone (88-95)

protocol/localtxsubmission/messages.go (1)

• NewMsgDone (118-125)

protocol/txsubmission/messages.go (1)

• NewMsgDone (169-176)

🪛 GitHub Actions: go-test

protocol/blockfetch/client_test.go

[error] 103-103: TestGetBlock: found unexpected goroutines after test execution

---

[error] 103-103: TestGetBlockNoBlocks: found unexpected goroutines after test execution

---

[error] 104-104: TestGetCurrentTip: found unexpected goroutines after test execution

---

[error] 104-104: TestGetAvailableBlockRange: found unexpected goroutines after test execution

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)

• GitHub Check: cubic · AI code reviewer
• GitHub Check: Analyze (go)

🔇 Additional comments (2)

protocol/chainsync/client.go (1)

150-153: Idempotent Stop via IsDone guard looks correct

Conditioning the Done send on !c.IsDone() under busyMutex keeps Stop idempotent and avoids spurious Done messages after a terminal state, while still propagating any SendMessage error via the outer err. This aligns with the intended shutdown semantics.

connection_test.go (1)

315-353: Basic error-handling tests (DialFailure/DoubleClose) look good

The new TestBasicErrorHandling subtests correctly exercise dial failure and Close idempotency without overreaching into protocol behavior. They’re small, focused, and compatible with goleak when Close() behaves as intended.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[wolf31o2]

I'm not sure if it's helpful, but I have some (abandoned) code on a similar path here: https://github.com/blinklabs-io/gouroboros/pull/1244

[wolf31o2]

@cubic-dev-ai review this PR

[cubic-dev-ai[bot]]

@cubic-dev-ai review this PR

@wolf31o2 I've started the AI code review. It'll take a few minutes to complete.