fix(app/sandbox): fix marketplace install and billing profile provisioning#4505
fix(app/sandbox): fix marketplace install and billing profile provisioning#4505vsengar-79 wants to merge 3 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces the Sandbox install entry with ChangesSandbox App Installation Flow
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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: 1
🤖 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 `@openmeter/app/sandbox/app.go`:
- Around line 235-237: The code indexes existing.Items[0] after checking
existing.TotalCount > 0 which can still panic if Items is empty; update the
guard in the create/install flow (the block that returns
models.NewGenericConflictError) to verify len(existing.Items) > 0 before
accessing existing.Items[0]; if Items is empty but TotalCount > 0, fall back to
a safe error message (e.g., use existing.GetName() only when present or include
the conflicting app name unknown) so you never index Items[0] without confirming
its length.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a3771f68-24f2-446e-9ab5-d31be412fbe6
📒 Files selected for processing (1)
openmeter/app/sandbox/app.go
|
Sandbox apps are auto-provisioned. It shouldn't be available for installs. |
Hi @tothandras, You're right that it's auto-provisioned, but right now, the marketplace listing still publicly advertises a 'No Credentials' install. The endpoint was just completely broken (returning a 400) because the backend code was written for allowing API key installs, even when the API key wasn't even validated. And the error message was also very confusing stating that this installation method wasn't supported, while listing the same installation method in the 'Supported methods'. Keeping your opinion in mind, I already wrote a check to stop the app from being installed when it already is (which is the case for 99.9% percent of instances). This is basically just implementing dead code and fixing confusing mess. If we shouldn't allow manual installs at all, should I change this PR to remove the installation capability from the sandbox marketplace listing entirely? This PR was made to keep in mind that the Sandbox app is provisioned for the set default namespace. If a user wrote some code to allow dynamic namespace support, this would just make it easy for them too to initiate the Sandbox app for subscriptions/features. And this would make no negative effect on already existing instances due to the duplicate check. Currently, multiple sandbox apps are being allowed to install which isn't useful for anything. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // Sandbox is a singleton per namespace — only one instance makes sense since all | ||
| // instances are functionally identical (no credentials, no external state). | ||
| existing, err := a.appService.ListApps(ctx, app.ListAppInput{ | ||
| Namespace: input.Namespace, | ||
| Type: lo.ToPtr(app.AppTypeSandbox), | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to list sandbox apps: %w", err) | ||
| } | ||
|
|
||
| if existing.TotalCount > 0 { | ||
| return nil, models.NewGenericConflictError(fmt.Errorf("sandbox app: %s already exists", existing.Items[0].GetName())) | ||
| } |
There was a problem hiding this comment.
The list-then-create pattern is not atomic: two concurrent InstallApp calls that both arrive when no sandbox app exists will both pass the TotalCount > 0 guard and each call CreateApp, producing two sandbox instances despite the comment stating this is a singleton. Because the check and creation are separate DB operations with no unique constraint or advisory lock, the race window is small but real. A database-level unique partial index on (namespace, type) — or wrapping the check and insert in the same transaction — would make the guarantee reliable.
|
@turip, should I do anything about the failing checks or can you merge it regardless? |
Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Implemented sandbox billing profile creation by provisioning a default billing profile and returning relevant app capabilities. Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
Add singleton check for sandbox app creation to prevent duplicates. Signed-off-by: vsengar-79 <146079793+vsengar-79@users.noreply.github.com>
3 participants
Summary
Fix
POST /api/v1/marketplace/listings/sandbox/installreturning 400 "does not support this installation method": the sandboxFactoryadvertisedno_credentials_requiredin its marketplace listing but only implementedAppFactoryInstallWithAPIKey. Added the missingInstallAppmethod and inlined the logic directly, removing the now-redundantInstallAppWithAPIKey.Fix
createBillingProfilebeing a no-op for sandbox: the marketplace install handler had a// TODO: Implement sandbox billing profile creationstub that always returned nil. Replaced it with a call toProvisionDefaultBillingProfile, matching what server startup does for the default namespace.Together these two fixes make
POST /api/v1/marketplace/listings/sandbox/installwith{"createBillingProfile": true}fully self-contained — one call installs the app and provisions the default billing profile.Summary by CodeRabbit