fix(api): surface subscription deletion errors to users - #11
Conversation
Previously, when rebuilding a trigger subscription, errors from the unsubscribe operation were silently caught and logged without propagating to the user. This left users unaware of failures during subscription management. Changes: - Check UnsubscribeResult.success and raise ValueError with the error message when unsubscribe fails - Simplify the rebuild logic by removing unnecessary try/except wrapper - Refactor update API to use cleaner conditional logic - Remove redundant test cases that tested silent error handling
… TriggerSubscriptionUpdateRequest - Introduced a model validator in TriggerSubscriptionUpdateRequest to enforce that at least one of the fields (name, credentials, parameters, properties) must be provided. - Refactored the TriggerSubscriptionUpdateApi to use the validated request object and simplified the logic for updating subscriptions based on the credential type. - Updated the credential type check in TriggerProviderService to use a set for better performance and clarity.
…i and TriggerProviderService
…ptionUpdateApi and TriggerProviderService
…oviderService - Deleted the test for rebuilding trigger subscriptions with unsupported credential types, as it was deemed unnecessary. - This change helps streamline the test suite by focusing on relevant scenarios.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 3 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| endpoint=generate_plugin_trigger_endpoint_url(subscription.endpoint_id), | ||
| parameters=parameters, | ||
| credentials=credentials, | ||
| credential_type=credential_type, |
There was a problem hiding this comment.
HIDDEN_VALUE credentials sent to third-party APIs
High Severity
The refactored rebuild_trigger_subscription removes the HIDDEN_VALUE credential merging logic. When users update subscriptions, frontends typically send HIDDEN_VALUE (like [__HIDDEN__]) for unchanged sensitive fields. The old code replaced these with actual stored credentials before calling external APIs. Now, the literal HIDDEN_VALUE string is passed directly to TriggerManager.subscribe_trigger(), causing third-party API calls (e.g., GitHub webhook creation) to receive invalid credentials and fail. The tests still expect this merging behavior, confirming it's required functionality.
Additional Locations (1)
| credentials=request.credentials or subscription.credentials, | ||
| parameters=request.parameters or subscription.parameters, | ||
| ) | ||
| return 200 |
There was a problem hiding this comment.
UNAUTHORIZED subscriptions cannot update properties anymore
Medium Severity
The manually_created variable is computed but never used. The old code had a separate branch for CredentialType.UNAUTHORIZED subscriptions that allowed updating name and properties without rebuilding. The new code only handles the "rename only" case. When an UNAUTHORIZED subscription tries to update properties (with or without name), it falls through to rebuild_trigger_subscription, which rejects UNAUTHORIZED credential types with an error. This breaks property updates for manually-created subscriptions.
| credentials=credentials, | ||
| properties=new_subscription.properties, | ||
| expires_at=new_subscription.expires_at, | ||
| ) |
There was a problem hiding this comment.
Concurrent rebuilds cause race conditions without distributed lock
Medium Severity
The refactored rebuild_trigger_subscription removes the distributed lock (redis_client.lock with key trigger_subscription_rebuild_lock:{tenant_id}_{subscription_id}) that protected against concurrent operations. Other methods in the same service like add_trigger_subscription and update_trigger_subscription still use Redis locks. Without the lock, concurrent rebuild requests on the same subscription can cause duplicate webhook registrations at third-party services, or one request may fail confusingly because the other already unsubscribed the webhook.
2 participants
Benchmark PR from qodo-benchmark#181
Note
Streamlines trigger subscription updates and rebuilds with stricter validation and a simpler service workflow.
@model_validatortoTriggerSubscriptionUpdateRequestrequiring at least one ofname,credentials,parameters, orproperties.credentials/parameterswhen omitted).unsubscribethensubscribe, then persist viaupdate_trigger_subscription; no credential merging or error suppression onunsubscribe.unsubscribeerrors, provider-not-found, and unsupported credential type; retains/updates rebuild and name uniqueness coverage.Written by Cursor Bugbot for commit 5489175. Configure here.