Skip to content

fix(transport): handle cross-realm AbortSignal - #383

Open
AdamsGH wants to merge 1 commit into
s2b-dev:devfrom
AdamsGH:fix/electron-net-cross-realm-abort-signal
Open

fix(transport): handle cross-realm AbortSignal#383
AdamsGH wants to merge 1 commit into
s2b-dev:devfrom
AdamsGH:fix/electron-net-cross-realm-abort-signal

Conversation

@AdamsGH

@AdamsGH AdamsGH commented Aug 14, 2026

Copy link
Copy Markdown

Summary

Fix chat requests failing on Obsidian 1.13.7 with Electron 43.1.1, plus two failure paths found while reproducing the renderer crash.

The transport failure is reproducible by passing a renderer AbortSignal through electron.remote.net.fetch. Electron validates it against the main process realm and throws:

RequestInit: Expected signal (...) to be an instance of AbortSignal.

Changes

  • Keep Electron net.fetch as the CORS bypass and preserve incremental streaming.
  • Remove the renderer AbortSignal only from the Electron RequestInit boundary.
  • Keep the signal for renderer fetch, check cancellation before Electron fetch, and cancel a response received after the renderer signal aborts.
  • Remove the direct loopback requestUrl branch. Local providers keep incremental streaming.
  • Catch transport failures raised while agent.stream() initializes, not only while consuming the stream.
  • Apply the initialization fix to new, edited, and regenerated messages.
  • Limit tool output UI previews to 20,000 characters before markdown rendering. Full output remains in checkpoints and model context.

Verification

  • bun run test: 1000 tests passed across 64 files
  • bun run build: production build succeeded
  • bun format: clean
  • Live runtime reproduction: Obsidian 1.13.7, Electron 43.1.1, Chrome 150, Node 24.18.0
  • Regression coverage for Electron signal removal, renderer signal preservation, stream initialization fallback, and large tool output previews

bun check reports the same 18 diagnostics present on dev, all in unchanged files. They cover missing Node builtin declarations and nullable Pixi screen coordinates.

@greptile-apps

greptile-apps Bot commented Aug 14, 2026

Copy link
Copy Markdown

Greptile Summary

Fixes Electron OpenAI-compatible requests by removing renderer-owned AbortSignals before calling cross-realm electron.net.fetch, while retaining cancellation checks around the request.

  • Adds Electron-specific RequestInit normalization without changing the browser fetch path.
  • Cancels and rejects responses when cancellation occurs before Electron fetch resolves.
  • Adds regression tests for signal removal and preservation of signal-free request identity.

Confidence Score: 5/5

The PR appears safe to merge with no actionable defects identified.

The Electron-only normalization addresses the cross-realm constructor mismatch, preserves the browser fetch path, and retains the cancellation behavior explicitly supported by the renderer and LangChain layers.

Important Files Changed

Filename Overview
src/lib/aiTransport.ts Removes cross-realm signals only from Electron fetch requests and adds pre-request and post-response cancellation enforcement without altering browser fetch signaling.
test/lib/aiTransportElectronSignal.test.ts Verifies signal removal, preservation of other request fields, non-mutation of the input, and identity preservation for signal-free initialization objects.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Renderer RequestInit with AbortSignal] --> B{Signal already aborted?}
    B -->|Yes| C[Throw abort reason]
    B -->|No| D{Electron net.fetch available?}
    D -->|No| E[Browser fetch with native signal]
    D -->|Yes| F[Remove cross-realm signal]
    F --> G[Electron net.fetch]
    G --> H{Renderer signal aborted before response?}
    H -->|Yes| I[Cancel response body and throw]
    H -->|No| J[Return normalized response]
Loading

Reviews (1): Last reviewed commit: "fix(transport): omit cross-realm AbortSi..." | Re-trigger Greptile

@AdamsGH
AdamsGH force-pushed the fix/electron-net-cross-realm-abort-signal branch from b9a5c6c to d8eda52 Compare August 14, 2026 10:41
@AdamsGH AdamsGH changed the title fix(transport): handle cross-realm AbortSignal in Electron fetch fix(transport): avoid Electron remote fetch in renderer Aug 14, 2026
@AdamsGH
AdamsGH force-pushed the fix/electron-net-cross-realm-abort-signal branch 3 times, most recently from ef8f1a4 to b8413cd Compare August 14, 2026 11:22
@Leo310

Leo310 commented Aug 15, 2026

Copy link
Copy Markdown
Member

@AdamsGH thanks for the detailed writeup! The agent.stream()-inside-try fix is a good catch (init-time transport failures were escaping the catch entirely, so the buffered fallback was dead code for that case), and the 20k UI cap looks right. I'd take both.

I'm hesitant about dropping net.fetch though. It was added in #267 specifically as our CORS bypass — it runs outside the renderer's origin sandbox, so no preflight. I tested it here against a local server that rejects preflight: remote.net.fetch returns 200 and streams incrementally (9 chunks ~250ms apart), while renderer fetch fails with Failed to fetch. So removing it means CORS-blocked providers lose token streaming and pay a failed request plus a full turn restart per message.

I also couldn't reproduce the Expected signal (...) to be an instance of AbortSignal error, a renderer AbortSignal worked fine in my testing. What Obsidian/Electron version are you on, and do you have the stack trace? Want to make sure I'm not missing something version-specific before we drop the bypass.

One other thing on the loopback branch: it's the only requestUrlFetch call site that skips disableStreaming. A buffered SSE body reads as a single chunk, so local providers (Ollama, LM Studio) would silently lose incremental streaming, no crash, just a wait then the full reply.

@AdamsGH
AdamsGH force-pushed the fix/electron-net-cross-realm-abort-signal branch from b8413cd to 8ffb1b6 Compare August 17, 2026 15:00
@AdamsGH AdamsGH changed the title fix(transport): avoid Electron remote fetch in renderer fix(transport): handle cross-realm AbortSignal Aug 17, 2026
@AdamsGH

AdamsGH commented Aug 17, 2026

Copy link
Copy Markdown
Author

Confirmed, this is version-specific. I am on Obsidian 1.13.7 with Electron 43.1.1, Chrome 150, and Node 24.18.0.

I can reproduce it directly in the Obsidian renderer with:

require("electron").remote.net.fetch("http://127.0.0.1:1", {
  signal: new AbortController().signal,
})

The relevant stack is:

TypeError: RequestInit: Expected signal (...) to be an instance of AbortSignal.
    at webidl.errors.exception (node:internal/deps/undici/undici:4852:14)
    at Object.AbortSignal (node:internal/deps/undici/undici:5111:31)
    at Object.RequestInit (node:internal/deps/undici/undici:5145:21)
    at new Request (node:internal/deps/undici/undici:12047:34)
    at fetchWithSession (node:electron/js2c/browser_init:2:54432)
    at Session.fetch (node:electron/js2c/browser_init:2:65943)
    at Object.fetch (node:electron/js2c/browser_init:2:56732)
    at @electron/remote/dist/src/main/server.js:480:71

Agreed on both transport points. I restored net.fetch, removed the loopback requestUrl branch, and now strip only the renderer signal at the Electron remote boundary. Renderer fetch still receives its signal. Electron fetch checks cancellation before the request and cancels a response received after the renderer signal aborts.

The PR now keeps CORS bypass and incremental streaming. The agent.stream() initialization fix and 20k UI cap remain unchanged.

@Leo310 Leo310 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good — checked out the latest revision, ran the full suite (1001 tests / 64 files passing, one net new), and did a production build (clean). Also verified the core claim live: raw net.fetch against a local server that rejects CORS preflight still bypasses it and streams incrementally (7 chunks at ~250ms intervals matching the server's emission cadence).

The fix is properly scoped now — normalizeElectronNetRequestInit strips only signal, only on the Electron leg; the renderer-only path is untouched and still relies on native fetch's own signal handling. Cancellation isn't dropped, just replaced with explicit throwIfAborted() checks before/after the Electron call, and a mid-flight abort is still caught by the per-chunk options.signal?.aborted check in Agent.ts on the next pull.

agent.stream()-inside-try and the 20k UI cap are unchanged and still correct.

Thanks for chasing down the repro on 1.13.7/Electron 43.1.1 — that was the missing piece.

@Leo310

Leo310 commented Aug 22, 2026

Copy link
Copy Markdown
Member

Heads up — this now conflicts with dev. I resolved it locally to check it's tractable; it is, but there are two traps worth flagging before you rebase.

Cause: dev refactored streamTokens, editFromCheckpoint and regenerateFromCheckpoint into a single shared runStream method (#411 landed in between). Your fix was applied three times, once per copy. Only src/agent/Agent.ts genuinely conflicts.

Resolution: apply the fix once inside runStream, and take dev's side for the other two blocks — their bodies are now just yield* this.runStream({...}). The Agent.ts diff drops from ~40 lines to 17:

 		let preambleAccumulator = "";
 		try {
+			const stream = bindAsyncIterableToTransportContext(
+				await runWithAiTransportContext(transportContext, () =>
+					agent.stream(input, {
+						...invokeConfig,
+						streamMode: ["messages", "tools", "values"] as const,
+					}),
+				),
+				transportContext,
+			);
 			for await (const chunk of stream) {

Two things git gets wrong on its own:

  1. streamInput is undefined. Auto-merge carries your call through as agent.stream(streamInput, ...), but streamInput was a local in the old streamTokens — the shared method's parameter is input. Hard compile error until renamed.

  2. Duplicate requestUrl export in test/__mocks__/obsidian.ts. Git reports this file as auto-merged cleanly, but your bare vi.fn() and dev's newer typed version both survive, and esbuild fails with Multiple exports with the same name "requestUrl". It surfaces as 44 failed test files, which reads like flaky infra rather than a merge defect — worth knowing so you don't chase it. Drop the bare one; dev's is typed with a default return. (This was the minor nit I raised on the first pass; dev has since fixed it properly.)

After resolving: bun run check 0 errors, bun run test 1551 passed / 95 files, production build clean. All four changes survive intact — runWithAiTransportContext appears exactly once in runStream, normalizeElectronNetRequestInit present, loopback branch absent, 20k cap present.

Approval still stands, just needs the rebase.

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