[Fix] Enforce project quota shrink against real usage#210
Merged
Conversation
The project quota shrink guard summed VM and cluster usage from resources.metadata and resources.machine_pool_size, but nothing on the create path ever writes those columns, so usage always computed to zero and a project quota could be shrunk below what its running resources actually consume. Derive usage from data the create path really persists: the size label on VM rows and each cluster's node-pool rows (size x count), resolved through the models.Sizes catalog passed into the query as unnest arrays. Bastions keep their fixed 2 CPU / 4 GB footprint and storage stays 0 until a volumes table exists. The covering test previously masked the bug by seeding metadata with a raw INSERT; it now creates resources through Repository.Create so it fails if the create path stops writing the column the guard reads. The capacity suite now also runs in the cluster-free CI job, and a test-name pattern that matches no test was dropped from its filter.
📝 WalkthroughWalkthroughThis PR changes how project capacity usage is calculated: ChangesCapacity usage recalculation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test as ProjectsCapTest
participant API as UpdateProjectQuota
participant Calc as sumProjectResourceUsageTx
participant DB as Database
Test->>API: PATCH project CPU quota to 10
API->>Calc: compute in-use CPU/memory
Calc->>DB: query size_specs CTE joined with VM resources
Calc->>DB: query size_specs CTE joined with cluster_node_pools
DB-->>Calc: aggregated usage (26 CPU, 52 GB)
Calc-->>API: TenantCapUsage
API-->>Test: 400 quota_below_usage with in_use totals
Metadata
🐰 A catalog of sizes, both small and grand, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dc-api/internal/db/projects.go (1)
347-354: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a named constant for the bastion 2 CPU / 4 GB footprint.
The fixed bastion allocation is duplicated as magic numbers here and restated in the doc comment (Lines 291, 348-349). Extracting a small constant (or a
models.Sizes["bastion"]-style entry) would keep the two in sync if this value ever changes.♻️ Suggested refactor
+// bastionCPU/bastionMemGB is the fixed footprint counted per bastion resource. +const bastionCPU = 2 +const bastionMemGB = 4 + bast AS ( - SELECT COUNT(*) * 2 AS cpu, - COUNT(*) * 4 AS mem + SELECT COUNT(*) * $5 AS cpu, + COUNT(*) * $6 AS mem FROM resources r WHERE r.project_uuid = $1 AND r.status IN ('PENDING', 'ACTIVE') AND r.type = 'BASTION' )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dc-api/internal/db/projects.go` around lines 347 - 354, The bastion resource footprint is hardcoded as magic numbers in the allocation query, so it can drift from the documented value. Update the logic in the project sizing query around the bastion aggregation to use a named constant or shared size entry (for example a dedicated bastion size in the existing sizing model) instead of repeating 2 CPU / 4 GB inline. Make sure the same source is used by the related doc comment and any helper that computes project sizes so the bastion allocation stays consistent if it changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@dc-api/internal/db/projects.go`:
- Around line 347-354: The bastion resource footprint is hardcoded as magic
numbers in the allocation query, so it can drift from the documented value.
Update the logic in the project sizing query around the bastion aggregation to
use a named constant or shared size entry (for example a dedicated bastion size
in the existing sizing model) instead of repeating 2 CPU / 4 GB inline. Make
sure the same source is used by the related doc comment and any helper that
computes project sizes so the bastion allocation stays consistent if it changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31c41562-5cb0-4b38-a465-aa1fee03576e
📒 Files selected for processing (4)
.github/workflows/authz.yamldc-api/internal/db/projects.godc-api/test/integration/nop_env_test.godc-api/test/integration/projects_cap_test.go
gnudeep
approved these changes
Jul 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
The project-quota shrink guard computed a project's in-use CPU/memory by reading
resources.metadataandresources.machine_pool_size— columns that nothing on the create path ever writes. Every VM and cluster therefore summed to zero usage, and a project owner could shrink a project's quota below what its running resources actually consume without the guard firing. Quota enforcement is a stated core principle of the API, so this was a real correctness bug rather than a cosmetic one.The fix
sumProjectResourceUsageTxnow derives usage from data the create path really persists: each VM'ssizelabel and each cluster's node-pool rows (size × count, all roles), resolved through themodels.Sizescatalog, which is passed into the query asunnestarrays. Bastions keep their fixed 2 CPU / 4 GB footprint, and storage stays 0 until a volumes table exists (pre-existing TODO).Why it's safe
It is a single query rewrite inside the existing
UpdateProjectQuotatransaction — no schema change, no API change, no handler change. Because size labels and node-pool rows have always been persisted, the fix is retroactively correct for every existing row; no backfill is needed. A row with an unknown size label contributes 0, same as before.Tests
The covering test previously masked the bug: it seeded
metadatawith a raw SQL INSERT, so it passed while the production path wrote nothing. It now creates the VM and cluster throughRepository.Create(the production insert) plus realcluster_node_poolsrows, so it fails if the create path ever stops writing the column the guard reads. TheTestProjectCap_suite (5 tests) now also runs in the cluster-free authz CI job on every PR. A-runpattern in that job that matches no existing test was dropped, along with its comment references.Verified
go build ./...,go vet ./..., the full unit suite, and the nop-mode integration run (real Postgres via testcontainers) are green: 5/5TestProjectCap_PASS including the de-masked shrink-below-usage rejection (asserts bothin_use.cpu_cores = 26andin_use.memory_gb = 52from an xlarge VM + small×1 and medium×2 pools). Self-reviewed: diff-scoped scanners (golangci-lint, govulncheck, gitleaks, actionlint) + multi-lens adversarial review; the adversarial pass found no defects in the query (multi-tenancy isolation, NULL/empty-project handling, and the CI filter's full match set were each verified by execution).Not included
Storage usage still sums to 0 (needs the volumes table, pre-existing TODO). Databases and key vaults are not yet counted — their instance classes have no cpu/memory catalog to resolve against; that follow-up is now documented in the function comment. A node-pool row whose own status is failed still counts toward usage (conservative over-count; pool rows can't distinguish "never provisioned" from "running but a later update failed"). The unused
resources.metadata/machine_pool_sizecolumns are left in place — the migrator is additive-only.Summary by CodeRabbit
Bug Fixes
Tests