feat: support multiple concurrent Sentry instances#1469
Conversation
|
Important Review skippedDraft detected. 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 Sentry integration is migrated from a singleton configuration to a named multi-instance model. The backend gains instance CRUD APIs, per-instance secret keying, generation-safe client caching, schema migration from the legacy singleton row, and instance-scoped browse endpoints. The frontend adds instance selectors across the settings page, issue dialogs, and link button, with all API calls now requiring ChangesSentry Multi-Instance Support
Sequence Diagram(s)sequenceDiagram
participant UI as Frontend UI
participant API as sentry-api.ts
participant Handlers as HTTP Handlers
participant Service as Service
participant Store as Store
participant Secrets as SecretStore
rect rgba(70, 130, 180, 0.5)
note over UI,Secrets: Instance Create + Probe Flow
UI->>API: createSentryInstance({ name, url, secret })
API->>Handlers: POST /api/v1/sentry/instances
Handlers->>Service: CreateInstance(ctx, req)
Service->>Store: CreateConfig(cfg)
Service->>Secrets: Set(secretKeyFor(id), token)
Service-->>Service: go probeAuthFor(id) async
Service-->>Handlers: *SentryConfig
Handlers-->>API: 201 Created
API-->>UI: SentryConfig { id, name }
end
rect rgba(100, 160, 100, 0.5)
note over UI,Secrets: Browse Issues (instance-scoped)
UI->>API: searchSentryIssues(instanceId, filter)
API->>Handlers: GET /issues?instanceId=...
Handlers->>Service: SearchIssues(ctx, instanceId, filter)
Service->>Service: clientForInstance(instanceId)
Service->>Secrets: Exists + Reveal(secretKeyFor(instanceId))
Service-->>UI: issue list
end
rect rgba(180, 80, 80, 0.5)
note over UI,Store: Delete Blocked by Watch
UI->>API: deleteSentryInstance(id)
API->>Handlers: DELETE /instances/:id
Handlers->>Service: DeleteInstance(ctx, id)
Service->>Store: CountWatchesForInstance(id)
Store-->>Service: count > 0
Service-->>Handlers: ErrInstanceInUse{ WatchCount }
Handlers-->>UI: 409 SENTRY_INSTANCE_IN_USE + watchCount
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
| Filename | Overview |
|---|---|
| apps/backend/internal/sentry/store.go | Core schema + migration rewrite: removes CHECK singleton constraint via DROP+CREATE tx, adds ListConfigs/CreateConfig/UpdateConfig/DeleteConfig/CountWatchesForInstance; watch backfill in addWatchInstanceColumn lacks the crash-recovery fallback that migrateLegacySecret has, creating a window where pre-existing watches lose their instance binding. |
| apps/backend/internal/sentry/service.go | Service converted from singleton to per-instance map[string]Client with per-id generation counters; CreateInstance/UpdateInstance/DeleteInstance added; DeleteInstance uses check-then-delete across two transactions (TOCTOU window); otherwise clean refactor preserving the existing invalidation safety pattern. |
| apps/backend/internal/sentry/provider.go | Adds migrateLegacySecret with crash-safe recovery (sole-config fallback when migratedSingletonID is empty); best-effort with warn-only on failure; logic is sound. |
| apps/backend/internal/sentry/service_issue_watch.go | CheckIssueWatch now routes through clientForInstance(w.InstanceID); requireInstance validates instance existence on create and update; event carries instanceId; all looks correct. |
| apps/backend/internal/sentry/handlers.go | CRUD routes for /instances, shared httpTestInstance across two routes (correct empty-id behavior), writeMissingInstance for browse endpoints without instanceId; clean refactor. |
| apps/backend/internal/sentry/store_issue_watch.go | Adds sentry_instance_id to insert/select/update column lists and issueWatchRow; straightforward column addition. |
| apps/web/components/sentry/sentry-settings.tsx | Settings page refactored from singleton form to multi-instance list; per-instance auth banner, add/edit/delete, 409 in-use error surfaced with watch count; looks complete. |
| apps/web/lib/api/domains/sentry-api.ts | API client updated to /instances CRUD; testSentryConnection routes to /instances/:id/test or /test-connection correctly; instanceId threaded through browse endpoints. |
| apps/web/hooks/domains/sentry/use-sentry-availability.ts | useSentryAvailable now reports true when any instance has hasSecret && lastOk; correct multi-instance semantics. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Boot as Server Boot
participant Store as Store.initSchema
participant Provider as provider.migrateLegacySecret
participant SecretStore as SecretStore
Boot->>Store: initSchema()
Store->>Store: addConfigURLColumn() [noop if done]
Store->>Store: migrateConfigsToInstances()
Note over Store: DROP+CREATE in tx, assigns new UUID, sets migratedSingletonID
Store->>Store: addWatchInstanceColumn()
Note over Store: ALTER + backfill using migratedSingletonID
Note over Store: crash here leaves migratedSingletonID=empty on next boot
Boot->>Provider: migrateLegacySecret(store, secrets)
Provider->>Store: MigratedSingletonID()
alt migratedSingletonID is empty
Provider->>Store: ListConfigs()
Store-->>Provider: [cfg] exactly 1 → use cfg.ID
end
Provider->>SecretStore: Exists(sentry:id:token)
alt not exists
Provider->>SecretStore: Reveal(sentry:singleton:token)
Provider->>SecretStore: Set(sentry:id:token, value)
Provider->>SecretStore: Delete(sentry:singleton:token)
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Boot as Server Boot
participant Store as Store.initSchema
participant Provider as provider.migrateLegacySecret
participant SecretStore as SecretStore
Boot->>Store: initSchema()
Store->>Store: addConfigURLColumn() [noop if done]
Store->>Store: migrateConfigsToInstances()
Note over Store: DROP+CREATE in tx, assigns new UUID, sets migratedSingletonID
Store->>Store: addWatchInstanceColumn()
Note over Store: ALTER + backfill using migratedSingletonID
Note over Store: crash here leaves migratedSingletonID=empty on next boot
Boot->>Provider: migrateLegacySecret(store, secrets)
Provider->>Store: MigratedSingletonID()
alt migratedSingletonID is empty
Provider->>Store: ListConfigs()
Store-->>Provider: [cfg] exactly 1 → use cfg.ID
end
Provider->>SecretStore: Exists(sentry:id:token)
alt not exists
Provider->>SecretStore: Reveal(sentry:singleton:token)
Provider->>SecretStore: Set(sentry:id:token, value)
Provider->>SecretStore: Delete(sentry:singleton:token)
end
Comments Outside Diff (1)
-
apps/backend/internal/sentry/store.go, line 852-876 (link)Watch backfill has no crash-recovery path
addWatchInstanceColumnonly backfillssentry_instance_idwhens.migratedSingletonID != "", which is only set bymigrateConfigsToInstancesin the same boot. If the process crashes aftermigrateConfigsToInstancescommits (new table in place) but beforeaddWatchInstanceColumnruns — for example, the OOM killer fires between those two calls ininitSchema— then on the next bootmigrateConfigsToInstancessees thenamecolumn and returns without settingmigratedSingletonID.addWatchInstanceColumnthen adds the column (since it's missing) but skips the backfill, leaving all pre-existing watches withsentry_instance_id = ''.clientForInstancereturnsErrNotConfiguredfor an empty ID, so every pre-existing watch silently stops firing.migrateLegacySecretalready handles this crash window by falling back to "exactly-one-config" recovery whenMigratedSingletonID()is empty. The same pattern is needed here: whenmigratedSingletonID == "", look up the sole config (if any) and use its ID for theUPDATE ... WHERE sentry_instance_id = ''backfill.
Reviews (1): Last reviewed commit: "feat: support multiple concurrent Sentry..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return nil, err | ||
| } | ||
| return client.ListOrganizations(ctx) | ||
| } | ||
|
|
||
| // ListProjects returns the projects the stored token can access. | ||
| func (s *Service) ListProjects(ctx context.Context) ([]SentryProject, error) { | ||
| client, err := s.clientFor(ctx) | ||
| // ListProjects returns the projects the instance's token can access. | ||
| func (s *Service) ListProjects(ctx context.Context, instanceID string) ([]SentryProject, error) { | ||
| client, err := s.clientForInstance(ctx, instanceID) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return client.ListProjects(ctx) | ||
| } | ||
|
|
||
| // SearchIssues runs a filtered search. The caller supplies the org/project to | ||
| // search — there is no install-wide default to fall back on. | ||
| func (s *Service) SearchIssues(ctx context.Context, filter SearchFilter, cursor string) (*SearchResult, error) { | ||
| client, err := s.clientFor(ctx) | ||
| // SearchIssues runs a filtered search against one instance. The caller supplies | ||
| // the org/project to search — there is no install-wide default to fall back on. | ||
| func (s *Service) SearchIssues(ctx context.Context, instanceID string, filter SearchFilter, cursor string) (*SearchResult, error) { | ||
| client, err := s.clientForInstance(ctx, instanceID) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
DeleteInstance
CountWatchesForInstance and DeleteConfig run in separate transactions. If a watch is created for this instance in the gap between the two calls — by a concurrent CreateIssueWatch request — the delete proceeds and the newly created watch now references a non-existent config. Subsequent polling for that watch will call clientForInstance → store.GetConfig → nil → ErrNotConfigured, meaning the watch silently never fires and there is no UI indication that the instance it points to is gone.
Wrapping both operations in a single SQLite transaction (read the count and delete the row in one BEGIN...COMMIT) eliminates the window entirely.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found across 30 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/backend/internal/sentry/handlers_test.go">
<violation number="1" location="apps/backend/internal/sentry/handlers_test.go:78">
P2: Missing HTTP test coverage for the UpdateInstance (PUT /api/v1/sentry/instances/:id) endpoint</violation>
</file>
<file name="apps/web/components/sentry/sentry-issue-dialog.tsx">
<violation number="1" location="apps/web/components/sentry/sentry-issue-dialog.tsx:179">
P2: Switching Sentry instances does not reset org/project filters or search results, leaving stale cross-instance state in the UI. Add a reset of `filter`, `issues`, `nextCursor`, and `isLast` when `instanceId` changes so the old instance's org slug, project slug, and issue list do not persist after the switch.</violation>
</file>
<file name="apps/backend/internal/sentry/store_issue_watch.go">
<violation number="1" location="apps/backend/internal/sentry/store_issue_watch.go:208">
P1: UpdateIssueWatch allows changing sentry_instance_id in-place but does not reset the watch's dedup rows or last_polled_at. Previously-seen issue_short_id values from the old instance will continue to suppress matching issues on the new instance (since dedup is keyed by watch_id + short_id, not instance), and retained last_polled_at can skip historical issues on the new instance. Consider clearing watch state (dedup rows and last_polled_at) when the instance ID changes, or reject instance changes on the update path.</violation>
</file>
<file name="apps/web/components/sentry/sentry-link-button.tsx">
<violation number="1" location="apps/web/components/sentry/sentry-link-button.tsx:39">
P2: Instance-list fetch failures are silently swallowed, creating a non-recoverable UI dead-end where the user can see the Link button and submit an issue key but receives a 'Select a Sentry instance' error with no picker visible.</violation>
</file>
<file name="apps/backend/internal/sentry/store.go">
<violation number="1" location="apps/backend/internal/sentry/store.go:308">
P2: Migration/backfill is not restart-safe: legacy watches may remain with empty sentry_instance_id after an interrupted upgrade</violation>
</file>
<file name="apps/web/components/sentry/sentry-settings.tsx">
<violation number="1" location="apps/web/components/sentry/sentry-settings.tsx:423">
P2: Concurrent fetch responses can cause stale state overwrites in `useSentryInstances`; add a request-version guard before applying fetched instance lists.</violation>
<violation number="2" location="apps/web/components/sentry/sentry-settings.tsx:534">
P1: Add and edit forms can be shown concurrently, producing duplicate input IDs and test IDs. The `adding` and `editingId` states are independent with no mutual exclusion, so a user can open the "Add instance" form while an existing instance is in edit mode. Both `InstanceFormCard` components use hardcoded IDs (`sentry-name`, `sentry-url`, `sentry-secret`) and identical `data-testid` values, creating invalid duplicate DOM IDs and ambiguous test selectors.</violation>
</file>
<file name="apps/backend/internal/sentry/service.go">
<violation number="1" location="apps/backend/internal/sentry/service.go:208">
P1: `DeleteInstance` has a race window between the watch-count check and the delete. A concurrent watch create can attach to this instance after the count check but before deletion, leaving an orphaned watch that can no longer resolve its config. Make the in-use check and delete atomic in a single DB transaction.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| } | ||
| _, err = s.db.ExecContext(ctx, ` | ||
| UPDATE sentry_issue_watches SET workflow_id = ?, workflow_step_id = ?, filter_json = ?, | ||
| UPDATE sentry_issue_watches SET sentry_instance_id = ?, workflow_id = ?, workflow_step_id = ?, filter_json = ?, |
There was a problem hiding this comment.
P1: UpdateIssueWatch allows changing sentry_instance_id in-place but does not reset the watch's dedup rows or last_polled_at. Previously-seen issue_short_id values from the old instance will continue to suppress matching issues on the new instance (since dedup is keyed by watch_id + short_id, not instance), and retained last_polled_at can skip historical issues on the new instance. Consider clearing watch state (dedup rows and last_polled_at) when the instance ID changes, or reject instance changes on the update path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/backend/internal/sentry/store_issue_watch.go, line 208:
<comment>UpdateIssueWatch allows changing sentry_instance_id in-place but does not reset the watch's dedup rows or last_polled_at. Previously-seen issue_short_id values from the old instance will continue to suppress matching issues on the new instance (since dedup is keyed by watch_id + short_id, not instance), and retained last_polled_at can skip historical issues on the new instance. Consider clearing watch state (dedup rows and last_polled_at) when the instance ID changes, or reject instance changes on the update path.</comment>
<file context>
@@ -203,11 +205,11 @@ func (s *Store) UpdateIssueWatch(ctx context.Context, w *IssueWatch) error {
}
_, err = s.db.ExecContext(ctx, `
- UPDATE sentry_issue_watches SET workflow_id = ?, workflow_step_id = ?, filter_json = ?,
+ UPDATE sentry_issue_watches SET sentry_instance_id = ?, workflow_id = ?, workflow_step_id = ?, filter_json = ?,
agent_profile_id = ?, executor_profile_id = ?, prompt = ?,
enabled = ?, poll_interval_seconds = ?, max_inflight_tasks = ?, updated_at = ?
</file context>
| const disableTest = missingSecret; | ||
| const { toast } = useToast(); | ||
| const { instances, loading, reload } = useSentryInstances(); | ||
| const [adding, setAdding] = useState(false); |
There was a problem hiding this comment.
P1: Add and edit forms can be shown concurrently, producing duplicate input IDs and test IDs. The adding and editingId states are independent with no mutual exclusion, so a user can open the "Add instance" form while an existing instance is in edit mode. Both InstanceFormCard components use hardcoded IDs (sentry-name, sentry-url, sentry-secret) and identical data-testid values, creating invalid duplicate DOM IDs and ambiguous test selectors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/sentry/sentry-settings.tsx, line 534:
<comment>Add and edit forms can be shown concurrently, producing duplicate input IDs and test IDs. The `adding` and `editingId` states are independent with no mutual exclusion, so a user can open the "Add instance" form while an existing instance is in edit mode. Both `InstanceFormCard` components use hardcoded IDs (`sentry-name`, `sentry-url`, `sentry-secret`) and identical `data-testid` values, creating invalid duplicate DOM IDs and ambiguous test selectors.</comment>
<file context>
@@ -374,42 +463,133 @@ function EnabledPill() {
- const disableTest = missingSecret;
+ const { toast } = useToast();
+ const { instances, loading, reload } = useSentryInstances();
+ const [adding, setAdding] = useState(false);
+ const [editingId, setEditingId] = useState<string | null>(null);
+
</file context>
| // with ErrInstanceInUse when issue watches still reference the instance, so a | ||
| // delete can never silently orphan watches. | ||
| func (s *Service) DeleteInstance(ctx context.Context, id string) error { | ||
| n, err := s.store.CountWatchesForInstance(ctx, id) |
There was a problem hiding this comment.
P1: DeleteInstance has a race window between the watch-count check and the delete. A concurrent watch create can attach to this instance after the count check but before deletion, leaving an orphaned watch that can no longer resolve its config. Make the in-use check and delete atomic in a single DB transaction.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/backend/internal/sentry/service.go, line 208:
<comment>`DeleteInstance` has a race window between the watch-count check and the delete. A concurrent watch create can attach to this instance after the count check but before deletion, leaving an orphaned watch that can no longer resolve its config. Make the in-use check and delete atomic in a single DB transaction.</comment>
<file context>
@@ -93,76 +98,155 @@ func (s *Service) Store() *Store {
+// with ErrInstanceInUse when issue watches still reference the instance, so a
+// delete can never silently orphan watches.
+func (s *Service) DeleteInstance(ctx context.Context, id string) error {
+ n, err := s.store.CountWatchesForInstance(ctx, id)
+ if err != nil {
+ return err
</file context>
| ctrl, router, _ := newTestController(t) | ||
| seedConfig(t, ctrl) | ||
| req := httptest.NewRequest(http.MethodGet, "/api/v1/sentry/config", nil) | ||
| func TestHTTPInstances_CreateListGetDelete(t *testing.T) { |
There was a problem hiding this comment.
P2: Missing HTTP test coverage for the UpdateInstance (PUT /api/v1/sentry/instances/:id) endpoint
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/backend/internal/sentry/handlers_test.go, line 78:
<comment>Missing HTTP test coverage for the UpdateInstance (PUT /api/v1/sentry/instances/:id) endpoint</comment>
<file context>
@@ -30,59 +30,160 @@ func newTestController(t *testing.T) (*Controller, *gin.Engine, *fakeClient) {
- ctrl, router, _ := newTestController(t)
- seedConfig(t, ctrl)
- req := httptest.NewRequest(http.MethodGet, "/api/v1/sentry/config", nil)
+func TestHTTPInstances_CreateListGetDelete(t *testing.T) {
+ _, router, _ := newTestController(t)
+
</file context>
| return; | ||
| } | ||
| if (loaded) return; | ||
| if (loadedFor.current === instanceId) return; |
There was a problem hiding this comment.
P2: Switching Sentry instances does not reset org/project filters or search results, leaving stale cross-instance state in the UI. Add a reset of filter, issues, nextCursor, and isLast when instanceId changes so the old instance's org slug, project slug, and issue list do not persist after the switch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/sentry/sentry-issue-dialog.tsx, line 179:
<comment>Switching Sentry instances does not reset org/project filters or search results, leaving stale cross-instance state in the UI. Add a reset of `filter`, `issues`, `nextCursor`, and `isLast` when `instanceId` changes so the old instance's org slug, project slug, and issue list do not persist after the switch.</comment>
<file context>
@@ -146,47 +160,94 @@ function toSearchFilter(filter: FilterState): SentrySearchFilter {
return;
}
- if (loaded) return;
+ if (loadedFor.current === instanceId) return;
let cancelled = false;
(async () => {
</file context>
| setInstances(list); | ||
| if (list.length === 1) setSelected(list[0].id); | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
P2: Instance-list fetch failures are silently swallowed, creating a non-recoverable UI dead-end where the user can see the Link button and submit an issue key but receives a 'Select a Sentry instance' error with no picker visible.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/sentry/sentry-link-button.tsx, line 39:
<comment>Instance-list fetch failures are silently swallowed, creating a non-recoverable UI dead-end where the user can see the Link button and submit an issue key but receives a 'Select a Sentry instance' error with no picker visible.</comment>
<file context>
@@ -13,12 +16,79 @@ type SentryLinkButtonProps = {
+ setInstances(list);
+ if (list.length === 1) setSelected(list[0].id);
+ })
+ .catch(() => {
+ if (!cancelled) setInstances([]);
+ });
</file context>
| return fmt.Errorf("add sentry_instance_id column: %w", err) | ||
| } | ||
| } | ||
| if s.migratedSingletonID != "" { |
There was a problem hiding this comment.
P2: Migration/backfill is not restart-safe: legacy watches may remain with empty sentry_instance_id after an interrupted upgrade
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/backend/internal/sentry/store.go, line 308:
<comment>Migration/backfill is not restart-safe: legacy watches may remain with empty sentry_instance_id after an interrupted upgrade</comment>
<file context>
@@ -158,6 +288,37 @@ func (s *Store) addIssueWatchLastErrorColumns() error {
+ return fmt.Errorf("add sentry_instance_id column: %w", err)
+ }
+ }
+ if s.migratedSingletonID != "" {
+ if _, err := s.db.Exec(
+ `UPDATE sentry_issue_watches SET sentry_instance_id = ? WHERE sentry_instance_id = ''`,
</file context>
| const cfg = (await fetchSentryConfig()) ?? null; | ||
| setConfig(cfg); | ||
| setForm(configToForm(cfg)); | ||
| setInstances(await listSentryInstances()); |
There was a problem hiding this comment.
P2: Concurrent fetch responses can cause stale state overwrites in useSentryInstances; add a request-version guard before applying fetched instance lists.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/web/components/sentry/sentry-settings.tsx, line 423:
<comment>Concurrent fetch responses can cause stale state overwrites in `useSentryInstances`; add a request-version guard before applying fetched instance lists.</comment>
<file context>
@@ -173,142 +215,214 @@ function TestResultAlert({ result }: { result: TestSentryConnectionResult | null
- const cfg = (await fetchSentryConfig()) ?? null;
- setConfig(cfg);
- setForm(configToForm(cfg));
+ setInstances(await listSentryInstances());
} catch (err) {
- toast({ description: `Failed to load Sentry config: ${String(err)}`, variant: "error" });
</file context>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/backend/internal/sentry/handlers_test.go (1)
105-105: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winCheck the unmarshal error for consistency and clearer test failures.
The discarded error on line 105 is inconsistent with the pattern used elsewhere in this file (lines 70-72, 91-93, 275-277). If JSON unmarshaling fails, the test will fail with a misleading message ("expected 1 instance, got 0") instead of a clear decode error, making debugging harder.
🔧 Suggested fix
_ = json.Unmarshal(w.Body.Bytes(), &listResp) +if err := json.Unmarshal(w.Body.Bytes(), &listResp); err != nil { + t.Fatalf("decode list: %v", err) +} -if len(listResp.Instances) != 1 { +if len(listResp.Instances) != 1 {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/backend/internal/sentry/handlers_test.go` at line 105, The json.Unmarshal error on line 105 is being discarded with the blank identifier (_), which is inconsistent with how errors are handled in other parts of this file (lines 70-72, 91-93, 275-277). Instead of ignoring the error, capture it and add a check similar to the pattern used elsewhere in the file to ensure that if unmarshaling fails, the test will fail with the actual decode error message rather than a misleading message about unexpected data. Apply the same error-checking pattern used in the other test cases to the json.Unmarshal call for the listResp variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/backend/internal/sentry/store.go`:
- Line 65: The sentry_instance_id column lacks database-level referential
integrity constraints, allowing orphaned watches when an instance is deleted
concurrently with watch creation. Add a foreign key constraint on the
sentry_instance_id column that references the instances table with DELETE
RESTRICT to prevent deletion of instances with existing watches, or refactor the
watch creation logic (particularly in the operations around line ranges 405-408
and 421-427) and the DeleteConfig function to use atomic transactions that make
the instance existence check and watch insert indivisible, eliminating the race
condition between CountWatchesForInstance and DeleteConfig.
- Around line 224-228: The migration in the Commit block saves the randomly
generated replacement ID only to the in-memory variable s.migratedSingletonID,
but if the process exits after tx.Commit() succeeds but before
addWatchInstanceColumn runs, the next boot will see the migration already
applied while s.migratedSingletonID remains empty, causing legacy watches to
stay bound incorrectly. Move the persistence of the legacy-to-new ID mapping
into the same database transaction before the commit, include the watch backfill
logic (addWatchInstanceColumn) within that same transaction, or implement
recovery logic in addWatchInstanceColumn to query the database for the migrated
config ID when s.migratedSingletonID is empty to ensure the migration remains
recoverable at every step.
---
Nitpick comments:
In `@apps/backend/internal/sentry/handlers_test.go`:
- Line 105: The json.Unmarshal error on line 105 is being discarded with the
blank identifier (_), which is inconsistent with how errors are handled in other
parts of this file (lines 70-72, 91-93, 275-277). Instead of ignoring the error,
capture it and add a check similar to the pattern used elsewhere in the file to
ensure that if unmarshaling fails, the test will fail with the actual decode
error message rather than a misleading message about unexpected data. Apply the
same error-checking pattern used in the other test cases to the json.Unmarshal
call for the listResp variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e25f478-5fbc-49df-a385-785e6e14c97a
📒 Files selected for processing (30)
apps/backend/internal/integrations/AGENTS.mdapps/backend/internal/orchestrator/source_sentry.goapps/backend/internal/orchestrator/source_sentry_test.goapps/backend/internal/sentry/handlers.goapps/backend/internal/sentry/handlers_test.goapps/backend/internal/sentry/mock_controller.goapps/backend/internal/sentry/models.goapps/backend/internal/sentry/poller.goapps/backend/internal/sentry/poller_issue_watch_test.goapps/backend/internal/sentry/poller_test.goapps/backend/internal/sentry/provider.goapps/backend/internal/sentry/service.goapps/backend/internal/sentry/service_issue_watch.goapps/backend/internal/sentry/service_issue_watch_test.goapps/backend/internal/sentry/service_test.goapps/backend/internal/sentry/store.goapps/backend/internal/sentry/store_issue_watch.goapps/backend/internal/sentry/store_test.goapps/web/components/integrations/validated-popover.tsxapps/web/components/sentry/sentry-issue-dialog.tsxapps/web/components/sentry/sentry-issue-watch-dialog.tsxapps/web/components/sentry/sentry-issue-watch-form.tsapps/web/components/sentry/sentry-link-button.tsxapps/web/components/sentry/sentry-settings.tsxapps/web/e2e/helpers/api-client.tsapps/web/e2e/pages/sentry-settings-page.tsapps/web/e2e/tests/integrations/sentry-settings.spec.tsapps/web/hooks/domains/sentry/use-sentry-availability.tsapps/web/lib/api/domains/sentry-api.tsapps/web/lib/types/sentry.ts
| CREATE TABLE IF NOT EXISTS sentry_issue_watches ( | ||
| id TEXT PRIMARY KEY, | ||
| workspace_id TEXT NOT NULL, | ||
| sentry_instance_id TEXT NOT NULL DEFAULT '', |
There was a problem hiding this comment.
Enforce instance/watch referential integrity atomically.
CountWatchesForInstance and DeleteConfig are separate operations, while sentry_instance_id has no DB-level restriction. A concurrent watch create can validate an instance before another request deletes it, leaving an orphaned watch that later cannot resolve a client. Add a foreign key with delete restriction, or use atomic conditional insert/delete paths that make the existence check and write indivisible.
Also applies to: 405-408, 421-427
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/internal/sentry/store.go` at line 65, The sentry_instance_id
column lacks database-level referential integrity constraints, allowing orphaned
watches when an instance is deleted concurrently with watch creation. Add a
foreign key constraint on the sentry_instance_id column that references the
instances table with DELETE RESTRICT to prevent deletion of instances with
existing watches, or refactor the watch creation logic (particularly in the
operations around line ranges 405-408 and 421-427) and the DeleteConfig function
to use atomic transactions that make the instance existence check and watch
insert indivisible, eliminating the race condition between
CountWatchesForInstance and DeleteConfig.
| if err := tx.Commit(); err != nil { | ||
| return err | ||
| } | ||
| s.migratedSingletonID = newID | ||
| return nil |
There was a problem hiding this comment.
Make the singleton migration recoverable before committing the generated ID.
Line 224 commits a random replacement ID, but Line 227 only keeps that ID in memory and Line 308 relies on it later to backfill watches. If the process exits after the config rebuild commits but before addWatchInstanceColumn runs, the next boot sees the config schema already migrated, migratedSingletonID stays empty, and legacy watches remain bound to sentry_instance_id = ''. Move the watch backfill into the same recoverable migration transaction, persist the legacy→new ID mapping, or recover the sole migrated config ID when empty legacy watches are present.
Also applies to: 308-313
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/backend/internal/sentry/store.go` around lines 224 - 228, The migration
in the Commit block saves the randomly generated replacement ID only to the
in-memory variable s.migratedSingletonID, but if the process exits after
tx.Commit() succeeds but before addWatchInstanceColumn runs, the next boot will
see the migration already applied while s.migratedSingletonID remains empty,
causing legacy watches to stay bound incorrectly. Move the persistence of the
legacy-to-new ID mapping into the same database transaction before the commit,
include the watch backfill logic (addWatchInstanceColumn) within that same
transaction, or implement recovery logic in addWatchInstanceColumn to query the
database for the migrated config ID when s.migratedSingletonID is empty to
ensure the migration remains recoverable at every step.
|
Just to check in, are you going to proceed with this one? you have another PR in draft 💪 Also, i am changing a bit the integrations settings in another PR Each integration settings will have a top right workspace selector So you can configure different sentry settings per workspace, is it what you're trying to accomplish here? |
|
Yes, I still need to properly review and test this. I've been quite busy with work, but will find time for this ASAP. Thanks for the push, and notification about the setting changes. |
ClemDNL
force-pushed
the
feature/multi-sentry-instanc-b7u
branch
from
2e11a6b to
e3d2abb
Compare
2 participants
Summary
Converts the Sentry integration from a single install-wide config to multiple named Sentry instances that run concurrently (e.g.
sentry.ioSaaS + a self-hosted host + extra org tokens). Each issue-watch and issue-browse operation binds to a specific instance. Existing single-instance installs migrate transparently on boot.Implements the saved task plan (model: multiple named install-wide instances with optional per-workspace scoping).
Backend
sentry_configs): dropped theCHECK(id='singleton')singleton; rows are nowid (UUID) + name + url + health. New id-keyed CRUD (ListConfigs/Get/Create/Update/Delete/HasAnyConfig/CountWatchesForInstance/UpdateAuthHealth(id,…)). On-boot migration rebuilds the legacy table, promotes the'singleton'row to a generated id named after its URL host, exposesMigratedSingletonID(), and backfillssentry_issue_watches.sentry_instance_id.map[string]Client+ per-id generation counter, preserving the TOCTOU race fix); browse methods take aninstanceId;CreateInstance/UpdateInstance/DeleteInstance(409ErrInstanceInUsewhen watches reference it);RecordAuthHealthprobes every instance.sentry:<id>:token(secretKeyFor). Provider migrates the legacysentry:singleton:tokencrash-safely (in-memory id, with a sole-config recovery fallback).sentry_instance_id, validated on create/update);CheckIssueWatchroutes to the watch's instance client;NewSentryIssueEvent+ task metadata carrysentry_instance_id. Per-watch throttle gate unchanged./api/v1/sentry/instancesCRUD +/instances/:id/test+/test-connection; browse endpoints require?instanceId=. New codes:SENTRY_INSTANCE_NOT_FOUND(404),SENTRY_INSTANCE_IN_USE(409),SENTRY_INSTANCE_REQUIRED(400).Frontend
instanceId(auto when one instance, selector when many).useSentryAvailable= "≥1 healthy instance".Testing
instanceIdrequirement, event metadata. All green;golangci-lintclean on changed packages.typecheck+lintclean.next dev, driven via Chromium): created two instances (Production SaaS@ sentry.io +Self-hosted Staging@ a self-hosted host), both showing independent "✓ Authenticated" health; confirmed the watcher dialog's instance selector lists both instances and gates org/project. Playwright spec (e2e/tests/integrations/sentry-settings.spec.ts) updated for the two-instance + in-use-delete flows.Migration / compat
Clean cutover:
/config→/instances(FE updated in the same change, no shim). DB + secret migration is idempotent and crash-safe.