fix: reduce sync worker ticker period to 5m to prevent threshold drift at minimum sync interval

[Pratham-Mishra04]

Summary

The sync worker ticker period was set to 1 hour, which created a subtle scheduling bug: when pricingSyncInterval is set near the minimum supported value, the few seconds a sync takes to complete causes the next ticker wake-up to land just under the elapsed-time threshold, effectively doubling the actual sync cadence. Reducing the ticker period to 5 minutes ensures the check granularity stays well below the minimum supported pricingSyncInterval, preventing ticker drift from defeating the threshold check.

Changes

• Reduced syncWorkerTickerPeriod from 1 hour to 5 minutes so that ticker drift (caused by sync execution time) does not push the next wake-up just under the pricingSyncInterval threshold and inadvertently double the effective sync interval.
• Updated the scheduling model comment to accurately describe the relationship between the ticker period and pricingSyncInterval, removing the outdated note that implied the 1-hour ticker was a hard lower bound on sync frequency.

Type of change

• Bug fix
• Feature
• Refactor
• Documentation
• Chore/CI

Affected areas

• Core (Go)
• Transports (HTTP)
• Providers/Integrations
• Plugins
• UI (React)
• Docs

How to test

go test ./framework/modelcatalog/...

Set pricingSyncInterval to a value near MinimumPricingSyncIntervalSec and verify that syncs occur at the expected cadence without doubling.

Screenshots/Recordings

N/A

Breaking changes

• Yes
• No

Related issues

N/A

Security considerations

None.

Checklist

• I read docs/contributing/README.md and followed the guidelines
• I added/updated tests where appropriate
• I updated documentation where needed
• I verified builds succeed (Go and UI)
• I verified the CI pipeline passes locally if applicable

Summary by CodeRabbit

• Bug Fixes

  • Increased frequency of pricing synchronization checks from hourly intervals to 5-minute intervals, improving update timeliness and overall system reliability across different configurations.

• Documentation

  • Updated internal documentation clarifying the pricing synchronization scheduling mechanism and how timing parameters influence sync behavior.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 36801ba8-ab02-44cd-9af1-544530a9d3f7

📥 Commits

Reviewing files that changed from the base of the PR and between bd9d432 and dbc0e68.

📒 Files selected for processing (2)

• framework/modelcatalog/config.go
• framework/modelcatalog/sync.go

---

📝 Walkthrough

Walkthrough

The PR reduces the background sync worker ticker period from 1 hour to 5 minutes and updates accompanying documentation. The change clarifies that the shorter interval is intentionally maintained well below the minimum pricing sync threshold to prevent ticker drift from effectively changing the sync cadence when the configured sync interval is set near the minimum.

Changes

Sync Worker Ticker Period Update

Layer / File(s)	Summary
Sync worker ticker period and documentation framework/modelcatalog/config.go, framework/modelcatalog/sync.go	syncWorkerTickerPeriod constant is reduced from 1 hour to 5 minutes. Comments in both files are updated to explain that the ticker period is intentionally kept well below MinimumPricingSyncIntervalSec to avoid ticker drift from undermining the minimum-threshold check when pricingSyncInterval is set near the minimum.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A ticker tocks five minutes bright,
Where once an hour held the night,
No drift shall sneak when thresholds creep,
The worker stays awake, its vigil deep! ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The pull request title clearly and concisely summarizes the main change: reducing the sync worker ticker period from 1 hour to 5 minutes to prevent threshold drift at the minimum sync interval.
Description check	✅ Passed	The pull request description covers all major template sections including summary, changes, type of change, affected areas, how to test, and breaking changes. Minor checklist items are unmarked but the description itself is complete and well-structured.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch 06-03-fix_pricing_tick_miss_fixes

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[Pratham-Mishra04]

• fix: reduce sync worker ticker period to 5m to prevent threshold drift at minimum sync interval #4023 👈 (View in Graphite)
• dev

This stack of pull requests is managed by Graphite. Learn more about stacking.

[greptile-apps]

Confidence Score: 4/5

Safe to merge — the change is a single constant reduction with no logic modifications and no risk of regression in the sync path itself.

The fix correctly addresses the described drift: reducing the ticker period from 1h to 5m means the elapsed-time check will fire on the first tick after the 3600-second minimum elapses rather than missing it by a few seconds and waiting a full extra cycle. The only gap is that the key invariant (ticker period must stay well below the minimum sync interval) is documented only in comments and is not enforced by any test, leaving it open to silent regression.

No files require special attention; both changed files are minimal and correct.

Important Files Changed

Filename	Overview
framework/modelcatalog/config.go	Reduces syncWorkerTickerPeriod from 1h to 5m and updates the constant's comment to describe the granularity-vs-minimum-interval invariant.
framework/modelcatalog/sync.go	Updates the scheduling model comment in startSyncWorker to reflect the corrected ticker period and remove the stale "hard lower bound" language; no logic changes.

_{Reviews (1): Last reviewed commit: "fix: pricing tick miss fixes" | Re-trigger Greptile}

[Pratham-Mishra04]

@coderabbitai full-review

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Get the PR diff
gh pr view 4023 --repo maximhq/bifrost --json files,title,body

# Find relevant files
fd -e go . framework/modelcatalog/

Unrecoverable error: Failed to clone repository

---

🏁 Script executed:

#!/bin/bash
# Fetch PR diff via GitHub API
gh pr diff 4023 --repo maximhq/bifrost

Unrecoverable error: Stream initialization permanently failed: Received unknown message on Sandbox stream

---

@Pratham-Mishra04 — I was unable to clone the repository or access the raw diff directly, but based on the PR description and objectives I can provide a thorough review of the change.

---

PR Review: fix: reduce sync worker ticker period to 5m to prevent threshold drift

Summary

The change is minimal and well-reasoned: the syncWorkerTickerPeriod constant is reduced from 1h to 5m in framework/modelcatalog. This prevents ticker-drift from causing the elapsed-time threshold check to always land just under pricingSyncInterval, which would silently double the effective sync cadence.

---

✅ Correctness

The root cause analysis is accurate. A ticker-based scheduler that checks elapsed time against a threshold will misbehave when:

• tickerPeriod ≈ pricingSyncInterval (or tickerPeriod > pricingSyncInterval), or
• the sync itself takes non-trivial time, nudging the next tick to just miss the threshold.

Reducing syncWorkerTickerPeriod to 5m increases check granularity significantly, so drift of a few seconds never causes a missed cycle — as long as MinimumPricingSyncIntervalSec is well above 5 minutes. Could you confirm the value of MinimumPricingSyncIntervalSec? If it's, say, 6 minutes, the fix is correct but barely adequate; if it's ≥ 30 minutes, the 5m choice has ample headroom.

---

⚠️ Considerations

1. Ticker overhead: Waking every 5 minutes instead of every hour is a 12× increase in timer-callback frequency. For a single goroutine doing only a time comparison, this is negligible in practice. No concern here.

2. Missing test coverage: The PR checklist marks "I added/updated tests where appropriate" as unchecked. Given that this fix targets a specific scheduling edge case, a unit test that:

   • mocks time or uses a very short pricingSyncInterval near the minimum, and
   • asserts syncs occur at the expected cadence (not doubled),

   would make this regression-proof. Is there an existing time-mock or fast-clock pattern in framework/modelcatalog/ that could be leveraged?

3. Comment update: The PR description mentions that the outdated comment implying a 1-hour hard lower bound was removed. This is a good housekeeping change that should accompany any constant rename/re-value.

4. UpdateSyncConfig path: The model catalog supports runtime config reload via UpdateSyncConfig. If someone updates pricingSyncInterval at runtime to a value near the minimum, the ticker period fix applies immediately without restart — which is the correct behavior. No additional handling needed here.

---

🔍 Minor Nit

Consider naming the constant more expressively to convey its purpose relative to pricingSyncInterval, e.g.:

// syncWorkerTickerPeriod must be significantly smaller than MinimumPricingSyncIntervalSec
// to prevent ticker drift from doubling the effective sync cadence.
syncWorkerTickerPeriod = 5 * time.Minute

This comment pattern ties the constant's value to the invariant it must uphold, making it self-documenting for future maintainers.

---

Verdict

The fix is correct, low-risk, and well-scoped. The primary ask before merge is clarity on MinimumPricingSyncIntervalSec to confirm 5m provides adequate headroom, and optionally a regression test for the scheduling logic.

✅ Actions performed

Full review triggered.

[akshaydeo]

Merge activity

• Jun 3, 9:46 PM UTC: A user started a stack merge that includes this pull request via Graphite.
• Jun 3, 9:46 PM UTC: @akshaydeo merged this pull request with Graphite.