Skip to content

Conversation

@jog1t
Copy link
Contributor

@jog1t jog1t commented Jan 2, 2026

No description provided.

@vercel
Copy link

vercel bot commented Jan 2, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Review Updated (UTC)
rivetkit-serverless Error Error Jan 3, 2026 0:06am
3 Skipped Deployments
Project Deployment Review Updated (UTC)
rivet-cloud Ignored Ignored Jan 3, 2026 0:06am
rivet-inspector Ignored Ignored Preview Jan 3, 2026 0:06am
rivet-site Ignored Ignored Preview Jan 3, 2026 0:06am

Copy link
Contributor Author

jog1t commented Jan 2, 2026

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@jog1t jog1t marked this pull request as ready for review January 2, 2026 23:37
@claude
Copy link

claude bot commented Jan 2, 2026

PR Review: Update engine-full SDK

This PR updates the generated SDKs (Go and Rust) for the engine-full API, adding error reporting structures for Rivet Actors and runner pools. While the OpenAPI schema changes look good, there are critical bugs in the generated Go SDK that will prevent compilation.


Critical Issues ❌

1. Duplicate Field Names in Go Struct (engine/sdks/go/api-full/types.go:1827-1838)

The RunnerPoolError struct has duplicate field name stringLiteral declared three times:

type RunnerPoolError struct {
    typeName string
    RunnerPoolErrorServerlessHttpError *RunnerPoolErrorServerlessHttpError
    stringLiteral                      string  // ❌ First declaration
    RunnerPoolErrorServerlessConnectionError *RunnerPoolErrorServerlessConnectionError
    stringLiteral                            string  // ❌ Duplicate!
    RunnerPoolErrorServerlessInvalidPayload *RunnerPoolErrorServerlessInvalidPayload
    stringLiteral                           string  // ❌ Duplicate!
}

Impact: This code will not compile in Go. Go does not allow duplicate field names in structs.

Root Cause: The OpenAPI generator is incorrectly handling the oneOf with multiple string literal enum variants. Each string literal variant (serverless_stream_ended_early, serverless_invalid_base64, internal_error) should share a single stringLiteral field, not create separate ones.

2. Duplicate Method Declarations (engine/sdks/go/api-full/types.go:1864-1874)

The StringLiteral() method is declared three times on the same receiver:

func (r *RunnerPoolError) StringLiteral() string {
    return r.stringLiteral
}

func (r *RunnerPoolError) StringLiteral() string {  // ❌ Duplicate!
    return r.stringLiteral
}

func (r *RunnerPoolError) StringLiteral() string {  // ❌ Duplicate!
    return r.stringLiteral
}

Impact: Go does not allow duplicate method declarations. This will cause a compilation error.

3. Duplicate NewRunnerPoolErrorWithStringLiteral() Constructor (engine/sdks/go/api-full/types.go:1844-1862)

The same constructor function name is used three times, each hardcoding different string literal values:

func NewRunnerPoolErrorWithStringLiteral() *RunnerPoolError {
    return &RunnerPoolError{typeName: "stringLiteral", stringLiteral: "serverless_stream_ended_early"}
}

func NewRunnerPoolErrorWithStringLiteral() *RunnerPoolError {  // ❌ Duplicate!
    return &RunnerPoolError{typeName: "stringLiteral", stringLiteral: "serverless_invalid_base64"}
}

func NewRunnerPoolErrorWithStringLiteral() *RunnerPoolError {  // ❌ Duplicate!
    return &RunnerPoolError{typeName: "stringLiteral", stringLiteral: "internal_error"}
}

Impact: Only the last function declaration will be used, making it impossible to construct the other string literal variants.

Expected: Each variant should have a unique constructor name like:

  • NewRunnerPoolErrorServerlessStreamEndedEarly()
  • NewRunnerPoolErrorServerlessInvalidBase64()
  • NewRunnerPoolErrorInternalError()

4. Duplicate VisitStringLiteral Methods in Visitor Interface (engine/sdks/go/api-full/types.go:1941-1948)

The visitor interface has the same method signature declared three times:

type RunnerPoolErrorVisitor interface {
    VisitRunnerPoolErrorServerlessHttpError(*RunnerPoolErrorServerlessHttpError) error
    VisitStringLiteral(string) error
    VisitRunnerPoolErrorServerlessConnectionError(*RunnerPoolErrorServerlessConnectionError) error
    VisitStringLiteral(string) error  // ❌ Duplicate!
    VisitRunnerPoolErrorServerlessInvalidPayload(*RunnerPoolErrorServerlessInvalidPayload) error
    VisitStringLiteral(string) error  // ❌ Duplicate!
}

Impact: While this might technically compile (Go allows duplicate method signatures in interfaces), it's semantically incorrect and suggests the generator doesn't understand that all string literal variants should use the same visitor method.

5. Non-Deterministic MarshalJSON Behavior (engine/sdks/go/api-full/types.go:1922-1939)

The MarshalJSON has duplicate case labels for "stringLiteral":

func (r RunnerPoolError) MarshalJSON() ([]byte, error) {
    switch r.typeName {
    case "runnerPoolErrorServerlessHttpError":
        return json.Marshal(r.RunnerPoolErrorServerlessHttpError)
    case "stringLiteral":
        return json.Marshal("serverless_stream_ended_early")
    case "runnerPoolErrorServerlessConnectionError":
        return json.Marshal(r.RunnerPoolErrorServerlessConnectionError)
    case "stringLiteral":  // ❌ Unreachable code!
        return json.Marshal("serverless_invalid_base64")
    case "runnerPoolErrorServerlessInvalidPayload":
        return json.Marshal(r.RunnerPoolErrorServerlessInvalidPayload)
    case "stringLiteral":  // ❌ Unreachable code!
        return json.Marshal("internal_error")
    }
}

Impact: Only the first "stringLiteral" case will ever execute. The other two variants can never be serialized correctly.


The Same Issues Likely Exist in ActorError Type

The ActorError type (engine/sdks/go/api-full/types.go:145+) appears to have similar structure with string literal variants, so it likely suffers from the same bugs, though it has fewer variants (only no_capacity).


Root Cause Analysis

These issues stem from the OpenAPI code generator's handling of oneOf schemas that mix:

  1. Object types with discriminator fields
  2. String enum literals

The generator appears to create separate fields/methods for each variant instead of recognizing that all string literal variants should:

  • Share a single field in the union struct
  • Use unique type names to distinguish between variants
  • Have unique constructor names

Recommendations

Immediate Actions Required:

  1. Do not merge this PR - The Go SDK is broken and will not compile
  2. Fix the OpenAPI generator configuration or update the generator version to handle oneOf with mixed types correctly
  3. Add compilation checks to the SDK generation pipeline to catch these issues automatically
  4. Regenerate the SDKs after fixing the generator

Long-term Improvements:

  1. Add SDK build tests to CI/CD - Run go build on the Go SDK and cargo build on the Rust SDK as part of PR validation
  2. Consider manual SDK review for complex schema changes involving unions/oneOf
  3. Document the OpenAPI generator version and configuration used for reproducibility

Positive Notes ✅

  1. OpenAPI schema looks correct - The oneOf structure in openapi.json is valid and well-documented
  2. Rust SDK appears correct - Quick review shows proper enum variants and no obvious compilation issues
  3. Good error modeling - The error taxonomy (pool errors, capacity, runner non-response) provides useful diagnostic information

Questions for the Team

  1. What OpenAPI generator version is being used?
  2. Is there a custom template or configuration for the Go SDK generation?
  3. Are there existing tests that should have caught these compilation errors?
  4. Should we consider switching to a different Go SDK generator (e.g., oapi-codegen, ogen)?

Verdict: Cannot approve due to critical compilation errors in the Go SDK. The Rust SDK and OpenAPI schema changes are good, but the Go SDK must be regenerated with a fixed generator configuration.

@graphite-app
Copy link
Contributor

graphite-app bot commented Jan 2, 2026

Graphite Automations

"Test" took an action on this PR • (01/02/26)

1 assignee was added to this PR based on Kacper Wojciechowski's automation.

@NathanFlurry NathanFlurry changed the base branch from 12-23-feat_engine_return_error_for_pending_actors to graphite-base/3740 January 2, 2026 23:59
@jog1t jog1t force-pushed the 01-02-chore_update_engine-full_sdk branch from 93bc627 to 0d44a67 Compare January 3, 2026 00:05
@jog1t jog1t force-pushed the graphite-base/3740 branch from 653f032 to d0406b1 Compare January 3, 2026 00:05
@jog1t jog1t changed the base branch from graphite-base/3740 to 12-23-feat_engine_return_error_for_pending_actors January 3, 2026 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants