diff --git a/backend/__tests__/services/usageEnableDisableRace.test.js b/backend/__tests__/services/usageEnableDisableRace.test.js index 11ca2b2e..d89769d2 100644 --- a/backend/__tests__/services/usageEnableDisableRace.test.js +++ b/backend/__tests__/services/usageEnableDisableRace.test.js @@ -188,4 +188,35 @@ describe('withdrawal during an in-flight activation', () => { expect(posted).not.toContain('report'); }); + + it('honours a withdrawal that lands between the lease claim and the state read', async () => { + // locked() claims the lease and reads the row in two statements. A + // /disable completing in that gap used to be adopted as this + // activation's own baseline and absorbed, so registration went ahead + // after the operator had withdrawn. + db = await bootDb(); + const service = makeService(db); + const realState = service.state.bind(service); + let fired = false; + service.state = async () => { + // The withdrawal must land BEFORE this read returns, so the row carries + // the incremented counter. Incrementing afterwards would hand back the + // old value and both the broken and fixed versions would behave the + // same — which is exactly how an earlier version of this test passed + // against the bug it was meant to catch. + const first = await realState(); + if (!fired && first.lease_token) { + fired = true; + await db('product_usage_state').where({ id: 1 }).increment('cancel_seq', 1); + return realState(); + } + return first; + }; + + 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(); + }); }); diff --git a/backend/src/usage/UsageService.js b/backend/src/usage/UsageService.js index 76dd0e57..95491f18 100644 --- a/backend/src/usage/UsageService.js +++ b/backend/src/usage/UsageService.js @@ -214,20 +214,22 @@ class UsageService { async enable(consent) { if (consent !== 'usage-consent.v1') throw new ValidationError('Explicit usage consent is required'); + // Read BEFORE the lease, deliberately. locked() claims the lease and then + // reads the row in a second statement; a /disable completing between + // those two would be adopted as this activation's own baseline and + // silently absorbed. Taking the baseline first inverts that: every + // increment from this point on — including one in that gap — is later + // than the value the claim tests for, so the claim fails and the + // withdrawal wins. An increment from BEFORE this read is a withdrawal the + // operator already completed, and a deliberate opt-in afterwards should + // not be vetoed by it. + const cancelSeq = Number((await this.state())?.cancel_seq || 0); await this.locked(async (state) => { if (state.status !== 'disabled') throw new ConflictError( 'Finish the current participation before rejoining' ); this.collectorUrl(); - // 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, { consent_version: consent