Vault sharing notification is not sent

[CMCDragonkai]

Describe the bug

Sharing a vault to a node ID causes this log in the agent logs:

WARN:polykey.PolykeyAgent.NotificationsManager:Could not send VaultShare notification to vqsq2ca7vjbsiv3paletajdduecm0v36j00jbnbg5eg084bm46g6g: ErrorPolykeyRemote
WARN:polykey.PolykeyAgent.task v0pocjo3thlo01bik4q0di1273s:Failed - Reason: ErrorPolykeyRemote

There's no notifications sent.

To Reproduce

1. Try sharing a vault and observe the agent logs.

Expected behavior

I'm able to connect and share the vault, and the remote target can clone the vault. But notifications is not sent, so this sounds like a trivial bug. It should just sent without any other problems.

Screenshots

Platform (please complete the following information)

version          	1.14.0-1-1
sourceVersion    	1.14.0
stateVersion     	1
networkVersion   	1
versionMetadata  	

Notify maintainers

@tegefaulkes

[linear]

ENG-427 Vault sharing notification is not sent

[CMCDragonkai]

@CDeltakai do systemctl status ... to get the logs specific to receiving notifications.

[CMCDragonkai]

We need a better regression testing here.

[CMCDragonkai]

There's no error on the remote side that shows up.

[tegefaulkes]

I did a quick test on my side. and got this when trying to share the vault.

ErrorPolykeyRemote
    at toError (/home/brian/workspace/Polykey-CLI/node_modules/polykey/src/network/utils.ts:581:29)
    at Object.transform (/home/brian/workspace/Polykey-CLI/node_modules/@matrixai/rpc/src/utils.ts:452:19)
    at ensureIsPromise (node:internal/webstreams/util:185:19)
    at transformStreamDefaultControllerPerformTransform (node:internal/webstreams/transformstream:515:18)
    at transformStreamDefaultSinkWriteAlgorithm (node:internal/webstreams/transformstream:565:10)
    at Object.write (node:internal/webstreams/transformstream:360:14)
    at ensureIsPromise (node:internal/webstreams/util:185:19)
    at writableStreamDefaultControllerProcessWrite (node:internal/webstreams/writablestream:1112:5)
    at writableStreamDefaultControllerAdvanceQueueIfNeeded (node:internal/webstreams/writablestream:1227:5)
    at writableStreamDefaultControllerWrite (node:internal/webstreams/writablestream:1101:3) {
  data: {},
  cause: ErrorNotificationsPermissionsNotFound
      at f.receiveNotification (/nix/store/gfwlbhw0b2fkh40kbb1sakwmpd53ln66-polykey-cli-0.10.0/lib/node_modules/polykey-cli/dist/polykey.js:2246:59829)
      at async withF (/nix/store/gfwlbhw0b2fkh40kbb1sakwmpd53ln66-polykey-cli-0.10.0/lib/node_modules/polykey-cli/node_modules/@matrixai/resources/dist/utils.js:17:16)
      at async handle (/nix/store/gfwlbhw0b2fkh40kbb1sakwmpd53ln66-polykey-cli-0.10.0/lib/node_modules/polykey-cli/dist/polykey.js:2131:215244)
      at async wrapperDuplex (/nix/store/gfwlbhw0b2fkh40kbb1sakwmpd53ln66-polykey-cli-0.10.0/lib/node_modules/polykey-cli/dist/polykey.js:2124:127100)
      at async outputGen (/nix/store/gfwlbhw0b2fkh40kbb1sakwmpd53ln66-polykey-cli-0.10.0/lib/node_modules/polykey-cli/dist/polykey.js:2124:126196)
      at async Object.pull (/nix/store/gfwlbhw0b2fkh40kbb1sakwmpd53ln66-polykey-cli-0.10.0/lib/node_modules/polykey-cli/dist/polykey.js:2124:126394) {
    data: {},
    cause: undefined,
    timestamp: 2024-10-14T04:55:06.381Z,
    exitCode: 67
  },
  timestamp: 2024-10-14T04:55:06.390Z,
  exitCode: 67,
  metadata: {
    localHost: '::',
    localPort: 36611,
    remoteHost: '::ffff:1.40.225.41',
    remotePort: 34140,
    command: 'notificationsSend'
  }
}
WARN:polykey.PolykeyAgent.NotificationsManager:Could not send VaultShare notification to vqsq2ca7vjbsiv3paletajdduecm0v36j00jbnbg5eg084bm46g6g: ErrorPolykeyRemote
WARN:polykey.PolykeyAgent.task v0pockgl5eho01823hj3jvapvrs:Failed - Reason: ErrorPolykeyRemote

After getting @CDeltakai to trust my node I was able to share a new vault with him without error.

This is designed behavior where you just outright reject notifications from vaults where you don't trust them first. However the feedback on the problem here is really bad. We need to include all the errors in the cause chain when reporting errors like this.

Along with sending notifications specifically, if we reject a notification due to lack of trust, There should be a logger message for it as well.

[tegefaulkes]

I'm going to address this in two parts.

1. I'm going to improve the details provided when logging out these errors. In the case where we have nested errors with causes I want to print out the whole cause chain description.
2. Specifically for this, we need to take the ErrorPolykeyRemote we're getting and wrap that in a more descriptive error stating that the notification was rejected by the receiving node.

[tegefaulkes]

Ok, so I've made the error logging better here and improved the related tests in the CLI.

Besides the fact that why the notification failed wasn't clear. This was intended behaviour. The vaults share command itself doesn't care if the notification fails. The only reason we got any feedback that the notification failed is because background tasks, regardless if the failure is allowed or not, is logged as a warning.

[tegefaulkes]

With the new changes release I'm going to consider this one done. Unless we want to consider changing the intended behaviour of vaults share and the failing notification.

[CMCDragonkai]

Ok, so I've made the error logging better here and improved the related tests in the CLI.

Besides the fact that why the notification failed wasn't clear. This was intended behaviour. The vaults share command itself doesn't care if the notification fails. The only reason we got any feedback that the notification failed is because background tasks, regardless if the failure is allowed or not, is logged as a warning.

I thought it was due to no trust/permission to share.

[CMCDragonkai]

Example of the new log?

[CMCDragonkai]

I still want to make sure we're structured in that though.

[tegefaulkes]

All PolykeyErrors will include the full error chain when stringified now. So thw warning looks like this.

WARN:NotificationsManager Test:Could not send VaultShare notification to vmi821ubhbelid409ocdlf19hphrf1ltv8fhvmr12pu4g4l5afsdg: ErrorPolykeyRemote()>ErrorNotificationsPermissionsNotFound()
WARN:task v0pog95uas5o01d8t2l68bqblk0:Failed - Reason: ErrorPolykeyRemote()>ErrorNotificationsPermissionsNotFound()

The stringified error part is ErrorPolykeyRemote()>ErrorNotificationsPermissionsNotFound() If it includes a message then it would look like SomeError("some message")>CauseError() The formatting could use some feedback.

It was due to a lack of trust to accept notifications. Sharing was unaffected.

[CMCDragonkai]

Hmmm, I would prefer to use json for structured messages, and a better serialisation of error messages. The () seems weird. Basically js-errors should be updated instead.

[tegefaulkes]

We could use a JSON format. I structured it this way to keep it succinct and avoid bloating out the log size. Let me play around with some formats.

Once we've settled on that I'll move the formatting change into js-errors. I'll create a new issue to track that.

[tegefaulkes]

There are 3 things left to do for this.

1. I made a branch or PR in the js-errors repo for changing the toString formatting of errors. Just rip that out, close it or what ever. its no longer needed.
2. The change in Polykey needs to remove the toString method from the ErrorPolykey that's creating the new format.
3. Where ever we do a e.toString() or String(e) on errors when logging out a warning or error in the logger. We need to use a custom utility that will convert errors into formatted strings for the logger message.
   
   There may be existing issues relating to this that need to be updated or closed.

[aryanjassal]

I will be taking over this issue now.

[CMCDragonkai]

There could be a default toString in the js-errors that is intended to be overridden by applications.

[tegefaulkes]

So we want to keep that now? That's what I intended to do but after discussion we went with having whatever was serialising the error to a string would decide how to format it.