RS-23152: Stop the donut label collision resolver spinning forever at the 0 degree seam

[chschan]

RS-23152

The bug

Applying a filter to a published dashboard froze the tab with repeated "Page
unresponsive" dialogs, and it never recovered. Chris Facer profiled the live
document and found ~36s of continuous main thread work inside this widget's
DescendingOrderCollisionResolver, with findAllActiveCollisionsWithLesserLabels
(the counter clockwise sweep's collision query) among the hottest frames.

Root cause

The resolver walks a colliding label around the label ellipse one angleIncrement
(0.5 degrees) at a time, normalising back into [0, 360) after each step:

const boundedAngle = (angle) => (angle < 0) ? 360 - angle : angle % 360

For a negative angle this reflects rather than wraps: -0.5 returns 360.5
instead of 359.5. That makes counter clockwise stepping across the seam a closed
two-cycle:

normaliseAngle(0     - 0.5) -> 360.5
normaliseAngle(360.5 - 0.5) -> 0

A label pushed counter clockwise past 0 therefore makes no angular progress, and
the sweep's while loop can never satisfy its exit condition. Nothing else bounds
that loop, so the tab locks up. It is a genuine infinite loop, not merely slow
work — which matches "even after clicking Wait, I couldn't get things to ever
update".

Instrumented trace of the largest label (segmentAngleMidpoint = 3.0):

first 12 angles:   3.0 2.5 2.0 1.5 1.0 0.5 0.0 360.5 3.0 2.5 2.0 1.5
angles 1000-1011:  0.0 360.5 0.0 360.5 0.0 360.5 0.0 360.5 0.0 360.5 0.0 360.5

This needs a label to actually reach the seam, which is why it reproduced on the
customer's document under one particular filter but not on a copy.

The fix

1. Wrap the angle correctly

Replaced boundedAngle with a new math.normaliseAngle, which wraps
correctly in both directions (((angle % 360) + 360) % 360). It lives in math.js
alongside the other angle helpers rather than staying private to the resolver.

Measured with a headless harness driving the resolver directly:

case	before	after
60 equal segments	>5,000,000 placements, never terminates	1,270 placements, ~11ms
90 / 100 / 150 / 200 segments	never terminates	~2,100–2,400 placements, 10–16ms

2. Bound the counter clockwise sweep to one revolution

Raised in review by @JustinCCYap, and verified: wrapping the angle correctly stops
the sweep cycling on the spot, but it does not bound it.
labelLineAngleExceededTooFarCounterClockWise only fires while
labelAngle < label.segmentAngleMidpoint, so once a label has wrapped past 0 that
guard is inert for the rest of the revolution and every revolution after it.
Termination then rests entirely on a collision free, in bounds slot existing
somewhere on the lap, and when none does the label walks laps forever — a second,
independent unbounded loop reachable at stock default settings.

So the CC descent is now capped at one revolution per label, with exhaustion routed
into the same resetLabel / recordHitMaxAngle(CC) path as the max angle case.
movesRemaining === 0 is a safe exhaustion test: after 720 steps of 0.5 degrees the
label is back at the angle it started from, which is known to be colliding, so there
is no valid position to misclassify. Routing it through recordHitMaxAngle also lets
keepSweeping() exit via lastTwoFrontiersAreSame() rather than grinding out all 18
sweeps, which it could not do before because CC never recorded a max angle hit for a
seam crossing label.

Four geometries that computePieLayoutDimensions derives at or near shipped defaults
walk laps without this cap and terminate with it — including 600x600 at the default
labels.max.width of 0.3:

canvas	labels.max.width	segments	before	after
600x600	0.4	40	never terminates	32,607 placements, 39/40 labels kept
600x600	0.35	70	never terminates	9,931 placements, 67/70 kept
600x600	0.3 (default)	40	never terminates	34,833 placements, 39/40 kept
800x400	0.4	70	never terminates	7,116 placements, 59/70 kept

Rendering impact

Small and targeted — placement only differs where a label actually crosses the
seam. Across 97 chart shapes exercised locally, 95 produce byte identical label
placement, one previously hung, and one (54 equal segments) resolves differently
because that is the case the bug was corrupting. I'd expect very few visual
baselines to move; CI will show exactly which.

The revolution cap adds nothing to that. Every layout that already terminated does so
in exactly the same number of label movements with the cap in place (1,987 / 3,805 /
2,914 / 4,633 / 7,373 / 435 / 6,783 / 5,984 placements, identical before and after),
and all ten pre-existing spec tests pass with the cap reverted. It only ever changes
behaviour where the alternative was not terminating.

Tests

• math.jest.test.js — normaliseAngle unit tests, including a case asserting
  that counter clockwise stepping across the seam always makes progress. This is
  the one that pins the root cause.
• DescendingOrderCollisionResolver.jest.test.js (new) — drives the resolver
  headlessly against a stub canvas, with a label movement budget so a regression
  fails the run instead of hanging it. The 60/90/100 segment cases all hang without
  the seam fix, and the four canvas geometries in bounds the counter clockwise sweep to one revolution per label all hang without the revolution cap (checked by
  reverting each fix in turn: the intended cases fail and no others do).

Also adds babel.config.js so jest can transform the import statements that most
of theSrc/scripts uses. Without it none of this code was unit testable — the one
existing spec test passed only because math.js happens to be CommonJS. esbuild
builds the bundle and does not read babel config, so this affects spec tests only.

npm run lint clean, 29/29 spec tests pass.

Left alone deliberately

The counter clockwise max-line-angle guard is still not symmetric with its
clockwise counterpart — the seam clause is commented out with a "not sure why but I
cannot make this symmetric" TODO. A label that legitimately crosses the seam can
therefore exceed labelMaxLineAngle by a few degrees. That is cosmetic and bounded,
and it is no longer a hang risk now that the sweep is capped at one revolution, so
fixing the asymmetry properly is still worth raising separately.

Per Chris's note on the ticket, ngviz-mono's ngviz-pie carries a TypeScript port
of this same resolver and likely has the same defect — worth checking there too.

🤖 Generated with Claude Code

---

Reopened from #82, which was merged into rhtmlBuildUtils-9 prematurely and has since been unpicked by a force-push — GitHub cannot reopen a merged PR, hence a new one. Now rebased onto master (c873aec) since #81 landed as a squash merge, and retargeted; the three original commits are unchanged in content.

[JustinCCYap]

The root cause analysis and the normaliseAngle fix are correct — I verified the seam two-cycle and that the new helper wraps correctly in both directions. One thing worth flagging before this is read as closing out the hang class: the counter clockwise sweep can still spin forever, at stock default settings.

This is not a regression from this PR — the same configurations hang on rhtmlBuildUtils-9 too. But they hang there via the seam two-cycle, and with this fix applied they hang by walking laps instead, so the "never terminates" rows in the benchmark table above aren't fully settled yet.

Why the CC loop is still unbounded

The CC while (DescendingOrderCollisionResolver.js:283) has two escapes: no collisions with lesser labels and in bounds, or labelLineAngleExceededTooFarCounterClockWise. That guard is unreachable for exactly the labels this PR is about, because it requires label.labelAngle < label.segmentAngleMidpoint:

1. Before the seam, for a label with segmentAngleMidpoint = 4.5, the guard's window is only [0, 4.5), where labelLineAngle is a few degrees at most — far below labelMaxLineAngle of 80, so it never fires.
2. After the wrap, labelAngle is 359.x, which is not < 4.5, so the guard is inert for the rest of the revolution and every revolution after it.

So termination now depends entirely on a collision-free, in-bounds slot existing somewhere on the lap. canvas.labelIsInBounds is a pure box test (SegmentLabeller.js:279), so when no such slot exists the label just keeps going.

Secondary effect of the same inert guard: recordHitMaxAngle(CC) is never called for a seam-crossing label, so keepSweeping() (line 134) keeps returning true until sweepCount > maxSweeps, forcing all 18 sweeps rather than exiting via lastTwoFrontiersAreSame().

Reproduction at default settings

A 300x300 donut, 40 equal-valued segments, every setting at its shipped default (labels.max.width = 0.3, labels.max.lines = 6, descending sort order, outer labels). The geometry computePieLayoutDimensions derives for that is outerRadius = 52, labelOffset = 6, maxLabelWidth = 90.

On this branch: 3,000,000 label movements without terminating. Label 0 (segmentAngleMidpoint = 4.5) walks counter clockwise and never stops — monotonically decreasing, no increasing steps anywhere in the trace:

first 10 angles:    4.5 4.0 3.5 3.0 2.5 2.0 1.5 1.0 0.5 0.0
angles 1500-1509:   210.5 210.0 209.5 209.0 208.5 208.0 207.5 207.0 206.5 206.0

Swept canvas size against labels.max.width and segment count, driving the resolver headlessly with the geometry the real widget would produce, budget 200,000 moves. Configurations that do not terminate:

canvas	labels.max.width	segments
300x300	0.25 (below default)	50
300x300	0.3 (default)	40
400x400	0.25 (below default)	70
400x400	0.3 (default)	70
400x400	0.35	40
600x600	0.35	70
600x600	0.4	40
800x400	0.4	70

Two qualifications. At 600x600 the defaults are safe across 20-100 segments, so the risk concentrates in small tiles rather than full-size charts. And the harness gives every label the maximum wrapped width with equal segment values, which is the worst case for a given size and segment count — a real chart needs uniformly longish category names to hit these thresholds.

Suggested fix

Cap the CC sweep at one revolution per label and treat exhaustion the same way the max-angle case is treated. Next to angleIncrement:

const maxMovesPerLabel = Math.ceil(360 / angleIncrement)

Then in the CC loop:

let movesRemaining = maxMovesPerLabel
while (
  (wrappedLabelSet.findAllActiveCollisionsWithLesserLabels(label).length > 0 || !this.canvas.labelIsInBounds(label)) &&
  !labelLineAngleExceededTooFarCounterClockWise(label) &&
  movesRemaining > 0
) {
  ...
  movesRemaining--
}

const exhaustedRevolution = movesRemaining === 0
if (labelLineAngleExceededTooFarCounterClockWise(label) || exhaustedRevolution) {
  wrappedLabelSet.resetLabel(label)
  recordHitMaxAngle(CC)
}

movesRemaining === 0 is a safe exhaustion test: after 720 steps of 0.5 degrees the label is back at its starting angle, which is known to be colliding, so there is no valid-position case to misclassify. Routing exhaustion into recordHitMaxAngle(CC) also fixes the secondary defect above.

Measured with that applied:

1. The 300x300 / 40 segment case terminates in 31,371 moves, keeping 39 of 40 labels.
2. Re-running the whole sweep above with it applied: no configuration hangs, and the slowest terminates in 32,607 moves.
3. Cases that already terminated are unchanged move for move (347, 1,407, 2,349, 7,763 across the sweep), so no visual baselines should move.
4. All 29 unit tests still pass.

Happy for this to be a follow-up ticket rather than a change to this PR, given it is pre-existing — but if it goes that way, it is worth a note here so the benchmark table is not read as covering it.

Smaller points

1. DescendingOrderCollisionResolver.jest.test.js:145 — the test named "never moves a single label more than once around the ellipse" does not enforce that. perLabelBudget = 720 * 20 allows 20 revolutions, the counter is keyed on this.id so it accumulates across every extraHeight clone, and the only assertion is Object.keys(movesPerLabel).length > 0, which passes even if nothing interesting happened. A regression that walks a label 15 laps per attempt stays green. Either rename it to what it enforces, or count moves per label per attempt against 720.
2. DescendingOrderCollisionResolver.jest.test.js:128 — the comment "The same freeze, reached with an uneven distribution rather than a uniform one" sits above it.each cases that are all Array(n).fill(10), i.e. uniform; the uneven case is the next test at line 135. The 60 entry also duplicates the test at line 122 exactly.
3. babel.config.js — @babel/preset-env is not in this repo's devDependencies; it resolves only because rhtmlBuildUtils depends on it and npm hoists it to the root node_modules. If that hoisting changes, rhtml testSpecs fails with "Cannot find package" rather than a test failure. Worth declaring @babel/preset-env and babel-jest directly.
4. babel.config.js is added at the repo root but not to .Rbuildignore, unlike its peers there (eslint.config.js, package.json, package-lock.json). Both files are hand-maintained rather than generated, so it needs adding by hand. Otherwise it ships in the R source package and trips a "Non-standard file/directory found at top level" NOTE in R CMD check.

[chschan]

Verified the structural argument and it holds. labelLineAngleExceededTooFarCounterClockWise requires label.labelAngle < label.segmentAngleMidpoint, so once a label has wrapped past 0 it is inert for that revolution and every one after it, and canvas.labelIsInBounds (SegmentLabeller.js:279) is a pure box test with nothing else bounding the loop. Confirmed the secondary effect too — recordHitMaxAngle(CC) never fires for a seam crossing label, so keepSweeping() grinds out all 18 sweeps instead of exiting via lastTwoFrontiersAreSame().

Taken into this PR rather than a follow up: the PR is titled "stop it spinning forever", so shipping it while it still hangs at a default labels.max.width would leave the headline claim untrue. Applied as you wrote it, in ab2312f.

Independent reproduction

Drove the resolver headlessly with the geometry computePieLayoutDimensions derives. Your derivation for 300x300 reproduces exactly — maxLabelWidth 90, outerRadius 52, labelOffset 6, maxVerticalOffset 98.

One difference worth recording: my per-configuration table is not the same as yours, and 300x300 / 40 segments at default max width terminated for me in 3,805 moves rather than hanging. The band is narrow and sensitive to label text length, which differs between our two harnesses — with Category N labels I could not make anything hang, and only with uniformly longish names (~35 chars) did the hangs appear. So I would treat the specific table as harness dependent, but the class is confirmed. Configurations that hang for me:

canvas	labels.max.width	segments	derived geometry
600x600	0.4	40	r=52, offset=6, maxLabelWidth=240
600x600	0.35	70	r=79, offset=9, maxLabelWidth=210
600x600	0.3 (default)	40	r=107, offset=11, maxLabelWidth=180
800x400	0.4	70	r=70, offset=8, maxLabelWidth=320

Note the third: 600x600 at the shipped default labels.max.width does hang, so I could not reproduce "at 600x600 the defaults are safe across 20-100 segments".

Two of your measurements reproduce to the digit, which is good corroboration that the harnesses agree where the geometry coincides: 600x600 / 0.4 / 40 terminates in 32,607 moves keeping 39 of 40 labels.

Non regression

All four hanging cases terminate with the cap. Every case that already terminated is unchanged move for move — 1987, 3805, 2914, 4633, 7373, 435, 6783 and 5984 placements, identical before and after — so no visual baselines are expected to move. All ten pre-existing spec tests also pass with the cap reverted, which is the same statement from the other direction.

The four geometries are now it.each cases in DescendingOrderCollisionResolver.jest.test.js. Checked they earn their keep: with the cap reverted all four fail on the movement budget and the other ten pass.

Smaller points

1. Took the second option rather than renaming — the budget is now an actual revolution (720) instead of 720 * 20, and it stays cumulative across extraHeight attempts, which makes it stricter than a per attempt bound rather than looser. That layout's busiest label settles in 107 moves, so the margin is wide and the name is now literally true. Assertions changed from Object.keys(...).length > 0 to pinning the label count at 60 and the observed maximum against the budget. (Per attempt against 720 would not have been a valid bound in any case — a label can legitimately be moved across several sweeps within one attempt.)
2. Fixed. Comment moved down to the long tail test it actually describes, and the duplicate 60 entry dropped from the it.each.
3. Fixed. Declared @babel/preset-env, babel-jest, and the @babel/core that babel-jest peer depends on (^7.8.0), which was also undeclared. Lock churn is three lines.
4. Fixed. Added babel.config.js next to eslint.config.js in .Rbuildignore.

One you did not raise

The PR was changing theSrc only. inst/htmlwidgets/rhtmlDonut.js still carried the reflecting boundedAngle (mu=t=>t<0?360-t:t%360), so neither fix would have reached R users — and DESCRIPTION had lost its bump in the rebase onto master, since master had already reached 1.0.14. Rebuilt the bundle and bumped to 1.0.15 in ed52e01, matching what #80 and #81 both did.

npm run lint clean, 32/32 spec tests pass. Visual baselines unverified locally — leaving that to CI, though the move for move evidence above says nothing should move.

[JustinCCYap]

LGTM