diff --git a/api/src/main/repositories/mongoose/ContractRepository.ts b/api/src/main/repositories/mongoose/ContractRepository.ts index ac5ad13..02e89e5 100644 --- a/api/src/main/repositories/mongoose/ContractRepository.ts +++ b/api/src/main/repositories/mongoose/ContractRepository.ts @@ -185,6 +185,46 @@ class ContractRepository extends RepositoryBase { return contract ? toPlainObject(contract.toJSON()) : null; } + /** + * Add to several usage levels of one contract in a single atomic update. + * + * `$inc` is evaluated by the database against the stored document rather than + * against a copy the process read earlier, so concurrent increments compose + * instead of overwriting one another. Read-modify-write cannot do this from + * the application: two callers who read the same value both write the same + * total, and one consumption disappears. + * + * Every path is required to exist by the filter, so a limit that is not part + * of the contract matches no document and is reported to the caller instead + * of being created by the update - `$inc` would otherwise happily add the + * field. That check and the increment are one operation, so a limit cannot be + * validated and then vanish before the write. + * + * @param increments usage level path (`service.limit`) to amount to add. + * @returns the contract as it is after the increment, or null when the filter + * matched nothing. + */ + async incrementUsageLevels( + userId: string, + increments: Record + ): Promise { + const filter: Record = { 'userContact.userId': userId }; + const inc: Record = {}; + + for (const [path, amount] of Object.entries(increments)) { + filter[`usageLevels.${path}.consumed`] = { $exists: true }; + inc[`usageLevels.${path}.consumed`] = amount; + } + + const contract = await ContractMongoose.findOneAndUpdate( + filter, + { $inc: inc }, + { new: true } + ); + + return contract ? toPlainObject(contract.toJSON()) : null; + } + async changeServiceName(oldServiceName: string, newServiceName: string, organizationId: string): Promise { const oldServiceKey = oldServiceName.toLowerCase(); const newServiceKey = newServiceName.toLowerCase(); diff --git a/api/src/main/services/ContractService.ts b/api/src/main/services/ContractService.ts index bb0b6cb..0df8ee4 100644 --- a/api/src/main/services/ContractService.ts +++ b/api/src/main/services/ContractService.ts @@ -459,38 +459,95 @@ class ContractService { usageLimitId: string, expectedConsumption: number ): Promise { - let contract = await this.cacheService.get(`contracts.${userId}`); + await this._applyExpectedConsumptions(userId, { [usageLimitId]: expectedConsumption }); + } - if (!contract) { - contract = await this.contractRepository.findByUserId(userId); + /** + * Apply several expected consumptions to a contract in one atomic update. + * + * Read-modify-write loses consumptions, and it does so in two ways. Applying + * the limits of one evaluation in turn made each application read the whole + * contract, change a single usage level in its own copy and write the whole + * contract back, so an evaluation touching two limits recorded one of them. + * Batching the limits into a single read and write fixes that, but not the + * case of two requests arriving together: both read the same consumed value, + * both write the same total, and one consumption is gone. + * + * The database is the only place that can settle this, so the increment is + * handed to it as `$inc` and evaluated against the stored document. Whatever + * order concurrent calls arrive in, every one of them is added. + */ + async _applyExpectedConsumptions( + userId: string, + expectedConsumptions: Record + ): Promise { + const usageLimitIds = Object.keys(expectedConsumptions); + if (usageLimitIds.length === 0) { + return; } - if (!contract) { - throw new Error(`Contract with userId ${userId} not found`); + const targets = usageLimitIds.map(usageLimitId => { + const serviceName: string = usageLimitId.split('-')[0]; + const usageLimit: string = usageLimitId.split('-')[1]; + + return { serviceName, usageLimit, amount: expectedConsumptions[usageLimitId] }; + }); + + const increments: Record = {}; + for (const { serviceName, usageLimit, amount } of targets) { + increments[`${serviceName}.${usageLimit}`] = amount; + } + + const updatedContract = await this.contractRepository.incrementUsageLevels(userId, increments); + + if (!updatedContract) { + // The update requires the contract and every named usage level to exist, + // so it matched nothing. Which of the two is missing only matters for the + // message, and is worth a read to get right. + await this._explainMissingUsageLevels(userId, targets); } - const serviceName: string = usageLimitId.split('-')[0]; - const usageLimit: string = usageLimitId.split('-')[1]; + const appliedAt = new Date().getTime(); - if (contract.usageLevels[serviceName][usageLimit]) { + for (const { serviceName, usageLimit, amount } of targets) { + // What the level held before this call, for `_revertExpectedConsumption`. + // Derived from the result rather than from a prior read, so it is this + // caller's own contribution that gets taken back even if others landed in + // between. await this.cacheService.set( - `${new Date().getTime()}.usageLevels.${userId}.${serviceName}.${usageLimit}`, - contract.usageLevels[serviceName][usageLimit].consumed, + `${appliedAt}.usageLevels.${userId}.${serviceName}.${usageLimit}`, + updatedContract!.usageLevels[serviceName][usageLimit].consumed - amount, 120 ); // 120 secs = 2 mins + } - contract.usageLevels[serviceName][usageLimit].consumed += expectedConsumption; + await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour + } - const updatedContract = await this.contractRepository.update(userId, contract); + /** + * Say which part of the contract was missing, having established that one was. + */ + private async _explainMissingUsageLevels( + userId: string, + targets: { serviceName: string; usageLimit: string }[] + ): Promise { + const contract = await this.contractRepository.findByUserId(userId); - if (!updatedContract) { - throw new Error(`Failed to update contract for userId ${userId}`); - } + if (!contract) { + throw new Error(`Contract with userId ${userId} not found`); + } - await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour - } else { - throw new Error(`Usage level ${usageLimit} not found in contract for userId ${userId}`); + const missing = targets.find( + ({ serviceName, usageLimit }) => !contract.usageLevels[serviceName]?.[usageLimit] + ); + + if (missing) { + throw new Error( + `Usage level ${missing.usageLimit} not found in contract for userId ${userId}` + ); } + + throw new Error(`Failed to update contract for userId ${userId}`); } async _revertExpectedConsumption( diff --git a/api/src/main/utils/feature-evaluation/featureEvaluation.ts b/api/src/main/utils/feature-evaluation/featureEvaluation.ts index fd0fd9e..4dd6b36 100644 --- a/api/src/main/utils/feature-evaluation/featureEvaluation.ts +++ b/api/src/main/utils/feature-evaluation/featureEvaluation.ts @@ -61,20 +61,21 @@ async function evaluateFeature( } } - // Then apply all consumptions after validation has passed + // Then apply all consumptions after validation has passed. + // + // In one call rather than one per limit: each application reads the whole + // contract, increments one usage level and writes the whole contract + // back, so running them concurrently made every one of them start from + // the same state and only the last write survive. if (options.userId) { const contractService: ContractService = container.resolve('contractService'); - const limits = Object.keys(featureEvaluation.used); - await Promise.all( - limits.map(limit => - contractService._applyExpectedConsumption( - options.userId!, - limit, - expectedConsumption[limit] - ) - ) - ); + const consumptions: Record = {}; + for (const limit of Object.keys(featureEvaluation.used)) { + consumptions[limit] = expectedConsumption[limit]; + } + + await contractService._applyExpectedConsumptions(options.userId, consumptions); } } } diff --git a/api/src/test/contract.expected-consumption.test.ts b/api/src/test/contract.expected-consumption.test.ts new file mode 100644 index 0000000..d903f54 --- /dev/null +++ b/api/src/test/contract.expected-consumption.test.ts @@ -0,0 +1,262 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import container from '../main/config/container'; + +/** + * Applying expected consumption to usage limits. + * + * Read-modify-write loses consumptions in two ways. Applying the limits of one + * evaluation in turn made each application read the whole contract, change one + * usage level in its own copy and write the whole contract back, so an + * evaluation touching two limits recorded one of them. Batching them into a + * single read and write fixed that, and left the other: two requests arriving + * together both read the same consumed value, both write the same total, and + * one consumption disappears. + * + * The increment is now handed to the database as `$inc`, which is the only + * place it can be settled. + * + * These tests work against a stubbed repository and cache so they can assert on + * the reads and writes themselves, which is where the defect lived. The stub + * models the database honestly: `incrementUsageLevels` adds to whatever is + * *stored* at the moment it runs, which is exactly the guarantee `$inc` gives + * and exactly the one an application-side read-modify-write cannot. + */ + +function aContract() { + return { + userContact: { userId: 'user1', username: 'user1' }, + contractedServices: { petclinic: '2025' }, + subscriptionPlans: { petclinic: 'BASIC' }, + usageLevels: { + petclinic: { + maxPets: { consumed: 0 }, + maxVisits: { consumed: 0 }, + }, + }, + }; +} + +const copy = (value: any) => JSON.parse(JSON.stringify(value)); + +function withStubs(contract: any) { + const state = { current: contract }; + let writes = 0; + let reads = 0; + let increments = 0; + + const contractRepository = { + findByUserId: vi.fn(async () => { + reads += 1; + return copy(state.current); + }), + + // Kept so a regression to read-modify-write is visible rather than a crash. + // The await between reading and writing is what any real round trip has, + // and what lets a second caller slip in between the two. + update: vi.fn(async (_userId: string, updated: any) => { + writes += 1; + await Promise.resolve(); + state.current = copy(updated); + return copy(state.current); + }), + + incrementUsageLevels: vi.fn(async (_userId: string, byPath: Record) => { + increments += 1; + await Promise.resolve(); + + // The filter requires every path to exist; a miss matches no document. + for (const path of Object.keys(byPath)) { + const [serviceName, usageLimit] = path.split('.'); + if (!state.current.usageLevels[serviceName]?.[usageLimit]) { + return null; + } + } + + for (const [path, amount] of Object.entries(byPath)) { + const [serviceName, usageLimit] = path.split('.'); + state.current.usageLevels[serviceName][usageLimit].consumed += amount; + } + + return copy(state.current); + }), + }; + + const cacheService = { + get: vi.fn(async () => null), + set: vi.fn(async () => undefined), + del: vi.fn(async () => undefined), + }; + + const original = container.resolve.bind(container); + vi.spyOn(container, 'resolve').mockImplementation((name: any) => { + if (name === 'contractRepository') return contractRepository as any; + if (name === 'cacheService') return cacheService as any; + return original(name); + }); + + return { + state, + contractRepository, + cacheService, + counts: () => ({ reads, writes, increments }), + consumed: () => state.current.usageLevels.petclinic, + }; +} + +async function aService() { + const { default: ContractService } = await import('../main/services/ContractService'); + return new (ContractService as any)(); +} + +describe('Applying expected consumption', () => { + beforeEach(() => { + vi.restoreAllMocks(); + }); + + it('records every limit, not just the last one written', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 3, + }); + + expect(stubs.consumed().maxPets.consumed).toBe(1); + expect(stubs.consumed().maxVisits.consumed).toBe(3); + }); + + it('loses nothing when two requests arrive together', async () => { + // The case the batch alone could not fix, and the reason for `$inc`: two + // callers spending the same limit at the same time. Under read-modify-write + // both start from 0, both write 1, and one consumption is gone. + const stubs = withStubs(aContract()); + const service = await aService(); + + await Promise.all([ + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + ]); + + expect(stubs.consumed().maxPets.consumed).toBe(2); + }); + + it('loses nothing across many concurrent requests', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await Promise.all( + Array.from({ length: 20 }, () => + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1) + ) + ); + + expect(stubs.consumed().maxPets.consumed).toBe(20); + }); + + it('composes concurrent requests that touch different limits', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await Promise.all([ + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + service._applyExpectedConsumption('user1', 'petclinic-maxVisits', 1), + ]); + + expect(stubs.consumed().maxPets.consumed).toBe(1); + expect(stubs.consumed().maxVisits.consumed).toBe(1); + }); + + it('touches the contract once however many limits there are', async () => { + // Not only correctness: one round trip instead of one per limit, and no + // read at all, since the database does the arithmetic. + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 1, + }); + + expect(stubs.counts()).toEqual({ reads: 0, writes: 0, increments: 1 }); + }); + + it('still applies a single limit', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumption('user1', 'petclinic-maxPets', 2); + + expect(stubs.consumed().maxPets.consumed).toBe(2); + }); + + it('does nothing at all when given nothing', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', {}); + + expect(stubs.counts()).toEqual({ reads: 0, writes: 0, increments: 0 }); + }); + + it('refuses the whole request when one limit does not exist', async () => { + // Rather than applying the valid ones and then failing, which would leave + // the contract half-updated. The check is part of the same operation, so a + // limit cannot be validated and then vanish before the write. + const stubs = withStubs(aContract()); + const service = await aService(); + + await expect( + service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-nosuchlimit': 1, + }) + ).rejects.toThrow(/not found in contract/); + + expect(stubs.consumed().maxPets.consumed).toBe(0); + }); + + it('says so when there is no contract at all', async () => { + const stubs = withStubs(aContract()); + stubs.contractRepository.incrementUsageLevels.mockResolvedValue(null); + stubs.contractRepository.findByUserId.mockResolvedValue(null); + const service = await aService(); + + await expect( + service._applyExpectedConsumptions('user1', { 'petclinic-maxPets': 1 }) + ).rejects.toThrow(/Contract with userId user1 not found/); + }); + + it('keeps the previous value of every limit for reverting', async () => { + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 1, + }); + + const cachedKeys = stubs.cacheService.set.mock.calls.map((call: any[]) => call[0]); + expect(cachedKeys.some((key: string) => key.includes('maxPets'))).toBe(true); + expect(cachedKeys.some((key: string) => key.includes('maxVisits'))).toBe(true); + }); + + it('records this caller’s own starting point, not whatever it read', async () => { + // The snapshot kept for reverting is derived from the result of the + // increment, so it is this caller's contribution that gets taken back even + // when other calls landed in between. + const contract = aContract(); + contract.usageLevels.petclinic.maxPets.consumed = 7; + + const stubs = withStubs(contract); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { 'petclinic-maxPets': 3 }); + + const snapshot = (stubs.cacheService.set.mock.calls as any[][]).find(call => + String(call[0]).includes('maxPets') + ); + expect(snapshot?.[1]).toBe(7); + expect(stubs.consumed().maxPets.consumed).toBe(10); + }); +});