← Commit history

Add RESTRUCTURE-PLAN.md: measured architectural assessment + staged migration plan. Data: 12,485-line server.js with a 3,601-line HTTP handler holding all 58 verbs inline, 525 catch blocks (failure suppression, not robustness), 98 uncoordinated timers, 19 ad-hoc createCDPSession sites with NO pipeline, and duplicate subsystems (6 park + 6 badge + 6 recovery + 4 identity functions) which is the measured cause of the fixed-it-in-one-spot-missed-another failures. Concludes AGAINST a Rust rewrite: every failure this week was structural (stale cache key, two competing code paths, window resolved by title not handle, un-detached CDP session) and would reproduce in any language, while the file holds hard-won correct knowledge (reparent survival, FedCM click sequence, park geometry, occlusion behaviour) a rewrite would risk. Proposes extracting core/ (cdp pipeline, sessions, identity, placement, taskbar, recovery, respond) + verbs/ modules behind the unchanged verb contract, in 7 independently-shippable steps, with each mapped to the specific open wiki issues it resolves.

John Lauer ·3189cf3c3c ·3mo ago ·parent 75e0501
1 file changed +170
RESTRUCTURE-PLAN.mdadded+170
@@ -0,0 +1,170 @@+# pup bridge — restructuring plan++**Written 2026-07-31, against v1.9.223.** Commissioned by John after a day where a single badge bug+took ten shipped revisions and four wrong theories to fix, and after an audit of 20 open wiki issues+found several describing the same structural causes.++This is a plan to rebuild the **interior** of the bridge. It is explicitly **not** a rewrite and+**not** a language change. See "Why not Rust" below.++---++## 1. What the measurements actually say++| Metric | Value | What it means |+|---|---|---|+| `server.js` | 12,485 lines | one file holds nearly everything |+| the HTTP request handler | **3,601 lines** | 29% of the file is ONE function containing 58 inline verb cases |+| `try`/`catch` blocks | 525 | one every 24 lines — *failure suppression*, not robustness |+| timers | 89 `setTimeout` + 9 `setInterval` | 98 uncoordinated schedulers racing on shared state |+| `createCDPSession` sites | 19 | **no CDP pipeline** — every call site rolls its own lifecycle |+| `page.evaluate` sites | 44 | same |+| park/placement functions | 6 distinct | same concept, six implementations |+| badge/overlay functions | 6 distinct | same |+| recovery functions | 6 distinct | same |+| hwnd/identity resolvers | 4 distinct | same |+| `sessionId` refs vs thread refs | **760 vs 24** | the announced migration never happened |++### The three failure modes these produce++1. **"Fixed in one place, still broken in another."** Proven, not theoretical: there were **two**+   separate no-favicon code paths. A fix landed in one, shipped, was declared done — and the user+   still saw the bug, because the *other* path threw first. This happened three times in one day.+   With 6 badge functions and 6 placement functions, this is the default outcome, not bad luck.++2. **Silent success.** 525 catch blocks, many of which swallow an error and let the caller report+   `ok: true`. The taskbar badge "painted successfully" for hours while painting nothing, because the+   paint target did not exist. A verb that cannot fail cannot be debugged.++3. **Unmediated CDP.** With 19 independent CDP sessions and no queue, one heavy page's console flood+   saturated the shared socket and froze every session on that Chrome. One site leaked a debugger+   session per tab for the life of the window. Nothing was arbitrating.++---++## 2. Why not Rust (or any rewrite)++The instinct is understandable but the evidence does not support it. Every failure this week was+**structural**, not runtime: a cache key missing a version, two competing code paths, a window+resolved by title instead of handle, an un-detached CDP session. Rust reproduces all four.++Meanwhile Node/puppeteer is the most mature CDP client available, and this file contains a great deal+of hard-won, *correct* knowledge that a rewrite would put at risk:++- Chrome reparenting so windows survive a bridge restart+- the FedCM account-chooser real-cursor click sequence+- park geometry, DPI scaling, and the standard inset frame+- occlusion behaviour and the throttle/highFps battery tradeoff+- Windows AUMID identity and its destroy/recreate side effect++**The knowledge is the asset. The structure around it is the liability.** Extract the former, replace+the latter, keep the verb contract stable so no user notices.++---++## 3. Target structure++```+src/+  server.js              # ~200 lines: http server + router + graceful start/stop ONLY+  core/+    cdp.js               # THE CDP PIPELINE — pooled sessions, serialized queue, backpressure+    sessions.js          # session/browser registries + lifecycle (single owner of shared state)+    identity.js          # ONE resolver: session -> OS window handle (never by title)+    placement.js         # ONE park/reveal/heal implementation+    taskbar.js           # ONE badge/overlay/identity implementation+    recovery.js          # ONE recovery path (reconnect, rescan, reclaim, relaunch)+    respond.js           # response envelope + hints + the _degraded/_cleanupReport contract+  verbs/+    window.js            # open/close/switch/list/maximize/raise/lower/focus/alert+    tab.js               # open_tab/close_tab/switch_tab/list_tabs+    navigate.js          # navigate/reload/back/forward/wait/fetch_url+    input.js             # click/hover/type/press_key/scroll/input_dispatch/assisted_click+    read.js              # eval/screenshot*/errors/render_html/set_viewport+    record.js            # browser_record_* + desktop_record_* + recorder window+    admin.js             # readiness/prewarm/status/describe/configure/use/deps/login/rescan+  chrome.js              # unchanged (Chrome-for-Testing self-heal)+  credential_vault.js    # unchanged+```++**Rule the structure enforces:** a verb file may only touch `core/`. It may never talk to CDP, the+OS, or the taskbar directly. That is what makes "did I fix it everywhere?" answerable by reading one+file instead of grepping six.++---++## 4. The CDP pipeline (the piece that does not exist today)++This is the single highest-value change. `core/cdp.js` owns:++- **One session per page**, created lazily, cached, and **always detached** on page close.+  (Today: 19 ad-hoc creations, at least one historic leak.)+- **A serialized command queue per target.** Commands are queued, not fired concurrently, so a slow+  or wedged page cannot starve siblings sharing the socket.+- **Mandatory timeouts.** No `await cdp.send()` without a deadline. A hung command fails its own+  request instead of the whole bridge.+- **Backpressure + event throttling.** Console/network event subscriptions are rate-limited at the+  source, not by suppressing the *log* after the flood has already crossed the socket.+- **Health probes** feeding the existing crash/hang self-heal, instead of each caller inventing one.++Every `createCDPSession`, `cdp.send`, and `page.evaluate` in the codebase routes through it.++---++## 5. Migration order (pup stays shippable throughout)++Each step is independently releasable and independently revertable. No big-bang cutover.++| Step | Work | Why this order | Risk |+|---|---|---|---|+| **0** | Freeze: characterization tests for all 58 verbs (request → response shape). The existing `pup-bridge-test` scenarios become the regression suite. | Nothing can be safely moved without a contract to check against | none |+| **1** | Extract `core/respond.js` + `core/sessions.js` | Pure moves, no behaviour change, gives every later step a home for shared state | low |+| **2** | Build `core/cdp.js`; migrate the 19 CDP sites to it | Highest value: kills the freeze class of bugs at the source | medium |+| **3** | Collapse the duplicates into `core/identity.js`, `placement.js`, `taskbar.js`, `recovery.js` — **6+6+6+4 functions → 4 modules** | This is the "fixed it in one spot but missed another" fix | medium |+| **4** | Split the 3,601-line handler into `verbs/*.js` behind a thin router | Mechanical once core exists; the handler becomes a dispatch table | low |+| **5** | Audit the 525 catches: every one either handles meaningfully or is removed. Nothing returns success on a swallowed error. | Restores debuggability | medium |+| **6** | Consolidate the 98 timers into a single named scheduler with one tick loop | Kills timer races on shared state | low |+| **7** | Finish `sessionId` → thread identity, or formally abandon it. 760/24 is the worst of both worlds. | Removes the deprecated split | medium |++**Shippability rule:** after every step, run the full verb suite plus the standing torture test, and+release. If a step cannot ship, it is too big — split it.++---++## 6. What this fixes, mapped to real issues++| Issue | Structural cause | Fixed by step |+|---|---|---|+| #16 windows unresponsive under load | unmediated CDP, no queue/backpressure | 2 |+| #26/#27 session wedges on a heavy page | same | 2 |+| #15 respawn breakage | (AD-side too) but deps/startup logic is buried in a 12k file | 1, 4 |+| #22 ownership advisory + cold-start race | session lifecycle has no single owner | 1, 7 |+| #5 raise-by-title fragility | 4 competing identity resolvers | 3 |+| #24 caller identity on only 14/43 callbacks | no central AD-call path to enforce it | 1, 2 |+| badge/overlay saga (10 revisions) | 6 badge functions, 2 no-favicon paths, silent catches | 3, 5 |+| PDF windows unpaintable/unparkable | identity by title, not handle | 3 |++---++## 7. Honest costs and risks++- **This is weeks, not days.** Steps 2 and 3 are the substantive ones.+- **Step 2 is the riskiest** — it touches every driving path. It must land behind the full test suite+  and the torture scenario, and it should ship alone, not bundled.+- **Regression risk is real.** Every one of the 525 catches is potentially load-bearing in a way+  nobody documented. Step 5 must be incremental with tests, never a bulk delete.+- **Do not do this while firefighting.** Interleaving a restructure with live bug reports is how the+  two-competing-paths bug got created in the first place.++## 8. The discipline that has to come with it++Structure alone will not save this. The recurring process failures this week were:++1. **Shipping a fix without reproducing the bug first.** Ten revisions, four wrong theories. The fix+   came from one measurement (dump the artifact, look at it) that should have been step one.+2. **Trusting an API's success report over the user's eyes.** "Badge painted" while nothing painted.+3. **Not reading the open issues.** Two of the hardest problems were already diagnosed in the tracker,+   one of them three weeks earlier by the user himself.++These are already recorded as rules in `dev-skills/pup-bridge-dev`. They belong in this plan because a+clean architecture built with the same habits will rot the same way.