From cc263f2e87170058ce5e38aaae4c0aa712297a37 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 5 Sep 2026 23:26:41 +0200 Subject: [PATCH] fix(usage): isolate the Postgres fixture, and stop two more wrong signals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings, one of them mine and CI-affecting. The Postgres suite gets its own schema. CI hands every gated suite the same PICPEAK_PG_TEST_URL and runs jest with parallel workers, and both picpeakRestorePg and externalRelpathFoldPg drop and recreate `events` and `app_settings` in it — so the suite I added would have destroyed their fixtures and vice versa, intermittently. It now creates and drops its own `usage_pg_test` schema and reaches the tables through searchPath, which works because the service queries unqualified names. Verified on a clean database: after the run `public` still holds zero tables. My first attempt at this silently did not apply — the replacement anchor had been reformatted by eslint and I printed success without asserting the match, which is why the first "isolated" claim was wrong. Webhook-only installs are no longer counted as SMTP users. With EMAIL_WEBHOOK_URL and EMAIL_WEBHOOK_SECRET set, adminEmail sends /email/test through the webhook transport and never touches SMTP (#1225 added that path), but the rule recorded the permanent `smtp` marker anyway. Gated on the transport that is actually configured. Activation is written atomically with its acknowledgement. Split across two updates, a failure or a stop between them left the row activation_pending with pending_packet already cleared — registered with the collector, and permanently stuck locally, because tick() has nothing to retry from there. The register case now sets status in the same write and is guarded precisely on activation_pending rather than merely "not withdrawing". Refs #1110 --- .../integration/productUsagePg.test.js | 30 ++++++++++++++----- backend/src/middleware/productUsage.js | 15 +++++++++- backend/src/usage/UsageService.js | 25 +++++++++++----- 3 files changed, 54 insertions(+), 16 deletions(-) diff --git a/backend/__tests__/integration/productUsagePg.test.js b/backend/__tests__/integration/productUsagePg.test.js index 34fa206f..ea4e3835 100644 --- a/backend/__tests__/integration/productUsagePg.test.js +++ b/backend/__tests__/integration/productUsagePg.test.js @@ -27,12 +27,25 @@ maybe('product usage on Postgres', () => { let UsageService; beforeAll(async () => { - db = knex({ client: 'pg', connection: PG_URL, pool: { min: 0, max: 10 } }); - for (const t of ['product_usage_markers', 'product_usage_state', 'app_settings', - 'feature_flags', 'events', 'css_templates', 'email_configs', - 'mail_accounts', 'whatsapp_configs']) { - await db.raw(`DROP TABLE IF EXISTS ${t} CASCADE`); - } + // Its own schema, not `public`. CI hands every gated suite the same + // PICPEAK_PG_TEST_URL and runs jest with parallel workers, and both + // picpeakRestorePg and externalRelpathFoldPg drop and recreate `events` + // and `app_settings` there. Sharing that would have made all three + // intermittently destroy each other's fixtures. The service queries + // unqualified table names, so a searchPath keeps it entirely in here. + const bootstrap = knex({ + client: 'pg', connection: PG_URL, pool: { min: 0, max: 2 } + }); + await bootstrap.raw('DROP SCHEMA IF EXISTS usage_pg_test CASCADE'); + await bootstrap.raw('CREATE SCHEMA usage_pg_test'); + await bootstrap.destroy(); + + db = knex({ + client: 'pg', + connection: PG_URL, + searchPath: ['usage_pg_test'], + pool: { min: 0, max: 10 } + }); // The real migrations, on the real engine. await require('../../migrations/core/201_product_usage').up(db); await require('../../migrations/core/202_product_usage_cancel_requested').up(db); @@ -61,7 +74,10 @@ maybe('product usage on Postgres', () => { }, 120000); afterAll(async () => { - if (db) await db.destroy(); + if (db) { + await db.raw('DROP SCHEMA IF EXISTS usage_pg_test CASCADE'); + await db.destroy(); + } fs.rmSync(bindingDir, { recursive: true, force: true }); }); diff --git a/backend/src/middleware/productUsage.js b/backend/src/middleware/productUsage.js index a8d41c14..853a74e5 100644 --- a/backend/src/middleware/productUsage.js +++ b/backend/src/middleware/productUsage.js @@ -3,6 +3,14 @@ // identifiers, paths, timing, or counts are retained or sent. const service = require('../services/productUsageService'); const logger = require('../utils/logger'); +// Mirrors emailWebhookTransport: the webhook is in play only when both are +// set, which is when adminEmail routes the test send through it. +const webhookTransportConfigured = () => + Boolean( + (process.env.EMAIL_WEBHOOK_URL || '').trim() && + (process.env.EMAIL_WEBHOOK_SECRET || '').trim() + ); + const RULES = [ [/^\/customers(?:\/|$)/, ['crm']], [/^\/quotes(?:\/|$)/, ['crm', 'crm_quotes']], @@ -38,9 +46,14 @@ function productUsage(req, res, next) { const pathname = req.path; res.once('finish', () => { if (!req.admin?.id || res.statusCode < 200 || res.statusCode >= 300) return; - const features = RULES.filter(([pattern]) => + let features = RULES.filter(([pattern]) => pattern.test(pathname) ).flatMap(([, keys]) => keys); + // A webhook-only install sends /email/test through the webhook transport + // and never touches SMTP (adminEmail.js has an explicit path for it, + // #1225), so recording smtp here would permanently misclassify it. + if (features.includes('smtp') && webhookTransportConfigured()) + features = features.filter((f) => f !== 'smtp'); if ( process.env.STORAGE_BACKEND === 's3' && /^\/(?:photos|events)\/[^/]+\/upload(?:\/|$)/.test(pathname) diff --git a/backend/src/usage/UsageService.js b/backend/src/usage/UsageService.js index c87584e7..2e8bd11f 100644 --- a/backend/src/usage/UsageService.js +++ b/backend/src/usage/UsageService.js @@ -474,17 +474,26 @@ class UsageService { update.last_packet = JSON.stringify(envelope); update.last_report_date = packet.payload.report_date; } - await this.db('product_usage_state') - .where({ id: 1 }) - .whereNot({ status: 'deletion_pending' }) - .update(update); + // Activation goes in with the acknowledgement, not after it. Split + // across two writes, a failure or a stop between them left the row + // `activation_pending` with pending_packet already cleared — and + // tick() has nothing to retry from there, so the installation was + // registered with the collector but permanently stuck locally. + // Still guarded on activation_pending, so a withdrawal that arrived + // first is not overwritten. + const ack = this.db('product_usage_state').where({ id: 1 }); + if (packet.action === 'register') { + update.status = 'active'; + // Precisely activation_pending, not merely "not withdrawing" — + // this write is the one that turns participation on. + ack.where({ status: 'activation_pending' }); + } else { + ack.whereNot({ status: 'deletion_pending' }); + } + await ack.update(update); await this.db('product_usage_state') .where({ id: 1, status: 'deletion_pending' }) .update({ sequence: packet.sequence, pending_packet: null }); - if (packet.action === 'register') - await this.db('product_usage_state') - .where({ id: 1, status: 'activation_pending' }) - .update({ status: 'active' }); } return receipt; } catch (error) {