Apply expected consumption for all limits in one write

[javiercavlop]

The problem

An evaluation that touches two usage limits records one of them.

_applyExpectedConsumption reads the whole contract, increments a single usage
level in its own copy, and writes the whole contract back:

let contract = await this.cacheService.get(`contracts.${userId}`);
if (!contract) contract = await this.contractRepository.findByUserId(userId);
…
contract.usageLevels[serviceName][usageLimit].consumed += expectedConsumption;
const updatedContract = await this.contractRepository.update(userId, contract);

and evaluateFeature called it once per limit, concurrently:

const limits = Object.keys(featureEvaluation.used);
await Promise.all(
  limits.map(limit =>
    contractService._applyExpectedConsumption(options.userId!, limit, expectedConsumption[limit])
  )
);

Every call reads the same starting state, and each writes back its own copy of
the whole document. The last write wins and the other increments are gone.

Why it matters

This is on the path that decides whether somebody may use a feature, and it
fails silently: the evaluation returns true, the response looks right, and the
usage level for every limit but one is simply not moved. Nothing errors, so the
first sign is a quota that never seems to run out.

For a feature with a single usage limit — which is most of them — it never
happens, which is why it can sit unnoticed.

The change

_applyExpectedConsumptions(userId, Record<limitId, amount>) applies every
limit to one contract copy and writes once. Correct, and one round trip instead
of one per limit.

Every limit is validated before anything is written, so a request naming one
limit that does not exist cannot leave the others half-applied — the previous
code could apply some and then throw.

_applyExpectedConsumption stays, delegating, so nothing that calls it breaks.

The root fix, as requested

The first version of this PR batched the limits into one read and one write,
which stopped an evaluation from losing its own limits but left two requests
losing each other's. I flagged that here and @Alex-GF asked for the $inc
version, which is what this now is.

ContractRepository.incrementUsageLevels hands the arithmetic to the database:

const filter: Record<string, unknown> = { 'userContact.userId': userId };
const inc: Record<string, number> = {};

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 });

Two things worth pointing out:

• $exists in the filter, not a prior read. Validation and the write are one
  operation, so a limit cannot be checked and then vanish before the update — and
  a limit that is not on the contract matches no document instead of being
  created by $inc, which is what would otherwise happen. Which of contract or
  limit was missing is worked out afterwards, only to phrase the error, and only
  once we know something was.
• The revert snapshot is derived, not read. _revertExpectedConsumption
  needs the value from before this call. Taking it from the result minus the
  amount applied means this caller takes back its own contribution even if other
  calls landed in between; reading it beforehand would reintroduce exactly the
  race being removed.

The service no longer reads the contract at all on this path, so the cached copy
can no longer be the basis of a write — it is only refreshed from the
authoritative result.

Verification

11 tests in src/test/contract.expected-consumption.test.ts, against a
stubbed repository and cache so the reads and writes themselves can be asserted.
The stub models the database honestly: incrementUsageLevels adds to whatever is
stored when it runs, which is the guarantee $inc gives and the one an
application-side read-modify-write cannot.

✓ src/test/contract.expected-consumption.test.ts (11 tests)
  ✓ records every limit, not just the last one written
  ✓ loses nothing when two requests arrive together
  ✓ loses nothing across many concurrent requests
  ✓ composes concurrent requests that touch different limits
  ✓ touches the contract once however many limits there are
  ✓ still applies a single limit
  ✓ does nothing at all when given nothing
  ✓ refuses the whole request when one limit does not exist
  ✓ says so when there is no contract at all
  ✓ keeps the previous value of every limit for reverting
  ✓ records this caller's own starting point, not whatever it read

Putting the batched read-modify-write back in place of the $inc call fails
four of them — the three concurrency tests and the round-trip count — so the
change is load-bearing rather than incidental:

× loses nothing when two requests arrive together
× loses nothing across many concurrent requests
× composes concurrent requests that touch different limits
× touches the contract once however many limits there are

Against a real MongoDB 7.0.16, exercising the $inc through the API rather than
a stub:

✓ src/test/contract.test.ts          (77 tests)
✓ src/test/feature-evaluation.test.ts (26 tests)
✓ src/test/service.test.ts           (44 tests)

Full suite: 13 files, 0 failures. npx tsc --noEmit is clean.

[javiercavlop]

The red Integration Tests Run check on this PR is not caused by the change. The job reads its Mongo port and database name from the testing environment, which GitHub withholds from pull requests opened from a fork, so the action is asked to publish port `` and docker refuses -p : before any test runs:

docker: invalid publish opts format (should be name=value but got ':').
Error starting MongoDB Docker container

The workflow has never passed on a fork PR — every green run in its history came from a branch inside the repository. #62 fixes it, and its own check is green, which is the fix running on a fork PR. Once that lands this check should go green here too.

[javiercavlop]

Full suite, verified locally. Since the workflow cannot run on a fork PR until #62 lands, I reproduced the CI environment (Mongo 7.0.16 on 27017, Redis 7 on 6379, an api/.env matching what the workflow generates) and ran every test file in its own vitest process, as run-tests.sh does:

13 files passed, 0 failed

That is the repository's 12 files plus the one this PR adds. The same run on main gives 12 files / 701 tests, all passing, so this branch adds tests and breaks none.

One note on method, since it changed a conclusion: an earlier run of mine reported failures in contract.test.ts and service.test.ts. Those were 5000 ms timeouts caused by my own machine being busy, not by any change here — repeated on an idle machine they pass (77/77 in 124 s, 44/44 in 46 s). One of them also draws a random pricing file per run, so it is flaky by construction.

[Alex-GF]

Thanks for your contribution. Actually, if you don't mind, would be great to solve the root of the problem by implementing the $inc version

[javiercavlop]

Done — this now does the $inc version rather than the batched read-modify-write, and I have rewritten the description accordingly.

ContractRepository.incrementUsageLevels builds one findOneAndUpdate with $inc for every limit, and puts { $exists: true } for each path in the filter rather than reading first — so validation and the write are a single operation, and a limit that is not on the contract matches no document instead of being created by $inc. The service no longer reads the contract on this path at all; the snapshot kept for _revertExpectedConsumption is derived from the result minus the amount applied, so a caller takes back its own contribution even when other calls landed in between.

The test that pinned the lossy behaviour is gone, replaced by three that assert it no longer happens (two concurrent calls on the same limit, twenty concurrent calls, and concurrent calls on different limits). Putting the read-modify-write back fails four of the eleven, so the change is load-bearing. contract.test.ts (77) passes against a real MongoDB 7.0.16, and the CI check on this PR is green.

Thanks for pushing for the root fix — it is the better change.