Skip to content

SF-3773 Improve onboarding request save speed to fix timeouts #3785

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

Merged
Nateowami merged 1 commit into from
Apr 16, 2026
Merged

SF-3773 Improve onboarding request save speed to fix timeouts #3785

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
200 changes: 125 additions & 75 deletions src/SIL.XForge.Scripture/Controllers/OnboardingRequestRpcController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System.Linq;
using System.Threading.Tasks;
using EdjCase.JsonRpc.Router.Abstractions;
using Microsoft.Extensions.DependencyInjection;
using MongoDB.Bson;
using SIL.XForge.Controllers;
using SIL.XForge.DataAccess;
Expand All @@ -17,25 +18,23 @@
namespace SIL.XForge.Scripture.Controllers;

/// <summary>
/// This controller handles drafting signup form submissions.
/// This controller handles onboarding request form submissions.
/// </summary>
public class OnboardingRequestRpcController(
IExceptionHandler exceptionHandler,
IUserAccessor userAccessor,
IRepository<OnboardingRequest> onboardingRequestRepository,
IUserService userService,
IRealtimeService realtimeService,
ISFProjectService projectService,
IEmailService emailService,
IHttpRequestAccessor httpRequestAccessor
IHttpRequestAccessor httpRequestAccessor,
IServiceScopeFactory serviceScopeFactory
) : RpcControllerBase(userAccessor, exceptionHandler)
{
private readonly IExceptionHandler _exceptionHandler = exceptionHandler;
private readonly IRealtimeService _realtimeService = realtimeService;
private readonly ISFProjectService _projectService = projectService;
private readonly IServiceScopeFactory _serviceScopeFactory = serviceScopeFactory;

/// <summary>
/// Submits a drafting signup request for the specified project.
/// Submits an onboarding request for the specified project.
/// The user must be on the project and have a Paratext role.
/// This stores the request, attempts to notify Serval admins by email, and connects any Paratext projects or DBL
/// resources that were listed in the form that aren't already connected.
Expand All @@ -56,13 +55,16 @@
return ForbiddenError();
}

string submittingUserId = UserId;
Uri siteRoot = httpRequestAccessor.SiteRoot;

var request = new OnboardingRequest
{
Id = ObjectId.GenerateNewId().ToString(),
Submission = new OnboardingSubmission
{
ProjectId = projectId,
UserId = UserId,
UserId = submittingUserId,
Timestamp = DateTime.UtcNow,
FormData = formData,
},
Expand All @@ -71,109 +73,157 @@

await onboardingRequestRepository.InsertAsync(request);

// Email notification to Serval admins
try
_ = Task.Run(async () =>
{
// Query for assignees in drafting signup requests, filter out duplicates, and then look up their email
var adminUserIds = onboardingRequestRepository
.Query()
.Where(r => !string.IsNullOrEmpty(r.AssigneeId))
.Select(r => r.AssigneeId)
.Distinct()
.ToList();

await using IConnection conn = await _realtimeService.ConnectAsync(UserId);
var adminEmails = (await conn.GetAndFetchDocsAsync<User>(adminUserIds)).Select(u => u.Data.Email);

// Send email to each admin using EmailService
string userName = await userService.GetUsernameFromUserId(UserId, UserId);
string subject = $"Onboarding request for {projectDoc.ShortName}";
string link = $"{httpRequestAccessor.SiteRoot}/serval-administration/onboarding-requests/{request.Id}";
string body =
$@"
<p>A new drafting signup request has been submitted for the project <strong>{projectDoc.ShortName} - {projectDoc.Name}</strong>.</p>
using IServiceScope scope = _serviceScopeFactory.CreateScope();
await SendOnboardingRequestEmailsAsync(request, submittingUserId, projectDoc, siteRoot, scope);
await SyncReferencedProjectsAsync(projectId, submittingUserId, formData, scope);
});
Comment on lines +76 to +81
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot Apr 13, 2026

Choose a reason for hiding this comment

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

🔴 Fire-and-forget Task.Run failure in SendOnboardingRequestEmailsAsync silently prevents SyncReferencedProjectsAsync from running

The Task.Run lambda at lines 76-81 calls SendOnboardingRequestEmailsAsync and then SyncReferencedProjectsAsync sequentially, with no top-level try-catch. In SendOnboardingRequestEmailsAsync, the service resolution at lines 107-112 (GetRequiredService calls) happens outside the method's try-catch block (which starts at line 114). If any service resolution fails, the exception propagates out of the method, preventing SyncReferencedProjectsAsync from executing and causing the exception to be silently lost (the task is discarded with _ = Task.Run(...)). This is a regression from the old code, where the email-sending try-catch (old lines 75-115) was independent of the project-syncing loop — email failures could not prevent project syncing. The same issue exists in SyncReferencedProjectsAsync where service resolution at lines 177-178 is outside the per-iteration try-catch at line 181.

Prompt for agents 
The Task.Run lambda at lines 76-81 needs a top-level try-catch to ensure that (1) an exception in SendOnboardingRequestEmailsAsync cannot prevent SyncReferencedProjectsAsync from running, and (2) any unhandled exception is properly reported rather than silently lost.

There are two complementary approaches:

1. Wrap the Task.Run body in a top-level try-catch that calls _exceptionHandler.ReportException. This ensures no exception is ever silently swallowed.

2. Make the two operations independent by calling each in its own try-catch within the Task.Run lambda. This preserves the old behavior where email failures don't prevent project syncing.

Additionally, move the GetRequiredService calls inside the try-catch blocks of SendOnboardingRequestEmailsAsync (lines 107-112) and SyncReferencedProjectsAsync (lines 177-178) so that service resolution failures are caught by the existing error handling rather than escaping the methods.

The fix in the lambda might look like:

_ = Task.Run(async () =>
{
    try
    {
        using IServiceScope scope = _serviceScopeFactory.CreateScope();
        await SendOnboardingRequestEmailsAsync(request, submittingUserId, projectDoc, siteRoot, scope);
        await SyncReferencedProjectsAsync(projectId, submittingUserId, formData, scope);
    }
    catch (Exception ex)
    {
        _exceptionHandler.ReportException(ex);
    }
});

Or better yet, ensure the two operations are independent by catching each separately within the lambda.
Open in Devin Review

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

Comment thread
devin-ai-integration[bot] marked this conversation as resolved.

return Ok(request.Id);
}
catch (Exception)
{
_exceptionHandler.RecordEndpointInfoForException(
new Dictionary<string, string>
{
{ "method", "SubmitOnboardingRequest" },
{ "projectId", projectId },
{ "userId", UserId },
}
);
throw;
}
}

private static async Task SendOnboardingRequestEmailsAsync(
OnboardingRequest request,
string userId,
SFProject projectDoc,
Uri siteRoot,
IServiceScope scope
)
{
var scopedOnboardingRequestRepository = scope.ServiceProvider.GetRequiredService<
IRepository<OnboardingRequest>
>();
var scopedRealtimeService = scope.ServiceProvider.GetRequiredService<IRealtimeService>();
var scopedUserService = scope.ServiceProvider.GetRequiredService<IUserService>();
var scopedEmailService = scope.ServiceProvider.GetRequiredService<IEmailService>();
var scopedExceptionHandler = scope.ServiceProvider.GetRequiredService<IExceptionHandler>();

try
{
var adminUserIds = scopedOnboardingRequestRepository
.Query()
.Where(r => !string.IsNullOrEmpty(r.AssigneeId))
.Select(r => r.AssigneeId)
.Distinct()
.ToList();

await using IConnection conn = await scopedRealtimeService.ConnectAsync();
var adminEmails = (await conn.GetAndFetchDocsAsync<User>(adminUserIds)).Select(u => u.Data.Email);

string userName = await scopedUserService.GetUsernameFromUserId(userId, userId);
string subject = $"Onboarding request for {projectDoc.ShortName}";
string link = $"{siteRoot}/serval-administration/onboarding-requests/{request.Id}";
string body =
$@"
<p>A new onboarding request has been submitted for the project <strong>{projectDoc.ShortName} - {projectDoc.Name}</strong>.</p>
<p><strong>Submitted by:</strong> {userName}</p>
<p><strong>Submission Time:</strong> {request.Submission.Timestamp:u}</p>
<p>The request can be viewed at <a href=""{link}"">{link}</a></p>
";
foreach (var email in adminEmails)
{
await emailService.SendEmailAsync(email, subject, body);
}
}
catch (Exception exception)
foreach (string email in adminEmails)
{
_exceptionHandler.RecordEndpointInfoForException(
new Dictionary<string, string>
{
{ "method", "SubmitOnboardingRequest" },
{ "projectId", projectId },
{ "userId", UserId },
}
);
// report the exception without failing the whole request
_exceptionHandler.ReportException(exception);
await scopedEmailService.SendEmailAsync(email, subject, body);
}
}
catch (Exception exception)
{
scopedExceptionHandler.RecordEndpointInfoForException(
new Dictionary<string, string>
{
{ "method", "SendOnboardingRequestEmailsAsync" },
{ "userId", userId },
{ "requestId", request.Id },
}
);
scopedExceptionHandler.ReportException(exception);
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
Comment thread
github-advanced-security[bot] marked this conversation as resolved.
Fixed
Comment thread
pmachapman marked this conversation as resolved.
Dismissed
}

// Find all the paratext project ids in the sign up request
// Start by collecting them into a set
var paratextProjectIds = new List<string>
{
formData.sourceProjectA,
formData.sourceProjectB,
formData.sourceProjectC,
formData.DraftingSourceProject,
formData.BackTranslationProject,
}
.Where(id => !string.IsNullOrEmpty(id))
.Distinct();
private static async Task SyncReferencedProjectsAsync(
string projectId,
string userId,
OnboardingRequestFormData formData,
IServiceScope scope
)
{
var scopedRealtimeService = scope.ServiceProvider.GetRequiredService<IRealtimeService>();
var scopedProjectService = scope.ServiceProvider.GetRequiredService<ISFProjectService>();
var scopedExceptionHandler = scope.ServiceProvider.GetRequiredService<IExceptionHandler>();

try
{
// Find all the paratext project ids in the sign up request.
List<string> paratextProjectIds =
[
.. new List<string>
{
formData.sourceProjectA,
formData.sourceProjectB,
formData.sourceProjectC,
formData.DraftingSourceProject,
formData.BackTranslationProject,
}
.Where(id => !string.IsNullOrEmpty(id))
.Distinct(),
];

// Connect each Paratext project that isn't already connected by creating resource projects
foreach (string paratextId in paratextProjectIds)
{
// Check if project already exists
SFProject existingProject = _realtimeService
SFProject existingProject = scopedRealtimeService
.QuerySnapshots<SFProject>()
.FirstOrDefault(p => p.ParatextId == paratextId);

if (existingProject is null)
{
// Create the resource/source project and add the user to it
string sourceProjectId = await _projectService.CreateResourceProjectAsync(
UserId,
string sourceProjectId = await scopedProjectService.CreateResourceProjectAsync(
userId,
paratextId,
addUser: true
);

// Sync the newly created project to get its data
await _projectService.SyncAsync(UserId, sourceProjectId);
await scopedProjectService.SyncAsync(userId, sourceProjectId);
}
else if (existingProject.Id == projectId)
{
// Verify that the source project is not the same as the target project
return InvalidParamsError("Source project cannot be the same as the target project");
// Skip syncing if the source project is the same as the target project.
continue;
}
Comment on lines 203 to 207
Copy link
Copy Markdown

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

Choose a reason for hiding this comment

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

🚩 Behavior change: source==target project error now silently skipped

The old code returned InvalidParamsError("Source project cannot be the same as the target project") when a referenced Paratext project resolved to the same SF project as the target. The new code at line 200-201 simply does continue, silently skipping the project. This is a deliberate design change since the background task can't return RPC errors to the caller, but it means invalid form data (listing the target project as a source) is no longer validated. If this validation is important, it should be performed before the Task.Run block, while the request can still return an error.

Open in Devin Review

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

else if (existingProject.Sync.LastSyncSuccessful == false)
{
// If the project exists but last sync failed, retry the sync
await _projectService.SyncAsync(UserId, existingProject.Id);
await scopedProjectService.SyncAsync(userId, existingProject.Id);
}
}

return Ok(request.Id);
}
catch (Exception)
catch (Exception exception)
{
_exceptionHandler.RecordEndpointInfoForException(
scopedExceptionHandler.RecordEndpointInfoForException(
new Dictionary<string, string>
{
{ "method", "SubmitOnboardingRequest" },
{ "method", "SyncReferencedProjectsAsync" },
{ "projectId", projectId },
{ "userId", UserId },
{ "userId", userId },
}
);
throw;
scopedExceptionHandler.ReportException(exception);
}

Check notice

Code scanning / CodeQL

Generic catch clause Note

Generic catch clause.
Comment on lines +167 to 226
Copy link
Copy Markdown

@devin-ai-integration devin-ai-integration Bot Apr 15, 2026

Choose a reason for hiding this comment

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

🟡 Single catch around entire foreach silently skips remaining projects on first sync failure

In SyncReferencedProjectsAsync, the foreach loop iterating over paratextProjectIds is wrapped in a single try-catch at lines 167-226. If CreateResourceProjectAsync or SyncAsync throws an exception for one project (e.g., project A), the catch block at line 215 catches it, reports it to Bugsnag, and returns — all subsequent projects (B, C, etc.) are silently never processed.

This is worse than the old code because previously (OnboardingRequestRpcController.cs:163-164 in old code) the exception would propagate to the outer catch and the user would receive an error response, prompting them to retry. Now, since this runs in a fire-and-forget Task.Run (line 76), the user receives a success response (Ok(request.Id) at line 83) and has no indication that some referenced projects were never synced. The per-iteration try-catch pattern would be more appropriate here to ensure all projects are attempted independently.

Prompt for agents 
In SyncReferencedProjectsAsync (lines 156-227 in OnboardingRequestRpcController.cs), the try-catch wraps the entire foreach loop over paratextProjectIds. If any single project sync fails with an exception, all remaining projects are skipped silently. Since this now runs in a fire-and-forget background task (the user already received a success response), each iteration of the foreach should have its own try-catch so that one project failure does not prevent processing of the remaining projects. Move the try-catch to be inside the foreach loop body, keeping the exception reporting logic for each individual project failure.
Open in Devin Review

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

}

/// <summary>
Expand Down Expand Up @@ -396,7 +446,7 @@
}

/// <summary>
/// Adds a comment to a drafting signup request.
/// Adds a comment to an onboarding request.
/// Only accessible to Serval admins.
/// </summary>
public async Task<IRpcMethodResult> AddComment(string requestId, string commentText)
Expand All @@ -413,7 +463,7 @@

if (request == null)
{
return NotFoundError("Drafting signup request not found");
return NotFoundError("Onboarding request not found");
}

// Create new comment
Expand Down Expand Up @@ -447,7 +497,7 @@
}

/// <summary>
/// Deletes a drafting signup request from the database.
/// Deletes an onboarding request from the database.
/// Only accessible to Serval admins.
/// </summary>
public async Task<IRpcMethodResult> DeleteRequest(string requestId)
Expand All @@ -464,7 +514,7 @@

if (deletedRequest == null)
{
return NotFoundError("Drafting signup request not found");
return NotFoundError("Onboarding request not found");
}

return Ok(true);
Expand Down
Loading