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

fix: keylist update response race condition #2391

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Jul 31, 2023

When receiving an OOB invitation and automatically accepting by sending a DID Exchange request, it is possible to send a keylist update to the mediator and receive a response back before the connection is saved. This resulted in errors like:

Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/protocols/coordinate_mediation/v1_0/route_manager.py", line 273, in connection_from_recipient_key
    conn = await ConnRecord.retrieve_by_tag_filter(
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/messaging/models/base_record.py", line 284, in retrieve_by_tag_filter
    raise StorageNotFoundError(
aries_cloudagent.storage.error.StorageNotFoundError: ConnRecord record not found for {'invitation_key': 'qHFoiMYB58Y25wRkFfYajhFLSUvJFZZbJStiaKL1LvW'}

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/protocols/coordinate_mediation/v1_0/handlers/keylist_update_response_handler.py", line 45, in notify_keylist_updated
    key_to_connection = {
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/protocols/coordinate_mediation/v1_0/handlers/keylist_update_response_handler.py", line 46, in <dictcomp>
    updated.recipient_key: await route_manager.connection_from_recipient_key(
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/protocols/coordinate_mediation/v1_0/route_manager.py", line 280, in connection_from_recipient_key
    conn = await ConnRecord.retrieve_by_did(session, my_did=did_info.did)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/connections/models/conn_record.py", line 313, in retrieve_by_did
    return await cls.retrieve_by_tag_filter(session, tag_filter, post_filter)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/messaging/models/base_record.py", line 284, in retrieve_by_tag_filter
    raise StorageNotFoundError(
aries_cloudagent.storage.error.StorageNotFoundError: ConnRecord record not found for {'my_did': '2XamEWAbXkzx4eKW24PmSY'}

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/local/lib/python3.9/asyncio/tasks.py", line 256, in __step
    result = coro.send(None)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/core/dispatcher.py", line 269, in handle_message
    await handler(context, responder)
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/protocols/coordinate_mediation/v1_0/handlers/keylist_update_response_handler.py", line 31, in handle
    await self.notify_keylist_updated(
  File "/home/aries/.local/lib/python3.9/site-packages/aries_cloudagent/protocols/coordinate_mediation/v1_0/handlers/keylist_update_response_handler.py", line 52, in notify_keylist_updated
    raise HandlerException(
aries_cloudagent.messaging.base_handler.HandlerException: Unknown recipient key received in keylist update response

Under this condition, this makes it impossible for the keylist update response event emitter to know what connection the updated key was associated with. This PR addresses this by ensuring the DID is associated with the connection record before emitting a keylist update to the mediator.

There is a side effect to this change: one additional connection webhook will be emitted where the only change from the previous webhook will be a value for my_did; the state will not change from invitation-received. It's possible a controller might not like this -- though I think that would be a sign of other issues. Nevermind, I solved this issue.

@dbluhm dbluhm force-pushed the fix/keylist-update-response-race branch from 1a3f014 to 9811395 Compare July 31, 2023 22:24
@dbluhm
Copy link
Contributor Author

dbluhm commented Jul 31, 2023

It strikes me that I might actually be understating the impact of receiving a webhook with the same state twice. This would require that controllers' handling of webhooks is idempotent for a given connection ID and state. This might be an unreasonable assumption. Interested in feedback.

@dbluhm
Copy link
Contributor Author

dbluhm commented Jul 31, 2023

I realized that the keylist update didn't need to take place at that exact point in the method so I just moved it to occur after the original save that was already happening in the method. This should solve the conundrum I created for myself.

Copy link
Contributor

@usingtechnology usingtechnology left a comment

Choose a reason for hiding this comment

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

LGTM!
Nice work figuring out your sticking point.

@dbluhm dbluhm enabled auto-merge August 2, 2023 16:04
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dbluhm dbluhm merged commit ad702a1 into openwallet-foundation:main Aug 2, 2023
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.

2 participants