fix(usage): close the remaining withdrawal races, reset per-item name consent

Follow-up review on the previous commit, including a hole in that
commit's own fix.

The cancellation flag became a counter. Clearing a boolean needed a
write of its own, and a /disable landing between the lease and that
write was erased — the same race one level down. enable() now records
the counter it started with and claims only if it is unchanged, so no
clearing write exists to lose. It also fixes the case a boolean could
not express at all: a stale cancellation already set, and a fresh one
arriving mid-activation, are indistinguishable as flags and obvious as
counts. Migration 203, separate from 202 for the reason 202 was separate
from 201 — knex will not re-run an applied migration.

deliver() re-checks immediately before dispatch. The existing check ran
before the binding lookup, which is asynchronous, so a withdrawal that
COMPLETED during it still had its registration or report sent
afterwards. Not an already-in-flight request — a new one started after
the operator had withdrawn.

The outbox writes in tick() and command() are conditional on still being
active. /disable clears pending_packet without holding the lease, so an
unconditional write put a report — or a feedback body and name — back
into an outbox the withdrawal had just emptied, where deliver() would
then leave it, since it declines to send anything but the delete.

Per-item name consent resets with the item. `named` stayed checked after
submitting, so the next item carried the previous name automatically,
contradicting the anonymous-by-default promise the disclosure makes for
each item. The remembered name stays in preferences; attaching it is
decided again each time.

Two of these tests were worthless when first written and are noted
because the pattern keeps recurring: the pre-dispatch case passed
without the guard because an empty report payload failed schema
validation during signing, so nothing reached the collector for reasons
unrelated to the check. With a valid payload it fails without the guard
and passes with it. Same for the counter: dropping it from the claim
fails two.

Refs #1110
This commit is contained in:
Paul Nothaft
2026-09-05 22:09:39 +02:00
parent 80e238f0ad
commit 22da018e1b
4 changed files with 152 additions and 19 deletions
@@ -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');
});
});
@@ -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');
});
};
+37 -17
View File
@@ -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();
@@ -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: '',