|
| 1 | +--- |
| 2 | +name: silent-degradation-needs-a-run-level-guard |
| 3 | +description: A pipeline stage that catches per-item errors and continues (returns empty, logs a per-item warning) will hide a GLOBAL failure of that stage — every item fails identically, the run completes, prints a count, and exits 0. Per-item warnings collapse and never become a verdict. The fix is a pair — (a) at the failure site, distinguish a global/init failure from a per-item failure and THROW on global; (b) a run-level aggregate guard ("processed N items but produced 0 of the expected output") that maps to a distinct exit code. Smoke tests must assert real output, not "any non-empty result". |
| 4 | +metadata: |
| 5 | + type: bug |
| 6 | + category: best-practices |
| 7 | +tags: [analyze, parse, wasm, silent-failure, exit-code, error-handling, piscina, smoke-test, guard] |
| 8 | +discovered: 2026-06-08 |
| 9 | +session: session-analyze-hardening |
| 10 | +related: |
| 11 | + - fixed-offset-asset-resolvers-break-on-bundle-collapse |
| 12 | + - doctor-probe-drift-after-rip-and-replace |
| 13 | + - kill-escalation-races-the-exit-handler |
| 14 | +--- |
| 15 | + |
| 16 | +# Silent degradation needs a run-level guard, not just per-item warnings |
| 17 | + |
| 18 | +## What bit us |
| 19 | + |
| 20 | +The fixed-offset WASM-resolver bug ([[fixed-offset-asset-resolvers-break-on-bundle-collapse]]) |
| 21 | +shipped invisibly for ~5 days. The resolver bug was the *cause*; the reason it |
| 22 | +was *invisible* is a separate, more general defect worth its own lesson: |
| 23 | + |
| 24 | +`codehub analyze` parses every source file in a Piscina worker. The worker's |
| 25 | +per-file `try/catch` (parse-worker.ts) mapped **any** thrown error to |
| 26 | +`{captures: [], warnings: [msg]}` and continued. So when the grammar runtime was |
| 27 | +globally dead (vendored `vendor/wasms/` unresolvable in the flat bundle), EVERY |
| 28 | +file took the catch, produced zero captures, and the pipeline built a |
| 29 | +File/Directory-only **skeleton graph** — then printed `"5 nodes"` and **exited |
| 30 | +0**. The per-file warnings existed but `logWarnings` collapsed them into |
| 31 | +`"parse: N warnings (use --verbose)"`, and nothing aggregated them into a |
| 32 | +run-level verdict or an exit code. A broken parser looked exactly like a healthy |
| 33 | +parse of a symbol-free repo. |
| 34 | + |
| 35 | +## The shape of the bug class |
| 36 | + |
| 37 | +Any stage with this shape is vulnerable: |
| 38 | + |
| 39 | +``` |
| 40 | +for (const item of items) { |
| 41 | + try { results.push(await process(item)); } |
| 42 | + catch (e) { warnings.push(e.message); results.push(EMPTY); } // continue |
| 43 | +} |
| 44 | +return results; // a GLOBAL failure = N identical EMPTY results = silent success |
| 45 | +``` |
| 46 | + |
| 47 | +The per-item resilience (good — one malformed file shouldn't fail the run) is |
| 48 | +exactly what masks a global failure (bad — a dead runtime should fail the run). |
| 49 | + |
| 50 | +## The fix is a PAIR, not one change |
| 51 | + |
| 52 | +You need both halves; either alone is insufficient. |
| 53 | + |
| 54 | +**(a) At the failure site, distinguish global from per-item and THROW on global.** |
| 55 | +Introduce a sentinel error (`WasmRuntimeUnavailableError`) for the |
| 56 | +init/deployment-level failure (vendored dir missing, runtime won't init). Throw |
| 57 | +it from the init path; rethrow it through the per-item catch *before* the |
| 58 | +generic warning-mapping. A global failure then aborts the run loudly with an |
| 59 | +actionable message, instead of becoming N warnings. Per-item errors (one bad |
| 60 | +file) still warn-and-skip. |
| 61 | + |
| 62 | +**(b) A run-level aggregate guard as the backstop.** Even with (a), a softer |
| 63 | +degradation can slip through (e.g. the runtime package is genuinely absent and |
| 64 | +the init path returns a soft `undefined` by design). So add a run-level |
| 65 | +predicate: "the run processed >= K items that SHOULD have produced output, but |
| 66 | +produced 0" → push a loud warning AND set a **distinct advisory exit code** |
| 67 | +(here: 3, separate from generic-failure 1) so CI can detect a silent-skeleton |
| 68 | +run without scraping logs. Make the predicate a pure exported function and |
| 69 | +table-test the threshold. |
| 70 | + |
| 71 | +Keep the two orthogonal: (a) keys off the thrown error, (b) keys off the output |
| 72 | +count. They must not double-fire on the same condition — (a) handles hard death, |
| 73 | +(b) handles the residual soft case. |
| 74 | + |
| 75 | +## Gotchas that matter |
| 76 | + |
| 77 | +- **Piscina structured-clones the rejection across threads** — a custom |
| 78 | + `class XError extends Error` arrives on the main thread as a plain `Error` |
| 79 | + with the prototype lost. Set `this.name` in the constructor and match on |
| 80 | + `err.name === "XError"` on the main thread (NOT `instanceof`). `instanceof` |
| 81 | + is fine *inside* the worker, before serialization. |
| 82 | +- **Pick the denominator carefully.** "Files scanned" is the wrong count if some |
| 83 | + legitimately produce no output (here: cobol routes through a regex provider, |
| 84 | + not tree-sitter; config-only/empty repos have zero parseable files). The guard |
| 85 | + must key off "items that SHOULD have produced output" (tree-sitter files), or |
| 86 | + it false-positives on legitimately-empty repos. And don't count placeholder |
| 87 | + output (external import stubs are `CodeElement` nodes — counting them as |
| 88 | + "symbols" would let an import-only repo mask a broken parser). |
| 89 | +- **Smoke tests must assert REAL output, not "any non-empty result".** The |
| 90 | + global-install verifier queried `'export default'` and passed on "≥1 hit" — |
| 91 | + but the query command's stderr header (`query: "..." (0 results)`) is itself |
| 92 | + non-empty, so the gate passed on a 0-symbol graph. Tighten to a KNOWN symbol |
| 93 | + from a KNOWN fixture with the expected KIND (`Function`) and FILE — something |
| 94 | + that can only appear if extraction actually ran. |
| 95 | +- **A separate `doctor`-style health command does NOT protect the hot path.** |
| 96 | + `doctor` had a vendored-wasms check, but it's a manual command never run by |
| 97 | + `analyze`. Don't assume an existing health probe covers the pipeline. |
| 98 | + |
| 99 | +## The one-line takeaway |
| 100 | + |
| 101 | +Per-item error resilience hides global stage failure. Whenever you write |
| 102 | +`catch → warn → continue` in a loop, ask "what does a GLOBAL failure of this |
| 103 | +stage look like?" — and add (a) a thrown sentinel for the global case + (b) a |
| 104 | +run-level "produced 0 of the expected output" guard with its own exit code. |
| 105 | +Then make the smoke test assert real output, not a non-empty string. |
0 commit comments