Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete unused ConnRecord generated - OOB invitation (use_exising_connection) #1521

Merged
merged 12 commits into from
Dec 1, 2021

Conversation

shaangill025
Copy link
Contributor

@shaangill025 shaangill025 commented Nov 25, 2021

Signed-off-by: Shaanjot Gill gill.shaanjots@gmail.com

  • resolve OOB invitation not deleted when using use_existing_connection #1511
  • The problem it solves:
    • If 2 agents [Alice and Bob] have an active connection setup (using an OOB public DID invitation)
    • Alice send another OOB invitation (public) to Bob
    • Bob accepts it with use_existing_connection option, then on Alice side, the invitation and associated connection generated remain unused.
  • This PR implements flush_stale_connections function, which deletes all ConnRecords with state: invitation and invitation_mode: once which were last updated at least 3 hours back. The trigger for this function is creating an OOB invitation, so whenever a new OOB invitation is created then this cleanup is performed. Does this strategy sound reasonable?
  • Following feedback from @swcurran, fixed initial flawed implementation of reuse message and used it to accomplish deletion of hanging connections.

Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
@swcurran
Copy link
Contributor

Is it not possible to identify the invitation that Bob is responding to with the "reuse" message, and directly delete that invitation for this use case? I would think the thread ID of the message identifies the original invitation, allowing it to be removed.

The "flush..." tactic seems overly broad for the scenario and potentially problematic.

I would definitely not want to have a fixed time for the "flush" -- it should be an easily changed variable at minimum, and likely configurable.

@shaangill025
Copy link
Contributor Author

shaangill025 commented Nov 25, 2021

Is it not possible to identify the invitation that Bob is responding to with the "reuse" message, and directly delete that invitation for this use case? I would think the thread ID of the message identifies the original invitation, allowing it to be removed.

The "flush..." tactic seems overly broad for the scenario and potentially problematic.

I would definitely not want to have a fixed time for the "flush" -- it should be an easily changed variable at minimum, and likely configurable.

@swcurran I like the approach of using reuse messages for this purpose. Currently, reuse messages hold the existing connection's invi_msg_id (pthid) to check if the connection is still active on the other agent and then send the reuse-accept message. For thid, it holds the identifier for the reuse message to process the reuse-accept message, if received.

I can update reuse to include a received_invi_msg_id field which can be used to delete the unused ConnRecord? I believe RFC0434 might need to be updated here.

@shaangill025 shaangill025 marked this pull request as draft November 25, 2021 17:33
@swcurran
Copy link
Contributor

Hmm...I don't see a need to revise the RFC. It looks to me like the RFC requires that Invitation ID be the pthid in the handshake_reuse_method, which should give you all you need.

What is it that you are seeing needs to be updated in the RFC?

I definitely think that it would be useful to clarify the RFC to talk about deleting a hanging invitation when reuse is used, to help future implementers, but I don't see a change to the spec.

@shaangill025
Copy link
Contributor Author

shaangill025 commented Nov 25, 2021

Hmm...I don't see a need to revise the RFC. It looks to me like the RFC requires that Invitation ID be the pthid in the handshake_reuse_method, which should give you all you need.

What is it that you are seeing needs to be updated in the RFC?

There are 2 different invitation ids needed in the reuse message workflow:

  • The one from the OOB invitation received by the invitee (to delete the unused connection at the inviter side)
  • The one from the existing active connection (to check if this connection still exists and is active at the inviter side)

The current structure of reuse message only allows the exchange of one of the invitation id.

@shaangill025
Copy link
Contributor Author

shaangill025 commented Nov 25, 2021

Another option, could be to assign the received invitation id as the @id for reuse message, which should be unique and meet that requirement. This way structure of reuse message remains unchanged.

@swcurran
Copy link
Contributor

There shouldn't be a hack needed. The reuse message comes back on an existing connection. Just by receiving and being able to see the message, you can tell what connection to associate the message with, and the response. Further, by looking at the pthid, you should get the invitation_id message, and thus be able to find and delete that "hanging" connection. That is what was expected to happen in designing the RFC Protocol. Is that not possible? Our expectation was that the inviter would have all the information needed to connect the reuse message with the both the connection to be used going forward, and with the invitation that triggered it.

Hmmm...maybe it is the Admin API that needs some work to tell the controller that the invitation that was sent out is now associated with this other connectionID?

Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
@shaangill025
Copy link
Contributor Author

There shouldn't be a hack needed. The reuse message comes back on an existing connection. Just by receiving and being able to see the message, you can tell what connection to associate the message with, and the response. Further, by looking at the pthid, you should get the invitation_id message, and thus be able to find and delete that "hanging" connection. That is what was expected to happen in designing the RFC Protocol. Is that not possible? Our expectation was that the inviter would have all the information needed to connect the reuse message with the both the connection to be used going forward, and with the invitation that triggered it.

Thanks @swcurran for the clarification, my initial implementation of reuse message was flawed and has now been fixed according to the above logic. With this, we can now accept reuse messages from the connection and delete hanging connection from the pthid.

@shaangill025 shaangill025 marked this pull request as ready for review November 25, 2021 21:04
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
ianco
ianco previously approved these changes Nov 25, 2021
Copy link
Contributor

@ianco ianco left a comment

Choose a reason for hiding this comment

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

LGTM will leave to @swcurran to review/merge

@shaangill025
Copy link
Contributor Author

shaangill025 commented Nov 25, 2021

Hmmm...maybe it is the Admin API that needs some work to tell the controller that the invitation that was sent out is now associated with this other connectionID?

On reusing an existing connection, when the inviter accepts reuse and deletes the hanging connection, it does not update the existing connection. So the identifier for the invitation that was sent out is not assigned to the existing connection. The changes below updates invitation_msg_id for the existing connection on both invitee and inviter sides.

Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Signed-off-by: Shaanjot Gill <gill.shaanjots@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OOB invitation not deleted when using use_existing_connection
3 participants