fix: prevent silent failures when releasing stream reader locks#3397
fix: prevent silent failures when releasing stream reader locks#3397meenaksh06 wants to merge 1 commit intotriggerdotdev:mainfrom
Conversation
|
|
Hi @meenaksh06, thanks for your interest in contributing! This project requires that pull request authors are vouched, and you are not in the list of vouched users. This PR will be closed automatically. See https://github.com/triggerdotdev/trigger.dev/blob/main/CONTRIBUTING.md for more details. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughUpdates three stream handler files to conditionally emit console warnings when Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| if (debug) { | ||
| // fallback if no logger available | ||
| console.warn("Failed to release stream reader lock", error); | ||
| } |
There was a problem hiding this comment.
🔴 Undefined debug variable in safeReleaseLock causes ReferenceError, defeating error safety
safeReleaseLock is a standalone module-level function, not a class method. The new code references debug in the catch block (if (debug)), but debug is never declared, imported, or passed as a parameter in this file. If reader.releaseLock() throws, the catch block will itself throw a ReferenceError: debug is not defined, converting a safely-caught error into an uncaught crash — the exact opposite of the function's purpose.
| if (debug) { | |
| // fallback if no logger available | |
| console.warn("Failed to release stream reader lock", error); | |
| } | |
| if (typeof debug !== "undefined" && debug) { | |
| // fallback if no logger available | |
| console.warn("Failed to release stream reader lock", error); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (debug) { | ||
| // fallback if no logger available | ||
| console.warn("Failed to release stream reader lock", error); | ||
| } |
There was a problem hiding this comment.
🔴 Undefined debug variable in safeReleaseLock causes ReferenceError, defeating error safety
Same issue as in streamInstance.ts. The standalone safeReleaseLock function references debug which is not in scope. The StreamsWriterV1 class has no debug property at all, and even if it did, this is not a class method so this.debug is inaccessible. If releaseLock() throws, the catch handler will crash with ReferenceError.
| if (debug) { | |
| // fallback if no logger available | |
| console.warn("Failed to release stream reader lock", error); | |
| } | |
| if (typeof debug !== "undefined" && debug) { | |
| // fallback if no logger available | |
| console.warn("Failed to release stream reader lock", error); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (debug) { | ||
| // fallback if no logger available | ||
| console.warn("Failed to release stream reader lock", error); | ||
| } |
There was a problem hiding this comment.
🔴 Undefined debug variable in safeReleaseLock causes ReferenceError, defeating error safety
Same issue as the other two files. The StreamsWriterV2 class has private readonly debug: boolean at line 52, but the standalone safeReleaseLock function at module scope cannot access class instance properties. The reference to bare debug at line 216 will throw ReferenceError if releaseLock() fails.
| if (debug) { | |
| // fallback if no logger available | |
| console.warn("Failed to release stream reader lock", error); | |
| } | |
| if (typeof debug !== "undefined" && debug) { | |
| // fallback if no logger available | |
| console.warn("Failed to release stream reader lock", error); | |
| } |
Was this helpful? React with 👍 or 👎 to provide feedback.
| function safeReleaseLock(reader: ReadableStreamDefaultReader<any>) { | ||
| try { | ||
| reader.releaseLock(); | ||
| } catch (error) {} | ||
| } catch (error) { | ||
| if (debug) { | ||
| // fallback if no logger available | ||
| console.warn("Failed to release stream reader lock", error); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🚩 Design consideration: safeReleaseLock has no access to debug context
Even after fixing the ReferenceError, the underlying design issue remains: safeReleaseLock is a standalone function with no way to know whether debug mode is enabled. In streamsWriterV2.ts, the class has this.debug (streamsWriterV2.ts:52), and in streamInstance.ts, the options have debug (streamInstance.ts:17), but streamsWriterV1.ts has no debug concept at all. A clean fix would either: (a) pass debug as a parameter to safeReleaseLock, (b) always log the warning unconditionally (it's in a catch block so it only fires on errors), or (c) extract it to a shared util that accepts a debug flag.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Closes #
✅ Checklist
Testing
reader.releaseLock()does not crash the application when an error occurs.Changelog
reader.releaseLock()to prevent silent failures.