Make releasing a realtime channel that isn't detached an error (RTS4c-e)

[SimonWoolf]

Implements RTS4c–e from spec 6.3.0 (ably/specification#557) for the next major version.

Breaking change: channels.release(name) no longer detaches the channel implicitly.

• If the channel is INITIALIZED, DETACHED or FAILED, it is removed from the collection before release() returns.
• In any other state, release() throws AblyException with code 90011 and status 400, and the channel is left unchanged.
• Releasing a name that isn't in the collection is a no-op.
• AblyRealtime.Channels.release now declares throws AblyException.

Migration: call channel.detach(listener) and wait for it to complete before calling channels.release(name).

Also:

• Removes the released-channel special cases from the detach path, which are now unreachable.
• Removes the test of the old detach-on-release behaviour.
• Stops the liveobjects integration test teardown from releasing channels that may still be attached.
• Adds UTS-derived tests for RTS4c, RTS4d and RTS4e.

90011 is registered in ably/ably-common#367. The deprecation warning for the current major is in #1250. Reference ably-js PRs: ably/ably-pubsub-js#2322 (deprecation) and ably/ably-pubsub-js#2323 (next major).

🤖 Generated with Claude Code

[coderabbitai]

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 59 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 301ed054-5e70-46e2-8470-232142e8e1eb

---

📥 Commits

Reviewing files that changed from the base of the PR and between 7186ce2 and d87239a.

---

📒 Files selected for processing (6)

• lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
• lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
• lib/src/test/java/io/ably/lib/test/realtime/RealtimeChannelTest.java
• lib/src/test/kotlin/io/ably/lib/uts/unit/realtime/ChannelsCollectionTest.kt
• liveobjects/src/test/kotlin/io/ably/lib/liveobjects/integration/setup/IntegrationTest.kt
• pubsub-adapter/src/main/kotlin/com/ably/pubsub/Channels.kt

---

---

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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

[ttypic]

LGTM

[Copilot]

🟡 Changes recommended

Address concurrency and released-channel callback safety, and add FAILED-state coverage.

2 open findings

• Synchronize channel state validation and removal · New
• Make channel removal and disposal atomic · New

What changed in this PR

Implements RTS4c–e for next-major realtime channel release semantics, rejecting release of non-terminal channels with error 90011.

Changes:

• Removes implicit detach-on-release behavior.
• Adds UTS-derived release tests.
• Updates teardown, documentation, and obsolete tests.

File	Summary
pubsub-adapter/​src/​main/​kotlin/​com/​ably/​pubsub/​Channels.kt	Updates release documentation.
liveobjects/​src/​test/​kotlin/​io/​ably/​lib/​liveobjects/​integration/​setup/​IntegrationTest.kt	Avoids releasing attached channels during teardown.
lib/​src/​test/​kotlin/​io/​ably/​lib/​uts/​unit/​realtime/​ChannelsCollectionTest.kt	Adds RTS4c–e release coverage.
lib/​src/​test/​java/​io/​ably/​lib/​test/​realtime/​RealtimeChannelTest.java	Removes obsolete detach-on-release coverage.
lib/​src/​main/​java/​io/​ably/​lib/​realtime/​ChannelBase.java	Removes obsolete released-channel detach handling.
lib/​src/​main/​java/​io/​ably/​lib/​realtime/​AblyRealtime.java	Enforces terminal-state release and LiveObjects disposal.

🧠 Review effort: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[sacOO7]

@coderabbitai can you validate and expand on the above

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd '^(ChannelBase|Channel|InternalMap)\.java$'
fd '^(ChannelBase|Channel|InternalMap)\.java$' --exec ast-grep outline {} --items all
rg -n -C3 --type java '\b(markAsReleased|released|setState|requestState|attach|detach)\b' lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java

Length of output: 8697

---

🏁 Script executed:

#!/bin/bash
rg -n -C5 'volatile|synchronized|isReleased|released|state =|markAsReleased|checkChannelIsNotReleased' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
ast-grep run --lang java --pattern '$RET $NAME($$$ARGS) { $$$BODY }' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java | head -n 20
rg -n -C25 'void (attachImpl|detachImpl|markAsReleased|checkChannelIsNotReleased|setState|attachWithTimeout)\(' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
cat -n lib/src/main/java/io/ably/lib/util/InternalMap.java
fd '^LiveObjectsPlugin\.java$' --exec ast-grep outline {} --items all
rg -n -C4 --type java 'void dispose\(String|void dispose\(.*channel|new ErrorInfo\(' lib/src/main/java/io/ably/lib/liveobjects

Length of output: 39767

---

🏁 Script executed:

#!/bin/bash
sed -n '65,102p' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
sed -n '195,233p' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java
sed -n '210,254p' lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java
sed -n '1670,1690p' lib/src/main/java/io/ably/lib/realtime/ChannelBase.java

Length of output: 6505

---

@sacOO7 The core finding is valid. I checked the current code. The proposed plugin cleanup rule needs one additional condition.

Why the state check is unsafe

In lib/src/main/java/io/ably/lib/realtime/ChannelBase.java:

• state is not volatile.
• setState() writes state under synchronized (this) at Line 150.
• attachWithTimeout() holds the channel monitor and checks the released flag at Line 528.

In AblyRealtime.java, release() reads state without that monitor. It then removes the channel and marks it released.

This sequence is possible:

Step	Release thread	Attach thread
1	Reads initialized; validation passes.
2		Acquires the channel monitor; the released flag is still false.
3		Changes the state to attaching and sends ATTACH.
4	Removes the channel and marks it released.

The result violates the release restriction. The channel leaves the collection while an attach is in progress. Subsequent channel messages can reach the “non-existent channel” path.

A stale state read is also possible because the read does not use the writer’s monitor. However, the sequence above proves the race without relying on a stale read.

What the fix must coordinate

Use the channel monitor, not just the collection monitor, for the state check, conditional removal, and markAsReleased().

This gives concurrent attach and release a defined order:

• If attach changes the state first, release sees attaching and throws error 90011.
• If release completes first, attach sees the released flag and throws the existing IllegalStateException.

Making state volatile alone would not fix the check-then-remove race.

Also check the return value of map.remove(channelName, channel). The current code ignores it. If another release already removed that instance, this invocation must not repeat plugin disposal.

Plugin cleanup needs additional coordination

Successful removal is necessary, but it is not sufficient.

dispose(channelName) identifies plugin resources by name. Meanwhile, channels.get(channelName) can create a replacement channel after removal. The replacement constructor calls liveObjectsPlugin.getInstance(name).

Therefore, old-channel cleanup can overlap with replacement-channel initialization. Synchronizing on the old channel does not protect the replacement channel.

The complete fix must either:

• Coordinate removal and plugin disposal with channel creation for the same name, including constructor initialization; or
• Make plugin disposal specific to the old channel’s resource instance.

The lock design must also respect existing lock ordering and callback behavior.

Useful regression cases are attach winning the race, release winning the race, two concurrent releases, and release overlapping with recreation of the same channel name. This validation used source inspection; I did not run tests.

You are interacting with an AI system.

[sacOO7]

@coderabbitai can you validate and expand on the above

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
fd -i '^(LiveObjectsPlugin|DefaultRealtimeObject|Channel|InternalMap)\.(java|kt)$'
rg -n -C3 'void dispose\(|fun dispose\(|class DefaultRealtimeObject|class LiveObjectsPlugin' --glob '*.java' --glob '*.kt' .
fd -i '^(LiveObjectsPlugin|DefaultRealtimeObject|Channel|InternalMap)\.(java|kt)$' -x ast-grep outline {}

Length of output: 14726

---

🏁 Script executed:

#!/bin/bash
ast-grep outline liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt --items all
fd '^ChannelBase\.java$' -x ast-grep outline {} --items all
ast-grep outline lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java --items all
nl -ba liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt | sed -n '1,150p'
nl -ba lib/src/main/java/io/ably/lib/liveobjects/LiveObjectsPlugin.java | sed -n '15,68p'
nl -ba lib/src/main/java/io/ably/lib/util/InternalMap.java | sed -n '1,100p'
nl -ba core/src/main/java/io/ably/lib/realtime/Channel.java | sed -n '1,75p'
fd '^ChannelBase\.java$' -x rg -n -C8 'getInstance\(|ChannelBase\(|markAsReleased\(|liveObjectsPlugin|RealtimeObject' {}
nl -ba liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultRealtimeObject.kt | sed -n '315,345p'
nl -ba lib/src/main/java/io/ably/lib/realtime/AblyRealtime.java | sed -n '208,270p'

Length of output: 26072

---

@sacOO7 The race is valid. I checked channel construction, the plugin registry, and object disposal. One detail needs correction: the replacement channel normally reuses the old DefaultRealtimeObject, rather than creating a new object.

How the race occurs

Assume channel C1 owns LiveObjects instance O1.

Step	Thread A: release(name)	Thread B: get(name)
1	Removes C1 from the channel map.
2		Constructs replacement channel C2.
3		The constructor calls liveObjectsPlugin.getInstance(name). The plugin still contains O1, so C2.object receives O1.
4		Inserts C2 into the channel map and returns it.
5	Calls dispose(name), which removes and disposes O1.

The code supports this sequence:

• AblyRealtime.java, Line 249–252: channel removal and plugin disposal are separate operations.
• lib/src/main/java/io/ably/lib/realtime/ChannelBase.java, Line 1685: channel construction retrieves the LiveObjects instance by name.
• liveobjects/src/main/kotlin/io/ably/lib/liveobjects/DefaultLiveObjectsPlugin.kt, Line 12–14: getInstance() uses computeIfAbsent, so it returns the existing instance.
• The same plugin file, Line 26–28: dispose(name) removes whichever instance is registered under that name.

Disposal cancels the incoming-object handler, disposes the object pool and manager, and removes path subscriptions. C2.object still references that disposed instance. This breaks the replacement channel’s LiveObjects functionality; it does not establish that ordinary channel messaging is unusable.

Both registries use ConcurrentHashMap. That protects individual map operations, not the lifecycle across both maps.

What a fix must cover

The simplest approach is to serialize get(name, options) and release(name) with the same lifecycle lock.

• In get(), acquire the lock before constructing Channel. The constructor already accesses the plugin registry.
• In release(), hold the lock through channel removal, markAsReleased(), and plugin disposal.
• Prefer checking for an existing channel before constructing a new channel.

Locking only release(), or only putIfAbsent(), does not prevent this race.

There is also a second race: release() ignores the result of map.remove(name, channel). Two concurrent release calls can capture C1. After the first call removes C1, a replacement can be created. The second call can then fail to remove C1 but still dispose the replacement’s LiveObjects instance. Cleanup should therefore depend on successful removal.

An ownership-based design is another option. However, conditional removal by object identity alone is insufficient here, because C2 can already share O1. That design must also prevent a replacement channel from acquiring the releasing channel’s LiveObjects instance.

A deterministic regression test should pause release after channel-map removal, start a concurrent get(), and check that the replacement retains a live, registered LiveObjects instance after release completes. A second test should cover two concurrent release calls.

This validation is based on source inspection. I did not run tests.

You are interacting with an AI system.

[sacOO7]

Seems, there are race conditions we need to take care of as a part of the fix

[SimonWoolf]

@sacOO7 done the check/remove/markAsReleased, that now happen under the channel's monitor. The dispose-by-name race was not introduced by this PR and is independent of it, and looks like the fix needs a lifecycle lock spanning get() plus a plugin API change, so not gonna pick it up in this PR, I'll leave it to you as someone more familiar with the sdk.