[AIT-280] Apply LiveObjects operations on ACK#419
[AIT-280] Apply LiveObjects operations on ACK#419lawrence-forooghian wants to merge 8 commits intomainfrom
Conversation
lawrence-forooghian
changed the base branch from
main
to
clarify-BufferedObjectsOperations-scope
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
from
f54eee0 to
09a0dad
Compare
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
2 times, most recently
from
78c6b72 to
12b5ae7
Compare
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
from
12b5ae7 to
de02ff2
Compare
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
from
d043e79 to
c3a4917
Compare
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). [1] ably/specification#419
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). TODO: - test for batch [1] ably/specification#419
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). TODO: - test for batch [1] ably/specification#419
Based on [1] at c3a4917. Implementation and tests are Claude-generated from the spec; I've largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). TODO: - test for batch [1] ably/specification#419
Simplification; see #419 (comment)
Written by Claude based on LODR-054 [1]. [1] https://ably.atlassian.net/wiki/spaces/LOB/pages/4671832082/LODR-054+SDK+apply+operations+on+ACK
Simplification; see #419 (comment)
lawrence-forooghian
force-pushed
the
AIT-280-LiveObjects-apply-on-ACK
branch
from
273b437 to
1d98e03
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Based on [1] at d809334. Implementation and tests are Claude-generated from the spec; I've reviewed them and given plenty of feedback, but largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). Summary of decisions re modifications to existing tests (written by Claude): - Removed redundant `waitFor*` calls after SDK operations (`map.set()`, `counter.increment()`, etc.) - with apply-on-ACK, values are available immediately after the operation promise resolves - Kept `waitFor*` calls after REST operations (`objectsHelper.operationRequest()`, `objectsHelper.createAndSetOnMap()`) - these still require waiting for the echo to arrive over Realtime - Added explanatory comment to `applyOperationsScenarios` noting that those tests cover operations received over Realtime (via REST), and pointing to the new "Apply on ACK" section for tests of locally-applied operations [1] ably/specification#419
This is the code that a LiveObjects mutation operation will fail with if the channel enters DETACHED, SUSPENDED, or FAILED whilst waiting for the objects sync state to become SYNCED. See [1]. [1] ably/specification#419
…20e1) The RTO20e wait for the objects sync to reach SYNCED has no failure condition, which means publishAndApply can hang indefinitely if the channel never completes the sync — for example, during a disconnection loop where publish succeeds (ACK received) but the OBJECT_SYNC sequence never finishes. This adds RTO20e1 to fail the operation when the channel enters DETACHED, SUSPENDED, or FAILED. The objects sync requires the channel to be ATTACHED to receive the OBJECT_SYNC sequence (RTO4c), so waiting for sync is essentially waiting for the channel to become attached. The decision of when to give up on that is already established by RTL11: queued presence messages — which are also waiting for the channel to become ATTACHED (RTP16b) — are failed when the channel enters DETACHED, SUSPENDED, or FAILED. We apply the same rule here. The error uses code 92008, in the objects error range in ably-common. Added in [1]. [1] ably/ably-common#328 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Added 57f4449 (new spec point RTO20e1) to address ably/ably-js#2155 (comment) (will squash everything before merge) |
…c, RTO20d1) Replace throwing errors with logging when publishAndApply cannot apply operations on ACK. RTO20c: log error and bail if siteCode or serials array unavailable (unexpected server behaviour). Split into RTO20c1 and RTO20c2. RTO20d1: log debug and skip individual operations with null serial (conflation — not currently expected but handled gracefully). This is the outcome of discussion [1] plus a conversation that I had with Paddy. [1] #419 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
textile/objects-features.textile
Outdated
There was a problem hiding this comment.
Seeing the outcome of the siteCode and serials discussion in #419 (comment) - where we decided not to throw an error - I'm curious whether this spec was intentionally kept as a throw. As we are still in the post-successful-ACK branch, the same rationale from RTO20c and RTO20d1 seems applicable here: log an error that we couldn't apply the operation locally rather than throwing.
There was a problem hiding this comment.
Also, the suggested log message - 'stating that the objects sync could not be completed due to the channel entering the respective state' - doesn't feel right in this context. A user is performing a mutation, so receiving an error about objects sync not completing is confusing IMO. It might be clearer to follow the phrasing pattern from RTO20c and RTO20d1 and say something like: 'the operation will not be applied locally because objects sync could not be completed due to the channel entering the respective state'.
There was a problem hiding this comment.
I think that both of those are fairly edge-case things at the moment — RTO20c is completely for handling server behaviour that should never occur, and RTO20d1 is for handling server behaviour that currently doesn't occur but may in the future (and I think that we may still end up revisiting the apply-on-ACK behaviour in such a case). I think that the general expectation remains that we make a best effort to apply on ACK, and I think that we shouldn't fail to do so silently in the case where we can't apply on ACK because we failed to sync.
I agree re the error message though, have changed; now reads "the operation could not be applied locally due to the channel entering the {state} state whilst waiting for objects sync to complete"
There was a problem hiding this comment.
I think that both of those are fairly edge-case things at the moment — RTO20c is completely for handling server behaviour that should never occur, and RTO20d1 is for handling server behaviour that currently doesn't occur but may in the future (and I think that we may still end up revisiting the apply-on-ACK behaviour in such a case). I think that the general expectation remains that we make a best effort to apply on ACK, and I think that we shouldn't fail to do so silently in the case where we can't apply on ACK because we failed to sync.
If you'd prefer to leave as throw then sure, just wanted to point out the difference with two other cases and to ensure this is an intentional behavior. Also in this ably-js PR comment, it seems that Paddy recognizes this "failed to apply because of sync" case as separate to two other mentioned above and is fine with leaving it as throw. Fine by me then.
I agree re the error message though, have changed; now reads "the operation could not be applied locally due to the channel entering the {state} state whilst waiting for objects sync to complete"
Thanks
There was a problem hiding this comment.
Going to leave this unresolved for visibility
Based on [1] at d809334. Implementation and tests are Claude-generated from the spec; I've reviewed them and given plenty of feedback, but largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). Summary of decisions re modifications to existing tests (written by Claude): - Removed redundant `waitFor*` calls after SDK operations (`map.set()`, `counter.increment()`, etc.) - with apply-on-ACK, values are available immediately after the operation promise resolves - Kept `waitFor*` calls after REST operations (`objectsHelper.operationRequest()`, `objectsHelper.createAndSetOnMap()`) - these still require waiting for the echo to arrive over Realtime - Added explanatory comment to `applyOperationsScenarios` noting that those tests cover operations received over Realtime (via REST), and pointing to the new "Apply on ACK" section for tests of locally-applied operations [1] ably/specification#419
Address #419 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Address ably/specification#419 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Based on [1] at d809334. Implementation and tests are Claude-generated from the spec; I've reviewed them and given plenty of feedback, but largely resisted the temptation to tweak things that aren't quite how I'd write them but which are still correct. The only behaviour here that's not in the spec is to also apply-on-ACK for batch operations (the batch API isn't in the spec yet). Summary of decisions re modifications to existing tests (written by Claude): - Removed redundant `waitFor*` calls after SDK operations (`map.set()`, `counter.increment()`, etc.) - with apply-on-ACK, values are available immediately after the operation promise resolves - Kept `waitFor*` calls after REST operations (`objectsHelper.operationRequest()`, `objectsHelper.createAndSetOnMap()`) - these still require waiting for the echo to arrive over Realtime - Added explanatory comment to `applyOperationsScenarios` noting that those tests cover operations received over Realtime (via REST), and pointing to the new "Apply on ACK" section for tests of locally-applied operations [1] ably/specification#419
Address ably/specification#419 (comment) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reflects the functionality implemented in [1], based on the DR [2] and spec [3]. Written by Claude. [1] ably/ably-js#2155 [2] https://ably.atlassian.net/wiki/spaces/LOB/pages/4671832082/LODR-054+SDK+apply+operations+on+ACK) [3] ably/specification#419
4 participants
Written by Claude based on LODR-054. JS implementation in ably/ably-js#2155.
There are a couple of places here where I'd appreciate input from reviewers; please see the unresolved comments towards the end of the PR.