fix: Add request timeout handling and auto-retry logic for e-Invoice/e-Waybill#4545
fix: Add request timeout handling and auto-retry logic for e-Invoice/e-Waybill#4545vorasmit wants to merge 5 commits into
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 15 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
A slow or unresponsive government server had no request timeout, so each bulk e-Invoice/e-Waybill job held its worker until the RQ job timeout (up to 300s on the short queue). A bulk submit of many invoices could occupy all short-queue workers and stall other jobs. A raw hang was also killed as a generic exception, so it never tripped the existing Auto-Retry path. - BaseAPI now sends a (connect, read) timeout on every request: a finite connect timeout for all APIs, and a 60s read timeout for e-Invoice / e-Waybill so they fail fast instead of holding a worker. Overridable per-site via the `ic_request_timeout` site config key. - requests Timeout / ConnectionError are mapped to GatewayTimeoutError / GSPServerError so a client-side hang flows into the existing handle_server_errors -> Auto-Retry path instead of hanging. - Sales Invoice on_submit skips enqueuing a per-invoice job when an auto-retry is already pending, marking the document Auto-Retry so the 5-minute scheduler drains the backlog in a batch. This stops a flood of jobs from piling up on the short queue during an outage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1Vg8LfdPy4Z4KAdEsS8kY
a4bee67 to
d1aba1b
Compare
📝 WalkthroughWalkthroughThis PR updates request timeout handling in 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5996ad80-5a9e-4685-bc1f-6bb5f101c4e4
📒 Files selected for processing (4)
india_compliance/gst_india/api_classes/base.pyindia_compliance/gst_india/api_classes/nic/e_invoice.pyindia_compliance/gst_india/api_classes/nic/e_waybill.pyindia_compliance/gst_india/overrides/sales_invoice.py
is_retry_einv_ewb_generation_pending can persist after GST Settings is switched to sandbox mode, where retry_e_invoice_e_waybill_generation() early-returns. Without this gate, invoices submitted in that state were marked Auto-Retry but never drained. Mirror the condition already used in handle_server_errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1Vg8LfdPy4Z4KAdEsS8kY
Confidence Score: 5/5Safe to merge; all new code paths are guarded and consistent with existing retry infrastructure. The timeout and auto-retry logic is well-scoped. Exception conversion in base.py preserves the original message via str(e), the on_submit guard mirrors handle_server_errors conditions, and the 604 recovery adds frappe.log_error so background failures are visible. The only nit is using a set for recovery_dates (non-deterministic iteration order) which is a style concern, not a correctness problem. india_compliance/gst_india/utils/e_waybill.py — the 604 recovery date ordering is the one place worth a second look.
|
| Filename | Overview |
|---|---|
| india_compliance/gst_india/api_classes/base.py | Adds configurable request timeout to BaseAPI with sensible defaults (10s connect, no read cap), catches Timeout/ConnectionError and converts them to GatewayTimeoutError/GSPServerError; implementation is sound. |
| india_compliance/gst_india/api_classes/nic/e_invoice.py | Adds REQUEST_TIMEOUT = (10, 60) override to fail fast on slow government servers; trivial and correct. |
| india_compliance/gst_india/api_classes/nic/e_waybill.py | Adds REQUEST_TIMEOUT = (10, 60) override to fail fast on slow government servers; trivial and correct. |
| india_compliance/gst_india/overrides/sales_invoice.py | on_submit now checks retry_pending flag and sets "Auto-Retry" status directly instead of enqueuing during outages; logic mirrors handle_server_errors and is correctly gated on sandbox_mode. |
| india_compliance/gst_india/utils/e_waybill.py | Refactors find_matching_e_waybill into shared link_matching_e_waybill; adds 604 auto-recovery inside the try block with frappe.log_error for background visibility; recovery_dates uses a set with non-deterministic iteration order. |
Reviews (4): Last reviewed commit: "fix: broaden e-Waybill 604 recovery date..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if retry_pending: | ||
| set_einvoice_status(doc, "Auto-Retry", notify=bool(frappe.request)) |
There was a problem hiding this comment.
When retry_pending is True, set_einvoice_status / set_ewaybill_status are called with notify=bool(frappe.request). The notify argument triggers a WebSocket field update (the status chip changes in the form), but it does not show a popup to the user. In the previous flow, the background job would fail with GatewayTimeoutError/GSPServerError, and handle_server_errors would always call frappe.msgprint(...) with "Government services are currently slow/down. Your e-Invoice generation will be automatically retried every 5 minutes." — the only message that explains the "Auto-Retry" status to the operator. With this change, invoices submitted during an active outage silently flip to "Auto-Retry" without any explanatory popup, leaving users unaware of what happened or that the retry will be automatic.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Bare `raise GatewayTimeoutError`/`GSPServerError` recorded an empty error string in the integration request log for the new client-side timeout and connection failure modes. Forward the original requests exception message so these failures stay diagnosable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1Vg8LfdPy4Z4KAdEsS8kY
When NIC returns 604 ("e-Waybill already generated for this document"),
generation previously threw a generic failure and left the existing
e-Waybill unlinked -- recoverable only via the manual "fetch by date"
flow. This is more likely now that client-side timeouts route to
auto-retry: a request can time out after NIC has already generated the
e-Waybill.
On 604, fetch active e-Waybills for today and link the one matching this
document, reusing the manual fetch logic (extracted into
link_matching_e_waybill). If none is found, fail with a clear message
pointing to the manual fetch-by-date action. Also removes the previously
unreachable 604 block that sat after the generic failure throw.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01J1Vg8LfdPy4Z4KAdEsS8kY
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: faedecb9-eedc-4dba-845d-667ed0d1054e
📒 Files selected for processing (3)
india_compliance/gst_india/api_classes/base.pyindia_compliance/gst_india/overrides/sales_invoice.pyindia_compliance/gst_india/utils/e_waybill.py
🚧 Files skipped from review as they are similar to previous changes (2)
- india_compliance/gst_india/api_classes/base.py
- india_compliance/gst_india/overrides/sales_invoice.py
Search the document date(s) and today when recovering an already-generated e-Waybill on 604, so a delayed auto-retry (running on a later day than the original generation) can still find and link it. Using the posting date alone would miss backdated invoices, whose e-Waybill is created on the submit day. When no active match is found, record a deferred Error Log (survives the rollback in the except handler) so the orphaned e-Waybill can be reconciled even on the background auto-retry path, which otherwise swallows the error. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01J1Vg8LfdPy4Z4KAdEsS8kY
Abdeali099
changed the title
4 participants
Summary
This PR implements request timeout handling for GST API calls and adds auto-retry logic during outages to prevent queue flooding. When a GST service outage is detected, invoices are marked with "Auto-Retry" status instead of being enqueued, allowing a centralized 5-minute drain process to handle retries.
Key Changes
Request Timeout Configuration: Added configurable request timeouts to
BaseAPIwith sensible defaults:ic_request_timeoutin frappe.confTimeout Exception Handling: Enhanced
_make_request()to catch timeout and connection errors, converting them to appropriate exceptions (GatewayTimeoutError,GSPServerError) that trigger auto-retry logicAuto-Retry During Outages: Modified
on_submit()in sales_invoice.py to:enable_retry_einv_ewb_generation+is_retry_einv_ewb_generation_pending)New Imports: Added
set_einvoice_statusandset_ewaybill_statusutility functions to support status updatesImplementation Details
notifyparameter in status setters respects whether the request came from an API callhttps://claude.ai/code/session_01J1Vg8LfdPy4Z4KAdEsS8kY