Skip to content

Fix SDK version and Service Lifetimes #1

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
punkouter26 wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix SDK version and Service Lifetimes #1

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",
"rollForward": "latestPatch",
"allowPrerelease": false
Comment on lines 2 to 5
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

global.json pins the SDK to 10.0.100 with rollForward: latestPatch. This setting will NOT roll forward to a different feature band (e.g., 10.0.102), so environments that only have newer 10.0.10x SDKs installed will fail with “SDK not found”. To keep builds resilient across dev/CI machines, consider using rollForward: latestFeature (or updating CI to install 10.0.100 specifically).

Copilot uses AI. Check for mistakes.
}
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"));
Copy link

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

Choose a reason for hiding this comment

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

⚠️ Potential runtime failure due to missing or malformed configuration

The line builder.Services.Configure<ApiSettings>(builder.Configuration.GetSection("ApiSettings")); binds configuration to the ApiSettings class. If the ApiSettings section is missing or contains invalid values, this may result in runtime errors or misconfigured services. It is recommended to validate the configuration at startup and fail fast if required settings are missing or malformed. Consider adding explicit validation logic or using IOptions<ApiSettings> with validation:

builder.Services.AddOptions<ApiSettings>()
    .Bind(builder.Configuration.GetSection("ApiSettings"))
    .ValidateDataAnnotations()
    .ValidateOnStart();

This ensures configuration issues are detected early.


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

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

With ILyricsService switched to singleton, LyricsService's SemaphoreSlim will live for the entire app lifetime. Since SemaphoreSlim is disposable and LyricsService doesn't implement IDisposable/IAsyncDisposable, consider disposing it (or refactoring to an alternative async-lock approach) to avoid holding undisposed resources for the process lifetime.

Suggested change
builder.Services.AddSingleton<ILyricsService, LyricsService>();
builder.Services.AddScoped<ILyricsService, LyricsService>();

Copilot uses AI. Check for mistakes.

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

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

IAudioSynthesisService is now registered as a singleton, but AudioSynthesisService maintains mutable shared state (_accessToken / _tokenExpiry) without any synchronization. Under concurrent requests this can lead to races and redundant token fetches, and (in the worst case) using a stale token for a request. Consider adding an async lock/semaphore around token refresh (or using IMemoryCache with a single-flight pattern) so token acquisition is thread-safe when used as a singleton.

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>();
builder.Services.AddSingleton<IConfigurationValidator, ConfigurationValidator>();
builder.Services.AddSingleton<IDiagnosticService, DiagnosticService>();

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

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

Choose a reason for hiding this comment

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

⚠️ Singleton lifetime may cause issues if services are not stateless or depend on scoped services

All core, validation, diagnostic, and utility services are registered as singletons. If any of these services maintain state or depend on scoped services (such as HttpContext or per-request data), this can lead to data races or incorrect behavior. Review the implementation of these services to ensure singleton lifetime is appropriate. If any service requires per-request data, consider using AddScoped instead.

Expand Down
Loading