set up webassembly runtime #363
Conversation
…dAt to be changed
search should exist on its own and it's always available as a feature so this makes dealing with enabled/disabled semdex simpler by removing the checks from the semdex impl and reducing the complexity of re-implementing the semdexer interface
…kind filtering for searches
Improve search filtering
upgrade orval to 7.20
Update module github.com/go-resty/resty/v2 to v2.16.0
account edge for question optional
- currently only applies to single thread get - still exploring internal API design for this - will maybe look at proper caching layer too
- server fetch is force-cache, 60s revalidation - client fetch is default behaviour which will trigger HTTP Conditional Requests
implement conditional requests
apply cache headers to some most used routes
Rework asker
…ving this specified with no issues...
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 2 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive plugin management system in Go, focusing on WebAssembly (Wasm) plugin handling. The implementation spans multiple packages including Changes
Sequence DiagramsequenceDiagram
participant Writer
participant Store
participant Runner
participant Reader
Writer->>Runner: Validate Plugin Binary
Runner-->>Writer: Return Metadata/Error
Writer->>Store: Write Plugin
Reader->>Store: List Plugins
Store-->>Reader: Return Plugin Files
Reader->>Runner: Validate Each Plugin
Runner-->>Reader: Return Plugin Metadata
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
internal/infrastructure/wrun/wasm.go (1)
91-96: Check scanner errors for reliability.
The scanner loop does not checkscanner.Err()to detect read failures or partial lines. Handling possible I/O errors could help diagnose runtime issues.You can add an error check right after the loop:
for s.Scan() { outputs = append(outputs, s.Bytes()) } +if s.Err() != nil { + return nil, fault.Wrap(s.Err(), fmsg.With("error reading output from module")) +}internal/infrastructure/wrun/testdata/test2.go (1)
12-20: Consider handling scan errors.
Currently, the code returns immediately ifs.Scan()is false, skipping potential error details. Checkings.Err()could improve debugging in case of input read failures.You could do:
if !s.Scan() { - return + if err := s.Err(); err != nil { + fmt.Fprintln(os.Stderr, "Error reading input:", err) + } + return }internal/infrastructure/object/object.go (1)
16-16: Provide pagination or filtering options for scalability.
When listing objects using a prefix, the list could grow large. Consider a paginated or stream-based approach in future iterations for systems handling large numbers of keys to avoid potential memory overhead.app/resources/plugin/plugin.go (2)
28-31: Consider adding validation tags to Metadata struct.Adding validation tags would make the requirements more explicit and enable using validation libraries.
type Metadata struct { - Name string - Version string + Name string `validate:"required"` + Version string `validate:"required,semver"` }
38-42: Consider adding validation state to Available struct.The Error field alone might not provide enough context about the validation state.
type Available struct { StoragePath string Loaded *Package Error error + State ValidationState } +type ValidationState string + +const ( + ValidationStateUnknown ValidationState = "" + ValidationStateValid ValidationState = "valid" + ValidationStateInvalid ValidationState = "invalid" + ValidationStateError ValidationState = "error" +)app/resources/plugin/plugin_reader/reader.go (1)
32-66: Consider adding concurrent processing for better performance.The List method could benefit from parallel processing when dealing with multiple plugins.
+func (r *Reader) List(ctx context.Context) ([]*plugin.Available, error) { + files, err := r.store.List(ctx, plugin.PluginDirectory) + if err != nil { + return nil, fault.Wrap(err, fctx.With(ctx)) + } + + // Process plugins concurrently with a worker pool + const maxWorkers = 4 + sem := make(chan struct{}, maxWorkers) + var wg sync.WaitGroup + ps := make([]*plugin.Available, len(files)) + + for i, s := range files { + wg.Add(1) + go func(i int, s string) { + defer wg.Done() + sem <- struct{}{} // Acquire + defer func() { <-sem }() // Release + + // ... rest of the processing logic ... + }(i, s) + } + + wg.Wait() + return ps, nil +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.suminternal/infrastructure/wrun/testdata/test1.wasmis excluded by!**/*.wasm,!**/*.wasminternal/infrastructure/wrun/testdata/test2.wasmis excluded by!**/*.wasm,!**/*.wasm
📒 Files selected for processing (11)
app/resources/plugin/plugin.go(1 hunks)app/resources/plugin/plugin_reader/reader.go(1 hunks)app/resources/plugin/plugin_writer/writer.go(1 hunks)app/services/plugin/plugin_manager/manager.go(1 hunks)internal/infrastructure/object/local.go(1 hunks)internal/infrastructure/object/object.go(1 hunks)internal/infrastructure/object/s3.go(1 hunks)internal/infrastructure/wrun/testdata/test1.go(1 hunks)internal/infrastructure/wrun/testdata/test2.go(1 hunks)internal/infrastructure/wrun/wasm.go(1 hunks)internal/infrastructure/wrun/wasm_test.go(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/services/plugin/plugin_manager/manager.go
- internal/infrastructure/wrun/testdata/test1.go
🔇 Additional comments (6)
internal/infrastructure/wrun/wasm.go (2)
20-22: Looks good on constructor.
No issues found with this constructor; it appears clean and straightforward.
28-36: Wazero runtime initialization looks fine.
Instantiating the WASI snapshot is done correctly. This is well-aligned with typical wazero usage.app/resources/plugin/plugin.go (1)
14-26: Add error handling for invalid metadata JSON format.The Validate method should verify the required fields in metadata after JSON unmarshaling to ensure plugin validity.
Add validation after unmarshaling:
func (b Binary) Validate(ctx context.Context, r wrun.Runner) (*Metadata, error) { mb, err := r.RunOnce(ctx, b, nil) if err != nil { return nil, err } var m Metadata if err = json.Unmarshal(mb, &m); err != nil { return nil, err } + if m.Name == "" || m.Version == "" { + return nil, fmt.Errorf("invalid plugin metadata: name and version are required") + } + return &m, nil }✅ Verification successful
The review comment is correct - Name and Version fields must be validated
The Metadata struct requires both Name and Version fields, but the current implementation doesn't validate them after JSON unmarshaling. The suggested validation is necessary to ensure plugin validity.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find Metadata struct definition ast-grep --pattern 'type Metadata struct { $$$ }' # Search for any existing validation patterns rg "func.*Validate.*Metadata" -A 5 # Look for Metadata usage rg "Metadata" -B 2 -A 2Length of output: 74949
internal/infrastructure/wrun/wasm_test.go (2)
12-28: Add error test cases for RunOnce.The test only covers the happy path. Add test cases for invalid WASM binaries and execution failures.
+func Test_wazeroRunner_RunOnce_InvalidWASM(t *testing.T) { + r := require.New(t) + ctx := context.Background() + runner := newWazeroRunner(ctx) + + _, err := runner.RunOnce(ctx, []byte("invalid wasm"), nil) + r.Error(err) +}✅ Verification successful
The review comment is correct - error test cases are needed
The implementation has multiple error paths that are currently untested:
- Invalid WASM binary handling
- Module instantiation failures
- No output scenarios
- Command encoding errors
The suggested test case in the review comment would help improve coverage of error scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Find implementation and related test files fd wasm.go fd wasm_test.go # Look for error handling in implementation ast-grep --pattern 'func ($runner *wazeroRunner) RunOnce($ctx context.Context, $data []byte, $input interface{}) ([]byte, error) { $$$ }' # Check test fixtures ls -la internal/infrastructure/wrun/testdata/Length of output: 667
Script:
#!/bin/bash # Check implementation file for error handling cat internal/infrastructure/wrun/wasm.go # Look for existing error test cases cat internal/infrastructure/wrun/wasm_test.go # Check test fixture source cat internal/infrastructure/wrun/testdata/test1.go cat internal/infrastructure/wrun/testdata/test2.goLength of output: 3578
30-50: Add timeout test case for RunOnce.Test should verify that the context timeout is respected during WASM execution.
+func Test_wazeroRunner_RunOnce_Timeout(t *testing.T) { + r := require.New(t) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + runner := newWazeroRunner(ctx) + f, err := os.ReadFile("./testdata/infinite_loop.wasm") + r.NoError(err) + + _, err = runner.RunOnce(ctx, f, nil) + r.ErrorIs(err, context.DeadlineExceeded) +}✅ Verification successful
Modify timeout test to use existing WASM file
The timeout test suggestion is valid, but needs to use an existing WASM file (test1.wasm or test2.wasm) instead of the non-existent infinite_loop.wasm. The test can still verify timeout behavior with a very short timeout duration:
func Test_wazeroRunner_RunOnce_Timeout(t *testing.T) { r := require.New(t) ctx, cancel := context.WithTimeout(context.Background(), 1*time.Nanosecond) defer cancel() runner := newWazeroRunner(ctx) f, err := os.ReadFile("./testdata/test1.wasm") r.NoError(err) _, err = runner.RunOnce(ctx, f, nil) r.ErrorIs(err, context.DeadlineExceeded) }🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check if infinite_loop.wasm exists fd infinite_loop.wasm # Check RunOnce implementation and existing timeout tests ast-grep --pattern 'func (r *wazeroRunner) RunOnce(ctx context.Context, $$$) { $$$ }' # Search for timeout-related tests rg -A 5 "context.*WithTimeout.*wazero" --type goLength of output: 164
Script:
#!/bin/bash # Find all WASM files fd .wasm # Check the implementation file rg -A 10 "type wazeroRunner struct" --type go # Look for RunOnce implementation with broader pattern rg -A 10 "func.*RunOnce.*context\.Context" --type go # Find all test files in the same directory fd ".*_test\.go$" --type fLength of output: 3667
internal/infrastructure/object/s3.go (1)
99-114: 🛠️ Refactor suggestionOptimize resource management and error handling.
The current implementation has several areas for improvement:
- The
doneChis created but not used, making it unnecessary.- Context cancellation is not properly handled.
- For buckets with many objects, consider implementing pagination.
Consider this optimized implementation:
func (s *s3Storer) List(ctx context.Context, prefix string) ([]string, error) { - doneCh := make(chan struct{}) - defer close(doneCh) - var objects []string for object := range s.minioClient.ListObjects(ctx, s.bucket, minio.ListObjectsOptions{ Prefix: prefix, + // Optional: Add pagination + MaxKeys: 1000, }) { + // Check for context cancellation + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + } + if object.Err != nil { return nil, fault.Wrap(object.Err, fctx.With(ctx)) } objects = append(objects, object.Key) } return objects, nil }Let's verify the MinIO client's behavior with context cancellation:
| o := outputs[len(outputs)-1] | ||
|
|
||
| if len(outputs) == 0 { | ||
| if command == nil { | ||
| return nil, fault.New("no output received from module: expected a manifest") | ||
| } else { | ||
| return nil, fault.New("no output received from module: expected a command response") | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix potential out-of-range panic.
outputs[len(outputs)-1] at line 97 is accessed before verifying that len(outputs) > 0, triggering a panic when no lines are produced by the module.
Below is a suggested fix, checking len(outputs) first:
- o := outputs[len(outputs)-1]
-
if len(outputs) == 0 {
if command == nil {
return nil, fault.New("no output received from module: expected a manifest")
} else {
return nil, fault.New("no output received from module: expected a command response")
}
}
+o := outputs[len(outputs)-1]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| o := outputs[len(outputs)-1] | |
| if len(outputs) == 0 { | |
| if command == nil { | |
| return nil, fault.New("no output received from module: expected a manifest") | |
| } else { | |
| return nil, fault.New("no output received from module: expected a command response") | |
| } | |
| } | |
| if len(outputs) == 0 { | |
| if command == nil { | |
| return nil, fault.New("no output received from module: expected a manifest") | |
| } else { | |
| return nil, fault.New("no output received from module: expected a command response") | |
| } | |
| } | |
| o := outputs[len(outputs)-1] |
| func (w *Writer) Create(ctx context.Context, r io.Reader) error { | ||
| b, err := io.ReadAll(r) | ||
| if err != nil { | ||
| return fault.Wrap(err, fctx.With(ctx)) | ||
| } | ||
|
|
||
| pm, err := plugin.Binary(b).Validate(ctx, w.run) | ||
| if err != nil { | ||
| return fault.Wrap(err, fctx.With(ctx)) | ||
| } | ||
|
|
||
| p := filepath.Join(plugin.PluginDirectory, pm.Name) | ||
|
|
||
| err = w.store.Write(ctx, p, r, int64(len(b))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Fix potential resource leak and improve error handling.
The code has several issues:
- The reader is used twice (ReadAll and Write) but the second use will be empty
- Missing cleanup in error cases
- Inconsistent error wrapping
func (w *Writer) Create(ctx context.Context, r io.Reader) error {
b, err := io.ReadAll(r)
if err != nil {
return fault.Wrap(err, fctx.With(ctx))
}
pm, err := plugin.Binary(b).Validate(ctx, w.run)
if err != nil {
return fault.Wrap(err, fctx.With(ctx))
}
p := filepath.Join(plugin.PluginDirectory, pm.Name)
+ // Sanitize the path to prevent directory traversal
+ if !strings.HasPrefix(p, plugin.PluginDirectory) {
+ return fault.New("invalid plugin name")
+ }
- err = w.store.Write(ctx, p, r, int64(len(b)))
+ err = w.store.Write(ctx, p, bytes.NewReader(b), int64(len(b)))
if err != nil {
- return err
+ return fault.Wrap(err, fctx.With(ctx))
}
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (w *Writer) Create(ctx context.Context, r io.Reader) error { | |
| b, err := io.ReadAll(r) | |
| if err != nil { | |
| return fault.Wrap(err, fctx.With(ctx)) | |
| } | |
| pm, err := plugin.Binary(b).Validate(ctx, w.run) | |
| if err != nil { | |
| return fault.Wrap(err, fctx.With(ctx)) | |
| } | |
| p := filepath.Join(plugin.PluginDirectory, pm.Name) | |
| err = w.store.Write(ctx, p, r, int64(len(b))) | |
| if err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| func (w *Writer) Create(ctx context.Context, r io.Reader) error { | |
| b, err := io.ReadAll(r) | |
| if err != nil { | |
| return fault.Wrap(err, fctx.With(ctx)) | |
| } | |
| pm, err := plugin.Binary(b).Validate(ctx, w.run) | |
| if err != nil { | |
| return fault.Wrap(err, fctx.With(ctx)) | |
| } | |
| p := filepath.Join(plugin.PluginDirectory, pm.Name) | |
| // Sanitize the path to prevent directory traversal | |
| if !strings.HasPrefix(p, plugin.PluginDirectory) { | |
| return fault.New("invalid plugin name") | |
| } | |
| err = w.store.Write(ctx, p, bytes.NewReader(b), int64(len(b))) | |
| if err != nil { | |
| return fault.Wrap(err, fctx.With(ctx)) | |
| } | |
| return nil | |
| } |
| ps := dt.Map(files, func(s string) *plugin.Available { | ||
| p := path.Join(plugin.PluginDirectory, s) | ||
|
|
||
| br, _, err := r.store.Read(ctx, p) | ||
| if err != nil { | ||
| return &plugin.Available{Error: err} | ||
| } | ||
|
|
||
| b, err := io.ReadAll(br) | ||
| if err != nil { | ||
| return &plugin.Available{Error: err} | ||
| } | ||
|
|
||
| m, err := plugin.Binary(b).Validate(ctx, r.run) | ||
| if err != nil { | ||
| return &plugin.Available{Error: err} | ||
| } | ||
|
|
||
| return &plugin.Available{ | ||
| StoragePath: p, | ||
| Loaded: &plugin.Package{ | ||
| Metadata: *m, | ||
| Binary: b, | ||
| }, | ||
| } | ||
| }) |
There was a problem hiding this comment.
Add resource cleanup and improve error handling in Map function.
The current implementation:
- Doesn't close the reader
- Could benefit from concurrent processing
- Missing context cancellation handling
ps := dt.Map(files, func(s string) *plugin.Available {
p := path.Join(plugin.PluginDirectory, s)
br, _, err := r.store.Read(ctx, p)
if err != nil {
return &plugin.Available{Error: err}
}
+ defer br.Close()
+
+ // Check if context is cancelled
+ if ctx.Err() != nil {
+ return &plugin.Available{Error: ctx.Err()}
+ }
b, err := io.ReadAll(br)
if err != nil {
return &plugin.Available{Error: err}
}
m, err := plugin.Binary(b).Validate(ctx, r.run)
if err != nil {
return &plugin.Available{Error: err}
}
return &plugin.Available{
StoragePath: p,
Loaded: &plugin.Package{
Metadata: *m,
Binary: b,
},
}
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ps := dt.Map(files, func(s string) *plugin.Available { | |
| p := path.Join(plugin.PluginDirectory, s) | |
| br, _, err := r.store.Read(ctx, p) | |
| if err != nil { | |
| return &plugin.Available{Error: err} | |
| } | |
| b, err := io.ReadAll(br) | |
| if err != nil { | |
| return &plugin.Available{Error: err} | |
| } | |
| m, err := plugin.Binary(b).Validate(ctx, r.run) | |
| if err != nil { | |
| return &plugin.Available{Error: err} | |
| } | |
| return &plugin.Available{ | |
| StoragePath: p, | |
| Loaded: &plugin.Package{ | |
| Metadata: *m, | |
| Binary: b, | |
| }, | |
| } | |
| }) | |
| ps := dt.Map(files, func(s string) *plugin.Available { | |
| p := path.Join(plugin.PluginDirectory, s) | |
| br, _, err := r.store.Read(ctx, p) | |
| if err != nil { | |
| return &plugin.Available{Error: err} | |
| } | |
| defer br.Close() | |
| // Check if context is cancelled | |
| if ctx.Err() != nil { | |
| return &plugin.Available{Error: ctx.Err()} | |
| } | |
| b, err := io.ReadAll(br) | |
| if err != nil { | |
| return &plugin.Available{Error: err} | |
| } | |
| m, err := plugin.Binary(b).Validate(ctx, r.run) | |
| if err != nil { | |
| return &plugin.Available{Error: err} | |
| } | |
| return &plugin.Available{ | |
| StoragePath: p, | |
| Loaded: &plugin.Package{ | |
| Metadata: *m, | |
| Binary: b, | |
| }, | |
| } | |
| }) |
| func (s *localStorer) List(ctx context.Context, prefix string) ([]string, error) { | ||
| dir := filepath.Join(s.path, prefix) | ||
| entries, err := fs.ReadDir(s.s, dir) | ||
| if err != nil { | ||
| return nil, fault.Wrap(err, fctx.With(ctx)) | ||
| } | ||
|
|
||
| var names []string | ||
| for _, entry := range entries { | ||
| names = append(names, entry.Name()) | ||
| } | ||
|
|
||
| return names, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider security and performance improvements.
The current implementation has several areas for improvement:
- Path traversal vulnerability: Consider sanitizing the
prefixparameter to prevent directory traversal attacks. - Memory efficiency: For directories with many entries, consider implementing pagination or streaming.
- Entry types: The method doesn't distinguish between files and directories, which might be important for plugin management.
Consider this safer implementation:
func (s *localStorer) List(ctx context.Context, prefix string) ([]string, error) {
+ // Sanitize the prefix to prevent directory traversal
+ if cleaned := filepath.Clean(prefix); cleaned != prefix {
+ return nil, fault.New("invalid prefix path")
+ }
+
dir := filepath.Join(s.path, prefix)
entries, err := fs.ReadDir(s.s, dir)
if err != nil {
return nil, fault.Wrap(err, fctx.With(ctx))
}
var names []string
for _, entry := range entries {
+ // Optionally filter by entry type
+ if !entry.IsDir() {
names = append(names, entry.Name())
+ }
}
return names, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *localStorer) List(ctx context.Context, prefix string) ([]string, error) { | |
| dir := filepath.Join(s.path, prefix) | |
| entries, err := fs.ReadDir(s.s, dir) | |
| if err != nil { | |
| return nil, fault.Wrap(err, fctx.With(ctx)) | |
| } | |
| var names []string | |
| for _, entry := range entries { | |
| names = append(names, entry.Name()) | |
| } | |
| return names, nil | |
| } | |
| func (s *localStorer) List(ctx context.Context, prefix string) ([]string, error) { | |
| // Sanitize the prefix to prevent directory traversal | |
| if cleaned := filepath.Clean(prefix); cleaned != prefix { | |
| return nil, fault.New("invalid prefix path") | |
| } | |
| dir := filepath.Join(s.path, prefix) | |
| entries, err := fs.ReadDir(s.s, dir) | |
| if err != nil { | |
| return nil, fault.Wrap(err, fctx.With(ctx)) | |
| } | |
| var names []string | |
| for _, entry := range entries { | |
| // Optionally filter by entry type | |
| if !entry.IsDir() { | |
| names = append(names, entry.Name()) | |
| } | |
| } | |
| return names, nil | |
| } |
No description provided.