Skip to content

camel-salesforce: Improve error resilience in subscription failure handling#22840

Merged
davsclaus merged 2 commits intoapache:mainfrom
shaipan:salesforce-improve-error-resilience
Apr 30, 2026
Merged

camel-salesforce: Improve error resilience in subscription failure handling#22840
davsclaus merged 2 commits intoapache:mainfrom
shaipan:salesforce-improve-error-resilience

Conversation

@shaipan
Copy link
Copy Markdown
Contributor

@shaipan shaipan commented Apr 29, 2026

Improve error handling in SubscriptionHelper to recover from more transient Salesforce
Streaming API failures instead of aborting the subscription.

  • Treat IOException, TransportException, and TimeoutException as retryable temporary errors
  • Retry with backoff on authentication (401), authorization (403), security policy denial,
    org daily event limit, and missing error message failures
  • Handle invalid channel errors (400) by removing the channel from the subscription list
    instead of retrying indefinitely

@shaipan shaipan force-pushed the salesforce-improve-error-resilience branch from c75c2ec to d416819 Compare April 29, 2026 14:33
@davsclaus
Copy link
Copy Markdown
Contributor

@jeremyross just in case you have a bit time to quickly take a look

@github-actions
Copy link
Copy Markdown
Contributor

🌟 Thank you for your contribution to the Apache Camel project! 🌟
🤖 CI automation will test this PR automatically.

🐫 Apache Camel Committers, please review the following items:

  • First-time contributors require MANUAL approval for the GitHub Actions to run
  • You can use the command /component-test (camel-)component-name1 (camel-)component-name2.. to request a test from the test bot although they are normally detected and executed by CI.
  • You can label PRs using skip-tests and test-dependents to fine-tune the checks executed by this PR.
  • Build and test logs are available in the summary page. Only Apache Camel committers have access to the summary.

⚠️ Be careful when sharing logs. Review their contents before sharing them publicly.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Apr 29, 2026

🧪 CI tested the following changed modules:

  • components/camel-salesforce/camel-salesforce-component
All tested modules (10 modules)
  • Camel :: JBang :: MCP
  • Camel :: JBang :: Plugin :: Route Parser
  • Camel :: JBang :: Plugin :: TUI
  • Camel :: JBang :: Plugin :: Validate
  • Camel :: Launcher :: Container
  • Camel :: Salesforce
  • Camel :: Salesforce :: CodeGen
  • Camel :: Salesforce :: Maven Plugin
  • Camel :: YAML DSL :: Validator
  • Camel :: YAML DSL :: Validator Maven Plugin

⚙️ View full build and test results

Copy link
Copy Markdown
Contributor

@gnodet gnodet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving the error resilience in the Salesforce Streaming subscription handling, @shaipan! This addresses a real operational pain point. A few observations:

Missing tests: Per project standards, every PR should include tests for new behavior. The new error handling paths in subscriptionFailed() and the extended isTemporaryError() method are not covered by the existing SubscriptionHelperTest. Unit tests verifying that IOException, TransportException, and TimeoutException are treated as temporary, and that the various 401/403 error strings trigger retry, would strengthen confidence in these changes.

401/403 retry without token refresh: Treating AUTHENTICATION_INVALID (401) and authorization errors (403) as retryable is reasonable for transient failures. However, the retry path re-calls subscribe() without explicitly triggering a session token refresh. If the token is genuinely expired, all retries may fail identically, consuming the full backoff budget before aborting. Worth confirming that the CometD handshake/subscribe path handles token refresh internally.

Minor: Two comments explaining the non-obvious Tasks.foregroundTask() usage (using initialDelay as a sleep and maxIterations(1) to run once) were removed. The pattern isn't self-documenting — consider keeping them.

Claude Code on behalf of Guillaume Nodet

@shaipan
Copy link
Copy Markdown
Contributor Author

shaipan commented Apr 30, 2026

Thanks for improving the error resilience in the Salesforce Streaming subscription handling, @shaipan! This addresses a real operational pain point. A few observations:

Missing tests: Per project standards, every PR should include tests for new behavior. The new error handling paths in subscriptionFailed() and the extended isTemporaryError() method are not covered by the existing SubscriptionHelperTest. Unit tests verifying that IOException, TransportException, and TimeoutException are treated as temporary, and that the various 401/403 error strings trigger retry, would strengthen confidence in these changes.

401/403 retry without token refresh: Treating AUTHENTICATION_INVALID (401) and authorization errors (403) as retryable is reasonable for transient failures. However, the retry path re-calls subscribe() without explicitly triggering a session token refresh. If the token is genuinely expired, all retries may fail identically, consuming the full backoff budget before aborting. Worth confirming that the CometD handshake/subscribe path handles token refresh internally.

Minor: Two comments explaining the non-obvious Tasks.foregroundTask() usage (using initialDelay as a sleep and maxIterations(1) to run once) were removed. The pattern isn't self-documenting — consider keeping them.

Claude Code on behalf of Guillaume Nodet

@gnodet The methods are private and the existing test pattern in the file doesn't cover listener callbacks.
Reverted the comments that were removed in earlier PR.

@davsclaus
Copy link
Copy Markdown
Contributor

Thanks @shaipan

@shaipan
Copy link
Copy Markdown
Contributor Author

shaipan commented Apr 30, 2026

Hi @gnodet @davsclaus are we good to commit this? Or any more chanegs needed from my side. I have 2 3 more bugs in this area to commit.

@davsclaus davsclaus merged commit faeb2c2 into apache:main Apr 30, 2026
6 checks passed
@davsclaus
Copy link
Copy Markdown
Contributor

I created a ticket to capture your PRs in CAMEL-23391 as we need this for the release notes to track notice changes in this component

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants