Convert from Sqlite to Redis, Add Bulk Requests, Queues, and Genre Caching#274
Convert from Sqlite to Redis, Add Bulk Requests, Queues, and Genre Caching#274
Conversation
Bumps caddy from `efb93f7` to `b6424b4`. --- updated-dependencies: - dependency-name: caddy dependency-version: '2' dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [actions/setup-dotnet](https://github.com/actions/setup-dotnet) from 5.0.1 to 5.1.0. - [Release notes](https://github.com/actions/setup-dotnet/releases) - [Commits](actions/setup-dotnet@2016bd2...baa11fb) --- updated-dependencies: - dependency-name: actions/setup-dotnet dependency-version: 5.1.0 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
* Fix Apple Music link parser to properly handle query parameters
…the queuing system for requests.
…umentation for Redis migration (#242)
…encies, and credential validation (#253)
- Run formatter, fix line endings and encodings
… and comprehensive tests (#254)
In-Progress merge to reduce the number of PRs see the fan out of PRs for more context. - Add initial ATProto user OAuth configuration - Add genre caching functionality - Overhaul ATProto process to add a unified session manager to vastly reduce the number of refreshSession and getSession calls we're making - Update package lock files for CI with linux-x64 runtime (#256) - Implement Initial ATProto OAuth token exchange with DPoP and fix base branch issues - Add genre caching for playlist organization (#258) - Fix additional bugs
…#259) * - Add initial ATProto user OAuth configuration - Add genre caching functionality * Overhaul ATProto process to add a unified session manager to vastly reduce the number of refreshSession and getSession calls we're making * Update package lock files for CI with linux-x64 runtime (#256) * Initial plan * Address non-resolved review comments: OAuth methods and Dockerfile improvements Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Move ATProtoOAuthResult to its own file in Records directory Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Implement full OAuth token exchange with DPoP support Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Improve redirect URI handling with constants and validation Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Address code review feedback: sealed modifier, error handling, and redirect URI fix Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Refactor: narrow try-catch scope and simplify JWT return Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Address review feedback: validation, error handling, and DPoP improvements Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Refine error handling and nbf timing for clock skew tolerance Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Clarify error messages for token response parsing Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Add comprehensive unit tests for ATProto OAuth service and DPoP JWT creation Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Fix HttpClient anti-pattern: use IHttpClientFactory instead of storing HttpClient Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Run formatter * Enhance error handling and JWK validation, reorganize utility methods Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * refactor: standardize logging with Serilog and consolidate cache layer - Migrate all workers to Serilog via ServiceDefaults - Remove MediaLinkCacheRepository, consolidate into RedisMediaLinkCache - Enhance SagaCoordinator with comprehensive logging and retry logic - Add integration tests for cache layer - Update documentation and deployment scripts * Add resilience configuration, documentation, and TODOs for OAuth improvements Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * 1. Increase maintainability index scores by a. Breaking out shared logic into the BridgeBeats.Worker.Common project b. reducing top-level namespaces and breaking code up into methods and classes. * Implement ATProto OAuth token exchange with DPoP and fix base branch issues (#257) * Move dependabot file back out of workflows. * [WIP] Add genre caching for playlist organization (#258) * Initial plan * Fix Dockerfile ARG, encrypt DPoP key storage, add OAuth cleanup service Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Replace silent OAuth session failure with explicit NotImplementedException Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Remove old unencrypted DPoP key property and add migration Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Run initial OAuth cleanup on startup and simplify exception message Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Fix Data Protection config and scoped service dependency in cleanup service Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Fix Data Protection wiring and update comments to reflect actual behavior Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Document plain text storage reality and security risks honestly Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Fix ASP0015 issues * - Resolve CA1041 issues - Fix tests - Run formatter - Prefer string.Empty to "" * - Resolve CA1822 issues * - Resolve CA1848 and add event Ids. - Resolve IDE0305 too because I forgot to hit commit before updating these. * Additional IDE recommended improvements. * Adjustments to address MSTEST0049 * Queues seem functional * Regenerate package lock files for linux-x64 runtime Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> --------- Co-authored-by: Taylor Marvin <taylor@marvin.email> Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.com> Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com>
…est forgery token validation Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…est forgery token validation Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
…ry test helper, break up TestUtilities file into individual class files.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR converts BridgeBeats from SQLite to Redis for caching, introduces queue-based processing with per-provider rate limit tracking, adds bulk lookup capabilities, and implements genre data caching. The changes reorganize the project structure around .NET Aspire orchestration with separate worker services for each music provider.
Changes:
- Replaced SQLite with Redis for all lookup cache operations and introduced distributed request deduplication
- Added queue-based request processing with priority weights and per-endpoint rate limit tracking
- Introduced separate worker services for Spotify, Apple Music, Tidal, Discord bot, and saga coordination
- Added genre caching infrastructure for future metadata enrichment features
Reviewed changes
Copilot reviewed 123 out of 375 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BridgeBeats.Core/Domain/Extensions/AspireServiceExtensionsLog.cs | New logging definitions for HTTP retry operations |
| src/BridgeBeats.Core/Domain/Extensions/AspireServiceExtensions.cs | Enhanced resilience configuration with rate limit handling and file logging |
| src/BridgeBeats.Contracts/packages.lock.json | Updated idunno.AtProto packages to version 1.5.0 |
| src/BridgeBeats.Contracts/Records/*.cs | New record types for worker API requests/responses and queue management |
| src/BridgeBeats.Contracts/Interfaces/*.cs | New interfaces for queue management, rate limiting, and saga coordination |
| src/BridgeBeats.Contracts/Exceptions/RetryAfterExceededException.cs | New exception for handling excessive rate limit delays |
| src/BridgeBeats.Contracts/Constants/SpotifyConstants.cs | Batch operation limits for Spotify API |
| src/BridgeBeats.AppHost/. | Aspire orchestration configuration for Redis and worker services |
| docs/*.md | Updated documentation for Redis, queue system, and worker architecture |
| containers/. | Updated Docker configuration for Aspire AppHost and multi-service deployment |
| Tests/*.cs | Comprehensive test updates for Redis integration and new services |
- services start up and stay running
| # 2) Create new appsettings.json | ||
| cat > /app/appsettings.json <<EOF | ||
| # 3) Create new appsettings.json | ||
| cat > /src/BridgeBeats.Web/appsettings.json <<EOF |
There was a problem hiding this comment.
The script writes to /src/BridgeBeats.Web/appsettings.json but earlier removed /app/appsettings.json. Ensure the paths are consistent with the new orchestration model. If projects are now in /src/, update the comment on line 95 to reflect the correct path being removed.
| - internal | ||
| command: > | ||
| sh -c "if [ -f /run/secrets/redis_password ]; then | ||
| redis-server --appendonly yes --requirepass \"$$(cat /run/secrets/redis_password)\"; |
There was a problem hiding this comment.
The Redis password is read from /run/secrets/redis_password but passed directly to the command line via --requirepass. This exposes the password in process listings. Consider using Redis ACL config files or environment variables with proper escaping instead.
| redis-server --appendonly yes --requirepass \"$$(cat /run/secrets/redis_password)\"; | |
| mkdir -p /usr/local/etc/redis && | |
| printf 'requirepass %s\nappendonly yes\n' \"$(cat /run/secrets/redis_password)\" > /usr/local/etc/redis/redis.conf && | |
| redis-server /usr/local/etc/redis/redis.conf; |
|
|
||
| # Logging configuration | ||
| LOG_FILE_PATH="${LOG_FILE_PATH:-/app/data/logs/bridgebeats-.log}" | ||
| LOG_DIR_PATH="${LOG_DIR_PATH:-/app/data/logs}" |
There was a problem hiding this comment.
The variable name changed from LOG_FILE_PATH to LOG_DIR_PATH, but the comment in docs/CONFIGURATION.md line 54 still references LOG_FILE_PATH in the configuration table. Ensure all documentation is updated to reflect this naming change.
| @@ -0,0 +1,154 @@ | |||
| using BridgeBeats.Providers.AppleMusic; | |||
There was a problem hiding this comment.
The import references BridgeBeats.Providers.AppleMusic but based on other test files in this PR, the namespace should be BridgeBeats.Core.Domain.Providers.AppleMusic to match the new project structure mentioned in the PR description. Verify the correct namespace.
| using BridgeBeats.Providers.AppleMusic; | |
| using BridgeBeats.Core.Domain.Providers.AppleMusic; |
|
|
||
| /// <summary> | ||
| /// Ordered list of track record keys (rkeys) referencing link.bridgebeats.lookup records. | ||
| /// Maximum 16,384 entries. |
There was a problem hiding this comment.
The comment mentions a maximum of 16,384 entries but there is no validation in the code to enforce this limit. Consider adding validation in the setter or a method to check the count before adding tracks.
| using BridgeBeats.Contracts.Enums; | ||
| using BridgeBeats.Infrastructure.Storage; | ||
| using FluentAssertions; | ||
| using BridgeBeats.Core.Infrastructure.Storage; |
There was a problem hiding this comment.
The namespace has changed from BridgeBeats.Infrastructure.Storage to BridgeBeats.Core.Infrastructure.Storage. Ensure all references to RecordKeyGenerator throughout the codebase have been updated to use the new namespace.
| REDIS_HOST="${REDIS_HOST:-redis}" | ||
| REDIS_PORT="${REDIS_PORT:-6379}" | ||
| # Try to read Redis password from Docker secret | ||
| REDIS_PASSWORD="$(read_secret "redis_password")" |
There was a problem hiding this comment.
Redis password is being read from a secrets file but is stored in an environment variable. Ensure that this environment variable is not logged or exposed in any diagnostic outputs.
wip oauth public -> confidential client update
…Beats into refactor-develop
| # Read from Docker secret if available, otherwise generate a random one | ||
| INTERNAL_SERVICE_KEY="$(read_secret "internal_service_key")" | ||
| if [ -z "$INTERNAL_SERVICE_KEY" ]; then | ||
| INTERNAL_SERVICE_KEY="$(head -c 32 /dev/urandom | base64)" | ||
| fi |
There was a problem hiding this comment.
Generating a random internal service key on every container start creates authentication inconsistency across container restarts and prevents worker services from authenticating with the main API. The key should be read from a Docker secret or environment variable to persist across restarts and be shared with worker containers.
| # Read from Docker secret if available, otherwise generate a random one | |
| INTERNAL_SERVICE_KEY="$(read_secret "internal_service_key")" | |
| if [ -z "$INTERNAL_SERVICE_KEY" ]; then | |
| INTERNAL_SERVICE_KEY="$(head -c 32 /dev/urandom | base64)" | |
| fi | |
| # Must be provided via environment variable or Docker secret to remain stable across restarts | |
| INTERNAL_SERVICE_KEY="${INTERNAL_SERVICE_KEY:-}" | |
| if [ -z "$INTERNAL_SERVICE_KEY" ]; then | |
| INTERNAL_SERVICE_KEY="$(read_secret "internal_service_key")" | |
| fi | |
| if [ -z "$INTERNAL_SERVICE_KEY" ]; then | |
| echo "ERROR: INTERNAL_SERVICE_KEY is not set. Configure it via environment variable or Docker secret 'internal_service_key'." >&2 | |
| exit 1 | |
| fi |
|
@copilot open a new pull request to apply changes based on the comments in this thread, this thread, and this thread |
…285) * Initial plan * Address review comments: fix path comment, secure Redis password, require persistent internal service key Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Improve Redis password handling: add error checking and prevent command injection Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Use head -n 1 to ensure only first line of password file is used Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Improve error handling and use safer awk print instead of printf Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Fix Redis config generation, healthcheck auth, signal handling, and secret newlines Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Add --no-auth-warning flag and clarify secret handling comment Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> * Use head -n 1 in healthcheck for consistency and security Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: tsmarvin <57049894+tsmarvin@users.noreply.github.com> Co-authored-by: Taylor Marvin <Taylor@Marvin.Email>
Significant overhaul to the BridgeBeats process to