Multi-node fleet architecture: sous-api + souslet over mTLS gRPC - #1
Merged
Conversation
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
- Add protoc 27.0 download to Makefile proto target for reproducibility - Mark google.golang.org/grpc and google.golang.org/protobuf as direct dependencies in go.mod - Run go mod tidy to properly resolve all dependencies - Regenerate code with pinned protoc 27.0 - Add .bin/ to .gitignore to exclude downloaded protoc binary Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…lated Send Accepts each node's long-lived Connect stream, feeds NodeSnapshot into nodecatalog, and lets the rest of sous-api send a command to a connected node and block for the stream_id-correlated reply.
…nect
Connect's write loop only exited if nc.send was closed (never happened) or
its own stream.Send failed. Whenever the read loop was the one to notice
the stream died - the common case, a client hanging up surfaces as Recv
returning io.EOF - the write loop was left blocked forever on the now
orphaned, never-drained nc.send channel: one leaked goroutine per node
disconnect, forever, in a system whose whole premise is nodes reconnecting.
Add nc.done, closed exactly once by Connect's cleanup, that the write loop
and Send's enqueue step both select on. Never close nc.send itself -
Send can be writing to it concurrently, and closing a channel a writer may
still be sending on panics ("send on closed channel"); nc.done sidesteps
that entirely since only reads ever happen on it from any goroutine other
than the single closer.
Verified with two new tests: one drives 60 connect/disconnect cycles over
a shared connection and asserts the goroutine count stays flat (proven
non-leaking at both 20 and 60 cycles - a real per-cycle leak would scale
with cycle count and didn't); reverting the fix locally reproduced a
scaling +65 delta over 60 cycles, confirming the test actually catches the
regression. The other fires 50 concurrent Send calls against a node while
tearing down its connection and asserts no panic (via recover); trying the
naive close(nc.send) anti-pattern instead reliably reproduced "send on
closed channel" under this same test, confirming it's sensitive to exactly
the failure mode being avoided.
…s pending Send's reply-wait select only had <-waiter and the dead context.Background().Done() branch, so a call already blocked waiting for its correlated reply when the node dropped would hang forever - not a rare edge case for a system whose whole premise is nodes reconnecting. Worse, the stale nc.pending[stream_id] entry was never cleaned up, keeping the whole nodeConn (maps, channels, buffered envelopes) reachable through it indefinitely alongside the leaked goroutine. Add a <-nc.done case to that select, matching the one the enqueue step already had: return an error and delete the pending entry. Verified with TestSendUnblocksWithErrorWhenNodeDisconnectsMidWait: fires a Send with no reply loop driving it (so it's genuinely parked on <-waiter, not racing an incoming reply), disconnects the node, and asserts the call returns the expected error within 2s. Reverting just this change locally reproduced the hang (test correctly times out after 2s with the exact "did not unblock" failure), confirming the test catches the regression; restoring the fix returns it to 25/25 clean across 5 repeated full-package runs. Full suite: 443 passed, 27 packages. Assessed the reviewer's secondary note (buffered nc.send can still accept an enqueue even after nc.done closes, since Go's select doesn't prioritize among ready cases): any Send that gets past the enqueue step this way still lands in the reply-wait select fixed here, so it can no longer hang either way - the fix above covers the practical outcome without needing a second change on the enqueue side.
souslet-side handlers turning DeployCommand/UndeployCommand/FetchCommand/
DeleteWeightsCommand into calls against the existing deploy.Runtime,
fetch.Manager and engine packages, unchanged from single-node Sous.
HandleDeleteWeights wraps a deleteWeights placeholder ("not yet
implemented") that Task 11 replaces with the relocated larder guard logic.
Adapted from the plan's illustrative code to the real interfaces:
deploy.Runtime.Start takes engine.Spec directly (no ad-hoc
ContainerName()-only interface), engine.ContainerState has no Phase
field so Snapshot reports Docker's raw Status word instead, and the
test recipe needed Image/Modality set to pass recipe.Validate().
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…eightsGib/KvGib Snapshot previously left DeploymentState.WeightsGib/KvGib at zero unconditionally, which would misrepresent every node's capacity once a later task's UI sums these across deployments. HandleDeploy now caches each deployed recipe's declared footprint (recipe.Footprint) in an in-memory, mutex-guarded map keyed by recipe ID; HandleUndeploy evicts it; Snapshot reads from it, falling back to 0 (an honest "unknown", not a fabrication) for a recipe ID this souslet process never deployed itself, e.g. a container surviving a souslet restart. This is declared, not measured, accounting - single-node Sous's declared-vs-observed refinement has no equivalent here since souslet keeps no persistent store, which is an accepted simplification per the multi-node plan. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Client.Run dials sous-api, sends the initial NodeSnapshot, and dispatches incoming Envelopes to Task 5's Handlers, reconnecting with capped exponential backoff on any stream error. cmd/souslet is the process entrypoint, wiring engine.Docker/fetch.Manager/mtls into the client. Fixes one real concurrency bug found while implementing the brief as given: dispatch spawns one goroutine per incoming Envelope, and gRPC's ClientStream.SendMsg is explicitly documented as unsafe to call on the same stream from different goroutines - two commands arriving close together would otherwise call stream.Send concurrently. A per-connection sendMu (scoped to one connectOnce call, not the whole Client, so a stale goroutine from an already-dead stream can never block a fresh reconnect's sends) serializes every Send against a given stream. Verified in client_test.go: the brief's own dispatch test, a barrier-synchronized test proving two concurrent Handlers calls don't race on Send, and a test proving a dispatch goroutine still in flight when connectOnce returns (stream already dead) neither panics nor blocks - its Send just errors and gets logged, and Run still shuts down promptly on ctx cancellation.
connectOnce blocks inside its receive loop for as long as the stream stays healthy and only ever returns on error, so Run's success branch (backoff = time.Second) was unreachable dead code: backoff only ever climbed and never reset, even after a connection had been stable for days. Every later disconnect's first retry inherited whatever level the last failure streak had left it at. Fix: connectOnce now takes a resetBackoff callback, invoked the moment the initial NodeSnapshot send succeeds - the earliest point the connection is actually confirmed healthy - rather than waiting for connectOnce to return (which never happens while the connection is good). Also clamps backoff to the 30s cap after doubling instead of gating the doubling on the pre-multiply value, which let 16s double to 32s. Regression test (TestRunResetsBackoffAfterAConnectionBecomesHealthyAgain) forces two dial failures (ratcheting backoff 1s -> 2s with no chance to reset), then a third connection that completes its handshake and later drops, and asserts that drop's retry log reports 1s, not 2s. Verified against the unfixed code first: it reported 4s (three consecutive ratchets: 1s->2s->4s), confirming the test catches the regression it's meant to.
…oped
Adds node-scoped POST /api/deploy/{id}/{nodeID}, POST /api/undeploy/{id}/{nodeID}
and GET /api/plan/{id}/{nodeID} alongside the existing single-node routes,
which keep working through deploy.Manager unchanged (kept during the
multi-node migration period, per the rollout plan; removed in Task 14).
- deploy_grpc.go: deployToNode/undeployFromNode send DeployCommand/
UndeployCommand to a specific connected node over grpcserver.Server.Send
and wait for the correlated reply, per the task brief. planOnNode
deliberately reads margin from nodecatalog's cached snapshot instead of
a live RPC: no souslet-side handler for PlanCommand exists anywhere in
this design (grpcclient.Handlers only wires Deploy/Undeploy/Fetch/
DeleteWeights), so an RPC there would just go unanswered - same
brief-vs-reality call the Task 2 review made for VerifiedNodeID.
- Server gains gsrv *grpcserver.Server and nodes *nodecatalog.Catalog
fields alongside the existing mgr *deploy.Manager (not replacing it -
mgr stays load-bearing for status/modelview/plan/alias/gateway well
beyond this task's scope, and removing it now would break far more than
deploy/undeploy/plan). New(...) takes two new trailing params; nil is
valid for a single-node caller with no souslet fleet. cmd/sous/main.go
updated to pass nil, nil to keep building.
- handlers.go: deploy/undeploy/plan branch on r.PathValue("nodeID") -
present routes to the new gRPC path, absent falls through to the
untouched legacy s.mgr path.
TDD: confirmed RED (deploy_grpc_test.go fails to compile without
deploy_grpc.go), then GREEN. Full existing internal/httpapi suite (143
tests) plus 10 new tests all pass; go build ./... and go vet ./... clean.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ed plan Two findings from Task 8 review, both verified with a real repro before and after the fix: - Critical: the three node-scoped routes were registered unconditionally in New(), but cmd/sous - the single-node binary actually deployed on gx10 - passes nil for gsrv/nodes. grpcserver.Server.Send and nodecatalog.Catalog.Node both lock an embedded sync.RWMutex on entry, which nil-panics on a nil receiver, reachable via a bare HTTP request against the currently-deployed binary. Fix: only register the three routes when gsrv != nil && nodes != nil, so they simply don't exist on a nil-configured server (a normal 404/405 through net/http's ServeMux, never a panic - confirmed both status codes and the reasoning behind the asymmetry with an isolated repro). New test TestNodeScopedRoutesReturnCleanErrorsWhenGRPCIsNotConfigured builds a Server the same way cmd/sous does and hits all three node-scoped paths. - Important: planOnNode's capacity.Planner omitted WarnFreeGiB, silently dropping the swap-risk warning (a real, fleet-calibrated safety signal, not cosmetic) for every node-scoped plan/deploy. Fix: hardcode WarnFreeGiB: 12, matching the constant cmd/sous/main.go already ships for the legacy path. New test TestPlanOnNodeWarnsWhenMarginIsThin. Both fixes verified RED (reverted locally, confirmed the exact panic / the exact silently-empty Warning) then GREEN. Full internal/httpapi suite (151 tests) and the whole repo's go build/vet/test all clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Wires internal/catalog, internal/nodecatalog, internal/grpcserver and internal/httpapi together into the sous-api binary: an mTLS gRPC listener (souslet-facing) alongside the existing HTTP listener, both serving out of the same process. Every httpapi.New argument that isn't node/gRPC-specific is constructed the same way cmd/sous/main.go constructs it; gsrv/nodes are populated for real here instead of the nil, nil cmd/sous passes. Adds mtls.(*CA).Save/LoadCA (JSON-encoded cert PEM + key DER + known- node set, 0600) so the CA survives a restart without invalidating every already-issued node cert.
Gateway.Proxy gains an additive multi-node path (Nodes/GRPC fields): when both are set it resolves the target node via nodecatalog.NodeFor and relays the HTTP request/response over that node's gRPC Connect stream instead of dialing a local container port, with real chunk-by-chunk flushing so SSE streaming keeps working end to end. Pre-existing Res/Cat local-forward path is untouched and still the default when Nodes/GRPC are nil. grpcserver.Server gains OpenProxyStream/ProxyStream, with its own proxyStreams registration map (many replies per stream_id, cleaned up on Close) kept separate from Send's existing single-shot pending map - the read loop in Connect now routes an incoming envelope to whichever map actually has a waiter for its stream_id. RecvHead/RecvChunk are bounded: both select on nc.done, so a node that disconnects mid-response returns an error instead of hanging the original HTTP client forever. grpcclient.Client's dispatch loop reassembles a proxied request's head + chunk(s) synchronously (avoiding a goroutine-ordering race between a head and its own chunks) before spawning handleProxyRequest, which forwards to the local model container's HTTP port (looked up by the new Handlers.portFor, populated by HandleDeploy/HandleUndeploy alongside the existing footprint tracking) and streams the response back through the same sendMu-guarded send path Task 6 already established. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…isconnect Review found two Important issues in the gRPC proxy path: 1. The outbound request body was sent as one stream.SendChunk call for the whole already-buffered body, not actually chunked despite HTTPRequestChunk existing for exactly this. grpc-go defaults to a 4MB max receive message size, and this gateway's own maxRequestBytes (32MB, "audio uploads are the large case") documents that bodies past 4MB are the expected case, not an edge case - a single oversized message fails with ResourceExhausted inside souslet's receive loop, and per Run's reconnect-on-any-stream-error design, that drops the WHOLE node's connection, not just the one request. Fixed with sendChunkedProxyBody, 4096-byte chunks mirroring handleProxyRequest's existing response-side convention. Verified with a test that proves both byte-for-byte round-tripping AND that the body actually arrived as more than one HTTPRequestChunk (via a fake souslet's own chunk count). 2. The response relay loop discarded w.Write's error and never checked r.Context(), so a client disconnecting mid-stream (browser navigation, client-side cancel - normal for long LLM/TTS/ASR generations) left the loop draining and discarding chunks until the response completed naturally or the node disconnected. The comment claiming this "mirrors ReverseProxy's own behavior" was also wrong - ReverseProxy's copyBuffer does check its write error. Fixed: the loop now returns (releasing stream.Close()) on either a failed write or r.Context().Err() != nil. This bounds the GATEWAY side's resource usage; it does not (cannot, without a new proto message type) tell souslet to stop generating - documented as a disclosed, deferred limitation rather than left silent. Verified with a test using a fake souslet that streams indefinitely and a client context cancelled mid-stream. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
deployToNode now checks the node's last-known CachedWeightRepos before deploying: if the recipe's model isn't there, it sends a FetchCommand and waits for phase "done" before sending the DeployCommand, so a node deploy for a never-downloaded model gets an explicit, waited-on fetch step instead of failing (or silently triggering souslet's own on-demand fetch mid-deploy). This requires (*grpcserver.Server).Send to take a context.Context so a fetch can use a long-but-bounded timeout (30m) while ordinary deploy/undeploy/plan calls keep a short one (5s) - every call site across internal/httpapi and internal/grpcserver (including tests) is updated to pass one explicitly. Also adds Server.Catalog() and Server.Connected(nodeID), both used by the new dialFakeSousletRecording test helper: Connect's handshake snapshot is a full replace, so re-sending a bare NodeSnapshot on dial would wipe out a test's pre-configured CachedWeightRepos, and polling the catalog's own Connected flag races the moment the connection is actually registered in Server's internal map.
…s ordering Review found two real issues in the fetch-orchestration work: 1. deployToNode's fetch step sent exactly one FetchCommand and treated any reply other than an immediate "done" as a hard failure. Souslet's real fetch.Manager.Start returns "downloading" immediately for a genuine cache miss and only reaches "done" later - it does not itself wait for the download. Replaced the single Send-and-check with fetchWeights, a poll loop bounded by fetchTimeout: "done" succeeds, "failed"/"absent" fail immediately, "downloading" sleeps fetchPollInterval and re-sends FetchCommand (safe because fetch.Manager.Start joins an in-flight job rather than starting a second one). fetchTimeout/fetchPollInterval are package-level vars so tests can shrink them. 2. grpcserver.Server.Connect updated the catalog (marking a node connected) before registering its connection in the server's own conns map, which Send/OpenProxyStream actually check. A caller reading the catalog and immediately calling Send - exactly what deployToNode and the gateway's proxyOverGRPC both do - could hit a spurious "not connected" in that window. Reordered Connect to register conns first. Tests: deploy_grpc_test.go now exercises a realistic downloading-then-done poll sequence, a terminal "failed" phase, and a fetch that never completes within its timeout. server_test.go adds a concurrency-stress regression test for the Connect ordering (a sequential, low-contention version of the same test does not reliably reproduce the bug - it needs the scheduling pressure concurrent connects create). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…bserves done deployToNode's fetchWeights (Task 10) polls by resending FetchCommand, which dispatches to HandleFetch. HandleFetch previously always called fetch.Manager.Start, which is idempotent only against a fetch already IN FLIGHT - once a job finishes (success or failure), Start's own logic treats it as stale leftover and unconditionally removes + restarts it. A poll landing just after a real download finished would therefore silently wipe it out and restart from scratch, never reporting "done" to a caller that keeps asking - defeating the point of the poll loop for any download that actually completes. HandleFetch now checks fetch.Manager.Status (a pure read, no side effects) first: if the job already shows done/failed/downloading, that phase is reported directly and Start is never called. Start is only reached when Status reports the job genuinely absent (never attempted, or its container has been removed). internal/fetch/fetch.go itself is untouched, per the plan's own scope for that package - this fixes the one layer this plan owns (grpcclient's own Task 5 handler) rather than the shared, pre-existing fetch.Manager. Extended fakeFetchRuntime (grpcclient's Task 5 test double) to track StartJob/RemoveJob invocations, so the new tests can assert Start's destructive path is never reached once a job has already finished. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Relocates internal/larder/delete.go's guarded Delete (and the disk-scan
half of larder.go's Scan it needs) into grpcclient/weights.go, replacing
the Task 5 placeholder deleteWeights stub. The SAFETY guard (never delete
a currently-deployed repo, force included) and the symlink/path-escape
check carry over unchanged; the POLICY guard (StateProtected, requiring
force for an archived recipe's rollback weights) does not, since souslet
architecturally keeps no recipe catalog to classify it against - see
weights.go's package doc comment for the full reasoning.
Handlers gains a currentlyDeployed map (recipe ID -> model repo),
populated by HandleDeploy/cleared by HandleUndeploy alongside the
existing footprints/ports caches, so the guard reads Handlers' own live
state instead of a passed-in deployed list. Snapshot now also populates
NodeSnapshot.CachedWeightRepos (previously always empty), which the new
UI action needs to know what is actually resident on a node.
Wires POST /api/weights/{recipeID}/{nodeID}/delete (httpapi/weights.go)
and a per-node "clear weights" action in models.html, matching the
existing confirm-button pattern used elsewhere for destructive actions.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Task 11's initial cut dropped the guard that refuses to delete a repo still referenced by an archived recipe (rollback insurance), because souslet has no recipe catalog to make that judgment. Restores it as a POLICY guard in internal/httpapi/weights.go instead, where sous-api's real recipe catalog lives: deleteWeightsOnNode now checks whether any OTHER recipe, archived, still names the same model, and refuses with 409 before ever contacting the node unless force=true is passed. Deliberately reads internal/catalog directly rather than reusing internal/larder's classification, since that package is deleted once Task 14 of the multi-node plan lands. souslet's own SAFETY guard (never delete something currently deployed, regardless of force) is unchanged and remains the final backstop either way. internal/httpapi/handlers_test.go's buildServerFull now also returns the *grpcserver.Server it builds (new newTestServerWithGRPC helper), needed to drive a genuine end-to-end round trip through the real route with a faked souslet for the new tests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two real gaps from the previous fix round's review: 1. deleteWeightsOnNode never checked wantsHTML(r), unlike every other confirm-button-backed route in this codebase. confirm-button (confirm.html) renders a real <form method="post"> - form-urlencoded, full-page navigation - not fetch(), so clicking "Clear weights" for real navigated the whole page to a raw JSON blob instead of back to /models with a status banner. Every existing test posted with an empty/JSON Content-Type, which only exercises the path the button doesn't use, so this went uncaught. Now branches on wantsHTML at every exit (via a new weightsRefused helper) and gates on requireConfirm like every other confirm-button route. Also closed the "no way to override from the UI" gap this exposed: pageModels now computes per-recipe protection status (pageData.WeightsProtection), and models.html renders one of three things per resident (recipe, node) pair - a plain "Clear weights" button, a force-only "Force clear weights" button (?force=true baked into its Action, Cost text naming the protecting archived recipe), or no button at all when an active reference makes the guard unconditional - matching larder.html's own "a button that exists to say no" precedent. 2. The archived-recipe guard alone was narrower than the original StateReferenced, which also covers "referenced by any ACTIVE recipe" - unconditionally, force never overrides. That case had zero protection. classifyProtection now splits every other recipe naming a repo into activeBy (unconditional refusal) and archivedBy (force overrides), matching the original's severity split exactly - the active tier is checked first and force is never even consulted for it. Discovered while fixing #2: the seed catalog's qwen38/qwen38-dflash2 recipes already share one model, which the restored active-reference guard now correctly protects - several test fixtures that assumed qwen38 had no other referencing recipe had to move to qwen36 instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
pageNode now builds a NodeCardView per entry in nodecatalog.Catalog.All() (nodeCards()) and node.html renders them as a "Fleet" grid alongside the existing single-box dashboard, each card reusing poolbar.html's "pool-bar" partial with its own PoolGiB/ReserveGiB/MarginGiB and a connected/ disconnected chip. A disconnected node keeps rendering from its last-known snapshot (greyed out via card.is-idle) rather than vanishing, matching nodecatalog.Catalog.MarkDisconnected's own contract. pageData already had a Nodes []nodecatalog.NodeView field from Task 11's per-node weights UI on models.html, so the new per-card view lives on a separate NodeCards field rather than colliding with it. Margin uses the same PoolGiB-ReserveGiB-committed formula planOnNode uses for this node's own deploy/plan requests. Per-deployment bar segments are tagged deploy.PhaseReady uniformly rather than cast from pb.DeploymentState.Phase, which is Docker's raw status word (running/ exited/restarting), not deploy.Phase's vocabulary - casting it would draw a crash-looping container as a placid green "ready" segment. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code review on Task 12 found a real correctness defect: nodeCards() unconditionally tagged every fleet-card committed segment deploy.PhaseReady, which is documented as "the only phase that means usable" - not a neutral placeholder. Since d.Phase on a NodeSnapshot deployment is Docker's raw status word (running/exited/restarting/...), not deploy.Phase's own vocabulary, this was a guaranteed false-positive health signal on every fleet segment: a crash-looping or OOM-killed container rendered identically - green, "ready" - to a genuinely healthy one. Fix: Segment grows an Unknown bool + RawStatus string. nodeCards() now sets Unknown:true and RawStatus:d.Phase instead of a phase, and poolbar.html routes Unknown segments to their own neutral seg-unknown CSS class (a muted grey, distinct from every phase color) rather than through the phase-colored branch, with the real Docker status word kept visible in the tooltip so the coarseness is disclosed rather than disguised. This only affects fleet cards - the single-node dashboard's segments still carry real deploy.Phase data and are unaffected. Added TestFleetCardSegmentDoesNotClaimReadyForAnUnhealthyContainer, verified against the pre-fix code (via a temporary git stash of just the fix, keeping the new test) to confirm it actually fails against the bug before confirming it passes against the fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds draggable="true"/data-recipe-id to Models' recipe cards and
data-node-id/drop-target to Node's fleet cards (Task 12), alongside the
per-node weight chips Task 11 already added rather than replacing them.
dragdrop.js is the project's first client-side JS: it posts dropped
recipes to POST /api/deploy/{id}/{nodeID} (Task 8) and reloads on
success. Serving it required embed.go's first static-asset route.
recipe.Recipe.Archived's doc comment states the UI hiding the deploy control IS this codebase's enforcement of "archived means cannot run here" - there is no server-side guard backing it. draggable="true" had no equivalent gate to the existing "Deploy..." link's (not .Recipe.Archived) condition, making an archived card a new, undocumented click-path to an action the UI never exposed before. - models.html: draggable is now conditional on Archived, mirroring the Deploy link's own gate. - handlers.go: deployNode now refuses an archived recipe with 409, even with force=true, as a server-side backstop per the review's own warning against relying solely on UI-side enforcement. - dragdrop_test.go: two new tests proving both the UI gate and the server-side refusal. Found via independent review of the prior commit (9a59429).
…ode-cert CLI
Two-image CI build (sous-api, souslet) replaces the single-image workflow.
Retires internal/larder (fully superseded by Task 10/11's recipe-card
weight cleanup) and the old single-node cmd/sous binary, since neither has
any caller left that Tasks 1-13 didn't already replace with a node-scoped
equivalent. Adds `sous-api node add` as the minimal CLI admin surface for
issuing a souslet's mTLS client cert from the running server's own
persisted CA.
internal/deploy (deploy.Manager) is deliberately NOT removed in this
commit - it is still the sole implementation behind several pages and
endpoints (the Node dashboard's single-box section, /models, /model/{id}
including its log viewer, /model/{id}/plan, the /events stream, /api/
status, /api/logs/{id}, and Gateway's /v1/models listing) with no
node-scoped equivalent built anywhere in Tasks 1-13, most notably remote
log access, which has no wire-protocol support at all. Full details in
.superpowers/sdd/2026-09-01-sous-multinode-implementation/task-14-report.md.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Review fix for 5bce138: the badge and Quickstart still referenced the retired single ghcr.io/codemug/sous image and cmd/sous's flags. Rewrites the Quickstart as a three-step sous-api/node-add/souslet walkthrough with real docker run invocations, flag names verified against cmd/sous-api and cmd/souslet's own main.go. Also gofmt's screens_test.go's trailing blank line, flagged as trivial in the same review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… atomically
TLSConfigServer built the listener's own identity by calling
IssueNodeCert("sous-api"), which mints a NODE certificate: no SANs at all
and ExtKeyUsage {ClientAuth}. souslet's ClientTLSConfig verifies the server
in full (no InsecureSkipVerify, no ServerName override), so following the
README's own Quickstart verbatim, no souslet could ever complete the
handshake - three stdlib rejections stacked: missing SANs, the legacy
CommonName fallback modern Go refuses, then the ExtKeyUsage mismatch. Every
gRPC test in the repo dials bufconn with insecure credentials and ca_test.go
only ran x509.Verify in isolation, so nothing exercised a real handshake.
IssueServerCert now signs the control plane's own identity separately:
ServerAuth key usage plus real DNS/IP SANs for the hosts souslets dial,
which TLSConfigServer takes as a parameter and cmd/sous-api feeds from the
host half of -grpc-listen. Loopback is always included so a same-box souslet
works without extra configuration. The server's own CN is deliberately not
registered in the CA's known-NODE set.
Save also writes through a temp file and os.Rename instead of os.WriteFile.
A crash or full disk mid-write left a truncated ca-state.json, which makes
sous-api log.Fatalf on start AND destroys the key material behind every node
certificate ever issued - the most critical persisted state in the system
had the least durable write.
Tests: a real tls.Listen/tls.Dial round trip proving mutual auth end to end
(fails on all three counts against the old code), its negative counterpart
for an address outside the SAN list, a property test for the certificate
shape, and an atomic-save test that rewrites through a read-only destination.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…etting uploads block commands Three connection-level defects, all in Connect's neighbourhood. Node identity was self-asserted. Connect took the node ID from snap.NodeId - the client's own claim in its first message - and never looked at the verified peer certificate, and CA.IsKnown/CA.Revoke had no production call sites at all. Consequences: revocation was a total no-op (a decommissioned node kept full control-plane access forever), and any holder of any CA-signed certificate could claim any other node's ID, evicting the real node and receiving its deploys and proxied inference. Connect now matches the claimed ID against the peer certificate's CommonName and against the CA's registration set, via a narrow NodeAuthority interface threaded in through New(cat, ca). A nil authority (this package's bufconn tests, which cannot present a certificate) skips the check; a configured authority refuses any connection it cannot identify, including a plaintext one, so the check can't be dodged by connecting without TLS. This also makes the CLI's and README's "restart sous-api before a newly added node can connect" note true, where before nothing consulted the known set at all. A stale connection's teardown could unregister a live one. The cleanup defer deleted s.conns[nodeID] and marked the node disconnected unconditionally, so when a partitioned or rebooted node's old stream finally errored out - after the node had already reconnected and registered a new nodeConn under the same ID - it tore down the working connection. The node then showed disconnected and unreachable until a restart. Cleanup now only acts if the map still points at its own nodeConn. Large proxied uploads starved control commands. One 32-deep channel carried both proxy body frames and deploy/undeploy/fetch/weight-delete commands, and Send's enqueue fails fast rather than waiting - so during a 32MB upload (8000+ 4096-byte frames, this gateway's own documented large case) any command failed with a confusing "send queue is full". Commands now have their own channel, drained with priority by the same write loop, so the interference is gone rather than merely less likely. Tests: real-mTLS gRPC tests over a loopback listener for the accepted, impersonating and revoked cases plus the plaintext bypass; a reconnect-race test that registers two generations and tears the first down; and a test that saturates the proxy buffer and proves a deploy still gets through. The reconnect and starvation tests were both confirmed to fail against the pre-fix code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ce at connect souslet sent exactly one NodeSnapshot, at connect time, and never again - no ticker, no post-command push - so sous-api's view of a node was accurate only at the instant that node connected and went stale the moment anything changed. The load-bearing consequence: planOnNode sizes every node-scoped deploy against view.Deployments, so the second, third and fourth drag-and-drop deploy onto an already-full node all passed the capacity gate against a snapshot taken when it was empty - exactly the over-commitment the capacity planner exists to prevent. Fleet cards, MarginGiB and the "weights cached" chips went stale the same way, indefinitely. connectOnce now runs a snapshot ticker for the life of the connection (SnapshotInterval, 15s by default), scoped to that connection generation so a reconnect never leaves an older generation writing to a dead stream. A successful deploy/undeploy/weight-delete also pushes a fresh snapshot immediately - after its reply, never before, since sous-api's caller is blocked on that reply - so the common case converges in milliseconds rather than waiting for the next tick. The design was already level-triggered by intent; this makes the level actually get re-read. Test: the real client against the real grpcserver, changing what Docker reports mid-connection and asserting nodecatalog.Catalog.Node follows it (both a model appearing and one going away), with a log assertion that no reconnect happened - so it cannot pass via a reconnect's handshake snapshot. Confirmed to fail against the pre-fix code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ports.Allocator lived only inside the legacy deploy.Manager, so the node-scoped path passed the query-param port straight through un-allocated - and the drag-and-drop UI sends none at all, making WantPort always 0. engine.BuildSpec put that 0 into HostPort, Docker read it as "pick an ephemeral port", and nothing recorded what it picked: DeployResult.HostPort stayed 0, NodeSnapshot.DeploymentState.HostPort stayed 0, and portFor returned 0, so souslet's own proxy built http://127.0.0.1:0/... for every request. The headline flow produced a model that was running and unreachable. Allocation happens on the NODE (Handlers.resolvePort), not on sous-api as the finding first suggested. ports.Allocator decides availability by actually binding - that is the whole point of the package, after k3s Traefik silently held 443 across this fleet - and binding on sous-api answers a question about sous-api's sockets, not the node's. It would hand a node a port some other process there already holds. The rule otherwise mirrors deploy.Manager.Deploy exactly: 0 means pick a free one from the configured range, an explicitly requested port must genuinely be free (adoption stays supported, silently starting a container that cannot bind does not). Snapshot now reports the same real port from the cache HandleDeploy fills in, so the catalog and the UI see a live address rather than 0. cmd/souslet grows -port-low/-port-high/-bind-host, defaulting to the same 18000-18100 on 127.0.0.1 that sous-api already uses; a Handlers built without them still allocates from that default range rather than falling back to 0. Tests: a WantPort-0 deploy (the drag-and-drop case) must end up with a non-zero port on the container spec, the DeployResult, portFor and the next snapshot, and that port must be genuinely bindable; allocation must skip a port a foreign process holds; an explicitly requested taken port must be refused without starting anything. The first and third fail against the pre-fix code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…orce key scope on it The gateway's multi-node proxy was dead code in the shipped binary. httpapi.New built its Gateway without setting Nodes or GRPC - the only places that ever set them were hand-written literals in gateway_test.go - so Proxy's node branch was unreachable in production and every inference request for a model running on a connected node fell through to the local deploy.Manager and 404'd. Task 9 owned the proxy, Task 8 owned httpapi and ran before it; no diff and no test ever built a full server AND asserted a proxied request reached a node. New() now passes the same nodes/gsrv it already hands the node-scoped deploy routes. Proxy asks the node catalog first and falls back to the local path when no connected node runs the requested model, rather than dispatching to the node path unconditionally as the finding suggested: sous-api still carries a live deploy.Manager during the migration, and an unconditional dispatch would have made every locally-deployed model unreachable through the gateway - trading one 404 for another. With no local Resolver at all (the design's end state, and every node-path test) the node path still owns the request outright, error answers included. Scoped API keys are now enforced on that path too. proxyOverGRPC never called auth.FromContext, and a comment justified it as matching the local path for an unscoped caller - but the local path's gate exists for SCOPED callers, so a key restricted to particular models could reach any model on any node. internal/apikey scoping is a shipped feature; this was a real bypass. The same allowedBy helper now gates the node path, after node resolution and before anything is forwarded, so the caller gets the local path's own 403/model_not_permitted rather than a 404. Tests: an httpapi-level test that builds a real Server through New(), attaches a fake souslet reporting a deployed model, and drives POST /v1/chat/completions to a 200 from the node; its counterpart proving a model no node runs still gets the local path's answer; and gateway-level scope tests for the refused, permitted and unscoped cases plus the 404-not-403 routing order. The node-reachability and scope-refusal tests were both confirmed to fail against the pre-fix code. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Both are new operator-visible behavior from this fix round: souslet picks each model's host port from -port-low/-port-high on -bind-host (deciding availability by binding, which only means anything on the node itself), and it re-reports its full state every few seconds rather than only at connect. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… the port fix superseded
ports.Allocator.Free binds to check availability, and net.Listen("tcp",
host+":0") always succeeds - so -port-low 0 would hand out port 0 on the
first try, silently reproducing the "deployed but unaddressable" bug the
real port allocation fix just closed. Also removes Snapshot's stale doc
line claiming HostPort is always left at zero, contradicted by the
portFor-based comment and code six lines below it.
Found by the final-review re-review as two Minor residuals in the
consolidated fix round; fixed directly rather than spinning up another
implementer round for a one-line guard and a doc deletion.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Rewrites Sous from a single-node deployer into a multi-node control plane, per the approved design (
docs/superpowers/specs/2026-09-01-sous-multinode-design.md) and implementation plan (docs/superpowers/plans/2026-09-01-sous-multinode-implementation.md, 14 tasks):sous-api(wascmd/sous): control plane — recipe catalog, node catalog, UI, gRPC server, self-issued mTLS CA.souslet(new): per-node worker — Docker engine control, weight fetch, gRPC client. Dials out tosous-api(NAT/firewall-friendly); all API→souslet commands and proxied inference traffic are multiplexed over that one souslet-initiated stream.sous-api node add <id>issues a node's cert.NodeSnapshotat connect and periodically thereafter (every 15s); the API does a full replace, never a merge.internal/larderremoved. Weight presence is now a per-(recipe, node) fact in each snapshot. Deploying triggers a fetch-first orchestration (poll-based) when weights are missing on the target node. Cleanup is a per-(recipe, node) recipe-card action with a two-tier safety guard (active-recipe reference refuses unconditionally; archived-only reference refuses unlessforce=true).Built via Subagent-Driven Development: 14 tasks, each with an independent implementer + task review + fix-round loop, followed by an independent whole-branch review (opus) that caught 5 integration-seam Critical findings no single task's diff could have seen (see "Findings from the final review" below), all fixed and re-verified in a follow-up consolidated round.
Deliberately out of scope for this PR — please read before merging
1. Live cutover not executed. No SSH, no real deployment happened against uae-homenode, asus-gx10, or aorus-ubuntu.
sous-api node add(the CLI to issue a node cert) is implemented and tested; actually registering a real node, installingsousleton it, stopping the currently-serving single-node container, and redeploying is a separate, later action needing its own explicit go-ahead — this is a live-infrastructure change outside what a code review can gate.2.
internal/deploy(the old single-node manager) is deliberately kept, not deleted. It's still the sole implementation behind the log viewer (deploy.Runtime.Logs— no wire-protocol equivalent exists in the souslet proto at all), the/eventsSSE stream,GET /api/status//api/deployments//api/logs/{id}, and the/models//model/{id}single-node pages. Most importantly:GET /v1/modelsis unconditional and reportsdeploy.Manager's local view, never the node catalog's fleet-wide view — a real control-plane deployment (no local models) will report an empty model list to external OpenAI-API-compatible clients even though models are running on nodes. Closing this gap needs either real proto/design work (aLogsRPC, a fleet-wideListModelsaggregation) or an explicit decision to drop those features — neither was this branch's call to make unilaterally.3. No operator-facing node revocation yet.
CA.Revoke's mechanism works and is tested (a revoked node's cert is refused atConnect), but nothing calls it in production — there's asous-api node add, nosous-api node revoke. Today, revoking a node means hand-editingca-state.jsonand restartingsous-api.4. First-time deploy can block the request up to 30 minutes. A drag-and-drop deploy onto a node missing the weights runs the fetch synchronously inline; there's no 202+polling UX yet, so the browser tab just hangs during a cold download.
Findings from the final whole-branch review (all fixed, independently re-verified)
An independent final review (separate from the 14 task-level reviews) found 5 Critical, integration-seam-only defects — each invisible to every individual task's own tests because the existing gRPC test suite uses insecure/bufconn transport everywhere, which is structurally blind to real TLS wiring:
sous-api.Nodes/GRPCleft nil) — every inference request 404'd unless served locally.Plus 4 Important findings (control-command starvation during large uploads, unenforced API-key scoping on the node-proxy path, a stale-connection teardown race, non-atomic CA-state writes). All 9 were fixed in a consolidated round and independently re-verified — the reviewer mechanically reverted each fix in a scratch copy and confirmed its test genuinely fails pre-fix, not just trusted the report.
Test plan
go build ./...,go vet ./...cleango test ./...— 534 passed, 0 failed, 28 packagesDockerfile.sous-api/Dockerfile.sousletbuilt locally and smoke-tested (fail with real config-validation errors on no args, not silent/crashing)tls.Listen/tls.Dial, not bufconn) covering the fixed mTLS server cert🤖 Generated with Claude Code