diff --git a/backend/__tests__/services/usageEnableDisableRace.test.js b/backend/__tests__/services/usageEnableDisableRace.test.js index f387859c..11ca2b2e 100644 --- a/backend/__tests__/services/usageEnableDisableRace.test.js +++ b/backend/__tests__/services/usageEnableDisableRace.test.js @@ -15,6 +15,22 @@ const { UsageService } = require('../../src/usage/UsageService'); const SECRET = 'z'.repeat(48); +// A report the envelope schema accepts. An empty payload fails validation +// during signing, so the packet would never reach the collector for reasons +// unrelated to what the test is checking. +function validReport() { + const { FEATURE_KEYS } = require('../../src/usage/protocol.cjs'); + return { + picpeak_version: '3.0.0', + report_date: '2026-09-05', + generated_at: '2026-09-05T00:00:00.000Z', + features: Object.fromEntries( + FEATURE_KEYS.map((k) => [k, { configured: false, used: false }]) + ), + gallery_layouts: ['grid'], + }; +} + async function bootDb() { const db = knex({ client: 'sqlite3', @@ -38,7 +54,7 @@ async function bootDb() { t.text('feedback_preferences'); t.string('lease_token', 36); t.bigInteger('lease_until').notNullable().defaultTo(0); - t.boolean('cancel_requested').notNullable().defaultTo(false); + t.bigInteger('cancel_seq').notNullable().defaultTo(0); }); await db.schema.createTable('product_usage_markers', (t) => { t.string('feature', 60).primary(); @@ -99,7 +115,9 @@ describe('withdrawal during an in-flight activation', () => { it('does not let a stale cancellation veto a later deliberate opt-in', async () => { db = await bootDb(); - await db('product_usage_state').where({ id: 1 }).update({ cancel_requested: true }); + // A withdrawal from an earlier participation is already reflected in the + // counter when this activation reads it, so it cannot veto anything. + await db('product_usage_state').where({ id: 1 }).update({ cancel_seq: 7 }); const service = makeService(db); await service.enable('usage-consent.v1'); @@ -108,4 +126,66 @@ describe('withdrawal during an in-flight activation', () => { expect(row.status).toBe('activation_pending'); expect(row.installation_id).not.toBeNull(); }); + + it('honours a withdrawal even when an earlier one was never cleared', async () => { + // The case a boolean could not express: a stale cancellation is already + // set, and a fresh one lands mid-activation. With a flag both look the + // same; with a counter the second increment is visible. + db = await bootDb(); + await db('product_usage_state').where({ id: 1 }).update({ cancel_seq: 3 }); + + const service = makeService(db, { + onBinding: async () => { await service.disable(); }, + }); + await service.enable('usage-consent.v1'); + + const row = await db('product_usage_state').where({ id: 1 }).first(); + expect(row.status).toBe('disabled'); + expect(row.installation_id).toBeNull(); + }); + + it('does not dispatch a report when the withdrawal completes during preparation', async () => { + // deliver() checks for a withdrawal before the binding lookup, which is + // asynchronous. A /disable that COMPLETED during it used to have the + // report sent anyway — not an already-in-flight request, but a new one + // started after the operator had withdrawn. + db = await bootDb(); + const posted = []; + const service = new UsageService(db, { + secret: SECRET, + endpoint: 'http://127.0.0.1:9/', + fetch: async (_url, init) => { + posted.push(JSON.parse(init.body).packet.action); + throw new Error('collector unreachable'); + }, + }); + const identity = require('../../src/usage/protocol.cjs').generateIdentity(); + await db('product_usage_state').where({ id: 1 }).update({ + status: 'active', + installation_id: identity.installation_id, + public_key: identity.public_key, + private_key_encrypted: service.encrypt(identity.private_key), + instance_binding: 'b'.repeat(64), + sequence: 1, + pending_packet: JSON.stringify( + require('../../src/usage/protocol.cjs').makePacket( + { installation_id: identity.installation_id }, + 'report', + 2, + validReport() + ) + ), + }); + // The withdrawal lands while the binding lookup is awaited. + service.binding = async () => { + await db('product_usage_state').where({ id: 1 }).update({ + status: 'deletion_pending', pending_packet: null, + }); + return 'b'.repeat(64); + }; + + await service.deliver(await db('product_usage_state').where({ id: 1 }).first()); + + expect(posted).not.toContain('report'); + }); }); diff --git a/backend/migrations/core/203_product_usage_cancel_seq.js b/backend/migrations/core/203_product_usage_cancel_seq.js new file mode 100644 index 00000000..7fb8af2e --- /dev/null +++ b/backend/migrations/core/203_product_usage_cancel_seq.js @@ -0,0 +1,26 @@ +// Supersedes the boolean added in 202. A boolean cannot distinguish "a +// withdrawal arrived while this activation was starting" from "a withdrawal +// from an earlier participation was never cleared": clearing it needed its +// own write, and a /disable landing between the lease and that write was +// erased. A monotonic counter needs no clearing — enable() records the value +// it started with and claims only if it is unchanged, so any intervening +// withdrawal is visible whatever the previous state was. +exports.up = async function (knex) { + if (!(await knex.schema.hasTable('product_usage_state'))) return; + if (!(await knex.schema.hasColumn('product_usage_state', 'cancel_seq'))) + await knex.schema.alterTable('product_usage_state', (t) => { + t.bigInteger('cancel_seq').notNullable().defaultTo(0); + }); + if (await knex.schema.hasColumn('product_usage_state', 'cancel_requested')) + await knex.schema.alterTable('product_usage_state', (t) => { + t.dropColumn('cancel_requested'); + }); +}; + +exports.down = async function (knex) { + if (!(await knex.schema.hasTable('product_usage_state'))) return; + if (await knex.schema.hasColumn('product_usage_state', 'cancel_seq')) + await knex.schema.alterTable('product_usage_state', (t) => { + t.dropColumn('cancel_seq'); + }); +}; diff --git a/backend/src/usage/UsageService.js b/backend/src/usage/UsageService.js index 705f73ca..76dd0e57 100644 --- a/backend/src/usage/UsageService.js +++ b/backend/src/usage/UsageService.js @@ -220,13 +220,13 @@ class UsageService { 'Finish the current participation before rejoining' ); this.collectorUrl(); - // A cancellation left over from an earlier participation must not veto - // this deliberate opt-in, so the flag is cleared before the slow work - // starts. Anything set from here on is a withdrawal aimed at THIS - // activation. - await this.db('product_usage_state') - .where({ id: 1 }) - .update({ cancel_requested: formatBoolean(false) }); + // The withdrawal counter as it stood when this activation began. A + // /disable from an earlier participation is already reflected here and + // must not veto a deliberate opt-in; anything that increments it from + // now on is aimed at THIS activation. Recorded rather than cleared, + // because a clearing write of its own had the very race it was meant to + // close — a /disable landing between the lease and the clear was erased. + const cancelSeq = Number(state.cancel_seq || 0); const identity = generateIdentity(); const pending = makePacket(identity, 'register', 0, { @@ -243,8 +243,7 @@ class UsageService { // conditional UPDATE, so a /disable that lands first makes it match no // rows and the registration is never sent. const claimed = await this.db('product_usage_state') - .where({ id: 1, status: 'disabled' }) - .whereNot({ cancel_requested: formatBoolean(true) }) + .where({ id: 1, status: 'disabled', cancel_seq: cancelSeq }) .update({ status: 'activation_pending', notice_dismissed: formatBoolean(true), @@ -266,16 +265,16 @@ class UsageService { } async disable() { - // Recorded unconditionally and first, because the interesting case is the + // Counted unconditionally and first, because the interesting case is the // one where there is seemingly nothing to stop: while /enable is still // generating an identity the row reads `disabled`, so the conditional // update below matches nothing and the lease conflict from tick() is // swallowed — the admin was told participation was off while the - // activation went on to complete. enable() claims its state conditionally - // on this flag, so a withdrawal that lands during that window wins. + // activation went on to complete. enable() claims its state only if this + // counter is unchanged, so a withdrawal landing in that window wins. await this.db('product_usage_state') .where({ id: 1 }) - .update({ cancel_requested: formatBoolean(true) }); + .increment('cancel_seq', 1); // Stop collection before waiting for an in-flight send. The sender checks // state again before delivery and preserves this stop after its response. @@ -363,6 +362,16 @@ class UsageService { }, new Date(this.now()) ); + // Last check before anything leaves. The guard at the top of this + // method runs before the binding lookup above, which is asynchronous — + // so a withdrawal that COMPLETED during it would previously still have + // had its registration or report dispatched afterwards. This is not + // about an already-in-flight request; it is about not starting one. + if ( + packet.action !== 'delete' && + (await this.state()).status === 'deletion_pending' + ) + return null; const receipt = await this.post('/api/envelopes', envelope); if ( receipt.packet_id !== packet.packet_id || @@ -490,9 +499,15 @@ class UsageService { payload ); state.pending_packet = JSON.stringify(packet); - await this.db('product_usage_state') - .where({ id: 1 }) + // Only while still active. /disable clears pending_packet and moves the + // status without taking the lease, so an unconditional write here could + // put a report back into the outbox after the withdrawal had emptied + // it — and deliver() would then leave it there, since it declines to + // send anything but the delete. + const enqueued = await this.db('product_usage_state') + .where({ id: 1, status: 'active' }) .update({ pending_packet: state.pending_packet }); + if (!enqueued) return; await this.deliver(state); }); return this.status(); @@ -664,9 +679,14 @@ class UsageService { this.now() ); state.pending_packet = JSON.stringify(packet); - await this.db('product_usage_state') - .where({ id: 1 }) + // Same guard as the report enqueue in tick(): a command captured while + // active must not restore its payload — feedback body and name + // included — into the outbox that /disable has just cleared. + const enqueued = await this.db('product_usage_state') + .where({ id: 1, status: 'active' }) .update({ pending_packet: state.pending_packet }); + if (!enqueued) + throw new ConflictError('Usage participation is not active'); receipt = await this.deliver(state); }); const state = await this.status(); diff --git a/frontend/src/features/settings/tabs/ProductUsageTab.tsx b/frontend/src/features/settings/tabs/ProductUsageTab.tsx index e69a2e8f..c512d86f 100644 --- a/frontend/src/features/settings/tabs/ProductUsageTab.tsx +++ b/frontend/src/features/settings/tabs/ProductUsageTab.tsx @@ -304,6 +304,13 @@ export default function ProductUsageTab() { : 'productUsage.failed' ) ); + // Every consent choice resets with the item it was made for. + // Leaving `named` checked meant the next submission carried + // the previous name automatically, which contradicts the + // per-item, anonymous-by-default promise the disclosure makes + // — the remembered name stays in preferences, but attaching + // it is a decision taken again each time. + setNamed(false); setForm({ ...form, title: '',