Skip to content

Commit 07e26d5

Browse files
fix: place gRPC proto at core-standard path and make grpc boot import-safe (#14)
Addresses review findings on the optional gRPC server. #1 (main.py): the grpc server was imported at module top level, so importing src.main required the generated stubs. Runtime.Init runs GenerateOpenAPI, which imports src.main *before* Sync generates those stubs (and they are git-ignored, so a fresh clone has none) — Init failed with ModuleNotFoundError. The import now lives inside the startup handler; OpenAPI generation never fires startup, so the module imports cleanly without stubs. #2 (proto location): the proto was scaffolded at code/proto/api.proto, but core's LoadEndpoints re-derives the gRPC contract from standards.ProtoPath (proto/api.proto at the service root) and the manifest stores no proto bytes. The endpoint therefore reloaded with zero RPCs and dependent services could not generate clients. The proto now lives at the service-root standard path (Buf and grpcEndpoint read it there), and defaultProtoPath is bound to standards.ProtoPath so it can never drift again. #3 (server.py): add_insecure_port returns 0 instead of raising when a port can't be bound, so the server would "start" listening on nothing while FastAPI stayed healthy. It now raises, which also fails the pod's HTTP readiness probe via the shared lifespan (covers the gRPC-readiness gap). Tests: added a reload regression test asserting the gRPC endpoint keeps its RPCs after LoadEndpoints (would have caught #2), and a check that src.rpc is never imported at module top level (would have caught #1). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 2ff5d38 commit 07e26d5

9 files changed

Lines changed: 80 additions & 22 deletions

File tree

‎builder.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,12 @@ func (s *Builder) Sync(ctx context.Context, _ *builderv0.SyncRequest) (*builderv
140140
}
141141

142142
// syncGRPCServer regenerates the Python protobuf + grpc.aio server stubs from
143-
// the service-owned proto contract via Buf. Generation is cached on the proto
144-
// tree, so it re-runs deterministically only when the contract changes.
143+
// the service-owned proto contract via Buf. Buf reads proto/ under the service
144+
// root (matching the endpoint contract location) and writes the stubs into the
145+
// Python source tree. Generation is cached on the proto tree, so it re-runs
146+
// deterministically only when the contract changes.
145147
func (s *Builder) syncGRPCServer(ctx context.Context) error {
146-
buf, err := proto.NewBuf(ctx, s.Local("code"))
148+
buf, err := proto.NewBuf(ctx, s.Location)
147149
if err != nil {
148150
return s.Wool.Wrapf(err, "cannot create proto generator")
149151
}
@@ -406,11 +408,12 @@ func (s *Builder) CreateEndpoints(ctx context.Context) error {
406408
}
407409

408410
// grpcEndpoint builds the service-owned gRPC endpoint from the proto contract.
409-
// The proto path is resolved relative to the Python source dir (code/), where
410-
// the scaffolded proto/ tree and the generated stubs both live. It inherits the
411-
// same public/private visibility as the REST endpoint.
411+
// The proto path is resolved relative to the service root — the same location
412+
// core's LoadEndpoints re-reads it from — so the endpoint keeps its RPCs across
413+
// reloads and stays consumable by dependent services. It inherits the same
414+
// public/private visibility as the REST endpoint.
412415
func (s *Builder) grpcEndpoint(ctx context.Context) (*basev0.Endpoint, error) {
413-
protoPath := s.Local("code/%s", s.FastAPI.Settings.GRPCServer.Proto)
416+
protoPath := s.Local("%s", s.FastAPI.Settings.GRPCServer.Proto)
414417
grpc, err := resources.LoadGrpcAPI(ctx, shared.Pointer(protoPath))
415418
if err != nil {
416419
return nil, s.Wool.Wrapf(err, "cannot load grpc proto %q", protoPath)

‎grpc_test.go‎

Lines changed: 51 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"os"
77
"path"
88
"path/filepath"
9+
"strings"
910
"testing"
1011
"time"
1112

@@ -91,7 +92,7 @@ func TestCreateRESTOnlyByDefault(t *testing.T) {
9192
require.Equal(t, 1, len(builder.Endpoints))
9293
require.Nil(t, builder.FastAPI.GRPCEndpoint)
9394

94-
_, err := os.Stat(filepath.Join(root, "code/proto/api.proto"))
95+
_, err := os.Stat(filepath.Join(root, "proto/api.proto"))
9596
require.True(t, os.IsNotExist(err))
9697
_, err = os.Stat(filepath.Join(root, "code/src/rpc/server.py"))
9798
require.True(t, os.IsNotExist(err))
@@ -123,8 +124,8 @@ func TestCreateWithGRPCServer(t *testing.T) {
123124
require.Equal(t, standards.GRPC, builder.FastAPI.GRPCEndpoint.Api)
124125

125126
for _, rel := range []string{
126-
"code/proto/api.proto",
127-
"code/proto/buf.gen.yaml",
127+
"proto/api.proto",
128+
"proto/buf.gen.yaml",
128129
"code/src/rpc/server.py",
129130
"code/src/rpc/servicer.py",
130131
"code/tests/rpc/test_grpc.py",
@@ -139,10 +140,54 @@ func TestCreateWithGRPCServer(t *testing.T) {
139140
require.Contains(t, string(pyproject), "grpcio")
140141
require.Contains(t, string(pyproject), "grpcio-tools")
141142

142-
main, err := os.ReadFile(filepath.Join(root, "code/src/main.py"))
143+
assertMainImportsGRPCLazily(t, filepath.Join(root, "code/src/main.py"))
144+
}
145+
146+
// assertMainImportsGRPCLazily guards the fix for the eager-import bug: importing
147+
// src.main (which openapi.py does at Init, before Sync generates the stubs) must
148+
// not require the generated gRPC package. The server import therefore lives
149+
// inside the startup handler (indented), never at module top level.
150+
func assertMainImportsGRPCLazily(t *testing.T, mainPath string) {
151+
t.Helper()
152+
content, err := os.ReadFile(mainPath)
153+
require.NoError(t, err)
154+
main := string(content)
155+
require.Contains(t, main, "CODEFLY_GRPC_PORT")
156+
require.Contains(t, main, "from src.rpc.server import serve")
157+
for _, line := range strings.Split(main, "\n") {
158+
if strings.HasPrefix(line, "from src.rpc.server import") || strings.HasPrefix(line, "import src.rpc") {
159+
t.Fatalf("src.rpc must not be imported at module top level (breaks openapi generation before sync): %q", line)
160+
}
161+
}
162+
}
163+
164+
// TestGRPCEndpointReloadsContract is the regression guard for the proto
165+
// location: core's LoadEndpoints re-derives the gRPC contract from disk at
166+
// standards.ProtoPath (service root). If the scaffolded proto lived anywhere
167+
// else, the reloaded endpoint would silently carry zero RPCs and dependent
168+
// services could not generate clients.
169+
func TestGRPCEndpointReloadsContract(t *testing.T) {
170+
builder, _ := createServiceForTest(t, map[string]*agentv0.Answer{
171+
HotReload: confirmAnswer(false),
172+
PublicEndpoint: confirmAnswer(false),
173+
GRPCServer: confirmAnswer(true),
174+
})
175+
ctx := context.Background()
176+
177+
endpoints, err := builder.Base.Service.LoadEndpoints(ctx)
143178
require.NoError(t, err)
144-
require.Contains(t, string(main), "CODEFLY_GRPC_PORT")
145-
require.Contains(t, string(main), "from src.rpc.server import serve")
179+
180+
grpcEndpoint, err := resources.FindGRPCEndpoint(ctx, endpoints)
181+
require.NoError(t, err)
182+
grpc := resources.IsGRPC(ctx, grpcEndpoint)
183+
require.NotNil(t, grpc)
184+
require.NotEmpty(t, grpc.Rpcs, "reloaded gRPC endpoint lost its RPCs — proto not at core-standard path")
185+
186+
var names []string
187+
for _, rpc := range grpc.Rpcs {
188+
names = append(names, rpc.Name)
189+
}
190+
require.Contains(t, names, "Echo")
146191
}
147192

148193
// TestDeploymentRendersGRPCPort proves the gRPC container/service ports appear

‎main.go‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
runnersbase "github.com/codefly-dev/core/runners/base"
1919
pythonrunner "github.com/codefly-dev/core/runners/python"
2020
"github.com/codefly-dev/core/shared"
21+
"github.com/codefly-dev/core/standards"
2122
"github.com/codefly-dev/core/templates"
2223
"github.com/codefly-dev/core/toolbox/lang"
2324

@@ -43,9 +44,11 @@ const (
4344
)
4445

4546
// defaultProtoPath is where the service-owned proto contract lives, relative
46-
// to the Python source dir (code/). Mirrors how openapi/api.swagger.json sits
47-
// beside the source. Buf generation and the gRPC endpoint both resolve it.
48-
const defaultProtoPath = "proto/api.proto"
47+
// to the service root. It is core's standards.ProtoPath: LoadEndpoints
48+
// (Builder + Runtime) and any dependent service re-derive the gRPC contract
49+
// from that exact location, so the proto has to sit there — the same way the
50+
// REST contract lives at openapi/api.swagger.json.
51+
const defaultProtoPath = standards.ProtoPath
4952

5053
// GRPCServerSettings configures the optional service-owned grpc.aio listener.
5154
// Disabled by default: an unset grpc-server block leaves the service REST-only

‎templates/factory/code/src/main.py.tmpl‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -56,14 +56,17 @@ for plugin in plugins:
5656
# gRPC server — runs a grpc.aio listener in this process alongside FastAPI.
5757
# The port is provided by codefly at runtime; it falls back to the standard
5858
# gRPC port in container/Kubernetes where the mapping is fixed.
59-
from src.rpc.server import serve as _grpc_serve
60-
6159
_grpc_server = None
6260

6361

6462
@app.on_event("startup")
6563
async def _start_grpc():
6664
global _grpc_server
65+
# Imported inside the handler, not at module load: the generated stubs
66+
# are produced by `codefly sync` and importing this module must not depend
67+
# on them (openapi.py imports src.main to emit the schema before sync).
68+
from src.rpc.server import serve as _grpc_serve
69+
6770
port = int(os.environ.get("CODEFLY_GRPC_PORT", "9090"))
6871
_grpc_server = await _grpc_serve(port)
6972

‎templates/grpc/code/src/rpc/server.py.tmpl‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ async def serve(port: int) -> grpc.aio.Server:
3434
"""Start a grpc.aio server on ``port`` and return it (already started)."""
3535
server = grpc.aio.server()
3636
register(server)
37-
server.add_insecure_port(f"[::]:{port}")
37+
# add_insecure_port returns 0 (it does not raise) when the port can't be
38+
# bound; without this check the server would "start" listening on nothing
39+
# while FastAPI stays healthy, masking the failure.
40+
if server.add_insecure_port(f"[::]:{port}") == 0:
41+
raise RuntimeError(f"gRPC server could not bind port {port}")
3842
await server.start()
3943
return server

‎templates/grpc/code/tests/rpc/conftest.py.tmpl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import sys
55
# The gRPC stubs are produced by `codefly sync` (Buf) before a normal test run.
66
# Generate them here too so the suite is self-contained when run directly.
77
_CODE = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", ".."))
8-
_PROTO = os.path.join(_CODE, "proto")
8+
_PROTO = os.path.abspath(os.path.join(_CODE, "..", "proto"))
99
_GENERATED = os.path.join(_CODE, "src", "rpc", "_generated")
1010

1111

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
version: v1
22
plugins:
33
- plugin: buf.build/protocolbuffers/python
4-
out: ../src/rpc/_generated
4+
out: ../code/src/rpc/_generated
55
- plugin: buf.build/grpc/python
6-
out: ../src/rpc/_generated
6+
out: ../code/src/rpc/_generated

0 commit comments

Comments
 (0)