Skip to content

Free-tier inspection cap counts what exists, not what was created - #298

Closed
important-new wants to merge 5 commits into
InspectorHub:mainfrom
important-new:fix/quota-counts-existing-inspections
Closed

Free-tier inspection cap counts what exists, not what was created#298
important-new wants to merge 5 commits into
InspectorHub:mainfrom
important-new:fix/quota-counts-existing-inspections

Conversation

@important-new

Copy link
Copy Markdown
Contributor

The free-tier inspection cap read a monotonic counter, so deleting an
inspection never returned the allowance. A tenant who cleaned up a duplicate
stayed capped, and a counter that drifted above the real row count refused
someone who was genuinely under the cap. The only remedy was editing the
counter by hand, which is not a remedy.

The gate now counts the inspection rows a tenant has. usage_counters.value
for this metric degrades from source-of-truth to a self-healing display cache.
No migration, no schema change.

The statement

Still one statement, so meta.changes === 0 remains the authoritative "at cap"
answer with no read-then-write window inside the guard. Two things a probe
forced (tests/unit/quota/inspection-cap-counts-rows.spec.ts pins both):

  • INSERT ... SELECT ... WHERE, not VALUES. A conflict-free INSERT never
    reaches DO UPDATE, so that branch's WHERE cannot refuse anyone — a tenant
    with rows but no counter row walked straight past the cap. One predicate now
    governs both branches. (excluded.tenant_id is in scope inside
    DO UPDATE ... WHERE and resolves per-tenant; verified on real D1, so no
    bound-parameter fallback is needed.)

  • consumeInspection(tenantId, count). Because the gate counts rows the
    caller inserts only afterwards, N looped calls all read the same count and
    all pass. A multi-sub inspection request consumed in a loop, which took a
    tenant from 3 inspections to 6 deterministically. The batch is now consumed
    as one unit and is admitted whole or not at all.

The trade this makes, deliberately

The caller inserts its row after consumeInspection returns, so two creates
that overlap before either row lands both pass. No formulation recovers the old
serialization: an in-flight claim and a counter left high by a delete are the
same stored state, so any predicate that lets the counter fall to match rows can
be raced, and any predicate that blocks a high counter re-breaks the case this
fixes.

So the cap becomes a steady-state invariant rather than a serialized claim.
The overshoot is bounded by in-flight concurrency and self-corrects, because the
next create counts what actually exists and refuses. That is the better trade:
the previous design bought serialization with a counter that could only climb,
which is what cost users their allowance permanently.

tests/workers/quota-cap-concurrency.spec.ts pins both halves under real
workerd — at the cap, six genuinely concurrent consumes all fail; below it, the
bound reproduces and then self-corrects.

Not touched

sms and email keep tallying. They are consumed events with no rows to count,
so converting them to a row count would silently uncap them. Asserted.

Standalone deploys are unaffected — the guard is only constructed when the
deployment profile enables usage quotas.

Also

GET /api/usage/summary reports the live row count for a tenant whose
inspections are capped: the number shown against a cap has to be the number the
cap is enforced against, or someone who deletes three inspections is told they
are at the limit while creating works. Uncapped tenants keep the cumulative
lifetime counter, which is measured against nothing and should not quietly
change meaning.

Verification

lint · test:unit · test:web · test:workers · db:check — all green.

🤖 Generated with Claude Code

Probes the primitive the cap's concurrency guarantee rests on, before any
code relies on it: a conditional upsert whose WHERE holds a correlated
subquery over another table.

Settles three things:
- `excluded.tenant_id` IS in scope inside `DO UPDATE ... WHERE`, and
  resolves to the calling tenant (a sibling tenant's rows do not cap it).
  No bound-parameter fallback is needed.
- `changes === 0` is reported when that subquery is false.
- The plain `INSERT ... VALUES` form leaves the INSERT branch UNGATED: a
  conflict-free insert never reaches DO UPDATE, so a tenant already over
  the cap but with no counter row yet gets a free pass. The
  `INSERT ... SELECT ... WHERE` form gates both branches with the same
  predicate and is what the guard will use.
The free-tier cap read a monotonic counter, so a tenant who deleted an
inspection never got the allowance back. Found in production 2026-08-05:
one tenant had 1 inspection and 4 of 5 consumed; another was capped at 5
with 3 rows. Five counters were corrected by hand that day — this makes
that correction unnecessary rather than repeatable.

The gate now counts inspection rows. `usage_counters.value` for this
metric degrades from source-of-truth to a self-healing display cache.
Still one statement, so `meta.changes === 0` stays the authoritative
"at cap" answer with no read-then-write window inside the guard.

Two things the probe (previous commit) forced:

- `INSERT ... SELECT ... WHERE` rather than VALUES, so the INSERT branch
  is gated too. A conflict-free INSERT never reaches DO UPDATE, so the
  VALUES form waves through a tenant who has rows but no counter row.

- `consumeInspection(tenantId, count)`. Because the gate counts rows the
  caller inserts only afterwards, N looped calls all read the same count
  and all pass — a 3-sub inspection request took a tenant from 3
  inspections to 6, deterministically. InspectionRequestService.create
  now consumes the batch as one unit; it is admitted whole or not at all.

What counting rows gives up, unavoidably: two creates that overlap before
either row lands both pass. The cap becomes a steady-state invariant
rather than a serialized claim — the overshoot is bounded by in-flight
concurrency and self-corrects, since the next create counts what exists.
That is the better trade: the old design bought serialization with a
counter that could only climb, which is what cost real tenants their
allowance permanently.

sms/email are untouched — consumed events with no rows to count.
Converting them to a row count would silently uncap them.
…rrency

The unit suite runs the guard against better-sqlite3, which is synchronous
— Promise.all there overlaps nothing, so it can only assert the statement's
logic. Two things are testable only under workerd:

- That D1 accepts the statement at all. `INSERT ... SELECT ... WHERE` with
  an `ON CONFLICT ... DO UPDATE ... WHERE` whose predicate is a correlated
  subquery over another table, plus `excluded.` inside that predicate, is
  not a shape D1 code usually reaches for, and `meta.changes === 0` on the
  no-rows-selected path is the signal the whole gate reads. Confirmed.

- What concurrent callers actually observe. At the cap, six genuinely
  overlapping consumes ALL fail — no phantom pass. Below the cap, two that
  overlap before either row lands both pass, which pins the bound recorded
  in guard.ts as real-engine behaviour rather than a test-driver artefact.
Retires the 2026-08-05 manual production correction rather than leaving it
to be repeated.

guard.ts header now records that `usage_counters.value` for `inspections`
is a cache and not the gate, so the next person who finds a value higher
than the row count does not "fix" it. Deleting the row is worse than
leaving it, and the header says why. sms/email are called out as the
opposite case: consumed events whose counters ARE the source of truth.

/api/usage decision (plan Task 4 Step 2), taken rather than left open: a
tenant whose inspections are capped now gets the live row count — the
number shown against a cap has to be the number the cap is enforced
against, or a tenant who deletes three inspections is told "5 of 5 used"
while creating works, which is the visible half of the defect. Uncapped
tenants keep the cumulative lifetime counter: with `caps: null` it is
measured against nothing, and silently redefining it from "ever created"
to "currently have" would change an analytics figure nobody asked to
change.
lint:provider-helpers hard-fails on `drizzle(c.env.DB)` in an API route;
route handlers go through the helper.
@important-new

Copy link
Copy Markdown
Contributor Author

Superseded by #299, which already contains this work.

Verified rather than assumed: 7 of the 9 files this PR touches — including all six test files — are byte-identical to main. The two that differ do so in the other direction (main has more: the aiCaps loader and the request-service extraction), and main already carries consumeInspection(tenantId, count = 1) plus the counter-is-a-cache note that were the substance here.

Note for anyone auditing this later: git cherry reports all five commits as unmerged, and that is an artefact of the squash — patch-ids cannot survive it. The file comparison is the reliable check.

@important-new
important-new deleted the fix/quota-counts-existing-inspections branch August 7, 2026 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant