ποΈ PUT-1758: cap workspaces per user and seats per workspace - #3725
Conversation
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
70fc391 to
7edf3f1
Compare
7edf3f1 to
0b1e545
Compare
0b1e545 to
9f7867c
Compare
9f7867c to
80ce2ba
Compare
80ce2ba to
a8d6cd7
Compare
a8d6cd7 to
1795adc
Compare
Local validationEngine: mysql 8 Β· caps read from the real "teams_enabled": true,
"max_workspaces_per_user": 1,
"max_seats_per_workspace": 3Workspace cap β the second create by the same owner: Note the cap is checked before handle validation, so an owner at their limit Seat cap β the fourth seat in a workspace limited to 3: The owner is not counted against the seat cap β 3 seats plus the owner is 4 Both limits are surfaced with a machine-readable |
1795adc to
a76fec1
Compare
a76fec1 to
9652fa8
Compare
9652fa8 to
95b9d04
Compare
95b9d04 to
b21ca6c
Compare
b21ca6c to
94c7dd7
Compare
94c7dd7 to
5403557
Compare
5403557 to
012ee1d
Compare
Review fix: the caps did not hold under concurrencyYou measured 5 workspaces under a cap of 1. Confirmed β both caps were count-then-insert with nothing serializing them, so concurrent requests each read a count below the limit and all inserted. Reproduced as a test first, with the lock removed: Five and six β your number exactly. The fix
Not a unique constraint: the limits are configurable ( After: Both new tests fail with the lock removed and nothing else does. β Worth your eye: this serializes per owner and per workspace within one Redis. Two regions with separate Redis instances would still race. That matches the existing posture for Full suite 7883 passed | 26 skipped, typecheck clean, all 9 commits in the stack typecheck individually. |
|
Rebased onto One collision worth naming: those commits took migration Verified per commit rather than at the tip β each of the 9 typechecks on its own, so no intermediate commit is broken. Full suite 7883 passed | 26 skipped. |
Salazareo
left a comment
There was a problem hiding this comment.
one question but might be non issue
| export const DISABLED_BY_WORKSPACE = 'disabled_by_workspace'; | ||
|
|
||
| /** Cap-lock bounds; past the retry budget a 409 beats waiting. */ | ||
| const CAP_LOCK_ATTEMPTS = 20; |
There was a problem hiding this comment.
not sure if attempts too high? this might hold connection for up to a second like this
012ee1d to
c02fb25
Compare
|
Follow-up on the cap lock: waiting out the budget is no longer a refusal. The first version threw A flaky test is the cheap version of the real symptom: a legitimate single create refused because something unrelated held the key. So the timeout now proceeds unserialized, exactly as the Redis-unreachable path already did: // Waiting out the budget is not a refusal: the cap is a bound, and
// a spurious 409 on a lone create is worse than a rare overshoot.
if (!held) return run();The cap check still runs in both cases β what is lost is only the serialization of the count-then-insert window, which is the same thing that is lost when Redis is down. Consistent with what this PR already says: the cap is a bound, not a correctness invariant. Both concurrency tests still pass and still falsify: with the lock removed they measure 5 workspaces under a cap of 1 and 6 seats under 3. |
c02fb25 to
d4d25f4
Compare
Terminology: "workspace" is now "team", everywhereThe feature said Applied to every commit rather than added on top, so each PR shows its own code already named correctly and no intermediate commit references a name a later one changes. All 13 commits still typecheck individually. The contract-bearing renamesThese were free to change now and expensive after release, which is why they went in this pass:
Plus Scoped, and checked"workspace" means other things in this repo. These were deliberately left alone, and verified untouched:
Audited three ways: every changed line mentions team or workspace (no collateral edits), no stutters or mangled words ( One thing not renamedThe git branch names still say |
d4d25f4 to
eee777c
Compare
You are right, and it was worse than just holding: the first version also threw 409 when the budget ran out, so unrelated contention could refuse a legitimate create. That already showed up as a flaky Two changes: The budget is now 200ms, not 1s β Running out is no longer a refusal. It proceeds unserialized, exactly as the Redis-unreachable path already did: // Waiting out the budget is not a refusal: the cap is a bound, and
// a spurious 409 on a lone create is worse than a rare overshoot.
if (!held) return run();So the worst case is now 200ms of waiting and then the unserialized path β never a held connection for a second, and never a spurious error. Both concurrency tests still pass and still falsify: with the lock removed they measure 5 teams under a cap of 1 and 6 seats under 3. |
eee777c to
66d76a4
Compare
A seat is a real Puter account: it takes a name from the global username pool and gets a home directory. Nothing charges for one β that is prod's job β so until it does, the only bound on creation is the request rate limit, which bounds the rate and not the total. max_teams_per_user default 1 max_seats_per_team default 50 Ordering is the substance of both checks. The team cap is tested before the handle, so a capped user is told they are capped rather than that the name they picked was unusable. The seat cap is tested before any account state exists, so a refused provision does not burn a global username. Soft-deleted teams do not count toward the owner's cap, so deleting frees the slot β which does mean create, provision, delete, repeat still consumes usernames over time, bounded by the daily rate limit. The caps raise the cost and make the cycle audited; they do not close it. Lowering the seat limit blocks new provisioning and disables nobody. Both limits are published in rate-limits-and-quotas.md, and both keys are documented in config.template.jsonc and config.default.json. Closes PUT-1758.
66d76a4 to
f870975
Compare
Bounds on how much a workspace can create. This is what has to exist before
teams_enabledcan go on, even internally.Why
A seat is a real Puter account. It takes a name from the global username pool, gets a home directory, and is stamped with a storage allowance from the plan. Nothing charges for one β the payment integration is out of this repo and does not exist yet β so until it lands the only thing bounding creation is the request rate limit, and that bounds the rate, not the total: 500 mutations/day is 500 accounts/day, every day.
Two limits, both from config so a deployment can move them without shipping code:
Ordering is the substance of both checks
The workspace cap is checked before the handle. Otherwise a user at their limit who also picks a taken handle is told the handle was the problem, and goes off to pick another one. There is a test that a capped user asking for a syntactically invalid handle still hears
workspace_limit_reached.The seat cap is checked before any account state is created.
provisionAccountcreates a user row, then a filesystem tree, then the membership. Checking late would mean a refused provision had already consumed a global username. The test asserts the refused name is still free afterwards.β What these caps do not do
Soft-deleted workspaces do not count toward the owner's cap, so deleting frees the slot β which is the behaviour you want, and also means create β provision 50 β delete β repeat still works. Each cycle is deliberate and audited, and the daily rate limit bounds the total, but the caps raise the cost rather than closing the hole. Billing closes it.
Worth stating because the deleted accounts are not recovered:
softDeletenulls the workspace handle, but the accounts it created still exist, still hold their files, and keep their usernames permanently. Deleting a workspace is not a way to stop paying for the accounts in it β the same point PUT-1740's confirmation dialog has to make.Lowering a limit never disables anyone
A workspace already above a reduced seat limit keeps every account it has and is simply refused new ones. There is a test that drops the cap below the current seat count and asserts every existing seat is still unsuspended β being over a limit is not a reason to suspend people.
Defaults, and where they actually come from
The code carries
?? 1and?? 50fallbacks, butconfig.default.jsonis the merge base and now sets both keys, so the code fallback never runs in a normal deployment. That is fine, but it means a mutation to the fallback changes nothing observable β I found this when falsifying it and the suite stayed green. There are now two tests that reach it: one deleting the key at runtime, one setting a nonsensical0(which must not lock everyone out).Also
checkOwnerInvariantwas running its own inlineCOUNT(*)in the service whileTeamStore.countPayersexisted and had never been called. Dead store method plus SQL in a service β it now calls the store.Verification
The five existing team suites now pass
max_workspaces_per_user: 100explicitly. They create several workspaces per master and started failing the moment the cap landed β which is the cap working, but they are not testing it, so they say so in a comment rather than being quietly reshaped around it.Per-site falsification
workspace cap defaultinitially reported0 failedβ not because the default was untested but because it is unreachable behindconfig.default.json. The two fallback tests above were written in response.countSeats excludes masteris worth its own note: the master isorg_owned = 0, so counting all memberships instead of seats would spend one of the 50 on the person paying for them.Not here
Per-workspace seat limits β "Acme gets 200, everyone else 50" β are genuinely per-workspace state and really mean "the master bought 200 seats", which is billing. When that lands it is an additive
group.seat_limitcolumn, nullable,NULLfalling back to the config default, with no rework of this.Closes PUT-1758.