fix(ttx): fix localSession data race and unblock peers on close

[SurbhiAgarwal1]

Fixes #2285

Problem

1. info.Closed in localSession was read and written without synchronization, causing a data race under -race during self-auditing flows (where an issuer is also an auditor).
2. When a session is closed (e.g. via cleanupSessions), the peer sitting in <-session.Receive() was never unblocked and waited out the full DefaultReceiveTimeout (10 seconds).

Solution

• Added sync.RWMutex, closedChan, inFlight tracking, and writeClosed boolean to localSession.
• Info(), Receive(), Send(), and Close() are now synchronized and thread-safe.
• Close() safely closes writeChannel when inFlight == 0, instantly releasing any peer waiting on <-peer.Receive().
• Added unit tests in session_test.go covering peer unblocking on close, concurrent usage under -race, and double session closure.

[AkramBitar]

In TestLocalBidirectionalChannel_ConcurrentCloseAndOperations, the send goroutine calls leftSession.Send(ctx, []byte{byte(i)}), but the Send method on view.Session takes only []byte — no context argument. This should be either leftSession.SendWithContext(ctx, []byte{byte(i)}) or leftSession.Send([]byte{byte(i)}). Please verify before merge.

[SurbhiAgarwal1]

Hi @AkramBitar, thanks for the review!

I verified the view.Session interface definition in github.com/hyperledger-labs/fabric-smart-client/platform/view/view. In FSC, view.Session's Send method signature explicitly takes (ctx context.Context, payload []byte) error (with context as the first argument).

Omitting ctx causes a Go compilation error (*localSession does not implement view.Session: have Send([]byte) error, want Send(context.Context, []byte) error). So leftSession.Send(ctx, payload) matches the view.Session interface requirement.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review is ineligible. To be eligible to request a review, you need a paid Copilot license, or your organization must enable Copilot code review.

[AkramBitar]

Review summary: the core fix is correct — I could not find a send-on-closed-channel or double-close path (the closed check and inFlight++ share one critical section with Close(), and writeClosed guards the close), buffered messages survive the sender's Close(), and go test ./token/services/ttx/... ./token/services/utils/session/... -race is green at this head.

Two things are worth fixing before merge; the other three are nits that can be follow-ups.

1. send became nondeterministic when the caller's context is already cancelled (inline comment on the select). This one has a real caller path in the self-audit responder.
2. The close only unblocks the peer's receive; a peer blocked in send on a full buffer still stalls — the same class of hang #2285 is about, in the other direction.

Also, two test notes: TestLocalBidirectionalChannel_ConcurrentCloseAndOperations asserts nothing, and its receive goroutine's select { case <-ch: default: } almost always takes default, so it exercises the race detector but barely the receive path. The time.Sleep(10 * time.Millisecond) in TestLocalBidirectionalChannel_CloseReleasesPeerReceive is unnecessary — the peer gets the closed channel regardless of ordering.

[AkramBitar]

Medium: Send/SendError is now nondeterministic when the caller's context is already cancelled.

writeChannel is buffered (cap 10, set in NewLocalBidirectionalChannel), so with a cancelled ctx both case s.writeChannel <- msg and case <-ctx.Done() are ready, and Go picks pseudo-randomly. Measured on this head: 88 successes / 112 failures over 200 iterations with an already-cancelled context. Before this PR the buffered send always succeeded.

Concrete path: the self-audit responder runs via view3.RunView(logger, context, responderView, view.AsResponder(right)) on the initiator's view context (token/services/ttx/auditor.go:279). If the initiator returns first — its 1-minute signature timeout at auditor.go:189, or RunViewWithTimeout's defer cancel() — then the responder's signAndSendBack -> SendTyped(context.Context(), ...) delivers the signature about 44% of the time and returns context cancelled while sending message the rest, with no way for the caller to distinguish the two.

Suggested fix: test ctx.Err() before the select, or give the send case priority with a nested select { case ch <- msg: default: } before falling through to the three-way select.

[AkramBitar]

Medium: the unblock-the-peer-on-close fix is one-directional — a peer blocked in send is never released.

send selects only on s.closedChan, which is its own close signal. When left.Close() runs it closes lr (right's read channel), but nothing touches rl, so a right.Send blocked on a full rl keeps waiting.

Verified: after filling the 10-slot buffer and blocking an 11th send, calling right.Close() on the peer left the send blocked past a 500 ms deadline. It only returns when the sender's own ctx is cancelled or its own session is closed — i.e. the same stall class #2285 is about, just on the send side.

Suggested fix: share a close signal between the two localSessions, or also select on the read channel of the writing side.

[AkramBitar]

Medium: Receive() still returns a nil channel after the local Close(), so the closing side still burns the full 10 s DefaultReceiveTimeout.

A nil channel blocks forever, so ReceiveRawWithTimeout (token/services/utils/session/session.go:83) falls through to case <-timeout.C and returns ErrTimeout only after DefaultReceiveTimeout. This PR now has a closedChan that would make it instant, but doesn't use it here — returning s.readChannel (which the peer closes) or a pre-closed channel would fail fast with ErrNilMessage.

This is pre-existing behaviour, so not a regression, but it is exactly the 10-second stall the PR is meant to remove, and as written the two sides of the same fake session end up with asymmetric close semantics.

[AkramBitar]

Low: Send(nil, payload) now panics with a nil pointer dereference on ctx.Done(). Before this PR a nil context was only stored in msg.Ctx and was harmless.

No in-repo caller passes nil today, but NewLocalBidirectionalChannel / LeftSession / RightSession are exported, so this is a silent contract change. An if ctx == nil { ctx = context.Background() } guard — or dropping the ctx case per the fix suggested above — avoids it.

[AkramBitar]

Low: the !s.writeClosed term is dead here. writeClosed is only ever set inside send's deferred block, which requires s.closed == true; Close() has already returned early above in that case, so writeClosed is always false at this point.

Harmless, but it obscures the invariant — worth either dropping the term or adding a short comment stating why it cannot be true.

[AkramBitar]

@SurbhiAgarwal1

Could you please have a look at the failures?

Regards,
Akram