[WIP, AIT-324] Apply operations on ACK#118
Draft
lawrence-forooghian wants to merge 10 commits intomainfrom
Draft
[WIP, AIT-324] Apply operations on ACK#118lawrence-forooghian wants to merge 10 commits intomainfrom
lawrence-forooghian wants to merge 10 commits intomainfrom
Conversation
Instead of waiting for the server to echo back an operation before applying it locally, operations are now applied immediately upon receiving the ACK from Realtime. Core changes: - ObjectsOperationSource enum (.local/.channel) per RTO22 - PublishResult struct wrapping serials from ACK - publishAndApply() on InternalDefaultRealtimeObjects (RTO20): publishes, awaits ACK with serials, constructs synthetic InboundObjectMessages with serial+siteCode, applies with source .local - appliedOnAckSerials set (RTO7b) to prevent double-application when the echoed OBJECT message arrives (RTO9a3); cleared on sync completion (RTO5c9) - Conditional siteTimeserials update: only for .channel source (RTLC7c, RTLM15c), not .local - nosync_apply returns Bool indicating whether op was applied Callers updated to use publishAndApply: - LiveCounter.increment/decrement (RTLC12g) - LiveMap.set/remove (RTLM20g, RTLM21f) - RealtimeObjects.createMap/createCounter (RTO11i, RTO12i) - createMap/createCounter now expect the object to exist in the pool after publishAndApply (RTO11h3d, RTO12h3d) Bridge layer: - CoreSDK.publish now returns PublishResult - DefaultInternalPlugin.sendObjectWithResult calls the new @optional plugin API method - DefaultInternalPlugin.siteCode retrieves CD2j siteCode - Package.swift pinned to local paths for development Known TODO: RTO20e1 channel state guard in publishAndApply (requires channel state change subscription infrastructure). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
When publishAndApply is waiting for objects sync to complete and the channel enters detached, suspended, or failed, reject with error code 92008 per RTO20e1. The error includes the channel's errorReason as cause. Implementation: - Add publishAndApplySyncWaiters to MutableState: an array of CheckedContinuation<Result<Void, ARTErrorInfo>, Never> for publishAndApply calls waiting for sync completion - nosync_onChannelStateChanged resumes all waiters with error 92008 (with cause from RealtimeChannel.errorReason) when channel enters detached/suspended/failed - Sync completion (both ATTACHED no-HAS_OBJECTS and OBJECT_SYNC complete paths) resumes all waiters with .success - Replace old onInternal(.synced) subscription in publishAndApply with the new waiter mechanism - DefaultInternalPlugin forwards nosync_onChannelStateChanged from ably-cocoa to InternalDefaultRealtimeObjects Test updates: - MockCoreSDK now has configurable siteCode (defaults to "site1") - 12 tests across 3 files updated to transition realtimeObjects to synced state (via nosync_onChannelAttached(hasObjects:false)) and return real serials from publish handlers, as required by publishAndApply This is the second commit on this branch. The first (febe6cf) implemented the core apply-on-ACK logic (RTO20). Together they implement the following spec points: RTO7b, RTO7b1, RTO8b, RTO9a2a4, RTO9a3, RTO5c6, RTO5c9, RTO15h, RTO20, RTO20b, RTO20c1, RTO20c2, RTO20d, RTO20d1, RTO20d2, RTO20e, RTO20e1, RTO20f, RTO22, RTLC7c/RTLM15c, RTLC7g/RTLM15g, RTLC12g, RTLM20g/RTLM21f, RTO11i/RTO12i, RTO11h3d/RTO12h3d, CD2j ## Remaining work on apply-on-ACK ### Push and pin dependencies All three repos have local commits on AIT-324-apply-on-ACK but are not yet pushed. After pushing, the swift plugin's Package.swift must be updated from local paths back to commit SHA pins. Currently Package.swift uses: .package(name: "ably-cocoa", path: "../ably-cocoa") .package(name: "ably-cocoa-plugin-support", path: "../ably-cocoa-plugin-support") ### Integration tests Port the ably-js "Apply on ACK" integration tests from commit 6a9b9142 (test/realtime/liveobjects.test.js, ~line 8368). Six test groups: 1. "Operations applied locally on ACK" — hold OBJECT echo messages, perform write ops, assert values visible immediately from ACK 2. "Does not double-apply" — echo after ACK, ACK after echo 3. "Does not incorrectly skip operations" — inject op with serial between create and increment 4. "ACKs buffered during OBJECT_SYNC" — operation during sync, appliedOnAckSerials cleared on sync 5. "publishAndApply rejects on channel state change" — verify error 92008 on detached/suspended/failed during sync wait 6. "Subscription events" — subscribe fires from apply-on-ACK and from Realtime The tests require intercepting OBJECT and ACK protocol messages. In ably-js this uses createEchoInterceptor() and createAckInterceptor(). Check if the Swift tests have equivalent infrastructure in TestProxyTransport and build similar interceptors if needed. The ably-js commit also removes many waitForMapKeyUpdate() / waitForCounterUpdate() calls that previously waited for the echo. With apply-on-ACK, the write operation's await is sufficient. Apply the same simplification to the Swift integration tests. ### Verification checklist - [x] swift build — all three repos - [x] swift build --build-tests — swift plugin - [x] swift test — unit tests pass (109 tests) - [ ] Push repos and pin Package.swift to commit SHAs - [ ] Integration tests pass against sandbox - [ ] swift run BuildTool lint (requires mint) ### References - Spec: worktree-groups/apply-on-ACK/specification/ (56a0bbaa) - ably-js: worktree-groups/apply-on-ACK/ably-js/ (6a9b9142) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace local path dependencies with revision-pinned remote dependencies pointing to the AIT-324-apply-on-ACK branch commits. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
messages Change ObjectMessageBox from private to fileprivate so the new ARTProtocolMessage.testsOnly_inboundObjectMessages extension (in the same file) can access it. This extension extracts InboundObjectMessage values from protocol messages held by test interceptors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add EchoInterceptor, AckInterceptor classes and injectAttachedMessage helper to ObjectsIntegrationTests.swift. These use TestProxyTransport's setBeforeIncomingMessageModifier to hold OBJECT echoes or ACK messages, allowing tests to control message ordering and verify apply-on-ACK behaviour. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add five groups of tests mirroring the JS apply-on-ACK test suite: 1. Operations applied locally on ACK (parameterized, 5 scenarios): LiveCounter create, increment; LiveMap create, set, remove. 2. Does not double-apply (2 tests): echo-after-ACK, ACK-after-echo. 3. Does not incorrectly skip operations (1 test): verifies siteTimeserials is not updated by apply-on-ACK. 4. ACKs buffered during OBJECT_SYNC (3 tests): buffered during sync, appliedOnAckSerials cleared on sync, publishAndApply rejects on channel state change. 5. Subscription events (1 test): callbacks fire for both locally applied and Realtime-received operations. Batch operations test is skipped (no Swift batch API). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
With apply-on-ACK, SDK write operations (createCounter, createMap, set, remove, increment, decrement) now apply locally on ACK, so awaiting the write is sufficient — no need to additionally wait for the echo to arrive via a subscription. Keep waitFor* calls after REST API operations (createAndSetOnMap, operationRequest) which are received over Realtime. Simplifies: OBJECT_SYNC builds object tree, OBJECT_SYNC does not change references, COUNTER_INC operations, MAP_REMOVE operations, LiveCounter.increment, LiveCounter.decrement, LiveMap.set with primitives, LiveMap.set with references, LiveMap.remove. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The initial ably-cocoa revision had a local path dependency on ably-cocoa-plugin-support which SPM cannot resolve from a revision-based requirement. Update to the new ably-cocoa commit that pins ably-cocoa-plugin-support by revision. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes several issues discovered while running the full test suite
against the sandbox after the initial apply-on-ACK implementation.
Package.resolved: Updated to track the remote ably-cocoa and
ably-cocoa-plugin-support pins now that Package.swift uses
revision-based dependencies instead of local paths.
DefaultInternalPlugin.swift:
- Change ObjectMessageBox from fileprivate to internal so that
tests can downcast protocol message state items to extract
InboundObjectMessage values.
- Fix ARTPublishResult bridging crash: cast publishResultProtocol
to the concrete ARTPublishResult type instead of using the
protocol's typed array property. ARTPublishResultSerial's
conformance to PublishResultSerialProtocol is declared behind
#ifdef ABLY_SUPPORTS_PLUGINS in a private header not visible to
Swift, causing the NSArray bridge to crash at runtime.
- Move the testsOnly_inboundObjectMessages extension from source
code into the test file, reimplemented using KVC (value(forKey:
"state")) since the state property is behind #ifdef.
ObjectsIntegrationTests.swift:
- Add @preconcurrency import Ably.Private for ChannelStateChange-
Params access without concurrency warnings.
- Add testsOnly_inboundObjectMessages extension using KVC to
extract InboundObjectMessage from protocol message state.
- Add waitForEchoCount(_:) to EchoInterceptor for waiting until a
specific number of echoes have been intercepted.
- Fix AckInterceptor.releaseAll() crash (SIGTRAP): transport.-
receive(ack) was called from the Swift concurrency executor, but
ably-cocoa's ACK completion handler has dispatchPrecondition(-
condition: .onQueue(internalQueue)). Now dispatches the entire
clear-replay-restore sequence onto client.internal.queue, with
an async signature so callers await completion.
- Fix applyOnAckDoesNotUpdateSiteTimeserials: createCounter +
root.set generate two echoes (COUNTER_CREATE and MAP_SET). The
test searched only the last echo for COUNTER_CREATE. Now uses
waitForEchoCount(2) and flatMap across all held echoes.
- Fix operationBufferedDuringSyncIsAppliedAfterSyncCompletes: the
test awaited counter.increment(amount:) which waits for sync to
complete, but the sync could only be completed by code after the
await (deadlock). Now starts the increment in a Task and sleeps
2s before completing the sync. Also fixed syncSerial from
"cursor_end" to "serial:" — the sync sequence ends when the
serial has an empty cursor after the colon.
- Fix appliedOnAckSerialsIsClearedOnSync: same syncSerial fix.
- Fix publishAndApplyRejectsOnChannelStateChangeDuringSync for the
detached case: setDetached: initiates a reattach when the
channel is already attached (per RTL12). Use detachChannel:
directly which calls performTransitionToState and properly
notifies the plugin.
- Fix ackAfterEchoDoesNotDoubleApply: use waitForAck + Task.sleep
approach instead of subscription-based waiting, with the fixed
async releaseAll().
- Fix 6 PublishResult(serials: []) overrides to return proper
serials (objectMessages.map { _ in "fake-serial" }) so that
apply-on-ACK can function in tests that override publish.
- Update 4 assertion comments to reflect that initial values now
come from local apply-on-ACK rather than CREATE operation echos.
All 244 tests pass (109 unit + 135 integration).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
github-actions
bot
temporarily deployed
to
staging/pull/118/AblyLiveObjects
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Pin to ably-cocoa eab1993 and plugin-support 693fb5e, which add the supporting API needed for apply-on-ACK. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When you call a LiveObjects mutation method (e.g.
map.set()), the SDK now applies the effects of this operation to the local LiveObjects data as soon as it receives the server's acknowledgement of this operation. This is an improvement over earlier versions, in which the SDK did not apply such an operation until receiving the operation's echo.Supporting PRs:
Related PRs: