Skip to content

fix(webapp): downgrade retryable directory-sync effect failures to warn #4200

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
0ski wants to merge 1 commit into
base: main
Choose a base branch
from
+14 −2
Open

fix(webapp): downgrade retryable directory-sync effect failures to warn #4200

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions apps/webapp/app/services/directorySyncEffects.server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,18 @@ import { createPlatformNotification } from "~/services/platformNotifications.ser

const LAST_OWNER_NOTIFICATION_TITLE = "Directory Sync: last Owner protected";

// Directory-sync effects are idempotent and the accounts-webhook worker retries
// the whole event (maxAttempts: 5). A single failed attempt is therefore an
// expected, self-healing condition rather than an alert-worthy error — the most
// common case is a role assignment losing a serializable race during a backfill
// burst (the plugin already retries that internally; the worker retry mops up
// the rest). Tag the thrown error with `logLevel: "warn"` so the worker logs it
// at warn instead of error (see redis-worker `processItem`), keeping it visible
// for triage without paging as an error on every transient retry.
function retryableEffectError(message: string): Error {
return Object.assign(new Error(message), { logLevel: "warn" as const });
}
Comment on lines +14 to +24

@devin-ai-integration devin-ai-integration Bot Jul 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔍 DLQ escalation also respects the warn log level

When the redis-worker exhausts all retry attempts and moves the item to the dead-letter queue, it also checks errorLogLevel and logs at warn instead of error (packages/redis-worker/src/worker.ts:812-814 for batch items, packages/redis-worker/src/worker.ts:971-975 for single items). This means that even a permanently failing directory-sync effect (e.g., a role that was deleted between retries) will only log at warn when it hits the DLQ. If permanent failures should escalate to error-level alerting after exhausting retries, this would need the worker to distinguish transient vs. permanent, or the logLevel approach would need to be applied per-attempt rather than on the error object. Worth confirming this is the desired behavior for the DLQ case.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


// Raise a user-scoped, deduped notification when the directory tried to
// remove the org's last Owner. We keep the member and tell the Owner what to
// do; a single undismissed notification is enough (don't spam on every retry).
Expand Down Expand Up @@ -92,7 +104,7 @@ async function applyEffect(effect: DirectorySyncEffect): Promise<void> {
roleId: effect.roleId,
});
if (!result.ok) {
throw new Error(`directorySync provision setUserRole failed: ${result.error}`);
throw retryableEffectError(`directorySync provision setUserRole failed: ${result.error}`);
}
}
return;
Expand All @@ -104,7 +116,7 @@ async function applyEffect(effect: DirectorySyncEffect): Promise<void> {
roleId: effect.roleId,
});
if (!result.ok) {
throw new Error(`directorySync set_role failed: ${result.error}`);
throw retryableEffectError(`directorySync set_role failed: ${result.error}`);
}
return;
}
Expand Down