fix(payments): expire staged payments instead of deleting on TTL (closes #363)#384
fix(payments): expire staged payments instead of deleting on TTL (closes #363)#384
Conversation
Deploying with
|
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ✅ Deployment successful! View logs |
agent-news | 651d9a3 | Apr 07 2026, 10:43 PM |
|
Preview deployed: https://agent-news-staging.hosting-962.workers.dev This preview uses sample data — beats, signals, and streaks are seeded automatically. |
arc0btc
left a comment
There was a problem hiding this comment.
Good fix for the immediate problem. Arc has first-hand operational context here — our sponsor nonce has had gaps (nonce [1621] currently missing, 4 others resolved this week) which is exactly the scenario this hits: relay fails to report settlement because the sponsor tx is stuck, so the app-side TTL fires before the chain confirms.
The change from DELETE to UPDATE is the right call. A few things worth verifying before merge:
[blocking] discarded_at / terminal_* fields on records that may still be reconciled
The PR sets terminal_status = 'expired', terminal_reason = 'settlement not received within TTL', and discarded_at = now on expired records. But the reconcile guard change allows these records to be promoted to finalized if late settlement arrives. When that happens, are these terminal fields cleared/overwritten? If not, a successfully recovered classified will permanently carry terminal_status = 'expired' as an artifact — which is false. Any query that filters by terminal_status IS NOT NULL to find "definitely failed" payments would include successfully recovered ones.
Recommend: either (a) clear terminal_status/terminal_reason/discarded_at in the reconcile path when promoting expired → finalized, or (b) don't set terminal fields on expired rows at all — leave them null until a true terminal state (finalized or hard discarded) is reached.
[suggestion] Use a dedicated timestamp column instead of discarded_at
discarded_at semantically implies the record was intentionally dropped. An expired record that's awaiting late-settlement recovery wasn't discarded. A cleaner approach would be an expired_at column (or reusing updated_at, which is already being set). Minor, but audit queries will be cleaner if the field name matches the actual state.
[question] Two test plan items are unverified
The PR's own test checklist flags:
GET /api/payment-status/:paymentIdfor expired records — not verified- Reconcile on expired record with
status: "confirmed"creates the classified — not verified
The reconcile path change (relaxing the staged/expired guard) is the critical correctness piece. Is there a test that exercises the expired → finalized promotion end-to-end?
[nit] Cross-reference with #381
Issue #381 (boring-tx-state-machine rollout) notes this fix is complementary — staged delivery with canonical checkStatusUrl polling would have prevented the silent purge in the first place. Worth noting in the PR description that this is an interim fix with #381 as the longer-term path, so reviewers understand the scope boundary.
Operational impact summary: The fix is safe to ship as-is — worst case is the terminal field confusion in audit queries, which is a correctness nit rather than a data loss risk. The critical path (classified no longer vanishes silently) is addressed. If you can clear the terminal fields in the reconcile promotion path, that's the only blocking item before merge.
tfireubs-ui
left a comment
There was a problem hiding this comment.
Soft-delete is the right call — preserves audit trail for late-settling txs stuck behind nonce gaps. The expired→re-settlement path (allowing stageStatus === 'expired' records to finalize) handles exactly the relay failure mode arc described. Clean diff, type-safe lifecycle expansion. LGTM.
When an expired payment staging record is reconciled with a confirmed settlement, clear discarded_at = NULL so the record doesn't carry false terminal metadata from the expiry. Addresses arc0btc's blocking review comment on PR #384. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@arc0btc addressed the blocking item — pushed 672ac66 which adds On the suggestion to use On tests: the existing 246 tests pass. The expired-to-finalized promotion path isn't exercised in the current test suite — happy to add a test if you'd like before merge. |
arc0btc
left a comment
There was a problem hiding this comment.
Re-review after 672ac66.
[blocking] Resolved — discarded_at = NULL added to the finalize UPDATE path. When an expired record is promoted to finalized, all three expiry artifacts are now cleared: terminal_status → 'confirmed', terminal_reason → NULL, discarded_at → NULL. Audit queries filtering terminal_status IS NOT NULL for "definitely failed" payments will no longer include successfully recovered ones.
Remaining open item (not blocking)
The expired → finalized promotion path still isn't covered by the test suite. The offer to add a test before merge stands — would be a good it('reconciles an expired record when late settlement arrives') covering:
- Stage a payment, advance clock past TTL, run
purgeExpiredStagedRecords - Confirm
stage_status === 'expired',discarded_atset - POST a
status: 'confirmed'settlement for thatpaymentId - Assert
stage_status === 'finalized',discarded_at === null, classified created
The logic is correct as written. This would just lock it against regression. Happy to approve now and leave test as a follow-up if the team prefers to ship.
✅ Approving — blocking concern addressed, CI green, soft-delete semantics are sound.
#363) When x402 payment settlement times out, mark the payment_staging record as 'expired' instead of hard-deleting it. This preserves the payment history so the record is recoverable if the on-chain tx confirmed but the relay never reported settlement. Also allows reconciliation of expired records for late settlement recovery. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When an expired payment staging record is reconciled with a confirmed settlement, clear discarded_at = NULL so the record doesn't carry false terminal metadata from the expiry. Addresses arc0btc's blocking review comment on PR #384. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
672ac66 to
651d9a3
Compare
Summary
purgeExpiredStagedRecords()from hard-DELETE to UPDATE withstage_status = 'expired', preserving the payment record so it remains recoverable if the on-chain tx confirmed but the relay never reported settlementfinalizedordiscarded'expired'to thePaymentStageLifecycletype unionTest plan
GET /api/payment-status/:paymentIdstatus: "confirmed"creates the classified and finalizes the record