diff --git a/backend/__tests__/services/logActivityTrx.test.js b/backend/__tests__/services/logActivityTrx.test.js new file mode 100644 index 00000000..b31be732 --- /dev/null +++ b/backend/__tests__/services/logActivityTrx.test.js @@ -0,0 +1,155 @@ +/** + * Regression tests for logActivity calls inside transactions (#850 review + * find). createContract / updateContract / createStorno / reissueInvoice + * called logActivity() (and contract paths also adminActor()) from inside + * a knex transaction WITHOUT the trx executor. On single-connection SQLite + * the audit insert then waits on a second pool connection while the trx + * holds the only one — a 60s acquire-timeout stall per call, after which + * logActivity's catch swallows the failure and the audit row is silently + * lost. Postgres was unaffected. + * + * The observable fix: the activity_logs rows now exist, and the calls + * complete without waiting on the pool. The shrunken acquire timeout + * below makes any reintroduced deadlock fail the test quickly instead + * of appearing to pass after a long stall. + */ +const path = require('path'); +const { + bootCrmDb, seedMinimal, assignAdminRole, +} = require('../integration/helpers/crmDb'); + +jest.setTimeout(120000); + +let db; +let cleanup; +let tmpDir; +let adminId; +let customerId; +let contractService; +let invoiceService; + +const prevCwd = process.cwd(); + +beforeAll(async () => { + ({ db, cleanup, tmpDir } = await bootCrmDb()); + // Business-doc artifacts land under process.cwd()/storage — isolate. + process.chdir(tmpDir); + + // A reintroduced in-trx pool grab should fail fast (2s), not stall 60s. + db.client.pool.acquireTimeoutMillis = 2000; + + // node-sqlite3 detects Date bindings via the NATIVE realm's Date — + // under jest's vm sandbox that check fails and Dates stringify to + // "[object Object]". Normalize to ISO strings on the client prototype + // (transaction clients are Object.create()d from it). Same shim as + // crmMintPaths.test.js. + const clientProto = Object.getPrototypeOf(db.client); + const origQuery = clientProto._query; + clientProto._query = function patchedQuery(connection, obj) { + if (obj && Array.isArray(obj.bindings)) { + obj.bindings = obj.bindings.map( + (b) => (b && typeof b === 'object' && typeof b.toISOString === 'function' ? b.toISOString() : b), + ); + } + return origQuery.call(this, connection, obj); + }; + + ({ adminId, customerId } = await seedMinimal(db)); + await assignAdminRole(db, adminId, 'super_admin'); + + contractService = require('../../src/services/contractService'); + invoiceService = require('../../src/services/invoiceService'); +}, 120000); + +afterAll(async () => { + process.chdir(prevCwd); + if (cleanup) await cleanup(); +}); + +test('createContract persists the contract_created audit row (was silently lost on SQLite)', async () => { + const contractId = await contractService.createContract({ + customerAccountId: customerId, + title: 'Audit-Trail-Vertrag', + }, adminId); + + const row = await db('activity_logs') + .where({ activity_type: 'contract_created' }) + .orderBy('id', 'desc') + .first(); + expect(row).toBeTruthy(); + expect(JSON.parse(row.metadata).contractId).toBe(contractId); + expect(row.actor_type).toBe('admin'); +}); + +test('updateContract persists the contract_updated audit row', async () => { + const contractId = await contractService.createContract({ + customerAccountId: customerId, + title: 'Vorher', + }, adminId); + + await contractService.updateContract(contractId, { title: 'Nachher' }, adminId); + + const row = await db('activity_logs') + .where({ activity_type: 'contract_updated' }) + .orderBy('id', 'desc') + .first(); + expect(row).toBeTruthy(); + expect(JSON.parse(row.metadata).contractId).toBe(contractId); +}); + +test('cancelInvoice (Storno mint) persists the invoice_cancelled_via_storno audit row', async () => { + const { invoiceIds } = await invoiceService.createInvoice({ + customerAccountId: customerId, + currency: 'CHF', + vatRate: 0, + lineItems: [ + { position: 1, quantity: 1, description: 'Coverage', unit_price_minor: 100000, discount_percent: 0 }, + ], + }, adminId); + const id = invoiceIds[0]; + await db('invoices').where({ id }).update({ status: 'sent', sent_at: new Date(), updated_at: new Date() }); + + const result = await invoiceService.cancelInvoice(id, adminId); + expect(result.cancelled).toBe(true); + + const row = await db('activity_logs') + .where({ activity_type: 'invoice_cancelled_via_storno' }) + .orderBy('id', 'desc') + .first(); + expect(row).toBeTruthy(); + const meta = JSON.parse(row.metadata); + expect(meta.invoiceId).toBe(id); + expect(meta.stornoId).toBe(result.stornoId); +}); + +test('reissueInvoice completes on SQLite and persists the invoice_reissued audit row', async () => { + const { invoiceIds } = await invoiceService.createInvoice({ + customerAccountId: customerId, + currency: 'CHF', + vatRate: 0, + lineItems: [ + { position: 1, quantity: 1, description: 'Album', unit_price_minor: 50000, discount_percent: 0 }, + ], + }, adminId); + const id = invoiceIds[0]; + await db('invoices').where({ id }).update({ status: 'sent', sent_at: new Date(), updated_at: new Date() }); + + // Pre-fix this stalled inside the wrapping transaction (createInvoice's + // global-connection reads vs. the single-connection pool) and aborted + // before the replacement existed — with the Storno already committed. + const result = await invoiceService.reissueInvoice(id, adminId); + expect(result.id).toBeGreaterThan(0); + expect(result.replaces).toBe(id); + + const replacement = await db('invoices').where({ id: result.id }).first(); + expect(replacement.replaces_invoice_id).toBe(id); + + const row = await db('activity_logs') + .where({ activity_type: 'invoice_reissued' }) + .orderBy('id', 'desc') + .first(); + expect(row).toBeTruthy(); + expect(JSON.parse(row.metadata).newInvoiceId).toBe(result.id); +}); + +void path; // referenced for parity with sibling suites diff --git a/backend/src/services/businessProfileService.js b/backend/src/services/businessProfileService.js index 492a2996..7922aaff 100644 --- a/backend/src/services/businessProfileService.js +++ b/backend/src/services/businessProfileService.js @@ -230,18 +230,21 @@ function sanitiseBankPayload(payload) { * Always returns a profile object even if the row is empty — the * Settings UI binds straight to this shape. */ -async function getProfile() { +// `conn` lets transaction callers (invoice/create.js) route the reads +// through their trx — on single-connection SQLite a global-db read +// inside an open trx deadlocks the pool (codex review of #851). +async function getProfile(conn = db) { return await withRetry(async () => { - let profile = await db('business_profile').where({ id: 1 }).first(); + let profile = await conn('business_profile').where({ id: 1 }).first(); if (!profile) { // Belt-and-braces: migration 102 seeds id=1, but if a fresh install // ran an earlier rollback that wiped the row, re-create it so the // service never throws. - await db('business_profile').insert({ id: 1 }); - profile = await db('business_profile').where({ id: 1 }).first(); + await conn('business_profile').insert({ id: 1 }); + profile = await conn('business_profile').where({ id: 1 }).first(); } - const accounts = await db('business_bank_accounts') + const accounts = await conn('business_bank_accounts') .where({ business_profile_id: 1 }) .orderBy('display_order', 'asc') .orderBy('id', 'asc'); @@ -344,23 +347,23 @@ async function deleteBankAccount(id, adminId) { * given currency: explicit override → default for that currency → * default for the profile's default_currency → first by display_order. */ -async function resolveBankAccountForCurrency(currency, overrideId = null) { +async function resolveBankAccountForCurrency(currency, overrideId = null, conn = db) { return await withRetry(async () => { if (overrideId) { - const explicit = await db('business_bank_accounts').where({ id: overrideId }).first(); + const explicit = await conn('business_bank_accounts').where({ id: overrideId }).first(); if (explicit) return explicit; } if (currency) { - const match = await db('business_bank_accounts') + const match = await conn('business_bank_accounts') .where({ business_profile_id: 1, currency, is_default: formatBoolean(true) }) .first(); if (match) return match; } - const anyDefault = await db('business_bank_accounts') + const anyDefault = await conn('business_bank_accounts') .where({ business_profile_id: 1, is_default: formatBoolean(true) }) .first(); if (anyDefault) return anyDefault; - return await db('business_bank_accounts') + return await conn('business_bank_accounts') .where({ business_profile_id: 1 }) .orderBy('display_order', 'asc').orderBy('id', 'asc').first(); }); diff --git a/backend/src/services/contract/crud.js b/backend/src/services/contract/crud.js index f5d834f5..8f729c17 100644 --- a/backend/src/services/contract/crud.js +++ b/backend/src/services/contract/crud.js @@ -171,6 +171,12 @@ async function createContract(payload, adminId) { // simply skip these fields (contract still saves successfully). const hasEventCols = await hasColumnCached('contracts', 'event_name'); + // Resolve the audit actor BEFORE the transaction — adminActor reads + // admin_users via the global db, which inside the trx would grab a + // second connection from the single-connection SQLite pool and + // deadlock (60s acquire-timeout stall, audit row silently lost). + const actor = await adminActor(adminId); + return await db.transaction(async (trx) => { // Pass trx so the sequence claim joins our outer transaction — // SQLite deadlocks otherwise (1-connection default). @@ -239,7 +245,9 @@ async function createContract(payload, adminId) { } try { - await logActivity('contract_created', { contractId, contractNumber, customerAccountId: payload.customerAccountId }, null, await adminActor(adminId)); + // Pass `trx` so the audit insert rides the transaction's connection; + // the global db here deadlocks the single-connection SQLite pool. + await logActivity('contract_created', { contractId, contractNumber, customerAccountId: payload.customerAccountId }, null, actor, trx); } catch (_) { /* logging is best-effort */ } logger.info('Contract created', { adminId, contractId, contractNumber }); @@ -269,6 +277,9 @@ async function updateContract(id, payload, adminId) { const hasEventCols = await hasColumnCached('contracts', 'event_name'); + // Resolve the audit actor BEFORE the transaction — see createContract. + const actor = await adminActor(adminId); + return await db.transaction(async (trx) => { const updates = { updated_at: new Date() }; const map = { @@ -349,7 +360,9 @@ async function updateContract(id, payload, adminId) { } try { - await logActivity('contract_updated', { contractId: id }, null, await adminActor(adminId)); + // Pass `trx` so the audit insert rides the transaction's connection; + // the global db here deadlocks the single-connection SQLite pool. + await logActivity('contract_updated', { contractId: id }, null, actor, trx); } catch (_) { /* logging is best-effort */ } return id; }); diff --git a/backend/src/services/invoice/create.js b/backend/src/services/invoice/create.js index 4ee8ac9f..735a7817 100644 --- a/backend/src/services/invoice/create.js +++ b/backend/src/services/invoice/create.js @@ -54,7 +54,8 @@ async function createInvoice(payload, adminId, trx = db) { return { invoiceIds: draft?.id ? [draft.id] : [] }; } - const profile = (await businessProfileService.getProfile()).profile; + // Route reads through the caller's trx (no-op when trx === db) — see #851. + const profile = (await businessProfileService.getProfile(trx)).profile; const currency = (payload.currency || profile?.default_currency || 'CHF').toUpperCase(); const language = payload.language || customer.preferred_language || profile?.default_locale || 'de'; @@ -115,7 +116,7 @@ async function createInvoice(payload, adminId, trx = db) { // qty × unit arithmetic; the per-line rounding drift is surfaced as a // "Rundung" row at render time (storedNet − Σ line totals). Off by // default ⇒ net stays the sum of rounded lines, unchanged behaviour. - const roundTotal = (await getAppSetting('crm_invoice_round_total', false)) === true; + const roundTotal = (await getAppSetting('crm_invoice_round_total', false, trx)) === true; if (roundTotal) { netMinor = cleanNetMinor(items, { parentKey: 'parent_position', positionKey: 'position' }); } @@ -136,7 +137,7 @@ async function createInvoice(payload, adminId, trx = db) { ); } - const bank = await businessProfileService.resolveBankAccountForCurrency(currency, payload.businessBankAccountId); + const bank = await businessProfileService.resolveBankAccountForCurrency(currency, payload.businessBankAccountId, trx); // Snapshot the selected payment-term template (net days / Skonto / // installment plan) onto the invoice itself. Mirrors how the quote @@ -305,7 +306,7 @@ async function createInvoice(payload, adminId, trx = db) { await insertLineItemsHierarchical(trx, 'invoice_line_items', 'invoice_id', invoiceId, items); } - try { await logActivity('invoice_created', { invoiceId, invoiceNumber }, payload.eventId || null, `admin:${adminId}`); } catch (_) {} + try { await logActivity('invoice_created', { invoiceId, invoiceNumber }, payload.eventId || null, `admin:${adminId}`, trx); } catch (_) {} return { invoiceIds: [invoiceId] }; } diff --git a/backend/src/services/invoice/helpers.js b/backend/src/services/invoice/helpers.js index 0b0c6ea9..d0bec973 100644 --- a/backend/src/services/invoice/helpers.js +++ b/backend/src/services/invoice/helpers.js @@ -101,7 +101,8 @@ async function resolveNetDays(payload, trx = db) { .first(); if (probe && probe.net_days != null) return ensureInt(probe.net_days) || 30; } - const setting = ensureInt(await getAppSetting('crm_payment_default_net_days')); + // Route through the caller's trx — global-db here deadlocks SQLite (#851). + const setting = ensureInt(await getAppSetting('crm_payment_default_net_days', null, trx)); if (setting) return setting; return 30; } diff --git a/backend/src/services/invoice/sending.js b/backend/src/services/invoice/sending.js index d1055fa4..6658736d 100644 --- a/backend/src/services/invoice/sending.js +++ b/backend/src/services/invoice/sending.js @@ -307,9 +307,11 @@ async function createStorno(originalId, adminId, trx = db) { }); try { + // Pass `trx` so the audit insert rides the transaction's connection; + // the global db here deadlocks the single-connection SQLite pool. await logActivity('invoice_cancelled_via_storno', { invoiceId: originalId, stornoId, stornoNumber }, - original.event_id || null, `admin:${adminId}`); + original.event_id || null, `admin:${adminId}`, trx); } catch (_) {} return stornoId; @@ -439,6 +441,13 @@ async function reissueInvoice(id, adminId) { // createInvoice so totals are recomputed authoritatively from // line items (any rounding drift gets normalised). Self-join // carries parent_position so migration-119 sub-items survive. + // + // The wrapping transaction is REQUIRED (codex review of #851 round 2): + // without it, createInvoice's early insert + sequence claim survive a + // later validation failure, leaving orphan drafts after the Storno + // already committed. createInvoice's internal reads (getProfile, + // getAppSetting, bank resolution, audit) all accept the trx now, so + // the round-1 SQLite deadlock is gone the right way. return await db.transaction(async (trx) => { const lineItems = await trx('invoice_line_items as li') .leftJoin('invoice_line_items as parent', 'parent.id', 'li.parent_line_item_id') @@ -502,9 +511,10 @@ async function reissueInvoice(id, adminId) { }); try { + // Pass `trx` so the audit insert rides the transaction's connection. await logActivity('invoice_reissued', { originalInvoiceId: id, newInvoiceId: newId, stornoId }, - original.event_id || null, `admin:${adminId}`); + original.event_id || null, `admin:${adminId}`, trx); } catch (_) {} return { id: newId, replaces: id, stornoId };