Add debug stack trace#239
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> Sequence Diagram(s)sequenceDiagram
participant Client_UI
participant Main_Thread
participant Worker_Process
participant WASM_Module
participant Debug_Service
Client_UI->>Main_Thread: createCompletion(stream:true)
Main_Thread->>Worker_Process: post start request
Worker_Process->>WASM_Module: invoke model (may abort/oob)
WASM_Module->>Worker_Process: printErr "@@STACK@@" and onAbort
Worker_Process->>Main_Thread: post signal.abort [type,message,lastStack]
Main_Thread->>Debug_Service: decodeStackTrace(rawStack, isCompat)
Debug_Service-->>Main_Thread: annotated stack
Main_Thread->>Client_UI: reject with WllamaRuntimeError(message, annotatedStack)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
README-dev.md (1)
134-150: 💤 Low valueConsider adding language specifier to satisfy linter.
The fenced code block containing the binary format diagram does not specify a language, triggering a linter warning. While the ASCII diagram is not executable code, you can add
textas the language specifier to satisfy the linter:-``` +```text ┌──────────────────────────────────────────────────────────┐ │ HEADER (12 bytes) │🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README-dev.md` around lines 134 - 150, The fenced ASCII diagram block lacks a language tag and triggers the linter; update the code fence around the diagram in the README-dev.md to include a language specifier (use "text") by changing the opening triple-backticks to ```text so the linter recognizes it as a non-code block while preserving the ASCII diagram content.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/build_source_map.js`:
- Line 257: The inline comment that says "per-function: u8 nameLen + bytes (0 =
unknown)" is incorrect; update it to match the actual encoder which writes a u32
numNames, then a deduplicated name table, then a u16 index array (with 0xFFFF
meaning unknown) instead of per-function length-prefixed names. Locate the
comment near the name/index section in build_source_map.js (the line currently
documenting "u8 nameLen + bytes") and replace it with a concise description: u32
firstId, u32 funcCount, u32 numNames, [deduplicated name table], then u16
per-function name indices (0xFFFF = unknown), so future decoders match the real
encoding.
- Around line 21-27: When parsing CLI flags in build_source_map.js, guard
accesses to args[++i] for the '--input' and '--output' branches to avoid
out-of-bounds reads: before incrementing i, verify that args[i+1] exists and is
non-empty; for '--input' ensure args[i+1] contains a ':' and both parts are
present before splitting and pushing into inputs, and for '--output' ensure
args[i+1] exists before assigning to outputFile; if the value is missing, print
a clear error (same behavior as current validation) and call process.exit(1).
Use the existing symbols args, inputs, outputFile, resolve to locate and update
the logic around those branches.
In `@src/debug.ts`:
- Around line 72-88: Update the JSDoc block above the wasm stack trace annotator
to accurately describe what the function actually emits (e.g., "wasm-func[id]
(name)" or "func[id] -> wasm-func[id]" instead of claiming it decodes original
source file and line like `common.cpp:123`); edit the comment block in
src/debug.ts (the JSDoc immediately preceding the wasm stack trace annotation
function) so examples and the "Example output" reflect the real output format
produced by the function (leave implementation unchanged).
- Around line 19-21: The DecompressionStream writer calls (writer.write and
writer.close) are not awaited so errors or backpressure can be missed; update
the block around writer, gzipped, ds and buf to await the Promises from
writer.write(gzipped) and writer.close() (or use await writer.ready if
appropriate) before creating the Response from ds.readable and calling
arrayBuffer(), ensuring write completion and proper error propagation for the
DecompressionStream flow.
In `@src/wllama.test.ts`:
- Around line 341-347: The test around wllama.createCompletion currently
swallows the case where no exception is thrown; after the await call add an
explicit failure if execution reaches past createCompletion (e.g., call
fail('Expected createCompletion to throw') or throw new Error(...)) so the test
fails when no error occurs, and apply the same change to the other occurrence
around lines 359-364; keep the existing catch assertions that check (e as
Error).name and .stack.
In `@src/worker.ts`:
- Around line 418-428: The abort method currently only rejects promises in
resultQueue leaving entries in taskQueue unresolved; update the abort(text,
stack) implementation to also iterate over this.taskQueue and reject each queued
task with the same WllamaRuntimeError (using text fallback '(unknown error)' and
the provided stack) so no tasks remain pending after a terminal failure; ensure
you reference and reject the same task objects/types used in taskQueue (same
promise rejectors used elsewhere) and keep the existing behavior for clearing
resultQueue.
- Around line 372-389: The abort branch currently returns early and skips
calling this.abort, leaving pending operations unresolved; instead handle
'abort' by setting stack appropriately (e.g., convert rawStack to
newline-separated form or use rawStack as-is), then proceed to call
Debug.decodeStackTrace(stack, isCompatBuild) and invoke this.abort(newMsg,
decoded) just like the 'exception' path. Update the async IIFE around the tuple
unpacking of args ([signalType, message, rawStack]) so both signalType ===
'abort' and 'exception' set a stack value and continue to decoding and calling
this.abort (methods referenced: Debug.decodeStackTrace and this.abort).
In `@src/workers-code/llama-cpp.js`:
- Around line 310-314: The handleError function should guard against non-Error
throw values before accessing message/stack; instead of calling
err.stack.toString() directly, construct safe message and stack strings (e.g.,
when err is a string or non-object use String(err) for message and fallback
stack to the same or an empty string) and then call msg({ verb: 'signal.abort',
args: [ 'exception', safeMessage, safeStack ] }); locate handleError in
llama-cpp.js and replace direct property access with these guarded conversions
to avoid exceptions when non-Error values are thrown.
---
Nitpick comments:
In `@README-dev.md`:
- Around line 134-150: The fenced ASCII diagram block lacks a language tag and
triggers the linter; update the code fence around the diagram in the
README-dev.md to include a language specifier (use "text") by changing the
opening triple-backticks to ```text so the linter recognizes it as a non-code
block while preserving the ASCII diagram content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a8f7f99-b4a5-4cbd-8be4-5eb1bd1dad99
⛔ Files ignored due to path filters (1)
src/wasm/wllama.wasmis excluded by!**/*.wasm
📒 Files selected for processing (22)
CMakeLists.txtREADME-dev.mdcompat/wasm/wllama.jscpp/glue.hppcpp/wllama-context.hexamples/basic/index.htmlexamples/main/src/utils/wllama.context.tsxllama.cppscripts/build_source_map.jsscripts/docker-compose.ymlsrc/debug.tssrc/glue/messages.tssrc/types/oai-compat.tssrc/types/types.tssrc/utils.tssrc/wasm/source-map.tssrc/wasm/wllama.jssrc/wllama.test.tssrc/wllama.tssrc/worker.tssrc/workers-code/generated.tssrc/workers-code/llama-cpp.js
💤 Files with no reviewable changes (2)
- src/types/types.ts
- examples/main/src/utils/wllama.context.tsx
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/wllama.test.ts`:
- Around line 4-8: The top-level condition in src/wllama.test.ts uses
process.env.GITHUB_ACTIONS unguarded and can throw in browser test runs; change
the if-check to first verify process is defined (e.g., typeof process !==
"undefined") and that process.env.GITHUB_ACTIONS is truthy before registering
the afterEach hook (the block referencing afterEach and
process.env.GITHUB_ACTIONS should be updated accordingly) so tests running in
browser contexts won't hit a ReferenceError.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f7b5259-63e1-47a7-ab99-0b0996adacee
📒 Files selected for processing (7)
scripts/build_source_map.jssrc/debug.tssrc/wasm/source-map.tssrc/wllama.test.tssrc/worker.tssrc/workers-code/generated.tssrc/workers-code/llama-cpp.js
💤 Files with no reviewable changes (1)
- src/wasm/source-map.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/debug.ts
- src/worker.ts
- src/workers-code/llama-cpp.js
- scripts/build_source_map.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a very tiny
source-map.ts(~73KB) to each build in order to trace back the wasm function name.Upon receiving an error, a stack trace will be printed to console:
User can also use this in downstream code:
And for streaming async iterator case:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
API
Tests