diff --git a/migration/1785470000000-AddTradingOrderCreatedIndex.js b/migration/1785470000000-AddTradingOrderCreatedIndex.js new file mode 100644 index 0000000000..487fed0f27 --- /dev/null +++ b/migration/1785470000000-AddTradingOrderCreatedIndex.js @@ -0,0 +1,137 @@ +/** + * @typedef {import('typeorm').MigrationInterface} MigrationInterface + * @typedef {import('typeorm').QueryRunner} QueryRunner + */ + +/** + * Add a single-column index on `trading_order ("created")` so the per-minute financial-log + * yield query stops doing a full sequential scan on this 5.4-million-row / 922 MB table. + * + * The query is: + * `SELECT SUM("tradingOrder"."profitChf") AS "profit", SUM("tradingOrder"."txFeeAmountChf") AS "fee" + * FROM "trading_order" "tradingOrder" WHERE "tradingOrder"."created" >= $1`. + * Source: `TradingOrderService.getTradingOrderYield` + * (`src/subdomains/core/trading/services/trading-order.service.ts:51-60`), called from + * `LogJobService` (`src/subdomains/supporting/log/log-job.service.ts:1154`) with + * `firstDayOfMonth` — i.e. once per minute, because that is where the `LogJobService` writes the + * financial log. + * + * Production `EXPLAIN (ANALYZE, BUFFERS)` for the actual query predicate — `created >= '2026-07-01 + * 00:00:00'` (`Util.firstDayOfMonth()` always returns the first day of the month, see + * `src/shared/utils/util.ts:410-412`) — showed a Parallel Seq Scan on `trading_order`, Rows Removed + * by Filter 1,765,454 (x3 workers), rows=41,306 (x3 workers) = 123,918 matching rows (2.3% of the + * table), Buffers shared hit=49 read=89282, Execution Time 141.283 ms. This is practically the same + * cost profile as the scan the sibling migration (`AddLedgerContentChangeScanIndexes`) already fixed + * for a different query — just from a different source, found via high-frequency + * `pg_stat_activity` sampling after the sibling migration's indexes were already live in production + * and being used (confirmed via `pg_stat_user_indexes`), yet the seq-scan load on `trading_order` + * had not gone down (45s-window delta: 15 scans, 27.1M rows, 3,486 MB). The cost of this scan is + * practically independent of the date filter, because a Seq Scan reads the whole table regardless of + * how selective the predicate is: a separate measurement with `created >= '2026-07-30 00:00:00'` + * (only 454 matching rows) still cost Execution Time 146.399 ms and Buffers read=88994 — almost + * identical to the 141.283 ms / read=89282 above. What matters for the index decision is the + * query's selectivity (2.3% of the table matches `created >= firstDayOfMonth`), not the runtime of + * any one scan. Stated honestly: that the Postgres planner will actually pick this new index for + * this query is NOT verified in production, because the index does not yet exist there — the + * decision to add it rests on the measured selectivity (2.3%) and the correlation of `created` to + * physical row order (0.796, discussed below), not on an observed plan change. + * + * Why a single-column index and not a covering index: the monthly window matched 123,918 of + * 5.4 million rows (2.3%) at measurement time — a snapshot, since the table grows by roughly + * 4,350 rows/day; the stable figure the decision rests on is the ~2.3% selectivity, not the + * absolute row count. That is clearly within the range where an index scan beats a sequential scan. + * The correlation of `created` to physical row order is 0.796 per `pg_stats`, so heap accesses via + * the index are largely sequential. An `INCLUDE (profitChf, txFeeAmountChf)` would bloat the index + * from an estimated ~103 MB to ~190 MB, and would additionally depend on an up-to-date visibility + * map for index-only scans to pay off — which is not reliably available here (`last_autovacuum` is + * NULL on this table). That trade-off is the reason to omit `INCLUDE`. + * + * Why only `trading_order` and not the other tables queried in the same `LogJobService` block: + * `buy_fiat`, `buy_crypto`, and `crypto_input` are also queried with `created >= firstDayOfMonth` and + * have no index containing `created`. `bank_tx` does have one — `IDX_bank_tx_type_created` on + * `("type", "created")`, added by `1784117860216-AddBankTxTypeCreatedIndex.js` for + * `BankTxService.getBankTxFee`, the same function `LogJobService` calls in the same minute — but it + * only serves one of `getBankTxFee`'s three sub-queries (the `type = ... AND created >= ...` one); + * with `type` leading, it can't serve the legacy sum (`created` only, no `type` predicate) or the + * inline-charges query (`type IS NULL OR type != ...`). All four tables are excluded here regardless, + * because they are orders of magnitude smaller (`crypto_input` 234 MB, `bank_tx` 147 MB, `buy_crypto` + * 100 MB, `buy_fiat` 35 MB, versus `trading_order` at 922 MB / 5.4 million rows), and no production + * measurement exists to justify an index for them. This is a deliberate, considered exclusion, not an + * oversight. + * + * CREATE INDEX CONCURRENTLY is not used: migrations in this codebase run transactionally and + * boot-blockingly (see src/config/config.ts, migrationsRun gated by the SQL_MIGRATE env var). + * CREATE INDEX CONCURRENTLY is not allowed inside a transaction and would crash the migration. + * + * Lock behaviour, stated precisely: all pending migrations run inside a single database + * transaction (TypeORM default `migrationsTransactionMode: "all"`), and PostgreSQL only releases + * locks at COMMIT, not at the end of each statement. Evidence: + * `node_modules/typeorm/data-source/DataSource.js:263-265` (`migrationExecutor.transaction = + * options?.transaction || this.options?.migrationsTransactionMode || "all"` — default `"all"`); + * `src/config/config.ts:265` only sets `migrationsRun` and never overrides + * `migrationsTransactionMode` (corroborated by the comment in + * `src/shared/models/asset/__tests__/add-binance-custody-assets-ondo-ada.migration.spec.ts:348` — + * "no migrationsTransactionMode override → default 'all'"); + * `node_modules/typeorm/migration/MigrationExecutor.js:206` starts one transaction for pending + * migrations and commits only at the end. A plain CREATE INDEX holds a SHARE lock for the entire + * build; reads continue throughout, but writes to the table are blocked while that lock is held. + * Because locks are held until COMMIT, if other migrations are pending in the same batch, this + * index's SHARE lock is held until all of them commit together, not just until this statement + * finishes. `SET LOCAL lock_timeout` bounds only how long we wait to ACQUIRE the lock, not how + * long we hold it once acquired, and it is scoped to the whole transaction — that is exactly why + * it is set only once at the top of `up()` and once at the top of `down()`, not per statement + * (there is only one `CREATE INDEX` / one `DROP INDEX` in this migration). + * + * Production reference data point only: the nine `(updated, id)` indexes in the sibling migration + * (`AddLedgerContentChangeScanIndexes`) built in under 40 seconds combined in production, with + * zero lock-wait. That figure is a reference data point, NOT a measured or asserted upper bound + * for this index's build time, and must NOT be treated as a hard ceiling. + * + * Risk framing: this migration runs boot-blockingly at app startup (`migrationsRun`, gated by the + * `SQL_MIGRATE` env var), so the starting instance itself is not yet serving requests and is not + * itself a writer. Concurrent writers would be a still-running predecessor instance during a + * rolling deploy, or external consumers. If a lock conflict occurs, the migration aborts after + * `lock_timeout` and so does the app start — that is fail-closed and intentional, but it is a + * deploy abort and must be named as such. + * + * `down()` reverses this with `DROP INDEX` and is subject to a stricter lock: PostgreSQL takes an + * ACCESS EXCLUSIVE lock for `DROP INDEX` (vs. the SHARE lock `CREATE INDEX` takes above), and + * ACCESS EXCLUSIVE conflicts with every other lock mode, including the AccessShareLock a plain + * `SELECT` takes — so `down()` blocks reads as well as writes, not writes alone. Because `down()` + * also runs inside the single-batch transaction (same TypeORM default + * `migrationsTransactionMode: "all"`), the same hold-until-COMMIT reasoning applies. + * + * Index name: `IDX_280bd108621ace81e32f26d224` on `trading_order ("created")`. + * This is not an arbitrary name but the deterministic name TypeORM's DefaultNamingStrategy would + * generate itself, since custom index naming is disallowed by CONTRIBUTING.md. The name is + * `IDX_` followed by the first 26 hex characters of `sha1('trading_order_created')` (table name + + * `_` + column name, per TypeORM's DefaultNamingStrategy). + * + * @class + * @implements {MigrationInterface} + */ +module.exports = class AddTradingOrderCreatedIndex1785470000000 { + name = 'AddTradingOrderCreatedIndex1785470000000'; + + /** + * @param {QueryRunner} queryRunner + */ + async up(queryRunner) { + // SET LOCAL is scoped to the whole transaction. Bounds WAIT time to acquire the lock, not how + // long the lock is held. Set once: this migration has a single CREATE INDEX statement. + await queryRunner.query(`SET LOCAL lock_timeout = '5s'`); + await queryRunner.query( + `CREATE INDEX "IDX_280bd108621ace81e32f26d224" ON "trading_order" ("created")`, + ); + } + + /** + * @param {QueryRunner} queryRunner + */ + async down(queryRunner) { + // SET LOCAL is scoped to the whole transaction. Bounds WAIT time to acquire the lock, not how + // long the lock is held. Set once: this migration has a single DROP INDEX statement. + await queryRunner.query(`SET LOCAL lock_timeout = '5s'`); + await queryRunner.query(`DROP INDEX "public"."IDX_280bd108621ace81e32f26d224"`); + } +}; diff --git a/src/subdomains/supporting/dashboard/dashboard-financial.service.ts b/src/subdomains/supporting/dashboard/dashboard-financial.service.ts index 42fa6b397d..14695af6ef 100644 --- a/src/subdomains/supporting/dashboard/dashboard-financial.service.ts +++ b/src/subdomains/supporting/dashboard/dashboard-financial.service.ts @@ -233,15 +233,18 @@ export class DashboardFinancialService { } // Pure mapping over the SQL projection: for well-formed data, the response matches the previous - // mapLogToEntry path exactly, including the `?? 0` field defaults. One case is intentionally - // different: if a `balancesByFinancialType` entry has the value `null` (e.g. `{"Crypto": null}`), - // the row is now kept (with an empty balancesByType entry) instead of the previous mapLogToEntry's - // per-row try/catch silently dropping the whole log line — a silent gap in a financial curve is - // worse than an empty partial entry, so this was changed on purpose. A malformed `message` document - // still fails loud: the `message::jsonb` cast in SQL throws for that. Individual scalar field - // values, though, are tolerated via `jsonb_typeof` guards (nulled rather than thrown), and - // non-numeric values inside `balancesByFinancialType` are passed through unchanged - // (see log.repository.ts). + // mapLogToEntry path exactly, including the `?? 0` field defaults. Two cases are intentionally + // different from that old path: a `balancesByFinancialType` entry with the value `null` (e.g. + // `{"Crypto": null}`) now keeps the row (with an empty balancesByType entry) instead of the old + // per-row try/catch dropping the whole log line; and a wrongly-typed `balancesByFinancialType` + // property (string, boolean, `null`) — previously passed through unchanged, breaking the + // `number | undefined` DTO contract — now becomes `undefined` (see the `asNumber` guard in + // log.repository.ts). Both changes favour a visible gap over a silently dropped row. The same + // reasoning covers a top-level `message: null` (not a sub-field, the whole document): the old + // path threw on the following property access and dropped the row, the new one keeps it as a + // zero point via the SQL projection and the `?? 0` defaults below — not seen in current + // production data. A malformed `message` document still fails loud (the `message::jsonb` cast + // throws in SQL); individual scalar fields are only nulled via `jsonb_typeof` guards. private mapSummaryToEntry(summary: FinancialLogSummary): FinancialLogEntryDto { return { timestamp: summary.created, diff --git a/src/subdomains/supporting/log/__tests__/log.repository.spec.ts b/src/subdomains/supporting/log/__tests__/log.repository.spec.ts index 98ba6df5d4..adc4e70447 100644 --- a/src/subdomains/supporting/log/__tests__/log.repository.spec.ts +++ b/src/subdomains/supporting/log/__tests__/log.repository.spec.ts @@ -547,6 +547,41 @@ describe('LogRepository', () => { expect(rows[0].btcPriceChf).toBe(65000.25); }); + // This raw shape is what the SQL projection produces for a top-level `message: null` JSON + // document. The old mapLogToEntry/JSON.parse path threw on the resulting property access and + // dropped the whole line for this case; not observed in production. The downstream null-to-0 + // mapping for btcPriceChf (mapSummaryToEntry) is covered separately in + // dashboard-financial.service.spec.ts:91, not exercised by this repository-level test. + it('keeps the row and passes all number fields through as null, sets btcPriceChf to 0, and returns balancesByType as an empty object for a raw row where all projected number fields and balancesByFinancialType are null (F20b)', async () => { + const repo = new LogRepository({} as EntityManager); + const created = new Date('2026-07-14T00:00:00Z'); + jest.spyOn(repo, 'query').mockResolvedValue([ + { + created, + id: 3, + // Same raw shape the SQL projection produces for a top-level JSON `null` message: every + // `jsonb_typeof(message::jsonb -> ...)` guard sees a non-object/non-number operand and nulls + // its field, same as a missing key. + totalBalanceChf: null, + plusBalanceChf: null, + minusBalanceChf: null, + fxPnlChf: null, + btcPriceChf: null, + balancesByFinancialType: null, + }, + ]); + + const rows = await repo.getFinancialLogSummaries(7); + + expect(rows).toHaveLength(1); + expect(rows[0].totalBalanceChf).toBeNull(); + expect(rows[0].plusBalanceChf).toBeNull(); + expect(rows[0].minusBalanceChf).toBeNull(); + expect(rows[0].fxPnlChf).toBeNull(); + expect(rows[0].btcPriceChf).toBe(0); + expect(rows[0].balancesByType).toEqual({}); + }); + it('maps a raw row with JSON null for only totalBalanceChf/plusBalanceChf (minusBalanceChf/fxPnlChf unaffected) to null/null, not 0 (F13)', async () => { const repo = new LogRepository({} as EntityManager); const created = new Date('2026-07-14T00:00:00Z'); diff --git a/src/subdomains/supporting/log/log.repository.ts b/src/subdomains/supporting/log/log.repository.ts index 6da5244332..79f1becb03 100644 --- a/src/subdomains/supporting/log/log.repository.ts +++ b/src/subdomains/supporting/log/log.repository.ts @@ -521,19 +521,22 @@ ${limitClause}`; string, { plusBalanceChf?: number; minusBalanceChf?: number } >; + // Only real numbers are kept for plusBalanceChf / minusBalanceChf; any non-number value + // (string, boolean, null, nested object, or missing key) becomes undefined so the result + // matches the number | undefined contract. On current production data this is a no-op + // (287,989 entries both numbers, one missing plusBalanceChf key — no string/boolean/null), + // and exists only to protect the contract for future/other data. The previous mapLogToEntry + // passed contract-breaking values through unchanged; this closes that hole. Same hardening + // idea as the five scalar fields above (jsonb_typeof = 'number' in SQL), applied in + // TypeScript because balancesByFinancialType is passed through as a raw JSON object. Note: + // this is a type check, not a finiteness check — it also lets `Infinity` through (e.g. from + // a JSON number like `1e999`, which `JSON.parse` turns into `Infinity`); `NaN` cannot occur + // in valid jsonb. + const asNumber = (v: unknown): number | undefined => (typeof v === 'number' ? v : undefined); for (const [type, data] of Object.entries(byType)) { // Optional chaining keeps non-object entries (null / number / string / boolean) from throwing: // property access yields undefined and the row is retained with empty fields, rather than // failing the whole request. - // Only real numbers are kept for plusBalanceChf / minusBalanceChf; any non-number value - // (string, boolean, null, nested object, or missing key) becomes undefined so the result - // matches the number | undefined contract. On current production data this is a no-op - // (287,989 entries both numbers, one missing plusBalanceChf key — no string/boolean/null), - // and exists only to protect the contract for future/other data. The previous mapLogToEntry - // passed contract-breaking values through unchanged; this closes that hole. Same hardening - // idea as the five scalar fields above (jsonb_typeof = 'number' in SQL), applied in - // TypeScript because balancesByFinancialType is passed through as a raw JSON object. - const asNumber = (v: unknown): number | undefined => (typeof v === 'number' ? v : undefined); balancesByType[type] = { plusBalanceChf: asNumber(data?.plusBalanceChf), minusBalanceChf: asNumber(data?.minusBalanceChf),