Unregister dev mode session on shutdown #379
Conversation
|
""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant API
Client->>Server: Call Close()
Server->>API: DELETE /cli/devmode/{projectId}/{serverId}
API-->>Server: Response (success or error)
Server->>Server: Wait for goroutines, close TLS connection
Server-->>Client: Close() returns
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🧪 Generate Unit Tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
internal/dev/server.go (1)
108-116: 🛠️ Refactor suggestionOrder of operations could unintentionally rely on implementation details
Close()cancelss.ctxbefore invokings.closeConnection().
Becauses.apiclientwas built withcontext.WithoutCancel(ctx), this works today, but the correctness now relies on:
util.NewAPIClientonly using the supplied context and never the later-cancelleds.ctx.- Future maintainers remembering this subtle coupling.
Placing
s.closeConnection()befores.cancel()would avoid the dependency and make the intent obvious.- s.cancel() - s.closeConnection() + s.closeConnection() + s.cancel()
🧹 Nitpick comments (2)
internal/dev/server.go (2)
123-127:closeConnectionlacks timeout / 404 handlingThe DELETE call can hang indefinitely or treat a legitimate “already gone” 404 as an error.
Consider:
- Wrapping the request in a short context timeout so shutdown cannot block forever.
- Treating
404/410responses as success.func (s *Server) closeConnection() { - if err := s.apiclient.Do("DELETE", "/cli/devmode/"+s.Project.Project.ProjectId+"/"+s.ID, nil, nil); err != nil { - s.logger.Error("failed to send close connection: %s", err) - } + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + if err := s.apiclient.DoWithContext(ctx, "DELETE", + "/cli/devmode/"+s.Project.Project.ProjectId+"/"+s.ID, nil, nil); err != nil && + !util.IsNotFound(err) { // helper that maps 404/410 to nil + s.logger.Warn("devmode unregister failed: %s", err) + } }
668-672: Good call oncontext.WithoutCancel, but watch for orphaned requestsUsing
context.WithoutCancel(ctx)ensures API calls surviveServercancellation, which fixes premature shutdowns.
Just make sureutil.APIClientrespects request-scoped contexts (e.g., takes one inDo(...)) so long-running or hung requests can still be aborted by upper layers; otherwise they may leak.No action required, just flagging the trade-off for future maintenance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/dev/server.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/util/api.go (1)
238-260:⚠️ Potential issueDev container rewrite now misses the new
.iodomain
TransformUrlstill only recognisesapi.agentuity.dev; after this PR, dev traffic will useapi.agentuity.io, so the Docker-host rewrite will silently stop working.
Add the.iochecks (and prefix variant) to keep “inside-container” flows functional.-if strings.Contains(urlString, "api.agentuity.dev") || strings.Contains(urlString, "localhost:") || strings.Contains(urlString, "127.0.0.1:") { +if strings.Contains(urlString, "api.agentuity.dev") || // legacy + strings.Contains(urlString, "api.agentuity.io") || // new + strings.Contains(urlString, "localhost:") || + strings.Contains(urlString, "127.0.0.1:") { @@ - if strings.HasPrefix(urlString, "https://api.agentuity.dev") { + if strings.HasPrefix(urlString, "https://api.agentuity.dev") || + strings.HasPrefix(urlString, "https://api.agentuity.io") {
🧹 Nitpick comments (1)
internal/util/api.go (1)
269-272: Nit: misleading log messageThe message still says “dev” but we are matching the
.iodomain now.
Consider clarifying to avoid confusion when reading logs.-logger.Debug("switching app url to dev since the api url is dev") +logger.Debug("switching app url to io-dev since the api url is io")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/dev/server.go(3 hunks)internal/util/api.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/dev/server.go
🧰 Additional context used
🧠 Learnings (1)
internal/util/api.go (1)
Learnt from: pec1985
PR: agentuity/cli#342
File: cmd/logs.go:58-60
Timestamp: 2025-05-26T08:15:51.542Z
Learning: In the agentuity/cli codebase, util.GetURLs returns (apiUrl, appUrl, transportUrl string) and util.EnsureLoggedIn returns (apikey, userId string). These functions handle errors internally and call os.Exit(1) for fatal conditions rather than returning errors. The blank identifiers in calls to these functions are for ignoring unused return values, not errors.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test CLI Upgrade Path (windows-latest)
Summary by CodeRabbit