feat: multi-host RAG service provisioning#335
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe changes propagate Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | -2 |
TIP This summary will be updated as you push new changes. Give us feedback
|
@coderabbitai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
server/internal/workflows/plan_update.go (1)
169-183: Makepostgrestexplicit inresolveTargetSessionAttrs.
postgrestis a supported service type but currently falls throughdefault. Adding an explicit case keeps intent stable if the fallback behavior changes later.♻️ Suggested refactor
switch serviceSpec.ServiceType { case "mcp": // MCP maps allow_writes → primary/prefer-standby if allowWrites, ok := serviceSpec.Config["allow_writes"].(bool); ok && allowWrites { return database.TargetSessionAttrsPrimary } return database.TargetSessionAttrsPreferStandby + case "postgrest": + // PostgREST defaults to read-only routing unless explicitly overridden. + return database.TargetSessionAttrsPreferStandby case "rag": // RAG is read-only; always prefer a standby when available. return database.TargetSessionAttrsPreferStandby default:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/workflows/plan_update.go` around lines 169 - 183, The switch on serviceSpec.ServiceType inside resolveTargetSessionAttrs currently treats "postgrest" via the default branch; add an explicit case for "postgrest" so its intent is preserved if the default behavior changes. Modify resolveTargetSessionAttrs to include case "postgrest": returning database.TargetSessionAttrsPreferStandby (same behavior as current default), keeping "mcp" and "rag" cases unchanged and ensuring serviceSpec.ServiceType is compared against the literal "postgrest".server/internal/orchestrator/swarm/rag_service_user_role_test.go (1)
370-385: Avoid index-coupled lookup in this test.Using
result.Resources[5]makes the test brittle to unrelated resource-order changes. LocateServiceInstanceSpecResourceby identifier type, and prefer the constant over a string literal.♻️ Suggested refactor
- TargetSessionAttrs: "prefer-standby", + TargetSessionAttrs: database.TargetSessionAttrsPreferStandby, } @@ - // ServiceInstanceSpecResource is at index 5 (single node: Network + RO + dir + keys + config + instanceSpec + instance). - sis, err := resource.ToResource[*ServiceInstanceSpecResource](result.Resources[5]) - if err != nil { - t.Fatalf("ToResource ServiceInstanceSpecResource: %v", err) - } - if sis.TargetSessionAttrs != "prefer-standby" { - t.Errorf("TargetSessionAttrs = %q, want %q", sis.TargetSessionAttrs, "prefer-standby") - } + var sis *ServiceInstanceSpecResource + for _, rd := range result.Resources { + if rd.Identifier.Type != ResourceTypeServiceInstanceSpec { + continue + } + sis, err = resource.ToResource[*ServiceInstanceSpecResource](rd) + if err != nil { + t.Fatalf("ToResource ServiceInstanceSpecResource: %v", err) + } + break + } + if sis == nil { + t.Fatal("ServiceInstanceSpecResource not found") + } + if sis.TargetSessionAttrs != database.TargetSessionAttrsPreferStandby { + t.Errorf("TargetSessionAttrs = %q, want %q", sis.TargetSessionAttrs, database.TargetSessionAttrsPreferStandby) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/rag_service_user_role_test.go` around lines 370 - 385, The test is brittle because it picks ServiceInstanceSpecResource by fixed index; change it to find the resource by type/identifier instead: iterate over result.Resources, use resource.ToResource[*ServiceInstanceSpecResource] (or check the resource's Type()/Kind() constant) to locate the ServiceInstanceSpecResource instance, prefer the package constant for the resource identifier rather than a string literal, then assert on sis.TargetSessionAttrs as before; update the test around generateRAGInstanceResources/result.Resources to perform this type-based lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/internal/orchestrator/swarm/rag_service_user_role_test.go`:
- Around line 370-385: The test is brittle because it picks
ServiceInstanceSpecResource by fixed index; change it to find the resource by
type/identifier instead: iterate over result.Resources, use
resource.ToResource[*ServiceInstanceSpecResource] (or check the resource's
Type()/Kind() constant) to locate the ServiceInstanceSpecResource instance,
prefer the package constant for the resource identifier rather than a string
literal, then assert on sis.TargetSessionAttrs as before; update the test around
generateRAGInstanceResources/result.Resources to perform this type-based lookup.
In `@server/internal/workflows/plan_update.go`:
- Around line 169-183: The switch on serviceSpec.ServiceType inside
resolveTargetSessionAttrs currently treats "postgrest" via the default branch;
add an explicit case for "postgrest" so its intent is preserved if the default
behavior changes. Modify resolveTargetSessionAttrs to include case "postgrest":
returning database.TargetSessionAttrsPreferStandby (same behavior as current
default), keeping "mcp" and "rag" cases unchanged and ensuring
serviceSpec.ServiceType is compared against the literal "postgrest".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f6781a41-3554-4b5e-bf79-53c0c3aaa128
📒 Files selected for processing (4)
server/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/rag_service_user_role_test.goserver/internal/workflows/plan_update.goserver/internal/workflows/plan_update_test.go
| // Future service types add cases here. | ||
| case "rag": | ||
| // RAG is read-only; always prefer a standby when available. | ||
| return database.TargetSessionAttrsPreferStandby |
There was a problem hiding this comment.
Question since I'm unfamiliar with how RAG is used: is the RAG server typically used as a standalone application or is it a component in a larger application? The reason I'm asking is that if it's used alongside something that writes to the database, then this would introduce a delay between when the data is written and when it's available for the RAG server. That can cause odd behavior if you're expecting "read-your-writes" consistency. But, none of that is an issue if the RAG server is just a standalone service.
There was a problem hiding this comment.
The RAG server is read-only — it queries embeddings and document chunks to answer prompts, but never writes to Postgres. Writes (embedding) happen separately through manual process and we have separate ticket for that documentation how to do it. So there's no read-your-writes concern and prefer-standby is the right choice.
server/internal/orchestrator/swarm/rag_service_user_role_test.go
Outdated
Show resolved
Hide resolved
Resolve conflict in orchestrator.go by retaining the `TargetSessionAttrs` field added by this branch. Also cleans up the test file rename (rag_service_user_role_test.go → rag_instance_resources_test.go) introduced earlier in this branch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2 participants
Summary
This PR provisions independent RAG service instances across multiple hosts defined in host_ids. Each host receives its own container, config file, API key files, and Postgres service user connected to its co-located Patroni primary.
Changes
Forward TargetSessionAttrs into the RAG ServiceInstanceSpecResource so all instances correctly use prefer-standby for read-only connections
Add explicit case "rag" to resolveTargetSessionAttrs to document intent and guard against future default changes
Add TestGenerateRAGInstanceResources_TargetSessionAttrs to verify TargetSessionAttrs is propagated correctly into the spec resource
Testing
Verification:
rag_create_multi_host_db.json
Checklist
PLAT-493