[BUG] Subscription Timeout retries was broken when fixing the Readhandler creation issue

[mkardous-silabs]

Reproduction steps

PR #30491 Modified the Subscription Timeout logic in a such a way where the server will only retry once before deleting the persisted subscription entry.

SubscriptionResumptionSessionEstablisher::HandleDeviceConnectionFailure deletes the subscription entry if the server fails to establish a case session. This modifies the previously released feature without the PR mentionning this change.

To reproduce :

• Build a sample app with chip_persist_subscriptions=true chip_subscription_timeout_resumption=true
• Commission the device
• Establish a subscription with chip-tool without being in interactive mode
• Let the subscription fail since the chip-tool will not send a status response
• After 5 minutes, server will try to resub and fail
• Device will never retry to resub

Bug prevalence

Everytime

GitHub hash of the SDK that was being used

166c4b5

Platform

core

Platform Version(s)

No response

Anything else?

This is a regression of a feature released in v1.2.

[mkardous-silabs]

Can't assign the issue to @wqx6 for some reason who had done #30491.

[wqx6]

When the server fails to establish the session, the client might be shutdown. Since the client will never store the subscription information, the client will forget the ReadClient which holds the resumed subscription after it reboots. Then the subscription will be meaningless since the report will never be handled. That's the reason why I delete the the subscription entry.

[mkardous-silabs]

I'm not sure the explanation justifies unilaterally changing the entires features behavior...
I don't think we can assume clients don't have a means to store subscriptions. Even if they do not, they will most likely create a new one if they receive a subscription resumption from a server if they support the feature.

At the end of the day, i don't think it this is an ok change. In my opignion, a better approach would have been to add a number of retries before deleting the entry that could have been set to one to allow the behavior you want without changing how the feature behaves.

[jtung-apple]

I agree with Mathieu. Subscription resumption has to retry multiple times, in case of temporary network outage, for example. I failed to notice the Delete change in the previous PR and I feel that part has to be reverted to keep the original logic.

[wqx6]

Also I found another cause why the sever app does not retry on resuming subscriptions.

The previous work flow is:
server app tries to resume subscription and create a read handler for it-> server app fails to establish the session-> the read handler is closed and InteractionModelEngine::OnDone() is called->sever app schedules another subscription resumption

But the current work flow is:
sever app tries to resume subscription->server app fails to establish the session

The read handler will not be created and InterationModelEngine::OnDone() will not be called. So server app will never schedule another subscription resumption.

Should we schedule subscription resumption when failing to establish the session? @mkardous-silabs @jtung-apple

[jtung-apple]

Also I found another cause why the sever app does not retry on resuming subscriptions.

The previous work flow is: server app tries to resume subscription and create a read handler for it-> server app fails to establish the session-> the read handler is closed and InteractionModelEngine::OnDone() is called->sever app schedules another subscription resumption

But the current work flow is: sever app tries to resume subscription->server app fails to establish the session

The read handler will not be created and InterationModelEngine::OnDone() will not be called. So server app will never schedule another subscription resumption.

Should we schedule subscription resumption when failing to establish the session? @mkardous-silabs @jtung-apple

Yes we should schedule subscription resumption in this case. Please restore the intended logic. Thanks!

[jtung-apple]

@wqx6 Will you be making a fix soon for this?