Skip to content

P0: main CI red — Deploy MCP failure on b846653 #3127

Description

@github-actions

P0: main went red

Workflow: Deploy MCP concluded failure on main.

Failing run: https://github.com/edobry/minsky/actions/runs/32204884767
HEAD SHA: b846653919efa9dcba7a5a90b096538c04568550
Head commit: perf(mt#4274): Take the peek's in-flight drag width out of React state

Summary

Dragging the peek's resize handle cost 11.82ms of JavaScript per pointermove against a 16.7ms frame budget, because the in-flight width lived in React state on PeekHost and re-rendered every pane's full detail body on every move. It now repaints through a CSS custom property and commits to state once on release: 0.60ms per move.

Reported by the principal hours after mt#4261 shipped: "its weirdly slow? doesnt feel v fluid/responsive and theres a perceptible delay in how long it takes for the content to reflow" — with the observation that the machine was not loaded, which the measurement confirms.

Root cause, measured before anything was designed

Over CDP against the running cockpit, 60 synthetic pointermoves at 1440x900 (Performance.getMetrics deltas):

ScriptDuration      + 0.709 s   ->  11.82 ms per move
LayoutDuration      + 0.073 s   ->   1.22 ms per move
RecalcStyleDuration + 0.018 s   ->   0.30 ms per move

Not reflow (layout is 1.22ms of 13.34ms) and not the machine. The only JS running per move was the resize path: PaneDivider fires onChange per pointermove, PeekHost passed setWidth, and that is React state on the host — so each move re-rendered PeekHost → every SheetSheetContentPeekBody → the full detail widget. Subtree size, same page:

{"elementsInPane":518,"elementsInBody":498,"elementsInDoc":1405}

96% of the re-rendered pane is the body, and grep -rn "React.memo|= memo(" over src/cockpit/web returns zero files.

Two hypotheses were falsified by the same measurement rather than argued away. localStorage was the leading suspect (savePaneWidth writes synchronously on every move) — timed in the same page, 200 setItem calls took 0.5ms total, ~0.0025ms each. And layout, above. Neither is touched by this PR.

Why the structural fix rather than React.memo

Memoizing PeekBody was the cheaper change and I reached for it first. React's own reference chose otherwise (https://react.dev/reference/react/memo, read 2026-08-19):

"You should only rely on memo as a performance optimization. If your code doesn't work without it, find the underlying problem and fix it first."

and, among the principles that make memoization unnecessary:

"Prefer local state and don't lift state up any further than necessary."

This defect is lifted transient state: the in-flight width of a drag is 60-120 values a second that exist only until the pointer comes up, and only the COMMITTED value needs to reach the host. Recorded because the vendor doc pointed away from the change I was about to make.

It also matches in-repo precedent — PaneDivider already writes document.body.style.cursor and userSelect imperatively for the duration of a drag.

Key changes

  • PaneDivider gains onCommit. onChange stays the high-frequency signal; onCommit fires once on release, and once per discrete keyboard step (a keystroke has no release to wait for). Optional, so SessionFilm is unaffected — its sized child is a virtualized ribbon, not a 498-element body.
  • PaneDivider keeps its own live value locally, so aria-valuenow tracks the drag while the host deliberately does not re-render. Local state on a leaf, which is the same React principle applied one level down.
  • PeekHost writes the live width to --peek-pane-width on its container through a ref — no setState — and commits through usePeekWidth on release. Panes size from var(--peek-pane-width, <committed>px).
  • usePeekWidth exposes previewWidth, the render clamp that path needs (distinct from setWidth's preference clamp).

The trade, stated rather than banked

per pointermove before after
Script 11.82 ms 0.60 ms
Layout 1.22 ms 1.38 ms
Style recalc 0.30 ms 2.10 ms
Total 13.34 ms 4.08 ms

A custom property is INHERITED, so writing it invalidates style for the subtree — that is where the recalc increase comes from, and it scales with the pane's descendant count. Taken deliberately: the total is ~24% of a frame and the blocking half is 20x cheaper. The obvious alternative — writing style.width on each pane node — avoids the inherited invalidation and reintroduces the problem the indirection solves: React would own width as a rendered prop AND the drag would overwrite it, and React diffs against its own previous value rather than the DOM, so a commit landing on the pre-drag number would silently leave the imperative value in place. Both facts are recorded in the code beside the constant.

A defect this change introduced, and the live script caught

Seeding lastReportedRef with the current width at pointerdown made a bare CLICK commit a preference. That broke the double-click reset — its two clicks each recorded a width before onReset could clear one. Fixed (seed null; commit only if a move happened) and pinned by a test.

scripts/verify-peek-resize.ts also now polls until two reads agree instead of sleeping a fixed 200-400ms. Its fixed sleeps reported two product regressions that were not there: a pane read at 421px on its way to 416px, and a viewport read taken before the resize listener had re-rendered. That is the exact hazard verify-peek-pane-layout.ts's readWhenStable docblock already warned about; this script shipped without the lesson and paid for it the same day.

Testing

Execution evidence:

$ bun test --preload ./tests/dom-setup.ts --preload ./tests/setup.ts \
    --timeout=15000 --path-ignore-patterns='services/**' src/cockpit/web
 2397 pass
 0 fail
 5067 expect() calls
Ran 2397 tests across 180 files. [18.19s]

SC1 — per-move ScriptDuration under 2ms (baseline 11.82ms). Measured by the same method, against a production build of this branch served at 127.0.0.1:3842:

60 pointermoves over 3609ms wall
  ScriptDuration      + 0.036 s
  LayoutDuration      + 0.083 s
  RecalcStyleDuration + 0.126 s
  LayoutCount         + 68
  RecalcStyleCount    + 146

per-move: script 0.60 ms | layout 1.38 ms | recalc 2.10 ms

LayoutCount + 68 is load-bearing: an earlier run of this measurement reported LayoutCount + 0 because a leftover preference had the pane pinned at its ceiling, so the drag produced no layout and the run measured nothing. A measurement of a drag that did not move is not a measurement.

SC2-SC5 — all 8 assertions of verify-peek-resize.ts pass unchanged, including pointer-conservation, the clamp, reset, persistence, and the held pair:

$ MINSKY_COCKPIT_URL=http://127.0.0.1:3842 bun scripts/verify-peek-resize.ts
default: pane=416px page=1024px divider=6x900 role=separator aria-controls="peek-pane-0" (resolves=true) range=[280,800] now=416
after drag -120px: panes=1 pane=536px page=904px stored=536
after reload: pane=536px stored=536
after double-click: pane=416px stored=null
after a full-width drag: pane=800px page=640px
at 620px with an 800px preference: pane=320px page=300px stored=800
held pair at 1440px: widths=[446,446] page=548px

PASS: the peek's handle is real, drags, persists, resets, and its clamp leaves the page readable.

SC3 (aria-valuenow stays live mid-drag) is additionally covered in the component suite — aria-valuenow tracks the drag while the host's value prop stays put asserts it moves while the host's value is frozen, which is the exact condition the fix creates.

SC5 (film host not regressed) — a host that passes no onCommit still works — SessionFilm's shape, plus the whole pre-existing "left"-default suite, and mt#4261's layout script on the same build:

$ MINSKY_COCKPIT_URL=http://127.0.0.1:3842 bun scripts/verify-peek-pane-layout.ts
wide (1440, desktop) (1440x900): pane=416px (29%) page=1024px gutters header=17 body=17 scrollers=1
narrow (620, the reported width) (620x900): pane=279px (45%) page=341px gutters header=17 body=17 scrollers=1

PASS: the peek pane has real gutters, one scrollport, and leaves the page readable.

Negative control — the click-commit defect:

The 8 new tests were written against the fixed code, so the one that matters is a click that never moves commits NOTHING. Its control is the live script, which is what found the defect in the first place: with lastReportedRef seeded at pointerdown, verify-peek-resize.ts reported after double-click: pane=421px stored=null and failed the reset assertion. With the seed removed it reports pane=416px stored=null and passes. Both runs are above and below in this body.

Negative control — the performance claim:

The 11.82ms baseline is the control, and it is stronger than a synthetic revert: it was measured on the real unfixed build serving from the running cockpit before any of this was written, not on a temporarily-reverted tree. Same script, same viewport, same 60-move drag, same metric.

Live verification

Both live scripts pass against a production build of this branch (bun run cockpit:build + cockpit start --port=3842); output above. The behaviour was additionally sampled directly across five states — default, full-width drag, shrink to 620, restore to 1440, double-click reset — and the property, the rendered box, the stored preference and aria-valuenow agree at every one.

Deploy verification: this changes src/cockpit/web/**, which the cockpit service bundles, so the post-merge deploy will be verified with deployment_wait-for-latest bound to the merge timestamp, asserting the health body's service: minsky-cockpit rather than the status code alone.

Scope

Out of scope, both recorded in the spec: the localStorage write frequency (measured innocent at 0.0025ms/write — changing it would be a fix on a falsified premise), and the film host's own drag path (it shares PaneDivider but sizes a virtualized ribbon; no report and no measurement implicates it).

Task: mt#4274

Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

What this means

A push to main triggered CI and the workflow above did not conclude success. Per
CLAUDE.md user preference ("main must never be broken"), this is severity-1.

Diagnostic checklist

  1. Open the failing run URL above; identify which job/step failed.
  2. Check whether the offending PR was merged with a known-failing required check
    (operator-API bypass via gh api PUT /merge despite enforce_admins).
  3. Confirm enforce_admins is currently enabled:
    gh api repos/edobry/minsky/branches/main/protection --jq .enforce_admins.enabled
    
    Expected: true post-mt#1938. If false, that is itself a separate finding.

Recovery

  1. Open a hotfix branch off current main.
  2. Apply the smallest fix that turns CI green (often a formatter pass or a config
    flip).
  3. Land via the standard Minsky session flow:
    tasks_create → session_start → session_commit → session_pr_create → /review-pr → session_pr_merge.
  4. Verify the post-merge main build is green within ~5 minutes.
  5. Close this issue with a link to the hotfix PR.

Cross-references

Metadata

Metadata

Assignees

No one assigned

    Labels

    main-redAuto-filed by .github/workflows/main-watch.yml when main CI fails (mt#1938)p0Severity 0: production breakage requiring immediate attention

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions