Skip to content

[AIT-30] BatchContext and batch API on PathObject and Instance#471

Open
lawrence-forooghian wants to merge 1 commit into
AIT-30/liveobjects-path-based-api-specfrom
AIT-30/liveobjects-batch-api
Open

[AIT-30] BatchContext and batch API on PathObject and Instance#471
lawrence-forooghian wants to merge 1 commit into
AIT-30/liveobjects-path-based-api-specfrom
AIT-30/liveobjects-batch-api

Conversation

@lawrence-forooghian
Copy link
Copy Markdown
Collaborator

Note: This PR is based on #427.

Split from #427 to reduce the review burden of that PR (since we don't need to do the batch API for the initial implementation of the path-based API).

@lawrence-forooghian lawrence-forooghian added the live-objects Related to LiveObjects functionality. label May 12, 2026
@lawrence-forooghian lawrence-forooghian changed the title BatchContext and batch API on PathObject and Instance [AIT-30] BatchContext and batch API on PathObject and Instance May 12, 2026
@lawrence-forooghian lawrence-forooghian force-pushed the AIT-30/liveobjects-path-based-api-spec branch 2 times, most recently from 035aef9 to babc296 Compare May 12, 2026 20:09
@sacOO7 sacOO7 requested a review from Copilot May 13, 2026 10:44
- `(RTPO21b)` Returns a stream or iterable that yields `PathObjectSubscriptionEvent` objects, using the idiomatic construct for the language (e.g. async iterators, channels, flows, or async sequences)
- `(RTPO21c)` Internally wraps `PathObject#subscribe` ([RTPO19](#RTPO19)), converting the callback-based subscription into the appropriate streaming or iterable pattern
- `(RTPO22)` `PathObject#batch` function:
- `(RTPO22a)` Expects a synchronous function `fn` that receives a `BatchContext` as its argument
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.

Suggested change
- `(RTPO22a)` Expects a synchronous function `fn` that receives a `BatchContext` as its argument
- `(RTPO22a)` Accepts a synchronous anonymous function as a argument. This anonymous function is called with `BatchContext` as its argument internally.

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.

https://ably.com/docs/liveobjects/concepts/path-object#batch-multiple-updates
Similarly, other spec points might not be explicit, will do a full review soon

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR extends the LiveObjects path-based API specification by introducing a batching mechanism for object mutations, enabling multiple map/counter operations to be queued and then published atomically as a single channel message.

Changes:

  • Adds PathObject#batch and Instance#batch spec points, describing lifecycle and permission requirements.
  • Introduces the BatchContext / internal RootBatchContext specification for queuing and flushing batched operations.
  • Reserves the new RTBC spec point prefix in the main features index.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.

File Description
specifications/objects-features.md Specifies batch APIs for PathObject/Instance and defines BatchContext + internal batching flush mechanics.
specifications/features.md Updates reserved spec point prefixes to include RTBC.
Comments suppressed due to low confidence (1)

specifications/objects-features.md:1004

  • RTINS19 similarly specifies flush/close only after fn returns. Please define behavior when fn throws (and, where applicable, when an async function is passed): queued operations should not be flushed/published, but the RootBatchContext should still be closed so the BatchContext and any children become unusable and consistently throw the “batch is closed” error.
  - `(RTINS19d)` Creates a `RootBatchContext` ([RTBC16](#RTBC16)) wrapping this `Instance`
  - `(RTINS19e)` Executes `fn`, passing the `BatchContext` as argument
  - `(RTINS19f)` After `fn` returns, flushes the `RootBatchContext` ([RTBC16d](#RTBC16d)) to publish all queued operations atomically
  - `(RTINS19g)` The `RootBatchContext` is closed after flush completes, regardless of success or failure

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- `(RTPO22)` `PathObject#batch` function:
- `(RTPO22a)` Expects a synchronous function `fn` that receives a `BatchContext` as its argument
- `(RTPO22b)` Requires the `OBJECT_PUBLISH` channel mode to be granted per [RTO2](#RTO2)
- `(RTPO22c)` Resolves the path to a `LiveObject` using the internal path resolution procedure. If the path does not resolve to a `LiveObject`, the library must throw an `ErrorInfo` error with `statusCode` 400 and `code` 92007
Comment on lines +921 to +922
- `(RTPO22f)` After `fn` returns, flushes the `RootBatchContext` ([RTBC16d](#RTBC16d)) to publish all queued operations atomically
- `(RTPO22g)` The `RootBatchContext` is closed after flush completes, regardless of success or failure
- `(RTBC4)` `BatchContext#get` function:
- `(RTBC4a)` Expects a `key` `String` argument
- `(RTBC4b)` If the batch is closed, the library must throw an `ErrorInfo` error with `statusCode` 400 and `code` 40000
- `(RTBC4c)` Delegates to `Instance#get` ([RTINS5](#RTINS5)) on the underlying `Instance`. If the result is undefined, returns undefined
- `(RTBC14a1)` `amount` `Number` (optional) - the amount by which to increment the counter value. Defaults to 1
- `(RTBC14b)` If the batch is closed, the library must throw an `ErrorInfo` error with `statusCode` 400 and `code` 40000
- `(RTBC14c)` If the wrapped value is not a `LiveCounter`, the library must throw an `ErrorInfo` error with `statusCode` 400 and `code` 92007
- `(RTBC14d)` Queues a message constructor on the `RootBatchContext` that, when executed, creates an `ObjectMessage` for a `COUNTER_INC` operation in the same manner as `LiveCounter#increment` ([RTLC12](#RTLC12))
- `(RTBC16a)` Maintains an internal `wrappedInstances` map that memoizes `BatchContext` wrappers by `objectId`
- `(RTBC16b)` Maintains an internal `queuedMessageConstructors` list of deferred message constructor functions. Some `ObjectMessages` require asynchronous I/O during construction (e.g. generating an `objectId` for nested value types), so message constructors are queued during synchronous batch method calls and executed on flush
- `(RTBC16c)` `wrapInstance` function: wraps an `Instance` in a `BatchContext`. If the `Instance` has an `objectId` and a wrapper for that `objectId` already exists in `wrappedInstances`, the existing wrapper is returned. Otherwise, a new `BatchContext` is created and stored in `wrappedInstances`
- `(RTBC16d)` `flush` function: closes the batch context, executes all queued message constructors, flattens the resulting `ObjectMessages` into a single array, and publishes them using `RealtimeObject#publish` ([RTO15](#RTO15)). If there are no queued messages, no publish is performed
- `(RTBC15a1)` `amount` `Number` (optional) - the amount by which to decrement the counter value. Defaults to 1
- `(RTBC15b)` If the batch is closed, the library must throw an `ErrorInfo` error with `statusCode` 400 and `code` 40000
- `(RTBC15c)` If the wrapped value is not a `LiveCounter`, the library must throw an `ErrorInfo` error with `statusCode` 400 and `code` 92007
- `(RTBC15d)` Delegates to `BatchContext#increment` ([RTBC14](#RTBC14)) with the negated `amount`
@sacOO7
Copy link
Copy Markdown
Collaborator

sacOO7 commented May 18, 2026

Review against ably-js implementation

I've cross-checked this PR against the ably-js implementation at src/plugins/liveobjects/ (commit 3deeee8e, branch main). Most of the BatchContext semantics line up cleanly, but there are several gaps that I think need addressing before merge.

🔴 Critical: RTBC16d uses RealtimeObject#publish, but the implementation uses publishAndApply

(RTBC16d) states:

flush function: closes the batch context, executes all queued message constructors, flattens the resulting ObjectMessages into a single array, and publishes them using RealtimeObject#publish (RTO15).

The implementation calls publishAndApply (RTO20), not publish (rootbatchcontext.ts:27-40):

async flush(): Promise<void> {
  try {
    this.close();
    const msgs = (await Promise.all(this._queuedMessageConstructors.map((x) => x()))).flat();
    if (msgs.length > 0) {
      await this._realtimeObject.publishAndApply(msgs);   // ← RTO20, not RTO15
    }
  } finally {
    this._wrappedInstances.clear();
    this._queuedMessageConstructors = [];
  }
}

This is a meaningful semantic difference. publishAndApply waits for the server ACK, then locally applies the operations to the ObjectsPool (synthesizing site/serial info per RTO20d). Plain publish does not — local state would not reflect the batched mutations until the channel echoes them back.

The promise returned by batch(fn) in ably-js resolves only after the local apply completes, which is the behaviour users expect from "batched operations resolve after they are applied locally". RTBC16d should reference RTO20 instead.

🔴 RTPO22b / RTINS19b are missing channel-state and echoMessages checks

Both clauses only require the OBJECT_PUBLISH channel mode. The implementation does the full triple-check via throwIfInvalidWriteApiConfiguration:

throwIfInvalidWriteApiConfiguration(): void {
  this._throwIfMissingChannelMode('object_publish');
  this._throwIfInChannelState(['detached', 'failed', 'suspended']);
  this._throwIfEchoMessagesDisabled();
}

Called first in pathobject.ts:325 and pathobject.ts:419. So the spec is under-specified relative to impl — without the channel-state check the SDK could enqueue operations on a DETACHED channel.

Suggest tightening RTPO22b / RTINS19b to:

The wrapper MUST perform the channel-mode (OBJECT_PUBLISH) check per RTO2, the channel-state check of RTLM20c, and the echoMessages check of RTLM20d before any further work.

🟠 RTPO22c does not handle path-resolution failure

Resolves the path to a LiveObject using the internal path resolution procedure. If the path does not resolve to a LiveObject, the library must throw an ErrorInfo error with statusCode 400 and code 92007.

The current spec on main already defines RTPO3 (path resolution) and RTPO3c2 (write-side resolution failure → 92005). The implementation matches that pattern (pathobject.ts:439-467 _resolvePath throws 92005, pathobject.ts:418-437 batch propagates that error). RTPO22c collapses two distinct failure modes (path-not-resolved vs. resolved-to-primitive) into a single 92007 throw, which is inconsistent with how RTPO15/RTPO16/RTPO17/RTPO18 handle the same situation.

Suggest splitting:

- (RTPO22c) Resolves the path using the path resolution procedure ([RTPO3](#RTPO3)). On resolution failure, the library MUST throw per [RTPO3c2](#RTPO3c2) (code 92005).
- (RTPO22c1) If the resolved value is not a LiveObject (i.e. a primitive), the library MUST throw an ErrorInfo error with statusCode 400 and code 92007.

🟠 Raw LiveMap / LiveCounter references silently accepted by BatchContext#set

(RTBC12a2) lists the accepted types as:

Boolean | Binary | Number | String | JsonArray | JsonObject | LiveCounterValueType | LiveMapValueType

But the impl reuses LiveMap.validateKeyValue (livemap.ts:173-189) which accepts anything that is typeof === 'object'. Because real LiveMap and LiveCounter instances have only a TypeScript-only brand symbol (no runtime _livetype property — see livemap.ts:58 and livecounter.ts:18), LiveMapValueType.instanceof(realLiveMap) and LiveCounterValueType.instanceof(realLiveCounter) both return false. They fall through to primitiveToObjectData which writes them as { json: liveMapInstance }, producing garbage on the wire.

This is the same gap that exists in RTLM20 and the same fix can be applied here: explicitly reject raw live-object references with ErrorInfo 400/40013. Either add a normative sub-clause to RTBC12 or have RTBC12d link to the (also-needed) corresponding clause on RTLM20.

🟡 Read semantics during an open batch not specified

(RTBC4)(RTBC11) are stated as delegating to the corresponding Instance methods, but the spec does not say what state these reads observe. The impl does not mutate the underlying LiveObject data as the user enqueues writes — reads always see the pre-batch snapshot (batchcontext.ts:33-88 and rootbatchcontext.ts — the queued constructors run only on flush()).

Suggest a non-normative note:

Read methods on a BatchContext MUST NOT reflect operations queued in the current batch — they return the underlying LiveObject snapshot at call time. The queued mutations become visible only after the batch is published and applied locally via RTO20.

🟡 Operations on outer objects from within a batch function

The spec doesn't say what happens if the user does this:

await path.batch(async (ctx) => {
  ctx.set('a', 1);             // batched
  await someOtherInstance.set('b', 2);  // NOT batched — separate publish
});

The impl's behaviour: only writes against the BatchContext argument are batched. Worth a one-liner clarification.

🟡 RTPO22g / RTINS19g ordering

(RTPO22g)/(RTINS19g) say:

The RootBatchContext is closed after flush completes, regardless of success or failure

The impl actually closes the context inside flush(), before the queued constructors are even invoked. The outer try…finally in pathobject.ts:432-436 calls close() a second time as a safety net.

So the user-visible guarantee is "context is closed by the time batch()'s promise settles" — which is consistent with the spec text. But "after flush completes" suggests the close happens later than it does. Minor wording suggestion:

The RootBatchContext MUST be closed before the message constructors are executed, and remains closed regardless of whether publish-and-apply succeeds or fails. Any further method calls on the context (or memoized children) MUST throw ErrorInfo 400/40000.

🟡 RTBC3a returns objectId even for primitive instances

(RTBC3a) says:

Returns the objectId of the underlying Instance (RTINS3)

But Instance#id returns undefined when the wrapped value is a primitive (instance.ts:37-43). Recommend adding a (RTBC3b) (or amending 3a) to mirror RTINS3a/RTINS3b — return undefined for primitives.

⚪ Editorial

  • IDL block: please confirm batch(((BatchContext) ->) fn) => io is the intended syntax; the existing IDL uses the pattern subscribe((PathObjectSubscriptionEvent) -> listener, …) -> Subscription. Consistency check.
  • (RTBC1a) cross-refs RTINS1a; consider also linking RTPO1a to make it clear the same type-narrowing pattern applies on the navigational side too.

These findings are documented in the cross-branch review at LIVEOBJECTS_SPEC_REVIEW.md.

@sacOO7
Copy link
Copy Markdown
Collaborator

sacOO7 commented May 18, 2026

Follow-up: additional gaps surfaced after deeper cross-check

Following the initial review above, a closer pass over batchcontext.ts, rootbatchcontext.ts and pathobject.ts:418-437 (ably-js 3deeee8e) turned up five additional items the original comment didn't cover. Posting as a follow-up to preserve review history.

🟠 Per-method write-API checks on BatchContext.set / remove / increment / decrement

RTBC12d/13d/14d/15d say "queues a message constructor … in the same manner as LiveMap#set (RTLM20e)" — but RTLM20e is the construction procedure (createMapSetMessage), not the wrapper checks (RTLM20b/c/d). The impl runs the triple-check at call time on every write, before enqueuing:

Crucially, createMapSetMessage at livemap.ts:94-143 does not repeat the channel-mode/state/echo checks — so skipping the check at call time would NOT be caught at flush either.

Suggest adding a sub-clause to each of RTBC12/13/14/15 (or a shared RTBC0a referenced by all four):

Before queueing the deferred constructor, the method MUST perform the OBJECT_PUBLISH channel-mode check (RTO2), the channel-state check of RTLM20c, and the echoMessages check of RTLM20d. If any of these checks fail, the library MUST throw before queueing.

🟠 Synchronous-throw discard semantics for PathObject#batch / Instance#batch

Complementary to the RTPO22g/RTINS19g ordering note in the previous comment — that note covers the close-vs-flush ordering on the happy path; this section covers the orthogonal case where flush is never reached.

RTPO22f/g and RTINS19f/g describe the happy path only. What happens if the user-provided fn throws synchronously is unspecified.

Impl at pathobject.ts:431-436 (mirror at instance.ts:247-252):

try {
  fn(ctx as unknown as BatchContext<T>);
  await ctx.flush();
} finally {
  ctx.close();
}

If fn throws: flush() is skipped → no publish → queued constructors discarded; close() runs in finally → context closed; error propagates.

Suggest adding to both RTPO22 and RTINS19:

If the user-provided fn throws synchronously, the library MUST close the RootBatchContext (per RTBC16e) and MUST NOT attempt to flush the queued message constructors. The thrown error MUST propagate to the caller of batch() without any channel message being published.

Without this clause an implementer could reasonably interpret "closed after flush completes" (RTPO22g) as "always flush, regardless of throw", publishing a partial batch on user error.

🟡 Non-normative ordering of operations in the flushed array

RTBC16d says "flattens the resulting ObjectMessages into a single array" but is silent on the order. The on-the-wire order other clients observe is deterministic in impl:

  • rootbatchcontext.ts:30-31: (await Promise.all(_queuedMessageConstructors.map(x => x()))).flat()Promise.all preserves input order; _queuedMessageConstructors is appended in call order (line 71).
  • Within a single set that creates a nested value-type, livemap.ts:142 returns [...createValueTypesMessages, mapSetMsg] — CREATEs precede the parent MAP_SET per RTLM20h1.

Suggest a non-normative paragraph on RTBC16d:

Operations in the flushed array MUST appear in the order their corresponding write method was first invoked on the batch. Within a single set that creates a nested value-type, the value-type CREATE messages MUST precede the parent MAP_SET per RTLM20h1; deeply nested value-types are flattened depth-first per RTLMV4k.

Cross-SDK implementers need a deterministic spec; otherwise different SDKs could legitimately produce different on-wire orderings.

🟡 Read-method access checks (editorial)

RTBC4RTBC11 say "Delegates to Instance#…" and add a closed-throw, but never mention the OBJECT_SUBSCRIBE mode/state check. The impl performs it explicitly at each call (batchcontext.ts:34, 44, 50, 56, 62, 71, 77, 85) and also at the underlying Instance#… (instance.ts:53, 69, 85, 104, 136, 150, 167).

The access check at realtimeobject.ts:387-390:

throwIfInvalidAccessApiConfiguration(): void {
  this._throwIfMissingChannelMode('object_subscribe');
  this._throwIfInChannelState(['detached', 'failed']);
  // note: SUSPENDED is allowed for reads — local snapshot is still valid
}

Recommend either (a) making the access check explicit on RTBC4RTBC11, or (b) tightening the wording on RTINS5/6/7/8/9/10/11 to say the access check is part of the delegated procedure and having BatchContext reference it. Minor either way.

closed state location

RTBC2 lists instance and rootContext as BatchContext internal state; RTBC16 lists wrappedInstances and queuedMessageConstructors as RootBatchContext state. The spec is silent on where the closed flag lives or how a child BatchContext determines its parent is closed.

Impl: _isClosed lives only on RootBatchContext (rootbatchcontext.ts:17,43-50). Each DefaultBatchContext reads via this._rootContext.isClosed() (batchcontext.ts:132-136).

Suggest one-line addition to RTBC16 (or as a new RTBC16b1):

The closed flag is owned by the RootBatchContext; child BatchContext objects do not maintain their own copy. A child performs its closed-check by reading via the shared rootContext reference.

This avoids SDKs in other languages accidentally duplicating the flag per context (which would cause divergence under closure ordering).


These observations align with LIVEOBJECTS_UPDATE_SPEC_ACTIONABLE_PLAN.md.

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.

4 participants