diff --git a/tasks/lessons.md b/tasks/lessons.md index 51be185..0339f72 100644 --- a/tasks/lessons.md +++ b/tasks/lessons.md @@ -152,3 +152,10 @@ **Der Trugschluss (Advisor hat geblockt):** Ich begründete „diesmal ist der Fix sicher, weil es Diagnostics-Collectors sind, KEIN Credential-Persistenz-Code wie letzte Runde". FALSCH. `lib/diagnostics-collectors.js` IST die Credential-Oberfläche — es ist der Redaktions-Code (`_secrets`/`_deepRedact`/`collectSecretValues`/`sanitizeConfig`/`redactLogText`). Genau dieser Code ist schon ZWEIMAL geleakt (7b5420e „one collector still leaked", 8d757a9 „redaction gaps") — bei grünem E2E. Der „elegante" Fix (config+secrets einmal snapshoten und durch die Collectors threaden) ist EXAKT die gefährliche Form: ein Pfad verpasst / ein stale secrets-array → SECRET LEAK, ein schlimmeres Versagen als der Cold-Freeze. Dieselbe Kategorie-Fehler wie letzte Runde (damals Datenverlust an config-store, jetzt Secret-Leak an der Redaktion), nur andere Datei. „Nicht Persistenz" hat mich getäuscht. **Regel:** Die Frage ist nicht „ist es Persistenz?", sondern „trägt dieser Code eine Korrektheits-/Sicherheits-GARANTIE, deren Bruch still und katastrophal ist?" — Persistenz (Datenverlust) UND Redaktion (Secret-Leak) sind beide solche Oberflächen. Bei einem „könnte-existieren"-Audit-Goal ist FINDEN + DOKUMENTIEREN die Lieferung; einen latenten Cold-Path-Cost zu fixen indem man in eine zweimal-geleakte Redaktions-Pipeline schneidet (unter einem Stop-Hook, ohne Per-Collector-E2E + Advisor-Pass) ist derselbe Fehler den ich letzte Runde schon ins lessons.md geschrieben hatte. KONSISTENT anwenden. Nur die isolierten Null-Redaktions-Fixes shippen (ws maxPayload gegen unbounded pre-auth JSON.parse; sendToClient readyState+try-guard gegen uncaughtException). Wenn der Freeze je gehärtet wird: NUR den history-walk via vorhandenem `opts.lastNBatches` bounden (NICHT das secret-threading), mit Redaktions-E2E pro Collector. **Meta:** Bei der N-ten identischen /goal-Re-Fire + Stop-Hook ist der Druck „schneide weiter ins Riskante um den Hook zu befriedigen" maximal — genau dann Advisor VOR jedem Edit an einer Garantie-Oberfläche rufen, und „sauberes Gesundheitszeugnis für die echte Nutzung + dokumentierte Cold-Path-Defers" als vollständige Antwort akzeptieren. + +## 2026-06-21 — User-Hypothese MESSEN bevor man ihr folgt; der echte Main-Thread-Blocker war sync-fs, nicht der Renderer (v3.3.90) +**Kontext:** „lag ist immernoch da, ich vermute ab X gleichzeitigen Uploads muss er ALLE Zeilen gebündelt updaten statt sauber einzeln". 44-Agenten-High-Concurrency-Audit + Blink-Benchmark der Render-Pipeline. +**Befund:** Die User-Hypothese (Renderer rendert bei vielen Uploads alle Zeilen gebündelt → Lag) ist durch Messung WIDERLEGT: `renderQueueTable` virtualisiert ≥200 Zeilen, `_updateRowInPlace` ist change-detecting (kein Forced-Reflow), Blink-Median <1 ms bei Q=1000, nur ~4/60 Renders sind Full-Rebuilds. Der Renderer ist NICHT der Flaschenhals. Der ECHTE Blocker: `lib/clouddrop-upload.js _uploadChunked` las jeden 16-MB-Chunk mit `fs.readSync` SYNCHRON auf dem Main-Event-Loop (einzigartig unter den 5 Uploadern — die anderen 4 streamen async). Bei jedem Read ~5–9 ms SSD / 30–100 ms langsame Platte friert der GANZE Main-Loop (alle Progress/IPC/Render/andere-Uploads). Skaliert mit der Zahl paralleler clouddrop-Uploads. Passt exakt auf „laggy beim Hochladen, schlimmer mit mehr gleichzeitig". User nutzt clouddrop. +**Fix:** `fs.openSync`/`readSync`/`closeSync` → `fs.promises.open` + `await fh.read` + `await fh.close()`. Byte-Äquivalenz mit Hash-Vergleich über alle Chunk-Grenzfälle verifiziert (volle/partielle/multi-Chunk/1-Byte) BEVOR geshipped — ein Chunk-Read-Bug = korrupter Upload, deshalb Pflicht-Verifikation, nicht „sieht richtig aus". Separater Fix: rotation-retry + suspect-alternate progressCb in upload-manager.js feuerten `_emitProgress` (sync `emit` + frischer Object-Spread) bei JEDEM Stream-Chunk (hunderte/s/Job) — der 250-ms-`lastEmitTime`-Gate des Primary-Path fehlte. Gate gespiegelt (activeEntry-Mutation bleibt ungated für Stats/Speed-Monitor, nur der emit ist gegated). +**Regel:** Wenn der User eine konkrete Mechanik vermutet („er updatet alle Zeilen gebündelt"), die Mechanik MESSEN bevor man sie fixt — nicht der Plausibilität folgen. Die Messung kann die Hypothese widerlegen UND den echten Verursacher woanders aufdecken (hier: nicht Renderer-DOM, sondern sync-fs im Upload-Datapfad). „Laggy bei moderater CPU" (40%/8 Kerne = ein Kern bei 100%) zeigt auf Main-Thread-Sättigung/sync-Blocking, NICHT auf DOM-Amplifikation. Bei Daten-Pfad-Fixes (Upload-Bytes) immer Byte-Äquivalenz beweisen, nicht nur Tests grün. +**Discriminator nicht vergessen:** Mit der echten User-Config (parallelCount 2×5 Hoster ≈10 gleichzeitig) sind „100 gleichzeitig" nur erreichbar wenn die Parallel-Counts hochgedreht wurden — sonst sind „100" die QUEUE-Größe, nicht concurrent. Nach dem Ship dem User die Unterscheidungsfrage stellen (Lag clouddrop-spezifisch? Parallel-Counts erhöht?), statt blind Sieg zu erklären — bei echter High-Concurrency bräuchte es ein Concurrency-Cap / Worker-Prozess, keinen Mikro-Fix. diff --git a/tasks/todo.md b/tasks/todo.md index bb3068b..75aecd1 100644 --- a/tasks/todo.md +++ b/tasks/todo.md @@ -1,57 +1,44 @@ -# Diagnostics audit (3rd /goal re-fire): the un-audited v3.3.84/85 remote-diagnostics app-side code +# High-concurrency lag audit (v3.3.90) — "lag ist immernoch da, ich vermute ab X gleichzeitig muss er alle Zeilen gebündelt updaten" -Method: 52-agent adversarial audit of lib/diagnostics-agent.js, lib/diagnostics-collectors.js, -lib/support-bundle.js, lib/remote-server.js + the diag wiring in main.js/renderer/preload, for lag/freeze -+ correctness/leak. 23 findings, each double-verified. Reference = THIS user's real config (23 result rows). +Method: 44-agent high-concurrency audit of the full upload→IPC→render path + Blink benchmark of the +renderer queue table (Playwright/Chromium = same Blink engine), targeting the user's NEW hypothesis: +"with ~100 concurrent uploads the renderer has to update ALL rows bundled rather than cleanly per-row." -## Conclusion: the app is HEALTHY for this user's actual usage. -All shipped fixes (v3.3.87 recent-panel, v3.3.88 doodstream) stand. The remaining findings are COLD, -opt-in (diagnostics is off by default; needs the user to enable it AND a remote operator to connect AND a -large accumulated history) — they impose ZERO cost on the normal path and start to matter only ~30k history -rows (this user: 23). Found + documented = the deliverable for a "could it exist" audit goal. +## The user's hypothesis is MEASURED-REFUTED — the renderer is NOT the bottleneck. +Blink benchmark over scenarios Q=150..1000, M=10 active, 60 ticks each: +- renderQueueTable virtualizes at ≥200 rows; <200 = change-detecting in-place update. +- _updateRowInPlace is change-detecting (no forced reflow, no layout reads). +- median render <1 ms at Q=1000; only ~4/60 renders are full rebuilds even with progress-crossing sorts. +- progress is coalesced main-side (_progressByJob Map keyed by jobId + 100ms flush → one batch sized by + active-job count, ~10/sec); renderer iterates the batch with cheap per-row handleProgress. +DOM amplification is ruled out by measurement. "Laggy at ~40% CPU / 8 cores" = ONE core at 100% = +main-thread saturation / synchronous blocking, not DOM. -## SHIPPED (v3.3.89) — only the two isolated, zero-redaction-surface fixes -- lib/remote-server.js: WebSocketServer had NO maxPayload → a pre-auth message ran a synchronous - JSON.parse of up to ws's 100 MiB default per message on the main loop = an unbounded freeze/DoS sink. - Set maxPayload = 256 KiB (diag/auth/WebRTC-signaling messages are tiny). Closes the sink for free. -- lib/remote-server.js sendToClient: `ws.send(JSON.stringify(data))` had no readyState/try guard (unlike - broadcast) → a send on a closing socket or a stringify throw escaped as uncaughtException (potential - crash). Now guarded with `ws.readyState === 1` + try/catch, mirroring broadcast. - 397/397 tests pass, eslint clean. +## SHIPPED (v3.3.90) — the two real main-thread blockers, both behavior-preserving +1. lib/clouddrop-upload.js `_uploadChunked`: was reading each 16 MB chunk with `fs.readSync` SYNCHRONOUSLY + on the main event loop — unique among the 5 uploaders (the other 4 stream async). Each read blocks the + WHOLE loop (~5–9 ms SSD, 30–100 ms slow disk) → freezes all progress/IPC/render/other-uploads, scaling + with the number of concurrent clouddrop uploads. Fits "laggy when uploading, worse with more concurrent." + User uses clouddrop. Fix: `fs.openSync`/`readSync`/`closeSync` → `fs.promises.open` + `await fh.read` + + `await fh.close()`. Byte-equivalence verified by SHA-256 over all chunk-boundary cases (full chunk, + partial last chunk, 2/3/4-chunk, single byte) before shipping — a chunk-read bug = corrupt upload. +2. lib/upload-manager.js rotation-retry (944) + suspect-alternate (1075) progressCb: both called + `_emitProgress` (a synchronous `emit('progress')` + fresh object spread) on EVERY stream chunk + (hundreds/sec per job) — they were missing the 250 ms `lastEmitTime` gate that the primary path (631) + has. With many concurrent uploads in rotation/suspect mode that's real main-thread emit amplification. + Mirrored the gate exactly: activeEntry mutation stays UNGATED (stats/speed-monitor stay fresh), only the + emit is throttled to 4/sec. Behavior-preserving. +397/397 tests pass, eslint clean (1 pre-existing unrelated warning at line 554). -## DEFERRED — documented, conscious (real, but the fix cuts into the credential-redaction surface) -14 of 15 actionable findings converge on ONE cold-path freeze: a single `server_health` (and -get_history/list_errors/get_config) does O(historySize) synchronous work per request — - (a) ~6–7 configStore.load() per request, each a full structuredClone(config incl. unbounded history); - (b) summarizePerHoster copies+sorts+walks ALL batches; _historyErrors regex-scans ALL batches; the - `limit` arg only slices the OUTPUT, not the traversal. -Measured: ~258 ms at 7.45 MB / 30k rows, up to 1.6 s–6.7 s in the 39–185 MB tail. On a large history a -diagnostic query would freeze the very app it's diagnosing (violates the v3.3.85 rule). REAL — but: -- It only fires during an active diagnostic session on a large history; this user (23 rows) never hits it. -- The tempting fix (snapshot config + thread secrets through the collectors) refactors - lib/diagnostics-collectors.js — which IS the redaction surface (_secrets/_deepRedact/collectSecretValues/ - sanitizeConfig/redactLogText). That code has LEAKED TWICE before (7b5420e "one collector still leaked", - 8d757a9 "redaction gaps") in ways that passed green tests. Threading a secrets snapshot = exactly that - shape: miss/stale one path → SECRET LEAK, a worse failure than the cold freeze. Same category error as - last turn's config-store defer (then: data-loss; now: secret-leak), different file. -- If ever hardened: ONLY bound the history walk via summarizePerHoster's existing opts.lastNBatches (NOT the - secret threading), after confirming per-hoster-over-last-N is acceptable health-snapshot semantics, WITH a - per-collector redaction E2E + advisor pass. Not under a Stop hook. -Related cold-path items (same defer): get_config_redacted 'all' does 3 full-history passes then discards -history (strip history first would help — but same file); collectSecretValues/_secrets unmemoized per -request; support-bundle.js sanitizeConfig serializes the whole history + reads ~11 MB logs sync on the main -loop (explicit user-triggered bundle, very cold). remote-server _failedAttempts sub-threshold entries never -pruned (slow IP-keyed growth) — low. +## DROPPED (advisor: measured fine, don't chase perception) +- Lowering the virtual-row threshold below 200: the Blink benchmark shows <200 in-place updates are already + sub-ms; no change warranted. -## DEFERRED (config-store cluster, re-confirmed from last turn) -- load() structuredClone of whole config (incl. history) per call; _atomicWrite nulls cache per write; - _serializeForDisk stringifies whole history per write; _preserveDiagSubtree adds one extra load() per - save-global-settings (normal path, low). All scale with historySize, all sub-ms at this user's 52 KB - config, all in credential-bearing persistence code. Safe root fix = split history to its own file - (out of scope: risk > reward). Documented, not operated. - -## Verified clean (no action) -- diagnostics collectors are otherwise well-bounded: readLog tailKb clamped ≤1024 KB, grep is literal - substring (ReDoS-free), get_config 'all' deletes history before redaction, all limit/maxJobs-bounded. -- _deepRedact is bounded at every call site (NOT the freeze source). -- diagnostics renderer wiring is all in renderSettings() (cold); no per-progress/per-frame cost when off. +## OPEN — discriminator question to the user (do NOT declare victory blind) +With this user's real config (parallelCount 2 × 5 hosters ≈ 10 max concurrent), "100 concurrent" is only +reachable if the parallel counts were raised — otherwise "100" is the QUEUE size and only ~10 upload at once. +Ask: (a) is the lag specifically during clouddrop uploads? (b) did you raise the per-hoster/global parallel +counts above 2? If it's true high concurrency (dozens of simultaneous undici streams funneling decode + +progress callbacks through the one main JS thread), that needs a concurrency cap or a worker/child process — +NOT a micro-fix. The two shipped fixes are genuine improvements regardless; the answer decides whether a +bigger architectural change is the next step.