#136 배포/종료 중 녹화 중단 시 partial 녹화 파일 정책 - #144
Merged
Merged
Conversation
- partial cleanup 구현 - TOCTOU 방어 1. 파일 삭제/로그 기록 전에 현재 redis 마커가 아직 같은 recordingId 인지 확인 2. 삭제 직전에도 recordingId 가 같을 때만 삭제
- 팀 리더 에이전트가 세션 리밋으로 죽었을 때 감지 처리 - 브랜치 분리 작업 중 미승인 push 하지못하도록 명문화 - "idempotency ≠ TOCTOU" 구분 확보
Contributor
Reviewer's GuideRedis 마커를 스캔하여 오래된 부분 녹화 파일(로컬 및 MinIO)을 삭제하고, 시스템 자동 삭제 내역을 DownloadLog에 기록하며, Redis 마커 연산을 TOCTOU에 안전하도록 강화한 예약 기반 부분 녹화 정리 파이프라인을 구현합니다. 이에 대한 포괄적인 단위/QA 테스트와 일부 내부 팀 프로세스 문서 업데이트도 포함됩니다. 부분 녹화 정리 배치 파이프라인 시퀀스 다이어그램sequenceDiagram
participant Scheduler as SpringScheduler
participant BatchJob as RecordingPartialCleanupBatchJob
participant Service as RecordingPartialCleanupService
participant Redis as RedisService
participant FileSvc as RecordingFileService
participant LogSvc as DownloadLogService
Scheduler->>BatchJob: cleanupPartialRecording()
BatchJob->>Service: cleanupExpiredPartialRecordings()
Service->>Redis: getAllRecordingPartialMarkers()
Redis-->>Service: List<RecordingPartialMarker>
loop for each marker
Service->>Redis: getRecordingPartialMarker(roomId)
Redis-->>Service: RecordingPartialMarker
alt [markedAt age >= cleanupAgeSeconds && recordingId matches]
Service->>FileSvc: deleteFileDir(minioFilePath) [via deleteMinioObjectIfPresent]
Service->>Service: deleteLocalFile(marker)
Service->>LogSvc: saveDownloadLog(DownloadLog.ofSystemAutoDeleted(marker))
Service->>Redis: deleteRecordingPartialMarkerIfRecordingIdMatches(roomId, recordingId)
Redis-->>Service: boolean
else [age not reached or marker changed]
Service->>Service: skipped++
end
end
Service-->>BatchJob: CleanupResult(deleted, skipped, failed)
BatchJob-->>Scheduler: (log result)
TOCTOU 안전 Redis 마커 삭제 시퀀스 다이어그램sequenceDiagram
participant Service as RecordingPartialCleanupService
participant RedisSvc as RedisServiceImpl
participant Redis as RedisOperations
Service->>RedisSvc: deleteRecordingPartialMarkerIfRecordingIdMatches(roomId, recordingId)
RedisSvc->>Redis: watch(key)
RedisSvc->>Redis: opsForValue().get(key)
Redis-->>RedisSvc: value
alt [value is RecordingPartialMarker && recordingId matches]
RedisSvc->>Redis: multi()
RedisSvc->>Redis: delete(key)
Redis-->>RedisSvc: queued
RedisSvc->>Redis: exec()
Redis-->>RedisSvc: List<Object> results
RedisSvc-->>Service: true
else [value null or recordingId different]
RedisSvc->>Redis: unwatch()
RedisSvc-->>Service: false
end
파일 수준 변경 사항
관련 가능 이슈
Tips and commandsSourcery와 상호작용
내 경험 맞춤 설정dashboard에 접속하여:
도움 받기Original review guide in EnglishReviewer's GuideImplements a scheduled partial recording cleanup pipeline that scans Redis markers, deletes stale partial recording files (local and MinIO), logs system auto-deletion in DownloadLog, and hardens Redis marker operations against TOCTOU, with comprehensive unit/QA tests and some internal team-process documentation updates. Sequence diagram for partial recording cleanup batch pipelinesequenceDiagram
participant Scheduler as SpringScheduler
participant BatchJob as RecordingPartialCleanupBatchJob
participant Service as RecordingPartialCleanupService
participant Redis as RedisService
participant FileSvc as RecordingFileService
participant LogSvc as DownloadLogService
Scheduler->>BatchJob: cleanupPartialRecording()
BatchJob->>Service: cleanupExpiredPartialRecordings()
Service->>Redis: getAllRecordingPartialMarkers()
Redis-->>Service: List<RecordingPartialMarker>
loop for each marker
Service->>Redis: getRecordingPartialMarker(roomId)
Redis-->>Service: RecordingPartialMarker
alt [markedAt age >= cleanupAgeSeconds && recordingId matches]
Service->>FileSvc: deleteFileDir(minioFilePath) [via deleteMinioObjectIfPresent]
Service->>Service: deleteLocalFile(marker)
Service->>LogSvc: saveDownloadLog(DownloadLog.ofSystemAutoDeleted(marker))
Service->>Redis: deleteRecordingPartialMarkerIfRecordingIdMatches(roomId, recordingId)
Redis-->>Service: boolean
else [age not reached or marker changed]
Service->>Service: skipped++
end
end
Service-->>BatchJob: CleanupResult(deleted, skipped, failed)
BatchJob-->>Scheduler: (log result)
Sequence diagram for TOCTOU safe Redis marker deletionsequenceDiagram
participant Service as RecordingPartialCleanupService
participant RedisSvc as RedisServiceImpl
participant Redis as RedisOperations
Service->>RedisSvc: deleteRecordingPartialMarkerIfRecordingIdMatches(roomId, recordingId)
RedisSvc->>Redis: watch(key)
RedisSvc->>Redis: opsForValue().get(key)
Redis-->>RedisSvc: value
alt [value is RecordingPartialMarker && recordingId matches]
RedisSvc->>Redis: multi()
RedisSvc->>Redis: delete(key)
Redis-->>RedisSvc: queued
RedisSvc->>Redis: exec()
Redis-->>RedisSvc: List<Object> results
RedisSvc-->>Service: true
else [value null or recordingId different]
RedisSvc->>Redis: unwatch()
RedisSvc-->>Service: false
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - 몇 가지 이슈 3개를 발견했고, 전반적인 피드백을 몇 가지 남겼습니다.
RedisServiceImpl.getAllRecordingPartialMarkers에서slaveTemplate.getConnectionFactory().getConnection()으로 얻은RedisConnection이 명시적으로 닫히지 않고 있습니다. 커넥션 누수를 방지하기 위해slaveTemplate.execute와RedisCallback을 사용하거나, 커넥션 자체에 대해 try-with-resources를 사용하는 방식을 고려해 주세요.- 현재 정리(cleanup) 정책(크론 표현식과
cleanupAgeSeconds기본값 14400)이 하드코딩되어 있습니다. 향후 정책 변경 시 코드 수정 없이 대응할 수 있도록, 스케줄과 기간 임계값 둘 다를 설정 프로퍼티로 외부화하는 편이 더 유연할 것 같습니다.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RedisServiceImpl.getAllRecordingPartialMarkers`, the `RedisConnection` obtained from `slaveTemplate.getConnectionFactory().getConnection()` is never explicitly closed; consider using `slaveTemplate.execute` with a `RedisCallback` or a try-with-resources on the connection itself to avoid leaking connections.
- The cleanup policy (cron expression and `cleanupAgeSeconds` default of 14400) is currently hardcoded; it would be more flexible to externalize both the schedule and age threshold into configuration properties so future policy changes don’t require code changes.
## Individual Comments
### Comment 1
<location path="springboot-backend/src/main/java/webChat/service/redis/impl/RedisServiceImpl.java" line_range="563-572" />
<code_context>
}
+ @Override
+ public boolean deleteRecordingPartialMarkerIfRecordingIdMatches(String roomId, String recordingId) {
+ String key = RECORDING_PARTIAL_PREFIX.getPrefix() + roomId;
+ Boolean deleted = masterTemplate.execute(new SessionCallback<Boolean>() {
+ @Override
+ public Boolean execute(@NotNull RedisOperations operations) {
+ operations.watch(key);
+ Object value = operations.opsForValue().get(key);
+ if (!(value instanceof RecordingPartialMarker marker)
+ || !Objects.equals(marker.getRecordingId(), recordingId)) {
+ operations.unwatch();
+ return false;
+ }
+
+ operations.multi();
+ operations.delete(key);
+ List<Object> results = operations.exec();
+ return results != null && !results.isEmpty() && Boolean.TRUE.equals(results.get(0));
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify transactional delete result handling to avoid silent failures on unexpected exec results.
Inside the transaction you assume `operations.delete(key)` will yield a single `Boolean` at `results.get(0)`. Depending on the `RedisTemplate` / driver, `exec()` may return different types (e.g., status strings, `Long`, or `null`) for delete operations. If that changes, this method may silently return `false` (or `true`) even when the actual delete outcome differs. Please validate the `results` size and element type (with an `instanceof` check) and either log or fail fast when the shape is unexpected to avoid silent misreports.
Suggested implementation:
```java
operations.multi();
operations.delete(key);
List<Object> results = operations.exec();
if (results == null || results.isEmpty()) {
// No results returned from exec, treat as failure
// and avoid silently reporting success
if (log.isWarnEnabled()) {
log.warn("Unexpected empty/null Redis transaction result when deleting recording partial marker for key {}", key);
}
return false;
}
if (results.size() != 1) {
// We only expect a single operation in this transaction
if (log.isWarnEnabled()) {
log.warn("Unexpected Redis transaction result size {} when deleting recording partial marker for key {}. Results: {}",
results.size(), key, results);
}
return false;
}
Object deleteResult = results.get(0);
if (deleteResult instanceof Boolean booleanResult) {
return booleanResult;
}
if (deleteResult instanceof Long longResult) {
// Redis DEL commonly returns the number of keys removed
return longResult > 0;
}
if (log.isWarnEnabled()) {
log.warn("Unexpected Redis DEL result type {} (value={}) when deleting recording partial marker for key {}",
deleteResult != null ? deleteResult.getClass().getName() : "null", deleteResult, key);
}
return false;
```
1. This change assumes that a logger named `log` is available in `RedisServiceImpl` (for example, `private static final Logger log = LoggerFactory.getLogger(RedisServiceImpl.class);`). If it is not already present, add such a logger following the existing logging conventions in this codebase.
2. If your project uses a different logging abstraction or message format, adjust the `log.warn` calls to match the existing style.
</issue_to_address>
### Comment 2
<location path="springboot-backend/src/test/java/webChat/service/recording/RecordingPartialCleanupServiceTest.java" line_range="67-76" />
<code_context>
+ return new CleanupResult(2, 1, 0);
+ }
+
+ @Test
+ @DisplayName("cleanupPartialRecording_서비스정상반환_예외없이카운트로깅하고종료")
+ void cleanupPartialRecording_serviceSucceeds_completesWithoutException() {
</code_context>
<issue_to_address>
**suggestion (testing):** cleanupExpiredPartialRecordings에서 마커 삭제 조건 실패(deleteRecordingPartialMarkerIfRecordingIdMatches=false) 경로를 검증하는 테스트가 누락된 것 같습니다
현재 구현에서는 `deleteRecordingPartialMarkerIfRecordingIdMatches`가 `false`를 반환하면 해당 건을 `deleted`가 아닌 `skipped`로 집계하고 warn 로그를 남기지만, 이 경로를 검증하는 테스트가 없습니다.
예시로는:
- expired 마커 1건에 대해 `deleteRecordingPartialMarkerIfRecordingIdMatches("room-1", "rec-1")`가 `false`를 반환하도록 설정
- `cleanupExpiredPartialRecordings()` 실행 후 `deleted=0`, `skipped=1`, `failed=0`을 기대
정도가 있을 것 같습니다.
이 케이스를 추가하면 "파일은 삭제되었지만 경쟁 조건 등으로 마커 삭제 조건이 불일치한 경우"에도 집계/로깅 동작이 의도대로 유지되는지 회귀 테스트가 가능해질 것 같습니다.
</issue_to_address>
### Comment 3
<location path="plan_docs/00-base_plan/2026/07/bug_136_recording_partial_cleanup_plan.md" line_range="65-74" />
<code_context>
+- **(OPEN-2 확정)**: `DownloadStatus` 신규 값 = **`SYSTEM_AUTO_DELETED`** (유저 직접 지정).
</code_context>
<issue_to_address>
**issue (bug_risk):** `DownloadStatus` 값 표기 불일치 (`SYSTEM_AUTO_DELETED` vs `SYSTEM_DELETED`)
여기서는 신규 값이 `SYSTEM_AUTO_DELETED`로 적혀 있는데, 아래 예시에서는 `SYSTEM_DELETED`를 사용하고 있습니다. 실제 enum 상수명을 무엇으로 쓸지 결정한 뒤, 문서/코드 전체에서 동일한 이름으로 맞춰 주세요.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey - I've found 3 issues, and left some high level feedback:
- In
RedisServiceImpl.getAllRecordingPartialMarkers, theRedisConnectionobtained fromslaveTemplate.getConnectionFactory().getConnection()is never explicitly closed; consider usingslaveTemplate.executewith aRedisCallbackor a try-with-resources on the connection itself to avoid leaking connections. - The cleanup policy (cron expression and
cleanupAgeSecondsdefault of 14400) is currently hardcoded; it would be more flexible to externalize both the schedule and age threshold into configuration properties so future policy changes don’t require code changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `RedisServiceImpl.getAllRecordingPartialMarkers`, the `RedisConnection` obtained from `slaveTemplate.getConnectionFactory().getConnection()` is never explicitly closed; consider using `slaveTemplate.execute` with a `RedisCallback` or a try-with-resources on the connection itself to avoid leaking connections.
- The cleanup policy (cron expression and `cleanupAgeSeconds` default of 14400) is currently hardcoded; it would be more flexible to externalize both the schedule and age threshold into configuration properties so future policy changes don’t require code changes.
## Individual Comments
### Comment 1
<location path="springboot-backend/src/main/java/webChat/service/redis/impl/RedisServiceImpl.java" line_range="563-572" />
<code_context>
}
+ @Override
+ public boolean deleteRecordingPartialMarkerIfRecordingIdMatches(String roomId, String recordingId) {
+ String key = RECORDING_PARTIAL_PREFIX.getPrefix() + roomId;
+ Boolean deleted = masterTemplate.execute(new SessionCallback<Boolean>() {
+ @Override
+ public Boolean execute(@NotNull RedisOperations operations) {
+ operations.watch(key);
+ Object value = operations.opsForValue().get(key);
+ if (!(value instanceof RecordingPartialMarker marker)
+ || !Objects.equals(marker.getRecordingId(), recordingId)) {
+ operations.unwatch();
+ return false;
+ }
+
+ operations.multi();
+ operations.delete(key);
+ List<Object> results = operations.exec();
+ return results != null && !results.isEmpty() && Boolean.TRUE.equals(results.get(0));
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Clarify transactional delete result handling to avoid silent failures on unexpected exec results.
Inside the transaction you assume `operations.delete(key)` will yield a single `Boolean` at `results.get(0)`. Depending on the `RedisTemplate` / driver, `exec()` may return different types (e.g., status strings, `Long`, or `null`) for delete operations. If that changes, this method may silently return `false` (or `true`) even when the actual delete outcome differs. Please validate the `results` size and element type (with an `instanceof` check) and either log or fail fast when the shape is unexpected to avoid silent misreports.
Suggested implementation:
```java
operations.multi();
operations.delete(key);
List<Object> results = operations.exec();
if (results == null || results.isEmpty()) {
// No results returned from exec, treat as failure
// and avoid silently reporting success
if (log.isWarnEnabled()) {
log.warn("Unexpected empty/null Redis transaction result when deleting recording partial marker for key {}", key);
}
return false;
}
if (results.size() != 1) {
// We only expect a single operation in this transaction
if (log.isWarnEnabled()) {
log.warn("Unexpected Redis transaction result size {} when deleting recording partial marker for key {}. Results: {}",
results.size(), key, results);
}
return false;
}
Object deleteResult = results.get(0);
if (deleteResult instanceof Boolean booleanResult) {
return booleanResult;
}
if (deleteResult instanceof Long longResult) {
// Redis DEL commonly returns the number of keys removed
return longResult > 0;
}
if (log.isWarnEnabled()) {
log.warn("Unexpected Redis DEL result type {} (value={}) when deleting recording partial marker for key {}",
deleteResult != null ? deleteResult.getClass().getName() : "null", deleteResult, key);
}
return false;
```
1. This change assumes that a logger named `log` is available in `RedisServiceImpl` (for example, `private static final Logger log = LoggerFactory.getLogger(RedisServiceImpl.class);`). If it is not already present, add such a logger following the existing logging conventions in this codebase.
2. If your project uses a different logging abstraction or message format, adjust the `log.warn` calls to match the existing style.
</issue_to_address>
### Comment 2
<location path="springboot-backend/src/test/java/webChat/service/recording/RecordingPartialCleanupServiceTest.java" line_range="67-76" />
<code_context>
+ return new CleanupResult(2, 1, 0);
+ }
+
+ @Test
+ @DisplayName("cleanupPartialRecording_서비스정상반환_예외없이카운트로깅하고종료")
+ void cleanupPartialRecording_serviceSucceeds_completesWithoutException() {
</code_context>
<issue_to_address>
**suggestion (testing):** cleanupExpiredPartialRecordings에서 마커 삭제 조건 실패(deleteRecordingPartialMarkerIfRecordingIdMatches=false) 경로를 검증하는 테스트가 누락된 것 같습니다
현재 구현에서는 `deleteRecordingPartialMarkerIfRecordingIdMatches`가 `false`를 반환하면 해당 건을 `deleted`가 아닌 `skipped`로 집계하고 warn 로그를 남기지만, 이 경로를 검증하는 테스트가 없습니다.
예시로는:
- expired 마커 1건에 대해 `deleteRecordingPartialMarkerIfRecordingIdMatches("room-1", "rec-1")`가 `false`를 반환하도록 설정
- `cleanupExpiredPartialRecordings()` 실행 후 `deleted=0`, `skipped=1`, `failed=0`을 기대
정도가 있을 것 같습니다.
이 케이스를 추가하면 "파일은 삭제되었지만 경쟁 조건 등으로 마커 삭제 조건이 불일치한 경우"에도 집계/로깅 동작이 의도대로 유지되는지 회귀 테스트가 가능해질 것 같습니다.
</issue_to_address>
### Comment 3
<location path="plan_docs/00-base_plan/2026/07/bug_136_recording_partial_cleanup_plan.md" line_range="65-74" />
<code_context>
+- **(OPEN-2 확정)**: `DownloadStatus` 신규 값 = **`SYSTEM_AUTO_DELETED`** (유저 직접 지정).
</code_context>
<issue_to_address>
**issue (bug_risk):** `DownloadStatus` 값 표기 불일치 (`SYSTEM_AUTO_DELETED` vs `SYSTEM_DELETED`)
여기서는 신규 값이 `SYSTEM_AUTO_DELETED`로 적혀 있는데, 아래 예시에서는 `SYSTEM_DELETED`를 사용하고 있습니다. 실제 enum 상수명을 무엇으로 쓸지 결정한 뒤, 문서/코드 전체에서 동일한 이름으로 맞춰 주세요.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
…al 녹화 정리 배치 버그수정 - sourcery-ai 리뷰에 따른 코드 수정 - 방 삭제 배치로 인해 roomId 와 관련된 모든 redis key 가 삭제되어 partial 녹화 정리 배치가 정상 실행되지 않는 버그 수정
SeJonJ
temporarily deployed
to
chatforyou-back-env
July 11, 2026 16:30 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
…Id/recordingId/fileId 로 삭제되도록 수정
SeJonJ
temporarily deployed
to
chatforyou-back-env
July 12, 2026 05:44 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
Owner
Author
|
26.07.12 녹화 파일 삭제확인 완료 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
TOCTOU-safe Redis 마커 처리와 시스템 자동 삭제 로깅을 포함한 부분 녹화 파일에 대한 스케줄 기반 정리 기능을 추가했습니다.
New Features:
RecordingPartialCleanupService및 스케줄 배치 잡을 도입했습니다.recordingId기반으로 녹화 부분 마커를 조회하고 조건부로 삭제할 수 있는 Redis 스캔 API를 추가했습니다.SYSTEM_AUTO_DELETED상태와 팩토리를DownloadLog에 확장 추가했습니다.Enhancements:
bug/136브랜치에서 실행될 수 있도록 CI 배포 워크플로를 개선했습니다.CI:
bug/136브랜치를 포함하도록 GitHub Actions k8s 배포 워크플로를 업데이트했습니다.Tests:
DownloadLog시스템 자동 삭제 매핑 및 배치 잡 예외 처리 검증 테스트를 추가했습니다.Original summary in English
Summary by Sourcery
Add scheduled cleanup for partial recording files with TOCTOU-safe Redis marker handling and system auto-delete logging.
New Features:
Enhancements:
CI:
Tests: