Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
46 changes: 43 additions & 3 deletions ProjectVG.Application/Models/Chat/ChatSegment.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

namespace ProjectVG.Application.Models.Chat
{
public record ChatSegment
public record ChatSegment : IDisposable
{

public string Content { get; init; } = string.Empty;
Expand Down Expand Up @@ -37,7 +37,9 @@ public ReadOnlySpan<byte> GetAudioSpan()
{
if (AudioMemoryOwner != null && AudioDataSize > 0)
{
return AudioMemoryOwner.Memory.Span.Slice(0, AudioDataSize);
var memory = AudioMemoryOwner.Memory;
var safeSize = Math.Min(AudioDataSize, memory.Length);
return memory.Span.Slice(0, safeSize);
}
if (AudioData != null)
{
Expand Down Expand Up @@ -82,9 +84,26 @@ public ChatSegment WithAudioData(byte[] audioData, string audioContentType, floa

/// <summary>
/// 메모리 효율적인 방식으로 음성 데이터를 추가합니다 (LOH 방지)
/// 소유권이 이전되므로 호출자는 더 이상 audioMemoryOwner를 해제하지 않아야 합니다.
/// </summary>
/// <param name="audioMemoryOwner">소유권이 이전될 메모리 소유자</param>
/// <param name="audioDataSize">실제 오디오 데이터 크기 (메모리 크기 이하여야 함)</param>
/// <param name="audioContentType">오디오 컨텐츠 타입</param>
/// <param name="audioLength">오디오 길이 (초)</param>
/// <returns>새로운 ChatSegment 인스턴스</returns>
/// <exception cref="ArgumentNullException">audioMemoryOwner가 null인 경우</exception>
/// <exception cref="ArgumentOutOfRangeException">audioDataSize가 유효하지 않은 경우</exception>
public ChatSegment WithAudioMemory(IMemoryOwner<byte> audioMemoryOwner, int audioDataSize, string audioContentType, float audioLength)
{
if (audioMemoryOwner is null)
throw new ArgumentNullException(nameof(audioMemoryOwner));

if (audioDataSize < 0 || audioDataSize > audioMemoryOwner.Memory.Length)
throw new ArgumentOutOfRangeException(
nameof(audioDataSize),
audioDataSize,
$"audioDataSize는 0 이상 {audioMemoryOwner.Memory.Length} 이하여야 합니다.");

return this with
{
AudioMemoryOwner = audioMemoryOwner,
Expand Down Expand Up @@ -120,7 +139,28 @@ public ChatSegment WithAudioMemory(IMemoryOwner<byte> audioMemoryOwner, int audi
/// </summary>
public void Dispose()
{
AudioMemoryOwner?.Dispose();
Dispose(true);
GC.SuppressFinalize(this);
}

/// <summary>
/// 보호된 Dispose 패턴 구현
/// </summary>
/// <param name="disposing">관리되는 리소스를 해제할지 여부</param>
protected virtual void Dispose(bool disposing)
{
if (disposing && AudioMemoryOwner != null)
{
AudioMemoryOwner.Dispose();
}
}

/// <summary>
/// Finalizer - 관리되지 않는 리소스 정리 (최후 안전장치)
/// </summary>
~ChatSegment()
{
Dispose(false);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,103 @@ public void ChatSegment_MemoryOwner_vs_ByteArray_Test()
_output.WriteLine($"최적화 방식 - HasAudio: {segment2.HasAudio}, 데이터 크기: {segment2.AudioDataSize}");
}

[Fact]
public void ChatSegment_GetAudioSpan_SafetyBoundaryTest()
{
// 경계 조건 테스트: 유효한 범위 내에서의 메모리 접근 안전성 검증
var testData = GenerateTestAudioData(1000);
using var memoryOwner = MemoryPool<byte>.Shared.Rent(500); // 더 작은 메모리 할당
var actualMemorySize = memoryOwner.Memory.Length; // 실제 할당된 메모리 크기
var copySize = Math.Min(500, actualMemorySize);
testData.AsSpan(0, copySize).CopyTo(memoryOwner.Memory.Span);

// 유효한 크기로 설정 (실제 메모리 크기 이하)
var validSize = actualMemorySize - 10; // 안전한 크기
var segment = ChatSegment.CreateText("Test content")
.WithAudioMemory(memoryOwner, validSize, "audio/wav", 5.0f);

// GetAudioSpan이 정확한 크기를 반환해야 함
var span = segment.GetAudioSpan();

// 요청한 크기만큼 반환되어야 함
Assert.Equal(validSize, span.Length);
_output.WriteLine($"요청 크기: {validSize}, 실제 메모리: {actualMemorySize}, 반환된 span 크기: {span.Length}");
}
Comment on lines +91 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

소유권 이전 규약 위반: WithAudioMemory 사용 시 memoryOwner를 using으로 해제하면 이중 Dispose 위험

WithAudioMemory는 소유권을 ChatSegment로 이전합니다. 현재 using var memoryOwner는 테스트 종료 시 메모리 소유자를 다시 해제하므로 규약을 위반합니다.

적용 예(요지: using 제거, segment.Dispose()만 호출):

-using var memoryOwner = MemoryPool<byte>.Shared.Rent(500); // 더 작은 메모리 할당
+var memoryOwner = MemoryPool<byte>.Shared.Rent(500); // 소유권은 ChatSegment로 이전됨
 ...
-var segment = ChatSegment.CreateText("Test content")
-    .WithAudioMemory(memoryOwner, validSize, "audio/wav", 5.0f);
+var segment = ChatSegment.CreateText("Test content")
+    .WithAudioMemory(memoryOwner, validSize, "audio/wav", 5.0f);
+try
+{
+    var span = segment.GetAudioSpan();
+    Assert.Equal(validSize, span.Length);
+}
+finally
+{
+    segment.Dispose();
+}

레포 전역에서 동일 패턴을 점검하려면 다음을 실행해주세요:


🏁 Script executed:

#!/bin/bash
# WithAudioMemory 사용 주변의 using var memoryOwner 패턴 탐지
rg -nP -C3 $'using\\s+var\\s+\\w+\\s*=\\s*MemoryPool<byte>\\.Shared\\.Rent\\(.+\\)|\\.WithAudioMemory\\(' --type cs

Length of output: 9773


소유권 이전 규약 위반 — WithAudioMemory에 IMemoryOwner 전달 시 memoryOwner를 using으로 해제하면 이중 Dispose(오류) 발생 위험

WithAudioMemory가 메모리 소유권을 ChatSegment로 이전한다고 가정하면, 현재 테스트들이 MemoryPool에서 Rent한 IMemoryOwner를 using var로 감싼 뒤 WithAudioMemory(...)에 넘기고 있어 범위 종료 시 이중 Dispose(또는 ObjectDisposed 예외)가 발생할 가능성이 큽니다. 동일 패턴이 ProjectVG.Tests/Infrastructure/Integrations/MemoryPoolingPerformanceTests.cs 여러 위치(예: l.74, l.93, l.120, l.137, l.167)에서 발견됨 — 테스트 전체에서 패턴 수정 필요.

수정 방안(예): memoryOwner를 using으로 해제하지 말고 ChatSegment가 소유권을 해제하도록 segment.Dispose()를 호출(또는 ChatSegment를 using으로 감싸 자동 Dispose). 예전 제안된 변경(diff)은 유효합니다:

-using var memoryOwner = MemoryPool<byte>.Shared.Rent(500); // 더 작은 메모리 할당
+var memoryOwner = MemoryPool<byte>.Shared.Rent(500); // 소유권은 ChatSegment로 이전됨
 ...
-var segment = ChatSegment.CreateText("Test content")
-    .WithAudioMemory(memoryOwner, validSize, "audio/wav", 5.0f);
+var segment = ChatSegment.CreateText("Test content")
+    .WithAudioMemory(memoryOwner, validSize, "audio/wav", 5.0f);
+try
+{
+    var span = segment.GetAudioSpan();
+    Assert.Equal(validSize, span.Length);
+}
+finally
+{
+    segment.Dispose();
+}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
[Fact]
public void ChatSegment_GetAudioSpan_SafetyBoundaryTest()
{
// 경계 조건 테스트: 유효한 범위 내에서의 메모리 접근 안전성 검증
var testData = GenerateTestAudioData(1000);
using var memoryOwner = MemoryPool<byte>.Shared.Rent(500); // 더 작은 메모리 할당
var actualMemorySize = memoryOwner.Memory.Length; // 실제 할당된 메모리 크기
var copySize = Math.Min(500, actualMemorySize);
testData.AsSpan(0, copySize).CopyTo(memoryOwner.Memory.Span);
// 유효한 크기로 설정 (실제 메모리 크기 이하)
var validSize = actualMemorySize - 10; // 안전한 크기
var segment = ChatSegment.CreateText("Test content")
.WithAudioMemory(memoryOwner, validSize, "audio/wav", 5.0f);
// GetAudioSpan이 정확한 크기를 반환해야 함
var span = segment.GetAudioSpan();
// 요청한 크기만큼 반환되어야 함
Assert.Equal(validSize, span.Length);
_output.WriteLine($"요청 크기: {validSize}, 실제 메모리: {actualMemorySize}, 반환된 span 크기: {span.Length}");
}
[Fact]
public void ChatSegment_GetAudioSpan_SafetyBoundaryTest()
{
// 경계 조건 테스트: 유효한 범위 내에서의 메모리 접근 안전성 검증
var testData = GenerateTestAudioData(1000);
var memoryOwner = MemoryPool<byte>.Shared.Rent(500); // 소유권은 ChatSegment로 이전됨
var actualMemorySize = memoryOwner.Memory.Length; // 실제 할당된 메모리 크기
var copySize = Math.Min(500, actualMemorySize);
testData.AsSpan(0, copySize).CopyTo(memoryOwner.Memory.Span);
// 유효한 크기로 설정 (실제 메모리 크기 이하)
var validSize = actualMemorySize - 10; // 안전한 크기
var segment = ChatSegment.CreateText("Test content")
.WithAudioMemory(memoryOwner, validSize, "audio/wav", 5.0f);
try
{
// GetAudioSpan이 정확한 크기를 반환해야 함
var span = segment.GetAudioSpan();
// 요청한 크기만큼 반환되어야 함
Assert.Equal(validSize, span.Length);
_output.WriteLine($"요청 크기: {validSize}, 실제 메모리: {actualMemorySize}, 반환된 span 크기: {span.Length}");
}
finally
{
segment.Dispose();
}
}
🤖 Prompt for AI Agents
ProjectVG.Tests/Infrastructure/Integrations/MemoryPoolingPerformanceTests.cs
lines 88-109: the test currently wraps the IMemoryOwner returned by
MemoryPool<byte>.Shared.Rent(...) in a using, then passes that memoryOwner into
ChatSegment.WithAudioMemory, which transfers ownership and causes a
double-dispose risk; remove the using around memoryOwner (do not dispose it
directly) and instead ensure the ChatSegment created takes responsibility for
disposal by either calling segment.Dispose() at the end of the test or wrapping
the segment in a using so ChatSegment will release the IMemoryOwner; apply the
same pattern to the other occurrences noted (around lines 74, 93, 120, 137,
167).


[Fact]
public void ChatSegment_GetAudioSpan_EmptyAndNullSafetyTest()
{
// null AudioMemoryOwner는 이제 예외가 발생해야 함 (ArgumentNullException)
var nullException = Assert.Throws<ArgumentNullException>(() =>
ChatSegment.CreateText("Test").WithAudioMemory(null!, 100, "audio/wav", 1.0f));
Assert.Equal("audioMemoryOwner", nullException.ParamName);

// AudioDataSize가 0인 경우는 여전히 정상 작동해야 함
using var memoryOwner = MemoryPool<byte>.Shared.Rent(100);
var segment2 = ChatSegment.CreateText("Test").WithAudioMemory(memoryOwner, 0, "audio/wav", 1.0f);
var span2 = segment2.GetAudioSpan();
Assert.True(span2.IsEmpty);

// 기존 AudioData 방식 (null 허용)
var segment3 = ChatSegment.CreateText("Test").WithAudioData(null!, "audio/wav", 1.0f);
var span3 = segment3.GetAudioSpan();
Assert.True(span3.IsEmpty);

_output.WriteLine("null 검증과 빈 케이스가 모두 안전하게 처리됨");
}

[Fact]
public void ChatSegment_WithAudioMemory_ValidationTest()
{
var testData = GenerateTestAudioData(100);
using var memoryOwner = MemoryPool<byte>.Shared.Rent(100);
testData.CopyTo(memoryOwner.Memory.Span);

// 정상 케이스
var validSegment = ChatSegment.CreateText("Test")
.WithAudioMemory(memoryOwner, 50, "audio/wav", 1.0f);
Assert.Equal(50, validSegment.AudioDataSize);

// null audioMemoryOwner 테스트
var nullException = Assert.Throws<ArgumentNullException>(() =>
ChatSegment.CreateText("Test").WithAudioMemory(null!, 100, "audio/wav", 1.0f));
Assert.Equal("audioMemoryOwner", nullException.ParamName);

// audioDataSize < 0 테스트
var negativeException = Assert.Throws<ArgumentOutOfRangeException>(() =>
ChatSegment.CreateText("Test").WithAudioMemory(memoryOwner, -1, "audio/wav", 1.0f));
Assert.Equal("audioDataSize", negativeException.ParamName);

// audioDataSize > memory.Length 테스트
var oversizeException = Assert.Throws<ArgumentOutOfRangeException>(() =>
ChatSegment.CreateText("Test").WithAudioMemory(memoryOwner, memoryOwner.Memory.Length + 1, "audio/wav", 1.0f));
Assert.Equal("audioDataSize", oversizeException.ParamName);

_output.WriteLine("모든 소유권 이전 검증 테스트 통과");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

검증 테스트에서도 동일한 소유권 이전 규약 위반

정상 케이스 포함 모든 분기에서 using var memoryOwner를 제거하고, 소유권을 이전한 segment만 Dispose하세요.

적용 예:

-using var memoryOwner = MemoryPool<byte>.Shared.Rent(100);
+var memoryOwner = MemoryPool<byte>.Shared.Rent(100);
 ...
-var validSegment = ChatSegment.CreateText("Test")
-    .WithAudioMemory(memoryOwner, 50, "audio/wav", 1.0f);
+var validSegment = ChatSegment.CreateText("Test")
+    .WithAudioMemory(memoryOwner, 50, "audio/wav", 1.0f);
+try
+{
+    Assert.Equal(50, validSegment.AudioDataSize);
+}
+finally
+{
+    validSegment.Dispose();
+}

예외 경로에서는 WithAudioMemory 호출이 실패하므로 별도 Dispose 불필요합니다.

Committable suggestion skipped: line range outside the PR's diff.

[Fact]
public void ChatSegment_Dispose_MemoryOwnerReleaseTest()
{
var testData = GenerateTestAudioData(100);
using var memoryOwner = MemoryPool<byte>.Shared.Rent(100);
testData.CopyTo(memoryOwner.Memory.Span);

var segment = ChatSegment.CreateText("Test")
.WithAudioMemory(memoryOwner, 100, "audio/wav", 1.0f);

// Dispose 호출 전에는 정상 접근 가능
Assert.True(segment.HasAudio);
Assert.Equal(100, segment.GetAudioSpan().Length);

// Dispose 호출
segment.Dispose();

// 메모리가 해제되었으므로 ObjectDisposedException 발생할 수 있음
// (실제 구현에 따라 다를 수 있음)
_output.WriteLine("Dispose 호출 완료 - 메모리 소유자 해제됨");
}

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Dispose 시나리오 테스트의 소유권/해제 순서 정정

이 테스트도 using var memoryOwner로 이중 해제 위험이 있습니다. segment가 소유하므로 segment만 Dispose해야 합니다. 또한 Dispose 이후 접근 시 예외 던지도록 변경 시 해당 검증을 추가하세요.

-using var memoryOwner = MemoryPool<byte>.Shared.Rent(100);
+var memoryOwner = MemoryPool<byte>.Shared.Rent(100);
 ...
-var segment = ChatSegment.CreateText("Test")
-    .WithAudioMemory(memoryOwner, 100, "audio/wav", 1.0f);
+var segment = ChatSegment.CreateText("Test")
+    .WithAudioMemory(memoryOwner, 100, "audio/wav", 1.0f);
 
 // Dispose 호출 전에는 정상 접근 가능
 Assert.True(segment.HasAudio);
 Assert.Equal(100, segment.GetAudioSpan().Length);
 
 // Dispose 호출
-segment.Dispose();
+segment.Dispose();
 
-// 메모리가 해제되었으므로 ObjectDisposedException 발생할 수 있음
-// (실제 구현에 따라 다를 수 있음)
-_output.WriteLine("Dispose 호출 완료 - 메모리 소유자 해제됨");
+// 구현을 Dispose 후 접근 금지로 바꿨다면 다음도 검증:
+Assert.Throws<ObjectDisposedException>(() => segment.GetAudioSpan());
+_output.WriteLine("Dispose 호출 완료 - 메모리 소유자 해제됨");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
[Fact]
public void ChatSegment_Dispose_MemoryOwnerReleaseTest()
{
var testData = GenerateTestAudioData(100);
using var memoryOwner = MemoryPool<byte>.Shared.Rent(100);
testData.CopyTo(memoryOwner.Memory.Span);
var segment = ChatSegment.CreateText("Test")
.WithAudioMemory(memoryOwner, 100, "audio/wav", 1.0f);
// Dispose 호출 전에는 정상 접근 가능
Assert.True(segment.HasAudio);
Assert.Equal(100, segment.GetAudioSpan().Length);
// Dispose 호출
segment.Dispose();
// 메모리가 해제되었으므로 ObjectDisposedException 발생할 수 있음
// (실제 구현에 따라 다를 수 있음)
_output.WriteLine("Dispose 호출 완료 - 메모리 소유자 해제됨");
}
[Fact]
public void ChatSegment_Dispose_MemoryOwnerReleaseTest()
{
var testData = GenerateTestAudioData(100);
var memoryOwner = MemoryPool<byte>.Shared.Rent(100);
testData.CopyTo(memoryOwner.Memory.Span);
var segment = ChatSegment.CreateText("Test")
.WithAudioMemory(memoryOwner, 100, "audio/wav", 1.0f);
// Dispose 호출 전에는 정상 접근 가능
Assert.True(segment.HasAudio);
Assert.Equal(100, segment.GetAudioSpan().Length);
// Dispose 호출
segment.Dispose();
// 구현을 Dispose 후 접근 금지로 바꿨다면 다음도 검증:
Assert.Throws<ObjectDisposedException>(() => segment.GetAudioSpan());
_output.WriteLine("Dispose 호출 완료 - 메모리 소유자 해제됨");
}
🤖 Prompt for AI Agents
In ProjectVG.Tests/Infrastructure/Integrations/MemoryPoolingPerformanceTests.cs
around lines 163-184, the test currently uses "using var memoryOwner" causing
double-disposal since ChatSegment is intended to take ownership; remove the
using so the MemoryOwner is not disposed by the test, create and copy test data
into the rented memory, call segment.Dispose() (not disposing memoryOwner
separately), and then add an assertion that accessing the segment's audio after
Dispose throws ObjectDisposedException (or the appropriate disposal exception
for your implementation) to validate ownership transfer and post-dispose
behavior.

private byte[] GenerateTestAudioData(int size)
{
var random = new Random(12345); // 고정 시드로 일관된 테스트
Expand Down
Loading