8d2cc4b3 - Run long commands as jobs, starting with the account merge - #4496
8d2cc4b3 - Run long commands as jobs, starting with the account merge#4496TaprootFreak wants to merge 8 commits into
Conversation
❌ ESLint: 2 errors, 1 warnings |
Measured in production, GET /v1/auth/mail/confirm takes 16.4 s at the 95th percentile and POST /v1/auth/mail 16.4 s — holding a request open for that is the wrong shape. Commands whose work no longer fits inside a request now persist a job, answer with a ticket, and a dispatcher executes it. Three pieces: The API gains its own OpenTelemetry metrics. Until now it exported only traces and Prometheus does not scrape it, so every API metric was derived from spans. That cannot carry state — queue depth or a job that is currently over its allowed time are not derivable from spans. The same instrumentation gives the 149 existing cron methods duration, outcome and a heartbeat, which they never had. The job core claims rows with a conditional status update, so two runners can never both process one job — including across instances, which the process-local lock never covered. That is what removes the need for any timing heuristic. Idempotency sits on a unique index rather than in code: two concurrent enqueues of the same business key cannot both create a job, so a retried confirmation link is harmless. A job carries the attempt limit it was accepted under, so a later configuration change cannot re-budget work already in flight. The merge endpoint waits up to 900 ms for the result and only then hands out a ticket. Below that, clients see exactly today's response, including the access token; 400 and 409 still come synchronously and without creating a job. The token is minted in the HTTP context, never stored: it depends on request state and would be a stored credential in the job table. The leading metric for stuck work is a gauge, not a counter. A completion counter is incremented when a job finishes, so a job that never finishes never increments it and cannot be reported. The configured limit is exported as its own series so dashboards and alerts follow a settings change without a deploy.
The new rule reserves the `wait` segment for routes that only wait for someone else to act. `GET /v1/auth/mail/confirm` enqueues the account-merge job, kicks the dispatcher and waits up to 900 ms for its result — the duration measures work the API itself started, so per the rule it must NOT carry the segment and stays visible in latency monitoring. Recording it next to the other examples so the decision is where a reader looks for it.
543c2a3 to
0894ff1
Compare
The require() inside jest.isolateModules violated no-require-imports; the same assertion now runs through isolateModulesAsync with a dynamic import, so the positive proof stays intact — one metric reader, 15000 ms, and no second one on a repeated call. The controller test asserted the absence of a `result` property via hasOwnProperty. With target es2023 every declared class field materialises as an own property on `new JobDto()`, and Object.assign — the repo's mapper convention — does not remove it. The property exists but carries no value, and JSON.stringify drops it, so the client never sees it. The assertion now checks the serialised response in both directions, which is what actually matters. The auth controller spec pulled TfaService in as a DI token and thereby the whole KYC entity chain, which is circular at import time: StepLog extends KycLog ran while KycLog was still undefined and took the entire suite down. Mocking that one module keeps the chain out of this spec; all three cases are unchanged.
The auth spec mocked src/.../tfa.service but exported only TfaService from it. The module also exports the TfaLevel enum, so every consumer of it got undefined and validation decorators failed with 'Cannot convert undefined or null to object' — the whole suite could not load. Both existing mocks of that module in this repo list TfaLevel; this one now matches them. Prettier over the five files the pipeline flagged.
The suite could not compile: AuthController's routes carry IpCountryGuard and RateLimitGuard, and Nest instantiates both when building the module. IpCountryGuard injects IpLogService, RateLimitGuard needs the throttler options — neither was present, so every test in the file failed on module creation rather than on anything it asserts. All four cases pass now: immediate result with token, pending job answered with the ticket, and a conflict from the pre-check that does not enqueue.
package.json declares the OTLP metrics exporter and sdk-metrics as direct dependencies, but the lockfile's root dependency block still listed only the trace exporter — both packages were present transitively via sdk-node, so installs worked while the manifest and the lockfile disagreed.
Two blocking defects from the review. The ticket path went nowhere. Past 900 ms the client got a ticket and could never collect its result: the generic status endpoint withholds it for this group by design, and calling the endpoint again hit 'Merge request is already completed'. The user saw an error for a merge that had in fact succeeded, and got neither kycHash nor access token. The endpoint now looks the job up by its idempotency key first and returns the result with a freshly minted token; the conflict remains only for merges completed before this mechanism existed, where no job row was ever written. Job rows overwrote claimedAt, claimedBy and error on every attempt, so after three tries the first two failures were unrecoverable — against the CRITICAL 'auditable mutations' rule. job_attempt now keeps one immutable row per attempt with owner, timing, outcome and the full error. It is written after a won claim, not before: the claim is a race, and writing first would leave every loser a row for an attempt it never made. It is closed before the snapshot update, as the rule requires. Also from the review: recoverStale now uses the same conditional update as claimNext, so it cannot overwrite a job that finished in the meantime, and it respects the attempt budget instead of running an exhausted job once more. Persistence failures on completion are re-thrown instead of being counted as a business failure - a metric claiming an outcome the database does not know makes the numbers useless. And the public DTO no longer leaks internal error text: dead-letter messages are statements about the job and stay, failed ones are replaced by a fixed text with the ticket number.
Three blocking findings from the final review. The first one this PR introduced itself. Closing the ticket dead end meant looking the job up before the isCompleted check — and that check was not only state logic, it was the only thing that spent the confirmation link. With it bypassed, anyone holding the merge code could collect a fresh access token for the merged account over and over, up to 30 days for IBAN and ident merges, and createAccessTokenAfterMerge falls back to generateAccountToken without verifying the caller belongs to that account. The result is now collectable only within 15 minutes of completion; the waiting client polls in seconds and gives up after ten minutes, so it is fully covered, and afterwards the link is spent as it always was. finish and abort wrote with an unconditional update by id. If recoverStale had meanwhile handed the job to another runner, the original one overwrote the new owner's state — and mergeUserData could run twice for the same pair. Both now update only while the row still carries the attempt and owner they claimed; a runner that lost the job logs and returns instead of overruling the winner. closeAttempt only writes to an attempt row that is still open. The kill switch only ever ran inside the cron wrapper, so kick() — which the code itself calls the normal path — was never gated. Operations could not actually stop merges while requests kept arriving. drain() now checks the group's process first.
|
Review complete: two rounds, two lenses each (conformance against Round one surfaced eleven, two of them blocking, both fixed: the ticket path was a dead end past 900 ms — the client could never collect its result and hit Round two surfaced nine, three of them blocking. All three are fixed here. The first was introduced by the round-one fix itself, which is worth stating plainly. Closing the dead end meant looking the job up before the
The kill switch was only evaluated inside the cron wrapper, so The remaining seven are not merge-blocking and are tracked in #4561 — the On the bot comment above: it is from 2026-07-30 11:24, several pushes ago. The review job at the current head reports CI is green on the latest push: API PR CI, API Migration Check, PR Review Bot, CodeQL and CodeQL Advanced. Gates were additionally run on a dedicated build host — prettier, lint, type-check and 111 tests across 14 suites. Both migrations were applied and rolled back against a real PostgreSQL 16 before they were committed. |
1 participant
Why
Two endpoints, measured in production over 24 h against the trace-derived span metrics:
GET /v1/auth/mail/confirm(account merge)POST /v1/auth/mailPOST /v1/authHolding a request open for sixteen seconds is not a tuning problem, it is the wrong call shape. Commands whose work no longer fits inside a request now persist a job, answer with a ticket, and a dispatcher executes it.
Deliberately not in scope: slow read endpoints (
GET /v1/fiat,/v1/dashboard/financial/log,/v1/statistic/status,/v1/asset— together roughly 6 700 calls over 1 s per day). A ticket on a data query is a regression; those are caching and query problems. This mechanism is for commands with side effects.What
Metrics first. The API had no metrics of its own — it exported only traces, and Prometheus does not scrape it, so every API metric was derived from spans. Spans cannot carry state: queue depth, or how many jobs are currently over their allowed time, are not derivable from them.
MetricServiceis a thin facade over the OpenTelemetry metrics API;tracing.tsgains a periodic exporter on the existing OTLP path. The same instrumentation gives the 149 existing@DfxCronmethods duration, outcome, skip-count and a heartbeat — none of which they had before. A set kill switch was previously indistinguishable from healthy operation.The job core (
src/subdomains/supporting/job/): entity, migration, service, dispatcher, metric snapshot, andGET /v1/job/:uid. OneProcesskill switch per group, as the house rule requires.The merge endpoint waits up to 900 ms and only then hands out a ticket.
Design decisions worth reviewing
Claiming, not a timing threshold. A conditional status update claims a row; only the runner whose update reports
affected: 1owns it. Two runners can never both process one job — including across instances, which the process-local lock never covered. This is why there is no age threshold anywhere in the dispatcher: the correctness does not rest on guessing how long a send takes.Idempotency on a unique index, not in code. Two concurrent enqueues of the same business key cannot both create a job; the loser catches the unique violation (Postgres
23505specifically, everything else is rethrown) and loads the winner's row. A retried confirmation link is therefore harmless.A job carries the limit it was accepted under.
maxAttemptsis persisted and read from the row, not from live configuration — otherwise a settings change would re-budget work already in flight.Three terminal states, not two.
Failedis exhausted attempts;DeadLetteris a task no retry can ever complete. Separating them keeps an unworkable job from burning attempts while still making it visible, instead of being skipped silently.The leading metric for stuck work is a gauge. A completion counter is incremented when a job finishes — so a job that never finishes never increments it and cannot be reported at all.
dfx_job_over_slais computed from database state and reports a stuck job while it is stuck. It is fed from an in-memory snapshot refreshed by a cron, because gauge callbacks run on the export cadence and a query inside them would tie metric export to database response time.The configured limit is exported as its own series (
dfx_job_sla_seconds), so dashboards and alerts follow a settings change without a deploy and without a second copy of the threshold. ThejobGroupssetting is validated against a DTO on write, so a typo in a group name is rejected rather than silently falling back to the default.The heartbeat attests a successful sweep, not merely that the method was entered — it is not updated when the sweep itself fails, so a permanently failing sweep stays detectable.
No access token is persisted. The merge job returns only
kycHashand the master account id; the token depends on request state (address, IP, 2FA marker) and is minted in the HTTP context on retrieval. Job groups carry anexposeResultflag,falsefor the merge, so the generic status endpoint shows progress but never result data.Backward compatibility: below 900 ms clients see exactly today's response including the token;
400(Invalid link) and409(Merge is already completed) still come synchronously and without creating a job.executeMergeitself is untouched — it is only wrapped.Verification
format:check,lint,type-checkand the affected Jest suites run on a dedicated build host.After the first review round
Eleven findings, two of them blocking. Both are fixed.
The ticket path was a dead end. Past 900 ms the client received a ticket and could never collect its result: the generic status endpoint withholds it for this group by design (
exposeResult: false), and calling the endpoint again ran intoMerge request is already completed. The user saw an error for a merge that had in fact succeeded, and got neitherkycHashnor an access token — so they could not reach their account. The endpoint now looks the job up by its idempotency key before anything else and returns the result with a freshly minted token. The conflict remains only for merges completed before this mechanism existed, where no job row was ever written. The long path — the one this feature exists for — is now the one that is tested.Job rows destroyed their own history.
claimedAt,claimedByanderrorwere overwritten on every attempt, so after three tries the first two failures were unrecoverable. That is what the CRITICAL „auditable mutations" rule forbids.job_attemptnow holds one immutable row per attempt with owner, timing, outcome and the full error. It is written after a won claim, not before — the claim is a race, and writing first would leave every loser a row for an attempt it never made — and closed before the snapshot update, as the rule requires.Also fixed from that round:
recoverStaleuses the same conditional update asclaimNext, so it can no longer overwrite a job that finished between the read and the write, and it respects the persisted attempt budget instead of running an exhausted job once more. Persistence failures on completion are re-thrown rather than counted as a business failure — a metric claiming an outcome the database does not know makes the numbers useless. The public DTO no longer passes internal error text outward: dead-letter messages are statements about the job and stay, failed ones are replaced by a fixed text carrying the ticket number, and every other state reports no error at all (a job inRetryholds the raw message of its last attempt).Plus the smaller ones: enum values in PascalCase per the naming rule — note that these travel outward as a metric label, so the alerting side was adjusted in the same breath — a dead logger removed, one German comment translated, and the lockfile brought in sync with the two dependencies
package.jsondeclares.On the
waitnaming rule: this endpoint enqueues the job, kicks the dispatcher and only then waits for the result, so it falls under the rule's explicit exemption for endpoints awaiting work they started themselves. It therefore keeps its path and stays visible in latency monitoring — which is also where the improvement from 16.4 s should become measurable. The reviewer confirmed that reading independently against the comparable paths in the repo.