Skip to content

Fix concurrent stat insertion duplicate key errors log message #5933

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

undaunt
Copy link

@undaunt undaunt commented Jun 18, 2025

⚠️ Please Note: We do not accept all types of pull requests, and we want to ensure we don't waste your time. Before submitting, make sure you have read our pull request guidelines: Pull Request Rules

📋 Overview

  • What problem does this pull request address?

    Users with many monitors (200+) frequently encounter database duplicate key errors when multiple monitors attempt to insert statistics simultaneously. This manifests as Error: Duplicate entry '1-1703123456' for key 'monitor_id' and occurs because the current implementation uses RedBean's R.store() which performs separate SELECT and INSERT/UPDATE operations, creating race conditions when multiple monitors update statistics for the same time period.

  • What features or functionality does this pull request introduce or enhance?

    Implements atomic upsert operations for stat table insertions using database-specific SQL syntax (SQLite ON CONFLICT and MariaDB ON DUPLICATE KEY UPDATE). Also resolves related circular dependency and schema naming issues that prevent the upsert functionality from working correctly.

🔄 Changes

🛠️ Type of change

  • 🐛 Bugfix (a non-breaking change that resolves an issue)

🔗 Related Issues

📄 Checklist

  • 🔍 My code adheres to the style guidelines of this project.
  • ✅ I ran ESLint and other code linters for modified files.
  • 🛠️ I have reviewed and tested my code.
  • 📝 I have commented my code, especially in hard-to-understand areas (e.g., using JSDoc for methods).
  • ⚠️ My changes generate no new warnings.
  • 🔒 I have considered potential security impacts and mitigated risks.
  • 📚 I have read and understood the Pull Request guidelines.

ℹ️ Additional Context

Key Considerations:

  • Design decisions – Used database-specific atomic upsert operations rather than application-level locking to ensure maximum performance and reliability. Falls back to original R.store() behavior if upsert fails, ensuring no data loss.

  • Alternative solutions – Considered application-level mutex/locking but atomic database operations are more efficient and reliable for this use case.

  • Dependencies – Required fixing circular dependency between database.js and uptime-calculator.js, and ensuring Database.dbConfig is properly initialized in all code paths.

Technical Details:

The fix addresses several interconnected issues:

  1. Race conditions: Multiple monitors hitting the same stat table rows simultaneously
  2. Circular dependency: database.js imports UptimeCalculator, but UptimeCalculator needs Database.dbConfig
  3. Missing config initialization: Database.dbConfig wasn't being set when db-config.json is missing
  4. Schema mismatch: RedBean uses camelCase (pingMin) but the actual database schema uses snake_case (ping_min)

Testing performed:

  • High-frequency monitor updates (every 10 seconds)
  • Concurrent database operations with 200+ monitors
  • Both SQLite and MariaDB backends
  • Migration scenarios and fresh installations

Backward compatibility: No breaking changes. If upsert fails for any reason, gracefully falls back to the original R.store() approach with appropriate logging.

undaunt added 2 commits June 17, 2025 21:27
Resolves issue where multiple monitors updating statistics simultaneously
can cause "Duplicate entry" database errors for the same monitor_id and
timestamp combination in stat_hourly and stat_daily tables.

Changes:
- Add database-specific upsert logic for SQLite and MariaDB
- Replace R.store() calls with atomic upsert operations
- Add fallback to original R.store() if upsert fails
- Initialize default values for new stat beans to prevent null conflicts
- Use ON CONFLICT/ON DUPLICATE KEY UPDATE for atomic stat updates

This fix is particularly important for high-volume monitoring scenarios
with 400+ monitors where concurrent heartbeats can trigger race conditions
in the stat insertion process.

Fixes louislam#5357
This commit resolves duplicate key errors that occur when multiple monitors
attempt to insert statistics simultaneously into stat_* tables.

The fix addresses three interconnected issues:

1. **Circular Dependency Resolution**: database.js imports UptimeCalculator at
   module level, but UptimeCalculator needs Database.dbConfig. Fixed by using
   local imports in UptimeCalculator methods to ensure Database.dbConfig is
   properly initialized when accessed.

2. **Database Configuration Initialization**: Database.dbConfig was not set
   in the catch block when db-config.json is missing, causing undefined access
   errors. Fixed by ensuring Database.dbConfig is always set.

3. **Schema Column Naming Mismatch**: RedBean ORM uses camelCase (pingMin/pingMax)
   but Knex migrations create snake_case columns (ping_min/ping_max). Fixed by
   using correct snake_case column names in SQL queries.

4. **Atomic Upsert Operations**: Implemented database-specific upsert logic:
   - SQLite: INSERT ... ON CONFLICT DO UPDATE
   - MariaDB: INSERT ... ON DUPLICATE KEY UPDATE

The solution maintains backward compatibility by falling back to R.store()
when upsert fails, ensuring no data loss while eliminating race conditions
for users with many monitors (200+).

Fixes louislam#5357
@louislam louislam added this to the 2.0.0-beta.4 milestone Jun 18, 2025
@CommanderStorm
Copy link
Collaborator

I don't like the solution here.. feels like a horrible hack.
@louislam do you think adding upserts to https://github.com/louislam/redbean-node would be hard? I have not looked at its internals..

@louislam
Copy link
Owner

louislam commented Jun 18, 2025

@CommanderStorm I afraid that could be hard too, because RedBeanNode underlying is also not using raw sql. It is using knex(...).insert({ ... });.

But before that, I would like to have a simple reproduce steps (or simple PoC), because I am still not fully understand.

In Uptime Kuma, getDailyStatBean is written in singleton pattern, it should always return the same object. In RedBean, R.store() will lock it before insert/update. Insert at the same time doesn't seem possible to happen.

Uptime Kuma's getDailyStatBean():

    async getDailyStatBean(timestamp) {
        if (this.lastDailyStatBean && this.lastDailyStatBean.timestamp === timestamp) {
            return this.lastDailyStatBean;
        }

RedBeanNode's R.store():

    async store(bean: Bean, changedFieldsOnly = true) {
        await bean.beanMeta.lock.acquireAsync();

@louislam
Copy link
Owner

Just checked knex's doc, it has a function for upsert:

https://knexjs.org/guide/query-builder.html#merge

@Ionys320
Copy link
Contributor

Hi @undaunt! Would you mind implementing the knew upsert feature as mentioned by @louislam?

@CommanderStorm CommanderStorm changed the title Fix concurrent stat insertion duplicate key errors Fix concurrent stat insertion duplicate key errors log message Jun 24, 2025
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.

Receiving a trace error: insert into 'stat_hourly' - Duplicate entry '43-1731949200' for key 'stat_hourly_monitor_id_timestamp_unique'
4 participants