refactor(deployment): instrument the fund-on-create deployment job - #3546
Conversation
Give the fund-on-create job the same OTel metrics tier as the hourly top-up cron it mirrors: job completions and duration, deposit count and amount, and skips by reason. Failures carry a coarse reason plus a retriable flag so the benign lease-not-visible retry does not drown the failure signal and alerts can key off status="failure" AND retriable=false. Skip and deposit logging moves into a dedicated instrumentation service, collapsing the granular INITIAL_FUNDING_* skip events into a single INITIAL_FUNDING_SKIPPED event with a reason attribute. The forensic INITIAL_FUNDING_TX_FAILED log stays in the service.
|
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)
📝 WalkthroughWalkthroughInitial deployment funding now emits structured metrics and logs for job, deposit, skipped, and failed outcomes. The funding services receive instrumentation dependencies. The deployment handler records duration and job status, while funding errors remain rethrown. ChangesInitial funding observability
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed: dependency version conflict. Check your lock file or package.json. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3546 +/- ##
==========================================
- Coverage 74.79% 74.07% -0.72%
==========================================
Files 1155 1072 -83
Lines 30092 27796 -2296
Branches 7509 7038 -471
==========================================
- Hits 22508 20591 -1917
+ Misses 6701 6356 -345
+ Partials 883 849 -34
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![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 (1)
apps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding-instrumentation.service.ts (1)
19-29: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winFailure classification depends on exact error wording from another file.
classifyFailurematches/not visible on chain yet/iagainsterror.message. This string must stay byte-for-byte in sync with the message thrown ininitial-deployment-funding.service.ts(`Lease for deployment ${dseq} owned by ${address} is not visible on chain yet`). If that message wording changes later, this silently falls through todeposit_tx_failedand theretriablemetric flag becomes wrong, with no compiler or test signal pointing at the real cause.Use a typed error (a small
LeaseNotVisibleErrorclass thrown at the source, checked withinstanceofhere) instead of message-text matching, so the classification survives message wording changes.♻️ Proposed fix using a typed error
+export class LeaseNotVisibleError extends Error {} + export function classifyFailure(error: unknown): FundingFailureReason { - if (!(error instanceof Error)) { + if (error instanceof LeaseNotVisibleError) { + return "lease_not_visible"; + } + + if (!(error instanceof Error)) { return "unknown"; } - if (/not visible on chain yet/i.test(error.message)) { - return "lease_not_visible"; - } - return "deposit_tx_failed"; }Then throw
new LeaseNotVisibleError(...)at the source instead of a plainError.🤖 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/deployment/services/initial-deployment-funding/initial-deployment-funding-instrumentation.service.ts` around lines 19 - 29, Replace the message-based check in classifyFailure with an instanceof check for a shared LeaseNotVisibleError type. Define or reuse this error class, update the lease visibility failure in the initial-deployment-funding service to throw LeaseNotVisibleError while preserving its diagnostic message, and keep non-typed errors classified as deposit_tx_failed or unknown as currently intended.
🤖 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/app/services/fund-deployment/fund-deployment.handler.ts`:
- Around line 28-40: The instrumentation calls in the job handler must be best
effort so logging failures cannot propagate to pg-boss after funding completes.
Update the success and failure paths around fundOnLeaseStarted, including
recordJobSucceeded and recordJobFailed, to catch or otherwise suppress
synchronous instrumentation errors while preserving the original funding error
and job outcome.
---
Nitpick comments:
In
`@apps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding-instrumentation.service.ts`:
- Around line 19-29: Replace the message-based check in classifyFailure with an
instanceof check for a shared LeaseNotVisibleError type. Define or reuse this
error class, update the lease visibility failure in the
initial-deployment-funding service to throw LeaseNotVisibleError while
preserving its diagnostic message, and keep non-typed errors classified as
deposit_tx_failed or unknown as currently intended.
🪄 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: d1c522c3-869d-4ac0-ae77-5958b4f06002
📒 Files selected for processing (6)
apps/api/src/app/services/fund-deployment/fund-deployment.handler.spec.tsapps/api/src/app/services/fund-deployment/fund-deployment.handler.tsapps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding-instrumentation.service.spec.tsapps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding-instrumentation.service.tsapps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding.service.spec.tsapps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding.service.ts
Record the deposit the moment it is confirmed on-chain and isolate the follow-up wallet reload scheduling so a scheduling failure no longer masquerades as a deposit failure or drops the deposit metric. Make the funding instrumentation logging best-effort so a synchronous logger failure cannot escape a record* call and make pg-boss retry an already-funded deployment.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding.service.ts (1)
64-75: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the scheduling-failure log call.
recordSkippedandrecordDepositisolate logger failures, and OpenTelemetry metric calls are no-throw.scheduleWalletReloadstill callsthis.logger.errorwithout protection. If logging fails, the confirmed deposit can become a retried job. Use a no-throw logging helper and add a regression test.🤖 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/deployment/services/initial-deployment-funding/initial-deployment-funding.service.ts` around lines 64 - 75, Protect the scheduling-failure logging in the initial funding flow around scheduleWalletReload so logger errors cannot escape after a confirmed deposit and trigger job retries. Reuse the existing no-throw logging helper pattern used by recordSkipped and recordDeposit, and add a regression test verifying a logger failure does not reject or retry the completed job.
🤖 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.
Outside diff comments:
In
`@apps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding.service.ts`:
- Around line 64-75: Protect the scheduling-failure logging in the initial
funding flow around scheduleWalletReload so logger errors cannot escape after a
confirmed deposit and trigger job retries. Reuse the existing no-throw logging
helper pattern used by recordSkipped and recordDeposit, and add a regression
test verifying a logger failure does not reject or retry the completed job.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12ab0f90-23c9-44df-b22a-cfcfd8e372c1
📒 Files selected for processing (4)
apps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding-instrumentation.service.spec.tsapps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding-instrumentation.service.tsapps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding.service.spec.tsapps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding.service.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding-instrumentation.service.spec.ts
- apps/api/src/deployment/services/initial-deployment-funding/initial-deployment-funding-instrumentation.service.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the reordering fixes the misclassification issue I flagged in my previous review (deposit is now recorded before the wallet-reload scheduling call, which is now isolated in its own try/catch). The one remaining finding is a genuine but low-probability nit (unguarded logger.error inside that catch), not blocking.
Extended reasoning...
Overview
This PR adds OTel metrics/logging instrumentation (InitialDeploymentFundingInstrumentationService) to the fund-on-create deployment job, mirroring the existing pattern used by the hourly top-up cron. It touches FundDeploymentHandler, InitialDeploymentFundingService, and the new instrumentation service, with corresponding spec updates.
Progress since last review
In my prior review pass on this PR, I flagged that a successful on-chain deposit could be misclassified as a non-retriable deposit_tx_failed because walletReloadJobService.scheduleImmediate() was awaited (and could throw) before instrumentation.recordDeposit() ran. The current diff has resolved this: recordDeposit is now called immediately after the deposit tx is confirmed, and the wallet-reload scheduling call has been extracted into a scheduleWalletReload helper wrapped in its own try/catch, so a scheduling failure no longer propagates up and misreports the already-successful deposit as a job failure.
Security risks
None specific to this change — it's additive telemetry around an existing money-moving flow; no new auth, crypto, or user-input handling is introduced. The reordering fix above also removes a false-positive-alert risk rather than introducing one.
Level of scrutiny
This is a money-moving job (), so I gave it a closer look despite being "just" instrumentation, since the diff also changes control flow (the try/catch extraction, log-event consolidation). The remaining finding is explicitly a nit: an unguarded logger.error call inside the new scheduleWalletReload catch block could in theory itself throw and re-escape, but this requires the logger call itself to fail synchronously, which is low-probability in practice, and the blast radius is bounded (a misleading metric/one wasted retry, no funds at risk). The PR's own emitLog helper in the instrumentation service already guards against this exact scenario, so the fix is a matter of consistency rather than correctness.
Other factors
Test coverage is thorough — 28 passing unit tests covering success, failure, skip, and deposit-recording paths, including a dedicated test for the wallet-reload-scheduling-failure case that motivated my previous comment. CodeRabbit and a human reviewer (non-blocking question about pg-boss metrics) have also reviewed without blocking concerns.
There was a problem hiding this comment.
No new issues found in this run. This PR touches the fund-on-create deposit path (money-moving job) and has already gone through two rounds of confirmed bugs across prior revisions (a deposit-vs-schedule-failure misclassification, and an unguarded logger throw in the reload-scheduling catch block) — both now fixed and covered by dedicated tests. Given the financial nature of the code path and that history, I'd like a human to confirm the final state before merge rather than auto-approve.
What was reviewed:
- The
InitialDeploymentFundingInstrumentationService'semitLogguard and the newscheduleWalletReloadisolation ininitial-deployment-funding.service.ts. - Confirmed the
recordDeposit-before-scheduleImmediatefix (fdca650) and the guardedlogger.errorcatch fix (de32320) are present in the current code and exercised by the new service tests. - Checked
classifyFailure's retriable taxonomy and the per-reason skip log-level mapping for other misclassification paths — none found beyond what was already flagged. - Confirmed the handler's try/catch preserves pg-boss retry semantics (rethrows after recording).
Extended reasoning...
Overview
This PR adds an OTel metrics/logging tier (InitialDeploymentFundingInstrumentationService) to the fund-on-create deposit job, wires it into InitialDeploymentFundingService and FundDeploymentHandler, consolidates several skip-reason logs into one structured event, and isolates the post-deposit wallet-reload scheduling call so a scheduling failure can't be misreported as a deposit failure.
Security risks
No auth/crypto/permission surface is touched. The residual risk is financial-correctness/observability: this job moves funds on-chain (escrow deposits), so a misclassification bug (like the two already caught in this PR's review history) could produce false alerts, mask a real failure, or cause unnecessary retries. Both previously identified issues (deposit-recording ordering vs. wallet-reload scheduling throw; an unguarded logger.error in the new isolation catch block) are fixed in the current code (commits fdca650, de32320) and each has a dedicated regression test.
Level of scrutiny
This is the kind of change that warrants at least a light human pass before merge: it touches the money-moving deposit path (apps/api/src/deployment/services/initial-deployment-funding), which falls under apps/*/src/ CODEOWNERS ownership, and the PR's own review history shows two rounds of genuine correctness bugs on subtle failure-classification edge cases — not the kind of mechanical change that's safe to rubber-stamp even with no new findings this run.
Other factors
Test coverage is solid: both previously-fixed bugs have targeted specs (deposit recorded before/despite a scheduling failure; job completes even if the failure-log call itself throws). CodeRabbit independently verified the fdca650 fix. The one open, non-blocking discussion (stalniy's pg-boss-metrics overlap question) was answered by the author and doesn't block merge. No new bugs were found in this run.
2 participants
Why
The fund-on-create deployment job (
FundDeploymentHandler->InitialDeploymentFundingService.fundOnLeaseStarted) deposits escrow funds right after a lease starts so high-cost deployments cannot drain their small initial deposit and close before the hourly top-up cron first sees them. Until now it was logs-only: no OTel metrics, unlike every other money-moving job (wallet-balance-reload-check, the hourlytop-up-managed-deploymentscron it mirrors, andactivate-trial). That left create-path deposit volume, skips, and failures invisible on dashboards and unalertable.Part of CON-735
What
Adds the metrics tier to the job, matching the hourly cron it mirrors:
InitialDeploymentFundingInstrumentationService(@singleton(), dedicated meterinitial-deployment-funding) exposingrecordJobSucceeded/recordJobFailed/recordDeposit/recordSkipped, each emitting a metric and a structured log together.initial_deployment_funding_job_completions_total({status}, plus{reason, retriable}on failure),_job_duration_ms,_deposits_total,_deposit_amount({denom}, raw base units),_skips_total({reason}).classifyFailuremaps the benign lease-not-visible indexer-lag throw toretriable=trueso it does not drown the failure signal; alerts can key offstatus="failure" AND retriable=false. The forensicINITIAL_FUNDING_TX_FAILEDlog stays in the service.INITIAL_FUNDING_INSUFFICIENT_BALANCE/_WALLET_NOT_FOUND/_NO_FEE_ALLOWANCEskip events collapse into a singleINITIAL_FUNDING_SKIPPEDevent carrying areasonattribute, at a severity chosen per reason (mirrors reload-check'sWALLET_BALANCE_RELOAD_SKIPPED).No DI registration needed (
@singleton()auto-resolves). Skips remain non-throwing and count as job successes.Verification
npm run test:unit -- initial-deployment-funding fund-deployment-> 28/28 passnpm run lint -- --quiet-> cleannpx tsc --noEmit-> 0 new errors vsorigin/mainbaselineFollow-up
initial_deployment_funding_*series appear in Grafana/Prometheus and build a panel/alert (status="failure" AND retriable=false) alongside the existingauto_top_up_*cron metrics. The pre-merge Grafana check for existing consumers of the now-collapsed granular skip event names could not be run from this environment (Grafana is behind the tailnet); the events are only days old so the risk is low, but worth a quick confirm.Summary by CodeRabbit
New Features
Bug Fixes
Tests