Don't return an error on already present #1623
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| so.teamSandboxesCreated.Add(ctx, 1, metric.WithAttributes(attributes...)) | ||
| func (so *TeamObserver) Add(ctx context.Context, teamID uuid.UUID) { | ||
| attributes := []attribute.KeyValue{ | ||
| attribute.String("team_id", teamID.String()), |
There was a problem hiding this comment.
I can't do it, because that changes it to team.id, that isn't compatible. Bless the tests 🙇🏻
There was a problem hiding this comment.
hmm, then one of the places is incorrect
| callback(ctx, sandbox, newlyCreated) | ||
| } | ||
| // Add sandbox info to node for routing | ||
| s.callbacks.AddToSandboxRoutingTable(ctx, sandbox) |
There was a problem hiding this comment.
this will be called twice for the race condition, is it idempotent correctly?
There was a problem hiding this comment.
Doesn't make sense to call it multiple times. Changed it, so we're calling it only on the first insert
jakubno
force-pushed
the
fix-sandbox-already-added
branch
from
433ae5b to
2319697
Compare
There's a race condition when the sandbox is added from sync and then the create fails
Note
Swallows ErrAlreadyExists on sandbox add and refactors insert flow into structured sync/async callbacks, updating orchestrator/metrics and adding comprehensive tests.
Callbacksstruct withAddSandboxToRoutingTable(sync),AsyncSandboxCounter, andAsyncNewlyCreatedSandbox(async) hooks.Addnow ignoresErrAlreadyExists(logs warning), calls routing-table + counter only on first insert, and triggers newly-created hook based onnewlyCreatedflag.InsertCallbacksignature (dropscreatedarg);SyncusesAdd(..., false).Addreturnssandbox.ErrAlreadyExistsinstead of string error.ErrAlreadyExiststosandbox/errors.go.addSandboxToRoutingTable,sandboxCounterInsert, andhandleNewlyCreatedSandbox(sends analytics, updates team metrics, increments created counter).TeamObserver.AddAPI simplified toAdd(ctx, teamID)and always incrementsteam_sandboxes_createdwithteam_id.store_test.gocovering new callback behavior, already-existing cases, storage error propagation, and high-concurrency scenarios.Written by Cursor Bugbot for commit 3a890d8. This will update automatically on new commits. Configure here.