Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions TIME_WASTERS.md
Original file line number Diff line number Diff line change
Expand Up @@ -182,3 +182,9 @@ Signature → cause → what to do. One lesson per line, no incident history.
- **A CI waiter that greps for "pending" fires on an output gap.** `until ! gh pr checks <n> 2>/dev/null | grep -q pending` reads "the word is absent" as "the run finished", so one transient gh failure ends the wait exactly like completion — and the `2>/dev/null` hides the error that would have shown it. `gh pr checks` also exits 8 while checks are still queued, so a waiter keyed on its status reports failure while its own banner says complete. Require all three: gh exited 0, output non-empty, and every check terminal via `[.statusCheckRollup[] | (.status // .state // "UNKNOWN")]` rejecting `QUEUED|IN_PROGRESS|PENDING|WAITING|REQUESTED|UNKNOWN`. That still only sees *registered* checks, so a partial list reads as complete — assert the expected set by name; a cranpose PR carries seven (Android release build, architecture budgets, fmt + tests + clippy, iOS build, robot e2e, robot external captures, wasm build). Require *pass*, not "nothing failed" — a skipped or cancelled check is not green. Then check what the merge actually did rather than printing success after calling it: a `gh pr merge` refused for a conflict still returns to the next line, and its exit status must be read directly, never through a pipe (same trap as the `| tail` entry above). Finish by confirming the squash commit is an ancestor of `origin/main` with the expected subject — `state == MERGED` can be set by auto-merge, a merge queue or an admin action, so it never asserts that *your* merge is what landed.
- **Verify the verifier: a guard you have not watched reject a bad input is not a guard.** Every hardening step is itself a predicate nobody tested, and the test is the easy thing to get silently wrong. `timeout` does not exist on macOS, so `timeout 8 ./wait.sh && echo FIRED || echo held` prints "held" from `command not found` — a green result for a script that never ran, and AGENTS.md sends people to `macm3` over ssh where a `timeout`-based test copied from Linux fails exactly this way. Feed the guard the states it exists to catch and watch each one: for a CI gate that is empty rollup, partial-but-all-green, one QUEUED, one FAILURE, one CANCELLED, one SKIPPED, an unexpected extra check, and only then all-green. The partial case is not hypothetical — a PR was observed going "no checks reported" to 4 registered to 7, so a gate that trusts the registered list can pass before the slowest job is created.
- **An app's own background work is measured as framework frame cost.** Scroll on a 2018-class Android device read as 15-17 fps with `present` p50 29-33 ms; the app was in fact still running on-device inference, settled at 442% CPU before a finger touched the screen. Re-measured on the same scene with the app quiet at 0-3% CPU: 22-28 fps, `present` p50 13.5-17 ms — the headline numbers were inflated about 1.5x. Note what did *not* move: the ranked causes filed alongside them — render passes, isolated layers, and a 35% layer-cache hit rate re-measured at 34.6% with 1.78 MP of blurred offscreen per frame — reproduced on the quiet device, two of them worse than filed. Contamination inflates the headline figures you quote in the title; it does not necessarily touch the per-frame counters underneath, so re-verify rather than retracting wholesale. Before trusting any on-device frame number, poll `adb shell top` for the package until its CPU settles and print what it settled at; a run that does not report its settled CPU is not a measurement, and the same applies to any app doing background indexing or sync. Never A/B across two package names either — separate installs carry separate app data and settings, so the arms differ in more than the build (theirs disagreed on `vsync_period_ms`, 15.841 vs 16.632, which is how the contamination surfaced). Install both arms over the same package, or flip them inside one binary with a `debug.cranpose.*` property override as `segment_surface` and `pipeline_disk_cache` already do. `idle_iters` tells you whether a telemetry window was a real scroll window or mostly idle.
- **Carry an untouched stage as a within-run control before believing a perf regression.** On a Kirin 980, patched `update` p50 measured 4.63-7.53 ms against 4.38-4.57 unpatched — the patched *minimum* above the unpatched *maximum*, on precisely the stage the change touched, which reads as a ~2 ms regression and nearly blocked its own merge. Carrying `render`, which the change did not touch, as a control gave ratios of 0.89/0.90 unpatched and 0.88/0.95/1.00/1.04 patched: flat. The governor moves every CPU stage together between sessions, so a same-direction shift in an untouched stage means you measured the device, not the patch. Second tell that two arms are not in the same state: `vsync_period_ms` disagreeing between them (15.841 vs 16.638). Detail on #504.
- **A knowledge PR has a convergence budget, and good material is not sufficient reason to spend it.** #510 took nine pushes and sixteen cancelled runs over two and a half hours; `heavy (self-hosted)` never completed once, because each new entry superseded the run validating the last. It converged on the first cycle after the branch was frozen. The check rollup for the current head shows only that head's checks and says nothing about the runs already killed — `gh run list --branch <b>` does. When entries keep arriving, freeze and open a follow-up.
- **A cache key can name the right thing at the wrong granularity, and the obvious fix ships a stale frame.** `LayerRasterCacheKey::backdrop_effect` (raster_cache.rs) takes `local_bounds: Rect` and stores its x/y/w/h in `local_bounds_bits`, but `backdrop_effect_cache_key` (render_paths.rs) passes a screen-space `visible_rect` into that slot. Every scrolled frame moves y, so the key changes and every lookup misses — the 34.6% hit rate and 1.78 MP of blurred offscreen re-rendered per frame in #500. Do **not** make the key translation-invariant: a backdrop caches what is *behind* the card, so when the card moves that region is genuinely different pixels and an invariant key serves a stale backdrop — subtle enough on a slow scroll to ship. The key is right about what it identifies; the granularity is what needs work.
- **Closing a PR does not cancel its CI, and on a constrained pool that cost falls on everyone else.** A PR closed at 16:00Z had a run *start* at 19:00Z on a machine the whole board was queued behind; deleting the branch does not stop it either. Both robot jobs require `[self-hosted, Linux, cranpose-heavy]` and only samarch-1's two runners match — the Macs carry the label but are macOS — so with ~19 robot jobs queued, one dead run holds roughly half the throughput. Cancel the runs when you close or supersede a PR, and note that a cancel *request* is not a cancel: a self-hosted job keeps reporting `in_progress` after accepting one, so read the status back. Worth evaluating when the board is quiet: a concurrency group keyed on the ref with `cancel-in-progress`.
- **`gh pr merge` can exit non-zero on a merge that succeeded.** Observed on #510: exit 1, yet the PR read `MERGED`, main's head was the squash commit, and `git merge-base --is-ancestor` confirmed it. Trusting the exit status alone reports failure on success, exactly as trusting `state == MERGED` alone reports success on someone else's merge. Only the ancestry check settles what landed. `--delete-branch` also did not delete the branch; that needed a separate push.
- **Test the accept path: a predicate that only ever holds is indistinguishable from a working one until the day it needs to fire.** A CI gate ran `gh pr view <n> --json statusCheckRollup --jq -c '<filter>'`, but `--jq` takes exactly one argument, so `-c` became the filter and the real filter a second positional — `accepts at most 1 arg(s), received 2`. gh exited non-zero and the rollup was empty on every poll for hours, across two arms. It had been tested against seven synthetic *bad* inputs and held on all of them, which is why it was trusted; the one untested input was a genuinely green rollup, the only one that ever had to pass. Corollary on reporting: when that gate first ended with an empty rollup it was described as the guard correctly holding — a bug narrated as the system working as designed. Watch for that shape in your own summaries.