Skip to content

Implicit attach on channel.objects.get() call#472

Merged
sacOO7 merged 1 commit into
mainfrom
AIT-119/implicit-attach-getroot
Jun 3, 2026
Merged

Implicit attach on channel.objects.get() call#472
sacOO7 merged 1 commit into
mainfrom
AIT-119/implicit-attach-getroot

Conversation

@lawrence-forooghian
Copy link
Copy Markdown
Collaborator

As already implemented in JS in ably/ably-js@9bde15.

Split out of #427 since it's a standalone behaviour.

@sacOO7
Copy link
Copy Markdown
Collaborator

sacOO7 commented May 18, 2026

Review against ably-js implementation

I've cross-checked this PR against ably-js realtimechannel.ts:1135-1150 (ensureAttached) and realtimeobject.ts:88-101 (get()). The new RTO1e/f line up with the implementation for the four states they enumerate, but there are a couple of gaps worth tightening before merge.

🟠 SUSPENDED state behaviour not addressed

The new RTO1e/f cover INITIALIZED, DETACHED, DETACHING, ATTACHING, and FAILED, but say nothing about SUSPENDED. The implementation's ensureAttached (source) explicitly passes SUSPENDED through without re-attaching:

async ensureAttached(): Promise<void> {
  switch (this.state) {
    case 'attached':
    case 'suspended':
      break;
    case 'initialized':
    case 'detached':
    case 'detaching':
    case 'attaching':
      await this.attach();
      break;
    case 'failed':
    default:
      throw ErrorInfo.fromValues(this.invalidStateError());
  }
}

That's deliberate — a SUSPENDED channel may have local object data that the user can still read while the connection is being re-established. The current RTO1 wording is ambiguous about this case (a reader might assume SUSPENDED falls into "or any other non-attached state" and so gets re-attached, which is wrong).

Suggest adding:

- (RTO1g) If the channel is in the `ATTACHED` or `SUSPENDED` state, proceed without attaching.

…and renumbering / making the relationship between RTO1e/f/g explicit. The simplest framing matches the impl's switch statement:

State Behaviour
ATTACHED, SUSPENDED Pass through; do not re-attach.
INITIALIZED, DETACHED, DETACHING, ATTACHING Implicit attach and wait.
FAILED Throw ErrorInfo 400/90001.

🟠 Behaviour during pending implicit-attach if the channel transitions to FAILED

RTO1e says "implicitly attach the RealtimeChannel and wait for the attach to complete" but doesn't define what happens if the attach attempt itself fails, or if the channel transitions to FAILED during the wait. The impl simply propagates the attach() rejection (realtimechannel.ts:1144 → standard attach() failure semantics).

Worth being explicit:

- (RTO1e1) If the implicit attach fails (e.g. the channel transitions to `FAILED` during the wait), the returned promise MUST reject with an `ErrorInfo` 400/90001.

🟡 RFC 2119 keyword precision

(RTO1f) says "the library should throw" — should this be MUST? The pre-existing (RTO1b) used the same "should" wording, but per CLAUDE.md's writing principles ("Use RFC 2119 keywords precisely"), throwing for a FAILED channel is a hard requirement. Suggest "MUST throw".

⚪ Minor: cross-reference to RTL27 lifecycle

For readers landing on this clause without context, a one-line cross-reference to channel-state transitions (RTL2) and the implicit-attach pattern used elsewhere (e.g. RTP11b for presence.get(), mentioned in the PR description) would help with consistency framing.


These observations correspond to issue B-3 in LIVEOBJECTS_SPEC_REVIEW.md (cross-branch review) and Task B-6 in LIVEOBJECTS_UPDATE_SPEC_ACTIONABLE_PLAN.md.

@github-actions github-actions Bot temporarily deployed to staging/pull/472 May 19, 2026 10:13 Inactive
@sacOO7 sacOO7 force-pushed the AIT-119/implicit-attach-getroot branch from 07e89c9 to 969aade Compare May 19, 2026 12:11
@github-actions github-actions Bot temporarily deployed to staging/pull/472 May 19, 2026 12:12 Inactive
Copy link
Copy Markdown
Collaborator

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

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

LGTM.
Addressed changes as per review comment and now behaviour is exactly similar to JS code.

@sacOO7 sacOO7 force-pushed the AIT-119/implicit-attach-getroot branch from 969aade to b5d9a12 Compare May 19, 2026 12:53
@github-actions github-actions Bot temporarily deployed to staging/pull/472 May 19, 2026 12:53 Inactive
@sacOO7 sacOO7 force-pushed the AIT-119/implicit-attach-getroot branch from b5d9a12 to 2c9b91b Compare May 19, 2026 13:07
@github-actions github-actions Bot temporarily deployed to staging/pull/472 May 19, 2026 13:07 Inactive
Comment thread specifications/objects-features.md Outdated
Comment thread specifications/objects-features.md Outdated
Comment thread specifications/objects-features.md Outdated
@github-actions github-actions Bot temporarily deployed to staging/pull/472 May 20, 2026 19:28 Inactive
@sacOO7 sacOO7 force-pushed the AIT-119/implicit-attach-getroot branch from b0638be to 0d680ca Compare May 20, 2026 19:39
@github-actions github-actions Bot temporarily deployed to staging/pull/472 May 20, 2026 19:39 Inactive
@sacOO7 sacOO7 force-pushed the AIT-119/implicit-attach-getroot branch from 0d680ca to 582f2e2 Compare May 20, 2026 19:45
@github-actions github-actions Bot temporarily deployed to staging/pull/472 May 20, 2026 19:45 Inactive
@github-actions github-actions Bot temporarily deployed to staging/pull/472 May 28, 2026 11:20 Inactive
@sacOO7 sacOO7 force-pushed the AIT-119/implicit-attach-getroot branch from df46e4e to d166566 Compare May 28, 2026 11:22
@github-actions github-actions Bot temporarily deployed to staging/pull/472 May 28, 2026 11:23 Inactive
@sacOO7 sacOO7 changed the title Implicit attach on channel.objects.getRoot() call Implicit attach on channel.objects.get() call May 28, 2026
Comment thread specifications/features.md Outdated
- `(RTL32d)` On success, returns an `UpdateDeleteResult` object containing the version serial of the published update, obtained from the first element of the `serials` array of the [TR4s](#TR4s) `res` field of the `ACK`. Indicates an error if the operation was not successful.
- `(RTL33)` *Ensure-active-channel* internal procedure. When invoked, the SDK MUST inspect the current state of the `RealtimeChannel` (see [RTL2](#RTL2)) and proceed as follows:
- `(RTL33a)` If the channel is in the `ATTACHED` or `SUSPENDED` state, the procedure completes immediately without performing any attach.
- `(RTL33b)` If the channel is in the `INITIALIZED`, `DETACHED`, `DETACHING`, or `ATTACHING` state, perform an implicit attach per [RTL4](#RTL4) and wait for it to complete
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This is a behavioural change compared to RTP11b i.e. a DETACHED channel now gets implicit attach, instead of throwing an error. Our docstrings aren't specific about the circumstances in which an implicit attach gets performed so I think we can get away with it, and I don't see any obvious reason why we wouldn't want to do it, but I thought it worth pointing out.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This behaviour follows current ably-js implementation -> #472 (comment)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This behaviour follows current ably-js implementation

Sure, I get that; I'm saying that the ably-js implementation is different to the spec and it's not obvious that the ably-js behaviour is the one that we want (but that it's minor)

Comment thread specifications/features.md Outdated
Comment thread specifications/objects-features.md Outdated
Comment thread specifications/features.md Outdated
@github-actions github-actions Bot temporarily deployed to staging/pull/472 June 3, 2026 09:54 Inactive
@sacOO7 sacOO7 force-pushed the AIT-119/implicit-attach-getroot branch from e305f50 to c5627f8 Compare June 3, 2026 10:03
@github-actions github-actions Bot temporarily deployed to staging/pull/472 June 3, 2026 10:04 Inactive
@lawrence-forooghian
Copy link
Copy Markdown
Collaborator Author

LGTM, I can't approve because it's my PR, but approved

Adds a shared *ensure-active-channel* procedure (RTL33) that consolidates
implicit-attach behaviour, and references it from `RealtimeObject#get`
(RTO23e) and `RealtimePresence#get` (RTP11e) to avoid spec duplication.

As already implemented in JS in 9bde15e.

Co-Authored-By: Andrew Bulat <andrii.bulat@gmail.com>
Co-Authored-By: Lawrence Forooghian <lawrence.forooghian@ably.com>
@sacOO7 sacOO7 force-pushed the AIT-119/implicit-attach-getroot branch from c5627f8 to cd5fe10 Compare June 3, 2026 12:34
@sacOO7 sacOO7 merged commit 99ded6b into main Jun 3, 2026
2 checks passed
@sacOO7 sacOO7 deleted the AIT-119/implicit-attach-getroot branch June 3, 2026 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

live-objects Related to LiveObjects functionality.

Development

Successfully merging this pull request may close these issues.

2 participants