Support retriable Chelonia actions #1768

snowteamer · 2023-10-16T13:28:11Z

Closes #1755

Summary of changes:

This PR implements the 'chelonia.persistentActions' SBP domain, according to requirements described in Implement a persistent action queue #1755

taoeffect

Preliminary review - looking really solid so far...!

frontend/main.js

shared/domains/chelonia/persistent-actions.js

taoeffect · 2023-10-18T19:52:59Z

shared/domains/chelonia/persistent-actions.js

+  'chelonia.persistentActions/_init' (): void {
+    sbp('okTurtles.events/on', LOGIN, (function () {
+      this.actionsByID = Object.create(null)
+      this.databaseKey = `chelonia/persistentActions/${sbp('state/vuex/getters').ourIdentityContractId}`


Similarly, as this is a utility for Chelonia (and possibly in the future, an independent library), we cannot access 'state/vuex/getters' either

shared/domains/chelonia/persistent-actions.js

taoeffect

Looking really good @snowteamer!!

I'm wondering if we can also adjust the default options? Perhaps using the existing selector 'chelonia.persistentActions/configure'?

taoeffect · 2023-10-24T18:19:30Z

frontend/main.js

+        const databaseKey = `chelonia/persistentActions/${sbp('state/vuex/getters').ourIdentityContractId}`
+        sbp('chelonia.persistentActions/configure', { databaseKey })
+        await sbp('chelonia.persistentActions/load')
+        await sbp('chelonia.persistentActions/retryAll')


I think 'chelonia.persistentActions/load' should automatically start the retries

That would be fine, but on the other hand /save doesn't stop the pending retries

It shouldn't. The persistent action queue is something that is always active as long as it has actions inside of it while the app is running.

taoeffect

I think this is almost ready to merge.

@snowteamer - I think you might have missed this request from the previous review:

I'm wondering if we can also adjust the default options? Perhaps using the existing selector 'chelonia.persistentActions/configure'?

This way we can override the default options for values like maxAttempts and retrySeconds.

Also added one more request about emitting events.

taoeffect · 2023-10-26T17:20:02Z

shared/domains/chelonia/persistent-actions.js

+    this.options.successInvocationSelector &&
+      await this.trySBP([this.options.successInvocationSelector, result])


Can we refactor this to use events instead of options.successInvocationSelector?

I think it makes sense to add at least two events using okTurtles.events:

CHEL_PERSISTENT_ACTIONS_SUCCESS - would be emitted above here

CHEL_PERSISTENT_ACTIONS_FAILURE - would be emitted next to line 82 above, next to the errorInvocation, along with the error object

And if you think of any others that we should emit, feel free to suggest

Silver-IT

Really impressive PR, @snowteamer. 👍👍👍
Just left a few comments.

Silver-IT · 2023-11-05T13:31:27Z

shared/domains/chelonia/persistent-actions.js

+    status.lastError = error.message
+    const anyAttemptLeft = options.maxAttempts > status.failedAttemptsSoFar
+    if (!anyAttemptLeft) status.resolved = true
+    status.nextRetry = anyAttemptLeft && !status.resolved


!status.resolved could be ignored, I think.

status.nextRetry = anyAttemptLeft ? new Date(Date.now() + options.retrySeconds * 1e3).toISOString() : ''

status.resolved can be set independently by .cancel(), therefore checking for anyAttemptLeft was not enough

shared/domains/chelonia/persistent-actions.js

Silver-IT · 2023-11-05T13:54:37Z

shared/domains/chelonia/persistent-actions.js

+    }
+  },
+
+  'chelonia.persistentActions/enqueue' (...args): UUIDV4[] {


I think it's preferable to declare possible types of args as a comment above this function.
BTW: Why is it necessary to accept two types of arg for PersistantAction?

[invocation, options] { invocation, ...options }

I think the second format isn't necessary. And also invocation could be added in options of PersistantAction instance.

Good point about the args comment.

Why is it necessary to accept two types of arg for PersistantAction?

Here I just followed the issue requirements.

And also invocation could be added in options of PersistantAction instance.

An invocation is required in any case, so I don't think invocation should go in options since it's not actually optional.

Here I just followed the issue requirements.

Oh, it was a good thing @Silver-IT brought this up, because I missed this in my review.

This is not what the args look like according to the issue:

[invocation, options] { invocation, ...options }

Rather, they look like this:

[selector, arg1, arg2, arg3] { invocation, ...options }

So to answer @Silver-IT's question: the reason for allowing both types is because it makes the syntax when enqueing invocations simpler.

In most cases you can just pass in the sbpInvocation like so: [selector, arg1, arg2, arg3]

But when you want to adjust the defaults for a specific invocation, you use the second syntax.

~~I'm noticing that this code does not correctly handle the first syntax.~~

EDIT: oops, my mistake, the code seems to correctly handle this.

Silver-IT · 2023-11-05T13:59:44Z

shared/domains/chelonia/persistent-actions.js

+      // TODO: find a cleaner alternative.
+      this.actionsByID[id].id = id


Why not to add id in the arguments of PersistantAction constructor?

That's a possible choice, but I felt like it was not that great either

Silver-IT · 2023-11-05T14:02:40Z

shared/domains/chelonia/persistent-actions.js

+  'chelonia.persistentActions/_init' (): void {
+    this.actionsByID = Object.create(null)
+    // Necessary for now as _init cannot be async. Becomes true when 'configure' has been called.
+    this.ready = false


I can't find the place to set this.ready true in configure function. Did you miss it by mistake?

Indeed, good catch. BTW I've since replaced it by a direct check for availability of databaseKey

corrideat · 2023-11-05T22:47:18Z

shared/domains/chelonia/persistent-actions.js

+    const anyAttemptLeft = options.maxAttempts > status.failedAttemptsSoFar
+    if (!anyAttemptLeft) status.resolved = true
+    status.nextRetry = anyAttemptLeft && !status.resolved
+      ? new Date(Date.now() + options.retrySeconds * 1e3).toISOString()


Using Date is a bad idea for anything besides maybe logging. The reason is that the date could change arbitrarily. The better alternative is to use a monotonic timer for this, such as performance.now(). I'm not sure exactly what this nextRetry field is used for, exactly, though.

The second issue is the, like for the this.timer assignment below, that 1e3 seems like a pretty low value for a retry

The nextRetry field is actually used for logging as per the issue requirement for 'chelonia.persistentActions/status'

1e3 seems like a pretty low value for a retry

options.retrySeconds is a number of seconds, but we need milliseconds here, hence the 1e3 multiplier

corrideat · 2023-11-05T22:48:41Z

shared/domains/chelonia/persistent-actions.js

+    // Schedule a retry if appropriate.
+    if (status.nextRetry) {
+      // Note: there should be no older active timeout to clear.
+      this.timer = setTimeout(() => this.attempt(), this.options.retrySeconds * 1e3)


1e3 seems like a pretty low value

taoeffect

Nice work @snowteamer. Looks almost ready to merge. Seems like the only thing remaining are the logging out related changes we discussed today.

taoeffect

Great work @snowteamer! Approved & merged! 😄 🎉

snowteamer added 2 commits October 16, 2023 09:30

Fix return value in 'chelonia/db/set'

c45ba09

Add chelonia/persistent-actions.js

421c340

taoeffect requested changes Oct 18, 2023

View reviewed changes

frontend/main.js Outdated Show resolved Hide resolved

shared/domains/chelonia/persistent-actions.js Outdated Show resolved Hide resolved

shared/domains/chelonia/persistent-actions.js Outdated Show resolved Hide resolved

taoeffect reviewed Oct 18, 2023

View reviewed changes

taoeffect reviewed Oct 19, 2023

View reviewed changes

shared/domains/chelonia/persistent-actions.js Outdated Show resolved Hide resolved

Apply review

21131ee

taoeffect requested changes Oct 24, 2023

View reviewed changes

taoeffect requested a review from corrideat October 24, 2023 17:43

taoeffect reviewed Oct 24, 2023

View reviewed changes

taoeffect mentioned this pull request Oct 25, 2023

Fix error in chatroom after leaving a group #1718

Closed

snowteamer added 2 commits October 26, 2023 09:46

Make '/load' call '/retryAll'

637e5a6

Make trySBP async

d357522

taoeffect requested changes Oct 26, 2023

View reviewed changes

Emit events on action success and failure

6192b3a

snowteamer force-pushed the retriable-chelonia-actions branch from d12f7aa to 6192b3a Compare October 30, 2023 17:11

Silver-IT requested changes Nov 5, 2023

View reviewed changes

corrideat reviewed Nov 5, 2023

View reviewed changes

Use .checkDatabaseKey rather than .ready

4ca17b0

taoeffect requested changes Nov 8, 2023

View reviewed changes

Pause and unload persistent actions on logout

9d14577

snowteamer changed the title ~~WIP - Support retriable Chelonia actions~~ Support retriable Chelonia actions Nov 9, 2023

snowteamer added 2 commits November 18, 2023 23:20

Add persistent-actions.test.js

4222f8f

Improve persistent action tests

3b57a3c

taoeffect approved these changes Dec 7, 2023

View reviewed changes

taoeffect merged commit bba9016 into okTurtles:master Dec 7, 2023
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support retriable Chelonia actions #1768

Support retriable Chelonia actions #1768

snowteamer commented Oct 16, 2023 •

edited

Loading

taoeffect left a comment

taoeffect Oct 18, 2023

snowteamer Oct 19, 2023

taoeffect left a comment •

edited

Loading

taoeffect Oct 24, 2023

snowteamer Oct 24, 2023

taoeffect Oct 24, 2023

taoeffect left a comment

taoeffect Oct 26, 2023

Silver-IT left a comment

Silver-IT Nov 5, 2023

snowteamer Nov 7, 2023

Silver-IT Nov 5, 2023

snowteamer Nov 7, 2023

taoeffect Nov 9, 2023 •

edited

Loading

Silver-IT Nov 5, 2023

snowteamer Nov 7, 2023

Silver-IT Nov 5, 2023

snowteamer Nov 7, 2023

corrideat Nov 5, 2023

snowteamer Nov 7, 2023 •

edited

Loading

corrideat Nov 5, 2023

snowteamer Nov 7, 2023

taoeffect left a comment

taoeffect left a comment

		this.options.successInvocationSelector &&
		await this.trySBP([this.options.successInvocationSelector, result])

		// TODO: find a cleaner alternative.
		this.actionsByID[id].id = id

Support retriable Chelonia actions #1768

Support retriable Chelonia actions #1768

Conversation

snowteamer commented Oct 16, 2023 • edited Loading

Summary of changes:

taoeffect left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoeffect left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoeffect left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Silver-IT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoeffect Nov 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snowteamer Nov 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taoeffect left a comment

Choose a reason for hiding this comment

taoeffect left a comment

Choose a reason for hiding this comment

snowteamer commented Oct 16, 2023 •

edited

Loading

taoeffect left a comment •

edited

Loading

taoeffect Nov 9, 2023 •

edited

Loading

snowteamer Nov 7, 2023 •

edited

Loading