Fixed retrying on run replication inserts on connection errors #2216
Conversation
|
WalkthroughThe changes implement a retry mechanism with exponential backoff and jitter for insert operations in the ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/app/services/runsReplicationService.server.ts (1)
522-522: Fix typo in log messageThe log message should match the service name for consistency when searching logs.
Apply this diff to fix the typo:
- this.logger.warn(`Retrying RunReplication insert due to connection error`, { + this.logger.warn(`Retrying RunsReplication insert due to connection error`, {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/services/runsReplicationService.server.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`apps/webapp/**/*.ts`: In the webapp, all environment variables must be accessed through the `env` export of `env.server.ts`, instead of directly accessing `process.env`.
apps/webapp/**/*.ts: In the webapp, all environment variables must be accessed through theenvexport ofenv.server.ts, instead of directly accessingprocess.env.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/app/services/runsReplicationService.server.ts
`apps/webapp/**/*.{ts,tsx}`: When importing from `@trigger.dev/core` in the weba...
apps/webapp/**/*.{ts,tsx}: When importing from@trigger.dev/corein the webapp, never import from the root@trigger.dev/corepath; always use one of the subpath exports as defined in the package's package.json.
📄 Source: CodeRabbit Inference Engine (.cursor/rules/webapp.mdc)
List of files the instruction was applied to:
apps/webapp/app/services/runsReplicationService.server.ts
`**/*.{ts,tsx}`: Always prefer using isomorphic code like fetch, ReadableStream,...
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
Use strict mode
No default exports, use function declarations
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/app/services/runsReplicationService.server.ts
`{packages/core,apps/webapp}/**/*.{ts,tsx}`: We use zod a lot in packages/core and in the webapp
{packages/core,apps/webapp}/**/*.{ts,tsx}: We use zod a lot in packages/core and in the webapp
📄 Source: CodeRabbit Inference Engine (.github/copilot-instructions.md)
List of files the instruction was applied to:
apps/webapp/app/services/runsReplicationService.server.ts
🧠 Learnings (1)
apps/webapp/app/services/runsReplicationService.server.ts (3)
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When using retry, queue, machine, or maxDuration options, configure them within the `task` function in Trigger.dev task files.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.494Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : If you are able to generate an example payload for a task, do so.
Learnt from: CR
PR: triggerdotdev/trigger.dev#0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-06-30T13:22:21.495Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : When triggering a task from inside another task, use the task instance's `trigger`, `batchTrigger`, `triggerAndWait`, `batchTriggerAndWait`, `batch.trigger`, `batch.triggerAndWait`, `batch.triggerByTask`, or `batch.triggerByTaskAndWait` methods.
⏰ Context from checks skipped due to timeout of 90000ms (25)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (10, 10)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (9, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 10)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 10)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
apps/webapp/app/services/runsReplicationService.server.ts (8)
56-59: Configuration options look good!The retry configuration options are well-designed with sensible defaults and clear naming conventions.
89-92: Good initialization with sensible defaults!The retry configuration defaults (3 retries, 100ms base delay, 2s max delay) provide a good balance between resilience and performance.
Also applies to: 165-168
464-474: Retry logic integration looks good!The implementation correctly wraps both insert operations with retry logic, using clear operation names for logging and proper arrow functions to pass the attempt number.
504-539: Well-implemented retry logic with proper error handling!The retry mechanism correctly handles connection errors with exponential backoff, includes comprehensive logging, and properly breaks on non-retryable errors. The tuple return pattern for error handling is clean and follows best practices.
558-569: Excellent exponential backoff implementation!The retry delay calculation correctly implements exponential backoff with a maximum cap and adds jitter to prevent thundering herd problems. The formula properly starts at the base delay for the first retry.
571-596: Proper error propagation for retry logic!The method correctly throws errors to enable retry handling and includes the attempt number in error logs for better debugging context.
598-623: Consistent error handling across insert methods!The payload insert method correctly mirrors the task run insert pattern with proper error throwing and attempt logging.
541-556: Review of #isRetryableConnectionError: no additional ClickHouse patterns found—please verifyI searched the ClickHouse client code (internal-packages/clickhouse/src/client/client.ts) for connection-related error messages and codes but didn’t uncover any extra patterns.
• If ClickHouseError surfaces specific connection-failure messages or codes, please add them to retryableConnectionPatterns.
• Otherwise, the existing list adequately covers retryable connection errors.
2 participants
No description provided.