Repository navigation
fix(client): retry SourceDataLine initialization to prevent silent playback - #65
Conversation
…ayback Add a retry mechanism (10 attempts, 100ms delay) in JavaSoundOutput.tryOpen() to handle LineUnavailableException caused by unreleased audio lines during song transitions (Fixes #64). Update ClientMusicFailReporter to map the new NETWORK_FAILED and HTTP_PERMANENT_FAILED failure codes. Bump mod version from 3.0.1 to 3.0.2.
There was a problem hiding this comment.
🌸 Sakura AI 审查报告 - 🔍 标准审查
❌ 请求变更
📋 查看详细审查报告
审查总结(Issue #64 修复:断流恢复 + 重试初始化)
本次 PR 对音乐播放链路做了较大重构,整体方向正确、工程质量较高:
做得好的方面
- 恢复循环 + 快照位置续播的设计合理,
CountDownLatch等待旧 worker 退出解决切歌时SourceDataLine竞争(含连切复查与 2s/500ms 兜底),所有等待都在新线程完成,不阻塞 MC 主线程。 - 失败码二分(网络型 NETWORK/STREAM 不投 quorum 票,音源型 PERMANENT/DECODE 参与聚合)正确对症 Issue #64 的"一次瞬断即跳歌"症状,配套新增 2 个单元测试覆盖单人/多人场景。
MusicPlaybackFailedPayload枚举越界以DecoderException拒绝并有编解码测试;lang 键中英文齐备。
必须修复的问题(major)
MusicPlayer.streamAttempt中缓存落盘发生在finally,而"提前 EOF 截断检测"在其后——半截 MP3 会先被finalizePartFile转正为{songId}.mp3,再返回 RETRY,与注释"避免半截音频被缓存落盘"直接矛盾。恢复耗尽或中途退出后,截断缓存将长期存续(256MB LRU 才淘汰),后续播放命中后要么提前"播完"(HUD 钉死在终点),要么触发BitstreamException整曲静音。
次要问题(minor)
2. 本地缓存路径下 streamAttempt 返回 RETRY(缓存损坏触发 BitstreamException 被归类 streamBroken)时被 reportFinal 的 RETRY 分支静默吞掉:不上报、不删坏缓存、不回退网络重取,整曲无声音且服务端无任何信号;与缺陷 1 组合形成"截断缓存 → 持续复发"链路。
3. 新增 INFO 日志(MusicServerBridge 失败终态/失败上报)直接输出 song.title()、requesterName()、玩家名等用户可编辑字段,无脱敏与控制字符过滤,恶意客户端可伪造带换行的 title 注入日志;项目规则"日志脱敏全级别统一"要求统一入口。
建议(suggestion)
4. FailureCode 枚举线上格式为 ordinal,本次重排/替换了常量(STREAM_INTERRUPTED 2→1 等)但 ModNetwork.NETWORK_VERSION 未变;当前靠 FireflyMCMod.VERSION 握手断开保护,建议同步 bump NETWORK_VERSION 作纵深防御。
5. EARLY_EOF_TOLERANCE_MS=3s 低于 Mp3DurationProbe CBR 估算对 VBR 歌曲的潜在误差,正常播完可能被误判为提前 EOF,触发最多 2 次全量重下 + 误弹"网络波动"提示,建议提高容差或对早 EOF 恢复单独限次。
🟡 src/main/java/firefly520/fireflymc/client/music/MusicPlayer.java:307-314 · major
提前 EOF 截断的 .part 先被 finalize 落盘,半截音频污染有效缓存
streamAttempt 的 finally(第 307-314 行)在早 EOF 检测(第 322-329 行)之前执行:CDN 提前断流时 completed=true、tee 分支健康,截断的 .part 会先被 finalizePartFile 转正为 {songId}.mp3,然后才返回 RETRY。这与第 322-323 行注释"按可恢复断流处理,避免半截音频被缓存落盘"直接矛盾。后果:恢复循环耗尽(或玩家中途退出)后,截断文件作为有效缓存长期存续(MusicCache 仅按 256MB LRU 淘汰);后续播放该歌命中缓存后走本地模式(第 99-109 行),本地模式明确不做早 EOF 检测,结果是整服提前"播完"、HUD 钉在终点,或触发 BitstreamException 整曲静音——正是 Issue #64 抱怨的"没声音"以新形态复发。
Suggestion:
// 缓存只在完整播完(EOF)、缓存分支健康、且非提前 EOF 截断时落盘;
// 失败/截断 attempt 的 .part 一律删除:既保证下一 attempt 的 beginPartFile
//(Files.createFile 语义)不冲突,也不让半截音频污染有效缓存
if (!localMode && completed && !silentDegraded && partFile != null
&& (tee == null || !tee.isBranchBroken())
&& finalPos >= durationMs - EARLY_EOF_TOLERANCE_MS) {
cache.finalizePartFile(partFile, songId);
} else {
cache.deletePartFile(partFile);
}
🔵 src/main/java/firefly520/fireflymc/client/music/MusicPlayer.java:100-109 · minor
本地缓存损坏/截断触发 RETRY 被 reportFinal 静默吞掉,无声音且无任何恢复
缓存命中路径直接 reportFinal(streamAttempt(in, basePositionMs), basePositionMs)。本地文件解码遇到损坏帧时抛 BitstreamException,第 287 行 streamBroken = stalled || e instanceof BitstreamException 判为 true(本地模式 httpStream==null,stalled 恒 false),返回 RETRY;而 reportFinal 的 RETRY 分支(第 155-156 行)为空操作——既不 onFinished 也不 onLocalFailure,坏缓存也不删除。结果:该曲整段静音、HUD 空转到权威时长、服务端收不到任何失败信号,且下次播放继续命中同一坏缓存无限复发。与 major 缺陷 1 组合后这是可达路径(截断缓存转正→命中→RETRY 吞掉)。修复方向:本地模式收到 RETRY 时删除该 songId 的缓存文件并回退到网络恢复循环(需要 MusicCache 提供"按 songId 删除缓存"的 API,属跨方法改动,故不给一键替换)。
🔵 src/main/java/firefly520/fireflymc/music/MusicServerBridge.java:64-66 · minor
新增日志输出未脱敏的用户可编辑字段(title/requesterName/玩家名)
本次新增的两处 INFO 日志(第 64-66 行失败终态记录 song.title()/song.requesterName(),第 134-136 行失败上报记录 reporter.getGameProfile().getName())均直接输出用户可编辑字段,无脱敏、无换行/控制字符过滤。客户端代搜索链路中 title 来自客户端回包(仅截断长度),恶意客户端可伪造含 \n/\r 的 title 实现日志注入与日志伪造;项目规则"日志脱敏全级别统一(major)"与项目记忆均要求新增日志统一走脱敏入口。建议封装统一的 logSafe/脱敏辅助(剥离控制字符、转义换行、超长截断)后统一替换音乐模块所有输出用户字段的日志点。
💡 src/main/java/firefly520/fireflymc/network/MusicPlaybackFailedPayload.java:18-27 · suggestion
线上枚举按 ordinal 编码但常量被重排,NETWORK_VERSION 未同步提升
STREAM_CODEC 以 failureCode.ordinal() 上线。本次枚举从 HTTP_FAILED(0), SOURCE_UNAVAILABLE(1), STREAM_INTERRUPTED(2), MP3_DECODE_FAILED(3) 变为 NETWORK_FAILED(0), STREAM_INTERRUPTED(1), HTTP_PERMANENT_FAILED(2), MP3_DECODE_FAILED(3),同一 ordinal 跨版本语义改变(如旧客户端发 STREAM_INTERRUPTED=2,会被新服务端解读为 HTTP_PERMANENT_FAILED 并计入 quorum——恰好复活本 PR 要修的误跳歌)。当前由 FireflyMCMod.VERSION 3.0.1→3.0.2 握手不匹配断开兜底,风险已被规避,但 ModNetwork.NETWORK_VERSION(仍为 1.3.0)作为协议版本未动。建议约定:线上枚举只增不改序,或每次改动线上枚举同步 bump NETWORK_VERSION 作纵深防御。
💡 src/main/java/firefly520/fireflymc/client/music/MusicPlayer.java:34-35 · suggestion
EARLY_EOF_TOLERANCE_MS=3s 可能低于时长探测误差,引发误恢复循环
Mp3DurationProbe.probeDurationMs 是按首帧 bitrate 的 CBR 估算(VBR 歌曲误差可轻松超过 3s,探测失败时 fallback 固定 240s 更不可信)。当权威 durationMs 高估超过 EARLY_EOF_TOLERANCE_MS 时,正常播完会被判为提前 EOF:每次播放触发最多 2 次全量重下 + discard 快进(带宽/CPU 浪费),并向玩家误弹"网络波动,正在重新连接音乐流…"提示。建议提高容差(如 5-10s)、或对"提前 EOF"类恢复单独限次(与网络异常恢复共享 3 次上限会放大抖动),有条件时优先采用字节级(Content-Length 对账)而非毫秒级截断判定。
代码质量评分:6/10
AI审查决策:需要修复后重新提交
阻断原因:存在 1 个 major 缺陷:提前 EOF 截断的半截 .part 会先被 finalize 进缓存再判定 RETRY,直接违反本 PR 自己声明的"避免半截音频被缓存落盘"不变量,且被污染的缓存会在后续播放中持续造成提前结束/整曲静音。另有 2 个 minor(本地缓存损坏路径被静默吞掉、新增日志未脱敏)与 2 个 suggestion。按项目规则 major 必须request_changes,修复点集中且有明确一键修复方案。
🟡 重要问题 (1个)
- 提前 EOF 截断的 .part 先被 finalize 落盘,半截音频污染有效缓存
🔵 次要问题 (2个)
-
本地缓存损坏/截断触发 RETRY 被 reportFinal 静默吞掉,无声音且无任何恢复
-
新增日志输出未脱敏的用户可编辑字段(title/requesterName/玩家名)
💡 优化建议 (2条)
-
线上枚举按 ordinal 编码但常量被重排,NETWORK_VERSION 未同步提升
-
EARLY_EOF_TOLERANCE_MS=3s 可能低于时长探测误差,引发误恢复循环
🏷️ 标签建议
✅ 已自动应用的标签
- bug (98%) - PR标题明确以'fix'开头,代码变更重试了SourceDataLine的初始化逻辑并增加了失败报告机制,旨在修复静默播放的错误。
此评论由 Sakura AI 自动生成。
…ry position - Call player.abandon() to release completion latch when worker is superseded by a new playback, preventing the next song from blocking indefinitely on awaitExit() - Replace JavaSound clock with authoritative monotonic clock for stream recovery position, as JavaSound clock freezes during stalls causing recovery to lag tens of seconds behind server position - Add MusicCache.invalidate() to delete corrupted cache files without affecting in-progress .part downloads - Increase EARLY_EOF_TOLERANCE_MS from 3s to 8s to accommodate duration estimation errors and line buffer latency - Track last stream scheme and detect HTTPS stalls to force original URL strategy on recovery attempts
There was a problem hiding this comment.
🌸 Sakura AI 审查报告 - ⚡️ 快速审查
💬 审查评论
📋 查看详细审查报告
审查总结(Issue #64 修复 · 增量第二轮)
上一轮发现闭环情况(全部有效修复)
- ✅ 早 EOF 截断 .part 先落盘:
earlyEof判定移入 try 块(第 320-324 行),finally 中validComplete = completed && !earlyEof && !silentDegraded拦截转正,注释与实现恢复一致。 - ✅ 本地坏缓存 RETRY 被静默吞掉:缓存命中路径对非 COMPLETED/CANCELLED 结果执行
cache.invalidate(songId)并落入网络恢复循环(warn 日志 + HUD 回调齐全),配套MusicCache.invalidate单测。 - ✅ FailureCode ordinal 上线:改为显式
wireId,与 3.0.1 旧 ordinal 交叉对齐(0/2/3 落网络型/相同语义,10 为全新值),LAN 集成服务器混跑场景安全;NETWORK_VERSION升至 1.4.0;failureCodeWireIdsAreStable+ 越界拒绝测试覆盖。 - ✅ 日志注入:新增
sanitize()(剔除\p{Cntrl})用于 title/requesterName。 - ✅ 连切 latch 永不释放:
abandon()在两处 stale 检查路径 countDown 完成信号,避免下一首白等 2s。 - ✅ 断流恢复真值改为权威单调时钟
authPositionMs()+ 快进 liveTarget 实时追平 + 起播重锚定,解决停滞 30s 恢复后落后服务端数十秒的问题;容差 3s→8s。
遗留问题(非阻断)
MusicServerBridge本次只给失败终态日志加了sanitize(),同 PR 新增的另一处日志(onClientFailure打印reporter.getGameProfile().getName())仍是原始输出——虽然原版登录对玩家名有字符集限制、风险较低,但违背项目"同文件日志脱敏全级别统一"规则的一致性要求,建议一并套用sanitize()。HttpSchemePolicy.ORIGINAL使断流恢复路径固定走明文 http;底层HTTP客户端仍为Redirect.ALWAYS且openFollowing返回后不校验最终 scheme,PREFER_HTTPS 的升级请求也可能被静默 302 降级。属已知历史债务,本 PR 加重了明文路径依赖,建议后续统一在跟随重定向后校验最终 URI scheme。- 早 EOF 判定依赖权威
durationMs:探测三连败时 fallback 固定 240s,实际时长偏差大的歌(非 4 分钟)会每次播放误触发恢复(全量重下 + 误弹"网络波动"提示 + 尾段静音),建议对 fallback 时长单独处理或提高容差/限次;相关阈值目前仍为硬编码。
整体修复质量高,方向正确,测试补充到位,可以合并后跟进上述事项。
💡 src/main/java/firefly520/fireflymc/client/music/MusicPlayer.java:36-41 · suggestion
早 EOF 判定在 fallback 时长(240s)下会对正常播完误触发恢复循环
早 EOF 判定(第 322 行)以权威 durationMs 为基准:时长探测三连败时服务端入队的是固定 fallback 240s(Mp3DurationProbe.FALLBACK_DURATION_MS)。实际时长与 4 分钟偏差超过 8s 的歌(很常见)会在真实 EOF 处被判为"提前结束",每次播放触发最多 2 次全量重下 + 快进、误弹"网络波动"action bar 提示,尾段静音到 240s。虽属低频路径(探测失败才发生)且降级有界,但用户感知为每次都"播放异常"。注释已规划字节级完整性判断(Content-Length 对账),可一并落地;同时 STREAM_RECOVERY_ATTEMPTS/RETRY_BACKOFF_MS/EARLY_EOF_TOLERANCE_MS 等阈值均为硬编码,按项目"容错阈值配置化"规则宜提取配置。
代码质量评分:8/10
AI审查决策:请人工复审
🔵 次要问题 (2个)
-
MusicServerBridge 另一处新增日志(失败上报)未套用 sanitize,脱敏覆盖不完整
-
ORIGINAL 策略加重明文 http 依赖,重定向后最终 scheme 仍未校验
💡 优化建议 (1条)
- 早 EOF 判定在 fallback 时长(240s)下会对正常播完误触发恢复循环
🏷️ 标签建议
✅ 已自动应用的标签
- test (95%) - Added new test cases
invalidateDeletesOnlyCompletedin MusicCacheTest andfailureCodeWireIdsAreStablein MusicPayloadCodecTest to verify the new cache invalidation logic and network protocol stability.
💡 建议的标签(需确认)
- documentation (45%) - Added Javadoc comments for new methods such as
invalidate()andHttpSchemePolicy, and updated protocol version documentation in ModNetwork.
注:勾选复选框即可应用标签,取消勾选即可移除标签(仅 PR 作者或仓库管理员/协作者可操作)
此评论由 Sakura AI 自动生成。
- Pre-compile `TIME_TAG` regex pattern in `LrcParser` to avoid hot path recompilation. - Move `MusicCache` stale `.part` cleanup from the constructor to a lazy `ensureInitialized()` method, preventing main thread I/O blocking and Windows-specific first-play lag. - Defer LRC parsing in `MusicPlaybackManager.start()` to a background thread to eliminate frame-time spikes during song transitions.
There was a problem hiding this comment.
🌸 Sakura AI 审查报告 - ⚡️ 快速审查
✅ 审查通过
📋 查看详细审查报告
审查总结(Issue #64 修复 · 增量第三轮:主线程 O(1) 化)
本增量验证结果
- 构造零 I/O + 延迟清理:
MusicCache构造不再扫描目录,ensureInitialized()(synchronized + 一次性标志)由首个 worker 在beginPartFile之前调用——串行播放约束(新 worker 先等旧 worker 退出)保证了绝不误删正在写的 .part,幂等测试覆盖到位。 - LRC 后台解析:虚拟线程解析 +
Minecraft.getInstance().execute主线程原子替换,双重 playbackId 校验(队列检查 + PlayingInfo 比对)在主线程单线程语义下无竞态;晚到的解析结果不会覆盖 onFinished 钉终点的时钟(保留了info.clock()引用)。 - cancel() 异步化:删除同步
closeQuietly(teeBranch)/deletePartFile是安全的——逐步核实所有取消路径(read 阻塞被 async close 解除、waitSilent/tryOpen 被 interrupt 唤醒、连击 abandon)最终都经 worker 的 finally 走 cancelled→delete 分支,且 teeBranch 先关闭再删 .part 的顺序满足 Windows 句柄约束;双线程并发 close 同一流已被 closeQuietly 吞掉,无害。 - 早 EOF 也置
httpsStalled:与流中断路径对称,合理。
前两轮发现闭环状态:早 EOF 判定前移、坏缓存 invalidate、wireId + NETWORK_VERSION bump、失败终态脱敏、abandon() 均已修复且本轮未回退。
遗留跟进项(均不在本次变更行内,非阻断)
MusicServerBridge.onClientFailure的失败上报日志仍未套用sanitize()(上上轮 minor,文件本轮未改)。HttpClient.Redirect.ALWAYS+openFollowing不校验最终 scheme 的明文降级债务(历史 major 规则挂账,需批量整改)。- fallback 时长(240s)与 8s 容差组合在探测失败的歌上仍会误触发恢复循环(历史 suggestion)。
logSlow为临时观测(注释已自证),确认修复后应降级或移除,避免生产 WARN 噪音。
💡 src/main/java/firefly520/fireflymc/client/music/MusicPlaybackManager.java:148-171 · suggestion
后台 LRC 解析无线程兜底:parse 意外异常时歌词静默缺失且未记日志
parseLrcAsync 的虚拟线程直接调用 LrcParser.parse(lrcText),无任何 catch。若解析抛出意外 RuntimeException(lrc 来自客户端代搜索回包,属不可信输入,经服务端仅做长度截断),虚拟线程将带默认 uncaught handler 退出:不产生模块日志(走 stderr)、Minecraft.getInstance().execute 永不执行,该曲歌词保持空白直到切歌,且无任何可追踪记录。按项目规则"异常分支不可空/catch 必须统一记录日志",建议补 try/catch 兜底:记录 error 后直接放弃本次替换(HUD 保持空歌词为可接受降级)。当前正则与 parseLong 均有输入约束、实际触发概率低,故评 suggestion。
Suggestion:
/** 后台解析歌词(virtual thread,不占主线程),完成后在 playbackId 仍匹配时原子替换 */
private static void parseLrcAsync(long playbackId, String lrcText) {
Thread.ofVirtual().name("fireflymc-music-lrc").start(() -> {
long t0 = System.nanoTime();
java.util.TreeMap<Long, String> parsed;
try {
parsed = LrcParser.parse(lrcText);
} catch (RuntimeException e) {
// 解析意外失败(lrc 为不可信输入):记日志放弃替换,HUD 保持空歌词
firefly520.fireflymc.FireflyMCMod.LOGGER.error(
"[Music] LRC 后台解析异常 playbackId={}", playbackId, e);
return;
}
long costUs = (System.nanoTime() - t0) / 1_000L;
if (costUs > SLOW_LOG_THRESHOLD_US) {
firefly520.fireflymc.FireflyMCMod.LOGGER.warn(
"[Music] LRC 后台解析耗时 {}μs lines={}", costUs, parsed.size());
}
Minecraft.getInstance().execute(() -> {
if (playbackId != currentPlaybackId) {
return; // 已切歌:丢弃过期解析结果
}
MusicPlaybackState.PlayingInfo info = MusicPlaybackState.current();
if (info != null && info.playbackId() == playbackId) {
// 原子替换歌词快照(clock 引用不变 → HUD 时钟连续)
MusicPlaybackState.setPlaying(new MusicPlaybackState.PlayingInfo(
info.playbackId(), info.songId(), info.title(), info.author(),
info.requesterName(), info.durationMs(), parsed, info.clock()));
}
});
});
}
代码质量评分:9/10
AI审查决策:批准合并
💡 优化建议 (1条)
- 后台 LRC 解析无线程兜底:parse 意外异常时歌词静默缺失且未记日志
🏷️ 标签建议
✅ 已自动应用的标签
- performance (95%) - 本次增量提交明确标识为性能优化(perf:),通过预编译正则表达式和延迟执行清理操作,减少了主线程阻塞和热路径开销。
此评论由 Sakura AI 自动生成。
- Add cache format versioning (v2) to detect and wipe potentially truncated MP3 files left over from version 3.0.1 - Implement safeResolve to prevent directory traversal attacks via malicious songId inputs - Update getCachedFile and beginPartFile to use the new path validation - Add logs/ to .gitignore
There was a problem hiding this comment.
🌸 Sakura AI 审查报告 - ⚡️ 快速审查
✅ 审查通过
📋 查看详细审查报告
审查总结(Issue #64 修复 · 增量第四轮:字节级完整性 + 缓存格式迁移 + 安全加固)
本增量验证结果
- 字节级截断判定:
expectedBytes取自最终响应的 Content-Length,guard.bytesRead()在两种 read 重载中正确累计(含 BufferedInputStream 预取——EOF 时缓冲已耗尽,计数恰好等于 Content-Length);byteVerified闸门确保无 Content-Length 的响应可播但绝不落盘。上轮的 fallback 时长误恢复问题随之消除(truncated只看字节差,240s 估算失真不再触发恢复)。 - v2 格式迁移:标记缺失/损坏视为 v1 → 全量作废 .mp3 与 .part 并写 v2 标记,清掉 3.0.1 时期可能落盘的半截缓存;标记写入失败仅导致下次重扫,幂等无害。两个迁移方向均有单测。
- 路径逃逸防护:
safeResolve(resolve+normalize+绝对路径 startsWith)覆盖 getCachedFile/beginPartFile/finalizePartFile/invalidate 全部入口,绝对路径与..均被拒绝;songId 此前已通过服务端与客户端isValidSongId双重校验,此为第三层防御。 - LRC 兜底:try/catch + error 日志已按上轮建议落实;慢日志降 DEBUG 合理。
- 虚拟线程化:worker 改虚拟线程在功能上安全(lambda 仅读 volatile 的 currentPlaybackId,已先于线程启动赋值;interrupt/join/latch 均兼容虚拟线程;未启动即被 stop 的场景经 abandon() 放行信号)。
遗留跟进项(均不在本次变更行内)
MusicServerBridge.onClientFailure失败上报日志未套用sanitize()(incr2 提出的 minor,连续多轮未动,建议随下次服务端侧改动一并处理)。HttpClient.Redirect.ALWAYS+openFollowing不校验最终 scheme 的明文降级债务(历史规则挂账,注意本增量 ORIGINAL 策略进一步强化了明文路径依赖)。logSlow内联硬编码2_000L而SLOW_LOG_THRESHOLD_US常量仍被 parseLrcAsync 使用,同一阈值两种写法,建议统一。
🔵 src/test/java/firefly520/fireflymc/client/music/MusicCacheTest.java:37-48 · minor
safeResolve 防目录逃逸为安全改动但无配套单元测试
本次新增的 safeResolve(resolve + normalize + 绝对路径前缀校验)是全部缓存 I/O 入口的安全防线,但 MusicCacheTest 中没有针对它的用例:路径逃逸 songId(如 ../escape)应被拒绝为 null/空命中、正常 songId 不受影响,这两点均未验证。项目规则"安全/可靠性改动须配套单元测试(major)"明确要求配套测试;由于该防线拦截的是已过双重校验(服务端 isValidSongId + 客户端 start 校验)之后的第三层防御、且实现本身正确,实际风险低,评 minor。建议按下方一键补齐。
Suggestion:
@Test
void currentFormatCacheSurvivesInitialize() throws IOException {
// 已是 v2 格式(标记存在):残留 .part 清理,正式缓存保留
MusicCache first = new MusicCache(tempDir);
first.ensureInitialized(); // 写入 v2 标记
Files.write(tempDir.resolve("654321.mp3"), new byte[10]);
Files.write(tempDir.resolve("654321.9.mp3.part"), new byte[10]);
MusicCache cache = new MusicCache(tempDir);
cache.ensureInitialized();
assertTrue(Files.exists(tempDir.resolve("654321.mp3")), "当前格式正式缓存不得误删");
assertFalse(Files.exists(tempDir.resolve("654321.9.mp3.part")), "残留 .part 必须被清理");
}
@Test
void safeResolveRejectsPathTraversal() throws IOException {
// safeResolve 防目录逃逸(安全改动须配套测试):含 .. 的 songId 一律降级拒绝,
// 不得在缓存目录外读/写/删;正常 songId 不受影响
MusicCache cache = new MusicCache(tempDir);
assertTrue(cache.getCachedFile("../escape").isEmpty(), "路径逃逸 songId 不得命中缓存");
assertNull(cache.beginPartFile("../escape", 1L), "路径逃逸 songId 不得创建 .part");
assertDoesNotThrow(() -> cache.invalidate("../escape"));
Files.write(tempDir.resolve("real.mp3"), new byte[5]);
assertTrue(cache.getCachedFile("real").isPresent(), "正常 songId 不受影响");
}
代码质量评分:8/10
AI审查决策:批准合并
🔵 次要问题 (1个)
- safeResolve 防目录逃逸为安全改动但无配套单元测试
💡 优化建议 (1条)
- 播放 worker 改为虚拟线程后存在 synchronized pinning 隐患,建议评估或观测
🏷️ 标签建议
💡 建议的标签(需确认)
- sakura-scan (60%) - 代码变更包含针对恶意 songId 的路径遍历防护,这通常是安全扫描工具(如 Sakura Scan)发现的高危漏洞修复点。
注:勾选复选框即可应用标签,取消勾选即可移除标签(仅 PR 作者或仓库管理员/协作者可操作)
此评论由 Sakura AI 自动生成。
Implement fail-closed logic for cache format migration to prevent bad cache files from persisting when deletion fails due to file handle locks or antivirus interference. If legacy cache deletion is incomplete, defer initialization and retry migration on next playback attempt instead of writing a success marker. Refine playback error classification to distinguish between network interruptions and corrupted audio sources using byte count verification. BitstreamExceptions with complete byte transfers now indicate source corruption rather than network failures, enabling proper quorum voting to skip bad audio sources across the server.
There was a problem hiding this comment.
🌸 Sakura AI 审查报告 - ⚡️ 快速审查
✅ 审查通过
📋 查看详细审查报告
审查总结(Issue #64 修复 · 增量第五轮:迁移 fail-closed + 206 防御 + 异常分类精化)
本增量验证结果
- 迁移 fail-closed(MusicCache):
hasLegacyCacheLeft()二次扫描 + IOException 视为未完成,删除失败(Windows 句柄/杀软占用)时 return 且不置initialized——同进程后续播放自动重试迁移。串行播放约束下重试 wipe 不会误删在写 .part(新 worker 先等旧 worker 退出,且 .part 带唯一 playbackId)。 - unsolicited 206 防御(MusicApiClient):无 Range 的播放请求只接受 200——否则升级响应的 Content-Length 是分段长度,会让
guard.bytesRead() >= expectedBytes对半截 MP3 误判"完整"并落盘截断缓存。主路径openFollowing返回的 206 由 MusicPlayer 的code != 200兜底拒绝,此修复把浪费一次恢复尝试变成优雅回退原始 http。 - 异常分类(MusicPlayer):stalled 与 BitstreamException 分离后,"字节完整仍解码失败"正确投音源票;本地坏缓存(expectedBytes=-1)的 BitstreamException 归 RETRY → invalidate 重下,闭环保持。
markHttpsStalled()去重合理,truncated 分支的 guard 非空性成立(truncated 蕴含网络模式)。
观察(不构成缺陷)
- 迁移未完成提前 return 时注释称"本曲按无缓存处理走网络下载",但
runPlayback随后仍会getCachedFile命中删不掉的旧缓存走本地播放;可接受——命中后若数据损坏会经 BitstreamException→RETRY→invalidate 回到网络路径,会话内自愈,仅注释与行为不完全一致。
遗留跟进项(均非本次新增)
MusicServerBridge.onClientFailure失败上报日志仍未套用sanitize()(连续多轮挂账)。HttpClient.Redirect.ALWAYS+openFollowing不校验最终 scheme 的明文降级债务。logSlow内联2_000L与SLOW_LOG_THRESHOLD_US常量并存,同一阈值两种写法。
💡 src/main/java/firefly520/fireflymc/client/music/MusicPlayer.java:347-360 · suggestion
字节维度判定未覆盖非 BitstreamException 的解码异常,本地损坏缓存可误投 quorum 票
catch 块中"字节不完整/未知 → 网络原因仍有可能,重试"的判定只应用于 BitstreamException 分支(第 353-358 行);其他解码异常(如解码器对损坏数据抛出的 ArrayIndexOutOfBoundsException 等 RuntimeException)直接落到第 360 行 return Attempt.DECODE_FAILED——即使 byteIncomplete 为 true。潜在后果:本地缓存文件损坏(expectedBytes=-1 的 localMode)触发非 BitstreamException 异常时会被当作音源损坏向服务端投 quorum 票,全服跳过一首实际可播的歌;网络截断字节不完整时的非 BitstreamException 同理失去重试机会。与第 337-339 行注释声明的"用字节信息区分"哲学不完全一致。统一按字节维度判定还可顺带消除对异常类型的耦合。属边缘路径精化,评 suggestion。
Suggestion:
if (stalled) {
// 恢复点 = 权威时钟当前位置:停滞 30s 期间它持续推进,恢复后与房间同步;
// line 时钟早已冻结,不能作为恢复点(会落后服务端数十秒)
markHttpsStalled();
return Attempt.RETRY;
}
// 字节维度统一判定(与异常类型解耦):字节不完整/未知(含本地坏缓存 expectedBytes=-1)
// 时网络原因仍有可能,一律按可恢复断流重试(本地路径由外层 invalidate 后重下);
// 字节已完整仍解码失败 = 音源本身损坏,投 quorum 票(全服跳过坏音源)
if (expectedBytes <= 0 || byteIncomplete) {
markHttpsStalled();
return Attempt.RETRY;
}
return Attempt.DECODE_FAILED;
代码质量评分:8/10
AI审查决策:批准合并
🔵 次要问题 (1个)
- 新增的迁移 fail-closed 路径无配套单元测试
💡 优化建议 (1条)
- 字节维度判定未覆盖非 BitstreamException 的解码异常,本地损坏缓存可误投 quorum 票
🏷️ 标签建议
💡 建议的标签(需确认)
- refactor (55%) - MusicCache.java 中的 ensureInitialized 方法进行了结构调整,引入了 CompletableFuture.runAsync 和 try-catch 块以实现 fail-closed 逻辑,虽然目的是修复 Bug,但代码结构有显著重构。
注:勾选复选框即可应用标签,取消勾选即可移除标签(仅 PR 作者或仓库管理员/协作者可操作)
此评论由 Sakura AI 自动生成。
| if (stalled) { | ||
| // 恢复点 = 权威时钟当前位置:停滞 30s 期间它持续推进,恢复后与房间同步; | ||
| // line 时钟早已冻结,不能作为恢复点(会落后服务端数十秒) | ||
| markHttpsStalled(); | ||
| return Attempt.RETRY; | ||
| } | ||
| if (e instanceof javazoom.jl.decoder.BitstreamException) { | ||
| if (expectedBytes <= 0 || byteIncomplete) { | ||
| markHttpsStalled(); | ||
| return Attempt.RETRY; // 字节不完整/未知:网络原因仍有可能 | ||
| } | ||
| return Attempt.DECODE_FAILED; // 字节完整仍解码失败:音源本身损坏 | ||
| } | ||
| return Attempt.DECODE_FAILED; |
There was a problem hiding this comment.
字节维度判定未覆盖非 BitstreamException 的解码异常,本地损坏缓存可误投 quorum 票
catch 块中"字节不完整/未知 → 网络原因仍有可能,重试"的判定只应用于 BitstreamException 分支(第 353-358 行);其他解码异常(如解码器对损坏数据抛出的 ArrayIndexOutOfBoundsException 等 RuntimeException)直接落到第 360 行 return Attempt.DECODE_FAILED——即使 byteIncomplete 为 true。潜在后果:本地缓存文件损坏(expectedBytes=-1 的 localMode)触发非 BitstreamException 异常时会被当作音源损坏向服务端投 quorum 票,全服跳过一首实际可播的歌;网络截断字节不完整时的非 BitstreamException 同理失去重试机会。与第 337-339 行注释声明的"用字节信息区分"哲学不完全一致。统一按字节维度判定还可顺带消除对异常类型的耦合。属边缘路径精化,评 suggestion。
Suggestion:
| if (stalled) { | |
| // 恢复点 = 权威时钟当前位置:停滞 30s 期间它持续推进,恢复后与房间同步; | |
| // line 时钟早已冻结,不能作为恢复点(会落后服务端数十秒) | |
| markHttpsStalled(); | |
| return Attempt.RETRY; | |
| } | |
| if (e instanceof javazoom.jl.decoder.BitstreamException) { | |
| if (expectedBytes <= 0 || byteIncomplete) { | |
| markHttpsStalled(); | |
| return Attempt.RETRY; // 字节不完整/未知:网络原因仍有可能 | |
| } | |
| return Attempt.DECODE_FAILED; // 字节完整仍解码失败:音源本身损坏 | |
| } | |
| return Attempt.DECODE_FAILED; | |
| if (stalled) { | |
| // 恢复点 = 权威时钟当前位置:停滞 30s 期间它持续推进,恢复后与房间同步; | |
| // line 时钟早已冻结,不能作为恢复点(会落后服务端数十秒) | |
| markHttpsStalled(); | |
| return Attempt.RETRY; | |
| } | |
| // 字节维度统一判定(与异常类型解耦):字节不完整/未知(含本地坏缓存 expectedBytes=-1) | |
| // 时网络原因仍有可能,一律按可恢复断流重试(本地路径由外层 invalidate 后重下); | |
| // 字节已完整仍解码失败 = 音源本身损坏,投 quorum 票(全服跳过坏音源) | |
| if (expectedBytes <= 0 || byteIncomplete) { | |
| markHttpsStalled(); | |
| return Attempt.RETRY; | |
| } | |
| return Attempt.DECODE_FAILED; |
此评论由 Sakura AI 自动生成。
…ails - Change `MusicCache.ensureInitialized()` to return a boolean indicating cache availability - Create cache directory before listing files to prevent errors on fresh installs - Make `writeFormatMarker()` return boolean and log failures instead of silently ignoring exceptions - Introduce `cacheReady` flag in `MusicPlayer` to guard against reading stale caches or writing new caches before the format marker is safely persisted
There was a problem hiding this comment.
🌸 Sakura AI 审查报告 - ⚡️ 快速审查
✅ 审查通过
📋 查看详细审查报告
审查总结(Issue #64 修复 · 增量第六轮:缓存禁用契约)
本增量验证结果
- boolean 契约闭环:
ensureInitialized()三条失败路径(目录创建失败、hasLegacyCacheLeft残留、writeFormatMarker失败)统一返回 false;MusicPlayer以cacheReady同时闸断读取(getCachedFile)与写入(beginPartFile)。这补上了 incr5 的观察缺口——此前迁移失败后runPlayback仍可能命中删不掉的旧缓存走本地播放,现在确实"完全禁用缓存",Javadoc 中的承诺(禁读防旧坏缓存、禁写防合法 v2 被下次迁移误删)与实现一致。cancel()/finally 中的deletePartFile(null)均已容 null,禁用写入路径无悬挂资源。 - 全新安装边界修复:先
createDirectories再迁移,消除"目录缺失被 Files.list 误判为删除失败→永远禁用缓存"的新装首播故障;freshInstallWithoutDirectoryInitializesCleanly测试覆盖(含幂等断言)。 - cacheReady 线程可见性:仅播放线程单线程读写(runPlayback 写、streamAttempt 读),无需 volatile,正确。
- marker 写失败也 fail-closed:防止本轮合法 v2 缓存被重启后的重试迁移误删,方向正确且带日志。
遗留跟进项
return false的 fail-closed 路径无测试(minor,见 finding——已确认存在无需注入点的可移植测法)。- 历史挂账:
MusicServerBridge.onClientFailure日志未套用sanitize();Redirect.ALWAYS+openFollowing不校验最终 scheme;logSlow内联2_000L与SLOW_LOG_THRESHOLD_US常量并存。
🔵 src/test/java/firefly520/fireflymc/client/music/MusicCacheTest.java:62-71 · minor
ensureInitialized 返回 false 的 fail-closed 路径仍无测试,且已确认可移植测法
本次为 ensureInitialized 新增了三条 return false 路径(目录创建失败、迁移残留、marker 写失败),测试文件同步更新了断言但只覆盖成功路径——false 契约是本次的核心语义(调用方据此完全禁用缓存读写),无任何用例验证:返回 false、不写 .format-version、initialized 保持 false 使下次调用重试。incr5 曾以"删除失败需 OS 级文件锁不可移植"为由仅要求注入点,但本次核对实现后确认存在两个可移植测法:(1) 用非空目录冒充 *.mp3.part(Files.deleteIfExists 对非空目录抛 DirectoryNotEmptyException,deleteQuietly 吞掉后 hasLegacyCacheLeft 检出残留);(2) 用普通文件占位 cacheDir(createDirectories 抛 FileAlreadyExistsException)。均为确定性纯 JVM 行为,可直接落地。按项目规则"安全/可靠性改动须配套单元测试",评 minor。
Suggestion:
@Test
void ensureInitializedIsIdempotentAndNeverDeletesClaimedParts() throws IOException {
// 幂等标志是正确性约束:重复清理若不挡住,会误删 worker 已 begin、正在写的新 .part
MusicCache cache = new MusicCache(tempDir);
assertTrue(cache.ensureInitialized(), "首次初始化必须成功");
Path fresh = cache.beginPartFile("999", 1L);
Files.write(fresh, new byte[10]);
assertTrue(cache.ensureInitialized(), "重复初始化必须成功(幂等)");
assertTrue(Files.exists(fresh), "重复初始化不得误删已认领的 .part");
}
@Test
void migrationFailureReturnsFalseAndKeepsRetrying() throws IOException {
// fail-closed:删除失败(非空目录占位 .part,deleteIfExists 抛 DirectoryNotEmptyException
// 被吞)→ 返回 false、不写版本标记、initialized 保持 false,下次调用重试迁移
Path undeletable = Files.createDirectories(tempDir.resolve("123456.1.mp3.part"));
Files.write(undeletable.resolve("data"), new byte[5]); // 非空:删除必失败
MusicCache cache = new MusicCache(tempDir);
assertFalse(cache.ensureInitialized(), "迁移未完成必须返回 false(调用方禁用缓存读写)");
assertFalse(Files.exists(tempDir.resolve(".format-version")), "fail-closed 不得写入版本标记");
assertFalse(cache.ensureInitialized(), "initialized 未置位:下次调用必须重试迁移");
assertFalse(Files.exists(tempDir.resolve(".format-version")), "重试仍失败不得写标记");
}
@Test
void unwritableCacheDirReturnsFalse() throws IOException {
// 目录创建失败(普通文件占位 cacheDir)→ 返回 false,全程无标记写入
Path blocked = tempDir.resolve("blocker");
Files.write(blocked, new byte[0]); // 常规文件:createDirectories 必失败
MusicCache cache = new MusicCache(blocked);
assertFalse(cache.ensureInitialized(), "目录创建失败必须返回 false(禁用缓存读写)");
}
代码质量评分:8/10
AI审查决策:批准合并
🔵 次要问题 (1个)
- ensureInitialized 返回 false 的 fail-closed 路径仍无测试,且已确认可移植测法
💡 优化建议 (1条)
- wipeLegacyCache 的 wiped 计数包含删除失败的文件,warn 日志可能自相矛盾
🏷️ 标签建议
✅ 已自动应用的标签
- refactor (75%) - Refactored MusicCache.ensureInitialized() to return a boolean status instead of throwing exceptions, enabling graceful degradation in the caller.
💡 建议的标签(需确认)
- documentation (45%) - Added @return javadoc to the ensureInitialized() method to document the new return value.
注:勾选复选框即可应用标签,取消勾选即可移除标签(仅 PR 作者或仓库管理员/协作者可操作)
此评论由 Sakura AI 自动生成。
| @Test | ||
| void ensureInitializedIsIdempotentAndNeverDeletesClaimedParts() throws IOException { | ||
| // 幂等标志是正确性约束:重复清理若不挡住,会误删 worker 已 begin、正在写的新 .part | ||
| MusicCache cache = new MusicCache(tempDir); | ||
| assertTrue(cache.ensureInitialized(), "首次初始化必须成功"); | ||
| Path fresh = cache.beginPartFile("999", 1L); | ||
| Files.write(fresh, new byte[10]); | ||
| assertTrue(cache.ensureInitialized(), "重复初始化必须成功(幂等)"); | ||
| assertTrue(Files.exists(fresh), "重复初始化不得误删已认领的 .part"); | ||
| } |
There was a problem hiding this comment.
ensureInitialized 返回 false 的 fail-closed 路径仍无测试,且已确认可移植测法
本次为 ensureInitialized 新增了三条 return false 路径(目录创建失败、迁移残留、marker 写失败),测试文件同步更新了断言但只覆盖成功路径——false 契约是本次的核心语义(调用方据此完全禁用缓存读写),无任何用例验证:返回 false、不写 .format-version、initialized 保持 false 使下次调用重试。incr5 曾以"删除失败需 OS 级文件锁不可移植"为由仅要求注入点,但本次核对实现后确认存在两个可移植测法:(1) 用非空目录冒充 *.mp3.part(Files.deleteIfExists 对非空目录抛 DirectoryNotEmptyException,deleteQuietly 吞掉后 hasLegacyCacheLeft 检出残留);(2) 用普通文件占位 cacheDir(createDirectories 抛 FileAlreadyExistsException)。均为确定性纯 JVM 行为,可直接落地。按项目规则"安全/可靠性改动须配套单元测试",评 minor。
Suggestion:
| @Test | |
| void ensureInitializedIsIdempotentAndNeverDeletesClaimedParts() throws IOException { | |
| // 幂等标志是正确性约束:重复清理若不挡住,会误删 worker 已 begin、正在写的新 .part | |
| MusicCache cache = new MusicCache(tempDir); | |
| assertTrue(cache.ensureInitialized(), "首次初始化必须成功"); | |
| Path fresh = cache.beginPartFile("999", 1L); | |
| Files.write(fresh, new byte[10]); | |
| assertTrue(cache.ensureInitialized(), "重复初始化必须成功(幂等)"); | |
| assertTrue(Files.exists(fresh), "重复初始化不得误删已认领的 .part"); | |
| } | |
| @Test | |
| void ensureInitializedIsIdempotentAndNeverDeletesClaimedParts() throws IOException { | |
| // 幂等标志是正确性约束:重复清理若不挡住,会误删 worker 已 begin、正在写的新 .part | |
| MusicCache cache = new MusicCache(tempDir); | |
| assertTrue(cache.ensureInitialized(), "首次初始化必须成功"); | |
| Path fresh = cache.beginPartFile("999", 1L); | |
| Files.write(fresh, new byte[10]); | |
| assertTrue(cache.ensureInitialized(), "重复初始化必须成功(幂等)"); | |
| assertTrue(Files.exists(fresh), "重复初始化不得误删已认领的 .part"); | |
| } | |
| @Test | |
| void migrationFailureReturnsFalseAndKeepsRetrying() throws IOException { | |
| // fail-closed:删除失败(非空目录占位 .part,deleteIfExists 抛 DirectoryNotEmptyException | |
| // 被吞)→ 返回 false、不写版本标记、initialized 保持 false,下次调用重试迁移 | |
| Path undeletable = Files.createDirectories(tempDir.resolve("123456.1.mp3.part")); | |
| Files.write(undeletable.resolve("data"), new byte[5]); // 非空:删除必失败 | |
| MusicCache cache = new MusicCache(tempDir); | |
| assertFalse(cache.ensureInitialized(), "迁移未完成必须返回 false(调用方禁用缓存读写)"); | |
| assertFalse(Files.exists(tempDir.resolve(".format-version")), "fail-closed 不得写入版本标记"); | |
| assertFalse(cache.ensureInitialized(), "initialized 未置位:下次调用必须重试迁移"); | |
| assertFalse(Files.exists(tempDir.resolve(".format-version")), "重试仍失败不得写标记"); | |
| } | |
| @Test | |
| void unwritableCacheDirReturnsFalse() throws IOException { | |
| // 目录创建失败(普通文件占位 cacheDir)→ 返回 false,全程无标记写入 | |
| Path blocked = tempDir.resolve("blocker"); | |
| Files.write(blocked, new byte[0]); // 常规文件:createDirectories 必失败 | |
| MusicCache cache = new MusicCache(blocked); | |
| assertFalse(cache.ensureInitialized(), "目录创建失败必须返回 false(禁用缓存读写)"); | |
| } |
此评论由 Sakura AI 自动生成。
|
📝 已创建 Release v3.0.2 描述 ✅ 变更已记录到 GitHub Releases 📦 附件正在打包中,请稍候... |
Add a retry mechanism (10 attempts, 100ms delay) in JavaSoundOutput.tryOpen() to handle LineUnavailableException caused by unreleased audio lines during song transitions (Fixes #64). Update ClientMusicFailReporter to map the new NETWORK_FAILED and HTTP_PERMANENT_FAILED failure codes. Bump mod version from 3.0.1 to 3.0.2.
🌸 Sakura AI的总结
变更概览
修复播放静音与切歌死锁,增强缓存容错与安全性,优化性能架构。
主要改动
MusicPlayer中增加 SourceDataLine 初始化重试机制,彻底解决静音播放问题。MusicCache在初始化或迁移失败时自动禁用读写,防止系统崩溃。StallGuardInputStream防止切歌死锁。影响范围
显著提升音频播放稳定性、系统健壮性及主线程响应速度。
依赖图
graph TD N1["src/main/java/firefly520/fireflymc/client/music/MusicPlayer.java"] N2["src/main/java/firefly520/fireflymc/music/MusicApiClient.java"] N3["src/main/java/firefly520/fireflymc/client/music/MusicPlaybackManager.java"] N4["src/main/java/firefly520/fireflymc/music/MusicServerBridge.java"] N5["src/main/java/firefly520/fireflymc/network/MusicPlaybackFailedPayload.java"] N6["src/main/java/firefly520/fireflymc/FireflyMCMod.java"] N7["src/main/java/firefly520/fireflymc/client/ClientMusicFailReporter.java"] N8["src/main/java/firefly520/fireflymc/music/MusicQueueManager.java"] N9["src/test/java/firefly520/fireflymc/music/MusicQueueManagerTest.java"] N10["src/main/java/firefly520/fireflymc/client/music/MusicCache.java"] N11["src/test/java/firefly520/fireflymc/client/music/MusicCacheTest.java"] N7 --> N1 N7 --> N5 N3 --> N7 N1 --> N2 N8 --> N5 N4 --> N5 N5 --> N6 N9 --> N5