Race Conditions in Timer Management System #4

New Issue

saulteafarmer · 2025-05-26T16:50:35Z

saulteafarmer commented

2025-05-26 16:50:35 +00:00

The Orangemart plugin has multiple overlapping timer systems that can create race conditions, leading to inconsistent payment processing, duplicate operations, and memory leaks.

Root Cause Analysis

1. Dual Timer Systems Checking Same Invoices

Global Timer (Line ~390):

timer.Every(checkIntervalSeconds, CheckPendingInvoices);

Per-Invoice Timer (Line ~610):

timer.Once(invoiceTimeoutSeconds, () => {
    if (pendingInvoices.Contains(pendingInvoice)) {
        pendingInvoices.Remove(pendingInvoice);
        // ... expiry logic
    }
});

Individual Payment Status Timer (Line ~680):

Timer timerInstance = null;
timerInstance = timer.Repeat(checkIntervalSeconds, maxRetries, () => {
    CheckInvoicePaid(paymentHash, isPaid => {
        // ... payment checking logic
    });
});

2. Race Condition Scenarios

Scenario A: Concurrent Invoice Removal

Global timer finds invoice is paid → removes from pendingInvoices
Expiry timer fires simultaneously → tries to remove same invoice
Result: Duplicate removal attempts, inconsistent logging

Scenario B: Retry Counter Conflicts

Global timer increments retryCounts[paymentHash]
Individual payment timer also increments same counter
Result: Incorrect retry counts, premature expiration

Scenario C: Memory Leak from Orphaned Timers

Invoice gets paid and removed by global timer
Individual payment timer continues running (not destroyed)
Result: Orphaned timers consuming resources

3. Specific Code Issues

Issue 1: Non-Atomic Operations

// In CheckPendingInvoices() - Lines ~475-520
if (isPaid) {
    pendingInvoices.Remove(invoice); // ← Not thread-safe
    // ... reward logic
    retryCounts.Remove(invoice.RHash); // ← Race condition
}

Issue 2: Timer Lifecycle Management

// In StartPaymentStatusCheck() - Lines ~680-700
Timer timerInstance = null;
timerInstance = timer.Repeat(checkIntervalSeconds, maxRetries, () => {
    CheckInvoicePaid(paymentHash, isPaid => {
        if (isPaid) {
            callback(true);
            timerInstance.Destroy(); // ← May not execute if other timer processes first
        }
    });
});

Issue 3: Inconsistent State Management

// Multiple places modify the same data structures concurrently:
pendingInvoices.Remove(invoice);           // Global timer
pendingInvoices.Remove(pendingInvoice);    // Expiry timer
retryCounts[paymentHash]++;                // Both timers

Impact

❌ Duplicate processing of the same payment
❌ Inconsistent retry counts leading to premature timeouts
❌ Memory leaks from orphaned timers
❌ Race conditions causing unpredictable behavior
❌ Double refunds or double rewards in edge cases
❌ Inconsistent logging and transaction records

Steps to Reproduce

Player initiates multiple purchases rapidly (/buyblood 10, /buyvip, etc.)
Pay some invoices while letting others timeout
Under high load, race conditions become more likely
Check logs for duplicate entries or inconsistent retry counts

Proposed Fix

Solution 1: Single Centralized Timer System

Remove the individual payment timers and rely only on the global timer:

private void OnServerInitialized()
{
    // Only use ONE timer system
    timer.Every(checkIntervalSeconds, CheckPendingInvoices);
    // Remove individual payment timers
}

private void CheckPendingInvoices()
{
    foreach (var invoice in pendingInvoices.ToList()) // Create copy to avoid modification during iteration
    {
        // Check if invoice has expired
        if ((DateTime.UtcNow - invoice.CreatedAt).TotalSeconds > invoiceTimeoutSeconds)
        {
            ProcessExpiredInvoice(invoice);
            continue;
        }

        // Check payment status
        CheckInvoicePaid(invoice.RHash, isPaid =>
        {
            if (isPaid)
            {
                ProcessPaidInvoice(invoice);
            }
            else
            {
                ProcessPendingInvoice(invoice);
            }
        });
    }
}

Solution 2: Thread-Safe Operations

Add locking mechanisms to prevent concurrent modifications:

private readonly object _pendingInvoicesLock = new object();
private readonly object _retryCountsLock = new object();

private void RemoveInvoiceSafely(PendingInvoice invoice)
{
    lock (_pendingInvoicesLock)
    {
        if (pendingInvoices.Contains(invoice))
        {
            pendingInvoices.Remove(invoice);
        }
    }
    
    lock (_retryCountsLock)
    {
        retryCounts.Remove(invoice.RHash);
    }
}

Solution 3: State Management Cleanup

Ensure proper cleanup and prevent orphaned resources:

private void Unload()
{
    // Destroy all active timers
    foreach (var timerInstance in activeTimers)
    {
        timerInstance?.Destroy();
    }
    
    pendingInvoices.Clear();
    retryCounts.Clear();
    activeTimers.Clear();
}

Environment

Plugin Version: 0.3.0
Oxide Framework: Multi-threaded environment
Concurrency Level: Multiple players making simultaneous purchases

Severity

🟠 Medium-High - Can cause unpredictable behavior and resource leaks under load

Additional Notes

This issue becomes more pronounced with:

Higher player counts
Faster checkIntervalSeconds settings
Multiple simultaneous purchases
Network latency causing delayed API responses

The plugin needs a unified, thread-safe timer management system to ensure reliable payment processing.

The Orangemart plugin has **multiple overlapping timer systems** that can create race conditions, leading to inconsistent payment processing, duplicate operations, and memory leaks. ## Root Cause Analysis ### 1. **Dual Timer Systems Checking Same Invoices** **Global Timer** (Line ~390): ```csharp timer.Every(checkIntervalSeconds, CheckPendingInvoices); ``` **Per-Invoice Timer** (Line ~610): ```csharp timer.Once(invoiceTimeoutSeconds, () => { if (pendingInvoices.Contains(pendingInvoice)) { pendingInvoices.Remove(pendingInvoice); // ... expiry logic } }); ``` **Individual Payment Status Timer** (Line ~680): ```csharp Timer timerInstance = null; timerInstance = timer.Repeat(checkIntervalSeconds, maxRetries, () => { CheckInvoicePaid(paymentHash, isPaid => { // ... payment checking logic }); }); ``` ### 2. **Race Condition Scenarios** #### **Scenario A: Concurrent Invoice Removal** 1. **Global timer** finds invoice is paid → removes from `pendingInvoices` 2. **Expiry timer** fires simultaneously → tries to remove same invoice 3. **Result**: Duplicate removal attempts, inconsistent logging #### **Scenario B: Retry Counter Conflicts** 1. **Global timer** increments `retryCounts[paymentHash]` 2. **Individual payment timer** also increments same counter 3. **Result**: Incorrect retry counts, premature expiration #### **Scenario C: Memory Leak from Orphaned Timers** 1. Invoice gets paid and removed by global timer 2. Individual payment timer continues running (not destroyed) 3. **Result**: Orphaned timers consuming resources ### 3. **Specific Code Issues** #### **Issue 1: Non-Atomic Operations** ```csharp // In CheckPendingInvoices() - Lines ~475-520 if (isPaid) { pendingInvoices.Remove(invoice); // ← Not thread-safe // ... reward logic retryCounts.Remove(invoice.RHash); // ← Race condition } ``` #### **Issue 2: Timer Lifecycle Management** ```csharp // In StartPaymentStatusCheck() - Lines ~680-700 Timer timerInstance = null; timerInstance = timer.Repeat(checkIntervalSeconds, maxRetries, () => { CheckInvoicePaid(paymentHash, isPaid => { if (isPaid) { callback(true); timerInstance.Destroy(); // ← May not execute if other timer processes first } }); }); ``` #### **Issue 3: Inconsistent State Management** ```csharp // Multiple places modify the same data structures concurrently: pendingInvoices.Remove(invoice); // Global timer pendingInvoices.Remove(pendingInvoice); // Expiry timer retryCounts[paymentHash]++; // Both timers ``` ## Impact - ❌ **Duplicate processing** of the same payment - ❌ **Inconsistent retry counts** leading to premature timeouts - ❌ **Memory leaks** from orphaned timers - ❌ **Race conditions** causing unpredictable behavior - ❌ **Double refunds** or **double rewards** in edge cases - ❌ **Inconsistent logging** and transaction records ## Steps to Reproduce 1. Player initiates multiple purchases rapidly (`/buyblood 10`, `/buyvip`, etc.) 2. Pay some invoices while letting others timeout 3. Under high load, race conditions become more likely 4. Check logs for duplicate entries or inconsistent retry counts ## Proposed Fix ### **Solution 1: Single Centralized Timer System** Remove the individual payment timers and rely only on the global timer: ```csharp private void OnServerInitialized() { // Only use ONE timer system timer.Every(checkIntervalSeconds, CheckPendingInvoices); // Remove individual payment timers } private void CheckPendingInvoices() { foreach (var invoice in pendingInvoices.ToList()) // Create copy to avoid modification during iteration { // Check if invoice has expired if ((DateTime.UtcNow - invoice.CreatedAt).TotalSeconds > invoiceTimeoutSeconds) { ProcessExpiredInvoice(invoice); continue; } // Check payment status CheckInvoicePaid(invoice.RHash, isPaid => { if (isPaid) { ProcessPaidInvoice(invoice); } else { ProcessPendingInvoice(invoice); } }); } } ``` ### **Solution 2: Thread-Safe Operations** Add locking mechanisms to prevent concurrent modifications: ```csharp private readonly object _pendingInvoicesLock = new object(); private readonly object _retryCountsLock = new object(); private void RemoveInvoiceSafely(PendingInvoice invoice) { lock (_pendingInvoicesLock) { if (pendingInvoices.Contains(invoice)) { pendingInvoices.Remove(invoice); } } lock (_retryCountsLock) { retryCounts.Remove(invoice.RHash); } } ``` ### **Solution 3: State Management Cleanup** Ensure proper cleanup and prevent orphaned resources: ```csharp private void Unload() { // Destroy all active timers foreach (var timerInstance in activeTimers) { timerInstance?.Destroy(); } pendingInvoices.Clear(); retryCounts.Clear(); activeTimers.Clear(); } ``` ## Environment - **Plugin Version**: 0.3.0 - **Oxide Framework**: Multi-threaded environment - **Concurrency Level**: Multiple players making simultaneous purchases ## Severity 🟠 **Medium-High** - Can cause unpredictable behavior and resource leaks under load ## Additional Notes This issue becomes more pronounced with: - Higher player counts - Faster `checkIntervalSeconds` settings - Multiple simultaneous purchases - Network latency causing delayed API responses The plugin needs a **unified, thread-safe timer management system** to ensure reliable payment processing.

saulteafarmer added reference main

2025-06-19 20:49:53 +00:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: rustysats/orangemart.cs#4