Skip to content

Edge functions: crash-safety hardening (timeouts + vendored deno_dom) - #194

Merged
JakeSCahill merged 6 commits into
mainfrom
fix/edge-function-crash-safety
Jul 29, 2026
Merged

Edge functions: crash-safety hardening (timeouts + vendored deno_dom)#194
JakeSCahill merged 6 commits into
mainfrom
fix/edge-function-crash-safety

Conversation

@JakeSCahill

Copy link
Copy Markdown
Contributor

Why

Crash-safety audit of all 9 edge functions, prompted by an intermittent "edge function invocation failed" reported during deploy-preview-190 testing. The reviewed functions are otherwise solid (most are static generators); this PR fixes the failure modes that can surface as a platform-level invocation crash.

Root cause addressed

The recurring pattern was a fetch() with no timeout. The subtlety: a hang never throws, so the surrounding try/catch doesn't catch it — the request holds the edge invocation open until Netlify's platform kills it and reports "edge function invocation failed." Timeouts convert the hang into an AbortError the existing fallback handles cleanly.

Fixes (4)

  1. serve-markdown (/*, runs on every request) — internal .md fetch had no timeout. Added a 1.5s AbortController; on abort/error, fall through to context.next() (serve HTML). The markdown variant is an optimization, never required.
  2. proxy-api-docs (/api/*) — the MCP-passthrough fetch to Bump.sh had no timeout (the main proxy path was already protected by fetchWithRetry's AbortSignal.timeout). Added AbortSignal.timeout(10000) so the existing catch returns a clean 502.
  3. agent-skills-index — same-origin digest fetches inside Promise.all had no timeout; a cold origin could hang the invocation. Added AbortSignal.timeout(3000) with placeholder-digest fallback.
  4. proxy-api-docs — vendored deno_dom — removed the deploy-time dependency on deno.land (a build there would fail if deno.land were down; it also pinned a critical dep to a third-party host). Self-contained copy of [email protected] (incl. inlined WASM) under _vendor/, imported by relative path. Every fetch() across all edge functions is now timeout-bounded.

Verification (local, offline)

  • Deno 2.9 (the edge runtime engine): vendored deno_dom parses HTML + createElement/innerHTML/appendChild/outerHTML with --no-remote (zero network); deno check --no-remote resolves.
  • esbuild --bundle: full JS/TS graph resolves, only .wasm external.

Please verify on the deploy preview

  • /api/* API reference pages render — confirms Netlify's bundler embedded the local WASM (the one thing not verifiable locally; the same module graph already bundles today from the remote URL).
  • _vendor/ files are NOT listed as deployed edge functions (they're in an underscore-prefixed subdir and have no config export, so they shouldn't be). If either fails: move _vendor out of the edge-functions dir and wire a deno_import_map in netlify.toml.

🤖 Generated with Claude Code

JakeSCahill and others added 3 commits July 21, 2026 10:25
The edge function's internal fetch for the .md variant had no timeout. On a
cold/slow origin it could hold the invocation open until the platform killed
it with "edge function invocation failed" (transient, first-request-only,
gone on retry -- matching the report from deploy-preview-190 testing).

Add a 1.5s AbortController timeout; on abort or any error, fall through to
context.next() and serve the HTML. The markdown variant is an optimization,
never required, so a timeout degrades cleanly instead of erroring the page.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
Two edge functions had fetch() calls with no timeout, the same crash class as
serve-markdown: a hung upstream holds the edge invocation open until the
platform kills it with "edge function invocation failed", bypassing the
surrounding try/catch (a hang never throws).

- proxy-api-docs: MCP passthrough fetch to Bump.sh had no timeout (the main
  proxy path was already protected via fetchWithRetry's AbortSignal.timeout).
  Add AbortSignal.timeout(10000) so the catch returns a clean 502 instead.
- agent-skills-index: same-origin digest fetches (to other edge functions /
  llms.txt) inside Promise.all had no timeout; a cold origin could hang the
  invocation. Add AbortSignal.timeout(3000); on timeout, fall back to the
  placeholder digest.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
The edge function imported deno_dom from https://deno.land at module load.
Netlify bundles it at deploy time, so it's not a runtime crash risk -- but a
build would fail if deno.land were down, and it pins a critical dependency to a
third-party host.

Vendor a self-contained copy of [email protected] (incl. the 513KB WASM, which
loads via local module import -- no runtime fetch) under _vendor/ and import it
by relative path. Underscore-prefixed subdir so it isn't treated as an edge
function.

Verified offline with Deno 2.9 (the edge runtime engine):
- deno run --no-remote: parses HTML + createElement/innerHTML/appendChild/
  outerHTML (every op proxy-api-docs uses)
- deno check --no-remote: type-resolves with zero network deps
- esbuild --bundle: full JS/TS graph resolves, only .wasm external
Final gate is a deploy-preview build (Netlify bundler embedding the local wasm
is the one thing not verifiable locally; the same graph already bundles today
from the remote URL).

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@JakeSCahill
JakeSCahill requested a review from a team as a code owner July 21, 2026 09:28
@netlify

netlify Bot commented Jul 21, 2026

Copy link
Copy Markdown

Deploy Preview for redpanda-documentation ready!

Name Link
🔨 Latest commit 81fe056
🔍 Latest deploy log https://app.netlify.com/projects/redpanda-documentation/deploys/6a68a569c073910008d04574
😎 Deploy Preview https://deploy-preview-194--redpanda-documentation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 85 (🔴 down 1 from production)
Accessibility: 93 (🔴 down 1 from production)
Best Practices: 92 (no change from production)
SEO: 83 (no change from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

The timeout guards convert previously-silent hangs into catchable errors; make
those catches log structured (JSON with an event tag) and distinguish a timeout
(AbortError, i.e. cold/slow origin — the class behind the reported edge crash)
from ordinary fetch/parse errors, so a recurrence is greppable.

- serve-markdown: event serve_markdown_fallback {reason, path, mdPath, error}
- agent-skills-index: event agent_skills_digest_fallback {reason, skill, url, error}

Scoped to the two functions with thin logging; proxy-api-docs already logs its
failure paths. Happy paths on hot routes are intentionally not logged (noise /
edge CPU budget).

Co-Authored-By: Claude Opus 4.8 <[email protected]>

@Feediver1 Feediver1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review

Scope: 3 edited edge functions + 28 vendored deno_dom files. Overall: high quality — correct diagnosis, minimal targeted fixes, honest verification notes. One minor real bug in the new logging, two hardening suggestions.

Correctness

  • serve-markdown.js — correct pattern: AbortController + setTimeout(1500), and crucially clearTimeout in finally, so no timer dangles on the happy path. Fallback to context.next() is right for an optional optimization, and the AbortError name check is correct for controller.abort().
  • agent-skills-index.ts — the per-skill try/catch sits inside the Promise.all map, so one timeout degrades one skill to a placeholder digest instead of rejecting the whole index.
  • 🐛 Minor bug (the one actionable finding): the same file classifies timeouts with e.name === 'AbortError', but AbortSignal.timeout() aborts with a TimeoutError DOMException (per spec, and in Deno) — so actual timeouts get logged as reason: 'error', defeating the stated purpose of making timeout recurrences greppable. Fallback behavior is unaffected; only the label is wrong.
    Fix: const isTimeout = e instanceof Error && (e.name === 'TimeoutError' || e.name === 'AbortError');
    (serve-markdown is unaffected — controller.abort() genuinely produces AbortError.)
  • proxy-api-docs.jsAbortSignal.timeout(10000) on the MCP passthrough matches the protection the main path already had via fetchWithRetry; the existing catch's 502 is the right conversion.
  • Timeout budgets (1.5s / 3s / 10s) are sensibly proportioned to each fallback's cost.

Supply chain

Verified: all 28 vendored files are byte-identical (SHA-256) to upstream b-fuze/deno-dom at tag v0.1.56 — no modifications, no injected code, and the vendored set is a sensible minimal subset. Removing the deploy-time deno.land dependency is both an availability and a supply-chain win.

Suggestions

  1. Add a _vendor/deno_dom/VENDOR.md recording the upstream repo, tag, file list, and re-vendor command, so the byte-identity stays re-verifiable without archaeology (the import-site comment covers most of this already).
  2. Promote deno check --no-remote netlify/edge-functions/*.ts into CI to lock in the newly won zero-remote-imports property and fail fast if a remote import is ever reintroduced.

Risks

Low. The 1.5s budget on serve-markdown runs on the hottest path; if origin latency regularly exceeds it, the markdown variant silently degrades to HTML — the structured logs will make that visible once the TimeoutError label fix lands.

🤖 Generated with Claude Code

@Feediver1 Feediver1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please take a look at the fixes from the review.

…md, CI check

- agent-skills-index: classify AbortSignal.timeout() aborts as timeouts.
  Per spec (and verified on Deno 2.2.7) the abort reason is a TimeoutError
  DOMException, not AbortError, so real timeouts were logged as
  reason:'error'. Match both names; fallback behavior unchanged.
- _vendor/deno_dom/VENDOR.md: record upstream repo, tag (v0.1.56, commit
  92ff9600), file list, re-vendor and byte-identity verification commands.
  All 28 files independently re-verified byte-identical to upstream.
- CI: new edge-function-checks workflow runs
  deno check --no-remote netlify/edge-functions/*.ts *.js on PRs touching
  edge functions, locking in the zero-remote-imports property. Verified
  the command passes on this tree with Deno 2.x.
@JakeSCahill

Copy link
Copy Markdown
Contributor Author

Thanks @Feediver1 — all three review items verified and applied in 16511ee.

1. TimeoutError classification bug (applied) — confirmed: AbortSignal.timeout() aborts with a TimeoutError DOMException, not AbortError (verified empirically on Deno 2.2.7 against a deliberately slow local server: name: TimeoutError, instanceof Error: true). agent-skills-index.ts now matches both names, so real timeouts log reason: 'timeout' as intended. serve-markdown.js is unchanged — its manual controller.abort() genuinely produces AbortError, as you noted.

2. VENDOR.md (applied) — added netlify/edge-functions/_vendor/deno_dom/VENDOR.md with the upstream repo, tag (v0.1.56, commit 92ff9600), the 28-file list, and copy-pasteable re-vendor and byte-identity verification commands. I also independently re-verified your supply-chain check: all 28 vendored files are byte-identical to upstream at that tag.

3. CI deno check --no-remote (applied) — new .github/workflows/edge-function-checks.yml runs deno check --no-remote over all edge-function .ts and .js files on PRs that touch netlify/edge-functions/**, so a reintroduced remote import fails fast. Verified the command passes on this tree with Deno 2.x before wiring it up (.js included so proxy-api-docs.js and the vendored graph are covered).

Nothing declined.

@JakeSCahill
JakeSCahill merged commit 4bb5534 into main Jul 29, 2026
6 checks passed
@JakeSCahill
JakeSCahill deleted the fix/edge-function-crash-safety branch July 29, 2026 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants