fix(x402): implement background reconciliation for silent orphan payments (#588)#589
Open
Meenbudha wants to merge 1 commit into
Open
fix(x402): implement background reconciliation for silent orphan payments (#588)#589Meenbudha wants to merge 1 commit into
Meenbudha wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughA new x402 reconciliation worker scans failed tasks, retries settlement, and updates task metadata when settlement succeeds. TaskManager now starts and cancels that background task, and a Hydra auth middleware return statement drops an inline type-ignore comment. Changesx402 reconciliation worker
Hydra middleware cleanup
Sequence Diagram(s)sequenceDiagram
participant TaskManager
participant run_x402_reconciliation_loop
participant reconcile_failed_payments
participant Storage
participant HTTPFacilitatorClient
TaskManager->>run_x402_reconciliation_loop: create_task(...)
loop every interval_seconds
run_x402_reconciliation_loop->>reconcile_failed_payments: invoke
reconcile_failed_payments->>Storage: list_tasks(...)
reconcile_failed_payments->>HTTPFacilitatorClient: settle(...)
HTTPFacilitatorClient-->>reconcile_failed_payments: settlement result
reconcile_failed_payments->>Storage: update_task(...)
end
run_x402_reconciliation_loop-->>TaskManager: CancelledError on shutdown
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
🧹 Nitpick comments (4)
tests/integration/x402/test_reconciliation.py (2)
43-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly the success path is covered.
This test exercises
settle_response.success is True. The non-confirmed branch (Line 113, status stayspayment-failed) and the facilitator-exception branch (Line 120) are uncovered. Adding a "not confirmed" case would guard against regressions that accidentally orphan unconfirmed payments.Want me to draft the additional test cases for the not-confirmed and exception branches?
🤖 Prompt for 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. In `@tests/integration/x402/test_reconciliation.py` around lines 43 - 109, The reconciliation test only covers the confirmed-success path in reconcile_failed_payments, so add coverage for the other settle_result branches in x402_reconciliation. Extend test_reconciliation_worker_success or add new async tests to simulate HTTPFacilitatorClient.settle returning a non-confirmed response and raising an exception, then assert the task metadata remains payment-failed and is not changed to payment-orphaned. Use the existing symbols reconcile_failed_payments and HTTPFacilitatorClient to locate the branch points.
46-46: 📐 Maintainability & Code Quality | 🔵 TrivialUse the
memory_storagefixture instead of instantiatingInMemoryStorageinline.The test file should depend on the shared
memory_storagefixture defined intests/fixtures/storage_fixtures.py, which is already registered intests/conftest.py.View diff
- from bindu.server.storage.memory_storage import InMemoryStorage ... - async def test_something(): - storage = InMemoryStorage() + async def test_something(memory_storage: InMemoryStorage): + storage = memory_storageThis aligns with the guideline to use fixtures rather than custom setup.
🤖 Prompt for 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. In `@tests/integration/x402/test_reconciliation.py` at line 46, The test setup in test_reconciliation should use the shared memory_storage fixture instead of creating InMemoryStorage directly inline. Update the test to accept and use memory_storage from the registered fixture in tests/fixtures/storage_fixtures.py, so the reconciliation test relies on the shared test infrastructure rather than custom storage construction.Source: Coding guidelines
bindu/server/workers/x402_reconciliation.py (2)
35-47: 🚀 Performance & Scalability | 🔵 TrivialRepeatedly-failing tasks are re-checked every tick with no backoff or attempt cap.
Tasks that stay
payment-failed(genuine failures, not transient timeouts) match the filter on every 30s scan of the most recent 100 tasks, each time validating models, constructing a freshHTTPFacilitatorClient, and re-callingsettle. Consider a max-attempt counter or last-checked timestamp in metadata, and reusing a single facilitator client, to bound facilitator load.🤖 Prompt for 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. In `@bindu/server/workers/x402_reconciliation.py` around lines 35 - 47, The reconciliation loop in reconcile_failed_payments keeps reprocessing the same failed tasks on every scan, which can repeatedly call validate models, create a new HTTPFacilitatorClient, and retry settle without any bound. Add a retry guard using task metadata such as a max-attempt counter or last-checked timestamp so terminal failures stop being rechecked every tick, and reuse a single facilitator client within the reconciliation flow instead of constructing one per task.
100-104: 📐 Maintainability & Code Quality | 🔵 TrivialDefine a canonical
status_orphanedconstant and use it.While
app_settings.x402.status_failedexists, the specific value"payment-orphaned"is currently hardcoded. Add a correspondingstatus_orphanedconstant tobindu/settings.pyto ensure consistency:# bindu/settings.py class X402Settings(BaseSettings): # ... existing fields status_failed: str = "payment-failed" status_orphaned: str = "payment-orphaned" # Add this lineThen update the code to reference the new constant:
# bindu/server/workers/x402_reconciliation.py updated_metadata = { **metadata, app_settings.x402.meta_status_key: app_settings.x402.status_orphaned, app_settings.x402.meta_receipts_key: [settle_response.model_dump()], }🤖 Prompt for 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. In `@bindu/server/workers/x402_reconciliation.py` around lines 100 - 104, Add a canonical X402 orphaned status constant in X402Settings inside bindu/settings.py and use it from x402_reconciliation instead of hardcoding "payment-orphaned". Update app_settings.x402 to expose status_orphaned alongside status_failed, then replace the literal in updated_metadata within x402_reconciliation with app_settings.x402.status_orphaned so the status value stays consistent across the codebase.
🤖 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 `@bindu/server/task_manager.py`:
- Around line 192-201: The shutdown path in TaskManager’s reconciliation cleanup
is fine, but the warning log in the exception handler is using printf-style
formatting with loguru, so the exception detail won’t be rendered. Update the
logger.warning call in the reconciliation task cancel/await block to use
loguru’s "{}" placeholder formatting, preserving the existing message and
exception variable so the error text is actually interpolated.
In `@bindu/server/workers/x402_reconciliation.py`:
- Around line 31-32: The logging in x402_reconciliation uses printf-style
placeholders, which Loguru ignores, so dynamic error details are dropped. Update
the logger.exception call in x402_reconciliation.py to use Loguru-style {}
formatting, and review any other log statements in the same file to ensure they
use {} instead of %s so messages and exception context are preserved.
---
Nitpick comments:
In `@bindu/server/workers/x402_reconciliation.py`:
- Around line 35-47: The reconciliation loop in reconcile_failed_payments keeps
reprocessing the same failed tasks on every scan, which can repeatedly call
validate models, create a new HTTPFacilitatorClient, and retry settle without
any bound. Add a retry guard using task metadata such as a max-attempt counter
or last-checked timestamp so terminal failures stop being rechecked every tick,
and reuse a single facilitator client within the reconciliation flow instead of
constructing one per task.
- Around line 100-104: Add a canonical X402 orphaned status constant in
X402Settings inside bindu/settings.py and use it from x402_reconciliation
instead of hardcoding "payment-orphaned". Update app_settings.x402 to expose
status_orphaned alongside status_failed, then replace the literal in
updated_metadata within x402_reconciliation with
app_settings.x402.status_orphaned so the status value stays consistent across
the codebase.
In `@tests/integration/x402/test_reconciliation.py`:
- Around line 43-109: The reconciliation test only covers the confirmed-success
path in reconcile_failed_payments, so add coverage for the other settle_result
branches in x402_reconciliation. Extend test_reconciliation_worker_success or
add new async tests to simulate HTTPFacilitatorClient.settle returning a
non-confirmed response and raising an exception, then assert the task metadata
remains payment-failed and is not changed to payment-orphaned. Use the existing
symbols reconcile_failed_payments and HTTPFacilitatorClient to locate the branch
points.
- Line 46: The test setup in test_reconciliation should use the shared
memory_storage fixture instead of creating InMemoryStorage directly inline.
Update the test to accept and use memory_storage from the registered fixture in
tests/fixtures/storage_fixtures.py, so the reconciliation test relies on the
shared test infrastructure rather than custom storage construction.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: abfbffac-6a69-4280-85d9-3099d798a3e9
📒 Files selected for processing (4)
bindu/server/applications.pybindu/server/task_manager.pybindu/server/workers/x402_reconciliation.pytests/integration/x402/test_reconciliation.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Describe the problem and fix in 2–5 bullets:
/settlecall can time out, leadingmanifest_worker.pyto record the payment status aspayment-failedand fail the task. However, the transaction can confirm on-chain shortly after, resulting in a silent debit to the client's wallet with no alert or fallback.run_x402_reconciliation_loopworker (inbindu/server/workers/x402_reconciliation.py) that scans recent failed tasks, extracts the payment context from the initial message metadata, and calls the facilitator again (idempotent/settlecall) to check if the transaction confirmed. If verified, it updates the task's payment status topayment-orphanedand saves the receipt.failed(we do not re-run the agent or auto-refund since Bindu does not manage an outbound wallet/key custody).Change Type (select all that apply)
Scope (select all touched areas)
Linked Issue/PR
User-Visible / Behavior Changes
List user-visible changes (including defaults/config).
If none, write
None.NoneSecurity Impact (required)
NoNoYes(Periodic background HTTP request to the configured facilitator's/settleAPI endpoint)NoNoYes, explain risk + mitigation: The background loop periodically calls/settleon the facilitator using the exact same client-signed EIP-3009 parameters from the original request metadata, presenting no new authorization exposure.Verification
Environment
Steps to Test
Expected Behavior
A task marked with payment status
payment-faileddue to a transient timeout should be picked up by the reconciliation worker, verified against the facilitator, and transitioned topayment-orphanedwith receipt metadata attached.Actual Behavior
The task successfully transitions to
payment-orphaned, the receipt is stored in task metadata, and the transient error key is cleared.Evidence (attach at least one)
Human Verification (required)
What you personally verified (not just CI):
_payment_contextmetadata, and tasks with other payment statuses are ignored.Compatibility / Migration
YesNoNoFailure Recovery (if this breaks)
create_taskcall insideTaskManager.__aenter__.bindu/server/task_manager.pyRisks and Mitigations
List only real risks for this PR. If none, write
None.failedstate andpayment-failedpayment status, creating a small and bounded query set.Checklist
uv run pytest)uv run pre-commit run --all-files)Summary by CodeRabbit
New Features
Bug Fixes
Tests