fix(server): charge an install key use when a device is accepted

[gustavosbarreto]

An install key's used_times was incremented only where the server decided the admission by itself during a registration request: the automatic mode, an allowlist match, a webhook answering accept, the deferred webhook callback, and the reconcile that runs on a later agent phone-home. Accepting a device from the review queue goes through UpdateDeviceStatus, which stamped the verdict on the history event and left the counter alone.

The consequence was that usage_limit had no effect on a key in manual mode. A single-use manual key admitted devices forever, because isOverused() reads used_times and that never grew. The namespace's legacy key is created in manual mode, so keyless tenant-only registration was on that path too.

The charge now lives in the device status transaction, which is the one place that owns a device becoming accepted. Every path that accepts a device gets it, whoever made the decision. The store's increment was already the limit check (WHERE usage_limit = 0 OR used_times < usage_limit), so an exhausted key surfaces as ErrInstallKeyExhausted and the accept fails with 403 rather than silently doing nothing. Because the charge runs inside the transaction, a failed status write rolls the use back on its own, so the enrollment paths and the webhook callback no longer reserve and release it by hand.

Automatic enrollment behaviour is unchanged: an exhausted key still leaves the device pending, logged, since no one is waiting on that response.

Note for reviewers

Existing installations may hold manual keys whose accepted device count already exceeds usage_limit. This change does not backfill used_times, so those keys keep admitting devices until the counter catches up with the limit through new accepts. Backfilling from the accepted device count would be truthful and would block them immediately, at the cost of turning an upgrade into a provisioning stop for whoever relied on the limit not working.

Closes #7068

[github-actions]

Claude finished @gustavosbarreto's task in 4m 17s —— View job

---

Code Review Complete

Reviewed 5 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

A few notes from the analysis, for the record (nothing blocking):

• Rollback correctness verified. chargeInstallKeyUse runs inside the WithTransaction closure in updateDeviceStatus, and the store increment joins the same transaction via the context connection, so a later DeviceUpdate failure rolls the charge back — as the commit message claims.
• Exhaustion blocks every accept path. Manual accept (UpdateDeviceStatus), enrollment auto-accept (applyEnrollmentDecision), the webhook callback (ResolveEnrollmentCallback), and pairing all funnel through the one transaction, and ErrInstallKeyExhausted (ErrCodeLimit → HTTP 403) is the same status the callback returned before.
• No double-charge, single-charge guarantee. The charge is gated by newStatus == accepted plus the early return on already-accepted devices, so a key is spent at most once per device lifecycle. The merge path charges once against the enrolling device's key.
• Cross-repo clean. cloud/ consumes this service via replace and reimplements none of these functions; it references InstallKeyIncrementUsage/InstallKeyDecrementUsage only in generated mocks, so no cloud build break or duplicate charge.
• Tests match the real paths, including the exhausted case correctly omitting DeviceUpdate/NamespaceIncrementDeviceCount and InstallKeyEventStampDecision.

---

To request another review round, comment /review.
· fix/install-key-usage-manual-accept

[otavio]

Two axes reviewed against the repo rules and #7068. Core fix is right; these need addressing:

Blocking

• The operator still doesn't see the reason. ErrInstallKeyExhausted correctly reaches 403 with
  install key usage limit reached, but ui/apps/console/src/utils/acceptErrors.ts maps every 403 to
  "You do not have permission to accept devices in this namespace." The issue asked for "an error saying
  the key is out of uses" — right now it's a wrong error instead of a silent no-op.
• Dead reserve/release protocol. InstallKeyDecrementUsage has zero callers after this change, but the
  interface (server/api/store/install_key.go:62), the pg impl and the mock remain, and their doc comments
  still describe the reserve/release the diff deleted. Same staleness in InstallKeyIncrementUsage's doc
  and enrollment_e2e_test.go:402. Remove it or rewrite the docs.

Should fix

• chargeInstallKeyUse is unexported, so its comment isn't one of the two exceptions code-style.md allows.
  Its one non-obvious fact — the store's increment is the limit check — didn't reach the commit message;
  move it there and drop the comment.
• removed → accepted charges a second use for a device that was already charged. Only accepted is
  guarded, so that edge falls through.
• The no-backfill decision is argued in the PR body only. It belongs in the commit message, where
  git blame reaches it.

Test gaps

• The issue's own reproduction (manual key, usage_limit: 1, accept two) has no e2e test, though
  enrollment_e2e_test.go already has the installKey/usedTimes seam that would prove the counter —
  a better place for this than a mock test asserting the store call.
• Nothing covers rejected → accepted charging, or the pairing path not double-charging.

[gustavosbarreto]

Addressed what this PR made, pushed. Three findings I'm leaving out, with reasons.

Dead reserve/release protocol — right, I made it dead. InstallKeyDecrementUsage is gone: interface, pg impl and mock. Nothing outside this repo referenced it. One correction: InstallKeyIncrementUsage's doc is not stale. It says it increments atomically only while the key has usage left and returns ErrNoDocuments once overused, which is still exact and now carries more weight, since that predicate is the only limit check left. Reworded the two places that described where the reservation happened (enrollment_e2e_test.go:402 and its subtest name).

chargeInstallKeyUse comment — dropped. The non-obvious fact moved to the commit message.

No-backfill decision — moved into the commit message, where git blame reaches it.

Test gap on the issue's reproduction — agreed, and the e2e seam was the right call: TestEnrollmentE2E_AcceptSpendsAUse builds a manual key with usage_limit: 1, enrolls two devices, asserts the counter is still 0 while both wait, then accepts one (counter 1) and asserts the second accept fails with ErrInstallKeyExhausted, stays pending, and spends nothing. It also covers rejected → accepted spending a use, through an allowlist key that rejected the MAC. That replaces the mock cases that asserted the same thing indirectly; what stays as a mock test is the one negative with no observable side effect, a device with no install_key_id reaching accepted without any call to the store's increment.

403 copy in the console — out of scope here. The finding is real: acceptErrors.ts maps every 403 to the permission sentence, so the server's message never reaches the operator. But that mapping already mistranslates any other 403 from that endpoint, it predates this change, and it is a UI file in a server fix. It also cannot be fixed without deciding something this PR should not smuggle in: the error body carries only message, no machine-readable code, so the options are a distinct status for the exhausted case, a code in the error body, or keying UI copy off server prose. That is API shape and wants its own issue. If you want it, open one.

removed → accepted charging twice — not introduced here. On master, re-registration of a removed device already went through applyEnrollmentDecision (auth.go:380) and incremented again. And under the semantics #7068 states, used_times means admissions granted and removal does not return the use, so charging a re-admission is correct rather than an unguarded edge. Wanting "devices the key currently holds" is a different model and a different change.

Pairing not double-charging — covered behaviour this PR does not alter. TestEnrollmentE2E_UsageLimitUnderConcurrency already pins the counter against a limit on the automatic path.

[gustavosbarreto]

/review

[github-actions]

Claude finished @gustavosbarreto's task in 3m 17s —— View job

---

Code Review Complete

Reviewed 9 files across code quality, security, testing, language patterns, and architecture — no new issues found. The code looks good as-is.

A few notes from the analysis, for the record (nothing blocking):

• Single charge point, correctly gated. InstallKeyIncrementUsage now has exactly one production caller — chargeInstallKeyUse inside the newStatus == accepted branch of updateDeviceStatus. The early return on already-accepted devices means a key is spent at most once per non-accepted → accepted transition; re-accepting cannot re-charge.
• Transactional rollback holds. The charge runs inside the WithTransaction closure on the tx-bound connection, before DeviceUpdate/NamespaceIncrementDeviceCount, so a later failure rolls the increment back — which is what made the reserve/release pair safe to delete.
• Exhaustion is fail-closed and reaches 403. The store increment (WHERE usage_limit = 0 OR used_times < usage_limit) returns ErrNoDocuments → ErrInstallKeyExhausted (ErrCodeLimit → HTTP 403). errors.Is matches correctly through the pkg/errors Is method, and the error propagates unwrapped through WithTransaction and UpdateDeviceStatus.
• Cross-repo clean. cloud/ satisfies the store interface by embedding this repo's Store and reimplements none of these functions. It references the removed InstallKeyDecrementUsage only in its generated mock — an extra method still satisfies the interface, so no build break or double-charge; a mockery regen would prune it.
• Tests tie to the fixed behavior. TestEnrollmentE2E_AcceptSpendsAUse proves usage is spent at accept (not registration), that the second accept against a usage_limit: 1 manual key fails with ErrInstallKeyExhausted and stays pending (exercising the rollback), and that rejected → accepted spends a use. The keyless unit test asserts no store increment when there's no install_key_id.

---

To request another review round, comment /review.
· fix/install-key-usage-manual-accept