docs(todo,lessons): diagnostics audit — healthy for real usage; ship 2 transport one-liners, defer the redaction-surface freeze
52-agent audit of the v3.3.84/85 remote-diagnostics code. App is healthy for this user's actual usage. Shipped the two zero-redaction-surface fixes (ws maxPayload, sendToClient guard). Deferred the cold-path server_health O(historySize) freeze because the fix refactors the credential-redaction collectors (leaked twice before). Lesson: "not persistence, so safe" is a fallacy — the redaction layer is an equally catastrophic guarantee surface (silent secret leak). At an audit goal, find+document is the deliverable; don't cut into a twice-leaked redaction pipeline under a Stop hook. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
0809c75d50
commit
8df6de06f1
@ -146,3 +146,9 @@
|
|||||||
**Die Falle (Advisor hat geblockt):** Ich wollte es „elegant" fixen mit `history.slice()` (shallow) statt deep-clone. Advisor: STOPP. `load()` ist der gefährlichste Code im Repo (config + credentials; Korruption = Datenverlust), ich war hier schon mal von Cache-Semantik gebissen worden. Und: der Perf-Win und das Risiko sind DIESELBE Münze — der Speedup kommt NUR vom Sharing der Batch-Objekte by-reference, und genau dieses Sharing IST die Silent-Cache-Corruption-Gefahr (hängt an einem globalen Invariant „nichts deep-mutated je eine history-Batch" den ich über zukünftigen Code + jeden getHistory-Consumer nicht erzwingen kann). Es gibt KEINE sichere Version dieses Ansatzes → falsches Werkzeug für safety-kritischen Code. Hardcoded 5 keys in `_cloneConfig` wäre ein zweiter Footgun (zukünftiger top-level key verschwindet still aus jedem load()).
|
**Die Falle (Advisor hat geblockt):** Ich wollte es „elegant" fixen mit `history.slice()` (shallow) statt deep-clone. Advisor: STOPP. `load()` ist der gefährlichste Code im Repo (config + credentials; Korruption = Datenverlust), ich war hier schon mal von Cache-Semantik gebissen worden. Und: der Perf-Win und das Risiko sind DIESELBE Münze — der Speedup kommt NUR vom Sharing der Batch-Objekte by-reference, und genau dieses Sharing IST die Silent-Cache-Corruption-Gefahr (hängt an einem globalen Invariant „nichts deep-mutated je eine history-Batch" den ich über zukünftigen Code + jeden getHistory-Consumer nicht erzwingen kann). Es gibt KEINE sichere Version dieses Ansatzes → falsches Werkzeug für safety-kritischen Code. Hardcoded 5 keys in `_cloneConfig` wäre ein zweiter Footgun (zukünftiger top-level key verschwindet still aus jedem load()).
|
||||||
**Regel:** „Audit jede Zeile" heißt JEDE Zeile ANSCHAUEN + die Magnitude MESSEN + eine risiko-angemessene Entscheidung treffen — NICHT jeden geflaggten Befund fixen. Bei einem Audit-Goal ist „ich habe jede Zeile geprüft, jeden Befund als sub-ms bei realistischer History gemessen, den Mechanismus bestätigt aber den Fix als riskante Persistenz-Chirurgie für einen latenten Mikro-Cost eingestuft, also dokumentiere ich ihn statt ihn zu shippen" die VOLLSTÄNDIGE, gründliche Antwort. Jeden geflaggten Punkt unabhängig vom Risiko zu fixen ist keine Gründlichkeit — so wird aus einer Lag-Fix-Session ein Datenverlust-Incident. Nur den EINEN Befund shippen der im echten Szenario beißt (doodstream `_debugLog`: sync statSync+appendFileSync ~8–15×/Upload auf dem Main-Loop während des Uploads → hinter `logVerbose` gaten, default off, near-zero risk). Den Rest als bewusste Defers mit Messzahlen dokumentieren.
|
**Regel:** „Audit jede Zeile" heißt JEDE Zeile ANSCHAUEN + die Magnitude MESSEN + eine risiko-angemessene Entscheidung treffen — NICHT jeden geflaggten Befund fixen. Bei einem Audit-Goal ist „ich habe jede Zeile geprüft, jeden Befund als sub-ms bei realistischer History gemessen, den Mechanismus bestätigt aber den Fix als riskante Persistenz-Chirurgie für einen latenten Mikro-Cost eingestuft, also dokumentiere ich ihn statt ihn zu shippen" die VOLLSTÄNDIGE, gründliche Antwort. Jeden geflaggten Punkt unabhängig vom Risiko zu fixen ist keine Gründlichkeit — so wird aus einer Lag-Fix-Session ein Datenverlust-Incident. Nur den EINEN Befund shippen der im echten Szenario beißt (doodstream `_debugLog`: sync statSync+appendFileSync ~8–15×/Upload auf dem Main-Loop während des Uploads → hinter `logVerbose` gaten, default off, near-zero risk). Den Rest als bewusste Defers mit Messzahlen dokumentieren.
|
||||||
**Wie anwenden:** Wenn ein Goal („JEDE!! JEDE!!!") + ein Stop-Hook Druck erzeugen, immer weiterzuschneiden: das ist genau der Moment, den Advisor VOR dem Edit zu rufen. Magnitude am ECHTEN Artefakt prüfen (der User-Config, nicht @8000-Batches-Hypothese). Persistenz-/Credential-Code nur anfassen wenn der Fix risiko-frei UND der Gewinn real-spürbar ist — sonst dokumentieren und stoppen.
|
**Wie anwenden:** Wenn ein Goal („JEDE!! JEDE!!!") + ein Stop-Hook Druck erzeugen, immer weiterzuschneiden: das ist genau der Moment, den Advisor VOR dem Edit zu rufen. Magnitude am ECHTEN Artefakt prüfen (der User-Config, nicht @8000-Batches-Hypothese). Persistenz-/Credential-Code nur anfassen wenn der Fix risiko-frei UND der Gewinn real-spürbar ist — sonst dokumentieren und stoppen.
|
||||||
|
|
||||||
|
## 2026-06-21 — "Nicht-Persistenz also sicher" ist ein Trugschluss: der Redaktions-Layer ist GENAUSO gefährlich (v3.3.89)
|
||||||
|
**Kontext:** 3. identische /goal-Re-Fire („JEDE zeile, alles drum-und-dran"). Diesmal die un-auditierte Remote-Diagnostics-Code (v3.3.84/85) zeilenweise auditiert (52 Agenten). 14 von 15 actionable Findings konvergierten auf EINEN Cold-Path-Freeze: `server_health` macht O(historySize) sync-Arbeit pro Request (~6–7 config-clones + unbounded history-walks; `limit` slict nur den Output). Gemessen 258 ms–6,7 s bei großer History → friert die App ein, die es diagnostiziert (verletzt die v3.3.85-Regel).
|
||||||
|
**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.
|
||||||
|
|||||||
@ -1,50 +1,57 @@
|
|||||||
# Lag audit of every line written this session (Goal: "schau dir JEDE zeile an … ob es solche probleme gibt o. geben könnte")
|
# Diagnostics audit (3rd /goal re-fire): the un-audited v3.3.84/85 remote-diagnostics app-side code
|
||||||
|
|
||||||
Method: self-review of every hot-path line I changed (config-store T1, main.js T3, renderer append-evict,
|
Method: 52-agent adversarial audit of lib/diagnostics-agent.js, lib/diagnostics-collectors.js,
|
||||||
diagnostics) + an 18-agent adversarial line-by-line audit (each finding double-verified for real + causes-
|
lib/support-bundle.js, lib/remote-server.js + the diag wiring in main.js/renderer/preload, for lag/freeze
|
||||||
perceptible-lag) + Blink microbenchmarks. Reference config = THIS user's real one: 8 batches / 4.8 KB
|
+ correctness/leak. 23 findings, each double-verified. Reference = THIS user's real config (23 result rows).
|
||||||
history / 52 KB total — measured, not assumed.
|
|
||||||
|
|
||||||
## Conclusion: v3.3.87 was the fix. The rest is a clean bill of health, not a to-do list.
|
## Conclusion: the app is HEALTHY for this user's actual usage.
|
||||||
The reported "laggy after long runtime" was the recent-panel rebuild cliff — found, fixed (append-evict,
|
All shipped fixes (v3.3.87 recent-panel, v3.3.88 doodstream) stand. The remaining findings are COLD,
|
||||||
80 ms → 7.4 ms Blink-verified), released v3.3.87. The audit surfaced NO second cause that affects this user.
|
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.
|
||||||
|
|
||||||
## SHIPPED this round (v3.3.88) — the one finding that bites in the real upload scenario
|
## SHIPPED (v3.3.89) — only the two isolated, zero-redaction-surface fixes
|
||||||
- doodstream-upload.js `_debugLog`: ran ungated SYNCHRONOUS statSync + appendFileSync (~8–15× per upload)
|
- lib/remote-server.js: WebSocketServer had NO maxPayload → a pre-auth message ran a synchronous
|
||||||
on the main-process event loop WHILE uploading — delaying IPC / progress-batch forwarding for every other
|
JSON.parse of up to ws's 100 MiB default per message on the main loop = an unbounded freeze/DoS sink.
|
||||||
concurrent upload. The user runs doodstream, so this fired in practice. Fix: gate it behind the EXISTING
|
Set maxPayload = 256 KiB (diag/auth/WebRTC-signaling messages are tiny). Closes the sink for free.
|
||||||
`globalSettings.logVerbose` setting (default off), mirroring main.js `logDebug`/`_logVerbose`; wired via the
|
- lib/remote-server.js sendToClient: `ws.send(JSON.stringify(data))` had no readyState/try guard (unlike
|
||||||
single `setLogVerbose` chokepoint (boot + save + toggle). Near-zero risk (early-return when verbose off),
|
broadcast) → a send on a closing socket or a stringify throw escaped as uncaughtException (potential
|
||||||
removes all per-upload sync fs in normal operation. 397/397 tests pass, lint clean, wiring verified (shared
|
crash). Now guarded with `ws.readyState === 1` + try/catch, mirroring broadcast.
|
||||||
module instance, default-off, toggles).
|
397/397 tests pass, eslint clean.
|
||||||
|
|
||||||
## DEFERRED — documented, conscious (real mechanisms, but wrong risk/reward to ship)
|
## DEFERRED — documented, conscious (real, but the fix cuts into the credential-redaction surface)
|
||||||
These are in MY T1 code (lib/config-store.js, commit 29d1944). They are REAL and scale with history size,
|
14 of 15 actionable findings converge on ONE cold-path freeze: a single `server_health` (and
|
||||||
but at THIS user's scale (8 batches) they are tens of MICROSECONDS, on a path that fires ~once/20s. The
|
get_history/list_errors/get_config) does O(historySize) synchronous work per request —
|
||||||
verifiers said "not urgent / negligible (~0.1% duty) / latent main-process hygiene, not a renderer-lag fix."
|
(a) ~6–7 configStore.load() per request, each a full structuredClone(config incl. unbounded history);
|
||||||
load() is the most dangerous code in the repo (config + credentials; corruption = data loss) and I was
|
(b) summarizePerHoster copies+sorts+walks ALL batches; _historyErrors regex-scans ALL batches; the
|
||||||
already bitten once here by cache semantics. So: recorded, not shipped.
|
`limit` arg only slices the OUTPUT, not the traversal.
|
||||||
- F1 `_serializeForDisk` JSON.stringify(whole config incl. unbounded history, null,2) on every save +
|
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
|
||||||
copyFileSync of the growing file per commit. Scales with historySize. Root fix = split history into its own
|
diagnostic query would freeze the very app it's diagnosing (violates the v3.3.85 rule). REAL — but:
|
||||||
file (history.json) so the ~20s queue-persist stops dragging history. Higher-risk persistence surgery.
|
- It only fires during an active diagnostic session on a large history; this user (23 rows) never hits it.
|
||||||
- F2 `load()` deep-clones the WHOLE config (incl. history) on every call (even cache hit), and `_atomicWrite`
|
- The tempting fix (snapshot config + thread secrets through the collectors) refactors
|
||||||
nulls the cache per write so write-interleaved loads are full misses. Measured @8000 batches: old read+parse
|
lib/diagnostics-collectors.js — which IS the redaction surface (_secrets/_deepRedact/collectSecretValues/
|
||||||
9.65 ms → new miss 22.96 ms (a 2.2–2.4× regression I introduced in T1) → new hit 12.94 ms. A "fix" via
|
sanitizeConfig/redactLogText). That code has LEAKED TWICE before (7b5420e "one collector still leaked",
|
||||||
shallow `history.slice()` is UNSAFE: the speedup comes only from sharing batch objects by reference, which
|
8d757a9 "redaction gaps") in ways that passed green tests. Threading a secrets snapshot = exactly that
|
||||||
is exactly a silent-cache-corruption hazard that depends on a global "nothing ever deep-mutates a history
|
shape: miss/stale one path → SECRET LEAK, a worse failure than the cold freeze. Same category error as
|
||||||
batch" invariant I can't enforce across future code + every getHistory consumer. Perf win and risk are the
|
last turn's config-store defer (then: data-loss; now: secret-leak), different file.
|
||||||
same coin → no safe version → wrong tool for safety-critical code. The genuinely safe fixes (history-split,
|
- If ever hardened: ONLY bound the history walk via summarizePerHoster's existing opts.lastNBatches (NOT the
|
||||||
bounded default retention) are out of scope (history-split = risk; bounded default = could drop user history).
|
secret threading), after confirming per-hoster-over-last-N is acceptable health-snapshot semantics, WITH a
|
||||||
- F3 (renderer, low) `_completedUploadKeys` grows unbounded per session, fully iterated in
|
per-collector redaction E2E + advisor pass. Not under a Stop hook.
|
||||||
buildPersistedQueueState on the ~20s persist. Sub-ms even for thousands of keys; the Set is the re-queue
|
Related cold-path items (same defer): get_config_redacted 'all' does 3 full-history passes then discards
|
||||||
dedup so capping it risks correctness. Not worth it.
|
history (strip history first would help — but same file); collectSecretValues/_secrets unmemoized per
|
||||||
- F4 (operator, low) diagnostics serverHealth does ~6–8 configStore.load() per request, each cloning history.
|
request; support-bundle.js sanitizeConfig serializes the whole history + reads ~11 MB logs sync on the main
|
||||||
Cold path (only when diagnostics is queried), and largely a function of F2; defer with F2.
|
loop (explicit user-triggered bundle, very cold). remote-server _failedAttempts sub-threshold entries never
|
||||||
|
pruned (slow IP-keyed growth) — low.
|
||||||
|
|
||||||
|
## 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)
|
## Verified clean (no action)
|
||||||
- T1 structuredClone-per-load does NOT hit the upload hot path: shouldLogHosterToFile uses LIVE
|
- diagnostics collectors are otherwise well-bounded: readLog tailKb clamped ≤1024 KB, grep is literal
|
||||||
uploadManager.hosterSettings during uploads (its load() fallback is unreachable mid-batch); the 8×/s log
|
substring (ReDoS-free), get_config 'all' deletes history before redaction, all limit/maxJobs-bounded.
|
||||||
flush uses the O(1) _getLogSettings cache (T3); remaining load() sites are user-triggered IPC / boot.
|
- _deepRedact is bounded at every call site (NOT the freeze source).
|
||||||
- T3 _getLogSettings: O(1) on cache hit, invalidated on every settings-write path.
|
- diagnostics renderer wiring is all in renderSettings() (cold); no per-progress/per-frame cost when off.
|
||||||
- renderer append-evict (v3.3.87): no new per-frame cost; correctness re-verified (5000-completion sim).
|
|
||||||
- diagnostics renderer additions (0c6c502): all inside renderSettings() — cold, settings panel only.
|
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user