Fix RTC10 violation: channel.off() could break attach and detach

[lawrence-forooghian]

The attach() and detach() methods used this.once() to listen for state changes on the public EventEmitter. A user calling channel.off() to remove their own listeners would also remove these internal listeners, causing the attach/detach promise to never resolve. This violates RTC10.

Add a dedicated _internalStateChanges EventEmitter that is not affected by public off() calls, and use it for attach and detach. The existing _allChannelChanges emitter would work here but is not used because its contract is specifically designed for setOptions (it unconditionally emits update for all ATTACHEDs, even when the public emitter suppresses the event) and reusing it would couple these consumers to semantics that could change independently.

Summary by CodeRabbit

• Bug Fixes

  • Improved channel attach/detach reliability and state transition handling so operations complete correctly even when listeners are removed, while preserving compatibility with external listeners.

• Tests

  • Added regression tests ensuring attach and detach resolve properly when listeners are removed mid-operation.

[coderabbitai]

Walkthrough

RealtimeChannel now separates internal state-change notifications from public listeners by introducing _attachedReceived and internalStateChanges emitters and overriding emit(...). Tests added to ensure attach() and detach() complete even if external listeners are removed during the operations.

Changes

Cohort / File(s)	Summary
Realtime channel internals src/common/lib/client/realtimechannel.ts	Removed public _allChannelChanges emitter; added _attachedReceived and internalStateChanges emitters; added emit(event: string, ...args: unknown[]) override to forward to public listeners and internalStateChanges; replaced internal usages of _allChannelChanges with internalStateChanges and emit _attachedReceived.emit('attached') when ATTACHED is processed.
Realtime channel tests test/realtime/channel.test.js	Updated tests to listen to listen.channel._attachedReceived.attached instead of _allChannelChanges variants; added regression tests verifying attach() and detach() resolve when channel.off() is called during attach/detach.
Private API recorder test/common/modules/private_api_recorder.js	Replaced private API identifiers: removed references to listen.channel._allChannelChanges.attached and .update, added listen.channel._attachedReceived.attached.
Manifest package.json	Minor changes recorded (manifest updated alongside tests/changes).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 I nibble at code with a twitch and a grin,
Internal bells ring quietly within.
Off() may wander and listeners depart,
Attach and detach finish, a steadfast heart.
Hooray for events that keep work apart!

🚥 Pre-merge checks | ✅ 3

✅ 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 accurately summarizes the main fix: addressing an RTC10 violation where channel.off() could break attach and detach operations by removing internal event listeners.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch fix-rtc10-channel-off

---

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

[VeskeR]

I was actually wondering if this is the case given we use public EventEmitter.once that is exposed to the user. Thanks for addressing this.

Also related, we should ideally just remove "unsubscribe all" behavior in the future v3 release. We did the same for chat, and for liveobjects (see https://ably.atlassian.net/wiki/spaces/LOB/pages/4690411539/LODR-056+SDK+remove+offAll+unsubscribeAll+from+LiveObjects+API)

[lawrence-forooghian]

Also related, we should ideally just remove "unsubscribe all" behavior in the future v3 release

agreed

[coderabbitai]

🧹 Nitpick comments (1)

src/common/lib/client/realtimechannel.ts (1)

91-98: Consider marking _internalStateChanges as private to enforce its isolation contract.

The JSDoc on lines 91–97 explicitly states this emitter must not be affected by public off() calls. Declaring it without an access modifier leaves it public, meaning any consumer could call channel._internalStateChanges.off() and silently re-introduce the exact RTC10 regression this PR fixes. _allChannelChanges shares this exposure, but its accidental .off() is far less critical than breaking attach/detach resolution.

♻️ Proposed change

-  _internalStateChanges: EventEmitter;
+  private _internalStateChanges: EventEmitter;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/common/lib/client/realtimechannel.ts` around lines 91 - 98, The
_internalStateChanges EventEmitter is currently public which allows external
callers to call .off() and break attach/detach behavior; change the declaration
of _internalStateChanges to be private (e.g., private _internalStateChanges:
EventEmitter) and update all internal references in the RealtimeChannel class to
use the private field; also consider making _allChannelChanges private as well
or providing controlled accessors if external consumers need to observe
events—update any tests or usages that referenced these fields directly to use
the new accessor or public APIs instead.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/common/lib/client/realtimechannel.ts`:
- Around line 91-98: The _internalStateChanges EventEmitter is currently public
which allows external callers to call .off() and break attach/detach behavior;
change the declaration of _internalStateChanges to be private (e.g., private
_internalStateChanges: EventEmitter) and update all internal references in the
RealtimeChannel class to use the private field; also consider making
_allChannelChanges private as well or providing controlled accessors if external
consumers need to observe events—update any tests or usages that referenced
these fields directly to use the new accessor or public APIs instead.

[VeskeR]

Need to also emit on internal emitter here:

ably-js/src/common/lib/client/realtimechannel.ts

Lines 572 to 575 in db6df43

	this._allChannelChanges.emit('update', change);
	if (!resumed || this.channelOptions.updateOnAttached) {
	this.emit('update', change);
	}

and here:

ably-js/src/common/lib/client/realtimepresence.ts

Line 389 in db6df43

this.channel.emit('update', change);

The _ensureMyMembersPresent is also currently missing this._allChannelChanges.emit call, should fix that too.
At this point we should just have a dedicated method on a RealtimeChannel to emit on all underlying emitters, so we don't forget any in the future

[lawrence-forooghian]

At this point we should just have a dedicated method on a RealtimeChannel to emit on all underlying emitters, so we don't forget any in the future

Thanks — I've added an override of emit() on the channel

[VeskeR]

Approving, but have a comment regarding property name.

Also, what do you think about adding this._allChannelChanges.emit to the RealtimePresence._ensureMyMembersPresent call here:

ably-js/src/common/lib/client/realtimepresence.ts

Line 389 in db6df43

this.channel.emit('update', change);

?
It seems like there is no reason not to emit an event there

[lawrence-forooghian]

Also, what do you think about adding this._allChannelChanges.emit to theRealtimePresence._ensureMyMembersPresent call here

I think it's unnecessary given what _allChannelChanges is really trying to achieve. I don't really think it's worth trying to keep _allChannelChanges in sync with all channel events. I've proposed a different approach in 3d1535a.

[VeskeR]

I think it's unnecessary given what _allChannelChanges is really trying to achieve. I don't really think it's worth trying to keep _allChannelChanges in sync with all channel events. I've proposed a different approach in 3d1535a.

I really like this approach. _attachedReceived is much clearer, and it's now obvious that it serves the purpose of enabling RTL16a. Thank you

[VeskeR]

Tests seem to be failing due to something not related to this PR, raised in https://ably-real-time.slack.com/archives/C09SY1AQGK0/p1771517162529679

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (1)

src/common/lib/client/realtimechannel.ts (1)

94-102: Naming inconsistency: internalStateChanges vs _attachedReceived

_attachedReceived uses the underscore prefix convention for internal members, but internalStateChanges does not — and the PR description referred to it as _internalStateChanges. Since internalStateChanges is only used within RealtimeChannel itself and never accessed from tests, consider either:

• Renaming to _internalStateChanges for consistency, or
• Marking it private (it doesn't need to be public)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/common/lib/client/realtimechannel.ts` around lines 94 - 102, Rename or
make internalStateChanges consistent with private/internal naming: either rename
the public field internalStateChanges to _internalStateChanges everywhere it's
referenced in RealtimeChannel (including its declaration and all uses) or change
its declaration to a private field (private internalStateChanges or private
_internalStateChanges as you prefer), and update any constructors or methods
that reference internalStateChanges or _attachedReceived to use the new
identifier so the class's internal event emitter follows the underscore/private
convention.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/common/lib/client/realtimechannel.ts`:
- Around line 142-143: The override of emit() references
this.internalStateChanges but internalStateChanges (and _attachedReceived) are
only initialized after calling this.setOptions(options) and after plugin
constructors receive this, so a plugin or setOptions path that calls this.emit()
can hit undefined; to fix, move the initialization of this._attachedReceived =
new EventEmitter(this.logger) and this.internalStateChanges = new
EventEmitter(this.logger) to run before calling this.setOptions(options) and
before invoking plugin constructors in the constructor, ensuring emit() always
sees a initialized internalStateChanges; update the constructor so the
EventEmitter initializations occur immediately after setting this.logger (or at
start of constructor) and before any calls to setOptions or plugin
instantiation.
- Around line 209-224: The setOptions promise can hang because the failure
listener registered in setOptions only listens for ['detached','failed'] and
misses the 'suspended' transition triggered by timeoutPendingState; update the
failure subscription in setOptions to include 'suspended' (i.e., call
this.internalStateChanges.once(['detached','failed','suspended'], onFailure)) so
onFailure runs, cleanup (this._attachedReceived.off/on removal) happens and the
promise rejects instead of stalling; keep using the existing onAttached,
onFailure, cleanup helpers as defined around _attachedReceived and
internalStateChanges.

---

Nitpick comments:
In `@src/common/lib/client/realtimechannel.ts`:
- Around line 94-102: Rename or make internalStateChanges consistent with
private/internal naming: either rename the public field internalStateChanges to
_internalStateChanges everywhere it's referenced in RealtimeChannel (including
its declaration and all uses) or change its declaration to a private field
(private internalStateChanges or private _internalStateChanges as you prefer),
and update any constructors or methods that reference internalStateChanges or
_attachedReceived to use the new identifier so the class's internal event
emitter follows the underscore/private convention.