Repository navigation
Conversation
Adds the replication_interval_minutes app setting (1-1440 minutes). Missing values keep the 5 minute default; invalid values fall back to it with a console warning.
dacar1988
changed the title
|
Note 🤖 This is an AI-generated review, requested by @jkuester. Its content and suggestions can be wrong. Please respond with a comment if you have doubts regarding any of the review. A maintainer will also review this PR. 🔍 OpenCodeReview found 1 issue(s) in this PR.
Requirements
Preconditions to confirm
Undisclosed ChangesNone Alternative Approaches
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
github-actions
Bot
removed
the
ai-review
jkuester
left a comment
jkuester
left a comment
There was a problem hiding this comment.
Thanks for the PR @dacar1988!
Could you also open one for https://github.com/medic/cht-docs so that we can capture the config details out on https://docs.communityhealthtoolkit.org/ ?
The main docs updates would be:
building/reference/app-settings/_index.md- Add entry to "Optional Settings" tablebuilding/navigation.md"Synchronization Triggers" → "Periodic Sync" - This line hardcodes "The main application database syncs every 5 minutes." Change it to say 5 minutes is the default and link to the new setting.technical-overview/data/performance/replication.md- This page covers replication tuning (depth, primary contacts, and similar) but never says how often replication runs.- A short "Replication interval" section would fit here, covering the tradeoffs: a shorter interval means fresher data but more server load; a longer interval could help ease CPU pressure for deployments with lots of users.
- Also add the app-settings page to its relatedContent.
| const SYNC_INTERVAL = 5 * 60 * 1000; // 5 minutes | ||
| const DEFAULT_SYNC_INTERVAL = 5 * 60 * 1000; // 5 minutes | ||
| const SYNC_INTERVAL_SETTING = 'replication_interval_minutes'; | ||
| const MIN_SYNC_INTERVAL_MINUTES = 1; |
There was a problem hiding this comment.
Lets just use the DEFAULT_SYNC_INTERVAL as the minimum. All of the users syncing puts a non-trivial amount of load on the server, so there are real problems with syncing more frequently (and few advantages).
There was a problem hiding this comment.
Done, the minimum is now the 5-minute default. Thanks.
| } | ||
|
|
||
| await this.migrateDb(); | ||
| await this.loadSyncInterval(); |
There was a problem hiding this comment.
The whole app automatically reloads whenever the settings doc changes. So, it should be safe to grab and cache the configured replication_interval_minutes value when this service is constructed (instead of re-checking here on every sync). Ideally let's return the proper sync interval from the loadSyncInterval method (falling back to the default if none is configured) and set this.syncInterval. Then syncInterval can be a readonly constant.
There was a problem hiding this comment.
Done. The interval is now loaded once when the service is created and kept in a readonly property. Since reading the settings is async, it’s a cached promise that both timers wait for.
| let settings; | ||
| try { | ||
| settings = await this.settingsService.get(); | ||
| } catch (err) { | ||
| console.warn('Error reading settings, keeping the current replication interval', err); | ||
| return; | ||
| } |
There was a problem hiding this comment.
We verify the settings exist before starting the app. So, should be safe to assume we get a value back here.
There was a problem hiding this comment.
I kept the catch on purpose. The interval is now read once and cached, so if that one read ever failed, every later sync would fail too until the app is reloaded. With the catch it just falls back to 5 minutes and logs a warning. Happy to remove it if you prefer.
There was a problem hiding this comment.
I appreciate the extra thinking here and concern for the blast radius if this fails. However, the existence of the settings doc is something we explicitly check for before the app starts.
So, this call should always succeed by the time we get here. The majority of the other places in the codebase where we call settingsService.get() we do not catch any failure and I would prefer we do not add it here because it could confuse future developers who will wonder if they need to also be checking for an error.
| if (typeof minutes !== 'number' || !Number.isFinite(minutes) || | ||
| minutes < MIN_SYNC_INTERVAL_MINUTES || minutes > MAX_SYNC_INTERVAL_MINUTES) { |
There was a problem hiding this comment.
No need for the typeof check here since Number.isFinite can handle non-number values.
| if (typeof minutes !== 'number' || !Number.isFinite(minutes) || | |
| minutes < MIN_SYNC_INTERVAL_MINUTES || minutes > MAX_SYNC_INTERVAL_MINUTES) { | |
| if (!Number.isFinite(minutes) || minutes < MIN_SYNC_INTERVAL_MINUTES || minutes > MAX_SYNC_INTERVAL_MINUTES) { |
| const SYNC_INTERVAL_SETTING = 'replication_interval_minutes'; | ||
| const MIN_SYNC_INTERVAL_MINUTES = 1; | ||
| const MAX_SYNC_INTERVAL_MINUTES = 24 * 60; | ||
| const META_SYNC_INTERVAL = 30 * 60 * 1000; // 30 minutes |
There was a problem hiding this comment.
It does not make much sense to be syncing the metadata more often than the actual data. 🤔
I think maybe we should have a this.metaSyncInterval property too. It should me Math.max(META_SYNC_INTERVAL, this.syncInterval);. This way, if the sync interval is configured to me more than 30min, we will also raise the meta sync interval too.
There was a problem hiding this comment.
Done, the metadata interval is now Math.max(META_SYNC_INTERVAL, syncInterval). With 60 minutes, both sync every hour.
Use the five-minute default as the minimum and adjust metadata replication to the configured interval when it exceeds 30 minutes. Cache settings once per service instance, retain a default on read failure, and cover timer initialization and reload behavior.
|
One thing I noticed while testing longer intervals: every local change triggers an upload-only sync, and that restarts the periodic timer (startSyncMedic → resetSyncInterval). With a 10-minute interval and a save every 4 minutes, the app didn’t download anything for 21 minutes. This already happens today with 5 minutes, but it matters more with long intervals. I mentioned it in the docs. Would you like me to change it (e.g. only restart the timer after a sync that downloaded), or leave it for a separate issue? |
|
CI note: ci-integration-all-k3d failed in setup before any test ran (docker pull from Public ECR returned toomanyrequests: Rate exceeded). Could someone re-run it, please? |
jkuester
left a comment
jkuester
left a comment
There was a problem hiding this comment.
every local change triggers an upload-only sync, and that restarts the periodic timer (startSyncMedic → resetSyncInterval).
Woah, great catch! I had not realized the local changes feed triggers an upload-only sync and not a download. I have done some digging on the history of this. Looks like the original implementation of the changes-driven sync from many years ago just triggered a full sync download+upload. But, in 2023 when we updated the replication protocol and added the ability to do the upload-only sync, this got used for the changes-driven sync (which makes sense because the sync is being triggered by a local change and that is what we want to send back to the server). However, the code that reset the sync interval during a changes-driven sync was not updated and the interval continued to get reset after an upload-only sync.
I agree with your assessment that this incongruity becomes a much bigger problem at higher intervals. (Such that it could pretty much prevent changes from getting synced back down to the device automatically during a session).
The only "risk" I can think of with updating the logic to not clear the sync interval for upload-only syncs is that it will result in more download syncs during a user's active session (when changes are being made on the device). The way things are now, any change on the device will reset the interval and so if a user makes 20 changes in an hour, a download sync may never be triggered at all during that hour. If we avoid resetting the interval for upload-only syncs, the we will sync 12 times in that hour (same as if the app was just open and idle). Honestly this just feels like things working as desired (seems undesired that user activity would suppress downloading).
I think in this PR we just need to update the interval reset logic to only trigger during a full sync and to not reset the interval for an upload-only sync. @witash can I get your feedback on this? 🙏 Can you think of any risks I am missing here? Are you concerned with the possible performance impact?
| expect(settingsService.get.callCount).to.equal(1); | ||
| }); | ||
|
|
||
| it('keeps the initial interval until reload, including after quick and forced syncs', async () => { |
There was a problem hiding this comment.
Minor, but I am not really sure we need this as a unit test. The scenario is coherent, but not really necessary to test given how the code is written. (Not really hitting any new logic paths.)
| const DEFAULT_SYNC_INTERVAL = 5 * 60 * 1000; // 5 minutes | ||
| const SYNC_INTERVAL_SETTING = 'replication_interval_minutes'; | ||
| const MIN_SYNC_INTERVAL_MINUTES = DEFAULT_SYNC_INTERVAL / (60 * 1000); | ||
| const MAX_SYNC_INTERVAL_MINUTES = 24 * 60; |
There was a problem hiding this comment.
Been doing some reading around how the setInterval behaves across longer time ranges. As far as I can tell it is actually not based on "wall-clock time", but instead is an internal timer that pauses during sleep (except on Windows...). This is not an issue for a 5m interval (since there is a good chance the user will be actively using the app for >5min). However, for a much longer interval (e.g. 24h) I am worried that the actual realized sync intervals could be much longer (by wall-clock time) than intended. An Android device will normally suspend when not in use (screen has been off for some time). I expect (though have not manually confirmed) this will result in the timer getting paused and then resuming when the device wakes up. If a user only opens their phone for a few hours of the day, then the rest of the time, the timer may be paused. In that case the setInterval might not trigger the sync for weeks.
The incongruity/risk here seems to grow as the actual interval value is raised. So, I think the best thing to do is set the actual max at 60m. That is still +10x the current limit, which gives admins some options. But it is low enough that we can expect it to actually be triggered in normal usage conditions. To properly support longer sync intervals, I think we would need to consider an alternative approach that would leverage wall-clock time instead.
| const MAX_SYNC_INTERVAL_MINUTES = 24 * 60; | |
| const MAX_SYNC_INTERVAL_MINUTES = 60; |
There was a problem hiding this comment.
😬 I did already merge the docs PR with the 24h max. So, if we do change this value, we will need another docs PR to correct that....
There was a problem hiding this comment.
Done, the max is now 60. Docs follow-up: medic/cht-docs#2271.
Remove the settings read fallback and two unnecessary tests, as requested in review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Description
The webapp syncs offline users with the server every 5 minutes, and this is hard-coded. This PR adds a global app setting,
replication_interval_minutes, so a deployment can sync less often to reduce server load.How it works
null): 5 minutes, so nothing changes for existing projects.7.5): the periodic sync runs at that interval. The maximum is 60 because timers can pause while a device sleeps, so much longer intervals may not fire in practice.1,4.99,61,"30","abc"…): 5 minutes, plus one console warning naming the setting and the value.DBSyncServiceis created. A changed value is used after the device has downloaded the new settings and the app has been reloaded (the usual "Update available" prompt).Changes after review (thanks @jkuester)
typeofcheck.Math.max(30 min, interval).Testing
db-sync.service.spec.ts(fake timers): the 5 and 60 limits, fractions, invalid values, overlapping first syncs, going online before the setting has loaded, metadata timing, and online-only users. All 76 tests in the file pass, and so donpm run lintandnpm run unit-webapp.chwuser in Chrome. For each value I changed the setting on the server, synced, and reloaded from the "Update available" prompt. I recorded the sync timers the app created and timed the 5- and 10-minute ones in real time:replication_interval_minutes57.510601,4.99,61,"abc","30"Before the reload the app kept the old value, as expected.
Closes #10280
Code review checklist
AI Disclosure: the first version was planned and tested with Claude Code (Opus) and implemented with OpenAI Codex (GPT-6.1 Sol). The review changes were implemented with OpenAI Codex. Claude Code reviewed them and ran the manual test above. Everything was done under my direction, and I reviewed the code and the results.
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.