[PR #5874] [MERGED] fix: auto-recover from corrupted sandbox state #5381

New issue

Closed

opened 2026-03-17 02:50:09 +03:00 by kerem · 0 comments

kerem commented

2026-03-17 02:50:09 +03:00

Owner

📋 Pull Request Information

Original PR: https://github.com/hoppscotch/hoppscotch/pull/5874
Author: @jamesgeorge007
Created: 2/12/2026
Status: ✅ Merged
Merged: 2/18/2026
Merged by: @jamesgeorge007

Base: patch ← Head: fix/sandbox-auto-recovery

📝 Commits (1)

248f97c fix: auto-recover from corrupted sandbox state

📊 Changes

13 files changed (+878 additions, -302 deletions)

View changed files

📝 packages/hoppscotch-common/src/helpers/RequestRunner.ts (+38 -2)
➕ packages/hoppscotch-js-sandbox/src/__tests__/combined/script-error-recovery.spec.ts (+206 -0)
📝 packages/hoppscotch-js-sandbox/src/__tests__/hopp-namespace/request.spec.ts (+5 -3)
➕ packages/hoppscotch-js-sandbox/src/__tests__/utils/cage.spec.ts (+50 -0)
📝 packages/hoppscotch-js-sandbox/src/cage-modules/scripting-modules.ts (+10 -8)
📝 packages/hoppscotch-js-sandbox/src/node/pre-request/experimental.ts (+115 -56)
📝 packages/hoppscotch-js-sandbox/src/node/test-runner/experimental.ts (+149 -103)
📝 packages/hoppscotch-js-sandbox/src/types/index.ts (+3 -1)
📝 packages/hoppscotch-js-sandbox/src/utils/cage.ts (+53 -10)
📝 packages/hoppscotch-js-sandbox/src/web/pre-request/index.ts (+111 -50)
📝 packages/hoppscotch-js-sandbox/src/web/pre-request/worker.ts (+5 -1)
📝 packages/hoppscotch-js-sandbox/src/web/test-runner/index.ts (+125 -65)
📝 packages/hoppscotch-js-sandbox/src/web/test-runner/worker.ts (+8 -3)

📄 Description

Closes #5866, #5810, FE-1128.

Persistent errors in the experimental scripting sandbox cause two user-visible bugs:

After fixing a broken pre/post-request script, the old error continues to surface because the singleton FaradayCage retains a corrupted state across executions.
Requests with empty scripts report script_fail when the cached cage carries a stale bootstrap failure. This PR detects bootstrap/internal failures, resets the cage when needed, retries transparently on a fresh cage, and short-circuits empty scripts to avoid entering the sandbox entirely.

What's changed

Bootstrap error detection via captureHook: Extended the captureHook type from { capture?: () => void } to { capture?: () => void; bootstrapError?: unknown } in scripting-modules.ts. When ctx.vm.callFunction(bootstrapIIFE) returns an error, it is dumped and recorded on captureHook.bootstrapError. Both pre-request/index.ts and test-runner/index.ts check this flag as the primary cage reset signal.
Heuristic fallback via isInfraError(): Defense-in-depth error classifier in cage.ts that catches errors escaping before callFunction() is reached — specifically from evalCode(bootstrapCode).unwrap(), createScriptingInputsObj(), and defineSandboxObject(). Uses instanceof Error as the sole discriminator: infrastructure errors (QuickJSUnwrapError from .unwrap(), marshal failures, WASM init) are real Error instances, while user script errors from QuickJS's dump() are plain objects with { name, message, stack } but are NOT instanceof Error. This structural difference makes the check reliable with no false positives.
Retry-on-bootstrap-error: When a bootstrap error is detected, the cage is reset and the script is retried once on a fresh cage within the same execution. This eliminates the regression where fixing a broken script still fails on the next immediate run before succeeding on the one after. Extracted executePreRequestOnCage() and executeTestOnCage() helper functions that return E.Either | "retry". The caller retries once; if the retry also fails with a bootstrap error, surfaces "sandbox initialisation error (persistent)".
Cage reset on bootstrap errors: The reset decision is inlined at each call site as bootstrapFailed || isInfraError(err). When bootstrapFailed is true, it short-circuits without inspecting the error. Otherwise, isInfraError() checks the error structure. When a reset is needed, callers invoke resetCage() which nulls cagePromise. User script errors (e.g., ReferenceError: a is not defined) do not trigger resets, preserving the singleton optimization from #5800.
cachedCage → cagePromise caching: The singleton now caches the creation promise rather than the resolved instance, which is the correct pattern for async singleton initialization. This prevents duplicate FaradayCage.create() calls if acquireCage() is called while a previous creation is still in-flight (e.g., after a cage reset, or on first use when two tabs fire requests before the cage is ready). Includes .catch() to null the promise on creation failure so subsequent retries work.
Empty script short-circuit: RequestRunner.ts returns immediately (with identity envs/cookies and empty test results) when stripModulePrefix(script).trim().length === 0, avoiding WASM initialisation and preventing stale cage errors from surfacing as script_fail (#5810).
Structured console logging: RequestRunner.ts error paths now use prefixed console.error ([Pre-Request Script Error], [Post-Request Script Error], [Request Error]) for dev-console visibility when debugging sandbox failures.
SandboxTestResult type: Simplified from TestResult & { tests: TestDescriptor } & { consoleEntries?; updatedCookies } to a standalone type ({ tests: TestDescriptor; envs: TestResult["envs"]; consoleEntries?: ConsoleEntry[]; updatedCookies: Cookie[] | null }). The old intersection produced a contradictory tests: TestDescriptor[] & TestDescriptor — TypeScript allowed it structurally but every construction site always assigned a single TestDescriptor. The new type correctly reflects actual usage. Construction sites use satisfies SandboxTestResult for compile-time validation.
TaskEither contract compliance: Both node experimental runners (pre-request/experimental.ts, test-runner/experimental.ts) now wrap the entire async body in try/catch, ensuring the returned TaskEither never rejects. The refactor replaces pipe(TE.tryCatch(...)) with explicit E.left/E.right returns and a manual try/catch, preserving identical behavior while enabling the retry-on-bootstrap-error pattern. The inner helper functions format QuickJS error objects with the error name when available (e.g., TypeError: cannot read property...); the outer catch handles unexpected infrastructure throws. Web implementations (web/pre-request/index.ts, web/test-runner/index.ts) follow the same try-catch pattern for consistency.
Worker error handling: Web worker catch blocks (pre-request/worker.ts, test-runner/worker.ts) now handle non-Error throws safely via error instanceof Error ? error.message : String(error) instead of the previous unsafe (error as Error).message cast. Web test-runner worker includes updatedCookies: null in the SandboxTestResult and uses satisfies for compile-time validation.

Notes to reviewers

Try the reproduction steps mentioned in #5866 and similar patterns. Also, ensure to perform benchmarks to validate the previous performance optimisation strategies hold.

Relationship to #5800: This PR directly extends the singleton cage introduced in the memory leak fix (#5800). The key invariant preserved here: user script errors must never trigger a cage reset — only bootstrap/internal failures (corrupted QuickJS context, WASM init failure, module setup errors) cause a reset. The singleton is preserved for all normal operations.

Two-layer detection is intentional, not redundant. captureHook.bootstrapError covers errors from ctx.vm.callFunction(funcHandle) in scripting-modules.ts (L476). isInfraError() covers errors that escape before callFunction() — specifically evalCode(bootstrapCode).unwrap() (L417), createScriptingInputsObj() (L423), and defineSandboxObject() (L474). These throw during FaradayCage.runCode()'s module def() phase and propagate as { type: "error", err } without ever setting captureHook.bootstrapError. An audit of the full error flow confirmed isInfraError() is reachable, not dead code.

instanceof Error as the discriminator in isInfraError(). This is intentional, not overbroad. In the FaradayCage/QuickJS pipeline, user script errors are always plain objects (from QuickJS's dump()), while infrastructure errors are always Error instances (QuickJSUnwrapError from .unwrap(), marshal failures, etc.). The two shapes never overlap. The primary caller path only sends result.err (plain objects) through isInfraError() when captureHook.bootstrapError is not set — the instanceof Error branch is a safety net for edge-case infrastructure throws.

Test coverage:

cage.spec.ts — 5 unit tests for isInfraError() covering instanceof Error discrimination, QuickJSUnwrapError subclass detection, plain-object user error exclusions, and edge cases (null, string errors)
script-error-recovery.spec.ts — 5 integration tests: 3 running real scripts through the full FaradayCage pipeline verifying error → recovery (matching the exact reproduction steps from #5866), plus 2 singleton retry tests that inject a corrupted cage via _setCagePromiseForTesting() and verify (a) bootstrap errors trigger transparent retry on a fresh cage, and (b) user script errors do not trigger the retry path
All assertions validate actual function return values or real cage execution results — no mocked units-under-test or trivially-true checks
request.spec.ts — 3 test expectations updated to include the TypeError: prefix in error messages, matching the new error name formatting in the script runners

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/hoppscotch/hoppscotch/pull/5874 **Author:** [@jamesgeorge007](https://github.com/jamesgeorge007) **Created:** 2/12/2026 **Status:** ✅ Merged **Merged:** 2/18/2026 **Merged by:** [@jamesgeorge007](https://github.com/jamesgeorge007) **Base:** `patch` ← **Head:** `fix/sandbox-auto-recovery` --- ### 📝 Commits (1) - [`248f97c`](https://github.com/hoppscotch/hoppscotch/commit/248f97c91904af234d823921e72529419f7ad673) fix: auto-recover from corrupted sandbox state ### 📊 Changes **13 files changed** (+878 additions, -302 deletions) <details> <summary>View changed files</summary> 📝 `packages/hoppscotch-common/src/helpers/RequestRunner.ts` (+38 -2) ➕ `packages/hoppscotch-js-sandbox/src/__tests__/combined/script-error-recovery.spec.ts` (+206 -0) 📝 `packages/hoppscotch-js-sandbox/src/__tests__/hopp-namespace/request.spec.ts` (+5 -3) ➕ `packages/hoppscotch-js-sandbox/src/__tests__/utils/cage.spec.ts` (+50 -0) 📝 `packages/hoppscotch-js-sandbox/src/cage-modules/scripting-modules.ts` (+10 -8) 📝 `packages/hoppscotch-js-sandbox/src/node/pre-request/experimental.ts` (+115 -56) 📝 `packages/hoppscotch-js-sandbox/src/node/test-runner/experimental.ts` (+149 -103) 📝 `packages/hoppscotch-js-sandbox/src/types/index.ts` (+3 -1) 📝 `packages/hoppscotch-js-sandbox/src/utils/cage.ts` (+53 -10) 📝 `packages/hoppscotch-js-sandbox/src/web/pre-request/index.ts` (+111 -50) 📝 `packages/hoppscotch-js-sandbox/src/web/pre-request/worker.ts` (+5 -1) 📝 `packages/hoppscotch-js-sandbox/src/web/test-runner/index.ts` (+125 -65) 📝 `packages/hoppscotch-js-sandbox/src/web/test-runner/worker.ts` (+8 -3) </details> ### 📄 Description Closes #5866, #5810, FE-1128. Persistent errors in the experimental scripting sandbox cause two user-visible bugs: - After fixing a broken pre/post-request script, the old error continues to surface because the singleton `FaradayCage` retains a corrupted state across executions. - Requests with empty scripts report `script_fail` when the cached cage carries a stale bootstrap failure. This PR detects bootstrap/internal failures, resets the cage when needed, retries transparently on a fresh cage, and short-circuits empty scripts to avoid entering the sandbox entirely. ### What's changed - **Bootstrap error detection via `captureHook`:** Extended the `captureHook` type from `{ capture?: () => void }` to `{ capture?: () => void; bootstrapError?: unknown }` in `scripting-modules.ts`. When `ctx.vm.callFunction(bootstrapIIFE)` returns an error, it is dumped and recorded on `captureHook.bootstrapError`. Both `pre-request/index.ts` and `test-runner/index.ts` check this flag as the primary cage reset signal. - **Heuristic fallback via `isInfraError()`:** Defense-in-depth error classifier in `cage.ts` that catches errors escaping _before_ `callFunction()` is reached — specifically from `evalCode(bootstrapCode).unwrap()`, `createScriptingInputsObj()`, and `defineSandboxObject()`. Uses `instanceof Error` as the sole discriminator: infrastructure errors (QuickJSUnwrapError from `.unwrap()`, marshal failures, WASM init) are real `Error` instances, while user script errors from QuickJS's `dump()` are plain objects with `{ name, message, stack }` but are NOT `instanceof Error`. This structural difference makes the check reliable with no false positives. - **Retry-on-bootstrap-error:** When a bootstrap error is detected, the cage is reset and the script is retried once on a fresh cage within the same execution. This eliminates the regression where fixing a broken script still fails on the _next immediate_ run before succeeding on the one after. Extracted `executePreRequestOnCage()` and `executeTestOnCage()` helper functions that return `E.Either | "retry"`. The caller retries once; if the retry also fails with a bootstrap error, surfaces `"sandbox initialisation error (persistent)"`. - **Cage reset on bootstrap errors:** The reset decision is inlined at each call site as `bootstrapFailed || isInfraError(err)`. When `bootstrapFailed` is `true`, it short-circuits without inspecting the error. Otherwise, `isInfraError()` checks the error structure. When a reset is needed, callers invoke `resetCage()` which nulls `cagePromise`. User script errors (e.g., `ReferenceError: a is not defined`) do **not** trigger resets, preserving the singleton optimization from #5800. - **`cachedCage` → `cagePromise` caching:** The singleton now caches the creation _promise_ rather than the resolved instance, which is the correct pattern for async singleton initialization. This prevents duplicate `FaradayCage.create()` calls if `acquireCage()` is called while a previous creation is still in-flight (e.g., after a cage reset, or on first use when two tabs fire requests before the cage is ready). Includes `.catch()` to null the promise on creation failure so subsequent retries work. - **Empty script short-circuit:** `RequestRunner.ts` returns immediately (with identity envs/cookies and empty test results) when `stripModulePrefix(script).trim().length === 0`, avoiding WASM initialisation and preventing stale cage errors from surfacing as `script_fail` (#5810). - **Structured console logging:** `RequestRunner.ts` error paths now use prefixed `console.error` (`[Pre-Request Script Error]`, `[Post-Request Script Error]`, `[Request Error]`) for dev-console visibility when debugging sandbox failures. - **`SandboxTestResult` type:** Simplified from `TestResult & { tests: TestDescriptor } & { consoleEntries?; updatedCookies }` to a standalone type (`{ tests: TestDescriptor; envs: TestResult["envs"]; consoleEntries?: ConsoleEntry[]; updatedCookies: Cookie[] | null }`). The old intersection produced a contradictory `tests: TestDescriptor[] & TestDescriptor` — TypeScript allowed it structurally but every construction site always assigned a single `TestDescriptor`. The new type correctly reflects actual usage. Construction sites use `satisfies SandboxTestResult` for compile-time validation. - **TaskEither contract compliance:** Both node experimental runners (`pre-request/experimental.ts`, `test-runner/experimental.ts`) now wrap the entire async body in `try/catch`, ensuring the returned `TaskEither` never rejects. The refactor replaces `pipe(TE.tryCatch(...))` with explicit `E.left`/`E.right` returns and a manual `try/catch`, preserving identical behavior while enabling the retry-on-bootstrap-error pattern. The inner helper functions format QuickJS error objects with the error `name` when available (e.g., `TypeError: cannot read property...`); the outer catch handles unexpected infrastructure throws. Web implementations (`web/pre-request/index.ts`, `web/test-runner/index.ts`) follow the same try-catch pattern for consistency. - **Worker error handling:** Web worker catch blocks (`pre-request/worker.ts`, `test-runner/worker.ts`) now handle non-Error throws safely via `error instanceof Error ? error.message : String(error)` instead of the previous unsafe `(error as Error).message` cast. Web test-runner worker includes `updatedCookies: null` in the `SandboxTestResult` and uses `satisfies` for compile-time validation. ### Notes to reviewers Try the reproduction steps mentioned in #5866 and similar patterns. Also, ensure to perform benchmarks to validate the previous performance optimisation strategies hold. **Relationship to #5800:** This PR directly extends the singleton cage introduced in the memory leak fix (#5800). The key invariant preserved here: user script errors must _never_ trigger a cage reset — only bootstrap/internal failures (corrupted QuickJS context, WASM init failure, module setup errors) cause a reset. The singleton is preserved for all normal operations. **Two-layer detection is intentional, not redundant.** `captureHook.bootstrapError` covers errors from `ctx.vm.callFunction(funcHandle)` in `scripting-modules.ts` (L476). `isInfraError()` covers errors that escape _before_ `callFunction()` — specifically `evalCode(bootstrapCode).unwrap()` (L417), `createScriptingInputsObj()` (L423), and `defineSandboxObject()` (L474). These throw during `FaradayCage.runCode()`'s module `def()` phase and propagate as `{ type: "error", err }` without ever setting `captureHook.bootstrapError`. An audit of the full error flow confirmed `isInfraError()` is reachable, not dead code. **`instanceof Error` as the discriminator in `isInfraError()`.** This is intentional, not overbroad. In the FaradayCage/QuickJS pipeline, user script errors are always plain objects (from QuickJS's `dump()`), while infrastructure errors are always `Error` instances (QuickJSUnwrapError from `.unwrap()`, marshal failures, etc.). The two shapes never overlap. The primary caller path only sends `result.err` (plain objects) through `isInfraError()` when `captureHook.bootstrapError` is not set — the `instanceof Error` branch is a safety net for edge-case infrastructure throws. **Test coverage:** - `cage.spec.ts` — 5 unit tests for `isInfraError()` covering `instanceof Error` discrimination, QuickJSUnwrapError subclass detection, plain-object user error exclusions, and edge cases (null, string errors) - `script-error-recovery.spec.ts` — 5 integration tests: 3 running real scripts through the full `FaradayCage` pipeline verifying error → recovery (matching the exact reproduction steps from #5866), plus 2 singleton retry tests that inject a corrupted cage via `_setCagePromiseForTesting()` and verify (a) bootstrap errors trigger transparent retry on a fresh cage, and (b) user script errors do not trigger the retry path - All assertions validate actual function return values or real cage execution results — no mocked units-under-test or trivially-true checks - `request.spec.ts` — 3 test expectations updated to include the `TypeError:` prefix in error messages, matching the new error name formatting in the script runners --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>