fix(billing): stamp wallet activation only after trial provisioning succeeds#3520
fix(billing): stamp wallet activation only after trial provisioning succeeds#3520baktun14 wants to merge 2 commits into
Conversation
…ucceeds A trial start claimed activation by stamping activated_at up front and rolled the stamp back on failure. The rollback was unconditional and the stamp was readable mid-flight, so the idempotency fast-path could report the trial as started while a slow failing attempt later cleared the stamp - leaving provisioned, actively-deploying wallets invisible to the refill cron, the wallets endpoint and the trial fingerprint check. Provisioning is now guarded by a dedicated activation_claimed_at column with stale-claim takeover, releases are scoped to the claim they own, and activated_at is written only together with the granted allowances.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a nullable wallet activation claim timestamp, stale-claim takeover, guarded claim release, and explicit activation completion. Trial initialization now releases failed claims, while refill activation uses ChangesWallet activation flow
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 1
🧹 Nitpick comments (2)
apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts (1)
153-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTighten the vacuous negative assertion.
not.toHaveBeenCalledWith(wallet.id, { activatedAt: null })passes even ifupdateByIdis called with something entirely different; in the new failure path it isn't called at all. Assert that directly.♻️ Proposed change
expect(releaseActivationClaim).toHaveBeenCalledWith(wallet.id, claimedAt); - expect(updateWalletById).not.toHaveBeenCalledWith(wallet.id, { activatedAt: null }); + expect(updateWalletById).not.toHaveBeenCalled();As per path instructions, "Verify meaningful assertions, not just snapshot coverage".
🤖 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 `@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts` around lines 153 - 170, In the test “releases only its own claim and keeps the wallet when authorization fails,” replace the vacuous updateWalletById argument assertion with a direct assertion that updateWalletById is not called at all. Keep the existing releaseActivationClaim assertion unchanged.Source: Path instructions
apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts (1)
94-111: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider computing the claim stamp and cutoff in the DB rather than the app clock.
new Date()for both the stamp andstaleClaimCutoffmeans the claim lifetime depends on per-instance clock skew: an instance whose clock runs behind can take over a claim that is younger than 5 minutes (or refuse to take over a genuinely abandoned one).sql\now()`/sql`now() - interval '5 minutes'`` would make the window skew-independent. With a 5-minute window this is unlikely to bite in practice, so it's a hardening nit rather than a defect.🤖 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 `@apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts` around lines 94 - 111, Update claimActivation to use database-side timestamps for both activationClaimedAt and staleClaimCutoff, using the existing SQL expression utilities to represent current time and five minutes earlier. Remove the app-clock Date calculations while preserving the existing accessibility, activation, and stale-claim conditions.
🤖 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/wallet-initializer/wallet-initializer.service.ts`:
- Around line 67-70: Update the catch block in the wallet-initializer
provisioning flow to preserve and rethrow the original error even when
releaseActivationClaim fails. Add the service’s standard
LoggerService/createOtelLogger setup and wrap releaseActivationClaim in its own
try/catch, logging release failures through that logger before rethrowing the
original error.
---
Nitpick comments:
In `@apps/api/src/billing/repositories/user-wallet/user-wallet.repository.ts`:
- Around line 94-111: Update claimActivation to use database-side timestamps for
both activationClaimedAt and staleClaimCutoff, using the existing SQL expression
utilities to represent current time and five minutes earlier. Remove the
app-clock Date calculations while preserving the existing accessibility,
activation, and stale-claim conditions.
In
`@apps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.ts`:
- Around line 153-170: In the test “releases only its own claim and keeps the
wallet when authorization fails,” replace the vacuous updateWalletById argument
assertion with a direct assertion that updateWalletById is not called at all.
Keep the existing releaseActivationClaim assertion unchanged.
🪄 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: f485078c-c9a6-4879-aef8-1fb14489198a
📒 Files selected for processing (12)
apps/api/drizzle/0034_natural_piledriver.sqlapps/api/drizzle/meta/0034_snapshot.jsonapps/api/drizzle/meta/_journal.jsonapps/api/src/billing/model-schemas/user-wallet/user-wallet.schema.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.integration.tsapps/api/src/billing/repositories/user-wallet/user-wallet.repository.tsapps/api/src/billing/services/refill/refill.service.spec.tsapps/api/src/billing/services/refill/refill.service.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.spec.tsapps/api/src/billing/services/wallet-initializer/wallet-initializer.service.tsapps/api/src/user/repositories/user/user.repository.integration.tsapps/api/test/seeders/user-wallet.seeder.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3520 +/- ##
==========================================
- Coverage 74.18% 73.22% -0.97%
==========================================
Files 1142 1052 -90
Lines 29721 27378 -2343
Branches 7447 6986 -461
==========================================
- Hits 22049 20047 -2002
+ Misses 6771 6461 -310
+ Partials 901 870 -31
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
Wrap releaseActivationClaim in a catch so a failing release logs and is swallowed instead of masking the original provisioning error. The claim is already protected by stale takeover, so release is best-effort.
Addressed CodeRabbit reviewPushed ✅ Fixed — best-effort claim release ( ✅ Fixed — tightened vacuous assertion ( ⏭️ Deferred — DB-side claim timestamp ( Note: the CodeRabbit ESLint "unused eslint-disable directive" tool-run warning on
|
1 participant
Why
Related to CON-744
After deploying #3513, every trial started in a ~2h prod window ended up with
activated_at = NULLeven though provisioning fully succeeded on-chain (fee + deposit grants landed, users were actively deploying). Verified against prod data and public LCD state.Root cause:
activated_atdoubled as both the in-flight claim marker and the completion marker.activated_atup front and rolled it back on failure. The rollback was unconditional, so a slow failing attempt could clear a stamp it no longer owned.Net effect: provisioned, actively-deploying wallets that are invisible to
GET /v1/wallets, skipped by the refill cron (findDrainingWallets), and ignored by the trial fingerprint check. Affected prod rows were repaired manually with a one-offUPDATE.What
user_wallets.activation_claimed_atcolumn (migration 0034, additive) guards in-flight provisioning:claimActivationsets it conditionally; a claim older than 5 minutes is considered abandoned (crashed/hung attempt) and can be taken over.releaseActivationClaimclears only the claim identified by its own timestamp, so a stale attempt cannot clobber a newer claim.activated_atis now written only on success, atomically with the granted allowances — there is no rollback to get wrong, and the fast-path/GET /v1/wallets/refill cron/fingerprint check can only ever observe completed activations.RefillServiceuses the newmarkActivated(direct stamp, no claim cycle) since funding-based activation has no post-claim provisioning step to guard.No API contract changes. The migration is a single nullable-column
ALTER TABLE— no backfill, no locks beyond a brief ACCESS EXCLUSIVE.Summary by CodeRabbit
New Features
Bug Fixes