Skip to content

Zendesk sender issues highlighted by UAT #455

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
davidgouge merged 3 commits into from
Sep 16, 2026
Merged

Zendesk sender issues highlighted by UAT #455

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fef98b
Updated Worker logs to use same serilog format as the web app
davidgouge Sep 16, 2026
3cd3198
Fixes for the Zendesk worker to improve resilience / retry and loggin…
davidgouge Sep 16, 2026
1d5a786
Merge branch 'main' into zendesk-sender-issues
davidgouge Sep 16, 2026
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
117 changes: 94 additions & 23 deletions src/DfE.CheckPerformanceData.RulesEngineWorker/Consumers/ZendeskConsumer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,24 +135,49 @@ public override async Task ProcessMessageBodyAsync(string messageBody, Cancellat
// Durably claim the "ticket created" transition before calling Zendesk. The claim flips
// the status out of RulesProcessed in a single atomic UPDATE, so of two concurrent
// deliveries that both saw CrmId == null only one flips a row — the loser gets zero rows
// affected and skips without ever calling Zendesk, so no duplicate ticket is created.
// A redelivery of this worker's own crashed attempt (status already Creating, CrmId
// still null) re-claims and retries rather than stranding the request.
// affected and never calls Zendesk, so no duplicate ticket is created. A failed attempt
// hands its claim back (see the catch below) so its own redelivery can claim afresh.
if (!await TryClaimTicketCreationAsync(message.ReferenceNumber, cancellationToken))
{
return;
// A lost claim is only benign when a ticket now exists (a concurrent delivery won and
// finished). Otherwise the row is stuck in ZendeskTicketCreating — an attempt that died
// mid-call, or another worker still in flight — and returning here would ACK the
// message as handled and lose the request with nothing dead-lettered. Throw instead:
// the queue retries, and a row that stays stuck reaches the DLQ with this reason.
var current = await LoadChangeRequestAsync(message.ReferenceNumber, cancellationToken);
if (!string.IsNullOrEmpty(current?.CrmId))
{
return;
}

throw new InvalidOperationException(
$"Could not claim ticket creation for Reference={message.ReferenceNumber}: " +
$"WorkerStatus={current?.WorkerStatus?.ToString() ?? "null"}, row found={current is not null}.");
}

var decision = DeriveDecision(message, changeRequest);
var ticket = BuildTicket(message, decision);
var response = await _zendeskService.CreateTicketAsync(ticket);

var files = message.Answers
.Where(a => a.Files is not null)
.SelectMany(a => a.Files!);
foreach (var file in files)
CreateTicketResponseDto response;
try
{
await UploadAttachmentToTicketAsync(response.Ticket.Id, message.CheckingWindowId, file, cancellationToken);
response = await _zendeskService.CreateTicketAsync(ticket);

var files = message.Answers
.Where(a => a.Files is not null)
.SelectMany(a => a.Files!);
foreach (var file in files)
{
await UploadAttachmentToTicketAsync(response.Ticket.Id, message.CheckingWindowId, file, cancellationToken);
}
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
// Hand the claim back so the redelivery can take it. Without this the retry finds the
// row in ZendeskTicketCreating, cannot claim it, and the request is lost — every one of
// a 120-request preprod replay went this way after Zendesk returned 422.
await ReleaseTicketCreationClaimAsync(message.ReferenceNumber, cancellationToken);
throw;
}

var crmId = response.Ticket.Id.ToString(CultureInfo.InvariantCulture);
Expand All @@ -174,15 +199,21 @@ await _dbContext.ChangeRequests
response.Ticket.Id, message.ReferenceNumber, decision.Status, decision.MatchedRuleId);
}

// Atomically flips the request into the in-progress "creating" state: a RulesProcessed row
// (rules-consumer path) or an enquiry row — which never passes the rules engine, so its
// WorkerStatus is NULL and it is claimed only when RequestType is ResultsEnquiry (FR-002).
// Returns true to the single delivery that wins the flip. The flip is exclusive: Postgres
// takes a row lock on the matching row, so of two concurrent deliveries the second re-checks
// its WHERE against the winner's committed row — which is no longer claimable — and
// matches zero rows. The loser therefore skips without ever calling Zendesk, so concurrent
// redelivery cannot create a duplicate ticket. CrmId == null keeps an already-ticketed
// request (whose status is ZendeskTicketCreated) from ever being re-claimed.
// Atomically claims the request for ticket creation. Returns true to the single delivery that
// wins. The flip is exclusive: Postgres takes a row lock on the matching row, so of two
// concurrent deliveries the second re-checks its WHERE against the winner's committed row —
// now ZendeskTicketCreating — and matches zero rows, so it never calls Zendesk and no duplicate
// ticket is created. CrmId == null keeps an already-ticketed request from being re-claimed.
//
// Two states are claimable, both meaning "no ticket exists and nobody is creating one":
// - RulesProcessed: the ordinary path, a rules decision has been recorded;
// - null, for a results enquiry only: an enquiry never passes the rules engine (FR-002), so
// its status is NULL by design and DeriveDecision falls back to Scrutiny for it. A null
// AMENDMENT is deliberately not claimable (SC-005): it means the rules decision was never
// recorded, and it must surface on the DLQ rather than be ticketed under a guess.
// ZendeskTicketCreating is deliberately NOT claimable — that is what makes the claim exclusive
// under concurrency — so a failed attempt must hand its claim back (ReleaseTicketCreationClaimAsync)
// for the redelivery to take.
private async Task<bool> TryClaimTicketCreationAsync(string referenceNumber, CancellationToken cancellationToken)
{
var claimed = 0;
Expand All @@ -201,6 +232,33 @@ await _dbContext.ExecuteInTransactionAsync(async () =>
return claimed == 1;
}

// Undoes TryClaimTicketCreationAsync after a failed create so the redelivery can claim again.
// Guarded on CrmId == null and the Creating state so it can never disturb a row that a
// concurrent delivery has since ticketed. Best effort: if this write itself fails, the original
// exception is the one worth surfacing, and the row's stuck state surfaces on the next attempt.
private async Task ReleaseTicketCreationClaimAsync(string referenceNumber, CancellationToken cancellationToken)
{
try
{
await _dbContext.ExecuteInTransactionAsync(async () =>
{
await _dbContext.ChangeRequests
.Where(r => r.ReferenceNumber == referenceNumber
&& r.CrmId == null
&& r.WorkerStatus == WorkerStatus.ZendeskTicketCreating)
.ExecuteUpdateAsync(s => s
.SetProperty(r => r.WorkerStatus, WorkerStatus.RulesProcessed),
cancellationToken);
}, cancellationToken);
}
catch (Exception ex) when (ex is not OperationCanceledException)
{
_logger.LogWarning(ex,
"Could not release the ticket-creation claim for Reference={Reference}; the next attempt will report the stuck row.",
referenceNumber);
}
}

private async Task<ChangeRequest?> LoadChangeRequestAsync(string referenceNumber, CancellationToken cancellationToken)
{
try
Expand Down Expand Up @@ -352,14 +410,27 @@ private void MapPupilFields(CreateTicketRequestDto dto, RequestDocument message)
});
}

// CYPMD_ID is an integer field in Zendesk, and a value that does not parse as one makes
// Zendesk reject the whole ticket (422 "CYPMD ID: is invalid") — which is how a UAT seed
// with "CY0045" ids sank an entire close-exercise replay. Losing the one field is the
// lesser harm: the ticket still carries the UPN, name and DOB to identify the pupil.
var cypmdFieldId = GetConfiguredFieldId(ZendeskTicketFieldConstants.CypmdName);
if (cypmdFieldId.HasValue)
{
dto.Ticket.CustomFields.Add(new CustomFieldDto
if (long.TryParse(message.Pupil.CypmdId, NumberStyles.None, CultureInfo.InvariantCulture, out _))
{
Id = cypmdFieldId.Value,
Value = message.Pupil.CypmdId,
});
dto.Ticket.CustomFields.Add(new CustomFieldDto
{
Id = cypmdFieldId.Value,
Value = message.Pupil.CypmdId,
});
}
else
{
_logger.LogWarning(
"CYPMD id for Reference={Reference} is not an integer; omitting the CYPMD_ID field from the ticket.",
message.ReferenceNumber);
}
}

var upnId = GetConfiguredFieldId(ZendeskTicketFieldConstants.UpnName);
Expand Down
4 changes: 4 additions & 0 deletions ....CheckPerformanceData.RulesEngineWorker/DfE.CheckPerformanceData.RulesEngineWorker.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@
<PackageReference Include="Azure.Storage.Blobs" />
<PackageReference Include="Azure.Storage.Common" />
<PackageReference Include="Microsoft.VisualStudio.Azure.Containers.Tools.Targets" />
<PackageReference Include="Serilog.AspNetCore" />
<PackageReference Include="Serilog.Expressions" />
<PackageReference Include="Serilog.Formatting.Compact" />
<PackageReference Include="Serilog.Sinks.Console" />
</ItemGroup>

<ItemGroup>
Expand Down
153 changes: 87 additions & 66 deletions src/DfE.CheckPerformanceData.RulesEngineWorker/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,81 +17,102 @@
using DfE.CheckPerformanceData.RulesEngineWorker.Zendesk;
using Microsoft.AspNetCore.Builder;
using Microsoft.EntityFrameworkCore;
using Serilog;
using Serilog.Formatting.Compact;

var builder = WebApplication.CreateBuilder(args);
builder.Configuration.AddUserSecrets<Program>();
// Bootstrap logger so a failure before the host is built is still one JSON entry in Logit.
Log.Logger = new LoggerConfiguration()
.WriteTo.Console(new CompactJsonFormatter())
.CreateBootstrapLogger();

builder.Services.Configure<QueueOptions>(builder.Configuration.GetSection("QueueOptions"));
builder.Services.AddScoped<IQueueService, PostgresQueueService>();
builder.Services.AddScoped<IQueueAdminService, QueueAdminService>();
try
{
Log.Information("Starting worker");

builder.Services.AddScoped<ISettingRepository, SettingRepository>();
builder.Services.AddScoped<ISettingService, SettingService>();
var builder = WebApplication.CreateBuilder(args);
builder.Configuration.AddUserSecrets<Program>();
builder.UseCpdSerilog();

builder.Services.AddScoped<IMetricsSink, DbMetricsSink>();
// Search analytics retention: the events sink + the messages service are the two purge
// dependencies of SearchAnalyticsRetentionJob below. Registered sibling to IMetricsSink
// rather than through AddPersistenceDependencies — the worker deliberately opts out of
// the shared registration bundle so its manual DbContext registration (lines below) is
// the single source of truth.
builder.Services.AddScoped<ISearchAnalyticsSink, DbSearchAnalyticsSink>();
builder.Services.AddScoped<ISearchMessageService, DbSearchMessageService>();
builder.Services.Configure<QueueOptions>(builder.Configuration.GetSection("QueueOptions"));
builder.Services.AddScoped<IQueueService, PostgresQueueService>();
builder.Services.AddScoped<IQueueAdminService, QueueAdminService>();

builder.Services.AddSingleton<ICurrentUserService, WorkerCurrentUserService>();
builder.Services.AddDbContext<PortalDbContext>(options =>
options.UseNpgsql(builder.Configuration.GetConnectionString("Postgres"),
sql => sql.EnableRetryOnFailure()));
builder.Services.AddScoped<IPortalDbContext>(sp => sp.GetRequiredService<PortalDbContext>());
builder.Services.AddScoped<ISettingRepository, SettingRepository>();
builder.Services.AddScoped<ISettingService, SettingService>();

// Whether the real Zendesk client is the one that will actually serve requests decides how
// strict its configuration has to be. With the fake selected — the default, so that a fresh dev
// or test environment never pushes to real Zendesk — blank settings are expected rather than an
// error, and demanding them would stop the worker starting in exactly the environment the fake
// exists to serve. The consumers and the retention jobs share this process, so that is all of
// them, over an integration none of them is using.
builder.Services.AddZendeskApiClient(
builder.Configuration,
requireRealClient: !ZendeskServiceRegistration.ShouldUseFake(builder.Configuration));
builder.Services.AddInfrastructureDependencies(builder.Configuration);
builder.Services.AddNotifyService(builder.Configuration);
builder.Services.AddScoped<IMetricsSink, DbMetricsSink>();
// Search analytics retention: the events sink + the messages service are the two purge
// dependencies of SearchAnalyticsRetentionJob below. Registered sibling to IMetricsSink
// rather than through AddPersistenceDependencies — the worker deliberately opts out of
// the shared registration bundle so its manual DbContext registration (lines below) is
// the single source of truth.
builder.Services.AddScoped<ISearchAnalyticsSink, DbSearchAnalyticsSink>();
builder.Services.AddScoped<ISearchMessageService, DbSearchMessageService>();

// When Zendesk:UseFake is set the real Zendesk service is replaced with a fake that captures
// "created" tickets into the shared dev outbox table, so the rules-engine pipeline can be
// driven and observed without a real Zendesk. The flag defaults to true so a fresh dev or test
// environment never pushes to real Zendesk; setting it to false routes to the real client.
// Gated on configuration rather than the environment name because the test site also runs as
// Development.
ZendeskServiceRegistration.ConfigureFakeZendesk(builder.Services, builder.Configuration);
builder.Services.AddSingleton<ICurrentUserService, WorkerCurrentUserService>();
builder.Services.AddDbContext<PortalDbContext>(options =>
options.UseNpgsql(builder.Configuration.GetConnectionString("Postgres"),
sql => sql.EnableRetryOnFailure()));
builder.Services.AddScoped<IPortalDbContext>(sp => sp.GetRequiredService<PortalDbContext>());

// The worker only needs the rules-engine pieces from the Application layer, not the
// portal's full service graph (which depends on web-only collaborators).
builder.Services.AddSingleton<IRulesEngine, RulesEngine>();
builder.Services.AddSingleton<IRuleContextMapper, RuleContextMapper>();
builder.Services.AddSingleton<RuleSetValidator>();
// Whether the real Zendesk client is the one that will actually serve requests decides how
// strict its configuration has to be. With the fake selected — the default, so that a fresh dev
// or test environment never pushes to real Zendesk — blank settings are expected rather than an
// error, and demanding them would stop the worker starting in exactly the environment the fake
// exists to serve. The consumers and the retention jobs share this process, so that is all of
// them, over an integration none of them is using.
builder.Services.AddZendeskApiClient(
builder.Configuration,
requireRealClient: !ZendeskServiceRegistration.ShouldUseFake(builder.Configuration));
builder.Services.AddInfrastructureDependencies(builder.Configuration);
builder.Services.AddNotifyService(builder.Configuration);

builder.Services.AddRulesProvider(builder.Configuration);
// When Zendesk:UseFake is set the real Zendesk service is replaced with a fake that captures
// "created" tickets into the shared dev outbox table, so the rules-engine pipeline can be
// driven and observed without a real Zendesk. The flag defaults to true so a fresh dev or test
// environment never pushes to real Zendesk; setting it to false routes to the real client.
// Gated on configuration rather than the environment name because the test site also runs as
// Development.
ZendeskServiceRegistration.ConfigureFakeZendesk(builder.Services, builder.Configuration);

builder.Services.AddHostedService<RulesConsumer>();
builder.Services.AddHostedService<ZendeskConsumer>();
builder.Services.AddHostedService(sp =>
new DlqRetentionJob(
sp.GetRequiredService<IServiceScopeFactory>(),
sp.GetRequiredService<ILogger<DlqRetentionJob>>()));
builder.Services.AddHostedService(sp =>
new MetricsRetentionJob(
sp.GetRequiredService<IServiceScopeFactory>(),
sp.GetRequiredService<ILogger<MetricsRetentionJob>>()));
builder.Services.AddHostedService(sp =>
new SearchAnalyticsRetentionJob(
sp.GetRequiredService<IServiceScopeFactory>(),
sp.GetRequiredService<ILogger<SearchAnalyticsRetentionJob>>()));
builder.Services.AddHostedService(sp =>
new ContentStagingSessionRetentionJob(
sp.GetRequiredService<IServiceScopeFactory>(),
sp.GetRequiredService<ILogger<ContentStagingSessionRetentionJob>>()));
// The worker only needs the rules-engine pieces from the Application layer, not the
// portal's full service graph (which depends on web-only collaborators).
builder.Services.AddSingleton<IRulesEngine, RulesEngine>();
builder.Services.AddSingleton<IRuleContextMapper, RuleContextMapper>();
builder.Services.AddSingleton<RuleSetValidator>();

builder.Services.AddWorkerHealthChecks();
builder.Services.AddRulesProvider(builder.Configuration);

var app = builder.Build();
app.MapWorkerHealthChecks();
app.Run();
builder.Services.AddHostedService<RulesConsumer>();
builder.Services.AddHostedService<ZendeskConsumer>();
builder.Services.AddHostedService(sp =>
new DlqRetentionJob(
sp.GetRequiredService<IServiceScopeFactory>(),
sp.GetRequiredService<ILogger<DlqRetentionJob>>()));
builder.Services.AddHostedService(sp =>
new MetricsRetentionJob(
sp.GetRequiredService<IServiceScopeFactory>(),
sp.GetRequiredService<ILogger<MetricsRetentionJob>>()));
builder.Services.AddHostedService(sp =>
new SearchAnalyticsRetentionJob(
sp.GetRequiredService<IServiceScopeFactory>(),
sp.GetRequiredService<ILogger<SearchAnalyticsRetentionJob>>()));
builder.Services.AddHostedService(sp =>
new ContentStagingSessionRetentionJob(
sp.GetRequiredService<IServiceScopeFactory>(),
sp.GetRequiredService<ILogger<ContentStagingSessionRetentionJob>>()));

builder.Services.AddWorkerHealthChecks();

var app = builder.Build();
app.MapWorkerHealthChecks();
app.Run();
}
catch (Exception e)
{
Log.Fatal(e, "Worker terminated unexpectedly");
}
finally
{
Log.CloseAndFlush();
}
Loading
Loading