fix: Prevent deadlock when releasing a realtime channel

[lmars]

The RealtimeChannels mutex is acquired to retrieve the channel relating to an inbound message, so holding that mutex whilst waiting for Detach to complete causes a deadlock because the inbound DETACHED message the Detach call is waiting for also tries to acquire the mutex.

This commit fixes the deadlock by releasing the mutex before calling Detach, and then acquiring it again once it completes in order to release the channel.

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability when releasing realtime channels: channels now detach cleanly before removal, preventing rare deadlocks and ensuring resources are fully freed.
  • After release, channels are correctly removed and no longer accessible.

• Tests

  • Added an integration test validating the full channel release lifecycle, including attach/detach state transitions and post-release cleanup.

[coderabbitai]

Walkthrough

Adjusts locking sequence in RealtimeChannels.Release to unlock before Detach and re-lock for map mutation. Adds an integration test to verify channel release triggers proper detach, state transitions, and channel removal.

Changes

Cohort / File(s)	Summary
Realtime channel release logic ably/realtime_channel.go	Changes Release: acquire lock, get channel, unlock; call Detach; re-lock, remove channel from map, unlock. Avoids holding the lock during Detach; ensures map mutation under lock.
Integration test for release flow ably/realtime_channel_integration_test.go	Adds TestRealtimeChannels_Release validating attach→attached, release triggers detaching→detached, and channel removal.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant App as Client Code
  participant RC as RealtimeChannels
  participant M as Mutex
  participant C as Channel
  participant Map as ChannelMap

  App->>RC: Release(name)
  RC->>M: Lock
  RC->>Map: Get(name)
  RC->>M: Unlock
  alt Channel exists
    RC->>C: Detach()
    Note right of C: Detach proceeds without RC lock
    RC->>M: Lock
    RC->>Map: Delete(name)
    RC->>M: Unlock
    RC-->>App: Return
  else No channel
    RC-->>App: Return (no-op)
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

I hop through code, ears tuned to locks,
Unlatch, detach—no deadlock shocks.
Re-lock, tidy maps in place,
Tests confirm a cleaner race.
Thump-thump! The channels softly cease—
A rabbit signs off, released in peace. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The title clearly and concisely describes the primary change of preventing a deadlock when releasing a realtime channel by adjusting the lock sequence.

✨ 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 fix-channels-release-deadlock

---

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.}

[sacOO7]

LGTM

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

ably/realtime_channel_integration_test.go (1)

345-388: Consider adding tests for the deadlock scenario and edge cases.

The test effectively verifies the happy path: channel attachment, release, state transitions, and cleanup. However, it doesn't test the specific deadlock scenario that the PR fixes or important edge cases.

Consider adding tests for:

1. Deadlock scenario: A test that verifies Release doesn't block when messages arrive during detach (the original bug)
2. Concurrent Release: Multiple goroutines calling Release on the same channel simultaneously
3. Release on detached channel: Calling Release on a channel that's already DETACHED or INITIALIZED
4. Release + Get race: Concurrent Get and Release on the same channel name

Would you like me to generate test cases for these scenarios?

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

• Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between b5c8e55 and 76f2b5b.

📒 Files selected for processing (2)

• ably/realtime_channel.go (1 hunks)
• ably/realtime_channel_integration_test.go (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

ably/realtime_channel_integration_test.go (6)

ablytest/sandbox.go (1)

• NewRealtime (108-115)

ablytest/recorders.go (1)

• FullRealtimeCloser (202-204)

ablytest/resultgroup.go (2)

• Wait (17-31)
• ConnWaiter (116-163)

ably/state.go (6)

• ChannelStateChange (601-621)
• ChannelStateChange (623-623)
• ChannelStateAttaching (524-524)
• ChannelStateAttached (528-528)
• ChannelStateDetaching (532-532)
• ChannelStateDetached (535-535)

ably/export_test.go (1)

• ChannelStateChanges (203-203)

ablytest/timeout.go (1)

• Soon (12-12)

⏰ 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). (6)

• GitHub Check: integration-test (1.24, json)
• GitHub Check: integration-test (1.23, json)
• GitHub Check: integration-test (1.25, json)
• GitHub Check: integration-test (1.19, msgpack)
• GitHub Check: integration-test (1.19, json)
• GitHub Check: integration-test (1.20, json)

🔇 Additional comments (1)

ably/realtime_channel.go (1)

208-223: Approve deadlock fix as-is. The revised lock/unlock around Detach correctly prevents the previous deadlock, and our search found no concurrent Get / Release usage that would trigger the narrow race window.