Added support for fallback

[RitoBose]

Type of Change

• Bugfix
• New feature
• [ x] Enhancement
• Refactoring
• Dependency updates

Description

Added support for priority List fallback

Additional Changes

• This PR modifies the database schema (database migration added)
• This PR modifies dhall configs/environment variables

Motivation and Context

How did you test it?

Checklist

• I formatted the code and addressed linter errors ./dev/format-all-files.sh
• I reviewed submitted code
• I added unit tests for my changes where possible
• I added a CHANGELOG entry if applicable

Summary by CodeRabbit

• New Features
  • Added a fallback mechanism for pending challan lookups that automatically tries multiple service providers in sequence if a lookup fails.
  • Improves reliability by returning the first successful result and provides clearer failure behavior when all providers are unavailable.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8147d76b-1086-4d88-97a3-c56b2fde8c5b

📥 Commits

Reviewing files that changed from the base of the PR and between 50974b6 and 5a0b00f.

📒 Files selected for processing (1)

• lib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hs

🚧 Files skipped from review as they are similar to previous changes (1)

• lib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hs

---

Walkthrough

This PR adds a provider-based fallback mechanism to the Challan Search interface. The new getPendingChallanCountWithFallback function accepts multiple service providers and iterates through them sequentially, logging each invocation, catching exceptions per provider, and returning the first successful result. It validates that providers are configured and throws an error if none are available or all fail.

Changes

Challan Search Provider Fallback

Layer / File(s)	Summary
Provider fallback implementation lib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hs	Updates imports to include ChallanSearchService and supporting error/utility dependencies, then implements getPendingChallanCountWithFallback, which validates provider list, logs each provider invocation, wraps per-provider calls in exception handlers, and returns the first successful response or throws error on complete failure.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 A fallback so clever, through providers we hop,
Each one gets a try till we find the right stop,
With logging to guide us and errors in hand,
The challan searches now have a backup plan grand! 🚗✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Added support for fallback' is vague and generic, lacking specificity about what fallback is being added or which component is affected.	Improve the title to be more specific, e.g., 'Add pending challan count fallback with provider retry logic' to clearly convey the main change.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
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 unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch signzy/enh

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@lib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hs`:
- Around line 30-36: The fallback path in function go currently discards
provider-specific failures (Left _) and only throws a generic InternalError when
all providers fail; change go to preserve error context by collecting each
provider's error (e.g., capture the Left value as err along with provider
identifier) and propagate them so the final failure includes provider names and
error messages; you can implement this by adding an accumulator parameter to go
(or return an Either with Right res or Left [(Provider, Error)]), replace Left _
-> go rest with Left err -> go rest ((provider, err):acc), and when the list is
exhausted throw InternalError containing the concatenated provider-specific
errors (or log them before throwing) so
getPendingChallanCount/withTryCatch/getServiceConfig and provider identifiers
are included in the final message.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c67b06bf-b989-49ab-b41b-8e6c3ea6e08d

📥 Commits

Reviewing files that changed from the base of the PR and between c72fffc and 50974b6.

📒 Files selected for processing (1)

• lib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hs

[RitikaHanish]

@ClaudeWeb Can you review this PR, by making sure existing principles are being followed also error handling is proper ?

[khuzema786]

I'll fetch the PR diff, description, and comments in parallel to get full context.Let me try using the GitHub MCP tools instead.Now let me get the PR files and read the existing source code for full context.Now let me read the key files in parallel for full context.Let me look at similar fallback patterns in the codebase and how withTryCatch works.Let me look at the Maps Interface fallback to understand the established pattern.Now I have enough context. Let me write the review.

---

PR Review: Added support for fallback (#1349)

Verdict: Request Changes — The feature logic is sound and the structure follows the existing provider pattern, but there is a clear deviation from the established fallback convention in this codebase that needs to be addressed before merging.

---

Warning — Missing error logging on provider failure (Confidence: 95)

File: lib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hs, lines 34–36

The PR's fallback go loop silently discards errors:

case result of
  Left _ -> go rest   -- ❌ error swallowed, nothing logged
  Right res -> pure res

This directly contradicts the established pattern in Kernel.External.Maps.Interface (snapToRoadWithFallback, lines 222–226), which is the canonical fallback reference for this codebase:

Left err -> do
  logError $ "Snap to road Pre Check failed with error : " <> show err <> " - Provider : " <> show preferredProvider
  callSnapToRoadWithFallback restProviders

Without logging, when all providers fail in production, there is no trace of which provider failed or why. The final InternalError "Not able to fetch challan count with all configured providers" gives zero actionable information.

Fix — align with established pattern:

    go [] = throwError $ InternalError "Not able to fetch challan count with all configured providers"
    go (provider : rest) = do
      logDebug $ "Calling getPendingChallanCount for provider: " <> show provider
      result <- withTryCatch "getPendingChallanCount" $ getServiceConfig provider >>= flip getPendingChallanCount req
      case result of
        Left err -> do
          logError $ "getPendingChallanCount failed with error: " <> show err <> " - Provider: " <> show provider
          go rest
        Right res -> pure res

---

Suggestion — Redundant guard before go (Confidence: 75)

getPendingChallanCountWithFallback getServiceConfig providers req = do
  when (null providers) $ throwError $ InternalError "No challan search service provider configured"
  go providers   -- go [] already throws below

The when (null providers) guard fires before go providers, giving a more descriptive message for the config-is-empty case, which is intentional and acceptable. However, snapToRoadWithFallback uses the same pattern and this is consistent — keep it, but note that go [] is only ever reached when all providers have been exhausted, not when the list starts empty. The distinction in error messages is valuable, so this is fine as-is.

---

Checklist items missing from the PR description (Confidence: 85)

The author has not confirmed:

• [ ] Code formatted with ./dev/format-all-files.sh — GHC is -Wall -Werror, any formatting issue will fail CI.
• [ ] No unit test added — the fallback logic (e.g. "falls back to second provider when first throws", "throws when all fail") is testable and should have coverage per the project's testing patterns (lib/mobility-core/test/).

---

What's correct

• Import cleanup is correct: removing Kernel.Utils.Servant.Client (no longer needed directly here) and adding Kernel.Types.Error + Kernel.Utils.Common is consistent with how throwError, InternalError, and logDebug are brought into scope across other interface modules.
• Constraint set on getPendingChallanCountWithFallback is correct — MonadFlow m is needed for withTryCatch, and the remaining constraints mirror the existing getPendingChallanCount.
• Module re-export works — the module header re-exports module Kernel.External.ChallanSearch.Interface wholesale, so getPendingChallanCountWithFallback is automatically exported.
• The withTryCatch usage correctly catches both getServiceConfig and getPendingChallanCount failures — consistent with how snapToRoadWithFallback wraps getProviderConfig and snapToRoad in one withTryCatch scope.

---

The one mandatory fix is the missing logError on Left — everything else is minor. The coderabbitai comment on this thread is also valid and points to the same root issue.

[khuzema786]

Let me fetch the latest state of the PR.The file SHA and patch are identical to the previous review — the branch hasn't received a new push yet. Let me verify by checking the commit history on the PR branch.The PR branch (signzy/enh) still has only one commit — 50974b6d from June 12 — and the file is unchanged. No new push has reached GitHub yet. The code reviewed earlier is still what's on the branch.

The previous review findings still stand in full:

The critical outstanding issue: Left _ -> go rest at line 35 silently drops errors with no logError, deviating from the snapToRoadWithFallback pattern. This needs to be fixed before merge.

What the author needs to do:

1. Apply the fix locally (add logError on the Left branch)
2. Commit and push to the signzy/enh branch — only then will the PR reflect the change

Once the push shows up, tag me again and I'll re-review the updated diff.

[RitoBose]

Warning — Missing error logging on provider failure (Confidence: 95)

File: lib/mobility-core/src/Kernel/External/ChallanSearch/Interface.hs, lines 34–36

The PR's fallback go loop silently discards errors:

case result of
  Left _ -> go rest   -- ❌ error swallowed, nothing logged
  Right res -> pure res

This directly contradicts the established pattern in Kernel.External.Maps.Interface (snapToRoadWithFallback, lines 222–226), which is the canonical fallback reference for this codebase:

Left err -> do
  logError $ "Snap to road Pre Check failed with error : " <> show err <> " - Provider : " <> show preferredProvider
  callSnapToRoadWithFallback restProviders

Without logging, when all providers fail in production, there is no trace of which provider failed or why. The final InternalError "Not able to fetch challan count with all configured providers" gives zero actionable information.

Fix — align with established pattern:

    go [] = throwError $ InternalError "Not able to fetch challan count with all configured providers"
    go (provider : rest) = do
      logDebug $ "Calling getPendingChallanCount for provider: " <> show provider
      result <- withTryCatch "getPendingChallanCount" $ getServiceConfig provider >>= flip getPendingChallanCount req
      case result of
        Left err -> do
          logError $ "getPendingChallanCount failed with error: " <> show err <> " - Provider: " <> show provider
          go rest
        Right res -> pure res

@ClaudeWeb for " I added for Left err -> logError $ "getPendingChallanCount failed with error: " <> show err <> " - Provider: " <> show provider
go rest
I want to traverse the whole list and when at the last index the go [] is hit if none of the providers match.
pl review once

[khuzema786]

The logic you've described is correct. Let me trace through it to confirm.

Trace with providers = [p1, p2, p3], all failing:

go [p1, p2, p3]
  → p1 fails → logError "... p1 ..." → go [p2, p3]
    → p2 fails → logError "... p2 ..." → go [p3]
      → p3 fails → logError "... p3 ..." → go []
        → throwError "Not able to fetch challan count with all configured providers"  ✅

Trace with providers = [p1, p2], p2 succeeds:

go [p1, p2]
  → p1 fails → logError "... p1 ..." → go [p2]
    → p2 succeeds → pure res  ✅

The key point: Left err does not throw — it only logs and recurses. Only go [] throws. So the full list is always traversed until a success or exhaustion.

One thing to confirm before pushing: The distinction between the two error messages is intentional and correct —

Scenario	Error thrown
providers = [] at call site	"No challan search service provider configured" (caught by when guard)
All providers tried and failed	"Not able to fetch challan count with all configured providers" (caught by go [])

Both paths are independently reachable. Keep them both.

The only remaining gap (low priority): When go [] is hit, the final throwError message doesn't include which providers were tried — but since each individual failure was logError-ed before recursing, the full failure chain is reconstructable from logs. This is acceptable and consistent with the Maps fallback pattern.

The logic is sound — push it and tag me for a final re-review.