fix(history): handle loadMarkets failure gracefully

[openoms]

Wrap ccxt loadMarkets() call in try/catch so that a single exchange
being unavailable (e.g. Bitfinex 503 maintenance) returns an error
instead of crashing the entire cron process.

Convert caught error to a string message for UnknownExchangeServiceError
since DomainError constructor expects a string, not an Error object.

Change updatePriceHistory() to continue to the next exchange on failure
instead of short-circuiting the entire cron run. This ensures other
enabled exchanges still update price history normally when one is down.

[Copilot]

Pull request overview

This PR aims to prevent the history cron from crashing when a CCXT exchange’s loadMarkets() call fails (e.g., during an exchange maintenance window), by handling that failure and returning a service error instead.

Changes:

• Wraps client.loadMarkets() in a try/catch and logs failures.
• Returns an UnknownExchangeServiceError when market loading fails so the caller can handle the failure path.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[openoms]

@blink-claw-bot review this pr

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

history/src/app/history/update-price-history.ts:53

• updatePrices() is called even when prices is empty. With knex v3, insert([]) throws (e.g. "Empty .insert() call detected"), which will be logged as an error and treated as a failed exchange update even though it's just a no-op. Add a guard to skip updatePrices() (and log at info/warn) when prices.length === 0.

    const result = await PriceRepository().updatePrices({
      exchange: name,
      base,
      quote,
      prices,
    })

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

updatePriceHistory() no longer returns an ApplicationError on failure paths (all errors are logged and the loop continues), but the signature is still Promise<boolean | ApplicationError>. Consider either updating the return type to Promise<boolean> or returning an error when no exchanges succeed (e.g. track the last error) so callers can reliably detect a completely failed cron run.

[blink-claw-bot]

PR Review: fix(history): handle loadMarkets failure gracefully

Summary

This PR addresses graceful error handling when ccxt.loadMarkets() fails and ensures price history updates continue when individual exchanges are down. The approach is generally sound, but there are several issues that need to be addressed.

✅ Issues Resolved

1. Error message conversion: The listPrices() catch block now properly converts Error objects to strings before passing to UnknownExchangeServiceError
2. Graceful degradation: updatePriceHistory() now continues processing other exchanges when one fails instead of short-circuiting
3. Proper failure indication: When all exchanges fail, the function now returns false instead of true

❌ Issues That Need Attention

1. Return Type Mismatch (Critical)

Problem: The function signature is Promise<boolean | ApplicationError> but the implementation no longer returns ApplicationError on any path.

Current behavior: All error paths now use continue and the function only returns boolean

Fix needed: Either:

• Update the return type to Promise<boolean>, OR
• Keep one path that returns ApplicationError for critical failures (e.g., database connection issues)

Recommendation: Update to Promise<boolean> since the graceful degradation approach is the intended behavior.

2. loadMarkets() Error Handling Location Issue

Problem: The loadMarkets() failure is handled in CcxtExchangeService(), but updatePriceHistory() doesn't handle the returned UnknownExchangeServiceError.

Current flow:

// CcxtExchangeService returns UnknownExchangeServiceError on loadMarkets failure
// But updatePriceHistory calls queryByRange which calls listPrices
// There's no explicit handling of the service creation error

Fix needed: Add error handling after queryByRange to detect if the exchange service failed to initialize.

3. Edge Case: Database Failures Still Continue

Problem: If PriceRepository().updatePrices() fails (e.g., database is down), the function continues to other exchanges. This might be correct behavior, but should be explicitly decided.

Question: Should database failures:

• Continue to next exchange (current behavior), OR
• Fail fast since it affects all exchanges?

4. Timeframe Validation Addition Not Mentioned

Problem: The PR description doesn't mention the new timeframe validation logic added to listPrices():

if (client.timeframes && !(timeframe in client.timeframes)) {
  // ... skip logic
}

Fix needed: Either remove this unrelated change or update the PR description to mention it.

🔍 Code Quality Observations

Good:

• Proper error message conversion (error.message vs String(error))
• Clear variable naming (anyExchangeSucceeded, exchangeFailed)
• Appropriate logging levels and context

Consider:

• The exchangeFailed flag could be eliminated by restructuring the loop (extract price querying to a separate function)
• Consider adding metrics/counters for monitoring exchange success rates

📝 Suggested Fixes

1. Update return type in function signature:

export const updatePriceHistory = async (): Promise<boolean> => {

2. Handle service creation errors in queryByRange or add validation before the range loop:

// Option 1: Check service health before processing
const service = await ExchangeService(exchange)
if (service instanceof Error) {
  baseLogger.warn({ exchange: name, error: service }, "Exchange unavailable, skipping")
  continue
}

3. Update PR description to mention timeframe validation if intentional, or remove it if accidental.

🎯 Verdict: REQUEST CHANGES

The core logic is correct, but the return type mismatch is a breaking change that needs to be addressed. The error handling improvement is valuable and should be merged once the type signature is corrected.

---

🤖 Review by Claude Sonnet 4 via Blink-Claw-Bot

[blink-claw-bot]

PR Review: fix(history): handle loadMarkets failure gracefully

This PR addresses a real pain point - single exchange failures bringing down the entire price history cron job. The approach is sound and follows good error handling principles.

✅ What's Good

Graceful Degradation: The core concept is excellent - continue processing other exchanges when one fails rather than short-circuiting the entire operation.

Proper Error Conversion: Converting the caught error to a string before passing to UnknownExchangeServiceError is correct since DomainError expects a string, not an Error object.

Comprehensive Coverage: Handles failures at both the loadMarkets level and the queryByRange level.

Meaningful Return Value: The anyExchangeSucceeded boolean provides useful feedback about whether the operation had any success.

🔧 Issues to Address

Missing Tests: This PR introduces significant changes to error handling behavior but includes no test coverage. According to our dev standards, "Every PR must include test coverage for additions/modifications — no exceptions, no waiting to be asked."

Required test cases:

1. Test that updatePriceHistory() continues to other exchanges when one fails during loadMarkets
2. Test that updatePriceHistory() continues to other exchanges when one fails during queryByRange
3. Test that CcxtExchangeService properly handles and converts errors from loadMarkets
4. Test the new timeframe validation logic
5. Test that anyExchangeSucceeded correctly reflects partial success scenarios

nit: Timeframe Validation Logic: The new timeframe validation in listPrices is good defensive programming, but it seems unrelated to the main PR goal. Consider splitting this into a separate commit or mentioning it in the PR description.

📝 Minor Style Observations

Consistent Error Logging: Good addition of exchangeId to error log contexts for better debugging.

Early Return Pattern: The if (exchangeFailed) continue pattern is clean and readable.

🧪 Testing Suggestions

// Example test structure for update-price-history.ts
describe('updatePriceHistory with exchange failures', () => {
  it('should continue to other exchanges when loadMarkets fails', async () => {
    // Mock one exchange to fail during setup, another to succeed
    // Verify that anyExchangeSucceeded = true and successful exchange processed
  })
  
  it('should return false when all exchanges fail', async () => {
    // Mock all exchanges to fail
    // Verify that anyExchangeSucceeded = false
  })
})

🎯 Verdict: Request Changes

The logic is sound and addresses the problem well, but the missing test coverage violates our development standards. Please add comprehensive tests covering the new error handling paths before this can be approved.

Once tests are added, this will be a solid improvement to system reliability.

🤖 Review by Claude Sonnet 4 via Blink-Claw-Bot