Asynchronous Notification Architecture and Notification Lifecycle Commands

[amydevs]

Specification

As the sending notifications in the NotificationsManager are being made asynchronous, we need to introduce commands that interact with the lifecycle of NotificationsManager and the Notifications.

Send

polykey notifications send

This command will be largely the same, but will no longer immediately error. Instead, errors are logged on the Polykey agent, and attempts to send a notification are retried.

Options

--retries [number] // the amount of retries that should be attempted for a notification.

Read

polykey notifications inbox read
polykey notifications outbox read

The notifications read command has now been renamed to inbox read. The outbox read command now also exists for checking what notifications are pending in the outbox. The notificationIdEncoded field is now available on the output so that the user can be given an index to remove a specific notification by.

Options

--limit [number] // has been renamed from --number, limits the amount of notifications shown
--order [newest|oldest]
--unread // this only exists on `inbox read`

Clear

polykey notifications inbox clear
polykey notifications outbox clear

Still the same, but outbox clear also cancels the deleted outbox notifications from being sent.

Remove

polykey notifications inbox remove
polykey notifications outbox remove

Removes a specific notification by the notificationIdEncoded that is returned from polykey notifications * read.

Additional context

#163
MatrixAI/Polykey#703

Tasks

1. Implement notifications read commands
2. Implement notifications clear commands
3. Implement notifications remove commands

[linear]

ENG-37 Failure to send notification for `vaults share` should fail silently

[amydevs]

I've made some changes in feature-notifications branch of Polykey that adds an option to make NotificationsManager.sendNotification non-blocking and allow retries. I've made it so that a new task is scheduled for each retry attempt, so that the NotificationsManager can shut down cleanly after each retry. Retries are delayed linearly by a factor of 2 each time.

In cases where blocking and multiple retries are used, i've made it so that if none of the retries succeeded, the promise will reject. Furthermore, it will reject if we receive a PolykeyRemoteError, because it indicates that the receiver and received the notification, but rejected it for some reason, hence we shouldnt keep badgering it with the same notification. If any of the retries succeed, promise will resolve.

This granularity allows for the method to also function as if it were the old blocking-only implementation.

To continue, I will convert all relevant usage of NotificationsManager.sendNotifification to include the non-blocking parameters. This will make it so that vault share will no longer fail if the notification has failed to send.

Then, I will implement a NotificationsManager.pendingNotifications method that lists the pending notifications that are in the outbox, as well as a way to cancel pending notifications.

[CMCDragonkai]

Can you open a PR and target this issue?

[CMCDragonkai]

Also complete the task list on the spec too! And you are using Task manager for this?

[amydevs]

👍

[amydevs]

the current progress: I've implemented NotificationIds using the same notification id generator for outbox notifications. I've also implemented a Map that maps `NotificationIds` to TaskIds. Entries get added at the start of NotificationsManager based on the existing tasks, and removed on stop of NotificationsManager. Notifications sending tasks also delete entries once they're created.

The current task I am up to now, is figuring out how to rework either the Notification type and/or NotificationsManager interface to expose the NotificationsIds so that the CLI is able to cancel certain pending outbox notifications.

The options i'm thinking of atm are:

1. have sender and receiver have different notification ids and expose them on readNotifications by returning { id: ..., notification: ... }
2. have sender and receiver have different notification ids and expose themby making getNotificationIds public, and changing the API of NotificationsManager so that users call getNotificationIds and then call readNotificationById manually.
3. have sender and receiver have different notification ids and expose them on the Notification type, but removing the id field when commiting it to state or sending across the network
4. have sender and receiver have same notification ids and expose them on the Notification type, but removing the id field when commiting it to state

Brian suggests that both the IDs on the sender side and the receiver side should be acknowledged, cos they also act as timestamps for when the notification is sent and when it is received. This would require a hybrid approach of no.4 and no.3.

This will have to have a little more speccing out to do then…

[CMCDragonkai]

If #695 is about turning notifications into delay tolerant asynchronous architecture, then this issue is underspecced ,and there needs to be rewritten to include both work that will occur in Polykey library and Polykey CLI. This issue therefore a like a "reason" to do the larger asynchronous architecture which should be specced out. The title of this needs to be rewritten.

[CMCDragonkai]

I just renamed this to "Asynchronous Notification Architecture for Delay-Tolerant Notifications". So it's more obvious what this entails.

[CMCDragonkai]

The command no longer will error if it has failed to be sent, unless the --blocking parameter is provided. The --retries parameter will also be available to set the amount of retries that the notification should attempt.

Will --retries | -r and --blocking | -b conflict with existing high level parameters? I believe we originally wanted to do a program wide --retry flag.

[CMCDragonkai]

polykey notifications inbox remove
polykey notifications outbox remove

Is remove or delete more common in our program?

[CMCDragonkai]

Also when printing out a notification, what is the default human format? Just a block of text?

Given a structured document - I'd suggest that our human formatting should provide it more like a dictionary. With metadata above.

Especially in terms of showing up as a inbox or outbox notification.

[amydevs]

Also when printing out a notification, what is the default human format? Just a block of text?

Given a structured document - I'd suggest that our human formatting should provide it more like a dictionary. With metadata above.

Especially in terms of showing up as a inbox or outbox notification.

I'll be following the existing inbox read command for the outbox

[amydevs]

polykey notifications inbox remove
polykey notifications outbox remove

Is remove or delete more common in our program?

i think the function in NotificationsManager was originally called removeNotification, so i just kept using it

[amydevs]

in action wip

[amydevs]

Notes

Blocking

None

TIme Estimates

About 3 days

Identify Uncertainties

• sub-sub-sub commands
• task metadata information

[CMCDragonkai]

Can you involve @CryptoTotalWar here to user-test this.

[CryptoTotalWar]

I think these new commands make sense. Looking forward to testing them out in the next release!

[tegefaulkes]

This may still be a problem. Was the fix confirmed @amydevs? Have we just not done a release yet?

#183 seems to be showing the same problem again.

rulerpablo@Pablos-MacBook-Air ~ % polykey vaults share my-software-project v4c11qv5fpq2fm3ropmma2sglfc9349jspqb1iutl3f7en1ckv500
ErrorRPCTimedOut: RPC has timed out

I don't think we've released a version of Polykey-CLI that includes the fix for this in Polykey yet.