Repository navigation
fix(server): hand out boards that own their containers, gate it with -race - #241
Merged
Merged
Conversation
kvaps
approved these changes
Sep 19, 2026
Member
There was a problem hiding this comment.
This fixes the cache races, including the two that came from my own changes: the People map read off the lock while the Load frame is built (now detached(sub.view(e.board)) in the fan-out) and fanoutCost. The clone is guarded by type-graph tests and mutation-tested, and CI runs -race now, which is the job that would have caught this in the first place.
Named-and-left items (reloadFromTip, asOfCache.get) are honest and out of scope here.
LGTM.
kvaps
force-pushed
the
feat/openapi-generated-server
branch
from
September 19, 2026 09:03
e5f77e5 to
902ebef
Compare
The fan-out builds and sends its frames with the board lock released, so that the forge lookups inside cannot freeze every read and write of the board. It records what the run cost after that, and the mutation paths read that same field while holding the lock. Two fan-outs landing together, or one landing beside a card write, race on it. Make the field atomic rather than putting it under the lock. Taking the lock back to record a duration would fold the wait for it into the duration being recorded, and that number paces the coalescing window every open board's freshness rides on: a flush queued behind a full reload would stretch the window for everyone, having measured the queue rather than the work. An atomic keeps the recording where the frames were built, off the lock, which is where the two comments above those call sites say the work belongs. Two fan-outs landing together can lose one of the two updates, because the read and the write are separate. That is the right trade for a number clamped at both ends that exists to pace a timer. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
The cache hands its board out by value, and a board.Board copy still points at the same card array, the same roster slices and the same maps. Every mutation path edits those in place under the board lock, so a request serialising its snapshot, or a fan-out building a frame, reads them under no lock at all. For the slices that is a torn multi-word field. For the maps Go answers with an unrecoverable throw rather than a bad value, and one of them is reachable: Visible rebuilds every container it projects except People, SetPersonCapacity writes that map in place, and MembersOf reads it while the frame is built. A scoped visitor reaches it too, so this is not confined to the case where a visitor may read everything. Clone every container reachable through a copy, wherever a board leaves the cache. One level is not enough — a card copy still aliases its own notes, and EditNote rewrites a note's body through that alias. install splits in two rather than cloning for everyone. The sync tick and the background revalidate install and walk away, so they get a version that hands nothing back; the full load takes its board out of the same critical section that installed it, because reading it back afterwards would let an invalidation land in between and answer with an empty board and no error. Two guards, because the clone is only complete while its assumptions hold. The first fails by name when a slice or a map is added to Board or Card and not cloned, and reaches the cards through whichever Board fields carry them rather than through a written-down pair. The second pins what the first rests on: that no struct below those two carries a container, that nothing anywhere carries a pointer or an interface, and that no container on Board or Card holds containers of its own, since the clone goes one level and a copy shares whatever sits below that. An unexported container is named too, because no clone outside the board package can reach one at all — the useful thing there is to say so rather than to compensate quietly. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
The board cache is read by HTTP handlers and the watch fan-out while the git sync writes it, and an uninstrumented build cannot see that class of bug at all. The suite takes roughly half again as long under it. make test runs the same command, so the claim that CI's checks pass locally stays reproducible by one, and the single-test recipe carries the flag for the same reason. The carry-over scaling test needs a looser bound to survive that, and the reason is the shape of its assertion rather than the detector. It budgets slack*lone + 250ms against a measured many. many reproduces across machines; lone does not, differing by more than twice between two of them while staying stable on each. So the budget scales with the quantity that varies and bounds the one that does not, and at a slack of 3 identical code passes on one machine and fails on another. Ten absorbs that. The regression it guards is a view rebuilt per card per board, two orders of magnitude, far outside the range either machine produced. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <f@lex.la>
kvaps
force-pushed
the
fix/board-cache-data-races
branch
from
September 19, 2026 09:06
badc9df to
79d76fd
Compare
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
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.
go test -race ./internal/server/is red and has been for a while. CI runsgo test ./...with no-race, so nothing ever saw it. Five runs on the merge base report 19 to 24 races each; the detector only reports what it witnesses, so the union across runs is the population and no single run is. Two causes behind all of them.The first one can take the process down. The cache hands its board out by value, and a
board.Boardcopy still points at the same card array, the same roster slices and the same maps. Every write path edits those in place undere.mu, so a request serialising its snapshot, or a fan-out building a frame, reads them with no lock at all. For the slices that is a torn field. For the maps Go throws and the process is gone. That path is live:Visiblerebuilds every container it projects exceptPeople,SetPersonCapacitywrites that map in place, andMembersOfreads it while the Load frame is built. Scoped visitors go throughVisibletoo, so this is not only the case where someone may read everything.The fix clones every container reachable through a copy, at every place a board leaves the cache. One level is not enough: a card copy still aliases its own notes, and
EditNotewrites a note body through that alias.The assumption the clone rests on was checked by someone other than the author, in both directions. Outward: no caller in
pkg/boardservice,pkg/apiserverorinternal/serverwrites through a board it received fromLoadBoard, andSetSprintStatealready re-reads the live cache rather than its snapshot. Inward, which is the half nobody had tested: no caller can install a slice it still owns back into the cache, which would mirror the same aliasing from the other side.cardFromInputallocatesAssigneesfresh,CreateInput.Assigneeis a string,touchedtakes its card fromLoadCards, and the only in-place element writes in the package are the two on the cache side. Had that been false, cloning each card's notes would have bought nothing.That clone is not free and the cost is per read: around 663KB on a thousand cards and 3.3MB on five thousand, measured twice from different directions. In time it is invisible where it matters and visible where it does not. The twelve-tab phase of the plain suite runs 26.8ms here against 28.5ms at the merge base, which is noise in the wrong direction; under the detector the same phase is around ten percent slower, and that is the one number here that has reproduced on a second machine: 831ms against 742ms measured on one, 834ms against 762ms on another, with the samples in each group not overlapping either time. The clone copies every card and the detector instruments every one of those writes, so the only place the copy shows up is the mode that is already slower for that reason.
What it buys is a board no reader can corrupt and no reader can see torn, which is the trade this change is making on purpose. Copy-on-write is the exit if the allocation ever matters more than the simplicity: writers replace the slice instead of editing it, reads cost nothing, and every mutation site has to be found. That is a much larger change and a much easier one to get silently wrong, so it is not this one.
Two whole-board clones of garbage on a cold read, where
cached()copies and the stale path drops it, are known and not taken here. Removing them needs a state-only accessor and a second lock acquisition on a read path, and this change has twice already paid for that trade: a lock that re-imported the contention it was added to avoid, and a void return that opened a gap where a full load could answer with an empty board. A third locking change to save an allocation is how the previous two defects arrived.Guards hold it there. One fails by name when a slice or a map is added to
BoardorCardand not cloned. The other pins what that one rests on: that no struct below those two carries a container, so stopping at one level is enough, and that nothing anywhere carries a pointer, since a copy shares its pointee and the clone follows none. A slice onEpicColor a pointer onCardwould otherwise slip past both.NoteandSprintStatecannot gain a slice at all, because both are compared with==and a struct holding one is not comparable. That is redundancy rather than a hole: remove the comparisons, add the slices, and the guard names both.Second cause is
fanoutCost, written with the lock released by the flushes that send frames and read under it everywhere else. It came in with the fan-out's move off the lock, v0.40.1.CI runs
-racenow, and so doesmake test, so "CI's checks pass locally" is reproducible by one command rather than by none. Both pay for it: the whole repo goes from about 21s to about 37s here, and your localmake testis the half you will feel.How to check it
Whole repo. Expect
okfor every package and no line matchingWARNING: DATA RACE, on the first attempt. A race count is schedule-dependent, so a clean run that took retries to get is not a clean run, and an average over runs says nothing about any single one.One test is loosened here, and the measurements say the old bound was not tight but already flaky.
TestACarryOverDoesNotCostMoreForEveryOpenBoardasserts a budget,many > slack*lone + 250ms, not the ratio its log line prints. Three runs per row, quiet machine, full package, which is the mode CI runs:-race-raceThe second row says the bound was already failing one run in three on a tree carrying none of these fixes, so raising it silences no symptom of mine. But the reason is not the detector and not this change, and it took a second machine to see it.
Measured elsewhere, under
-race,loneis 356 to 385ms where it is 165 to 168ms here, stable on each machine and more than twice apart between them.manyagrees across both, 807 to 859ms here against 765 to 1018ms there. So the budget at a slack of 3 is 748ms on this machine and 1354ms on that one, against the same measuredmanyof roughly 830ms, and identical code fails here and passes there.That is the actual defect in the assertion: it scales its budget by
lone, which varies with the machine, and boundsmany, which does not. In plain mode the 250ms constant is 96 percent of the budget and swamps the difference. Under the detectorlonegrows until the multiplier dominates, and the test starts deciding on whose hardware it ran. A test nobody runs in the job that matters is worse than a looser one, so it is loosened rather than skipped. But the bound is a workaround, and the assertion wants rewriting against something that does not move between machines.Do not reason from the printed ratio. It is dominated by how fast
lonehappens to be, andlonehas been seen at 10ms and at 370ms on the same tree on the same day whilemanystayed near 28ms. Three separate readers have produced three different true ratios for this assertion out of that instability.Keeping the old bound for plain mode behind a build tag was considered and dropped. Nothing automated runs plain mode now, so the tighter number protects nothing observable, and the split costs two files, two build tags, and a test file the linter cannot see.
A returning race fails the step rather than only printing it. Both mutations below came back red and non-zero, which is the point of the CI change: it gates, it does not report.
Because the detector makes the suite slower and more contended, and a runner is busier than a laptop, I ran the package five times under
-racewith the machine deliberately loaded: load sampled during the runs at 38 to 47 on twelve cores, no failures, no races, nothing unrun. That is a pre-flight and nothing more. The machine whose behaviour actually matters is the runner, and it measures itself on this branch the moment the job runs.Both fixes are mutation-tested rather than only asserted. Neutering
detachedwhile leaving its test in place failsTestDetachedSharesNoContainerWithTheCachenaming every container it checks, and brings back 9 races ininternal/server, all of them board containers. Removing the locking aroundnoteFanoutbrings back 14 races ininternal/server, all of themfanoutDelayagainstnoteFanout, and no board-container race, so neither fix is hiding the other. Surgical mutation rather than reverting the commits, because a test that fails to compile and a test that caught the bug look the same in the output.What this does not cover
One unsynchronised read is left in the tree, and turning the detector on is what will eventually find it.
reloadFromTippassese.board.BoardintoLoadBoardand takese.muon the line after; its four siblings take the lock for the identical read, so it is one missed site rather than the local convention. It predates this branch and nothing here touches it. No test interleaves it, which is the only reason-raceis green here: unwitnessed rather than absent. Worth saying plainly, because the first time CI goes red on it that will look like the new job being flaky, and it will not be.The same shape survives one cache over, and is left alone here.
asOfCache.getreturns its stored board by value, so the copy still points at the cache's own slices and maps. It is latent rather than live: every read path I traced builds fresh containers or sorts a slice it allocated, so nothing mutates what it got. It becomes real the first time one does, and the remedy is the same one line on the way out. Out of scope for a change already this size, and not something a reader should have to infer from its absence.The guards themselves have a limit worth naming, because it is the reason they were wrong four times before they were right. They read the type graph and ask what it declares; every hole found in them was a place where what the clone does diverged from what the declaration implies. The current shape closes that class by flagging any
Cardreachable other than through the two fields the clone descends, but those two names are written by hand in the guard and again in the clone, and nothing compares the two lists. A check that observed the clone's behaviour instead of reading types could not miss the class at all. That is a larger change than this one and it belongs after it.Not established:
install()is one of the escape sites and is fixed on the source argument, but no test witnesses it. The torn read is mechanism only, nothing crashed and no card was seen under the wrong domain in any run here. And two causes is what every run I could arrange names, which is not proof there is no third.