Skip to content

fix: reuse streams across session IDs#152

Merged
mpetrun5 merged 5 commits intomainfrom
fix/stream-deadlock
Feb 26, 2026
Merged

fix: reuse streams across session IDs#152
mpetrun5 merged 5 commits intomainfrom
fix/stream-deadlock

Conversation

@mpetrun5
Copy link
Copy Markdown
Collaborator

@mpetrun5 mpetrun5 commented Feb 26, 2026

Description

Related Issue Or Context

Closes: #149
Closes: #150

How Has This Been Tested? Testing details.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Checklist:

  • I have commented my code, particularly in hard-to-understand areas.
  • I have ensured that all acceptance criteria (or expected behavior) from issue are met
  • I have updated the documentation locally and in docs.
  • I have added tests to cover my changes.
  • I have ensured that all the checks are passing and green, I've signed the CLA bot

mpetrunic
mpetrunic previously approved these changes Feb 26, 2026
@github-actions
Copy link
Copy Markdown

Go Test coverage is 53.6 %\ ✨ ✨ ✨

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR changes libp2p stream management to reuse streams across session IDs, and addresses a coordinator mutex usage issue when checking pending processes.

Changes:

  • Update Coordinator.Execute to read pendingProcesses under lock before deciding whether a process is already pending.
  • Refactor StreamManager to cache streams per peer (instead of per session) and adjust libp2p communication to use the new API.
  • Update/trim stream manager tests and fix a typo in a communication health test name.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
tss/coordinator.go Moves the lock to cover the pendingProcesses read to avoid unsynchronized map access.
comm/p2p/manager.go Refactors stream cache from per-session to per-peer and introduces CloseStream + simplified Stream lookup/creation.
comm/p2p/libp2p.go Switches sending to the peer-cached stream manager and changes session close behavior.
comm/p2p/manager_test.go Updates tests for the new stream manager API and validates stream reuse.
comm/health_test.go Fixes test name typo (“Pears” → “Peers”).
Comments suppressed due to low confidence (1)

comm/p2p/libp2p.go:182

  • sendMessage() reuses a single cached stream per peer and writes to it without per-peer serialization. Concurrent broadcasts to the same peer can interleave writes and corrupt the newline-delimited framing used by WriteStream/ReadStream; add per-peer write locking/queueing or avoid sharing a stream across concurrent senders.
	stream, err = c.streamManager.Stream(to)
	if err != nil {
		return err
	}


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread comm/p2p/manager_test.go
Comment thread comm/p2p/manager.go
Comment thread comm/p2p/manager.go
Comment thread comm/p2p/manager.go
Comment thread comm/p2p/manager.go
Comment thread comm/p2p/libp2p.go
MakMuftic
MakMuftic previously approved these changes Feb 26, 2026
@mpetrun5 mpetrun5 dismissed stale reviews from MakMuftic and mpetrunic via 1ed1f02 February 26, 2026 12:17
@github-actions
Copy link
Copy Markdown

Go Test coverage is 53.6 %\ ✨ ✨ ✨

@mpetrun5 mpetrun5 merged commit 277fcce into main Feb 26, 2026
7 checks passed
@mpetrun5 mpetrun5 deleted the fix/stream-deadlock branch February 26, 2026 12:25
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.

Potential leader selecting race condition Failing to create stream, causes deadlock

4 participants