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