SF-3773 Improve onboarding request save speed to fix timeouts#3785
SF-3773 Improve onboarding request save speed to fix timeouts#3785
Conversation
Nateowami
added
the
will require testing
Nateowami
force-pushed
the
fix/SF-3773-onboarding-request-submission-timeouts
branch
from
239d8ea to
03a4ce4
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
|
||
| string userName = await scopedUserService.GetUsernameFromUserId(userId, userId); | ||
| string subject = $"Onboarding request for {projectDoc.ShortName}"; | ||
| string link = $"{httpRequestAccessor.SiteRoot}/serval-administration/draft-requests/{request.Id}"; |
There was a problem hiding this comment.
🔴 httpRequestAccessor.SiteRoot accessed in background task after HttpContext is disposed
httpRequestAccessor.SiteRoot is accessed at line 126 inside SendOnboardingRequestEmailsAsync, which runs in a fire-and-forget Task.Run (line 75). However, HttpRequestAccessor.SiteRoot reads from httpContextAccessor.HttpContext?.Request (HttpRequestAccessor.cs:18-21), and HttpContext is only valid during the lifetime of the HTTP request. Since the controller returns Ok(request.Id) at line 82 immediately without awaiting the background task, the HTTP request will complete and HttpContext will become null/recycled before or during the background task execution. This will cause SiteRoot to produce a malformed URI (scheme and host will be null) or throw, causing the email notification to fail silently.
The fix is to capture the SiteRoot value before entering Task.Run, while the HTTP context is still alive, and pass the captured value into the background task.
Prompt for agents
The bug is in SubmitOnboardingRequest: httpRequestAccessor.SiteRoot is lazily evaluated inside SendOnboardingRequestEmailsAsync, which runs in a fire-and-forget Task.Run. By the time this code executes, the HTTP request has already completed and HttpContext is no longer available.
Fix approach: In SubmitOnboardingRequest (around line 58, before the Task.Run on line 75), capture the SiteRoot eagerly:
Uri siteRoot = httpRequestAccessor.SiteRoot;
Then pass siteRoot as a parameter to SendOnboardingRequestEmailsAsync instead of accessing httpRequestAccessor inside the background task. Update SendOnboardingRequestEmailsAsync to accept a Uri siteRoot parameter and use it at line 126 instead of httpRequestAccessor.SiteRoot.
Was this helpful? React with 👍 or 👎 to provide feedback.
| 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; | ||
| } |
There was a problem hiding this comment.
🚩 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.
Was this helpful? React with 👍 or 👎 to provide feedback.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| catch (Exception exception) | ||
| { | ||
| _exceptionHandler.RecordEndpointInfoForException( | ||
| new Dictionary<string, string> | ||
| { | ||
| { "method", "SendOnboardingRequestEmailsAsync" }, | ||
| { "userId", userId }, | ||
| { "requestId", request.Id }, | ||
| } | ||
| ); | ||
| _exceptionHandler.ReportException(exception); | ||
| } |
| catch (Exception exception) | ||
| { | ||
| _exceptionHandler.RecordEndpointInfoForException( | ||
| new Dictionary<string, string> | ||
| { | ||
| { "method", "SyncReferencedProjectsAsync" }, | ||
| { "projectId", projectId }, | ||
| { "userId", userId }, | ||
| { "paratextId", paratextId }, | ||
| } | ||
| ); | ||
| _exceptionHandler.ReportException(exception); | ||
| } |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3785 +/- ##
==========================================
- Coverage 81.27% 81.19% -0.08%
==========================================
Files 622 622
Lines 39322 39359 +37
Branches 6391 6410 +19
==========================================
Hits 31958 31958
- Misses 6379 6403 +24
- Partials 985 998 +13 ☔ View full report in Codecov by Sentry. |
2 participants
I believe the reason we are getting duplicate onboarding requests is because the endpoint to submit the form is too slow and sometimes times out. This results in the user re-submitting the form, but by this point the projects are mostly all connected, and the second time it succeeds.
Instead, we should accept the form submission, and queue the email and project sync tasks, as these don't have to succeed for the form submission to be accepted.
This change is