fix(billing): classify escrow settlement underflow as non-retriable - #3526
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 19 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughChangesUnsettleable escrow handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3526 +/- ##
==========================================
- Coverage 74.51% 73.85% -0.67%
==========================================
Files 1150 1086 -64
Lines 29939 28030 -1909
Branches 7481 7086 -395
==========================================
- Hits 22310 20701 -1609
+ Misses 6739 6472 -267
+ Partials 890 857 -33
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
281ba41 to
24b392c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@apps/api/src/billing/services/chain-error/chain-error.service.ts`:
- Around line 129-131: The isUnsettleableDeploymentError method only recognizes
the raw escrow underflow message, so it misses errors normalized by toAppError.
Update this method to also classify the mapped “Deployment escrow cannot be
settled yet” message or inspect the available originalError, and add coverage
for errors thrown as toAppError results so cleanup handlers suppress them
correctly.
In
`@apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.spec.ts`:
- Around line 18-30: Update the “swallows an unsettleable escrow error” test and
its setup helper to return the logger, then assert that
DEPLOYMENT_CLEAN_UP_UNSETTLEABLE is emitted with the expected reason and owner
while preserving the existing no-refill and single-attempt assertions.
In
`@apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts`:
- Around line 80-85: Update the retry flow in the stale deployment cleanup
method around the fee-authorized retry and the existing
isUnsettleableDeploymentError classifier. Catch the retry’s underflow error,
classify it with the same unsettled-deployment handling, and emit
DEPLOYMENT_CLEAN_UP_UNSETTLEABLE with the deployment owner before suppressing
it, while preserving existing handling for other errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cd3d2062-aae1-4b9f-98e5-0be7b0f762f2
📒 Files selected for processing (8)
apps/api/src/app/services/close-trial-deployment/close-trial-deployment.handler.spec.tsapps/api/src/app/services/close-trial-deployment/close-trial-deployment.handler.tsapps/api/src/billing/services/chain-error/chain-error.service.spec.tsapps/api/src/billing/services/chain-error/chain-error.service.tsapps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.spec.tsapps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.tsapps/tx-signer/src/services/chain-error/chain-error.service.spec.tsapps/tx-signer/src/services/chain-error/chain-error.service.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deferring — the inline finding is confirmed and it undermines the entire purpose of this PR: I traced deploymentWriterService.close -> executeDecodedTxByUserWallet -> executeDerivedTx, and executeDerivedTx's catch block already rewrites the error via chainErrorService.toAppError before either call site ever checks isUnsettleableDeploymentError, so the new classification can never match in production.
Extended reasoning...
Overview
This PR adds isUnsettleableDeploymentError to classify a specific chain panic ("negative decimal coin amount") as non-retriable in CloseTrialDeploymentHandler and StaleManagedDeploymentsCleanerService, plus a matching status-code mapping in the tx-signer chain-error service.
Security risks
None — this is error classification/logging logic, no auth, injection, or data-exposure surface.
Level of scrutiny
This warrants a human look despite being a small, mechanical-looking diff, because the confirmed bug means the feature is currently a no-op: I verified ManagedSignerService.executeDerivedTx catches the raw chain error and rethrows it through chainErrorService.toAppError, which rewrites the message to "Deployment escrow cannot be settled yet" before either CloseTrialDeploymentHandler.handle's catch block or StaleManagedDeploymentsCleanerService.cleanUpForWallet ever calls isUnsettleableDeploymentError on it. Both call sites test for the raw clue string "negative decimal coin amount", which no longer appears in the message by the time it reaches them, so both always fall through to the retry/rethrow branch — exactly the pre-PR behavior the PR is meant to fix.
Other factors
The new/updated unit tests reject the raw chain-panic string directly into the mocked deploymentWriterService.close / managedSignerService.executeDerivedTx, bypassing the real toAppError rewrite that happens inside executeDerivedTx in production — which is why the tests pass despite the classification never firing end-to-end. This needs a fix (e.g. check the raw error via originalError, or also match the mapped message) before merge.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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
`@apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.spec.ts`:
- Around line 99-100: Update the stale-managed-deployments cleaner
error-handling tests around the ErrorService setup and unrelated-error case to
use the same mocked logger passed into ErrorService, then assert the generic
error-handler path is invoked with the unrelated error rather than only
verifying cleanup() resolves. Preserve the existing expected behavior for
handled errors.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 727ea69e-65ea-4b19-b24a-a3eb3b885f1a
📒 Files selected for processing (5)
apps/api/src/app/services/close-trial-deployment/close-trial-deployment.handler.spec.tsapps/api/src/billing/services/chain-error/chain-error.service.spec.tsapps/api/src/billing/services/chain-error/chain-error.service.tsapps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.spec.tsapps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/api/src/billing/services/chain-error/chain-error.service.ts
- apps/api/src/billing/services/chain-error/chain-error.service.spec.ts
- apps/api/src/deployment/services/stale-managed-deployments-cleaner/stale-managed-deployments-cleaner.service.ts
There was a problem hiding this comment.
No new issues found this run. The critical bug I flagged previously — isUnsettleableDeploymentError never matching in production because toAppError rewrites the error message before classification runs — looks fixed in commit 578e191, which now also inspects originalError and routes the stale-cleaner's fee-authorized retry through the same classifier. Given this is production-incident mitigation code around chain-error retry semantics, and it already needed a fix for a no-op bug once, I think it's worth a human confirming the fix end-to-end before merge.
What was reviewed:
- Confirmed
isUnsettleableDeploymentErrornow matches onoriginalError.messageas well as the top-level message, closing the gap that made the original mitigation a no-op against errors normalized bytoAppError. - Confirmed the stale-cleaner's fee-authorized retry (previously unguarded) now routes through the same classifier via the new
closeDeploymentshelper, so an underflow on retry is logged asDEPLOYMENT_CLEAN_UP_UNSETTLEABLEinstead of escaping and being retried. - Checked both
CloseTrialDeploymentHandlerand the stale cleaner return (rather than throw) only on classified errors, so unrelated errors still rethrow/retry as before. - Chain-error mapping additions in both
apps/apiandapps/tx-signerare simple substring-to-status entries consistent with the existing pattern in each service.
Extended reasoning...
Overview
This PR classifies a specific on-chain panic (negative decimal coin amount, caused by an escrow account settling with a negative balance) as a non-retriable error in both apps/api and apps/tx-signer's chain-error services, and wires that classification into CloseTrialDeploymentHandler and StaleManagedDeploymentsCleanerService so pg-boss stops retrying a deterministic failure and instead logs a recognized event with the affected wallet/dseq/owner.
Security risks
None specific to this PR — no new auth, crypto, or user-input-handling surface. The main risk is a functional one: misclassifying which errors are safe to swallow vs. rethrow could hide real failures or spuriously suppress retries for a recoverable error. I traced both call sites and confirmed non-matching errors still hit the throw error / rethrow branch.
Level of scrutiny
This touches production incident-mitigation code in the billing/chain-error retry path, which is sensitive because pg-boss retry behavior and escrow error classification are hard to fully exercise in unit tests (as the previous run's finding showed — a subtle message-rewriting order-of-operations bug made the entire original mitigation a no-op). Given that history and the fact the code needed a real fix rather than a nit, this warrants a human's confirmation despite the fix now looking correct on inspection.
Other factors
No bugs were found by the bug hunting system in this run, and my own read of commit 578e191 shows it correctly addresses the previously-flagged defect plus the two CodeRabbit-flagged gaps (retry-path underflow, and a test that didn't assert the log event). Test coverage for the new paths looks meaningful (asserts the actual log payload, exercises the real ChainErrorService against a toAppError-normalized error rather than a raw string). I'm deferring primarily due to the sensitivity of the code path and its recent history, not because of any issue found in this pass.
578e191 to
90aa255
Compare
The CloseTrialDeployment job panics on-chain during close simulation with "negative decimal coin amount" when an escrow account is overdrawn. It was surfaced as a generic 500, so pg-boss retried the deterministic failure 5 times and the dseq/owner were never logged. Classify the panic in the api and tx-signer chain-error services (400), skip-with-alert in the trial-close handler (logs CLOSE_TRIAL_DEPLOYMENT_UNSETTLEABLE with dseq/owner instead of retrying), and downgrade it to a recognized DEPLOYMENT_CLEAN_UP_UNSETTLEABLE event in the stale managed-deployments cleaner. executeDerivedTx rewrites the error through toAppError before it reaches either handler, so isUnsettleableDeploymentError also inspects the originalError that toAppError attaches (matching only the raw message made the classifier a no-op in production). In the cleaner, the fee-authorized retry runs through a closeDeployments helper so an underflow from either attempt is classified and logged. Inject LoggerService so the event is assertable, and exercise the real ChainErrorService against a normalized error in the specs.
90aa255 to
35b027f
Compare
There was a problem hiding this comment.
LGTM — the critical classification bug I flagged in a previous run (isUnsettleableDeploymentError not matching the toAppError-normalized message) is fixed in 578e191 and verified against the real production code path.
Extended reasoning...
Overview
This PR classifies the chain's escrow-settlement-underflow panic (negative decimal coin amount) as a non-retriable error across three consumers: ChainErrorService (api and tx-signer), CloseTrialDeploymentHandler, and StaleManagedDeploymentsCleanerService. On this specific error, the two job handlers now log a dedicated event and return instead of throwing, so pg-boss stops retrying a deterministic on-chain failure.
Security risks
None. This is error classification/control-flow logic for an internal billing job; no user input, auth, or crypto is touched. The main risk was purely functional: mis-classifying the error would either (a) keep retrying forever (status quo bug) or (b) silently swallow a real failure. Both are now covered by tests.
Level of scrutiny and verification performed
This required a high level of scrutiny given the risk of the fix being a silent no-op, which is exactly what happened in an earlier revision: I previously flagged that isUnsettleableDeploymentError only matched the raw chain panic text, but ManagedSignerService.executeDerivedTx always rewrites the error via chainErrorService.toAppError before either call site sees it, so the classifier would never match in production and the whole feature would be a no-op. I re-verified this against the current code: executeDerivedTx (managed-signer.service.ts:57-65) still throws the toAppError-normalized error, and isUnsettleableDeploymentError (chain-error.service.ts) now inspects both error.message and error.originalError.message, correctly catching the normalized case. This is covered by a regression test that runs isUnsettleableDeploymentError(await toAppError(rawPanic, [])) and asserts true, which fails against the old code.
I also checked the fee-authorized-retry path in the stale cleaner (extracted into closeDeployments()), confirming an underflow from the retry now reaches the same classifier, and confirmed the test-quality gap I raised about the 'rethrows unrelated errors' test (whether it could distinguish rethrow-then-swallow-by-ErrorService from silently-swallowed-before-ErrorService) is now resolved on disk — setup() returns the errorLogger passed into ErrorService, and the test asserts it receives the unrelated error with the DEPLOYMENT_CLEAN_UP_ERROR event.
Other factors
All four issues raised across this PR's review (three by CodeRabbit, one by me) were fixed with targeted regression coverage and confirmed by CodeRabbit and my own re-check of the current source. The bug-hunting system found nothing new this run.
2 participants
Why
The
CloseTrialDeploymentjob was panicking on-chain during close simulation withnegative decimal coin amount: -2.0when a trial deployment's escrow account is overdrawn. The panic surfaced as a generic 500, so pg-boss retried the deterministic failure 5 times over ~25 min, and the dseq/owner were never logged (identifying the affected deployment required a Grafana + on-chain dive).The underlying chain bug (escrow full-block settlement builds a
DecCoinfrom an overdraw-negative balance) is tracked for the Core team in Ref AKT-658. This PR is the console-side mitigation; the escrow account self-heals once it settles to zero, so retrying can never succeed and stop-and-alert is the correct behavior.What
negative decimal coin amount) as a non-retriable400in both the api and tx-signer chain-error services, and exposeisUnsettleableDeploymentError.CloseTrialDeploymentHandler: on this error, logCLOSE_TRIAL_DEPLOYMENT_UNSETTLEABLEwithwalletId/dseq/owner/userIdand return instead of throwing (so pg-boss doesn't retry a deterministic failure), and don't enqueue the "closed" notification. Other errors still rethrow (retry preserved).DEPLOYMENT_CLEAN_UP_UNSETTLEABLEevent instead of a generic cleanup error.Summary by CodeRabbit