daniel/deepdrft
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 13 Wiki Activity
Files
02cc83ed310259207f9a73e66faa4d35ea61257f
deepdrft/TODO-V2.md
T

24 KiB
Raw Blame History

DeepDrft — Design & Streaming Audit (2026-05-17)

Two parallel analyses commissioned on 2026-05-17:

  1. Design Audit — SOLID compliance, DRY violations, encapsulation, documentation, modularity across all 8 projects.
  2. Streaming Evaluation — Functional correctness, error handling, state machine integrity, performance, and missing capabilities in the audio streaming pipeline.

Part 1: SOLID / DRY / Modularity Design Audit

Reviewer: code-reviewer
Scope: All 8 projects — DeepDrftModels, DeepDrftWeb.Services, DeepDrftWeb (host), DeepDrftWeb.Client, DeepDrftContent (host), DeepDrftContent.Services, DeepDrftCli, DeepDrftTests.

Overall Assessment: The macro architecture is sound — the host/services split is honoured, dual-database responsibilities are cleanly divided, and the FileDatabase subsystem is notably well-structured. The most pressing issues are a concurrency hazard in the VaultIndexDirectory lock, a silent data-loss risk in TrackRepository.Update, and several smaller modularity leaks. Most findings are in the Major/Minor tier; there are no broad design failures.

🔴 Critical

DeepDrftContent.Services/FileDatabase/Services/IndexSystem.cs:98–128 — Lock released before async disk write; race window on index corruption

VaultIndexDirectory.AddToIndexAsync takes _indexLock to mutate _vaultIndex, then releases it before calling SaveIndexAsync (which writes JSON to disk). If a second call arrives after the lock drop but before the file write finishes, it can read stale in-memory state and overwrite with an older version of the index. The ReloadIndexAsync path shares the same lock pattern. Fix: persist the in-memory state to a local snapshot before the lock releases, or ensure SaveIndexAsync is called inside the lock (acceptable because file I/O is the only async operation and the lock is not held across unrelated waits).

DeepDrftWeb.Services/Repositories/TrackRepository.cs:47–65 — Update silently creates a new record when Id is not found

If a caller passes a TrackEntity with a non-existent Id, Update falls through to Create, issuing an INSERT instead of signalling an error. The service wraps this in a ResultContainer that will report success, giving the caller no indication that the semantics changed. This matters in the CLI's ValidateAndUpdateTrackAsync path. Fix: return a Result.CreateFailResult("Track not found") instead of calling Create.

🟡 Major

DeepDrftContent.Services/FileDatabase/Services/FileDatabase.cs:120–138 — Vacuous try/catch/throw in CreateVaultAsync

The try block catches everything and immediately rethrows. It adds no logging, no transformation, and no cleanup — it is pure noise that also prevents the method from following the project's established exception-swallowing contract for public FileDatabase operations. Either adopt the swallow-and-return-false pattern (matching the rest of the public API) or remove the try/catch entirely.

DeepDrftContent.Services/TrackService.cs:78–81 — Re-throws InvalidOperationException wrapping every exception, including cancellation

AddTrackFromWavAsync catches Exception ex in the outer handler and always rethrows as new InvalidOperationException(...). This swallows OperationCanceledException, which will lose the cooperative-cancellation semantics expected by the CLI and any future async callers. Fix: add when (ex is not OperationCanceledException) to the catch filter, or rethrow cancelled exceptions unwrapped.

DeepDrftContent.Services/FileDatabase/Services/IndexSystem.cs:122 — Console.WriteLine used for structured diagnostics in library code

VaultIndexDirectory.ReloadIndexAsync and IndexWatcher.cs both emit diagnostic output directly to Console.WriteLine. The DeepDrftContent.Services library has no dependency on ILogger, so when hosted inside ASP.NET Core the messages bypass the structured logging pipeline (no log level, no correlation, no sink routing). Fix: inject ILogger<T> into FileDatabase and propagate it to VaultIndexDirectory and IndexWatcher, or accept the logger as an optional constructor parameter so tests can omit it.

DeepDrftWeb.Services/TrackService.cs:60–76 — Sort-sentinel strings constructed per-service-instance with 64-char aggregations

The _sortLastAscending and _sortLastDescending fields use Enumerable.Repeat(..., 64).Aggregate(...) — a per-instance heap allocation at construction time. The idiomatic version is entity.Album ?? "" (for ascending nulls-to-end). Also note that _sortLastDescending is never actually used in the switch — only _sortLastAscending is referenced — so one field is entirely dead.

DeepDrftWeb.Services/TrackRepository.cs:28–36 — GetPage counts all rows then queries a potentially sorted subset; count and page can be inconsistent under concurrent writes

CountAsync() and ToListAsync() are two separate database round-trips without a transaction. Under concurrent inserts/deletes a caller could receive a page count for N rows but a page containing N±k rows. More concretely: if filtering is ever added, the count query will silently return the wrong TotalCount. Consider combining into a single query or adding a comment explaining the deliberate trade-off.

DeepDrftContent.Services/FileDatabase/Services/FileDatabase.cs:79–99 — IndexFactoryService instantiated on every GetVaultTypeFromIndex call

Each call constructs a fresh IndexFactoryService complete with two Dictionary<IndexType, Func<...>> initializations. FileDatabase already holds _vaults and _indexWatcher as fields; the factory should be the same. Promote to a private field.

DeepDrftModels/DTOs/TrackDto.cs:7–8 — TrackName and Artist are not required while their entity counterparts are

A new TrackDto() object silently has null TrackName and Artist, which can cause NREs or invalid-data bugs if a DTO is instantiated directly. Add required to both or annotate with [NotNull] and a comment explaining the intentional deviation.

DeepDrftWeb.Client/Services/AudioPlayerService.cs — Base class StreamAudio allocates a new byte[32KB] buffer on every loop iteration

Produces GC pressure proportional to audio file size. StreamingAudioPlayerService correctly uses ArrayPool<byte>.Shared. Use ArrayPool in both paths.

DeepDrftCli/Services/GuiService.cs — TruncateString is duplicated verbatim from CliService

GuiService.TruncateString and CliService.TruncateString are identical private methods. Extract to a shared CliUtils static class in the DeepDrftCli project.

🟢 Minor / Style

DeepDrftContent.Services/FileDatabase/Models/MediaModels.cs:119–123 and 169–173 — GetExtensionType duplicated in ImageBinary and AudioBinary

Both subclasses define identical single-line forwarding methods. The call sites could reference MimeTypeExtensions.GetExtension directly, or the method could live once in MediaBinary as protected static.

DeepDrftContent.Services/FileDatabase/Models/MediaFactories.cs:58 — Multiple SimpleMediaTypeRegistry instances across static factory classes

MediaVaultTypeMap, MetaDataFactory, MediaParamsFactory, FileBinaryFactory, and FileBinaryDtoFactory each hold their own private static readonly IMediaTypeRegistry instance. They should share one.

DeepDrftWeb.Services/TrackService.cs — No interface for TrackService; concrete type injected directly

TrackController and CliService both depend on concrete TrackService. Defining ITrackService (or even a minimal IReadTrackService) would improve testability and remove the namespace disambiguation noise in CliService.cs.

DeepDrftWeb.Services/TrackService.cs:11–12 — Dead field _sortLastDescending

Declared but never used in the sort switch. Remove it.

DeepDrftWeb.Client/Services/AudioPlayerService.cs — Commented-out code

DarkModeSettings.cs has commented-out IsDarkModeChanged event. AudioPlayerService.SelectTrack has // NotifyStateChanged(); commented out. Either remove or document with a // TODO explaining intent.

DeepDrftWeb.Services/TrackRepository.cs:32 — Skip recalculated manually

Uses (pageParameters.Page - 1) * pageParameters.PageSize instead of pageParameters.Skip. Use pageParameters.Skip.

DeepDrftModels/Models/PagedResult.cs:30 — ?? new List<T>() guard is unreachable

items.ToList() ?? new List<T>() — ToList() never returns null. Remove the null-coalescing branch.

DeepDrftContent/Controllers/TrackController.cs — Controller calls FileDatabase directly, bypassing TrackService

GetTrack and PutTrack call _fileDatabase.LoadResourceAsync and RegisterResourceAsync directly. The controller and CLI service have different entry paths into FileDatabase. If vault initialisation logic changes in TrackService, the controller will not pick it up.

DeepDrftWeb.Client/Services/AudioInteropService.cs:223–241 — InvokeJsAsync<T> uses runtime type-checking switch

Adding a new result type requires updating this method. Adding an IOperationResult marker interface would let the method use a generic constraint and remove the typeof switch.

💡 Suggestions

No rollback for orphaned vault entries on dual-database write failure. A compensating mechanism — even a simple DeadLetterLog recording orphaned entryKey values — would allow a maintenance pass to clean up without manually auditing the vault.

StructuralMap and StructuralSet are Dictionary<string, T> wrappers for string keys. The GetKeyString fast-path means no JSON overhead occurs in production. Consider whether plain Dictionary<string, T> and HashSet<string> would serve the same purpose with less indirection.

AudioPlayerService.StreamAudio and StreamingAudioPlayerService.StreamAudioWithEarlyPlayback share the read-loop skeleton. A shared protected async Task ReadChunks(...) helper in the base class would unify progress reporting and the do/while structure.

IPlayerService interface exposes EventCallback? (a Blazor-specific type) as a settable property. This leaks a UI framework dependency into a service contract. An event Action StateChanged pattern is framework-agnostic.

✅ Positive Observations

  • The host/services split is genuinely honoured. Every piece of domain logic is in a *.Services project; hosts contain only controllers, middleware, DI wiring, and config. DeepDrftCli consuming both service libraries without taking ASP.NET dependencies is a direct payoff of this discipline.
  • The FileDatabase abstraction layer is well-designed. The IMediaTypeRegistry / SimpleMediaTypeRegistry pattern cleanly centralises all type-specific factory logic, avoids reflection entirely, and the registration-time lambda approach is expressive.
  • WavOffsetService is precise and well-explained. Block alignment is correctly applied, the chunk-walking loop handles non-fixed-header WAV files (LIST chunks, INFO chunks), and the early-return guards are tight.
  • Result pattern usage is consistent. Services return ResultContainer<T> / Result; callers check .Success rather than catching; applied uniformly across the SQL-side and CLI paths.
  • Test isolation is solid. Unique temp-directory per test, best-effort [TearDown] cleanup, and ArrayPool-safe test data via TestData.cs mean test runs are genuinely independent and fast.
  • TrackDto / TrackEntity separation is preserved even though today they are structurally identical. This seam means the API response shape can diverge from the database shape without a refactor.

Part 2: Streaming System Functional Evaluation

Evaluator: staff-engineer
Scope: TypeScript audio pipeline (StreamDecoder, PlaybackScheduler, AudioContextManager, AudioPlayer, SpectrumAnalyzer, wavutils.ts) + C# streaming server (WavOffsetService, AudioProcessor, StreamingAudioPlayerService, AudioInteropService, TrackController).

✅ What's Working Well

  • Clean SRP decomposition on the JS side. AudioContextManager / StreamDecoder / PlaybackScheduler / SpectrumAnalyzer each own one responsibility; AudioPlayer is a thin composer.
  • The seek-beyond-buffer architecture is genuinely good. Synthesising a fresh 44-byte WAV header server-side and tearing down only the decoder (not the AudioContext or scheduler offset bookkeeping) is the right seam. playbackOffset cleanly bridges "the buffer array starts at time T."
  • Block alignment is observed on both sides (WavOffsetService.alignedOffset and StreamDecoder.calculateByteOffset). Both round down to blockAlign boundaries — correct invariant for click-free seek.
  • Adaptive chunk sizing on the C# side is sensible. 16–64 KB with hysteresis avoids thrashing.
  • HttpCompletionOption.ResponseHeadersRead used correctly — playback can start before the full payload arrives.
  • Sample-rate matching via recreateWithSampleRate re-creates the context at the source rate rather than forcing resampling.
  • Lookahead scheduler (500 ms) is well-chosen — enough to absorb GC jitter without scheduling so far ahead that pause/seek latency becomes user-visible.
  • ArrayPool<byte>.Shared rental in StreamAudioWithEarlyPlayback is correct for a hot loop on large files.
  • Spectrum animation throttled to ~30 fps and gated by subscriber count — JS interop is the expensive seam, not RAF.

🔴 Critical

1. Concurrent seeks race the streaming loop; cancellation does not await the previous loop's exit.

DeepDrftWeb.Client/Services/StreamingAudioPlayerService.cs — SeekBeyondBuffer cancels _streamingCancellation and immediately disposes it, then constructs a new one and calls StreamAudioWithEarlyPlayback. The previously-running loop's ReadAsync will throw OperationCanceledException asynchronously, after the new loop has already begun pushing chunks to JS. Both loops can be calling ProcessStreamingChunk interleaved against the same StreamDecoder instance until the old one unwinds. The JS decoder is single-instance per player and cannot distinguish "old stream's late chunk" from "new stream's first chunk."

Recommendation: Track the active stream generation as a counter; on each new stream, increment and capture locally; gate ProcessStreamingChunk calls on generation === currentGeneration. Also await the previous streaming task before reinitializing.

2. Garbage trailing bytes shipped to JS (silent audio corruption).

StreamingAudioPlayerService.cs:167 — currentBytes == _currentBufferSize ? buffer : buffer[..currentBytes]. When currentBytes == _currentBufferSize, the full MaxBufferSize rented array (dirty from ArrayPool) is passed to JS interop. Blazor serializes all of it including stale bytes past currentBytes. The decoder adds garbage audio data.

Recommendation: Drop the conditional; always pass buffer.AsMemory(0, currentBytes).ToArray().

3. Final-tail audio silently dropped at EOF.

DeepDrftWeb/Interop/wavutils.ts:176–191 — getSampleAlignedChunkSize returns 0 when remaining bytes < minimum decode frame (512 bytes / 10 frames). Those bytes are never decoded. isComplete eventually flips true, but tryDecodeNextSegment has already returned null and the trailing audio is silently discarded.

Recommendation: Track "stream-complete" state in StreamDecoder and, once isComplete, decode whatever remains regardless of the minimum.

4. First chunk assumed to contain the entire WAV header.

DeepDrftWeb/Interop/audio/StreamDecoder.ts:58–79 and wavutils.ts:12–101 — If the first network chunk is < 44 bytes (possible at TCP segment boundaries), parseHeader returns null and the decoder throws fatal "Invalid WAV header in first chunk." Also breaks on non-trivial headers with LIST/INFO/JUNK chunks extending past the first chunk.

Recommendation: Accumulate raw bytes until parseHeader returns non-null before flipping isFirstChunk. The current rawChunks array is already designed to grow — feed pre-header bytes into it the same way and re-attempt parse on each chunk until success.

🟡 Major

5. Format inconsistency: WavOffsetService / AudioProcessor / wavutils.ts disagree on PCM vs. IEEE Float.

AudioProcessor.ValidateWavStructure only accepts PCM (format 1). WavOffsetService accepts PCM + IEEE Float (format 3). WavOffsetService.CreateWavHeader hard-codes (short)1 in the synthesized header. A seek on a Float32 WAV would produce a corrupt header claiming PCM with Float-encoded data.

Recommendation: Standardize on PCM-only everywhere; enforce identically in all three places; have CreateWavHeader accept format as a parameter.

6. seekBeyondBuffer rejects offset 0 as invalid.

DeepDrftWeb/Interop/audio/AudioPlayer.ts:288 — if (byteOffset <= 0) return error. calculateByteOffset(0) returns 0 legitimately (seek to start). A play-from-zero after a long-running stream cannot recover.

Recommendation: Change the guard to byteOffset < 0.

7. Server allocates full byte[] for every request — no true server-side streaming.

DeepDrftContent/Controllers/TrackController.cs:47 — return File(file.Buffer, mimeType). LoadResourceAsync<AudioBinary> reads the entire file into memory. A 100 MB WAV = 100 MB LOH allocation per request.

Recommendation: Add a LoadResourceStreamAsync overload returning an open FileStream. Have the controller pass it to File(stream, mime, enableRangeProcessing: true).

8. ?offset= query parameter instead of HTTP Range headers.

The architecture re-invents byte-range streaming on top of a custom query parameter, defeating HTTP caching (CDN sees ?offset=1234567 as a distinct URL) and preventing use of enableRangeProcessing: true. This is architecture-intentional (requires decodeAudioData for spectrum analysis) — but the trade-off should be documented. Note: MediaElementAudioSourceNode would still permit FFT tap on the audio graph with native range/seek/cache.

9. 5-second decode timeout silently drops segments.

StreamDecoder.ts:166–175 — on decode timeout, tryDecodeNextSegment catches and returns null. The bytes are consumed (processedBytes was already advanced), creating a silent ~0.37 s audio gap.

Recommendation: On decode failure, decrement processedBytes by the segment size for retry, or fail the stream loudly.

10. playFromPosition silently no-ops when position equals total duration.

PlaybackScheduler.ts:101–113 — if position exactly equals total duration, the loop falls through with startBufferIndex == buffers.length. A pause near end-of-stream followed by play cannot resume.

Recommendation: Clamp to getTotalDuration() - epsilon before searching, or detect end-of-stream and call handlePlaybackEnded.

11. AudioPlayer.play() does not await ensureReady().

AudioPlayer.ts:190 — this.contextManager.ensureReady() returns a Promise that is never awaited. On a backgrounded tab with a suspended AudioContext, createBufferSource and source.start run against a suspended context; audio silently fails.

Recommendation: Make play() async and await ensureReady().

12. Progress callbacks fire after disposal; AudioPlayerProvider has no IAsyncDisposable.

AudioPlayer.ts:455–460 and AudioInteropService.cs:288–292 — the JS setInterval holds DotNetObjectReference after component unmount, throwing every 100 ms. AudioPlayerProvider has no IAsyncDisposable, so layout teardown leaks the streaming task, AudioContext, and HTTP connection.

Recommendation: Make AudioPlayerProvider implement IAsyncDisposable and call _audioPlayerService.DisposeAsync() on dispose.

13. No cancellation token on the HTTP GET for streaming.

StreamingAudioPlayerService streams without a CancellationToken passed to _http.GetAsync. Navigation mid-load leaves a server connection draining bytes into the void.

Recommendation: Pass the streaming CancellationToken through to TrackMediaClient.GetTrackMedia and the underlying _http.GetAsync.

14. WavOffsetService.CreateOffsetStream allocates the full offset stream upfront in a MemoryStream.

For a 1 MB seek into a 100 MB file, this allocates 99 MB into a fresh MemoryStream. Pure waste.

Recommendation: Return a custom composite Stream wrapping the synthesized header followed by new MemoryStream(fullAudioBuffer, headerSize + alignedOffset, newDataSize, writable: false).

🟢 Minor

15. playbackAnchorTime == 0 as is-playing sentinel is brittle. AudioContext.currentTime is technically allowed to be 0 at context creation. Use the isActive_ flag that already exists.

16. Non-WAV upload error message is unhelpful. "Invalid WAV header in first chunk" falls through to StreamingErrorHandler as "Unable to play audio. Please try again." Add a "header" / "wav" match to the error handler.

17. new TextDecoder() allocated inside chunk-walk loop in parseHeader. TextDecoder instantiation is non-trivial. Allocate once at the top of parseHeader.

18. WavOffsetService.CreateOffsetStream allocates full offset stream upfront. (See Major finding 14 — this Minor note is the scope annotation; the fix is the same.)

19. MimeTypeExtensions.GetExtension for unmapped mime returns .bin. Silently renames unknown types. Reject unknown mime types at the PUT controller instead.

20. AudioProcessor.cs FindChunk returns first match; WavOffsetService.ParseWavHeader uses last match. Inconsistent behaviour on malformed WAV with two fmt chunks. Pick one.

21. processedBytes and totalRawBytes are number (double) — correct for ≤ 2^53 bytes. Note for posterity; not a real risk at audio file sizes.

22. SpectrumAnalyzer.dataArray typed Float32Array<ArrayBuffer> — requires TypeScript ≥ 5.7. Confirm tsconfig.json target.

🚫 Missing Capabilities

  • Backward seek is broken. Seeks below playbackOffset silently clamp to start of the current buffer segment, not the user's chosen time. To truly seek backward requires the same "re-fetch from offset" path as forward-beyond-buffer. This is the highest-impact missing feature.
  • Format diversity. PCM/IEEE Float WAV only. MP3 / FLAC / Ogg / AAC / M4A are MIME-mapped but unimplemented.
  • Preload / prefetch next track. No mechanism to start the next track's stream during the tail of the current.
  • Crossfade. Two simultaneous PlaybackSchedulers would suffice architecturally; nothing wires it.
  • Gapless playback. Inter-track gap is unmanaged.
  • HTTP Range + cache integration. See finding 8.
  • Server-side stream from disk. See finding 7.
  • Track-skip on error. A failed processStreamingChunk aborts the entire load; no concept of skipping bad bytes to recover.

🌐 Browser Compatibility

  • AudioContext with explicit sampleRate — Safari ≤ 14 ignored the sampleRate constructor option. Safari 14.1+ honors it.
  • webkitAudioContext fallback exists (AudioContextManager.ts:21) — good. But webkitAudioContext.close() is async-but-not-Promise on older Safari; the await resolves immediately and the next initialize may run against a not-yet-closed context.
  • AnalyserNode.getFloatFrequencyData — supported everywhere modern.
  • AudioBufferSourceNode.onended — fires reliably; in Firefox, fires on next microtask rather than the audio thread. handleSourceEnded scheduling more buffers immediately is fine.
  • HttpCompletionOption.ResponseHeadersRead + WASM HttpClient — depends on browser's streaming fetch support. All evergreen browsers handle this; iOS Safari < 15 had quirks.
  • Uint8Array over JS interop — Blazor serializes via base64. 64 KB chunks incur base64 overhead on every chunk push.

🏆 Top 5 Priorities (Streaming)

  1. Fix finding 2 — garbage trailing bytes shipped to JS (silent audio corruption)
  2. Fix finding 3 — final-tail audio dropped at EOF (silent audio truncation)
  3. Fix finding 1 — concurrent seek race (reproducible by rapid scrub bar clicks)
  4. Implement backward seek (fundamental UX gap)
  5. Fix findings 12 & 13 — AudioPlayerProvider disposal + HTTP cancellation (session leaks)

Generated 2026-05-17. Both analyses are read-only — no code was changed.

Reference in New Issue View Git Blame Copy Permalink