FedWiki's site usage is a running total that many code paths adjust; derive it instead #165
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: wiki-cafe/member-console#165
Reference in New Issue
Block a user
The problem
core.numeric_entitlement_usagefor thefedwiki_siteskey holds one number per pool, shared by every workspace of the organization the pool belongs to. It is the figure the create form compares against the limit. Code ininternal/integrations/fedwikikeeps that number as a running total: eleven call sites raise or lower it withAtomicIncrementUsageandAtomicDecrementUsage(workflows/activities.go,workflows/reconcile.go,workflows/swap.go,workflows/sync.go), andusage/usage.goadds three more that raise it to, or bound it between, a count taken from the site rows (ReconcileWorkspace,BoundWorkspace). The paths cover create, delete, archive, restore, keep active, swap, the downgrade sweep and the re-upgrade, the farm sync and the repair that runs at boot.Each path runs in its own transaction, and some key the counter by workspace while others key it by pool. When two paths overlap, or one keys by the wrong grain, the total ends above or below the number of active sites the pool really has. Above, a member is refused a site they are entitled to. Below, a pool exceeds its limit.
The entitlements model page also says the counter has one mutator, the consumer that increments it. Nothing in the code holds the FedWiki paths to that: the boot repair, the sync and the swap each lower or overwrite the figure.
Why patching does not converge
Each review round of this code found a pair of paths that disagreed and fixed that pair. The next round found another pair. The fixes are correct one at a time, but every new status change in FedWiki adds a path that has to be reconciled by hand with each existing one, and the repair paths (
ReconcileWorkspace,BoundWorkspace) exist because the counter drifts. A total that is adjusted from many places cannot be shown correct by reading any one of them.Proposed direction
Compute the figure instead of maintaining it. Every transaction that changes a site's status first takes a lock on the pool's usage row, then writes the pool's absolute usage, counted from the site rows plus the lifecycle requests still in flight (a create in flight holds a slot). No path increments or decrements; the number written is always a count taken under the lock.
This matches how the integration contract already treats an application's usage, which an application reports as an absolute figure and never as an increment. It also fits the planned lease model for keys the console enforces itself, where usage is committed units plus live leases, still a count.
Open points for the discussion: whether the in-flight requests are read from the workflow tables or recorded as their own rows; whether the same rule is wanted for the other numeric keys; what happens to
RaiseNumericUsageTo,BoundNumericUsageand the boot repair once nothing drifts; and the wording of the model page's statement about who may change the counter.Related: #164 (what an unlimited limit is) changes how the limit side of the same comparison is read; #127 touches how several rules for one key combine into that limit.
Done when
fedwiki_sitesusage of a pool, and every FedWiki path that changes a site's status calls it under the pool lock.git grep AtomicIncrementUsage AtomicDecrementUsagefinds no call ininternal/integrations/fedwiki.Order: comes after #166 and before #171, the first part of the integration contract. #172 turns this counter into the absolute usage an application reports, so deriving it here is also the first step toward that.
Three more things the rewrite touches. Tests pay for the running total too:
assertCounterMatchesRowsand the counter writes in the sites walkthrough go once the figure is derived. The farm sync reads open requests once, without a lock, and deletes farm-absent rows only in the System workspace, so its count and the derived one have to agree on both. And creation should end as one function, with the JSON and HTML handlers only mapping its answer.This starts once the static-analysis gate and the test kit land (the first two steps of #166), and runs alongside the rest of that refactor, which touches no FedWiki code. It removes four of the complexity exceptions the gate lists: the three FedWiki functions it rewrites (
CreateSite,swapActiveSite,SyncSitesToDBActivity) andTestNumericUsageReconcile, which tests the two counter queries (RaiseNumericUsageTo,BoundNumericUsage) whose future the rewrite decides. It also adds the usage-row lock to the written lock order.The order has changed: this now starts once all of #166 has landed, steps 3 to 6 included, instead of alongside those steps. The integration contract (#171 to #174) still follows it.
The reason is the four workflow types that stay registered only for runs the previous release started (
CreateFedWikiSiteWorkflow,DeleteFedWikiSiteWorkflow,SetSiteStatusWorkflow,SwapActiveSiteWorkflow). Their activities still adjust the running total, so the rewrite removes them, and removing them is safe on a deployment only once it runs a release that no longer starts them and has no run of them open. Starting after the refactor gives deployments time to get there, and the refactor does not wait on it. If such a run could still be open when this starts, the rewrite adds a check that refuses to start the console while one is, rather than keeping the types.The FedWiki tests now build their fixtures through
internal/integrations/fedwiki/fedwikitest, so the tests for this change start from one place.Env.Counted(name, limit, used)gives a tenant's pool afedwiki_siteslimit and a usage row readingused, andEnv.LimitandEnv.Usageset the two halves on their own. A test sets the figure directly, which is how it reaches usage above the limit, a figure that disagrees with the active sites, or a limit no rule governs. Those are the drifted states a derived figure has to correct, and the concurrent test under "Done when" can start from them.The swap tests' fixture,
swapRequests, records the activation and then the park under one workflow id, in the order the swap handler uses, so those tests read request rows shaped like the handler's. Splitting the swap and sync tests into named subtests left every outcome as it was.Two tests still insert FedWiki rows by hand:
TestAcceptedCreateQueries, and part ofTestSyncSkipsAFarmSiteAnOpenCreateHolds.Before the rewrite, the FedWiki paths should be read the way the plan, billing and operator pages were read during #166: every read and what the page shows when it fails, every list's sort order, and every lock with the order it is taken in. #166 left
CreateSite,swapActiveSiteandSyncSitesToDBActivityto this issue, so nobody has read them,DeleteSiteor the custom-domain handlers that way. The instances known so far, so that the rewrite does not carry them over:DeleteSiteanswers a repeat from a second read outside the instance lock (internal/integrations/fedwiki/web/api.go:600-607). Described on #174.swapActiveSitetakes its two instance locks in the order activated site, then parked site (web/site_actions.go:606-622), not in instance order, which is safe only under the workspace lock. Described on #184.DeleteUnstartedLifecycleRequestin the shared integration layer,internal/integration/lifecycle.go:445, which FedWiki'sstartRequestWorkflowcalls atweb/requests.go:240), so a repeat can be answered as a conflict. Filed as #229.renderCustomDomainStatus,web/customdomain.go:407-411). Filed as #230.CheckQuotaActivity,workflows/activities.go:602-614). Filed as #231.store/queries/sites.sql:150-177); #225 covers the rule and these lists.FedWiki's
fedwiki_sitesusage is now a count, not a running total. A pool's usage row holds its active site rows plus the slots its creates and activations in flight hold, and every write counts under the usage row's own lock: the lock and the count are separate statements at READ COMMITTED, so each count sees every change committed before the lock was granted.How each part of "Done when" is met:
usage.Recount(internal/integrations/fedwiki/usage/usage.go) locks a pool's usage row, counts, and writes the count withSetNumericUsage; the slot gate (usage.Hold) takes the same lock, compares the count with the limit before it stamps a hold, and stores the figure the same way. Every FedWiki path that changes a site's status calls one of them in the transaction that records the change: the create, status, swap and delete outcomes, the failure outcomes, settle, the downgrade sweep, the farm sync, and the recount of every pool at boot and at the end of each sync. The lock is the usage row's, taken after the site rows and before the request rows, not a pool lock;docs/database-locks.mdgives the order for each path.AtomicIncrementUsage,AtomicDecrementUsage,RaiseNumericUsageTo,BoundNumericUsageand the boot repair are deleted; no code names them, andgit grepfinds them only in the records of earlier changes underopenspec/changes/archive/.TestConcurrentLifecycleRunLeavesTheFigureOnTheRows_DBruns a create, a permanent delete, an archive, a restore, a swap and the downgrade sweep at once against one pool under aforce_reducelimit, with every path held at the farm until all have reached it, and then all six queued on the pool's usage row at once behind a transaction the test holds. After each path returns, and at the end, one read-only transaction finds the stored figure equal to the count; at the end it equals the active sites and no hold is left. It passed twenty runs in a row and a run under the race detector, and it fails on each of five deliberate breaks: a count taken before the lock, the recount as oneUPDATEwith a subquery (twice, once labelled as the lock statement), an outcome that keeps its hold, and a park that writes no recount.docs/models/entitlements.mdanddocs/models/resource-pools.mdnow state, among their invariants, that a metered key's usage row holds an absolute count its owner writes under the row's lock, and that consumers only read it.In-flight creates and activations are recorded as their own rows,
fedwiki.slot_holds, one per lifecycle request, stamped by the slot gate inside the request's workflow and removed by the transaction that records its outcome. The create and status workflows gate the new steps on a version marker, so runs recorded before it replay unchanged. A closed activation that still holds a slot at boot is read back from the farm before it is settled, instead of being failed while the farm may have activated the site.The four workflow types that predate lifecycle requests (
CreateFedWikiSiteWorkflow,DeleteFedWikiSiteWorkflow,SetSiteStatusWorkflow,SwapActiveSiteWorkflow) are removed. The console refuses to start while a run of one of them is open, naming the types and counts, rather than keeping the types.Every surface reads the stored figure: the sites card, the create form, Keep active's choice between an activation and a swap, and
GET /api/fedwiki/quotanow agree with the member's entitlements panel and with the slot gate.The farm sync writes a changed status under the site row's lock after checking again for a request on the site, and reads a domain again from the farm when its request changed after the list was taken, so it no longer writes the farm's earlier status over an outcome. Two older gaps in the same code were closed with it: a site permanently deleted during a pass is no longer inserted again for a tick, and a row a create records during a pass is no longer overwritten by the sync's insert.
Fixed along the way: #231 (a failed read of the sites entitlement declined the create as a full limit) and FedWiki's three keep-and-park lists from #225; the JSON create now checks the assembled name's length as the form does, and a form create whose repeat lookup fails is refused instead of going on as a first create.
What went elsewhere:
sameCreate, and the complexity exceptions of the create handler (CreateSite) and the swap handler (swapActiveSite) go to #174, which deletes those functions when FedWiki moves onto the integration contract. The JSON delete's repeat read outside the instance lock stays there too.sameRowRequestgoes to #184.scripts/deadcode-testonly.txtentries name it.Defects found next to this work and filed: #245, #246, #247, #248.