Fix stale service port plans#1416
Conversation
|
| Filename | Overview |
|---|---|
| packages/server/src/server/workspace-service-port-registry.ts | Adds while(true) loop with planMatchesServiceDeclarations check to extend cached plans instead of short-circuiting on any existing plan; new resolvePlannedServicePort preserves existing ports while allocating for newly declared services. Logic is correct, but resolveServicePort is now a strict duplicate of resolvePlannedServicePort with existingPlan: undefined. |
| packages/server/src/server/workspace-service-port-registry.test.ts | Updates the stale-plan tests to cover extend (new services), explicit-port override, and service removal cases; adjusts two existing tests that broke because empty-services calls now trigger a rebuild. |
| packages/server/src/server/worktree-bootstrap.test.ts | Adds a bootstrap regression test that verifies ports are preserved and extended when a second service is added to paseo.json after the first service plan has already been built. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[ensureWorkspaceServicePortPlan called] --> B{existingPlan in cache?}
B -- No --> D
B -- Yes --> C{planMatchesServiceDeclarations?}
C -- Yes --> RET1[Return copy of existing plan]
C -- No: new/removed/explicit-port changed --> D
D{pendingPlan in-flight?}
D -- No --> E[createPendingWorkspaceServicePortPlan]
E --> F[buildWorkspaceServicePortPlan]
F --> G[resolvePlannedServicePort per service]
G -- explicit port set --> H[Use declared port]
G -- in existingPlan --> I[Reuse existing port]
G -- new service --> J[allocatePort]
H & I & J --> K[Store updated plan]
K --> RET2[Return new plan]
D -- Yes: another caller building --> L[await pendingPlan]
L --> B
Reviews (2): Last reviewed commit: "fix(server): extend workspace service po..." | Re-trigger Greptile
| while (true) { | ||
| const existingPlan = workspaceServicePortPlans.get(options.workspaceId); | ||
| if (existingPlan && planIncludesServices(existingPlan, options.services)) { | ||
| return new Map(existingPlan); | ||
| } | ||
|
|
||
| let pendingPlan = pendingWorkspaceServicePortPlans.get(options.workspaceId); | ||
| if (!pendingPlan) { | ||
| pendingPlan = createPendingWorkspaceServicePortPlan({ | ||
| workspaceId: options.workspaceId, | ||
| services: options.services, | ||
| allocatePort: options.allocatePort, | ||
| }); | ||
| pendingWorkspaceServicePortPlans.set(options.workspaceId, pendingPlan); | ||
| } | ||
| let pendingPlan = pendingWorkspaceServicePortPlans.get(options.workspaceId); | ||
| if (!pendingPlan) { | ||
| pendingPlan = createPendingWorkspaceServicePortPlan({ | ||
| workspaceId: options.workspaceId, | ||
| existingPlan, | ||
| services: options.services, | ||
| allocatePort: options.allocatePort, | ||
| }); | ||
| pendingWorkspaceServicePortPlans.set(options.workspaceId, pendingPlan); | ||
| return new Map(await pendingPlan); | ||
| } | ||
|
|
||
| return new Map(await pendingPlan); | ||
| await pendingPlan; | ||
| } |
There was a problem hiding this comment.
Concurrent extension with disjoint new services is untested
The while(true) loop handles a subtle case: two callers concurrently extending the plan with different new services (e.g. caller A adds new-service-1, caller B adds new-service-2). Caller B awaits A's pending plan, then loops back—finds A's result doesn't cover new-service-2, creates a second pending plan using A's result as existingPlan, and allocates only new-service-2.
The existing concurrent test ("shares one first-plan build across concurrent callers") only exercises the initial-plan case where both callers request the same services. The new code's most non-obvious branch (the loop-back after await pendingPlan) is not covered. If a regression breaks this path, the while(true) loop could silently allocate a duplicate port or fail to include an expected service in the plan.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
e8bdee6 to
f9fe118
Compare
Summary
paseo.jsongains additional service scripts.webafter a backend service plan already exists.Closes #1374
Test plan