fix(transport): handle cross-realm AbortSignal - #383
Conversation
Greptile SummaryFixes Electron OpenAI-compatible requests by removing renderer-owned AbortSignals before calling cross-realm
Confidence Score: 5/5The 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
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]
Reviews (1): Last reviewed commit: "fix(transport): omit cross-realm AbortSi..." | Re-trigger Greptile |
b9a5c6c to
d8eda52
Compare
ef8f1a4 to
b8413cd
Compare
|
@AdamsGH thanks for the detailed writeup! The I'm hesitant about dropping I also couldn't reproduce the One other thing on the loopback branch: it's the only |
b8413cd to
8ffb1b6
Compare
|
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: Agreed on both transport points. I restored The PR now keeps CORS bypass and incremental streaming. The |
Leo310
left a comment
There was a problem hiding this comment.
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.
|
Heads up — this now conflicts with Cause: Resolution: apply the fix once inside 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:
After resolving: Approval still stands, just needs the rebase. |
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
AbortSignalthroughelectron.remote.net.fetch. Electron validates it against the main process realm and throws:Changes
net.fetchas the CORS bypass and preserve incremental streaming.AbortSignalonly from the ElectronRequestInitboundary.requestUrlbranch. Local providers keep incremental streaming.agent.stream()initializes, not only while consuming the stream.Verification
bun run test: 1000 tests passed across 64 filesbun run build: production build succeededbun format: cleanbun checkreports the same 18 diagnostics present ondev, all in unchanged files. They cover missing Node builtin declarations and nullable Pixi screen coordinates.