juspay / juspay/hyperswitch

[BUG] v1: GET /refunds/{id}?force_sync=true silently no-ops for terminal refunds, diverging from v2 (regression from #11725)

Open
#13,680 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Rust
Stars
43.7k
Forks
5.1k
Avg merge
2d 20h
Merged PRs (30d)
205

Description

## Bug Description

Since #11725 (`4e0be58767`, first released in `2026.04.10.0`), v1 `should_call_refund` gates `force_sync` behind a non-terminal-status check (`crates/router/src/core/refunds.rs`, current shape: `all_keys_required || (force_sync && !matches!(refund.refund_status, Failure | Success))`). For a refund already in `Success` / `Failure`, `GET /refunds/{id}?force_sync=true` returns the stale DB row — no connector call, no error, no indication the sync was skipped (the call site's `else` just returns `Ok((refund, None))`).

If the connector later reverses a succeeded refund and the reversal webhook is lost, the refund row is permanently stuck at `success`: there is no supported way to pull the connector's truth back in.

Why this looks like a regression rather than a design choice:

- v2 (`crates/router/src/core/refunds_v2.rs`) still has the pre-#11725 shape — `force_sync` as an unconditional disjunct. The same public API behaves oppositely between v1 and v2.
- For ~3 years before 2026-04-09 (#1013 introduced the function in 2023; #2081 shows the unconditional shape later that year) v1 behaved like today's v2.
- #11725's own description says terminal refunds should skip sync when "all_keys_required **or** force_sync" is set — but the merged code only gates `force_sync`; the `all_keys_required` bypass survived. So the one lever that still crosses the terminal gate contradicts that PR's stated intent, and its documented contract is response verbosity only ("If true, returns stringified connector raw response body", `crates/api_models/src/refunds.rs`). Neither `force_sync` nor `all_keys_required` appears in the published OpenAPI parameters for `GET /refunds/{refund_id}` (and `/refunds/sync` is missing from the generated spec entirely — its utoipa annotation is marked `get` with no request body).
- The write-back machinery needs no change: when the connector *is* called, `sync_refund_with_gateway` materializes the returned status verbatim (including Success → Failure), fires the outgoing webhook and the intent state-metadata update. Only this predicate blocks it.
- The sanctioned alternative, `PUT /refunds/{id}/manual-update`, writes an operator-asserted status **without contacting the connector at all** (and since #13606 it can overwrite `connector_refund_id` too) — strictly less safe than a connector-verified sync.
- Minor inconsistencies in the same predicate: the terminal set omits `TransactionFailure` (also a terminal failure state); and #12755 removed the `connector_refund_id.is_some()` guard from v1 while v2 retains it — a second v1/v2 divergence in the same function.

## Expected Behavior

An explicit `force_sync=true` consults the connector, terminal status included — matching v2 and the ~3 years of v1 behavior before #11725.

## Actual Behavior

Silent no-op for terminal refunds; the stale row is returned; the only way to force a real sync is the undocumented `all_keys_required=true` side effect.

## Steps To Reproduce

1. Create a refund and let it reach `success`.
2. Simulate a connector-side reversal (a stub connector returning failure works) and drop the webhook.
3. `GET /refunds/{id}?force_sync=true` → router logs show no connector call; response still `success`.
4. Same request with `&all_keys_required=true` → the connector is called, the status flips to `failed`, the outgoing webhook fires — i.e. the machinery works; only the predicate gates it.

## Context For The Bug

Proposed fix: restore `force_sync` as an unconditional disjunct in v1 `should_call_refund` (parity with v2), or introduce an explicitly documented parameter for terminal-refund resync; either way, declare `force_sync` / `all_keys_required` in the OpenAPI parameters and fix the `/refunds/sync` annotation. I'd add unit tests for the predicate directions (terminal + force_sync → call; terminal without force_sync → no call; `TransactionFailure` treated like `Failure`).

I'm willing to submit a PR.

## Environment

Verified by code inspection on `main` @ `83d1d0c1` (2026.08.13.0); behavior reproduced on a self-hosted v1 deployment (2026.07.08.1, PostgreSQL-backed, default config).

Contributor guide

Open the contributing guide

Research direction

Start in crates/router/src/core/refunds.rs and compare should_call_refund with crates/router/src/core/refunds_v2.rs. Inspect the refund API models and utoipa annotations in crates/api_models/src/refunds.rs, then add or run unit coverage for terminal and non-terminal predicate directions. Done means force_sync behavior is consistent with the expected terminal-refund sync contract and the relevant API documentation is accurate.

Written by the indexing model from the issue text.

Assessment

Tech stack
rust
Domain
backend-api-design, payments
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
70/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.