chore: maybe init workspace from server #686
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughListenAndRoute now returns an error and propagates handler failures; workspace initialization was moved to Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as HTTP / Event Listener
participant Utils as GetWorkspace
participant DB as DB
participant Cache as Workspace Cache
participant Pop as PopulateWorkspaceWithInitialState
rect #F0FFF0
Note over Caller,Utils: New flow — GetWorkspace returns (ws, err)
Caller->>Utils: GetWorkspace(ctx, id)
Utils->>DB: WorkspaceExists(id)?
alt exists in DB
DB-->>Utils: true
Utils->>Cache: workspace.GetWorkspace(id)
Cache-->>Utils: ws
Utils-->>Caller: (ws, nil)
else not in DB
DB-->>Utils: false
Utils->>Cache: workspace.New(id)
Cache-->>Utils: ws
Utils->>Pop: PopulateWorkspaceWithInitialState(ctx, ws)
alt populate ok
Pop-->>Utils: nil
Utils->>Cache: workspace.Set(id, ws)
Utils-->>Caller: (ws, nil)
else populate failed
Pop-->>Utils: err
Utils-->>Caller: (nil, err)
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/workspace-engine/pkg/selector/resources.go (1)
35-87: Remove the unconditional stdout dumps.Line 35 onwards now pretty-prints every selector/condition/match result to stdout using
fmt.Printf.FilterResourcessits on the request path, so this will emit verbose JSON for every call, bypass structured logging, overwhelm logs under load, and leak tenant-scoped payloads (workspace IDs, selector predicates) into shared stdout. Please drop these prints or gate them behind the existing logger at a debug level with an explicit feature flag before merging.Apply this diff to remove the stdout logging:
- if sel != nil { - // Try to marshal the resource selector to JSON for readability - if jsonBytes, err := json.MarshalIndent(sel, "", " "); err == nil { - fmt.Printf("selector:\n%s\n", string(jsonBytes)) - } else { - fmt.Printf("ResourceSelector for selector (marshal error: %v): %#v\n", err, sel) - } - } else { - fmt.Printf("ResourceSelector for selector: <nil>\n") - } + if sel == nil { + return map[string]*oapi.Resource{}, nil + } - if jsonBytes, err := json.MarshalIndent(jsonSelector, "", " "); err == nil { - fmt.Printf("jsonSelector:\n%s\n", string(jsonBytes)) - } else { - fmt.Printf("jsonSelector (marshal error: %v): %#v\n", err, jsonSelector) - } - if jsonSelector.Json == nil { return map[string]*oapi.Resource{}, nil } - if jsonBytes, err := json.MarshalIndent(unknownCondition, "", " "); err == nil { - fmt.Printf("unknownCondition:\n%s\n", string(jsonBytes)) - } else { - fmt.Printf("unknownCondition (marshal error: %v): %#v\n", err, unknownCondition) - } - selector, err := jsonselector.ConvertToSelector(ctx, unknownCondition) if err != nil { return nil, err } - - if jsonBytes, err := json.MarshalIndent(selector, "", " "); err == nil { - fmt.Printf("selector:\n%s\n", string(jsonBytes)) - } else { - fmt.Printf("selector (marshal error: %v): %#v\n", err, selector) - } - - fmt.Printf("matchedResources: %d\n", len(matchedResources))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
apps/workspace-engine/pkg/events/handler/handler.go(2 hunks)apps/workspace-engine/pkg/selector/resources.go(5 hunks)apps/workspace-engine/pkg/server/openapi/deployments/server.go(1 hunks)apps/workspace-engine/pkg/server/openapi/environments/server.go(1 hunks)apps/workspace-engine/pkg/server/openapi/policies/policies.go(1 hunks)apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go(2 hunks)apps/workspace-engine/pkg/server/openapi/utils/utils.go(1 hunks)apps/workspace-engine/pkg/workspace/populate_workspace.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
apps/workspace-engine/**/*.go
📄 CodeRabbit inference engine (apps/workspace-engine/CLAUDE.md)
apps/workspace-engine/**/*.go: Do not add extraneous inline comments that state the obvious
Do not add comments that simply restate what the code does
Do not add comments for standard Go patterns (e.g., noting WaitGroup or semaphore usage)
Write comments that explain why, document complex logic/algorithms, provide non-obvious context, include TODO/FIXME, and document exported functions/types/methods
Files:
apps/workspace-engine/pkg/server/openapi/environments/server.goapps/workspace-engine/pkg/server/openapi/deployments/server.goapps/workspace-engine/pkg/selector/resources.goapps/workspace-engine/pkg/server/openapi/utils/utils.goapps/workspace-engine/pkg/workspace/populate_workspace.goapps/workspace-engine/pkg/events/handler/handler.goapps/workspace-engine/pkg/server/openapi/policies/policies.goapps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go
🧬 Code graph analysis (6)
apps/workspace-engine/pkg/server/openapi/environments/server.go (2)
apps/workspace-engine/pkg/server/openapi/utils/utils.go (1)
GetWorkspace(12-33)apps/workspace-engine/pkg/workspace/workspace.go (1)
GetWorkspace(161-168)
apps/workspace-engine/pkg/server/openapi/deployments/server.go (2)
apps/workspace-engine/pkg/server/openapi/utils/utils.go (1)
GetWorkspace(12-33)apps/workspace-engine/pkg/workspace/workspace.go (1)
GetWorkspace(161-168)
apps/workspace-engine/pkg/selector/resources.go (2)
apps/workspace-engine/pkg/selector/langs/jsonselector/compare/convert.go (1)
ConvertToSelector(12-34)apps/workspace-engine/pkg/selector/langs/jsonselector/jsonselector.go (1)
ConvertToSelector(14-19)
apps/workspace-engine/pkg/server/openapi/utils/utils.go (3)
apps/workspace-engine/pkg/workspace/workspace.go (5)
GetWorkspace(161-168)Workspace(41-47)Exists(148-151)New(15-26)Set(153-155)apps/workspace-engine/pkg/db/workspaces.go (1)
GetWorkspaceIDs(9-35)apps/workspace-engine/pkg/workspace/populate_workspace.go (1)
PopulateWorkspaceWithInitialState(8-60)
apps/workspace-engine/pkg/events/handler/handler.go (1)
apps/workspace-engine/pkg/workspace/populate_workspace.go (1)
PopulateWorkspaceWithInitialState(8-60)
apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go (1)
apps/workspace-engine/pkg/server/openapi/utils/utils.go (1)
GetWorkspace(12-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Typecheck
- GitHub Check: Lint
- GitHub Check: build (linux/amd64)
| ws, err := utils.GetWorkspace(c, workspaceId) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{ | ||
| "error": "Failed to get workspace: " + err.Error(), | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Apply the workspace not-found error handling pattern here.
As noted in my comment on utils.go lines 18-20, the current error handling returns 500 Internal Server Error for all GetWorkspace failures, including when a workspace doesn't exist in the database. Once utils.GetWorkspace is updated to return a sentinel ErrWorkspaceNotFound error, this handler should detect it and return 404 Not Found instead.
See the suggested fix in apps/workspace-engine/pkg/server/openapi/utils/utils.go at lines 18-20. After implementing the sentinel error pattern there, update this handler to distinguish between 404 and 500 cases.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/server/openapi/policies/policies.go around lines 15
to 21, the handler currently treats all errors from utils.GetWorkspace as 500;
update it to check for the sentinel utils.ErrWorkspaceNotFound and return
c.JSON(http.StatusNotFound, ...) when that error is returned, otherwise preserve
the existing InternalServerError behavior; ensure you perform a type/equality
check against utils.ErrWorkspaceNotFound (or errors.Is) and keep the existing
error message content for both branches.
| ws, err := utils.GetWorkspace(c, workspaceId) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{ | ||
| "error": err.Error(), | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Apply the workspace not-found error handling pattern here.
Similar to policies.go, this handler returns 500 Internal Server Error for all GetWorkspace failures. Once the sentinel error pattern is implemented in utils.GetWorkspace (see comment on utils.go lines 18-20), update this handler to return 404 Not Found when the workspace doesn't exist.
Apply the same error-checking pattern suggested for policies.go at lines 15-21.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go
around lines 18 to 24, the handler currently returns 500 for all errors from
utils.GetWorkspace; update it to follow the same sentinel error pattern used in
policies.go (lines 15-21): check if errors.Is(err, utils.ErrWorkspaceNotFound)
and if so respond with c.JSON(http.StatusNotFound, gin.H{"error": err.Error()}),
otherwise respond with 500; ensure you import the errors package if not present
and keep the early-return flow identical to the policies.go example.
| ws, err := utils.GetWorkspace(c, workspaceId) | ||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, gin.H{ | ||
| "error": "Failed to get workspace: " + err.Error(), | ||
| }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Apply the workspace not-found error handling pattern here.
This is another location where the workspace not-found error should be handled with a 404 Not Found response instead of 500 Internal Server Error.
Apply the same error-checking pattern suggested for policies.go at lines 15-21 and releasetargets.go at lines 18-24.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/server/openapi/releasetargets/releasetargets.go
around lines 72 to 78, the handler returns a 500 on any workspace lookup error;
change it to follow the workspace not-found pattern used in policies.go (lines
15-21) and releasetargets.go (lines 18-24) by checking whether the error
indicates the workspace was not found and if so return
c.JSON(http.StatusNotFound, gin.H{"error":"Workspace not found"}), otherwise
return the 500 with the existing error message; keep the error text consistent
with the other handlers.
| if !slices.Contains(dbWorkspaceIds, workspaceId) { | ||
| return nil, fmt.Errorf("workspace %s not found in database", workspaceId) | ||
| } |
There was a problem hiding this comment.
Distinguish "not found" errors for correct HTTP status codes.
Returning a generic error when the workspace doesn't exist in the database causes downstream handlers (policies.go:17, releasetargets.go:20,74) to respond with 500 Internal Server Error instead of 404 Not Found. This breaks REST semantics and may confuse API clients.
Solution: Use a sentinel error type that handlers can detect:
In utils.go:
+import "errors"
+
+var ErrWorkspaceNotFound = errors.New("workspace not found")
+
func GetWorkspace(c *gin.Context, workspaceId string) (*workspace.Workspace, error) {
dbWorkspaceIds, err := db.GetWorkspaceIDs(c.Request.Context())
if err != nil {
return nil, err
}
if !slices.Contains(dbWorkspaceIds, workspaceId) {
- return nil, fmt.Errorf("workspace %s not found in database", workspaceId)
+ return nil, fmt.Errorf("%w: %s", ErrWorkspaceNotFound, workspaceId)
}Then in handlers (e.g., policies.go):
ws, err := utils.GetWorkspace(c, workspaceId)
if err != nil {
+ if errors.Is(err, utils.ErrWorkspaceNotFound) {
+ c.JSON(http.StatusNotFound, gin.H{
+ "error": err.Error(),
+ })
+ return
+ }
c.JSON(http.StatusInternalServerError, gin.H{
"error": "Failed to get workspace: " + err.Error(),
})
return
}Apply the same pattern to releasetargets.go at lines 18-24 and 72-78.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In apps/workspace-engine/pkg/server/openapi/utils/utils.go around lines 18-20,
returning a generic fmt.Errorf for a missing workspace causes callers to return
500 instead of 404; define and export a sentinel error variable (e.g.,
ErrWorkspaceNotFound) in that package and return that sentinel when the
workspaceId is not present, then update handlers that call this util
(apps/workspace-engine/pkg/server/openapi/policies.go around line 17 and
apps/workspace-engine/pkg/server/openapi/releasetargets.go around lines 18-24
and 72-78) to detect the sentinel error and translate it to an HTTP 404 response
(only other errors remain 500). Ensure the sentinel is a package-level var
(errors.New(...)) so comparisons use errors.Is or direct equality as
appropriate.
| wsExists := workspace.Exists(workspaceId) | ||
| if wsExists { | ||
| return workspace.GetWorkspace(workspaceId), nil | ||
| } | ||
|
|
||
| ws := workspace.New(workspaceId) | ||
| if err := workspace.PopulateWorkspaceWithInitialState(c.Request.Context(), ws); err != nil { | ||
| return nil, fmt.Errorf("failed to populate workspace with initial state: %w", err) | ||
| } | ||
| workspace.Set(workspaceId, ws) | ||
| return ws, nil |
There was a problem hiding this comment.
🧩 Analysis chain
Potential race condition in workspace initialization.
Between checking workspace.Exists(workspaceId) (line 22) and calling workspace.Set(workspaceId, ws) (line 31), another concurrent request for the same workspace could proceed through the same initialization path. Both requests would:
- Create separate workspace instances
- Populate from the database (duplicate work)
- Call
workspace.Set, with the last write winning
While the final state should be consistent (both load the same DB snapshot), this wastes resources and could cause subtle issues if workspace initialization has side effects.
Consider using a sync mechanism to ensure single initialization per workspace:
var (
workspaceInitLocks sync.Map // map[string]*sync.Mutex
)
func getOrCreateInitLock(workspaceId string) *sync.Mutex {
lock, _ := workspaceInitLocks.LoadOrStore(workspaceId, &sync.Mutex{})
return lock.(*sync.Mutex)
}
func GetWorkspace(c *gin.Context, workspaceId string) (*workspace.Workspace, error) {
dbWorkspaceIds, err := db.GetWorkspaceIDs(c.Request.Context())
if err != nil {
return nil, err
}
if !slices.Contains(dbWorkspaceIds, workspaceId) {
return nil, fmt.Errorf("workspace %s not found in database", workspaceId)
}
if workspace.Exists(workspaceId) {
return workspace.GetWorkspace(workspaceId), nil
}
lock := getOrCreateInitLock(workspaceId)
lock.Lock()
defer lock.Unlock()
// Double-check after acquiring lock
if workspace.Exists(workspaceId) {
return workspace.GetWorkspace(workspaceId), nil
}
ws := workspace.New(workspaceId)
if err := workspace.PopulateWorkspaceWithInitialState(c.Request.Context(), ws); err != nil {
return nil, fmt.Errorf("failed to populate workspace with initial state: %w", err)
}
workspace.Set(workspaceId, ws)
return ws, nil
}Alternatively, verify if the current behavior is acceptable by checking:
- Whether duplicate initialization causes any data consistency issues
- How frequently this code path is hit under concurrent load
🏁 Script executed:
#!/bin/bash
# Search for other callers of PopulateWorkspaceWithInitialState to understand initialization patterns
rg -nP 'PopulateWorkspaceWithInitialState' --type=go -C3Length of output: 2045
I need to examine the workspace package implementation to verify if the race condition actually exists.
Let me search more broadly:
---
🏁 Script executed:
```shell
#!/bin/bash
# Find and examine workspace package Set and Exists implementations
rg -nP '^\s*func\s+(Set|Exists|New)\s*\(' --type=go -A5 apps/workspace-engine/pkg/workspace/
Length of output: 5483
🏁 Script executed:
#!/bin/bash
# Search for workspace package files and Set/Exists implementation
fd -e go apps/workspace-engine/pkg/workspace/ | head -20Length of output: 449
🏁 Script executed:
#!/bin/bash
# Find Set, Exists functions in workspace package with more context
rg -nP '(func.*Set|func.*Exists|var.*Map|sync\.)' --type=go apps/workspace-engine/pkg/workspace/ -C2Length of output: 11177
I need to verify what cmap is and confirm the TOCTOU race condition concern.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find cmap import and definition
rg -nP 'import.*cmap|package cmap' --type=go | head -10
Length of output: 132
🏁 Script executed:
#!/bin/bash
# Look at the utils.go file context to see the full GetWorkspace function
rg -nP 'func GetWorkspace' --type=go -B5 -A15 apps/workspace-engine/pkg/server/openapi/utils/Length of output: 1768
🏁 Script executed:
#!/bin/bash
# Check if there's any locking in handler.go around workspace initialization
rg -nP 'wsExists|PopulateWorkspaceWithInitialState' --type=go -B10 -A5 apps/workspace-engine/pkg/events/handler/handler.goLength of output: 936
I need to verify the cmap implementation to confirm the race condition exists.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine cmap Get and Set implementations
cat apps/workspace-engine/pkg/cmap/concurrent_map.go | head -100
Length of output: 2611
1 participant
Summary by CodeRabbit
Bug Fixes
Refactor
New Features
Tests