From af9d24412e8a1952f63f448ff6b67b8adda17002 Mon Sep 17 00:00:00 2001 From: Alex-GF Date: Thu, 14 May 2026 12:27:54 +0200 Subject: [PATCH 1/9] feat: added script to test production --- scripts/test-rate-limit-production.ts | 76 +++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 scripts/test-rate-limit-production.ts diff --git a/scripts/test-rate-limit-production.ts b/scripts/test-rate-limit-production.ts new file mode 100644 index 0000000..53ef7b4 --- /dev/null +++ b/scripts/test-rate-limit-production.ts @@ -0,0 +1,76 @@ +import https from "node:https"; + +const TOTAL_REQUESTS = 200; +const CONCURRENT_REQUESTS = 50; + +const BASE_URL = + process.env.RATE_LIMIT_URL ?? + "https://space.score.us.es/api/v1/healthcheck"; + +process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"; + +function makeRequest(): Promise { + return new Promise((resolve) => { + const req = https.get(BASE_URL, (res) => { + resolve(res.statusCode ?? 0); + res.resume(); + }); + + req.on("error", () => { + resolve(0); + }); + }); +} + +async function runBatch(size: number): Promise { + return Promise.all( + Array.from({ length: size }, () => makeRequest()) + ); +} + +async function main(): Promise { + const results: number[] = []; + + for ( + let i = 0; + i < TOTAL_REQUESTS; + i += CONCURRENT_REQUESTS + ) { + const batch = await runBatch(CONCURRENT_REQUESTS); + results.push(...batch); + } + + const grouped = results.reduce>( + (acc, code) => { + acc[code] = (acc[code] ?? 0) + 1; + return acc; + }, + {} + ); + + console.log("Response distribution:"); + console.log(grouped); + + const has429 = (grouped[429] ?? 0) > 0; + const has200 = (grouped[200] ?? 0) > 0; + + if (!has429) { + console.error( + "Rate limiting did not trigger any 429 responses." + ); + + process.exit(1); + } + + if (!has200) { + console.error( + "All requests failed. Backend may be unhealthy." + ); + + process.exit(1); + } + + console.log("Rate limiting is working."); +} + +main(); \ No newline at end of file From 465c4a8f3492663740a7b4ba33c719116e9bba16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Thu, 30 Jul 2026 18:20:51 +0200 Subject: [PATCH 2/9] Bump pricing4ts to ^0.11.1 for Pricing2Yaml 3.1 0.10.3 stops one version short of the current specification: 0.10.3: PRICING2YAML_VERSIONS = ["1.0","1.1","2.0","2.1","3.0"] 0.11.1: PRICING2YAML_VERSIONS = ["1.0","1.1","2.0","2.1","3.0","3.1"] so registering a pricing written against 3.1 is refused with "Unsupported version: 3.1", and the only way to register anything is to declare 3.0 and stay a version behind. No source change was needed: the imports SPACE uses - retrievePricingFromPath, retrievePricingFromText, and the Pricing/Plan/Feature/UsageLimit types - kept their signatures across the two releases. tsc passes and the API suite passes against a local MongoDB and Redis. --- api/package.json | 2 +- api/pnpm-lock.yaml | 11 ++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/api/package.json b/api/package.json index 147e6db..bf6c105 100644 --- a/api/package.json +++ b/api/package.json @@ -58,7 +58,7 @@ "multer": "1.4.5-lts.2", "nock": "^14.0.10", "node-fetch": "^3.3.2", - "pricing4ts": "^0.10.3", + "pricing4ts": "^0.11.1", "redis": "^4.7.0", "socket.io": "^4.8.1", "uuid": "^11.1.0" diff --git a/api/pnpm-lock.yaml b/api/pnpm-lock.yaml index 082c3c9..a6c07c0 100644 --- a/api/pnpm-lock.yaml +++ b/api/pnpm-lock.yaml @@ -63,8 +63,8 @@ importers: specifier: ^3.3.2 version: 3.3.2 pricing4ts: - specifier: ^0.10.3 - version: 0.10.3(@openfeature/core@1.8.0)(minizinc@4.4.3) + specifier: ^0.11.1 + version: 0.11.1(@openfeature/core@1.8.0)(minizinc@4.4.3) redis: specifier: ^4.7.0 version: 4.7.1 @@ -1783,8 +1783,8 @@ packages: engines: {node: '>=14'} hasBin: true - pricing4ts@0.10.3: - resolution: {integrity: sha512-RU9s3RvU7u3jByJsciihDZgF3XQCi2GWCyqMwU6wB+Hx/oRO5Wq+FidCYQadgExDMwD9KAkP/AXIPUCnolk95Q==} + pricing4ts@0.11.1: + resolution: {integrity: sha512-Q2lTrkhvgAGyefr9dh1qN7XL3hQ+NdU5OGHjD00yHMB2YOnbfV3gMfpc5hkaxjd93R1OGUqnfExQlHxesAq0wg==} peerDependencies: minizinc: ^4.3.5 @@ -2124,6 +2124,7 @@ packages: uuid@10.0.0: resolution: {integrity: sha512-8XkAphELsDnEGrDxUOHB3RGvXz6TeuYSGEZBOjtTtPm2lwhGBjLgOzLHB63IUWfBpNucQjND6d3AOudO+H3RWQ==} + deprecated: uuid@10 and below is no longer supported. For ESM codebases, update to uuid@latest. For CommonJS codebases, use uuid@11 (but be aware this version will likely be deprecated in 2028). hasBin: true uuid@11.1.0: @@ -3940,7 +3941,7 @@ snapshots: prettier@3.5.3: {} - pricing4ts@0.10.3(@openfeature/core@1.8.0)(minizinc@4.4.3): + pricing4ts@0.11.1(@openfeature/core@1.8.0)(minizinc@4.4.3): dependencies: '@openfeature/server-sdk': 1.18.0(@openfeature/core@1.8.0) '@types/papaparse': 5.3.16 From 988fd7da7d125a3dedf48964c7b01e8c1f17fc01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Thu, 30 Jul 2026 18:33:51 +0200 Subject: [PATCH 3/9] A database outage is not a 401, and not healthy either MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Authentication reads the database, so anything that can go wrong with the database surfaces as an exception in the auth middleware - and the catch turned every one of them into 401 with the driver's message as the body: 401 {"error":"connect ECONNREFUSED 127.0.0.1:27017"} 401 {"error":"Operation `users.findOne()` buffering timed out after 10000ms"} The status says the caller sent a bad key. It sends whoever is debugging to look at credentials that were never judged, because the request never got far enough to judge them. The healthcheck agreed: it returned 200 unconditionally, proving only that the HTTP listener was up. A SPACE whose MongoDB container had stopped reported healthy for fifteen hours while refusing every authenticated request as unauthorised. Two signals agreeing on the wrong answer is what made this expensive. authenticateUserApiKey and authenticateOrgApiKey now throw a typed InvalidApiKeyError for the two cases that really are bad credentials. The catch answers 401 only for those; anything else is 503 with Retry-After, since the credential was never judged and the caller should try again. The healthcheck reports 503 when Mongoose is not connected, and pings the database when it is - readyState is what the driver believes, a ping is what the database says, and they disagree when a connection has gone stale. Verified by stopping the database under a running server: main: healthcheck 200, authed call 401 {"error":"connect ECONNREFUSED…"} this PR: healthcheck 503 {"database":"disconnected"} authed call 503 {"error":"Space cannot verify credentials right now."} --- api/src/main/middlewares/AuthMiddleware.ts | 32 ++++++++++++-- api/src/main/routes/HealthcheckRoutes.ts | 49 ++++++++++++++++++---- 2 files changed, 69 insertions(+), 12 deletions(-) diff --git a/api/src/main/middlewares/AuthMiddleware.ts b/api/src/main/middlewares/AuthMiddleware.ts index 687c108..97a6209 100644 --- a/api/src/main/middlewares/AuthMiddleware.ts +++ b/api/src/main/middlewares/AuthMiddleware.ts @@ -19,6 +19,18 @@ import { HttpMethod, OrganizationApiKeyRole } from '../types/permissions'; * Sets req.user for User API Keys * Sets req.org for Organization API Keys */ +/** + * A credential that was read and found wanting. + * + * Distinguished from every other failure on purpose. Authentication reads the + * database, so anything that can go wrong with the database - a dropped + * connection, a replica-set election, a Mongo that is simply not running - + * surfaces as an exception here too. Answering 401 to those says the caller + * sent a bad key, which is untrue and sends whoever is debugging to look at + * their credentials. + */ +class InvalidApiKeyError extends Error {} + const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: NextFunction) => { const apiKey = req.headers['x-api-key'] as string; @@ -40,11 +52,25 @@ const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: N return checkPermissions(req, res, next); } catch (err: any) { - if (!res.headersSent) { + if (res.headersSent) { + return; + } + + if (err instanceof InvalidApiKeyError) { return res.status(401).json({ error: err.message || 'Invalid API Key', }); } + + // Anything else got as far as trying and could not finish - almost always + // the database. 503 rather than 401, because the credential was never + // judged, and Retry-After because it is worth trying again. + console.error('Authentication could not be completed:', err); + res.setHeader('Retry-After', '5'); + return res.status(503).json({ + error: 'Space cannot verify credentials right now.', + details: err?.message ?? String(err), + }); } }; @@ -57,7 +83,7 @@ async function authenticateUserApiKey(req: Request, apiKey: string): Promise { - res.status(200).json({ - message: 'Service is up and running!', - }); - } - ); + app.route(`${baseUrl}/healthcheck`).get(async (req: any, res: any) => { + // The database is not a detail of this service, it is the service: every + // authenticated route reads it before it can answer anything. A check that + // proves only the HTTP listener is up reports a Space that cannot serve a + // single request as healthy, and keeps reporting it indefinitely while + // every call fails. + if (mongoose.connection.readyState !== CONNECTED) { + return res.status(503).json({ + message: 'Service is up but cannot reach its database.', + database: 'disconnected', + }); + } + + try { + // readyState is what the driver believes. A ping is what the database + // says, and the two disagree when a connection has gone stale. + await mongoose.connection.db!.admin().ping(); + } catch (error: any) { + return res.status(503).json({ + message: 'Service is up but its database is not answering.', + database: 'unreachable', + details: error?.message ?? String(error), + }); + } + + res.status(200).json({ + message: 'Service is up and running!', + database: 'connected', + }); + }); }; export default loadFileRoutes; From 26fe9f64972d1607eb911f0cf4cefb272682c4f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Thu, 30 Jul 2026 18:38:45 +0200 Subject: [PATCH 4/9] Apply expected consumption for all limits in one write An evaluation that touches two usage limits recorded one of them. _applyExpectedConsumption reads the whole contract, increments a single usage level in its own copy, and writes the whole contract back. evaluateFeature called it once per limit through Promise.all, so every call read the same starting state and only the last write survived - a lost update, silent, on the path that decides whether somebody may use a feature. _applyExpectedConsumptions applies every limit to one contract copy and writes once: correct, and one round trip instead of one per limit. Limits are all validated before anything is written, so naming a limit that does not exist cannot leave the others half-applied. The single-limit method stays, delegating. Seven tests against stubbed repository and cache, including one that pins why the batch method exists: calling the single-limit method concurrently is still lossy, because read-modify-write on a whole document cannot be made safe by calling it more carefully. Making that safe means an atomic $inc in the repository, which is a larger change than this one. Their feature-evaluation suite (26 tests) passes unchanged. --- api/src/main/services/ContractService.ts | 57 ++++-- .../feature-evaluation/featureEvaluation.ts | 23 +-- .../contract.expected-consumption.test.ts | 171 ++++++++++++++++++ 3 files changed, 227 insertions(+), 24 deletions(-) create mode 100644 api/src/test/contract.expected-consumption.test.ts diff --git a/api/src/main/services/ContractService.ts b/api/src/main/services/ContractService.ts index bb0b6cb..09cb5e0 100644 --- a/api/src/main/services/ContractService.ts +++ b/api/src/main/services/ContractService.ts @@ -459,6 +459,27 @@ class ContractService { usageLimitId: string, expectedConsumption: number ): Promise { + await this._applyExpectedConsumptions(userId, { [usageLimitId]: expectedConsumption }); + } + + /** + * Apply several expected consumptions to a contract in one read and one write. + * + * Applying them one at a time loses all but one. Each application reads the + * whole contract, increments a single usage level in its own copy, and writes + * the whole contract back - so N concurrent applications all start from the + * same state and the last write wins. An evaluation touching two limits + * recorded one of them. + */ + async _applyExpectedConsumptions( + userId: string, + expectedConsumptions: Record + ): Promise { + const usageLimitIds = Object.keys(expectedConsumptions); + if (usageLimitIds.length === 0) { + return; + } + let contract = await this.cacheService.get(`contracts.${userId}`); if (!contract) { @@ -469,28 +490,38 @@ class ContractService { throw new Error(`Contract with userId ${userId} not found`); } - const serviceName: string = usageLimitId.split('-')[0]; - const usageLimit: string = usageLimitId.split('-')[1]; + // Every limit is validated before anything is written, so a request naming + // one limit that does not exist cannot leave the others half-applied. + const targets = usageLimitIds.map(usageLimitId => { + const serviceName: string = usageLimitId.split('-')[0]; + const usageLimit: string = usageLimitId.split('-')[1]; + + if (!contract.usageLevels[serviceName]?.[usageLimit]) { + throw new Error(`Usage level ${usageLimit} not found in contract for userId ${userId}`); + } + + return { serviceName, usageLimit, amount: expectedConsumptions[usageLimitId] }; + }); - if (contract.usageLevels[serviceName][usageLimit]) { + const appliedAt = new Date().getTime(); + + for (const { serviceName, usageLimit, amount } of targets) { await this.cacheService.set( - `${new Date().getTime()}.usageLevels.${userId}.${serviceName}.${usageLimit}`, + `${appliedAt}.usageLevels.${userId}.${serviceName}.${usageLimit}`, contract.usageLevels[serviceName][usageLimit].consumed, 120 ); // 120 secs = 2 mins - contract.usageLevels[serviceName][usageLimit].consumed += expectedConsumption; - - const updatedContract = await this.contractRepository.update(userId, contract); + contract.usageLevels[serviceName][usageLimit].consumed += amount; + } - if (!updatedContract) { - throw new Error(`Failed to update contract for userId ${userId}`); - } + const updatedContract = await this.contractRepository.update(userId, contract); - 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}`); + if (!updatedContract) { + throw new Error(`Failed to update contract for userId ${userId}`); } + + await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour } 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..7349d8d --- /dev/null +++ b/api/src/test/contract.expected-consumption.test.ts @@ -0,0 +1,171 @@ +import { describe, it, expect, beforeEach, vi } from 'vitest'; +import container from '../main/config/container'; + +/** + * Applying expected consumption to more than one usage limit. + * + * Each application reads the whole contract, increments one usage level in its + * own copy, and writes the whole contract back. Doing that once per limit - + * concurrently, as `evaluateFeature` did - means every application starts from + * the same state and only the last write survives, so an evaluation touching + * two limits recorded one of them. + * + * These tests work against stubbed repository and cache so they can assert on + * the reads and writes themselves, which is where the defect lives. + */ + +function aContract() { + return { + userContact: { userId: 'user1', username: 'user1' }, + contractedServices: { petclinic: '2025' }, + subscriptionPlans: { petclinic: 'BASIC' }, + usageLevels: { + petclinic: { + maxPets: { consumed: 0 }, + maxVisits: { consumed: 0 }, + }, + }, + }; +} + +function withStubs(contract: any) { + // The repository hands back a *copy* on read and keeps whatever it is given, + // exactly as a database does - which is what makes a lost update visible. + const state = { current: contract }; + let writes = 0; + let reads = 0; + + const contractRepository = { + findByUserId: vi.fn(async () => { + reads += 1; + return JSON.parse(JSON.stringify(state.current)); + }), + update: vi.fn(async (_userId: string, updated: any) => { + writes += 1; + state.current = JSON.parse(JSON.stringify(updated)); + return 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 }) }; +} + +async function aService() { + const { default: ContractService } = await import('../main/services/ContractService'); + return new (ContractService as any)(); +} + +describe('Applying expected consumption to several limits', () => { + 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.state.current.usageLevels.petclinic.maxPets.consumed).toBe(1); + expect(stubs.state.current.usageLevels.petclinic.maxVisits.consumed).toBe(3); + }); + + it('reads and writes the contract once however many limits there are', async () => { + // Not only correctness: one round trip instead of one per limit. + const stubs = withStubs(aContract()); + const service = await aService(); + + await service._applyExpectedConsumptions('user1', { + 'petclinic-maxPets': 1, + 'petclinic-maxVisits': 1, + }); + + expect(stubs.counts()).toEqual({ reads: 1, writes: 1 }); + }); + + it('pins why the batch method exists: one call per limit still races', async () => { + // This is what `evaluateFeature` used to do - one call per limit, in + // parallel - and it is still lossy, because read-modify-write on a whole + // document cannot be made safe by calling it more carefully. + // + // Asserted rather than fixed here so nobody simplifies the call site back + // to Promise.all: the read-modify-write itself would have to become an + // atomic $inc for that to be safe, which is a larger change than this one. + const stubs = withStubs(aContract()); + const service = await aService(); + + await Promise.all([ + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1), + service._applyExpectedConsumption('user1', 'petclinic-maxVisits', 1), + ]); + + const levels = stubs.state.current.usageLevels.petclinic; + const recorded = levels.maxPets.consumed + levels.maxVisits.consumed; + expect(recorded, 'one increment is lost, which is the point').toBe(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.state.current.usageLevels.petclinic.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 }); + }); + + it('refuses the whole request when one limit does not exist', async () => { + // Rather than applying the valid ones and then throwing, which would leave + // the contract half-updated. + 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.state.current.usageLevels.petclinic.maxPets.consumed).toBe(0); + expect(stubs.counts().writes).toBe(0); + }); + + 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); + }); +}); From 29182f97198b37e75d57a85bd5e8e4dce90ad7b3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Thu, 30 Jul 2026 18:46:38 +0200 Subject: [PATCH 5/9] Accept an expected consumption of zero Two falsy checks treated a numeric zero as an absent value, so a caller who named a limit and declared it costs nothing was told they had not named it at all. The same check on the sum also refused an ordinary positive consumption whenever the usage level was still zero, which is the state of every contract on its first call and after every renewal. --- .../feature-evaluation/featureEvaluation.ts | 7 +- ...eature-evaluation.zero-consumption.test.ts | 112 ++++++++++++++++++ 2 files changed, 117 insertions(+), 2 deletions(-) create mode 100644 api/src/test/feature-evaluation.zero-consumption.test.ts diff --git a/api/src/main/utils/feature-evaluation/featureEvaluation.ts b/api/src/main/utils/feature-evaluation/featureEvaluation.ts index fd0fd9e..4532307 100644 --- a/api/src/main/utils/feature-evaluation/featureEvaluation.ts +++ b/api/src/main/utils/feature-evaluation/featureEvaluation.ts @@ -226,7 +226,7 @@ function _buildSuccessResult( subscriptionContext[limitKey], expectedConsumption[limitKey] ); - if (!updatedUsageLevel) { + if (updatedUsageLevel === undefined) { return _createErrorResult( 'INVALID_EXPECTED_CONSUMPTION', `No expectedConsumption value was provided for limit '${limitKey}', which is used in the evaluation of feature '${featureId}'. Please note that if you provide an expectedConsumption for any limit, you must provide it for all limits involved in that feature's evaluation.` @@ -250,7 +250,10 @@ function _updateUsageLevel( currentUsageLevel: number, expectedConsumption?: number ): number | undefined { - if (!expectedConsumption) { + // `undefined` means the caller did not mention this limit, which is the + // error the caller is told about. `0` means they mentioned it and it costs + // nothing - a different statement, and one a falsy check cannot tell apart. + if (expectedConsumption === undefined || expectedConsumption === null) { return undefined; } diff --git a/api/src/test/feature-evaluation.zero-consumption.test.ts b/api/src/test/feature-evaluation.zero-consumption.test.ts new file mode 100644 index 0000000..153205f --- /dev/null +++ b/api/src/test/feature-evaluation.zero-consumption.test.ts @@ -0,0 +1,112 @@ +import { describe, it, expect } from 'vitest'; +import { evaluateFeature } from '../main/utils/feature-evaluation/featureEvaluation'; +import type { + EvaluationContext, + FeatureEvaluationResult, + PricingContext, + SubscriptionContext, +} from '../main/types/models/FeatureEvaluation'; + +/** + * An expected consumption of zero. + * + * A caller who provides `expectedConsumption` must provide it for every limit + * involved in the feature's evaluation, or be refused. So the only way to say + * "this limit takes part in the evaluation but this call does not spend it" is + * to pass zero - which two falsy checks rejected as if the limit had been left + * out altogether. + * + * The second of those checks also refused a perfectly ordinary positive + * consumption, whenever the current usage level happened to be zero: a brand + * new contract, or the first call of a renewal period. + */ + +const FEATURE = 'petclinic-pets'; +const LIMIT = 'petclinic-maxPets'; + +const EXPRESSION = `subscriptionContext['${LIMIT}'] < pricingContext['usageLimits']['${LIMIT}']`; + +const pricingContext: PricingContext = { + features: { [FEATURE]: true }, + usageLimits: { [LIMIT]: 10 }, +}; + +const evaluationContext: EvaluationContext = { [FEATURE]: EXPRESSION }; + +/** @param usageLevel what the contract has consumed so far. */ +async function evaluate(usageLevel: number, expectedConsumption?: Record) { + const subscriptionContext: SubscriptionContext = { [LIMIT]: usageLevel }; + + return (await evaluateFeature(FEATURE, pricingContext, subscriptionContext, evaluationContext, { + simple: false, + expectedConsumption, + // No userId, so nothing is written: this is about the verdict, not the + // bookkeeping that follows it. + })) as FeatureEvaluationResult; +} + +describe('expectedConsumption of zero', () => { + it('is accepted, and leaves the usage level where it was', async () => { + const result = await evaluate(5, { [LIMIT]: 0 }); + + expect(result.error).toBeNull(); + expect(result.eval).toBe(true); + expect(result.used).toEqual({ [LIMIT]: 5 }); + }); + + it('is accepted on a contract that has consumed nothing yet', async () => { + // Both zeroes at once: the usage level and the consumption. This is the + // case a `!updatedUsageLevel` check gets wrong even after `0 + 0` has been + // computed correctly. + const result = await evaluate(0, { [LIMIT]: 0 }); + + expect(result.error).toBeNull(); + expect(result.used).toEqual({ [LIMIT]: 0 }); + }); + + it('is not reported as a missing value', async () => { + const result = await evaluate(0, { [LIMIT]: 0 }); + + expect(result.error?.code).not.toBe('INVALID_EXPECTED_CONSUMPTION'); + }); +}); + +describe('expectedConsumption on an untouched usage level', () => { + it('adds to a usage level of zero', async () => { + // Not about zero consumption at all: a plain consumption of 1 on a brand + // new contract. `1` is truthy, but only because the addition happens to + // leave a truthy total. + const result = await evaluate(0, { [LIMIT]: 1 }); + + expect(result.error).toBeNull(); + expect(result.used).toEqual({ [LIMIT]: 1 }); + }); + + it('adds to a non-zero usage level, as before', async () => { + const result = await evaluate(5, { [LIMIT]: 3 }); + + expect(result.used).toEqual({ [LIMIT]: 8 }); + }); +}); + +describe('expectedConsumption that really is missing', () => { + it('is still refused when the limit is left out', async () => { + const result = await evaluate(5, { 'petclinic-someOtherLimit': 1 }); + + expect(result.error?.code).toBe('INVALID_EXPECTED_CONSUMPTION'); + }); + + it('reports the current usage level when no consumption is given at all', async () => { + const result = await evaluate(5, undefined); + + expect(result.error).toBeNull(); + expect(result.used).toEqual({ [LIMIT]: 5 }); + }); + + it('treats an empty object as no consumption', async () => { + const result = await evaluate(5, {}); + + expect(result.error).toBeNull(); + expect(result.used).toEqual({ [LIMIT]: 5 }); + }); +}); From c1c510b9c260e43fb867ecb8c30ac19cb41c3a36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Thu, 30 Jul 2026 19:00:18 +0200 Subject: [PATCH 6/9] Let the integration tests run on pull requests from forks The job reads the Mongo port and database name from the `testing` environment, which GitHub withholds from fork pull requests. Both resolved to an empty string, so the action was asked to publish port `` and gave docker `-p :`, which it rejects before any test runs. Defaults now stand in when the environment is absent. They are the only values that can work, since MONGO_URI already hard-codes 27017 and space_testing_db. --- .github/workflows/run-unit-tests.yml | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/.github/workflows/run-unit-tests.yml b/.github/workflows/run-unit-tests.yml index 65ae528..b731ddb 100644 --- a/.github/workflows/run-unit-tests.yml +++ b/.github/workflows/run-unit-tests.yml @@ -4,6 +4,17 @@ on: branches: - 'develop' - 'main' + +# A pull request opened from a fork does not receive the `testing` environment's +# secrets or variables, so every `vars.*` and `secrets.*` below resolves to an +# empty string on those runs. The `|| '...'` fallbacks keep the job +# self-sufficient; where the environment is available, its values still win. +# +# The fallbacks are not arbitrary. MONGO_URI is hard-coded, so the container the +# suite talks to has to listen on 27017 and hold `space_testing_db` for the +# tests to connect at all - the two variables cannot hold anything else without +# breaking the run. JWT_SECRET and JWT_SALT are only a key and a pbkdf2 salt for +# tokens minted and verified inside the same run, so any non-empty pair works. jobs: build: name: Test @@ -22,14 +33,14 @@ jobs: uses: SpicyPizza/create-envfile@v2.0 with: envkey_ENVIRONMENT: "testing" - envkey_DATABASE_NAME: ${{ vars.CI_MONGO_INITDB_DATABASE }} + envkey_DATABASE_NAME: ${{ vars.CI_MONGO_INITDB_DATABASE || 'space_testing_db' }} envkey_MONGO_URI: mongodb://localhost:27017/space_testing_db?authSource=space_testing_db envkey_ADMIN_USER: "admin" envkey_ADMIN_PASSWORD: "4dm1n" envkey_REDIS_URL: "redis://localhost:6379" - envkey_JWT_SECRET: ${{ secrets.CI_JWT_SECRET }} + envkey_JWT_SECRET: ${{ secrets.CI_JWT_SECRET || 'ci_test_secret' }} envkey_JWT_EXPIRATION: "1h" - envkey_JWT_SALT: ${{ secrets.CI_JWT_SALT }} + envkey_JWT_SALT: ${{ secrets.CI_JWT_SALT || 'ci_test_salt' }} directory: . file_name: api/.env @@ -40,8 +51,8 @@ jobs: uses: supercharge/mongodb-github-action@1.10.0 with: mongodb-version: '7.0.16' - mongodb-db: ${{ vars.CI_MONGO_INITDB_DATABASE }} - mongodb-port: ${{ vars.CI_MONGO_PORT }} + mongodb-db: ${{ vars.CI_MONGO_INITDB_DATABASE || 'space_testing_db' }} + mongodb-port: ${{ vars.CI_MONGO_PORT || '27017' }} - name: Start Redis uses: shogo82148/actions-setup-redis@v1 From 2179283d09cc4d5f4bb40fa712ca3a8932bc46d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Thu, 30 Jul 2026 19:26:40 +0200 Subject: [PATCH 7/9] Keep a rejected key a 401 while the lookup itself fails with 503 Narrowing the catch to a dedicated error type was not enough on its own: UserService.findByApiKey reports an unknown key by throwing rather than by returning nothing, so the commonest 401 in the suite was being answered 503. The lookup is now wrapped in one place that tells the codebase's own INVALID DATA: signal - a caller's mistake - apart from anything else, which is the database being unable to answer. Both directions are pinned by tests. --- api/src/main/middlewares/AuthMiddleware.ts | 47 ++++++++--- api/src/test/middlewares/authOutage.test.ts | 93 +++++++++++++++++++++ 2 files changed, 130 insertions(+), 10 deletions(-) create mode 100644 api/src/test/middlewares/authOutage.test.ts diff --git a/api/src/main/middlewares/AuthMiddleware.ts b/api/src/main/middlewares/AuthMiddleware.ts index 97a6209..bb68ec2 100644 --- a/api/src/main/middlewares/AuthMiddleware.ts +++ b/api/src/main/middlewares/AuthMiddleware.ts @@ -8,6 +8,7 @@ import { import { matchPath, extractApiPath } from '../utils/routeMatcher'; import { LeanOrganization, OrganizationMember, OrganizationUserRole } from '../types/models/Organization'; import { HttpMethod, OrganizationApiKeyRole } from '../types/permissions'; +import { LeanUser } from '../types/models/User'; /** * Middleware to authenticate API Keys (both User and Organization types) @@ -80,16 +81,43 @@ const authenticateApiKeyMiddleware = async (req: Request, res: Response, next: N async function authenticateUserApiKey(req: Request, apiKey: string): Promise { const userService = container.resolve('userService'); - const user = await userService.findByApiKey(apiKey); - - if (!user) { - throw new InvalidApiKeyError('Invalid User API Key'); - } + const user = await rejectionOrOutage( + () => userService.findByApiKey(apiKey), + 'Invalid User API Key' + ); req.user = user; req.authType = 'user'; } +/** + * Run a credential lookup, telling a refusal apart from a failure. + * + * `UserService.findByApiKey` reports an unknown key by throwing rather than by + * returning nothing, using the `INVALID DATA:` prefix this codebase gives to a + * caller's own mistake. That has to keep answering 401. Anything else thrown by + * a lookup is the database being unable to answer, which is the case this + * middleware exists to stop reporting as a bad credential. + */ +async function rejectionOrOutage(lookup: () => Promise, absent: string): Promise { + let found: T; + + try { + found = await lookup(); + } catch (err: any) { + if (typeof err?.message === 'string' && err.message.startsWith('INVALID DATA:')) { + throw new InvalidApiKeyError(err.message); + } + throw err; + } + + if (!found) { + throw new InvalidApiKeyError(absent); + } + + return found; +} + /** * Authenticates an Organization API Key and populates req.org */ @@ -97,11 +125,10 @@ async function authenticateOrgApiKey(req: Request, apiKey: string): Promise organizationRepository.findByApiKey(apiKey), + 'Invalid Organization API Key' + ); req.org = { id: result.id!, diff --git a/api/src/test/middlewares/authOutage.test.ts b/api/src/test/middlewares/authOutage.test.ts new file mode 100644 index 0000000..890ec45 --- /dev/null +++ b/api/src/test/middlewares/authOutage.test.ts @@ -0,0 +1,93 @@ +import request from 'supertest'; +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { Server } from 'http'; +import { getApp, shutdownApp, baseUrl } from '../utils/testApp'; +import { createTestUser, deleteTestUser } from '../utils/users/userTestUtils'; +import container from '../../main/config/container'; +import { LeanUser } from '../../main/types/models/User'; + +/** + * Telling "your key is wrong" apart from "we could not check your key". + * + * Authentication reads the database, so a dropped connection, a replica-set + * election or a Mongo that is simply not running all surface as an exception in + * the same place an unknown key does. Answering 401 to those states something + * untrue about the caller's credential and sends whoever is debugging to look + * at their API key, which is the one place the problem is not. + * + * Both directions are pinned here, because the interesting part of the change + * is the boundary: a refusal must stay a refusal. + */ +describe('Authentication when the database cannot answer', function () { + let app: Server; + let user: LeanUser; + + beforeAll(async function () { + app = await getApp(); + user = await createTestUser('ADMIN'); + }); + + afterAll(async function () { + await deleteTestUser(user.username); + vi.restoreAllMocks(); + await shutdownApp(); + }); + + it('answers 503, not 401, when the lookup fails', async function () { + const userService: any = container.resolve('userService'); + const outage = vi + .spyOn(userService, 'findByApiKey') + .mockRejectedValue(new Error('MongooseServerSelectionError: connection timed out')); + + try { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', user.apiKey); + + expect(response.status).toBe(503); + expect(response.body.error).toContain('cannot verify credentials'); + } finally { + outage.mockRestore(); + } + }); + + it('asks the caller to try again', async function () { + const userService: any = container.resolve('userService'); + const outage = vi + .spyOn(userService, 'findByApiKey') + .mockRejectedValue(new Error('MongooseServerSelectionError: connection timed out')); + + try { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', user.apiKey); + + // A 503 without Retry-After tells a client nothing about whether waiting + // is worth it. + expect(response.headers['retry-after']).toBeDefined(); + } finally { + outage.mockRestore(); + } + }); + + it('still answers 401 to a key that was read and rejected', async function () { + // The regression this pairs with. `UserService.findByApiKey` reports an + // unknown key by throwing rather than by returning nothing, so narrowing + // the catch to a dedicated error type is not by itself enough to keep this + // a 401. + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', 'usr_nosuchkeyatall'); + + expect(response.status).toBe(401); + expect(response.body.error).toContain('INVALID DATA: Invalid API Key'); + }); + + it('still answers 401 to a key of no recognisable kind', async function () { + const response = await request(app) + .get(`${baseUrl}/users`) + .set('x-api-key', 'not-a-prefixed-key'); + + expect(response.status).toBe(401); + }); +}); From 70931b42e5299ac6d4dc8b2a71b2f08c7a575aff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20Cavero=20L=C3=B3pez?= Date: Fri, 31 Jul 2026 11:23:26 +0200 Subject: [PATCH 8/9] Let the database settle the increment, with $inc Batching a whole evaluation into one read and one write stopped an evaluation from losing its own limits, but not two requests from losing each other's: both read the same consumed value, both write the same total, and one consumption disappears. The increment is now handed to Mongo as $inc and evaluated against the stored document, so concurrent calls compose. The filter requires every usage level to exist, which keeps validation and the write in one operation instead of leaving a window between them. --- .../mongoose/ContractRepository.ts | 40 ++++ api/src/main/services/ContractService.ts | 82 ++++++--- .../contract.expected-consumption.test.ts | 173 +++++++++++++----- 3 files changed, 226 insertions(+), 69 deletions(-) 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 09cb5e0..0df8ee4 100644 --- a/api/src/main/services/ContractService.ts +++ b/api/src/main/services/ContractService.ts @@ -463,13 +463,19 @@ class ContractService { } /** - * Apply several expected consumptions to a contract in one read and one write. + * Apply several expected consumptions to a contract in one atomic update. * - * Applying them one at a time loses all but one. Each application reads the - * whole contract, increments a single usage level in its own copy, and writes - * the whole contract back - so N concurrent applications all start from the - * same state and the last write wins. An evaluation touching two limits - * recorded one of them. + * 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, @@ -480,48 +486,68 @@ class ContractService { return; } - let contract = await this.cacheService.get(`contracts.${userId}`); - - if (!contract) { - contract = await this.contractRepository.findByUserId(userId); - } - - if (!contract) { - throw new Error(`Contract with userId ${userId} not found`); - } - - // Every limit is validated before anything is written, so a request naming - // one limit that does not exist cannot leave the others half-applied. const targets = usageLimitIds.map(usageLimitId => { const serviceName: string = usageLimitId.split('-')[0]; const usageLimit: string = usageLimitId.split('-')[1]; - if (!contract.usageLevels[serviceName]?.[usageLimit]) { - throw new Error(`Usage level ${usageLimit} not found in contract for userId ${userId}`); - } - 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 appliedAt = new Date().getTime(); 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( `${appliedAt}.usageLevels.${userId}.${serviceName}.${usageLimit}`, - contract.usageLevels[serviceName][usageLimit].consumed, + updatedContract!.usageLevels[serviceName][usageLimit].consumed - amount, 120 ); // 120 secs = 2 mins + } + + await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour + } + + /** + * 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); - contract.usageLevels[serviceName][usageLimit].consumed += amount; + if (!contract) { + throw new Error(`Contract with userId ${userId} not found`); } - const updatedContract = await this.contractRepository.update(userId, contract); + const missing = targets.find( + ({ serviceName, usageLimit }) => !contract.usageLevels[serviceName]?.[usageLimit] + ); - if (!updatedContract) { - throw new Error(`Failed to update contract for userId ${userId}`); + if (missing) { + throw new Error( + `Usage level ${missing.usageLimit} not found in contract for userId ${userId}` + ); } - await this.cacheService.set(`contracts.${userId}`, updatedContract, 3600, true); // Cache for 1 hour + throw new Error(`Failed to update contract for userId ${userId}`); } async _revertExpectedConsumption( diff --git a/api/src/test/contract.expected-consumption.test.ts b/api/src/test/contract.expected-consumption.test.ts index 7349d8d..d903f54 100644 --- a/api/src/test/contract.expected-consumption.test.ts +++ b/api/src/test/contract.expected-consumption.test.ts @@ -2,16 +2,24 @@ import { describe, it, expect, beforeEach, vi } from 'vitest'; import container from '../main/config/container'; /** - * Applying expected consumption to more than one usage limit. + * Applying expected consumption to usage limits. * - * Each application reads the whole contract, increments one usage level in its - * own copy, and writes the whole contract back. Doing that once per limit - - * concurrently, as `evaluateFeature` did - means every application starts from - * the same state and only the last write survives, so an evaluation touching - * two limits recorded one of them. + * 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. * - * These tests work against stubbed repository and cache so they can assert on - * the reads and writes themselves, which is where the defect lives. + * 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() { @@ -28,22 +36,48 @@ function aContract() { }; } +const copy = (value: any) => JSON.parse(JSON.stringify(value)); + function withStubs(contract: any) { - // The repository hands back a *copy* on read and keeps whatever it is given, - // exactly as a database does - which is what makes a lost update visible. const state = { current: contract }; let writes = 0; let reads = 0; + let increments = 0; const contractRepository = { findByUserId: vi.fn(async () => { reads += 1; - return JSON.parse(JSON.stringify(state.current)); + 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; - state.current = JSON.parse(JSON.stringify(updated)); - return state.current; + 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); }), }; @@ -60,7 +94,13 @@ function withStubs(contract: any) { return original(name); }); - return { state, contractRepository, cacheService, counts: () => ({ reads, writes }) }; + return { + state, + contractRepository, + cacheService, + counts: () => ({ reads, writes, increments }), + consumed: () => state.current.usageLevels.petclinic, + }; } async function aService() { @@ -68,7 +108,7 @@ async function aService() { return new (ContractService as any)(); } -describe('Applying expected consumption to several limits', () => { +describe('Applying expected consumption', () => { beforeEach(() => { vi.restoreAllMocks(); }); @@ -82,31 +122,39 @@ describe('Applying expected consumption to several limits', () => { 'petclinic-maxVisits': 3, }); - expect(stubs.state.current.usageLevels.petclinic.maxPets.consumed).toBe(1); - expect(stubs.state.current.usageLevels.petclinic.maxVisits.consumed).toBe(3); + expect(stubs.consumed().maxPets.consumed).toBe(1); + expect(stubs.consumed().maxVisits.consumed).toBe(3); }); - it('reads and writes the contract once however many limits there are', async () => { - // Not only correctness: one round trip instead of one per limit. + 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 service._applyExpectedConsumptions('user1', { - 'petclinic-maxPets': 1, - 'petclinic-maxVisits': 1, - }); + 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(); - expect(stubs.counts()).toEqual({ reads: 1, writes: 1 }); + await Promise.all( + Array.from({ length: 20 }, () => + service._applyExpectedConsumption('user1', 'petclinic-maxPets', 1) + ) + ); + + expect(stubs.consumed().maxPets.consumed).toBe(20); }); - it('pins why the batch method exists: one call per limit still races', async () => { - // This is what `evaluateFeature` used to do - one call per limit, in - // parallel - and it is still lossy, because read-modify-write on a whole - // document cannot be made safe by calling it more carefully. - // - // Asserted rather than fixed here so nobody simplifies the call site back - // to Promise.all: the read-modify-write itself would have to become an - // atomic $inc for that to be safe, which is a larger change than this one. + it('composes concurrent requests that touch different limits', async () => { const stubs = withStubs(aContract()); const service = await aService(); @@ -115,9 +163,22 @@ describe('Applying expected consumption to several limits', () => { service._applyExpectedConsumption('user1', 'petclinic-maxVisits', 1), ]); - const levels = stubs.state.current.usageLevels.petclinic; - const recorded = levels.maxPets.consumed + levels.maxVisits.consumed; - expect(recorded, 'one increment is lost, which is the point').toBe(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 () => { @@ -126,7 +187,7 @@ describe('Applying expected consumption to several limits', () => { await service._applyExpectedConsumption('user1', 'petclinic-maxPets', 2); - expect(stubs.state.current.usageLevels.petclinic.maxPets.consumed).toBe(2); + expect(stubs.consumed().maxPets.consumed).toBe(2); }); it('does nothing at all when given nothing', async () => { @@ -135,12 +196,13 @@ describe('Applying expected consumption to several limits', () => { await service._applyExpectedConsumptions('user1', {}); - expect(stubs.counts()).toEqual({ reads: 0, writes: 0 }); + 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 throwing, which would leave - // the contract half-updated. + // 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(); @@ -151,8 +213,18 @@ describe('Applying expected consumption to several limits', () => { }) ).rejects.toThrow(/not found in contract/); - expect(stubs.state.current.usageLevels.petclinic.maxPets.consumed).toBe(0); - expect(stubs.counts().writes).toBe(0); + 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 () => { @@ -168,4 +240,23 @@ describe('Applying expected consumption to several limits', () => { 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); + }); }); From 2da036fef139165dff6880df5b89f64e8133fd4a Mon Sep 17 00:00:00 2001 From: Alex-GF Date: Fri, 31 Jul 2026 13:09:05 +0200 Subject: [PATCH 9/9] docs: citation config --- CITATION.cff | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 CITATION.cff diff --git a/CITATION.cff b/CITATION.cff new file mode 100644 index 0000000..d106274 --- /dev/null +++ b/CITATION.cff @@ -0,0 +1,27 @@ +cff-version: 1.2.0 +title: "SPACE" +message: "If you use this software, please cite it as below." +type: software +authors: + - family-names: García-Fernández + given-names: Alejandro + email: agarcia29@us.es + orcid: https://orcid.org/0009-0005-0778-1432 + - family-names: Parejo + given-names: José Antonio + email: japarejo@us.es + orcid: https://orcid.org/0000-0002-4708-4606 + - family-names: Ruiz-Cortés + given-names: Antonio + email: aruiz@us.es + orcid: https://orcid.org/0000-0001-9827-1834 +repository-code: "https://github.com/isa-group/space" +license: MIT +version: 1.5.0 +date-released: "2026-07-31" +keywords: + - pricing-driven self-adaptation + - self-adaptive systems + - cloud computing + - software pricing + - service-oriented architecture