Merged
Conversation
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
18 tasks
Contributor
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
WalkthroughHandlers and config functions now perform DB mutations before updating in-memory state, add DB rollback on memory-update failures, change deletion order to DB-first, and update related error handling; supporting tests, E2E fixtures, and a workflow tweak were also added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler as HTTP Handler
participant DB as Database
participant Mem as In-Memory Store
Client->>Handler: updateMCPClient(request)
Handler->>DB: Fetch existing MCP config
DB-->>Handler: existingConfig
Handler->>DB: Update MCP config in DB
DB-->>Handler: DB update success
Handler->>Mem: Apply in-memory update using DB-derived fields
alt Mem update success
Mem-->>Handler: success
Handler->>Client: 200 OK
else Mem update failure
Mem-->>Handler: failure
Handler->>DB: Rollback to existingConfig
DB-->>Handler: rollback result
Handler->>Client: 500 Memory update failed (rollback attempted)
end
sequenceDiagram
participant Client
participant Handler as HTTP Handler
participant DB as Database
participant Mem as In-Memory Store
Client->>Handler: deleteMCPClient(id)
Handler->>DB: Delete MCP config from DB
alt DB delete success
DB-->>Handler: success
Handler->>Mem: Remove MCP client from memory
Mem-->>Handler: success
Handler->>Client: 200 OK
else DB delete failure
DB-->>Handler: error
Handler->>Client: 500 DB delete failed (no memory deletion)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
3 similar comments
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
transports/bifrost-http/handlers/plugins.go (1)
225-245:⚠️ Potential issue | 🟠 MajorRevert createPlugin to graceful-degradation response when ReloadPlugin fails
Lines 238-243 now return 500 after a successful database insert, contradicting the documented intentional plugin behavior (graceful degradation: return 201 with warning message). This creates confusing retry scenarios where callers see failure despite successful persistence. Plugins should follow the documented pattern of persisting to database first, then attempting reload; if reload fails, return 201 with a message explaining the failure rather than 500. Governance entities (routing rules) require strict consistency (500 on reload fail), but plugins support graceful degradation per PR 470.
🛠️ Suggested adjustment
if request.Enabled { if err := h.pluginsLoader.ReloadPlugin(ctx, request.Name, request.Path, request.Config); err != nil { logger.Error("failed to load plugin: %v", err) - SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Plugin created in database but failed to load: %v", err)) - return + plugin, getErr := h.configStore.GetPlugin(ctx, request.Name) + if getErr != nil { + SendError(ctx, fasthttp.StatusInternalServerError, fmt.Sprintf("Plugin created in database but failed to load (and fetch): %v", err)) + return + } + ctx.SetStatusCode(fasthttp.StatusCreated) + SendJSON(ctx, map[string]any{ + "message": fmt.Sprintf("Plugin created in database but failed to load: %v", err), + "plugin": plugin, + "load_error": err.Error(), + }) + return } }
akshaydeo
force-pushed
the
02-09-memory_db_inconsitency_fixes
branch
from
9c27faf to
9c9905a
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
3 similar comments
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
Author
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@transports/bifrost-http/handlers/plugins.go`:
- Around line 238-245: When ReloadPlugin (h.pluginsLoader.ReloadPlugin(ctx,
request.Name, request.Path, request.Config)) fails while request.Enabled is
true, perform a DB rollback to remove or disable the created plugin record
before returning the 500: call the service/repo method that created the DB entry
(e.g. the plugin store's Delete or Disable function keyed by request.Name)
inside the error branch, log the rollback result, and then call SendError as
before; ensure this rollback is attempted only on reload failure and does not
mask the original reload error.
akshaydeo
force-pushed
the
02-09-memory_db_inconsitency_fixes
branch
from
9c9905a to
d37b367
Compare
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
Implement transaction safety for database and in-memory state operations to prevent inconsistencies when updates fail.
Changes
Type of change
Affected areas
How to test
Test the following scenarios to verify transaction safety:
Breaking changes
Security considerations
This change improves system stability by preventing inconsistent states between database and in-memory configurations, which could lead to unexpected behavior or security issues.
Checklist