🦋 Changeset detected

Latest commit: d7eae64

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

No actionable comments were generated in the recent review. 🎉

Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).

Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.

Walkthrough

TracingSDK.flush() and TracingSDK.shutdown() now call providers' forceFlush()/shutdown() using Promise.allSettled rather than Promise.all. Rejected provider results are collected (non-Error reasons normalized to Error) and, if any fail, an AggregateError containing all provider errors is thrown. A Changeset entry was added documenting the patch release for @trigger.dev/core.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

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 help to get the list of available commands and usage tips.}

@claude can you address this review:

With Promise.allSettled, flush() and shutdown() now always resolve (never reject), even when individual providers fail. This changes the public API contract. The callers in managed-run-worker.ts:725-738 and dev-run-worker.ts:724-737 (flushTracingSDK) wrap _tracingSDK?.flush() in Promise.race inside an outer Promise.allSettled (managed-run-worker.ts:673, dev-run-worker.ts:687), which tracks success/failure via successfulFlushes/failedFlushes. After this PR, flushTracingSDK will always resolve successfully, so "tracingSDK" will always appear in successfulFlushes even if underlying providers failed. The failure tracking at the flushAll level becomes less precise for tracing SDK failures specifically — though the individual provider errors are still logged to console by the new code. This is arguably the right tradeoff (partial success is better than total failure), but reviewers should be aware the caller-level failure detection for tracing is now effectively bypassed.

Claude finished @ericallam's task in 1m 49s —— View job

Addressing Review Feedback

• Read current tracingSDK.ts implementation
• Read caller code in managed-run-worker.ts and dev-run-worker.ts
• Design solution to preserve failure detection while keeping Promise.allSettled resilience
• Implement changes
• Commit and push

Done. Updated flush() and shutdown() to throw an AggregateError after processing all results if any providers failed. This preserves both:

• Resilience: All providers get a chance to flush/shutdown (via Promise.allSettled)
• Failure detection: Callers can still catch failures for successfulFlushes/failedFlushes tracking

View commit | |

@claude improved this pr based on this review:

Code review — PR #3562 (fix(core): use Promise.allSettled in TracingSDK flush/shutdown)

Verdict

ACCEPT-WITH-NITS — small, well-targeted fix. Rebase before merge.

Important findings

None.

Nits

1. 🟡 Branch is behind main — diff includes an accidental revert of PR fix(core): retry TASK_PROCESS_SIGSEGV under the user's retry policy #3552 (SIGSEGV retry).
   → .changeset/retry-sigsegv.md, packages/core/src/v3/errors.ts:361,398, packages/core/test/errors.test.ts
   → Branch was cut before 8e675a4 landed on main; merging as-is would silently revert SIGSEGV retry behavior, its changeset, and its tests.
   → Rebase onto current main before merging.
2. 🟡 Duplicate failure reporting. Each rejected provider is console.error'd inside flush()/shutdown() and then thrown as part of the AggregateError. Existing callers (e.g. managed-run-worker.ts:690-696) already
   log rejected reasons, so each failure prints twice.
   → packages/core/src/v3/otel/tracingSDK.ts:381,400
   → Drop the per-provider console.error and rely on the AggregateError carrying the cause, or accept the duplication.
3. 🟡 AggregateError.message shadows per-provider detail. Existing caller at managed-run-worker.ts:692 logs result.reason (the AggregateError), which surfaces only the generic prefix unless .errors is inspected.
   → packages/core/src/v3/otel/tracingSDK.ts:386-388,405-407
   → Include provider names in the aggregate message (e.g. One or more providers failed to flush: ${failedNames.join(", ")}) so the existing log line stays informative.

Strengths

• Success-path behavior is identical to before — no extra logs, no spurious throw, same return.
• Provider-name array order correctly matches Promise.allSettled input order, so error logs identify the right exporter.
• Failure semantics preserved via AggregateError — callers that previously detected failure via a thrown rejection still do.
• AggregateError is safe for this package (target: es2022, engines.node >=18.20.0).
• Changeset present, correctly scoped to @trigger.dev/core as patch.

Claude finished @ericallam's task in 1m 55s —— View job

Addressing Review Feedback

• Rebase branch onto current main Skipping rebase (too many conflicts from diverged history). Should be done manually before merge.
• Remove duplicate console.error logging (rely on AggregateError)
• Improve AggregateError message to include failed provider names
• Push changes

Done. Pushed d7eae64 with the following changes:

1. Removed the per-provider console.error calls — the AggregateError now carries all failure information
2. Updated AggregateError messages to include which providers failed (e.g., "One or more providers failed to flush: trace, log")

Note on rebase: The branch has significant conflicts with main (many files across workflows, webapp components, run-engine, etc.). This should be rebased manually before merging to avoid reverting PR #3552 (SIGSEGV retry).
|