feature: velox server #248
Conversation
Signed-off-by: Valery Piashchynski <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds protobuf APIs and Buf configs, upgrades dependencies, refactors Builder to an options-based API, introduces a v2 templated builder and GitHub template downloader, implements an h2c BuildService server with validation, caching and concurrency guards, wires a CLI server command, and adds docs, tests, a k6 benchmark, and tooling targets. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerCLI as CLI (h2c)
participant S as BuildServer
participant V as Validator
participant C as LRU Cache
participant GH as GitHubClient
participant B as Builder
Client->>ServerCLI: POST BuildService/Build
ServerCLI->>S: Build(request)
S->>V: Validate(request)
alt invalid
S-->>ServerCLI: CodeInvalidArgument
ServerCLI-->>Client: Error
else valid
S->>C: Check in-progress/cache
alt in-progress
S-->>ServerCLI: CodeAlreadyExists
ServerCLI-->>Client: Error
else cached and not force
C-->>S: return path
S-->>ServerCLI: BuildResponse(path, logs note)
ServerCLI-->>Client: 200
else build needed
S->>GH: DownloadTemplate(rrVersion)
GH-->>S: template path
S->>B: NewBuilder(...options)
S->>B: Build(rrVersion)
B-->>S: binary path + logs
S->>C: Store(path)
S-->>ServerCLI: BuildResponse(path, logs)
ServerCLI-->>Client: 200
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
(github/gitlab/bitbucket/custome) Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new Velox server feature that provides HTTP/gRPC build-as-a-service functionality for RoadRunner binaries. The server supports cross-platform builds with caching, request validation, and concurrent request handling.
Key changes include:
- New HTTP/gRPC server with build service API using buf/connectrpc
- Refactored v2 architecture with improved builder, caching, and GitHub integration
- Template system supporting multiple RoadRunner versions (2024/2025) with cross-compilation support
Reviewed Changes
Copilot reviewed 37 out of 40 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/cli/server/ | New server command and build service implementation |
| v2/ | Refactored v2 components (builder, cache, GitHub client, plugin management) |
| api/ | Protocol buffer service definitions for build requests/responses |
| gen/ | Generated protobuf code for gRPC/Connect services |
| go.mod | Added dependencies for buf, connect-go, protovalidate, and LRU cache |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Signed-off-by: Valery Piashchynski <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🔭 Outside diff range comments (5)
github/pool.go (2)
59-60: Compile-time bug: invalid range over int
for range p.maxWorkersdoes not compile in Go (cannot range over an int). Use a classic for-loop to spawn workers.func (p *processor) run() { - for range p.maxWorkers { + for i := 0; i < p.maxWorkers; i++ { go func() { for v := range p.queueCh {
116-119: Resource leak: close the content reader (rc), not resp.Body
DownloadContentsreturns an io.ReadCloser (rc) that must be closed. Closingresp.Bodyhere is incorrect and can lead to leaks or double-closes. Closercafter scanning.- err = resp.Body.Close() - if err != nil { - p.log.Warn("failed to close response body, continuing", zap.Any("error", err)) - } + if err := rc.Close(); err != nil { + p.log.Warn("failed to close content reader, continuing", zap.Any("error", err)) + }v2/cache/cache.go (1)
1-57: Add eviction/TTL to RRCache to prevent unbounded memory growthAfter inspecting the codebase, I confirmed that v2/cache/cache.go’s RRCache
• Holds all entries in a plain map with no size or time‐based eviction
• Is used to cache potentially large GitHub release blobs (zipped binaries)Even though we already use an expirable LRU in internal/cli/server, RRCache remains unbounded—contradicting the PR’s 30-minute LRU promise. Without eviction, a long-running process can exhaust memory.
Recommended refactor (mandatory):
- In v2/cache/cache.go, replace the custom map with an expirable LRU (from github.com/hashicorp/golang-lru/v2/expirable) configured with TTL=30 minutes (and optionally a max entry count)
- Or extend RRCache to wrap each buffer with timestamps and run a background sweeper that deletes entries past their expireAt or when total size/count exceeds a cap
Example using hashicorp’s expirable LRU:
-import ( - "bytes"; "io"; "sync" -) +import ( + "bytes"; "io" + "github.com/hashicorp/golang-lru/v2/expirable" +) -type RRCache struct { - mu *sync.RWMutex - data map[string]*bytes.Buffer -} +type RRCache struct { + lru *expirable.LRU[string, *bytes.Buffer] +} func NewRRCache() *RRCache { - return &RRCache{ - mu: &sync.RWMutex{}, - data: make(map[string]*bytes.Buffer), - } + l, _ := expirable.New[string, *bytes.Buffer](128, 30*time.Minute, nil) + return &RRCache{lru: l} } func (c *RRCache) Get(key string) *bytes.Buffer { - c.mu.RLock() - defer c.mu.RUnlock() - if cache, ok := c.data[key]; ok { + if cache, ok := c.lru.Get(key); ok { buf := new(bytes.Buffer) buf.Grow(cache.Len()) _, err := io.Copy(buf, bytes.NewReader(cache.Bytes())) if err != nil { panic(err) } return buf } return nil } func (c *RRCache) Set(key string, buf *bytes.Buffer) { - c.mu.Lock() - defer c.mu.Unlock() - - if buf == nil { - panic("cannot set nil value in cache") - } - if _, ok := c.data[key]; ok { - return - } - cache := new(bytes.Buffer) - cache.Grow(buf.Len()) - _, err := io.Copy(cache, bytes.NewReader(buf.Bytes())) - if err != nil { - panic(err) - } - c.data[key] = cache + if buf == nil { + panic("cannot set nil value in cache") + } + // Copy buffer to avoid mutation + cache := new(bytes.Buffer) + cache.Grow(buf.Len()) + if _, err := io.Copy(cache, bytes.NewReader(buf.Bytes())); err != nil { + panic(err) + } + c.lru.Add(key, cache) }builder/builder.go (1)
288-296: Duplicate -ldflags usage likely overrides flags; combine into a single valueYou append -ldflags twice: once for “-s” and again for build metadata. The last one wins and drops “-s”. Merge them.
Apply this diff:
case false: - buildCmdArgs = append(buildCmdArgs, "-ldflags", "-s") + // will be merged with metadata below + buildCmdArgs = append(buildCmdArgs, "-ldflags", "-s") } // LDFLAGS for version and build time, always appended -buildCmdArgs = append(buildCmdArgs, "-ldflags") -buildCmdArgs = append(buildCmdArgs, fmt.Sprintf(ldflags, b.rrVersion, time.Now().UTC().Format(time.RFC3339))) +// Merge previous -ldflags (if any) with metadata flags. +// Collect last -ldflags index if present. +ld := fmt.Sprintf(ldflags, b.rrVersion, time.Now().UTC().Format(time.RFC3339)) +if n := len(buildCmdArgs); n >= 2 && buildCmdArgs[n-2] == "-ldflags" { + buildCmdArgs[n-1] = buildCmdArgs[n-1] + " " + ld +} else { + buildCmdArgs = append(buildCmdArgs, "-ldflags", ld) +}Optional cleanup: also consolidate gcflags like: "-gcflags", "all=-N -l".
internal/cli/root.go (1)
62-63: Potential nil pointer dereference when config is nilThe
configpointer is only initialized after the command validation passes. If the PersistentPreRunE hook is bypassed or if there's an error,configcould remain nil when accessed by the build command.Initialize the config pointer to prevent nil dereferences:
var ( pathToConfig string // path to the velox configuration outputFile string // output file (optionally with directory) address string - config *velox.Config // velox configuration + config = &velox.Config{} // velox configuration )
🧹 Nitpick comments (50)
v2/config/builder.go (1)
3-6: Add struct tags (json/yaml/toml) for config unmarshallingIf this struct is loaded from config, add tags to ensure consistent key mapping across formats. Also consider a short doc comment to satisfy linters.
Apply tags:
type BuilderConfig struct { - ModuleName string - Tag string + ModuleName string `json:"module_name" yaml:"module_name" toml:"module_name"` + Tag string `json:"tag" yaml:"tag" toml:"tag"` }Optional doc comment (outside the selected lines):
// BuilderConfig carries module configuration for the v2 builder.api/response/v1/response.proto (1)
7-10: Document fields; consider future-proofing logs deliveryAdd concise comments for lint/readability. Also consider that logs can be large; if you anticipate big payloads, think about streaming logs or returning a URL to logs instead of embedding full text in the response.
-message BuildResponse { - string path = 1; - string logs = 2; -} +// BuildResponse is returned by BuildService.Build. +message BuildResponse { + // Absolute (or container-local) path to the built RR binary. + string path = 1; + // Build output logs. Consider streaming or capping length if logs can be large. + string logs = 2; +}Optional: If caching status is useful to clients, add a boolean to indicate cache hits in a backward-compatible way:
bool cache_hit = 3;github/repo.go (4)
65-66: Fix log typo: “seding” → “sending”.Apply:
- r.log.Info("seding download request", zap.String("url", url.String())) + r.log.Info("sending download request", zap.String("url", url.String()))
73-76: Guard against non-200 download responses.After
Do, validate the status code to fail fast on errors (rate limits, 404, etc.).Apply:
do, err := r.client.Do(context.Background(), request, buf) if err != nil { return "", err } + if do.Response == nil || do.Response.StatusCode != http.StatusOK { + if do != nil && do.Response != nil { + return "", fmt.Errorf("unexpected status code from archive download: %d", do.Response.StatusCode) + } + return "", fmt.Errorf("nil response from archive download") + } _, _ = io.Copy(io.Discard, do.Body) _ = do.Body.Close()
87-96: Cleanup temp archive after extraction to avoid disk bloat.The
.zipfile under tmp is not removed. Remove it after use.Apply:
f, err := os.Create(name + zipExt) if err != nil { return "", err } defer func() { _ = f.Close() }() + // ensure temporary archive is removed after extraction succeeds/fails + defer func() { + _ = os.Remove(name + zipExt) + }()
133-145: Redundant “..” checks on zip entry names.The checks against
..on the absolute/relative zip entry names are redundant given the stronger prefix check inextract(). Consider removing to reduce noise.Makefile (1)
4-7: Add PHONY targets and consider buf lint/breaking checks.Marking targets as
.PHONYavoids accidental conflicts with files namedtestorregenerate. Optionally, addbuf lint(andbuf breakingin CI) to catch proto issues earlier.Apply:
+ .PHONY: test regenerate test: go test -v -race ./... regenerate: rm -rf ./gen buf generate buf format -wOptionally extend:
proto-check: buf lint # In CI, consider: buf breaking --against '.git#branch=main'config.go (1)
20-32: Avoid exporting a mutable global Config; provide a constructor to return a fresh copy.An exported global pointer can be mutated from multiple places, leading to hidden coupling and data races in long-running server processes. Prefer a constructor returning a new instance.
Keep
DefaultConfigfor backward compatibility, but add:// NewDefaultConfig returns a fresh default configuration instance. func NewDefaultConfig() *Config { return &Config{ Roadrunner: map[string]string{ref: defaultBranch}, Debug: &Debug{Enabled: false}, Log: map[string]string{"level": "debug", "mode": "development"}, } }Callers should use
velox.NewDefaultConfig()rather than mutatingvelox.DefaultConfig.v2/cache/cache.go (1)
9-12: RWMutex should not be a pointer.RWMutex is safe to embed by value; storing it as a pointer is unnecessary and invites accidental copying.
Apply:
type RRCache struct { - mu *sync.RWMutex + mu sync.RWMutex data map[string]*bytes.Buffer } func NewRRCache() *RRCache { return &RRCache{ - mu: &sync.RWMutex{}, + mu: sync.RWMutex{}, data: make(map[string]*bytes.Buffer), } }.claude/CLAUDE.md (2)
17-27: Add a language to this fenced block (MD040).markdownlint flags this block; add a language hint for better rendering and lint compliance.
Apply this diff:
-``` +```text
46-49: Align server run example with the PR’s server port (9000).PR text and server command use 9000; docs show 8080. Let’s avoid drift.
Apply this diff:
-./vx -c velox.toml server -a 127.0.0.1:8080 +./vx -c velox.toml server -a 127.0.0.1:9000v2/plugin/plugin.go (3)
3-6: Avoid weak/global RNG; use a deterministic alias for reproducible builds.math/rand with the default global source is predictable and varies per run if seeded. A deterministic alias derived from (moduleName, tag) improves reproducibility and cache hit rates while still minimizing collision risk.
Apply this diff to replace math/rand with a deterministic hash-based prefix:
import ( "fmt" - "math/rand" + "hash/fnv" )
23-29: Derive the prefix deterministically from moduleName and tag.This ensures stable imports and generated code across identical requests.
Apply this diff:
func NewPlugin(moduleName, tag string) *Plugin { return &Plugin{ - prefix: randStringBytes(5), + prefix: deterministicPrefix(moduleName, tag), moduleName: moduleName, tag: tag, } }
50-56: Replace rand-based generator with a small deterministic hash encoder.Keeps the 5-char alias while eliminating RNG. Collision probability remains very low for typical plugin sets.
Apply this diff:
-func randStringBytes(n int) string { - b := make([]byte, n) - for i := range b { - b[i] = letterBytes[rand.Intn(len(letterBytes))] //nolint:gosec - } - return string(b) -} +func deterministicPrefix(moduleName, tag string) string { + const n = 5 + h := fnv.New32a() + // Write input deterministically; no errors for these writes. + _, _ = h.Write([]byte(moduleName)) + _, _ = h.Write([]byte("@")) + _, _ = h.Write([]byte(tag)) + v := h.Sum32() + + alphabet := []byte(letterBytes) + base := uint32(len(alphabet)) + out := make([]byte, n) + for i := 0; i < n; i++ { + out[i] = alphabet[v%base] + v /= base + } + return string(out) +}v2/builder/templates/templateV2025.go (1)
6-8: Confirm Go 1.25 toolchain availability for consumers.The template pins go 1.25/toolchain 1.25.0. If your build fleet or users are on Go 1.24, template builds will fail. Consider downgrading to 1.24 until 1.25 is broadly available, or make this configurable.
Would you like me to make the Go version/toolchain templated off a CLI flag or RRModuleVersion?
v2/builder/options.go (2)
20-24: Defensive copy for plugins sliceAssigning the incoming slice directly ties Builder state to the caller’s backing array. A later append in caller scope can mutate Builder’s plugins unexpectedly. Make a shallow copy to decouple.
Apply this diff:
func WithPlugins(plugins ...*plugin.Plugin) Option { return func(b *Builder) { - b.plugins = plugins + if len(plugins) == 0 { + b.plugins = nil + return + } + cp := make([]*plugin.Plugin, len(plugins)) + copy(cp, plugins) + b.plugins = cp } }
47-52: Guard against nil logger usage downstreamMany call sites (e.g., Builder.Build, goBuildCmd) use b.log without nil checks. If WithLogger wasn’t provided, this will panic. Either default to zap.NewNop() in NewBuilder or add a setter ensuring a non-nil logger.
Would you like me to patch v2/builder/builder.go to default the logger to zap.NewNop() when not provided?
v2/builder/templates/template_test.go (1)
12-48: Add go.mod rendering tests to catch template/data mismatchesGiven recent shape changes, add tests for:
- CompileGoModTemplate2025: validate module line and require entries.
- (If supported) CompileGoModTemplate2024.
This would have caught the mismatch between Template and templateV2025/V2024.
Want me to add those tests with realistic plugin require lines?
builder/builder.go (2)
279-284: Consolidate gcflags into a single argumentPassing -gcflags twice is unusual; use “all=-N -l” in one argument.
Apply this diff:
-// turn off optimizations -buildCmdArgs = append(buildCmdArgs, "-gcflags", "-N") -// turn off inlining -buildCmdArgs = append(buildCmdArgs, "-gcflags", "-l") +// turn off optimizations and inlining +buildCmdArgs = append(buildCmdArgs, "-gcflags", "all=-N -l")
303-316: Guard GOPATH/GOCACHE composition when GOOS/GOARCH are emptyWhen not set, GOPATH/GOCACHE become “…/go//…”. Either default to runtime.GOOS/GOARCH or avoid embedding empty path components.
I can patch this to default GOOS/GOARCH from runtime if empty and ensure directories exist before build.
v2/github/github_test.go (2)
19-21: Use t.TempDir() for isolation and automatic cleanupUsing os.TempDir() can cause cross-test interference and leave artifacts behind. Prefer t.TempDir().
Apply this diff:
- tmpDir := os.TempDir() + tmpDir := t.TempDir()
11-34: Gate network calls and add a minimal assertionThis test performs a live network call and always passes (no assertions). Gate it behind an env var or testing.Short() and assert at least the error type or result invariants to make it meaningful.
Example minimal changes (safe for CI):
func TestGitHubClient_DownloadTemplate(t *testing.T) { + t.Parallel() + if os.Getenv("INTEGRATION_GITHUB") != "1" { + t.Skip("skipping integration test; set INTEGRATION_GITHUB=1 to run") + } @@ - // Basic assertions - just check if method executed without panicking - if err != nil { - t.Logf("DownloadTemplate returned error (expected for test without real token): %v", err) - } - - if resultPath != "" { - t.Logf("DownloadTemplate returned path: %s", resultPath) - } + // Basic assertion + if err == nil && resultPath == "" { + t.Fatalf("expected non-empty resultPath when err is nil") + }buf.gen.yaml (1)
6-10: Consider dropping grpc-go generation if you’re only serving ConnectIf the codebase exclusively uses Connect handlers/clients, the grpc/go stubs may be unnecessary. Removing them reduces gen surface and dependency footprint. If you plan to support plain gRPC too, keep as is.
internal/cli/server/command.go (4)
24-26: Nit: Rename variable to reflect role (server/handler, not client)This is a service implementation, not a client. A name like svc or buildSrv improves clarity.
- client := NewBuildServer(zlog) - path, handler := servicev1.NewBuildServiceHandler(client) + svc := NewBuildServer(zlog) + path, handler := servicev1.NewBuildServiceHandler(svc)
27-33: Add server timeouts to mitigate slowloris and stuck connectionsOnly ReadHeaderTimeout is set. Add ReadTimeout, WriteTimeout, and IdleTimeout for better resilience.
server := &http.Server{ Addr: *address, Handler: h2c.NewHandler(mux, &http2.Server{ MaxConcurrentStreams: 256, }), - ReadHeaderTimeout: time.Minute, + // Tighten header timeout and add full connection management timeouts + ReadHeaderTimeout: 10 * time.Second, + ReadTimeout: 30 * time.Second, + WriteTimeout: 2 * time.Minute, + IdleTimeout: 2 * time.Minute, }
20-44: Optional: Graceful shutdown via command contextLeverage cmd.Context() to shut down cleanly on cancellation (e.g., signals). This avoids relying solely on http.ErrServerClosed.
Example refactor (adds context import):
- RunE: func(_ *cobra.Command, _ []string) error { + RunE: func(cmd *cobra.Command, _ []string) error { + ctx := cmd.Context() @@ - err := server.ListenAndServe() - if err != nil { - if errors.Is(err, http.ErrServerClosed) { - return nil - } - - return err - } - return nil + errCh := make(chan error, 1) + go func() { + errCh <- server.ListenAndServe() + }() + select { + case <-ctx.Done(): + shCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _ = server.Shutdown(shCtx) + return nil + case err := <-errCh: + if errors.Is(err, http.ErrServerClosed) { + return nil + } + return err + }Remember to add:
- import "context"
29-31: Security note: h2c should be behind a trusted boundaryServing HTTP/2 cleartext (h2c) is fine for local/dev. For production, terminate TLS at a proxy or use TLS directly to avoid downgrades and on-path interference.
builder/builder_test.go (4)
122-122: Use file-appropriate permissions for go.mod0700 makes go.mod executable and unreadable to other users. Prefer 0644 for files.
- _ = os.WriteFile(filepath.Join(v.Replace, goModStr), associated[v.ModuleName], rights) + _ = os.WriteFile(filepath.Join(v.Replace, goModStr), associated[v.ModuleName], 0o644)
85-91: Avoid mutating Builder internals; pass modules via constructorYou instantiate with an empty modules slice and then assign b.modules. Prefer passing modules to NewBuilder to mirror real usage and reduce test brittleness.
Sketch:
mods := []*velox.ModulesInfo{ {Version: "master", ModuleName: "dummy_one_relative", Replace: "/tmp/dummy_one_relative"}, // ... } b := NewBuilder("/tmp", mods, WithRRVersion(version), WithDebug(false), WithLogger(l))
76-91: Make test sandboxed: use t.TempDir and pass t to setupWriting to hardcoded /tmp paths can collide across runs. Accept *testing.T in setup, and base on t.TempDir() for isolation and automatic cleanup.
Example:
func setup(t *testing.T, version string) *Builder { t.Helper() base := t.TempDir() // use filepath.Join(base, "dummy_one_relative") for Replace dirs }Then update callers to setup(t, "...") and drop clean().
Also applies to: 120-126
148-235: Parallelize independent tests for speedThese tests don’t share state when using t.TempDir; mark them parallel to reduce wall time.
func Test_Builder_getDepsReplace_multipleAbsolute_V2024(t *testing.T) { + t.Parallel() @@ func Test_Builder_getDepsReplace_multipleRelative_V2024(t *testing.T) { + t.Parallel() @@ // ... apply similarly to the remaining test functionsinternal/cli/build/build.go (4)
25-30: Use os.Getwd instead of syscall.Getwdsyscall.Getwd is low-level and unnecessary here.
- wd, err := syscall.Getwd() + wd, err := os.Getwd()
47-51: Prefer returning errors from RunE instead of os.Exit for testabilityLet Cobra handle exit codes. Returning errors improves composability and unit testing.
- path, err := rp.DownloadTemplate(os.TempDir(), cfg.Roadrunner[ref]) - if err != nil { - zlog.Error("downloading template", zap.Error(err)) - os.Exit(1) - } + path, err := rp.DownloadTemplate(os.TempDir(), cfg.Roadrunner[ref]) + if err != nil { + return fmt.Errorf("downloading template: %w", err) + }Remember to:
- add import "fmt"
53-57: Return error instead of exiting on GetPluginsModData failureSame rationale as above.
- pMod, err := rp.GetPluginsModData() - if err != nil { - zlog.Error("get plugins mod data", zap.Error(err)) - os.Exit(1) - } + pMod, err := rp.GetPluginsModData() + if err != nil { + return fmt.Errorf("get plugins mod data: %w", err) + }
64-70: Return error instead of exiting on build failureSame rationale as above.
- err = builder.NewBuilder(path, pMod, + err = builder.NewBuilder(path, pMod, builder.WithOutputDir(*out), builder.WithRRVersion(cfg.Roadrunner[ref]), builder.WithDebug(cfg.Debug.Enabled), builder.WithLogger(zlog.Named("Builder")), ).Build(cfg.Roadrunner[ref]) if err != nil { - zlog.Error("fatal", zap.Error(err)) - os.Exit(1) + return fmt.Errorf("build failed: %w", err) }(Remember import "fmt")
internal/cli/root.go (1)
35-38: Consider using a more specific check for the server commandInstead of checking if
cmd.Use == "server", consider usingcmd.Name()which is more idiomatic and reliable for command identification.- PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { - if cmd.Use == "server" { - return nil - } + PersistentPreRunE: func(cmd *cobra.Command, _ []string) error { + if cmd.Name() == "server" { + return nil + }v2/github/github.go (4)
102-144: Consider adding retry logic for network operationsThe download operation could benefit from retry logic to handle transient network failures, especially for production use.
Would you like me to provide an implementation of retry logic with exponential backoff for the download operations?
137-141: Improve error handling for HTTP response bodyThe deferred close of the response body should check for errors.
- _, err = io.Copy(buf, resp.Body) - _ = resp.Body.Close() - if err != nil { - return fmt.Errorf("failed to copy response body: %w", err) - } + _, err = io.Copy(buf, resp.Body) + closeErr := resp.Body.Close() + if err != nil { + return fmt.Errorf("failed to copy response body: %w", err) + } + if closeErr != nil { + return fmt.Errorf("failed to close response body: %w", closeErr) + }
248-250: Consider more robust commit SHA detectionThe current SHA detection only checks for length 40, but partial SHAs (7-40 characters) are also valid. Consider using a hex pattern check.
- // assume that we have a sha here - if len(rrVersion) == 40 { - return url.Parse(fmt.Sprintf("https://github.com/roadrunner-server/roadrunner/archive/%s.zip", rrVersion)) - } + // assume that we have a sha here (7-40 hex characters) + if len(rrVersion) >= 7 && len(rrVersion) <= 40 && isHexString(rrVersion) { + return url.Parse(fmt.Sprintf("https://github.com/roadrunner-server/roadrunner/archive/%s.zip", rrVersion)) + }And add this helper function:
func isHexString(s string) bool { for _, c := range s { if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')) { return false } } return true }
282-283: Good security awareness with decompression bomb commentThe G110 comment correctly identifies the potential DoS risk. Consider implementing size limits or using a limited reader for additional protection.
Consider adding size validation:
const maxZipSize = 100 * 1024 * 1024 // 100MB limit // Before extraction, check the uncompressed size var totalSize int64 for _, f := range rc.File { totalSize += int64(f.UncompressedSize64) } if totalSize > maxZipSize { return fmt.Errorf("archive too large: %d bytes exceeds maximum of %d", totalSize, maxZipSize) }api/request/v1/request.proto (2)
29-29: Consider simplifying the regex patternThe regex pattern has redundant escaping. In proto CEL expressions, you can use single backslashes.
- expression: "this.matches('^(v\\\\d+\\\\.\\\\d+\\\\.\\\\d+.*|master|[a-f0-9]{7,40})$')" + expression: "this.matches('^(v\\d+\\.\\d+\\.\\d+.*|master|[a-f0-9]{7,40})$')"
32-35: Consider making target_platform required if it affects build outputIf the target platform significantly affects the build output, consider making it required to avoid ambiguity about which platform was targeted.
- Platform target_platform = 4 [(buf.validate.field).required = false]; + Platform target_platform = 4 [(buf.validate.field).required = true];v2/builder/builder.go (2)
235-246: Debug build flags could be consolidatedThe debug gcflags are being set twice with different values, which may cause the second to override the first.
case true: // debug flags // turn off optimizations - buildCmdArgs = append(buildCmdArgs, "-gcflags", "-N") - // turn off inlining - buildCmdArgs = append(buildCmdArgs, "-gcflags", "-l") + // turn off optimizations and inlining + buildCmdArgs = append(buildCmdArgs, "-gcflags", "-N -l") // build with debug tags buildCmdArgs = append(buildCmdArgs, "-tags", "debug")
309-336: Consider using os.Rename for better performanceThe current implementation reads the entire file into memory and writes it back. For large binaries, this could be inefficient.
func moveFile(from, to string) error { + // Try rename first (atomic and efficient if on same filesystem) + err := os.Rename(from, to) + if err == nil { + return nil + } + + // Fall back to copy if rename fails (e.g., cross-device) ffInfo, err := os.Stat(from) if err != nil { return err }k6-benchmark.js (5)
51-51: Exercise caching by default (force_rebuild=false).With force_rebuild: true every request bypasses the 30-minute LRU cache, defeating one of the key PR goals. Recommend defaulting to false and toggling via an env var or scenario if you want to stress rebuilds.
- force_rebuild: true, + force_rebuild: false,
221-229: Add a correctness check on the response payload (path present).Validate that the service returns a JSON body with a non-empty "path". This guards against false positives where the server returns 200 with an error payload.
- const isSuccess = check(response, { + // Parse JSON body (best-effort) + let respJson = null; + try { + respJson = response.json(); + } catch (e) { + // ignore parse errors + } + + const isSuccess = check(response, { "status is 200": (r) => r.status === 200, "response time < 30000ms": (r) => r.timings.duration < 30000, "response has body": (r) => r.body && r.body.length > 0, "content-type is application/json": (r) => r.headers["Content-Type"] && r.headers["Content-Type"].includes("application/json"), + "response has non-empty path": () => + respJson && typeof respJson.path === "string" && respJson.path.length > 0, });
26-26: Fix misleading comment (duration is 60s, not 30s).- duration: "60s", // for 30 seconds + duration: "60s", // for 60 seconds
42-42: Fix misleading comment (threshold is 20s, not 5s).- http_req_duration: ["p(95)<20000"], // 95% of requests must complete below 5s + http_req_duration: ["p(95)<20000"], // 95% of requests must complete below 20s
217-217: Fix misleading comment (timeout is 120s).- timeout: "120s", // Allow up to 60 seconds for build operations + timeout: "120s", // Allow up to 120 seconds for build operationsinternal/cli/server/server.go (1)
44-57: Optional: remove redundant file deletion on cache eviction.You already remove the build directory (os.TempDir()/hash). Deleting the binary path first is redundant since it resides within that directory.
- err := os.RemoveAll(rrBinPath) - if err != nil { - log.Error("removing cached file", zap.String("path", rrBinPath), zap.Error(err)) - } - // remove path - log.Info("removing cached directory", zap.String("path", filepath.Join(os.TempDir(), hash))) - err = os.RemoveAll(filepath.Join(os.TempDir(), hash)) + log.Info("removing cached directory", zap.String("path", filepath.Join(os.TempDir(), hash))) + err := os.RemoveAll(filepath.Join(os.TempDir(), hash)) if err != nil { log.Error("failed to remove directory", zap.String("path", filepath.Join(os.TempDir(), hash)), zap.Error(err)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (7)
buf.lockis excluded by!**/*.lockgen/go/api/request/v1/request.pb.gois excluded by!**/*.pb.go,!**/gen/**gen/go/api/response/v1/response.pb.gois excluded by!**/*.pb.go,!**/gen/**gen/go/api/service/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**gen/go/api/service/v1/serviceV1connect/service.connect.gois excluded by!**/gen/**gen/go/api/service/v1/service_grpc.pb.gois excluded by!**/*.pb.go,!**/gen/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (33)
.claude/CLAUDE.md(1 hunks).gitignore(1 hunks)Makefile(1 hunks)api/request/v1/request.proto(1 hunks)api/response/v1/response.proto(1 hunks)api/service/v1/service.proto(1 hunks)buf.gen.yaml(1 hunks)buf.yaml(1 hunks)builder/builder.go(9 hunks)builder/builder_test.go(3 hunks)builder/options.go(1 hunks)builder/templates/templateV2025.go(0 hunks)config.go(1 hunks)github/pool.go(2 hunks)github/repo.go(2 hunks)gitlab/repo.go(2 hunks)go.mod(2 hunks)internal/cli/build/build.go(1 hunks)internal/cli/root.go(4 hunks)internal/cli/server/command.go(1 hunks)internal/cli/server/server.go(1 hunks)k6-benchmark.js(1 hunks)v2/builder/builder.go(1 hunks)v2/builder/options.go(1 hunks)v2/builder/templates/compile.go(1 hunks)v2/builder/templates/templateV2024.go(1 hunks)v2/builder/templates/templateV2025.go(1 hunks)v2/builder/templates/template_test.go(1 hunks)v2/cache/cache.go(1 hunks)v2/config/builder.go(1 hunks)v2/github/github.go(1 hunks)v2/github/github_test.go(1 hunks)v2/plugin/plugin.go(1 hunks)
💤 Files with no reviewable changes (1)
- builder/templates/templateV2025.go
🧰 Additional context used
🧬 Code Graph Analysis (14)
builder/options.go (1)
builder/builder.go (1)
Builder(36-48)
v2/github/github_test.go (2)
v2/cache/cache.go (1)
NewRRCache(14-19)v2/github/github.go (1)
NewHTTPClient(42-67)
v2/builder/options.go (4)
v2/builder/builder.go (1)
Builder(32-44)builder/builder.go (1)
Builder(36-48)gen/go/api/request/v1/request.pb.go (3)
Plugin(160-168)Plugin(181-181)Plugin(196-198)v2/plugin/plugin.go (1)
Plugin(13-17)
v2/builder/templates/template_test.go (2)
v2/plugin/plugin.go (1)
NewPlugin(23-29)v2/builder/templates/compile.go (1)
NewTemplate(22-37)
internal/cli/server/command.go (2)
internal/cli/server/server.go (1)
NewBuildServer(41-64)gen/go/api/service/v1/serviceV1connect/service.connect.go (1)
NewBuildServiceHandler(85-99)
v2/builder/builder.go (5)
builder/builder.go (1)
Builder(36-48)v2/plugin/plugin.go (1)
Plugin(13-17)v2/builder/options.go (1)
Option(11-11)config.go (2)
V2025(13-13)V2024(14-14)builder/templates/templateV2025.go (1)
CompileGoModTemplate2025(71-78)
v2/builder/templates/compile.go (5)
gen/go/api/request/v1/request.pb.go (3)
Plugin(160-168)Plugin(181-181)Plugin(196-198)v2/plugin/plugin.go (1)
Plugin(13-17)builder/templates/templateV2025.go (2)
GoModTemplateV2025(8-43)PluginsTemplateV2025(45-68)v2/builder/templates/templateV2025.go (2)
GoModTemplateV2025(3-33)PluginsTemplateV2025(35-59)v2/builder/templates/templateV2024.go (2)
GoModTemplateV2024(3-34)PluginsTemplateV2024(36-58)
internal/cli/server/server.go (9)
v2/cache/cache.go (1)
NewRRCache(14-19)gen/go/api/request/v1/request.pb.go (6)
BuildRequest(79-93)BuildRequest(106-106)BuildRequest(121-123)Plugin(160-168)Plugin(181-181)Plugin(196-198)gen/go/api/response/v1/response.pb.go (3)
BuildResponse(24-30)BuildResponse(43-43)BuildResponse(58-60)v2/builder/builder.go (2)
Builder(32-44)NewBuilder(47-58)builder/builder.go (2)
Builder(36-48)NewBuilder(51-63)v2/plugin/plugin.go (2)
Plugin(13-17)NewPlugin(23-29)v2/github/github.go (1)
NewHTTPClient(42-67)v2/builder/options.go (8)
Option(11-11)WithPlugins(20-24)WithOutputDir(55-59)WithRRVersion(62-66)WithLogs(41-45)WithLogger(48-52)WithGOOS(27-31)WithGOARCH(34-38)builder/options.go (7)
Option(10-10)WithOutputDir(48-52)WithRRVersion(55-59)WithLogs(34-38)WithLogger(41-45)WithGOOS(20-24)WithGOARCH(27-31)
internal/cli/root.go (3)
config.go (1)
Config(33-44)internal/cli/build/build.go (1)
BindCommand(19-79)internal/cli/server/command.go (1)
BindCommand(16-45)
internal/cli/build/build.go (3)
builder/builder.go (1)
NewBuilder(51-63)builder/options.go (4)
WithOutputDir(48-52)WithRRVersion(55-59)WithDebug(13-17)WithLogger(41-45)config.go (1)
Debug(46-48)
v2/github/github.go (2)
config.go (2)
Token(50-52)Debug(46-48)logger/logger.go (1)
Mode(13-13)
builder/builder_test.go (3)
builder/builder.go (1)
NewBuilder(51-63)modulesInfo.go (1)
ModulesInfo(13-22)builder/options.go (3)
WithRRVersion(55-59)WithDebug(13-17)WithLogger(41-45)
v2/plugin/plugin.go (1)
gen/go/api/request/v1/request.pb.go (3)
Plugin(160-168)Plugin(181-181)Plugin(196-198)
builder/builder.go (4)
modulesInfo.go (1)
ModulesInfo(13-22)v2/builder/builder.go (2)
Builder(32-44)NewBuilder(47-58)v2/builder/options.go (1)
Option(11-11)builder/options.go (1)
Option(10-10)
🪛 LanguageTool
.claude/CLAUDE.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...iew Velox is an automated build system for RoadRunner server and its plugins. It's...
(QB_NEW_EN)
[grammar] ~9-~9: There might be a mistake here.
Context: ...ilds. Key Technologies: - Go 1.24+ - Protocol Buffers (protobuf) with buf - G...
(QB_NEW_EN)
[grammar] ~10-~10: There might be a mistake here.
Context: ...+ - Protocol Buffers (protobuf) with buf - GitHub/GitLab API integration - gRPC and...
(QB_NEW_EN)
[grammar] ~11-~11: There might be a mistake here.
Context: ...with buf - GitHub/GitLab API integration - gRPC and Connect - Cobra CLI framework ...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...itLab API integration - gRPC and Connect - Cobra CLI framework ## Repository Struc...
(QB_NEW_EN)
[grammar] ~131-~131: There might be a mistake here.
Context: ...tiquette ### Branching - Main branch: master - Feature branches: feature/description ...
(QB_NEW_EN)
[grammar] ~132-~132: There might be a mistake here.
Context: ...in branch: master - Feature branches: feature/description - Use conventional commit messages ### Pl...
(QB_NEW_EN)
[grammar] ~139-~139: There might be a mistake here.
Context: ...mportant Plugin Guidelines:** - Do not use plugin's master branch - Use tags wit...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...isites - Go 1.24+ (toolchain: go1.24.0) - buf CLI for protocol buffer generation -...
(QB_NEW_EN)
[grammar] ~155-~155: There might be a mistake here.
Context: ...- buf CLI for protocol buffer generation - Git for version control ### Setup Steps...
(QB_NEW_EN)
[grammar] ~199-~199: There might be a mistake here.
Context: ...ld Templates 1. Create new template in builder/templates/ 2. Update builder.go to reference new tem...
(QB_NEW_EN)
[grammar] ~200-~200: There might be a mistake here.
Context: ...r/templates/2. Updatebuilder.go` to reference new template 3. Add corresponding tests...
(QB_NEW_EN)
[grammar] ~211-~211: There might be a mistake here.
Context: ...LI Commands - Follow cobra patterns in internal/cli/ - Use persistent flags for common options ...
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
.claude/CLAUDE.md
17-17: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (19)
.gitignore (1)
8-8: LGTM: Ignore .DS_StoreGood addition; this will keep macOS metadata out of the repo.
github/repo.go (2)
84-85: OS-aware path join: good change.Switching to
filepath.Joinis correct for cross-platform path handling.
15-19: Verified GetArchiveLink SignatureThe
Repositories.GetArchiveLinkmethod in go-github/v74 indeed has the signature:GetArchiveLink(ctx context.Context, owner, repo string, archiveformat ArchiveFormat, opts *RepositoryContentGetOptions, maxRedirects int) (*url.URL, *Response, error)The last parameter is
maxRedirects int, so passing10is correct. No changes needed.config.go (1)
20-32: Double-check token optionality vs. Validate() usage in server paths.PR objectives state GitHub token is optional for downloading RR base templates, but
Validate()(invoked by some flows) hard-requiresgithub.token. Ensure server paths that only need base templates don’t fail validation or consider relaxing checks when no plugins are specified.Would you like a targeted patch that:
- Requires token only if GitHub/GitLab plugins are configured, and
- Allows base template downloads without a token?
I can prepare it once you confirm where
Validate()is used in the server setup.go.mod (1)
28-29: Replace mapping is correct; no changes neededThe
replace github.com/roadrunner-server/velox/v2025/gen => ./gendirective applies to all subpackages undergen(includinggen/go/...), so imports like"github.com/roadrunner-server/velox/v2025/gen/go/api/request/v1"will be resolved to
./gen/go/api/request/v1on disk. There is intentionally nogen/go/go.mod, since these packages live under the root module once replaced.No adjustments to the
replacedirective or additionalgo.modfiles are required.v2/builder/options.go (1)
1-67: Solid functional-options setupThe options API is cohesive and straightforward. Constructors are small, side-effect free, and map cleanly to Builder fields.
v2/builder/templates/compile.go (1)
39-46: Adapt 2025 go.mod execution to expected data shapeBridge from Template.Requires (strings like "module version") to the GoModTemplateV2025’s expected
.Entriesat execution time.[ suggest_essential_refactor ]
Apply this diff:func CompileGoModTemplate2025(wr io.Writer, t *Template) error { - tmpl, err := template.New("go.mod").Parse(GoModTemplateV2025) + tmpl, err := template.New("go.mod").Parse(GoModTemplateV2025) if err != nil { return err } - return tmpl.Execute(wr, t) + // Build view model expected by the template: + // { RRModuleVersion string; Entries []struct{ Module, PseudoVersion, Replace string } } + type entry struct { + Module string + PseudoVersion string + Replace string + } + vm := struct { + RRModuleVersion string + Entries []entry + }{ + RRModuleVersion: t.RRModuleVersion, + Entries: make([]entry, 0, len(t.Requires)), + } + for _, req := range t.Requires { + // Expect format: "<module> <version>" + parts := strings.Fields(req) + if len(parts) >= 2 { + vm.Entries = append(vm.Entries, entry{Module: parts[0], PseudoVersion: parts[1]}) + } + } + return tmpl.Execute(wr, vm) }v2/builder/templates/template_test.go (1)
12-48: Good coverage of v2025 plugins template generationThe test covers core expectations (package, bundled imports, bundled plugin inits, and dynamic plugin rendering).
builder/builder.go (1)
235-241: Potential nil deref on logger in Write and elsewhereWrite (and other methods) call b.log.* unconditionally. If WithLogger isn’t supplied, this will panic.
Do all builder paths guarantee a non-nil logger? If not, set a default (zap.NewNop()) in NewBuilder.
api/service/v1/service.proto (1)
10-12: Service definition looks goodSimple, clear RPC with correct package wiring and go_package option.
buf.gen.yaml (1)
1-15: LGTM: Buf v2 plugins and options look consistentPinned plugin versions, consistent source_relative opts, and a single gen/go out dir are all good.
internal/cli/root.go (1)
85-86: Pass config safely to build commandSince build.BindCommand receives a pointer to a pointer (
*velox.Config), ensure proper initialization as mentioned above to prevent issues.v2/github/github.go (1)
71-100: LGTM! Well-structured template download logic with proper cachingThe caching implementation is well done - using the version as the key and properly managing the buffer lifecycle with
defer rrbuf.Reset().api/request/v1/request.proto (1)
1-8: LGTM! Proper proto3 syntax and package structureThe proto file follows best practices with proper syntax declaration, package naming, and Go package path configuration.
builder/options.go (1)
1-60: LGTM! Clean implementation of the functional options patternThe options are well-documented and follow Go best practices for the functional options pattern. This provides a flexible and extensible API for configuring the Builder.
v2/builder/builder.go (1)
62-64: LGTM! Good validation for required pluginsThe early validation ensures that the builder has plugins before proceeding with the build.
internal/cli/server/server.go (3)
88-94: LGTM: cache hit path and logs behavior.Returning the cached path and a short logs notice when ForceRebuild is false is appropriate and keeps responses lightweight.
66-72: LGTM: request validation via protovalidate.Good use of protovalidate to fail fast with CodeInvalidArgument on malformed inputs.
108-114: LGTM: GitHub client and template retrieval.Token-optional HTTP client with an RR in-memory cache is a solid approach to reduce rate-limit issues.
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
internal/cli/server/server.go (2)
152-173: Make cache key stable: exclude request_id/force_rebuild, canonicalize plugins, default target platform; never return empty keyIncluding RequestId breaks caching and the in-progress guard; plugin order variance and a nil target_platform also produce spurious misses. Returning "" on marshal failure collapses different requests into the same bucket.
Refactor to build a canonical request copy, sort plugins, default platform to host if nil, and fall back to a string concat if marshaling fails.
Apply this diff:
-func (b *BuildServer) generateCacheHash(req *connect.Request[requestV1.BuildRequest]) string { - cacheReq := &requestV1.BuildRequest{ - RequestId: req.Msg.GetRequestId(), - RrVersion: req.Msg.GetRrVersion(), - TargetPlatform: req.Msg.GetTargetPlatform(), - Plugins: req.Msg.GetPlugins(), - } - - data, err := proto.MarshalOptions{ - Deterministic: true, - AllowPartial: false, - }.Marshal(cacheReq) - if err != nil { - // TODO: might be just fail processing? - b.log.Error("marshaling cache key error, cache creation would be skipped", zap.Error(err)) - return "" - } - - h := fnv.New64a() - h.Write(data) - return strconv.FormatUint(h.Sum64(), 16) -} +func (b *BuildServer) generateCacheHash(req *connect.Request[requestV1.BuildRequest]) string { + // Build a deterministic, canonical copy of the request. + // - Excludes RequestId and ForceRebuild as they should not affect the artifact. + // - Defaults target platform to host if unspecified. + // - Canonicalizes plugin ordering to avoid order-dependent keys. + tp := req.Msg.GetTargetPlatform() + if tp == nil { + tp = &requestV1.Platform{ + Os: runtime.GOOS, + Arch: runtime.GOARCH, + } + } + + cacheReq := &requestV1.BuildRequest{ + RrVersion: req.Msg.GetRrVersion(), + TargetPlatform: tp, + Plugins: nil, + } + for _, p := range req.Msg.GetPlugins() { + if p == nil { + continue + } + cacheReq.Plugins = append(cacheReq.Plugins, &requestV1.Plugin{ + ModuleName: p.GetModuleName(), + Tag: p.GetTag(), + }) + } + sort.Slice(cacheReq.Plugins, func(i, j int) bool { + if cacheReq.Plugins[i].GetModuleName() == cacheReq.Plugins[j].GetModuleName() { + return cacheReq.Plugins[i].GetTag() < cacheReq.Plugins[j].GetTag() + } + return cacheReq.Plugins[i].GetModuleName() < cacheReq.Plugins[j].GetModuleName() + }) + + data, err := proto.MarshalOptions{Deterministic: true, AllowPartial: false}.Marshal(cacheReq) + if err != nil { + // Fallback to a plain string concat to avoid disabling caching entirely. + b.log.Error("marshaling cache key failed; falling back to string concat", zap.Error(err)) + var sb strings.Builder + sb.WriteString(cacheReq.GetRrVersion()) + if tp := cacheReq.GetTargetPlatform(); tp != nil { + sb.WriteString(tp.GetOs()) + sb.WriteString(tp.GetArch()) + } + for _, p := range cacheReq.Plugins { + sb.WriteString(p.GetModuleName()) + sb.WriteString(p.GetTag()) + } + h := fnv.New64a() + h.Write([]byte(sb.String())) + return strconv.FormatUint(h.Sum64(), 16) + } + + h := fnv.New64a() + h.Write(data) + return strconv.FormatUint(h.Sum64(), 16) +}
3-27: Add missing import for sorting plugins in cache keyRequired for the canonicalization in generateCacheHash.
- "runtime" + "runtime" + "sort"v2/builder/builder.go (2)
172-181: Fix incorrect os.IsExist usage after os.Stat.The error check pattern is incorrect. After
os.Stat(), you should checkerr == nilto determine if the directory exists, notos.IsExist(err).- if os.IsExist(err) { - b.log.Info("output path already exists, cleaning up", zap.String("dir", b.outputDir)) - _ = os.RemoveAll(b.outputDir) - } + if err == nil { + // Directory exists, clean it up + b.log.Info("output path already exists, cleaning up", zap.String("dir", b.outputDir)) + _ = os.RemoveAll(b.outputDir) + }
237-252: Fix duplicate -ldflags argument.You're appending "-ldflags" twice which could cause command parsing issues. The ldflags should be combined into a single argument.
case false: - buildCmdArgs = append(buildCmdArgs, "-ldflags", "-s") + // Strip symbols for release builds, combine with version flags + ldFlags := fmt.Sprintf("-s %s", fmt.Sprintf(ldflags, b.rrVersion, time.Now().UTC().Format(time.RFC3339))) + buildCmdArgs = append(buildCmdArgs, "-ldflags", ldFlags) } - // LDFLAGS for version and build time, always appended - buildCmdArgs = append(buildCmdArgs, "-ldflags") - buildCmdArgs = append(buildCmdArgs, fmt.Sprintf(ldflags, b.rrVersion, time.Now().UTC().Format(time.RFC3339))) + // For debug builds, add version flags separately + if b.debug { + buildCmdArgs = append(buildCmdArgs, "-ldflags") + buildCmdArgs = append(buildCmdArgs, fmt.Sprintf(ldflags, b.rrVersion, time.Now().UTC().Format(time.RFC3339))) + }
🧹 Nitpick comments (8)
internal/cli/server/server.go (6)
77-88: Non-atomic Contains/Add on currentlyProcessing; window for duplicate buildsContains followed by Add is a TOCTOU pattern. Two concurrent requests for the same key can pass Contains before either Add executes and both proceed to build.
Consider one of:
- Use a singleflight.Group to collapse concurrent builds of the same key and return the first result to followers.
- Or switch to an atomic ContainsOrAdd (if available in the chosen LRU), or replace with a sync.Map and LoadOrStore to ensure atomicity.
- As a minimal mitigation, guard with a separate mutex around the check-add pair.
Do you want a targeted patch using singleflight?
59-63: Guard TTL may expire mid-build; prefer manual removal or longer TTLcurrentlyProcessing uses a 5m TTL but you also remove the key with defer. If a build exceeds 5m, the guard can expire and allow a duplicate to start.
- Remove TTL (if supported) and rely on explicit Remove in defer.
- Or increase TTL beyond the maximum expected build duration.
67-73: Clarify validation vs. defaulting semantics for target_platformYou default target_platform later, but validate before defaulting. If protovalidate requires target_platform, requests relying on the “host default” will be rejected.
Decide one of:
- If host default is intended: default target_platform before validation, or relax the validator to make it optional.
- If it’s required: remove the defaulting block and update docs accordingly.
Want me to patch the defaulting to occur before Validate?
137-141: Clean up partial output on build failureOn error, the output directory may contain partial artifacts and wastes disk.
binaryPath, err := builder.NewBuilder(path, opts...).Build(req.Msg.GetRrVersion()) if err != nil { - b.log.Error("fatal", zap.Error(err)) + b.log.Error("build failed", zap.Error(err)) + // cleanup partial output on failure + if rmErr := os.RemoveAll(outputPath); rmErr != nil { + b.log.Warn("failed to cleanup output directory", zap.String("path", outputPath), zap.Error(rmErr)) + } return nil, connect.NewError(connect.CodeInternal, fmt.Errorf("building plugins: %w", err)) }
45-58: Eviction cleanup: remove the directory once; drop redundant file removalRemoving rrBinPath and then the directory results in two RemoveAll calls. Removing the directory alone is sufficient and reduces syscalls/log noise.
- err := os.RemoveAll(rrBinPath) - if err != nil { - log.Error("removing cached file", zap.String("path", rrBinPath), zap.Error(err)) - } - // remove path - log.Info("removing cached directory", zap.String("path", filepath.Join(os.TempDir(), hash))) - err = os.RemoveAll(filepath.Join(os.TempDir(), hash)) + // Remove the whole temp directory for this build (it contains the binary). + dir := filepath.Join(os.TempDir(), hash) + log.Info("removing cached directory", zap.String("path", dir)) + err := os.RemoveAll(dir) if err != nil { - log.Error("failed to remove directory", zap.String("path", filepath.Join(os.TempDir(), hash)), zap.Error(err)) + log.Error("failed to remove directory", zap.String("path", dir), zap.Error(err)) }
143-149: Returning a server-local filesystem path is not useful for remote clientsRemote callers can’t read a local path on the server. Consider:
- Streaming the artifact bytes in a separate Download RPC or via HTTP GET with auth.
- Returning a pre-signed URL or content-addressed link to an object store.
- Or embedding the artifact in the Build response as bytes (with size limits), if acceptable.
v2/builder/builder.go (2)
276-277: Consider making GOPATH and GOCACHE configurable.The hardcoded paths might not work well in all environments. Consider making these configurable through builder options.
+ // Use configurable paths or fallback to defaults + gopath := filepath.Join(hd, "go", b.goos, b.goarch) + gocache := filepath.Join(hd, "go", b.goos, b.goarch, "go-build") + + // These could be made configurable via options in the future cmd.Env = append(cmd.Env, fmt.Sprintf("GOPATH=%s", filepath.Join(hd, "go", b.goos, b.goarch))) cmd.Env = append(cmd.Env, fmt.Sprintf("GOCACHE=%s", filepath.Join(hd, "go", b.goos, b.goarch, "go-build")))
311-338: Consider using os.Rename for better performance.The
moveFilefunction reads the entire file into memory and recreates it. For better performance, consider usingos.Renamefirst, falling back to copy-and-delete only when moving across filesystems.func moveFile(from, to string) error { + // Try rename first (fast path for same filesystem) + if err := os.Rename(from, to); err == nil { + return nil + } + + // Fallback to copy-and-delete for cross-filesystem moves ffInfo, err := os.Stat(from) if err != nil { return err } fFile, err := os.ReadFile(from) if err != nil { return err } toFile, err := os.Create(to) if err != nil { return err } err = toFile.Chmod(ffInfo.Mode()) if err != nil { return err } _, err = toFile.Write(fFile) if err != nil { return err } - return toFile.Close() + err = toFile.Close() + if err != nil { + return err + } + + // Remove original file after successful copy + return os.Remove(from) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
internal/cli/server/server.go(1 hunks)v2/builder/builder.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
internal/cli/server/server.go (8)
v2/cache/cache.go (1)
NewRRCache(14-19)gen/go/api/request/v1/request.pb.go (9)
BuildRequest(79-93)BuildRequest(106-106)BuildRequest(121-123)Plugin(160-168)Plugin(181-181)Plugin(196-198)Platform(25-33)Platform(46-46)Platform(61-63)gen/go/api/response/v1/response.pb.go (3)
BuildResponse(24-30)BuildResponse(43-43)BuildResponse(58-60)v2/builder/builder.go (2)
Builder(32-44)NewBuilder(47-58)builder/builder.go (2)
Builder(36-48)NewBuilder(51-63)v2/plugin/plugin.go (2)
Plugin(13-17)NewPlugin(23-29)v2/github/github.go (1)
NewHTTPClient(42-67)builder/options.go (5)
Option(10-10)WithOutputDir(48-52)WithRRVersion(55-59)WithGOOS(20-24)WithGOARCH(27-31)
v2/builder/builder.go (6)
builder/builder.go (1)
Builder(36-48)v2/plugin/plugin.go (1)
Plugin(13-17)v2/builder/options.go (1)
Option(11-11)v2/builder/templates/compile.go (5)
NewTemplate(22-37)CompileTemplateV2025(48-60)CompileTemplateV2024(76-88)CompileGoModTemplate2025(39-46)CompileGoModTemplate2024(62-74)config.go (3)
V2025(13-13)V2024(14-14)Debug(46-48)builder/templates/templateV2025.go (1)
CompileGoModTemplate2025(71-78)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (Go stable, OS ubuntu-latest)
🔇 Additional comments (6)
v2/builder/builder.go (6)
32-44: Consider removing modules field from struct.The struct contains a
modulesfield that's referenced in the AI summary but doesn't appear in the actual struct definition, suggesting this field may have been refactored out. The struct is well-designed with clear separation of concerns.
46-58: Constructor follows Go best practices.The constructor implementation is clean and follows Go conventions with proper options pattern application.
61-64: Good validation of required plugins.Early validation ensures plugins are provided before attempting to build, preventing wasted work.
98-110: Cleanup logic is well-structured.The defer cleanup with proper error handling and logging provides good housekeeping.
214-227: Robust version validation logic.The version validation handles both "master" branch and semantic versioning correctly, providing good backwards compatibility.
205-212: Well-designed Write method for output capturing.The Write method properly implements io.Writer interface and handles both logging and optional string builder capture.
Signed-off-by: Valery Piashchynski <[email protected]>
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
k6-benchmark.js (2)
52-55: LGTM: correct field name target_platform (matches proto).This fixes the earlier build_platform mismatch and should pass proto validation.
Also applies to: 206-210
235-243: Prevent crash when logging failed responses with empty/non-string bodies.
response.body.substring(...)will throw if body is empty or not a string, skewing results.Apply:
- if (!isSuccess) { - console.log(`Request failed: - Status: ${response.status} - Duration: ${response.timings.duration}ms - Body: ${response.body.substring(0, 200)}... - Request ID: ${uniquePayload.request_id} - Platform: ${randomPlatform.os} + ${randomPlatform.arch} - `); - } + if (!isSuccess) { + const bodyPreview = + typeof response.body === "string" ? response.body.substring(0, 200) : "<empty>"; + console.log(`Request failed: + Status: ${response.status} + Duration: ${response.timings.duration}ms + Body: ${bodyPreview}... + Request ID: ${uniquePayload.request_id} + Platform: ${randomPlatform.os} + ${randomPlatform.arch} + `); + }v2/github/github.go (2)
21-25: Introduce a sane per-file extraction limit to mitigate decompression bombsAdd a max per-file extraction size constant. This will be used in extract() to guard against zip bombs.
const ( rrOwner string = "roadrunner-server" rrRepo string = "roadrunner" zipExt string = ".zip" + // Guard against decompression bombs; tune as appropriate for your use case. + maxFileExtractSize int64 = 100 << 20 // 100 MiB )
251-288: Harden extraction: ensure parent dirs, forbid symlinks, limit decompressed sizeCurrent extraction can fail if directory entries are missing, is vulnerable to zip bombs, and allows symlinks. Harden it:
- Ensure parent directories exist for file entries.
- Reject symlink entries.
- Enforce a per-file decompressed size limit using both header size and an io.LimitedReader.
func extract(dest string, zf *zip.File) error { pt := filepath.Join(dest, zf.Name) //nolint:gosec if !strings.HasPrefix(pt, filepath.Clean(dest)+string(os.PathSeparator)) { return fmt.Errorf("invalid file path: %s", pt) } + // Disallow symlinks + if zf.FileInfo().Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("symlink entries are not allowed: %s", zf.Name) + } + if zf.FileInfo().IsDir() { err := os.MkdirAll(pt, os.ModePerm) if err != nil { return err } return nil } - destFile, err := os.OpenFile(pt, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, zf.Mode()) + // Ensure parent directory exists (some ZIPs may omit explicit dir entries) + if err := os.MkdirAll(filepath.Dir(pt), os.ModePerm); err != nil { + return err + } + + destFile, err := os.OpenFile(pt, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, zf.Mode()) if err != nil { return err } zippedFile, err := zf.Open() if err != nil { _ = destFile.Close() return err } - // G110: Potential DoS vulnerability via decompression bomb - _, err = io.Copy(destFile, zippedFile) //nolint:gosec + // Guard against decompression bombs + if zf.UncompressedSize64 > uint64(maxFileExtractSize) { + _ = destFile.Close() + _ = zippedFile.Close() + return fmt.Errorf("file too large after decompression: %s (%d bytes)", zf.Name, zf.UncompressedSize64) + } + lr := &io.LimitedReader{R: zippedFile, N: maxFileExtractSize} + _, err = io.Copy(destFile, lr) if err != nil { _ = destFile.Close() _ = zippedFile.Close() return err } + if lr.N == 0 { + _ = destFile.Close() + _ = zippedFile.Close() + return fmt.Errorf("file exceeds max allowed extracted size: %s", zf.Name) + } _ = destFile.Close() _ = zippedFile.Close() return nil }
🧹 Nitpick comments (10)
k6-benchmark.js (6)
26-26: Fix misleading comments (60s vs 30s / 120s).Comments contradict the actual values; align them to avoid confusion.
- duration: "60s", // for 30 seconds + duration: "60s", // for 60 seconds- timeout: "120s", // Allow up to 60 seconds for build operations + timeout: "120s", // Allow up to 120 seconds for build operationsAlso applies to: 217-217
1-5: Parameterize target URL/timeout via env and tag requests by platform.Improves portability (e.g., remote targets, CI) and observability (per-OS/arch stats).
Add base configuration:
import { uuidv4 } from "https://jslib.k6.io/k6-utils/1.4.0/index.js"; +// Base configuration (overridable via environment) +const BASE_URL = __ENV.BASE_URL || "http://127.0.0.1:9000"; +const BUILD_PATH = "/api.service.v1.BuildService/Build"; +const HTTP_TIMEOUT = __ENV.HTTP_TIMEOUT || "120s";Use it and add tags:
- const response = http.post( - "http://127.0.0.1:9000/api.service.v1.BuildService/Build", + const response = http.post( + `${BASE_URL}${BUILD_PATH}`, JSON.stringify(uniquePayload), { headers: headers, - timeout: "120s", // Allow up to 60 seconds for build operations + timeout: HTTP_TIMEOUT, // Configurable via HTTP_TIMEOUT env var + tags: { + os: randomPlatform.os, + arch: randomPlatform.arch, + }, }, );Update setup log:
- console.log( - "Target: http://127.0.0.1:9000/api.service.v1.BuildService/Build", - ); + console.log(`Target: ${BASE_URL}${BUILD_PATH}`);Also applies to: 212-219, 249-251
226-229: Make Content-Type check robust (case-insensitive, support Connect JSON).Connect endpoints may respond with application/connect+json; also header casing can vary.
- "content-type is application/json": (r) => - r.headers["Content-Type"] && - r.headers["Content-Type"].includes("application/json"), + "content-type is JSON (Connect or plain)": (r) => { + const ct = String( + r.headers["Content-Type"] || r.headers["content-type"] || "", + ).toLowerCase(); + return ( + ct.includes("application/json") || + ct.includes("application/connect+json") + ); + },
194-197: Broaden Accept header to include Connect JSON.Avoids 406/negotiation issues when server replies with Connect JSON.
const headers = { "Content-Type": "application/json", - Accept: "application/json", + Accept: "application/json, application/connect+json", };
232-232: Use explicit 0/1 for Rate metric to avoid implicit coercion.Minor clarity/readability improvement.
-errorRate.add(!isSuccess); +errorRate.add(isSuccess ? 0 : 1);
48-55: Avoid redundant default target_platform in base payload.You always overwrite it per-request; remove from the base payload to reduce confusion.
const payload = { request_id: "", force_rebuild: true, - target_platform: { - os: "linux", - arch: "amd64", - }, rr_version: "v2025.1.2", plugins: [v2/github/github.go (4)
96-100: Store a clone in cache to avoid aliasing with future readersStoring the same buffer pointer in cache risks accidental mutation/aliasing. Prefer storing an independent copy.
- // save zipped rr buffer - r.cache.Set(rrVersion, buf) + // save zipped rr buffer (store an independent copy) + r.cache.Set(rrVersion, bytes.NewBuffer(bytes.Clone(buf.Bytes())))
169-175: Use io.Copy to avoid short writes and reduce memory overheadWriting the full buffer with f.Write risks short writes and makes the log count misleading. io.Copy handles partial writes and is idiomatic here.
- n, err := f.Write(buf.Bytes()) - if err != nil { - return "", err - } - - r.log.Debug("repository saved", zap.Int("bytes written", n)) + if _, err := io.Copy(f, bytes.NewReader(buf.Bytes())); err != nil { + return "", err + } + r.log.Debug("repository saved", zap.Int("bytes written", buf.Len()))
205-213: Redundant/ineffective path check (Abs removes “..”)Computing Abs(rc.File[0].Name) cleans the path so the subsequent strings.Contains(abs, "..") will never trigger. The following outDir check already covers “..”. Remove the dead code.
- abs, err := filepath.Abs(rc.File[0].Name) - if err != nil { - return "", err - } - // for this repository (roadrunner-server/roadrunner), 0-th element is a directory with content - if strings.Contains(abs, "..") { - return "", errors.New("path should not contain the '..' symbols") - } - outDir := rc.File[0].Name if strings.Contains(outDir, "..") { return "", errors.New("CWE-22, output dir from a zip file can't contain a '..' filesystem operation, more info: https://cwe.mitre.org/data/definitions/22.html") }
237-249: Heuristic for tag vs branch can misclassify branch names starting with 'v'If a branch is named e.g. "v-next", it will be treated as a tag. Consider making the tag detection stricter (e.g., v prefix), or allow caller to indicate type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
k6-benchmark.js(1 hunks)v2/github/github.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build (Go stable, OS ubuntu-latest)
Reason for This PR
Description of Changes
GITHUB_TOKENmight be set, but is not required to download an RR base template from GitHub.vx server -a 127.0.0.1:9000.Request example:
{ "request_id": "b66d5617-64dd-419b-a68b-b002938320ab", "force_rebuild": true, "target_platform": { "os": "windows", "arch": "amd64" }, "rr_version": "v2025.1.2", "plugins": [ { "module_name": "github.com/roadrunner-server/http/v5", "tag": "v5.2.7" } ] }cURL:
100 concurrent users are bombing with requests to build RR with all available plugins for the different platforms and archs:
Top memory usage - 42mb.
k6 results:
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit