fix(usage): stop local backups implying S3 use, and make the protocol-error branch reachable

Two findings from the review of the current head.

Local backups no longer imply S3. markUsed derived an s3_storage marker
from "a backup ran while backup_destination_type is s3" — but the
middleware also counts /database-backup/* and /backup/picpeak/export as
backups, and those write a local file wherever scheduled backups go. So
configuring S3 and downloading a local export reported s3_storage as
USED. The middleware now tells markUsed whether the operation writes to
the configured destination, and only then is the marker derived. A wrong
`true` in this dataset is worse than a missing signal: it is a claim
about an install that nobody can check.

The ProtocolError branch was dead code. adminUsage matched on
`error.name === 'ProtocolError'`, but the class extends Error without
setting `name`, so every instance reports 'Error' — verified — and a
malformed vote or feedback payload fell through to the global handler,
which logs it as an unhandled programming error and answers
INTERNAL_ERROR in production, losing the validation code the caller
needs. Now matched with instanceof. protocol.cjs is byte-identical with
picpeak-usage (diffed against the companion repo), so the fix belongs
here rather than in the class.

An existing assertion needed updating for the new markUsed argument, and
the path split is pinned: /backup/run is destination-driven,
/database-backup/backup and /backup/picpeak/export are not.

Refs #1110
This commit is contained in:
Paul Nothaft
2026-09-05 23:16:48 +02:00
parent c7cedb00d6
commit 32d745b575
5 changed files with 94 additions and 6 deletions
@@ -150,3 +150,45 @@ describe('status survives a misconfigured collector URL', () => {
expect(status.collector_url).toBe('https://usage.picpeak.app');
});
});
describe('S3 use is only implied by backups that write to the destination', () => {
let db;
afterEach(async () => { if (db) await db.destroy(); db = null; });
const withS3Destination = async (database) => {
await database('app_settings').insert({
setting_key: 'backup_destination_type',
setting_value: JSON.stringify('s3'),
});
await database('product_usage_state').where({ id: 1 }).update({ status: 'active' });
};
it('marks S3 for a backup that uses the configured destination', async () => {
db = await bootDb();
await withS3Destination(db);
await service(db).markUsed(['backup'], { destinationBackup: true });
expect((await db('product_usage_markers').pluck('feature')).sort())
.toEqual(['backup', 's3_storage']);
});
it('does NOT mark S3 for a local backup, even with S3 configured', async () => {
// /database-backup/* and /backup/picpeak/export produce a local file. They
// count as `backup`, but claiming S3 was used for them made merely
// configuring S3 and downloading an export report s3_storage.used.
db = await bootDb();
await withS3Destination(db);
await service(db).markUsed(['backup']);
expect(await db('product_usage_markers').pluck('feature')).toEqual(['backup']);
});
it('does not mark S3 when the destination is not S3', async () => {
db = await bootDb();
await db('app_settings').insert({
setting_key: 'backup_destination_type',
setting_value: JSON.stringify('local'),
});
await db('product_usage_state').where({ id: 1 }).update({ status: 'active' });
await service(db).markUsed(['backup'], { destinationBackup: true });
expect(await db('product_usage_markers').pluck('feature')).toEqual(['backup']);
});
});