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

feat: idle syncing #269

Merged
merged 4 commits into from
Jan 24, 2025
Merged

feat: idle syncing #269

merged 4 commits into from
Jan 24, 2025

Conversation

TalDerei
Copy link
Contributor

@TalDerei TalDerei commented Jan 20, 2025

uses chrome.alarms API to restart the service worker after extended periods of inactivity and make sync progress over time without any frontend providers or the extension being opened. This ensures the chain is already synced when the user returns to penumbra after that extended period of not using it. We aptly name this 'idle syncing'.

cc @hdevalence


when adding an event listener to log details as the alarm triggers (set periodInMinutes to 1 minute) and inspecting the service worker immediately after manually terminating it with chrome.runtime.reload(), it appears to working at first glance. However, we should contemplate potential pitfalls with this approach.

Screenshot 2025-01-20 at 3 19 24 PM

see #269 (comment) for an example

@TalDerei TalDerei self-assigned this Jan 20, 2025
@TalDerei TalDerei marked this pull request as draft January 20, 2025 22:26
@TalDerei TalDerei changed the title init background syncing idle syncing Jan 20, 2025

// https://developer.chrome.com/docs/extensions/reference/api/alarms
void chrome.alarms.create('blockSync', {
periodInMinutes: 30,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dynamically adjustable param

Copy link

Choose a reason for hiding this comment

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

FWIW, I think it will have to be much shorter than this (between 20 seconds and 4 minutes)

https://developer.chrome.com/docs/extensions/develop/concepts/service-workers/lifecycle

Screenshot 2025-01-21 at 7 52 48 AM

Copy link

Choose a reason for hiding this comment

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

I don't totally understand what causes the service worker to terminate, if it's long-polling the compact block stream or real idling.

Copy link
Contributor Author

@TalDerei TalDerei Jan 21, 2025

Choose a reason for hiding this comment

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

FWIW, I think it will have to be much shorter than this (between 20 seconds and 4 minutes)

alarms can now be set to a minimum period of 30s to match the service worker lifecycle: "starting in Chrome 120, the minimum alarm interval has been reduced from 1 minute to 30 seconds"

I've been testing locally with 1 minute.

@hdevalence
Copy link
Contributor

One test that comes to mind is, with each version of the code (pre and post change):

  1. ensure no frontends or any other tabs are open
  2. clear cache to reset view state
  3. immediately quit chrome (to kill the service worker)
  4. start chrome, do not open any tabs or frontends, wait 1h
  5. open minifront
  6. observe the current sync progress on minifront opening

(a custom chrome profile may be helpful here)

If background syncing is working we expect the post-change test to have significantly higher sync height at (6). Exactly how much is less important — as long as it's increasing we have reason to believe it will catch up and stay up to date.

@TalDerei
Copy link
Contributor Author

TalDerei commented Jan 21, 2025

per the docs: "starting in Chrome 117, the number of active alarms is limited to 500. Once this limit is reached, chrome.alarms.create() will fail. When using a callback, chrome.runtime.lastError will be set. When using promises, the promise will be rejected."

seems like a necessary extension to this might be to periodically clear all alarms using chrome.alarms.clearAll() in an event listener.

@TalDerei TalDerei changed the title idle syncing feat: idle syncing Jan 21, 2025
@turbocrime
Copy link
Contributor

turbocrime commented Jan 21, 2025

i considered alarms unnecessary because every content script (meaning every page load) will attempt to message the extension background with an init message, to connect pre-authorized origins. this message should wake up the extension background.

the init process changed a lot and things are scoped differently now, so probably at this point it just wakes up enough scope to handle the message listener callback, which doesn't access services. meaning service init is actually skipped.

the simplest way to handle this with alarms is, create an alarm with periodInMinutes, and a listener, in service-worker.ts. call CRSessionManager.init in the callback, and leave the current call where it is, so it also starts up immediately without waiting for the alarm.

CRSessionManager maintains a global singleton state that should prevent creation of multiple session managers. you should confirm this works the way i expect. if it doesn't, maybe you can assign the return value of CRSessionManager.init to a top-level variable.

it should be possible to refactor this in a way that avoids alarms and any new alarm permission, by refactoring the init message handler to require the return value of CRSessionManager.init to be in scope, or to execute CRSessionManager.init with a similar technique. the simplest way to do that would just mean relocating the content of listeners/content-script/init.ts into service-worker.ts so a top-level value can be captured in its callback's closure.

@TalDerei
Copy link
Contributor Author

TalDerei commented Jan 22, 2025

hey!!

i considered alarms unnecessary because every content script (meaning every page load) will attempt to message the extension background with an init message, to connect pre-authorized origins. this message should wake up the extension background.

...

it should be possible to refactor this in a way that avoids alarms and any new alarm permission, by refactoring the init message handler to require the return value of CRSessionManager.init to be in scope, or to execute CRSessionManager.init with a similar technique. the simplest way to do that would just mean relocating the content of listeners/content-script/init.ts into service-worker.ts so a top-level value can be captured in its callback's closure.

so the CRSessionClient is used by content scripts that's injected by the extension and run in the context of the web page, and the content scripts initiate connections to the manager, which causes chrome to queue the connection request while the service worker initializes.

but, the use case we're targeting is when the user doesn't open the extension and load a frontend for prolonged periods, which wouldn't trigger any content script init messages, and thus make no sync progress while the user's been offline.

the simplest way to handle this with alarms is, create an alarm with periodInMinutes, and a listener, in service-worker.ts. call CRSessionManager.init in the callback, and leave the current call where it is, so it also starts up immediately without waiting for the alarm.

thought the same thing initially, but in practice just creating the alarm is sufficient to wake up the service worker and kickstarting init - seems we don't need to put CRSessionManager.init inside a listener.

CRSessionManager maintains a global singleton state that should prevent creation of multiple session managers. you should confirm this works the way i expect. if it doesn't, maybe you can assign the return value of CRSessionManager.init to a top-level variable.

yeah, CRSessionManager's singleton pattern prevents multiple session managers, so subsequent calls just return the existing sessions map.


alarms are a nice idiomatic solution for periodically waking up the service worker. I do wonder however how the additional permissions will impact chrome store reviews.

@turbocrime
Copy link
Contributor

turbocrime commented Jan 22, 2025

an alarm is definitely the idiomatic way to do it. i expect reviewers may take issue with a high frequency.

worker kill conditions seem deliberately under-documented, and the conditions explained (pending fetch, pending promise resolution) are definitely something that the present behavior should meet. so at some point it's killed despite meeting the no-kill conditions.

i would investigate how long the worker runs if it's activated by the alarm but conducts no other activity, and experiment with setting an alarm

  • just before that time
  • just after that time
  • much after that time

to identify the lowest effective frequency. even if there is a significant kill time between, if it can catch up before the next kill, the frequency can be reduced.

then document and justify the alarm reason as much as possible for the reviewers and users.

@TalDerei
Copy link
Contributor Author

TalDerei commented Jan 23, 2025

One test that comes to mind is, with each version of the code (pre and post change):

@hdevalence replicated the procedure on my older 2020 m1 pro – the browser cache was cleared, and neither any frontends nor the extension were opened.

Screen.Recording.2025-01-22.at.4.18.52.PM.mov

after starting chrome and waiting 10 minutes, it synced the entire chain (with alarms firing every minute). Previously, it would have just blocked on genesis sync during service worker init without making any progress.

Screen.Recording.2025-01-22.at.4.29.37.PM.mov

BETA TESTERS will replicate this waiting 1 hour instead.

@hdevalence
Copy link
Contributor

Well, if the alarm approach solves the sync problem, that seems like a good improvement, and we could continue to refine it over time. A 30 minute alarm seems low frequency but we can find out if there are objections.

@TalDerei
Copy link
Contributor Author

TalDerei commented Jan 23, 2025

Well, if the alarm approach solves the sync problem, that seems like a good improvement, and we could continue to refine it over time. A 30 minute alarm seems low frequency but we can find out if there are objections.

an overnight test with a 30 minute frequency successfully kept up with the chain, and it was fully synced when I checked this morning. We just need to avoid falling behind, so if there's ~360 blocks / 30 min (current block times), then we only need to process ~12 blocks / sec if we assume the service worker has access to 30 seconds of execution every 30 min.


Screenshot 2025-01-23 at 7 36 31 AM

@TalDerei TalDerei marked this pull request as ready for review January 23, 2025 18:23
@TalDerei TalDerei requested a review from a team January 23, 2025 18:23
@TalDerei
Copy link
Contributor Author

TalDerei commented Jan 23, 2025

note to reviewers: this will be staged in #263, cut as part of the next prax v15.1 minor point release, and shared with beta testers.


chrome.alarms.onAlarm.addListener(alarm => {
if (alarm.name === 'blockSync') {
console.log('Block sync alarm fired, service worker restarting');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for checking beta reviewer logs

Copy link

Choose a reason for hiding this comment

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

This shows up in the service worker console right? not the main web one

Copy link
Contributor Author

@TalDerei TalDerei Jan 23, 2025

Choose a reason for hiding this comment

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

logs display in both since chrome forwards the service worker logs to the main extension console

chrome forwards the service worker logs to the main extension console, but it won't show up in the web browser's console.

Copy link

@erwanor erwanor left a comment

Choose a reason for hiding this comment

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

Cool, good work. I think we should get this in the hands of beta testers asap. The wake up frequency seems polite and reasonable so I hope Chrome review won't object.

@TalDerei TalDerei merged commit 0a6edc7 into main Jan 24, 2025
3 checks passed
@TalDerei TalDerei deleted the background-syncing branch January 24, 2025 00:27
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.

4 participants