Don’t replace existing discontinuity event

[lawrence-forooghian]

Make CHA-RL4b1 consistent with CHA-RL4a3.

[sacOO7]

Following our discussion here, we agree that only one discontinuity event will be stored per contributor. The remaining question is whether we should override it with the latest error.

I suggest we either override the discontinuity event with the latest error or store the discontinuity event with a list of unique errors. This approach would prevent overwhelming users with excessive notifications while providing relevant information about the discontinuity. This will also help users to identify discontinuity if caused by message rate limitations as per second point mentioned here or any other app limitations.

[AndyTWF]

Following our discussion here, we agree that only one discontinuity event will be stored per contributor. The remaining question is whether we should override it with the latest error.

I suggest we either override the discontinuity event with the latest error or store the discontinuity event with a list of unique errors. This approach would prevent overwhelming users with excessive notifications while providing relevant information about the discontinuity. This will also help users to identify discontinuity if caused by message rate limitations as per second point mentioned here or any other app limitations.

The original proposal is that we store and emit the original error, and only the original. From a user perspective - telling them about multiple discontinuities isn't particularly helpful in the case that the original event wasn't emitted immediately due to a lifecycle operation in progress. If anything, it could be more confusing - do we expect them to handle all the errors in some way, some of the discontinuities won't be important / something they should worry about, and are simply because of the room lifecycle atomicity.

I think we should merge this PR as-is, as it represents the current state-of-play (so-as not to block impeding releases), then we can discuss changes / improvements to the room lifecycle more broadly on the road to GA.

[sacOO7]

Added comments to rephrase spec points

[sacOO7]

Suggested change

*** @(CHA-RL4a3)@ @[Testable]@ If a room lifecycle operation is in progress, then a pending discontinuity event shall be recorded for this contributor - though it must not overwrite any existing discontinuity event. The @ErrorInfo@ associated with the discontinuity shall be the @reason@ for the underlying channel state change. The event will be notified to the contributor at a later point, as noted in this specification.

*** @(CHA-RL4a3)@ @[Testable]@ If a room lifecycle operation is in progress, then a pending discontinuity event shall be recorded for this contributor - though it must not overwrite any existing discontinuity event. The @ErrorInfo@ associated with the discontinuity shall be set to the @reason@ from the underlying channel state change. The event will be notified to the contributor at a later point, as noted in this specification.

[sacOO7]

Suggested change

*** @(CHA-RL4b11)@ @[Testable]@ If a room lifecycle operation is in progress, and the new channel state is @ATTACHED@, and the @resumed@ flag is false, @and@ the particular contributor has been attached previously (i.e. a previous call to @attach()@ on the channel, per @CHA-RL1f@, has succeeded), then a pending discontinuity event will be recorded for the contributor - though it must not overwrite any existing discontinuity event. The error associated with this event shall be the @reason@ for the channel state change.

*** @(CHA-RL4b11)@ @[Testable]@ If a room lifecycle operation is in progress, and the new channel state is @ATTACHED@, and the @resumed@ flag is false, @and@ the particular contributor has been attached previously (i.e. a previous call to @attach()@ on the channel, per @CHA-RL1f@, has succeeded), then a pending discontinuity event will be recorded for the contributor - though it must not overwrite any existing discontinuity event. The @ErrorInfo@ associated with the discontinuity shall be set to the @reason@ from the underlying channel state change.

[sacOO7]

I think we should merge this PR as-is, as it represents the current state-of-play (so-as not to block impeding releases), then we can discuss changes / improvements to the room lifecycle more broadly on the road to GA.

Okay, I don't want to block PR as such. Added few suggestions for better rephrasing the spec.
Also, had a last question, if we have an existing discontinuity with errorInfo as null, do we still not override with a new error?
Or do we set some default error when statechange.reason is null, to maintain original error : ?

[sacOO7]

Okay, I don't want to block PR as such. Added few suggestions for better rephrasing the spec. Also, had a last question, if we have an existing discontinuity with errorInfo as null, do we still not override with a new error? Or do we set some default error when statechange.reason is null, to maintain original error : ?

@AndyTWF do we set discontinuity error as null if statechange.reason is null for first discontinuity detected?