Skip to content

Refactor service lifetimes and improve TranslationService testability #3

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

Closed
punkouter26 wants to merge 1 commit into from
Closed

Refactor service lifetimes and improve TranslationService testability #3

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion global.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"sdk": {
"version": "10.0.102",
"version": "10.0.100",
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The SDK version is being downgraded from 10.0.102 to 10.0.100. While the PR description states this matches the available SDK, this change should be verified to ensure 10.0.102 was not actually available or required. If this is a temporary workaround for local development, consider documenting why 10.0.100 is required or whether this could cause issues for other developers or CI/CD pipelines expecting 10.0.102.

Copilot uses AI. Check for mistakes.
"rollForward": "latestPatch",
"allowPrerelease": false
}
Expand Down
16 changes: 8 additions & 8 deletions src/PoVicTranslate.Web/Program.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,20 +116,20 @@
builder.Services.Configure<ApiSettings>(builder.Configuration.GetSection("ApiSettings"));

// Register core services
builder.Services.AddScoped<ITranslationService, TranslationService>();
builder.Services.AddScoped<ILyricsService, LyricsService>();
builder.Services.AddSingleton<ITranslationService, TranslationService>();
builder.Services.AddSingleton<ILyricsService, LyricsService>();

// Register validation services
builder.Services.AddSingleton<ISpeechConfigValidator, SpeechConfigValidator>();
builder.Services.AddScoped<IAudioSynthesisService, AudioSynthesisService>();
builder.Services.AddSingleton<IAudioSynthesisService, AudioSynthesisService>();
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

Converting AudioSynthesisService to Singleton introduces a critical race condition. The service has mutable state in fields _accessToken (line 18) and _tokenExpiry (line 19) that are accessed and modified in GetAccessTokenAsync without synchronization. Multiple concurrent requests could:

  1. Simultaneously determine the token is expired
  2. Both fetch new tokens
  3. Overwrite each other's _accessToken and _tokenExpiry values

This could lead to inconsistent state where _accessToken and _tokenExpiry are from different token requests. Add a SemaphoreSlim (similar to LyricsService at line 15 of LyricsService.cs) to protect token refresh operations.

Suggested change
builder.Services.AddSingleton<IAudioSynthesisService, AudioSynthesisService>();
builder.Services.AddScoped<IAudioSynthesisService, AudioSynthesisService>();

Copilot uses AI. Check for mistakes.
builder.Services.AddSingleton<IInputValidator, InputValidator>();

// Register diagnostic validators
builder.Services.AddScoped<IDiagnosticValidator, AzureOpenAIDiagnosticValidator>();
builder.Services.AddScoped<IDiagnosticValidator, AzureSpeechDiagnosticValidator>();
builder.Services.AddScoped<IDiagnosticValidator, InternetConnectivityDiagnosticValidator>();
builder.Services.AddScoped<IConfigurationValidator, ConfigurationValidator>();
builder.Services.AddScoped<IDiagnosticService, DiagnosticService>();
builder.Services.AddSingleton<IDiagnosticValidator, AzureOpenAIDiagnosticValidator>();
builder.Services.AddSingleton<IDiagnosticValidator, AzureSpeechDiagnosticValidator>();
builder.Services.AddSingleton<IDiagnosticValidator, InternetConnectivityDiagnosticValidator>();
Comment on lines +128 to +130
Copy link

@codereviewbot-ai codereviewbot-ai bot Jan 30, 2026

Choose a reason for hiding this comment

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

Multiple implementations of IDiagnosticValidator are registered as singletons:

builder.Services.AddSingleton<IDiagnosticValidator, AzureOpenAIDiagnosticValidator>();
builder.Services.AddSingleton<IDiagnosticValidator, AzureSpeechDiagnosticValidator>();
builder.Services.AddSingleton<IDiagnosticValidator, InternetConnectivityDiagnosticValidator>();

If these are intended to be injected as a collection (e.g., IEnumerable<IDiagnosticValidator>), this is correct. However, if a single instance is expected, this will cause ambiguity and may result in only the last registration being resolved. Ensure that the consuming code expects a collection, or clarify the registration strategy.

builder.Services.AddSingleton<IConfigurationValidator, ConfigurationValidator>();
builder.Services.AddSingleton<IDiagnosticService, DiagnosticService>();
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

Converting services to Singleton while using IOptions means configuration values will be captured once at application startup and never refreshed. If runtime configuration reloading is needed in the future, these services would need to use IOptionsMonitor instead. This is acceptable if configuration is static for the application lifetime, which appears to be the current design (no reloadOnChange detected in configuration setup).

Suggested change
builder.Services.AddSingleton<IDiagnosticService, DiagnosticService>();
builder.Services.AddScoped<IDiagnosticService, DiagnosticService>();

Copilot uses AI. Check for mistakes.

// Register utility services
builder.Services.AddSingleton<ILyricsUtilityService, LyricsUtilityService>();
Comment on lines +119 to 135
Copy link

@codereviewbot-ai codereviewbot-ai bot Jan 30, 2026

Choose a reason for hiding this comment

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

All services in this range are registered as singletons. If any of these services depend on scoped or transient services, this can lead to data races or unintended shared state across requests. Review the constructors of these services to ensure that singleton lifetime is appropriate and does not introduce thread safety or resource contention issues. If any service requires per-request or per-operation state, consider using scoped or transient lifetimes instead.

Expand Down
18 changes: 18 additions & 0 deletions src/PoVicTranslate.Web/Services/TranslationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
using Azure.AI.OpenAI;
using Microsoft.ApplicationInsights;
using Microsoft.ApplicationInsights.DataContracts;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
using OpenAI.Chat;
using PoVicTranslate.Web.Configuration;
Expand All @@ -27,10 +28,20 @@ Maintain the sentiment and intent of the original text.
Do not add any explanations, only provide the translated text.
""";

[ActivatorUtilitiesConstructor]
public TranslationService(
IOptions<ApiSettings> apiSettings,
TelemetryClient telemetryClient,
ILogger<TranslationService> logger)
: this(apiSettings, telemetryClient, logger, null)
{
}

public TranslationService(
IOptions<ApiSettings> apiSettings,
TelemetryClient telemetryClient,
ILogger<TranslationService> logger,
ChatClient? chatClient)
Comment on lines +40 to +44
Copy link

Copilot AI Jan 30, 2026

Choose a reason for hiding this comment

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

The new TranslationService constructor that accepts a ChatClient parameter was added to improve testability, but there are no unit tests utilizing this feature. The existing tests in TranslationServiceTests.cs still only test the primary constructor and don't leverage the ability to inject a mock ChatClient. Consider adding tests that use the new constructor with a mocked ChatClient to test the TranslateToVictorianEnglishAsync method's behavior without requiring actual Azure OpenAI configuration.

Copilot uses AI. Check for mistakes.
{
ArgumentNullException.ThrowIfNull(apiSettings);
_telemetryClient = telemetryClient;
Comment on lines +41 to 47
Copy link

@codereviewbot-ai codereviewbot-ai bot Jan 30, 2026

Choose a reason for hiding this comment

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

The constructor does not validate that telemetryClient and logger are non-null before assignment. This could result in a NullReferenceException at runtime if these dependencies are not provided by the DI container.

Recommendation:
Add explicit null checks for telemetryClient and logger:

ArgumentNullException.ThrowIfNull(telemetryClient);
ArgumentNullException.ThrowIfNull(logger);

Insert these checks before assigning the fields.

Expand All @@ -39,6 +50,13 @@ public TranslationService(
var settings = apiSettings.Value;
_deploymentName = settings.AzureOpenAIDeploymentName;

if (chatClient is not null)
{
_chatClient = chatClient;
_logger.LogInformation("TranslationService initialized with injected ChatClient");
return;
}

if (string.IsNullOrWhiteSpace(settings.AzureOpenAIApiKey) ||
string.IsNullOrWhiteSpace(settings.AzureOpenAIEndpoint) ||
string.IsNullOrWhiteSpace(settings.AzureOpenAIDeploymentName))
Expand Down
Loading