Roll out Android MLIP v4 WebDAV handling#54
Conversation
📝 WalkthroughWalkthroughThe changes add centralized WebDAV request coordination, route WebDAV playback through Exo, introduce MLIP v4 artwork-pack caching and manifest generation, coordinate artwork downloads, and update application build identity configuration. ChangesWebDAV transport and playback
MLIP v4 artwork caching
Application build configuration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant PlaybackDataSource
participant WebDavRequestCoordinator
participant HttpURLConnection
participant WebDAVServer
PlaybackDataSource->>WebDavRequestCoordinator: submit gated WebDAV request
WebDavRequestCoordinator->>HttpURLConnection: execute coordinated transport
HttpURLConnection->>WebDAVServer: ranged HTTP request without redirects
WebDAVServer-->>HttpURLConnection: response bytes and status
HttpURLConnection-->>WebDavRequestCoordinator: transport result
WebDavRequestCoordinator-->>PlaybackDataSource: stream and lease
sequenceDiagram
participant MlipLibraryIndexImporter
participant ArtworkPackCache
participant MediaSource
participant ArtworkCache
MlipLibraryIndexImporter->>ArtworkPackCache: cache required artwork assets
ArtworkPackCache->>MediaSource: open artwork pack
MediaSource-->>ArtworkPackCache: pack stream
ArtworkPackCache->>ArtworkCache: verify and extract assets
ArtworkCache-->>MlipLibraryIndexImporter: cached asset paths
MlipLibraryIndexImporter-->>MlipLibraryIndexImporter: write artwork-bindings.json
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (11)
player-core/src/main/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactory.kt (2)
70-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSide effect in a
data classinitializer.Every
PlaybackHttpRequestConfigconstruction (includingcopy(), test fixtures, and equality-driven re-creations) mutates global coordinator state. Registration would sit better where the config is installed (setHttpConfig) to keep the value type pure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@player-core/src/main/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactory.kt` around lines 70 - 73, Remove the WebDavRequestCoordinator.register side effect from the PlaybackHttpRequestConfig initializer, keeping the data class value-only. Register normalizedBaseUrl in the PlaybackDataSourceFactory.setHttpConfig installation path instead, only when it is non-blank.
317-324: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused
invalidResponseCode()helper.
invalidResponseCode()has no callers,HttpDataSource.InvalidResponseCodeExceptionis only referenced at its declaration, and the WebDAV path throwsWebDavHttpStatusExceptiondirectly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@player-core/src/main/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactory.kt` around lines 317 - 324, Remove the unused Throwable.invalidResponseCode() extension and its HttpDataSource.InvalidResponseCodeException reference, leaving the surrounding playback data-source logic unchanged.media-source/src/main/kotlin/com/miruplay/tv/mediasource/WebDavMediaSource.kt (1)
138-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKind says
HEAD, request is aPROPFINDDepth: 0.The kind is what the coordinator uses for classification/diagnostics; labeling a PROPFIND as
HEADwill make endpoint traces misleading.PROPFINDis the accurate kind here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@media-source/src/main/kotlin/com/miruplay/tv/mediasource/WebDavMediaSource.kt` around lines 138 - 148, Update the executeBytesWithAnonymousFallback call in the WebDavMediaSource request flow so its kind argument uses the PROPFIND request classification instead of HEAD. Keep the existing PROPFIND method, Depth header, authorization, and response handling unchanged.media-source/src/test/kotlin/com/miruplay/tv/mediasource/WebDavRequestCoordinatorTest.kt (1)
48-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCoalescing test leans on wall-clock sleeps.
Thread.sleep(20L)plussecondStartedonly proves the second task entered, not that it observed the in-flight future. Under CI load this can pass vacuously or flake. Gating the second submission on an observable coordinator state (or assertingcalls.get() == 1after both futures complete only) would make it deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@media-source/src/test/kotlin/com/miruplay/tv/mediasource/WebDavRequestCoordinatorTest.kt` around lines 48 - 86, Make the duplicate path byte request test deterministic by replacing the Thread.sleep-based timing in `duplicate path byte requests are coalesced` with an observable synchronization point proving the second request has joined or observed the coordinator’s in-flight future before releasing the transport. Keep the existing assertions that both futures return “listing” and only one transport call occurs, and remove the redundant timing gate.player-core/src/test/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactoryTest.kt (1)
112-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLeaks global coordinator state across tests.
Constructing
PlaybackHttpRequestConfigregisters the endpoint and the firstopen()spawns a per-endpoint consumer thread in theWebDavRequestCoordinatorsingleton; nothing resets it. AddWebDavRequestCoordinator.resetForTests()in an@Afterso circuit/consumer state can't bleed into sibling tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@player-core/src/test/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactoryTest.kt` around lines 112 - 116, Add an `@After` cleanup method in PlaybackDataSourceFactoryTest that calls WebDavRequestCoordinator.resetForTests(). Ensure it runs after every test so endpoint registration, circuit state, and consumer threads created by PlaybackHttpRequestConfig and GatedPlaybackDataSource do not leak across tests.scanner/src/main/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporter.kt (3)
737-770: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winManifest write is not crash-safe against partial reads.
temporary.renameTo(output)falls back tocopyTo(output, overwrite = true), which can leave a truncatedartwork-bindings.jsonif it fails midway.ArtworkPackCachealready hasreplaceAtomically(Files.movewithATOMIC_MOVE); reusing it here would keep the manifest consistent.♻️ Reuse the atomic replace helper
- temporary.writeText(entries.toString()) - if (!temporary.renameTo(output)) temporary.copyTo(output, overwrite = true) + temporary.writeText(entries.toString()) + replaceAtomically(temporary, output)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporter.kt` around lines 737 - 770, Update persistArtworkBindingManifest to replace the temporary manifest using ArtworkPackCache.replaceAtomically instead of renameTo with a copyTo fallback. Preserve the existing temporary-file creation and cleanup, and ensure the final artwork-bindings.json replacement uses the helper’s atomic move behavior.
320-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the poster artwork kind into a named constant. The thumb kind uses
MLIP_ARTWORK_KIND_THUMB, but the poster kind is the bare literal1here (and as theartworkKinddefault inMlipArtworkBinding). AMLIP_ARTWORK_KIND_POSTERconstant keeps both call sites consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporter.kt` around lines 320 - 326, Define a named MLIP_ARTWORK_KIND_POSTER constant alongside MLIP_ARTWORK_KIND_THUMB, then replace the literal 1 in the posterBySeriesId filter and the artworkKind default in MlipArtworkBinding with that constant.
104-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwallowed artwork-pack failures leave no trace.
runCatching { … }.getOrDefault(emptyMap())discards the cause;ArtworkPackCacheonly logs per-pack rejections, so construction/IO failures at this level are invisible during scan diagnostics. ConsideronFailure { MiruLog.w(TAG, "MLIP artwork pack caching failed", it, …) }.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporter.kt` around lines 104 - 112, The artwork-pack caching flow in the packedArtworkByAssetId initialization swallows construction or I/O failures without logging them. Add failure logging to the runCatching chain using MiruLog.w with TAG, a clear caching-failed message, and the caught exception, while preserving the existing emptyMap fallback.scanner/src/main/kotlin/com/miruplay/tv/scanner/ArtworkPackCache.kt (1)
144-144: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueDuplicate
memberNames across assets silently reject the whole pack.associateBy(memberName)keeps only the last asset for a given member, so two asset rows pointing at the same tar member cause thecontainsAll(requiredAssets ids)check to fail and the entire pack to be discarded. Grouping by member name and extracting for each matching asset id would be more forgiving.Also applies to: 188-191
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/ArtworkPackCache.kt` at line 144, Update the expected-asset handling around associateBy(MlipArtworkAsset::memberName) and the corresponding extraction logic at the additional referenced block so duplicate memberName values are grouped rather than overwritten. For each tar member, retain and extract all matching required assets by id, ensuring duplicate member names do not cause the containsAll validation to reject an otherwise valid pack.scanner/src/test/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporterTest.kt (2)
894-896: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBuild the diagnostic message lazily and import
ShadowLog.packLogsis computed on every run just to serve as a failure message, and the fully-qualifiedorg.robolectric.shadows.ShadowLogreference is inconsistent with the rest of the file's imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scanner/src/test/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporterTest.kt` around lines 894 - 896, Update the assertion around packLogs to build the diagnostic log message lazily using the assertion API’s failure-message lambda, and add the appropriate ShadowLog import so the fully qualified reference is removed. Preserve the existing log formatting and file-count assertion.
880-885: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the new test and companion object above the private helper classes. The file otherwise groups all
@Testmethods first, then private fakes;rust generated v4 fixture …and theONE_PIXEL_PNG_BASE64companion are wedged betweenRecordingMetadataRepositoryandFakeMediaSource.Also applies to: 927-931
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scanner/src/test/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporterTest.kt` around lines 880 - 885, Move the test method `rust generated v4 fixture caches all bindings and only the incremental pack` and its `ONE_PIXEL_PNG_BASE64` companion object above the private helper classes, preserving the file’s organization with all `@Test` methods first and fake implementations afterward. Keep `RecordingMetadataRepository` and `FakeMediaSource` together in the helper section.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/build.gradle.kts`:
- Around line 40-41: Update the appApplicationId resolution to also read the
APPLICATION_ID environment variable, while preserving the existing Gradle
property precedence and fallback to "com.miruplay.tv".
In
`@media-source/src/main/kotlin/com/miruplay/tv/mediasource/WebDavRequestCoordinator.kt`:
- Around line 152-155: Update the timeout handling in submit so enqueue and
completion deadline failures are surfaced as IOException-compatible errors,
preferably InterruptedIOException while preserving the timeout context and
cause. Ensure no bare java.util.concurrent.TimeoutException escapes, while
leaving existing WebDavHttpStatusException and WebDavCircuitOpenException
behavior unchanged.
- Around line 211-224: Bound the streaming wait in the WebDavRequestCoordinator
flow around WebDavLease and released.await() using the configured streaming
lease timeout. If the wait expires, force-close the lease/underlying transport
and ensure the worker can complete; preserve the existing immediate close
behavior for non-streaming requests.
In `@player-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.kt`:
- Around line 213-220: Update the WebDAV routing flow in play() and the
associated runtime backend selection, including refreshRuntimeConfig or its
fallback-reason handling, so WebDAV sources set _activeRenderBackend to the
standard ExoPlayer backend before playback begins. Ensure all controller
methods, player identity checks, callbacks, and UI surface selection use that
synchronized backend state rather than only selecting standardExoPlayer()
locally.
In `@player-core/src/main/kotlin/com/miruplay/tv/player/Mp4DolbyVisionProbe.kt`:
- Around line 344-351: Update the response-status validation in the surrounding
WebDAV probe method to reject every status outside 200..299, while preserving
the existing exception for 405 so the coordinator can handle it. Keep successful
2xx responses flowing to WebDavTransportResult unchanged.
In
`@player-core/src/main/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactory.kt`:
- Around line 210-235: Update the data source implementation around
addTransferListener, open, read, and close to retain registered TransferListener
instances locally instead of forwarding them only to upstream. Dispatch
onTransferStart when either upstream or gated WebDAV opens, report each
successful read through onBytesTransferred, and dispatch onTransferEnd when the
transfer closes, while preserving existing upstream behavior and WebDAV lease
handling.
- Around line 237-238: Update the DataSource implementation around read() to
track bytesRemaining from resolvedLength, clamp each underlying webDavInput or
upstream read to the remaining bound, decrement it after successful reads, and
return C.RESULT_END_OF_INPUT once exhausted. Preserve unbounded behavior when
resolvedLength is unknown.
In
`@player-core/src/test/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactoryTest.kt`:
- Around line 95-125: Prevent the redirect test from hanging when
dataSource.open fails before connecting: set redirectServer.soTimeout to a
bounded duration before starting redirectTask, and replace the unbounded
redirectTask.join() with a bounded join. Keep the existing assertions and
redirect-server behavior unchanged.
In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/ArtworkPackCache.kt`:
- Around line 104-106: In the pack-fetch flow around packTemp and
mediaSource.openStream, open and validate the remote stream before calling
File.createTempFile. Ensure the early emptyMap return occurs before
temporary-file creation, while preserving the existing try/finally cleanup for
successfully opened streams.
- Around line 228-238: Update validateImage to fully read up to 12 signature
bytes instead of relying on one input.read call. Use readNBytes or equivalent
readFully semantics, retain the actual byte count, and reject inputs shorter
than the required signature before checking PNG, JPEG, or WEBP markers.
In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.kt`:
- Around line 863-879: Extract the duplicated artwork download behavior into one
shared media-source helper accepting a URL, optional Proxy, and target File;
centralize the HttpURLConnection configuration, 2xx validation with
WebDavHttpStatusException, stream copying, and guaranteed disconnect. In
scanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.kt lines
863-879, replace the local download lambda with this helper while preserving the
existing WebDavRequestCoordinator.execute artwork wrapper. In
ui-tv/src/main/kotlin/com/miruplay/tv/ui/components/RemoteImage.kt lines 94-110,
replace the duplicated implementation with the same helper using Proxy.NO_PROXY.
- Around line 880-885: Update the poster download flow around
WebDavRequestCoordinator.execute and cachePoster so poster URLs do not pass
through WebDAV endpoint coalescing or circuit tracking. Use a direct
non-coalescing download path for non-WebDAV poster URLs, while preserving WebDAV
routing only when the URL is a WebDAV backend resource.
In `@ui-tv/src/main/kotlin/com/miruplay/tv/ui/components/RemoteImage.kt`:
- Around line 93-121: Move the temporary-file cleanup scope to encompass the
entire coordinated download flow in the RemoteImage download logic, including
WebDavRequestCoordinator.execute and download(). Ensure temp is deleted whenever
any download, HTTP-status, timeout, or coordination failure occurs, while
preserving the existing rename behavior for successful downloads.
---
Nitpick comments:
In
`@media-source/src/main/kotlin/com/miruplay/tv/mediasource/WebDavMediaSource.kt`:
- Around line 138-148: Update the executeBytesWithAnonymousFallback call in the
WebDavMediaSource request flow so its kind argument uses the PROPFIND request
classification instead of HEAD. Keep the existing PROPFIND method, Depth header,
authorization, and response handling unchanged.
In
`@media-source/src/test/kotlin/com/miruplay/tv/mediasource/WebDavRequestCoordinatorTest.kt`:
- Around line 48-86: Make the duplicate path byte request test deterministic by
replacing the Thread.sleep-based timing in `duplicate path byte requests are
coalesced` with an observable synchronization point proving the second request
has joined or observed the coordinator’s in-flight future before releasing the
transport. Keep the existing assertions that both futures return “listing” and
only one transport call occurs, and remove the redundant timing gate.
In
`@player-core/src/main/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactory.kt`:
- Around line 70-73: Remove the WebDavRequestCoordinator.register side effect
from the PlaybackHttpRequestConfig initializer, keeping the data class
value-only. Register normalizedBaseUrl in the
PlaybackDataSourceFactory.setHttpConfig installation path instead, only when it
is non-blank.
- Around line 317-324: Remove the unused Throwable.invalidResponseCode()
extension and its HttpDataSource.InvalidResponseCodeException reference, leaving
the surrounding playback data-source logic unchanged.
In
`@player-core/src/test/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactoryTest.kt`:
- Around line 112-116: Add an `@After` cleanup method in
PlaybackDataSourceFactoryTest that calls
WebDavRequestCoordinator.resetForTests(). Ensure it runs after every test so
endpoint registration, circuit state, and consumer threads created by
PlaybackHttpRequestConfig and GatedPlaybackDataSource do not leak across tests.
In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/ArtworkPackCache.kt`:
- Line 144: Update the expected-asset handling around
associateBy(MlipArtworkAsset::memberName) and the corresponding extraction logic
at the additional referenced block so duplicate memberName values are grouped
rather than overwritten. For each tar member, retain and extract all matching
required assets by id, ensuring duplicate member names do not cause the
containsAll validation to reject an otherwise valid pack.
In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporter.kt`:
- Around line 737-770: Update persistArtworkBindingManifest to replace the
temporary manifest using ArtworkPackCache.replaceAtomically instead of renameTo
with a copyTo fallback. Preserve the existing temporary-file creation and
cleanup, and ensure the final artwork-bindings.json replacement uses the
helper’s atomic move behavior.
- Around line 320-326: Define a named MLIP_ARTWORK_KIND_POSTER constant
alongside MLIP_ARTWORK_KIND_THUMB, then replace the literal 1 in the
posterBySeriesId filter and the artworkKind default in MlipArtworkBinding with
that constant.
- Around line 104-112: The artwork-pack caching flow in the
packedArtworkByAssetId initialization swallows construction or I/O failures
without logging them. Add failure logging to the runCatching chain using
MiruLog.w with TAG, a clear caching-failed message, and the caught exception,
while preserving the existing emptyMap fallback.
In
`@scanner/src/test/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporterTest.kt`:
- Around line 894-896: Update the assertion around packLogs to build the
diagnostic log message lazily using the assertion API’s failure-message lambda,
and add the appropriate ShadowLog import so the fully qualified reference is
removed. Preserve the existing log formatting and file-count assertion.
- Around line 880-885: Move the test method `rust generated v4 fixture caches
all bindings and only the incremental pack` and its `ONE_PIXEL_PNG_BASE64`
companion object above the private helper classes, preserving the file’s
organization with all `@Test` methods first and fake implementations afterward.
Keep `RecordingMetadataRepository` and `FakeMediaSource` together in the helper
section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 55e252fa-a698-45fb-8901-2a9dcdfcf1ba
⛔ Files ignored due to path filters (5)
scanner/src/test/resources/mlip-v4/base/MLIP-Artwork/8f4d445d8d8a13d6dd423c277b6f4c8212f8b73584cd0220462d0d6c68411487.taris excluded by!**/*.tarscanner/src/test/resources/mlip-v4/base/library.dbis excluded by!**/*.dbscanner/src/test/resources/mlip-v4/incremental/MLIP-Artwork/2a3b31f96f6b40f8cdcb55051e48b5f815f676f017a08f505dc795c71439e42d.taris excluded by!**/*.tarscanner/src/test/resources/mlip-v4/incremental/MLIP-Artwork/8f4d445d8d8a13d6dd423c277b6f4c8212f8b73584cd0220462d0d6c68411487.taris excluded by!**/*.tarscanner/src/test/resources/mlip-v4/incremental/library.dbis excluded by!**/*.db
📒 Files selected for processing (15)
app/build.gradle.ktsmedia-source/src/main/kotlin/com/miruplay/tv/mediasource/WebDavMediaSource.ktmedia-source/src/main/kotlin/com/miruplay/tv/mediasource/WebDavRequestCoordinator.ktmedia-source/src/test/kotlin/com/miruplay/tv/mediasource/WebDavRequestCoordinatorTest.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/Mp4DolbyVisionProbe.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactory.ktplayer-core/src/main/kotlin/com/miruplay/tv/player/RuntimeVideoTrackProbe.ktplayer-core/src/test/kotlin/com/miruplay/tv/player/PlaybackDataSourceFactoryTest.ktscanner/build.gradle.ktsscanner/src/main/kotlin/com/miruplay/tv/scanner/ArtworkPackCache.ktscanner/src/main/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporter.ktscanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.ktscanner/src/test/kotlin/com/miruplay/tv/scanner/MlipLibraryIndexImporterTest.ktui-tv/src/main/kotlin/com/miruplay/tv/ui/components/RemoteImage.kt
| val appApplicationId = providers.gradleProperty("APPLICATION_ID").orNull.nonBlankOrNull() | ||
| ?: "com.miruplay.tv" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== changed files/stat =="
git diff --stat || true
echo "== app/build.gradle.kts relevant lines =="
cat -n app/build.gradle.kts | sed -n '1,70p'
echo "== search for APPLICATION_ID references =="
rg -n 'APPLICATION_ID|applicationId|gradleProperty|environmentVariable' -S .
echo "== Gradle docs from current build files =="
if [ -f gradle/libs.versions.toml ]; then cat gradle/libs.versions.toml; fi
if [ -f gradle.properties ]; then cat gradle.properties; fiRepository: ModerRAS/MiruPlay
Length of output: 12839
Read APPLICATION_ID from the environment as well.
providers.gradleProperty("APPLICATION_ID") only handles -PAPPLICATION_ID=...; it does not pick up APPLICATION_ID from the Gradle process environment. Environment-only builds will stay on the fallback com.miruplay.tv.
val appApplicationId = providers.gradleProperty("APPLICATION_ID").orNull.nonBlankOrNull()
+ ?: providers.environmentVariable("APPLICATION_ID").orNull.nonBlankOrNull()
?: "com.miruplay.tv"📝 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.
| val appApplicationId = providers.gradleProperty("APPLICATION_ID").orNull.nonBlankOrNull() | |
| ?: "com.miruplay.tv" | |
| val appApplicationId = providers.gradleProperty("APPLICATION_ID").orNull.nonBlankOrNull() | |
| ?: providers.environmentVariable("APPLICATION_ID").orNull.nonBlankOrNull() | |
| ?: "com.miruplay.tv" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/build.gradle.kts` around lines 40 - 41, Update the appApplicationId
resolution to also read the APPLICATION_ID environment variable, while
preserving the existing Gradle property precedence and fallback to
"com.miruplay.tv".
| val remaining = request.deadlineEpochMillis - now() | ||
| if (remaining <= 0L || !queue.offer(work, remaining, TimeUnit.MILLISECONDS)) { | ||
| throw TimeoutException("Timed out enqueuing ${request.kind} request") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
TimeoutException escapes as a non-IOException.
submit throws bare java.util.concurrent.TimeoutException on enqueue/completion deadlines, while every other failure mode here is an IOException (WebDavHttpStatusException, WebDavCircuitOpenException). Downstream Media3 consumers (GatedPlaybackDataSource.open) and WebDavMediaSource's toWebDavError both assume IO-shaped failures, so a deadline surfaces as an unclassified error rather than a load error. Wrapping in InterruptedIOException/IOException keeps the contract uniform.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@media-source/src/main/kotlin/com/miruplay/tv/mediasource/WebDavRequestCoordinator.kt`
around lines 152 - 155, Update the timeout handling in submit so enqueue and
completion deadline failures are surfaced as IOException-compatible errors,
preferably InterruptedIOException while preserving the timeout context and
cause. Ensure no bare java.util.concurrent.TimeoutException escapes, while
leaving existing WebDavHttpStatusException and WebDavCircuitOpenException
behavior unchanged.
| val released = CountDownLatch(1) | ||
| val lease = WebDavLease(result.value) { | ||
| try { | ||
| result.close() | ||
| } finally { | ||
| released.countDown() | ||
| } | ||
| } | ||
| work.complete(lease) | ||
| if (work.request.streaming) { | ||
| released.await() | ||
| } else { | ||
| lease.close() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Unbounded released.await() can wedge an endpoint forever.
For streaming work the consumer thread blocks until the lease is closed, with no deadline. If any caller leaks a lease (early return, exception path in a caller, or an abandoned GatedPlaybackDataSource that never gets close()d), this endpoint's single worker never returns and every subsequent request on that authority (scan, artwork, probe, playback) fails by deadline — permanently, for the process lifetime. Consider bounding the wait (e.g. released.await(streamingLeaseTimeout)) and force-closing the transport when it elapses.
🛡️ Sketch
work.complete(lease)
if (work.request.streaming) {
- released.await()
+ if (!released.await(STREAMING_LEASE_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS)) {
+ runCatching { lease.close() }
+ }
} else {
lease.close()
}📝 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.
| val released = CountDownLatch(1) | |
| val lease = WebDavLease(result.value) { | |
| try { | |
| result.close() | |
| } finally { | |
| released.countDown() | |
| } | |
| } | |
| work.complete(lease) | |
| if (work.request.streaming) { | |
| released.await() | |
| } else { | |
| lease.close() | |
| } | |
| val released = CountDownLatch(1) | |
| val lease = WebDavLease(result.value) { | |
| try { | |
| result.close() | |
| } finally { | |
| released.countDown() | |
| } | |
| } | |
| work.complete(lease) | |
| if (work.request.streaming) { | |
| if (!released.await(STREAMING_LEASE_TIMEOUT_MILLIS, TimeUnit.MILLISECONDS)) { | |
| runCatching { lease.close() } | |
| } | |
| } else { | |
| lease.close() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@media-source/src/main/kotlin/com/miruplay/tv/mediasource/WebDavRequestCoordinator.kt`
around lines 211 - 224, Bound the streaming wait in the WebDavRequestCoordinator
flow around WebDavLease and released.await() using the configured streaming
lease timeout. If the wait expires, force-close the lease/underlying transport
and ensure the worker can complete; preserve the existing immediate close
behavior for non-streaming requests.
| if ( | ||
| _activeRenderBackend.value == PlaybackRenderBackend.EXPERIMENTAL_MPV_EMBEDDED && | ||
| !httpConfig.isWebDav(source.uri) | ||
| ) { | ||
| playWithEmbeddedMpv(source, httpConfig) | ||
| return@withContext | ||
| } | ||
| val player = activeExoPlayer() | ||
| val player = if (httpConfig.isWebDav(source.uri)) standardExoPlayer() else activeExoPlayer() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
WebDAV bypass changes the player but not _activeRenderBackend, so the rest of the controller talks to the wrong backend.
play() now routes WebDAV sources to standardExoPlayer() while _activeRenderBackend.value is left as EXPERIMENTAL_MPV_ANDROID/EXPERIMENTAL_MPV_EMBEDDED/EXPERIMENTAL_GL. Every other member still dispatches on that state:
pause()/resume()/seekTo()/setPlaybackSpeed()mutate mpv fields and return early — the actual Exo player keeps playing and ignores transport commands.getCurrentPosition()/getDuration()/isPlaying()report stale mpv values (0L).getPlayer()returnsnullandusesVlcVideoLayout()staystrue, so the UI binds an mpv/VLC surface that nothing renders into.- With
EXPERIMENTAL_GLrequested,isCurrentPlayer(player)compares the standard player againstactiveExoPlayer()(experimental) and returnsfalse, so everyPlayer.Listener/AnalyticsListenercallback is dropped and state never leavesLoading.
Force the backend state itself for WebDAV sources (e.g. in refreshRuntimeConfig/a fallback reason) instead of overriding only the local player reference.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@player-core/src/main/kotlin/com/miruplay/tv/player/ExoPlaybackController.kt`
around lines 213 - 220, Update the WebDAV routing flow in play() and the
associated runtime backend selection, including refreshRuntimeConfig or its
fallback-reason handling, so WebDAV sources set _activeRenderBackend to the
standard ExoPlayer backend before playback begins. Ensure all controller
methods, player identity checks, callbacks, and UI surface selection use that
synchronized backend state rather than only selecting standardExoPlayer()
locally.
| val connection = openConnection() | ||
| val statusCode = connection.responseCode | ||
| if (statusCode >= 400 && statusCode != 405) { | ||
| connection.disconnect() | ||
| throw WebDavHttpStatusException(statusCode) | ||
| } | ||
| WebDavTransportResult(connection, statusCode, connection::disconnect) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
3xx responses are accepted and probed as if they were media bytes.
With instanceFollowRedirects = false, a 302/307 now passes the statusCode >= 400 gate, and the redirect body (typically empty) is read as MP4 payload — the probe silently reports "no Dolby Vision" instead of failing. GatedPlaybackDataSource.openWebDav already uses statusCode !in 200..299; mirror that here (still letting 405 through so the coordinator can trip the circuit).
🐛 Proposed fix
val connection = openConnection()
val statusCode = connection.responseCode
- if (statusCode >= 400 && statusCode != 405) {
+ if (statusCode !in 200..299 && statusCode != 405) {
connection.disconnect()
throw WebDavHttpStatusException(statusCode)
}📝 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.
| val connection = openConnection() | |
| val statusCode = connection.responseCode | |
| if (statusCode >= 400 && statusCode != 405) { | |
| connection.disconnect() | |
| throw WebDavHttpStatusException(statusCode) | |
| } | |
| WebDavTransportResult(connection, statusCode, connection::disconnect) | |
| } | |
| val connection = openConnection() | |
| val statusCode = connection.responseCode | |
| if (statusCode !in 200..299 && statusCode != 405) { | |
| connection.disconnect() | |
| throw WebDavHttpStatusException(statusCode) | |
| } | |
| WebDavTransportResult(connection, statusCode, connection::disconnect) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@player-core/src/main/kotlin/com/miruplay/tv/player/Mp4DolbyVisionProbe.kt`
around lines 344 - 351, Update the response-status validation in the surrounding
WebDAV probe method to reject every status outside 200..299, while preserving
the existing exception for 405 so the coordinator can handle it. Keep successful
2xx responses flowing to WebDavTransportResult unchanged.
| val packTemp = File.createTempFile(pack.sha256.lowercase(), ".tmp", packsDirectory) | ||
| val stream = mediaSource.openStream(remotePath).getOrNull() ?: return emptyMap() | ||
| try { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Temp pack file leaks when the stream cannot be opened. packTemp is created before openStream, and the ?: return emptyMap() path exits before the try/finally that deletes it. Every failed pack fetch leaves a zero-byte <sha>*.tmp in packs/ forever.
🐛 Open the stream before creating the temp file
- val packTemp = File.createTempFile(pack.sha256.lowercase(), ".tmp", packsDirectory)
- val stream = mediaSource.openStream(remotePath).getOrNull() ?: return emptyMap()
+ val stream = mediaSource.openStream(remotePath).getOrNull() ?: return emptyMap()
+ val packTemp = File.createTempFile(pack.sha256.lowercase(), ".tmp", packsDirectory)📝 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.
| val packTemp = File.createTempFile(pack.sha256.lowercase(), ".tmp", packsDirectory) | |
| val stream = mediaSource.openStream(remotePath).getOrNull() ?: return emptyMap() | |
| try { | |
| val stream = mediaSource.openStream(remotePath).getOrNull() ?: return emptyMap() | |
| val packTemp = File.createTempFile(pack.sha256.lowercase(), ".tmp", packsDirectory) | |
| try |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/ArtworkPackCache.kt` around
lines 104 - 106, In the pack-fetch flow around packTemp and
mediaSource.openStream, open and validate the remote stream before calling
File.createTempFile. Ensure the early emptyMap return occurs before
temporary-file creation, while preserving the existing try/finally cleanup for
successfully opened streams.
| private fun validateImage(file: File, asset: MlipArtworkAsset): Boolean { | ||
| val prefix = file.inputStream().use { input -> ByteArray(12).also { input.read(it) } } | ||
| val expectedType = when { | ||
| prefix.size >= 3 && prefix[0] == 0xFF.toByte() && prefix[1] == 0xD8.toByte() && | ||
| prefix[2] == 0xFF.toByte() -> "image/jpeg" | ||
| prefix.copyOfRange(0, 8).contentEquals(PNG_SIGNATURE) -> "image/png" | ||
| prefix.copyOfRange(0, 4).decodeToString() == "RIFF" && | ||
| prefix.copyOfRange(8, 12).decodeToString() == "WEBP" -> "image/webp" | ||
| else -> return false | ||
| } | ||
| if (expectedType != asset.mediaType.lowercase()) return false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Signature sniffing relies on a single read filling 12 bytes. InputStream.read(ByteArray) may return fewer bytes than requested; a short read leaves zeros in the tail and makes a valid WEBP (RIFF….WEBP) fail validation, silently rejecting the asset. Use readNBytes(12)/DataInputStream.readFully semantics with a length check instead.
🐛 Read the full prefix
- val prefix = file.inputStream().use { input -> ByteArray(12).also { input.read(it) } }
+ val prefix = file.inputStream().use { input ->
+ val buffer = ByteArray(12)
+ var filled = 0
+ while (filled < buffer.size) {
+ val read = input.read(buffer, filled, buffer.size - filled)
+ if (read < 0) break
+ filled += read
+ }
+ if (filled < buffer.size) return false
+ buffer
+ }📝 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.
| private fun validateImage(file: File, asset: MlipArtworkAsset): Boolean { | |
| val prefix = file.inputStream().use { input -> ByteArray(12).also { input.read(it) } } | |
| val expectedType = when { | |
| prefix.size >= 3 && prefix[0] == 0xFF.toByte() && prefix[1] == 0xD8.toByte() && | |
| prefix[2] == 0xFF.toByte() -> "image/jpeg" | |
| prefix.copyOfRange(0, 8).contentEquals(PNG_SIGNATURE) -> "image/png" | |
| prefix.copyOfRange(0, 4).decodeToString() == "RIFF" && | |
| prefix.copyOfRange(8, 12).decodeToString() == "WEBP" -> "image/webp" | |
| else -> return false | |
| } | |
| if (expectedType != asset.mediaType.lowercase()) return false | |
| private fun validateImage(file: File, asset: MlipArtworkAsset): Boolean { | |
| val prefix = file.inputStream().use { input -> | |
| val buffer = ByteArray(12) | |
| var filled = 0 | |
| while (filled < buffer.size) { | |
| val read = input.read(buffer, filled, buffer.size - filled) | |
| if (read < 0) break | |
| filled += read | |
| } | |
| if (filled < buffer.size) return false | |
| buffer | |
| } | |
| val expectedType = when { | |
| prefix.size >= 3 && prefix[0] == 0xFF.toByte() && prefix[1] == 0xD8.toByte() && | |
| prefix[2] == 0xFF.toByte() -> "image/jpeg" | |
| prefix.copyOfRange(0, 8).contentEquals(PNG_SIGNATURE) -> "image/png" | |
| prefix.copyOfRange(0, 4).decodeToString() == "RIFF" && | |
| prefix.copyOfRange(8, 12).decodeToString() == "WEBP" -> "image/webp" | |
| else -> return false | |
| } | |
| if (expectedType != asset.mediaType.lowercase()) return false |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/ArtworkPackCache.kt` around
lines 228 - 238, Update validateImage to fully read up to 12 signature bytes
instead of relying on one input.read call. Use readNBytes or equivalent
readFully semantics, retain the actual byte count, and reject inputs shorter
than the required signature before checking PNG, JPEG, or WEBP markers.
| val download = { | ||
| val connection = (URL(url).openConnection(proxy) as HttpURLConnection).apply { | ||
| instanceFollowRedirects = false | ||
| connectTimeout = 10_000 | ||
| readTimeout = 20_000 | ||
| } | ||
| try { | ||
| if (connection.responseCode !in 200..299) { | ||
| throw WebDavHttpStatusException(connection.responseCode) | ||
| } | ||
| connection.getInputStream().use { input -> | ||
| temp.outputStream().use { output -> input.copyTo(output) } | ||
| } | ||
| } finally { | ||
| connection.disconnect() | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Duplicated coordinated artwork download logic in two modules. The same HttpURLConnection setup (redirects disabled, 10s/20s timeouts, 2xx check throwing WebDavHttpStatusException, disconnect() in finally) plus the WebDavRequestCoordinator.execute(… kind = ARTWORK) { WebDavTransportResult(Unit, 200) } wrapper is copy-pasted; timeout or status-handling changes will drift.
scanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.kt#L863-L879: replace with a call into a shared helper (e.g.media-source) that accepts URL, optionalProxy, and targetFile.ui-tv/src/main/kotlin/com/miruplay/tv/ui/components/RemoteImage.kt#L94-L110: call the same helper withProxy.NO_PROXY.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 863-863: An outbound HTTP request is built from a non-literal URL: URL(...).openConnection(), OkHttp's Request.Builder().url(...), or a Spring RestTemplate call (getForObject/getForEntity/postForObject/postForEntity/exchange/execute). When the URL is derived from a variable, method call, concatenation, or interpolation, an attacker can point the request at internal services, cloud metadata endpoints (e.g. 169.254.169.254), or arbitrary hosts (Server-Side Request Forgery). Do not build request targets from untrusted input: validate the URL against a strict allowlist of permitted hosts/schemes, resolve and re-check the host after DNS resolution, and reject internal/link-local/loopback addresses before connecting.
Context: URL(url).openConnection(proxy)
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-outbound-request-tainted-url-kotlin)
📍 Affects 2 files
scanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.kt#L863-L879(this comment)ui-tv/src/main/kotlin/com/miruplay/tv/ui/components/RemoteImage.kt#L94-L110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.kt` around
lines 863 - 879, Extract the duplicated artwork download behavior into one
shared media-source helper accepting a URL, optional Proxy, and target File;
centralize the HttpURLConnection configuration, 2xx validation with
WebDavHttpStatusException, stream copying, and guaranteed disconnect. In
scanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.kt lines
863-879, replace the local download lambda with this helper while preserving the
existing WebDavRequestCoordinator.execute artwork wrapper. In
ui-tv/src/main/kotlin/com/miruplay/tv/ui/components/RemoteImage.kt lines 94-110,
replace the duplicated implementation with the same helper using Proxy.NO_PROXY.
| WebDavRequestCoordinator.execute( | ||
| WebDavRequest(method = "GET", url = url, kind = WebDavRequestKind.ARTWORK), | ||
| ) { | ||
| download() | ||
| WebDavTransportResult(Unit, 200) | ||
| }.close() |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f 'WebDavRequestCoordinator.kt' --exec sed -n '1,200p'Repository: ModerRAS/MiruPlay
Length of output: 6986
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== WebDavRequestCoordinator.kt lines 200-360 =="
file="$(fd -t f 'WebDavRequestCoordinator.kt' | head -n 1)"
sed -n '200,360p' "$file"
echo "== ScanCoordinator.kt outline candidates around artwork =="
ast-grep outline scanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.kt --match "Download|Artwork|Artwork|poster" --view expanded || true
echo "== Relevant ScanCoordinator.kt lines 840-910 =="
scan_file="$(fd -t f 'ScanCoordinator.kt' | head -n 1)"
sed -n '840,910p' "$scan_file"
echo "== WebDavRequestKind.ARTWORK usages =="
rg -n "ARTWORK|WebDavRequestKind\.ARTWORK|executeBytes\(.*ARTWORK|execute\(" scanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.kt .Repository: ModerRAS/MiruPlay
Length of output: 14216
Avoid routing scan poster URLs through the WebDAV endpoint coalescer. cachePoster() always returns WebDavTransportResult(Unit, 200) after downloading, so the coordinator only processes a successful 405/circuit state and ignores all other HTTP status codes. At the same time, this path normalizes every downloaded URL host/port into a WebDAV endpoint queue, so unrelated CDN poster hosts are serialized and circuit-tracked with WebDAV media-same-endpoint traffic; move this to a non-coalescing download path unless poster URLs are WebDAV backend resources.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scanner/src/main/kotlin/com/miruplay/tv/scanner/ScanCoordinator.kt` around
lines 880 - 885, Update the poster download flow around
WebDavRequestCoordinator.execute and cachePoster so poster URLs do not pass
through WebDAV endpoint coalescing or circuit tracking. Use a direct
non-coalescing download path for non-WebDAV poster URLs, while preserving WebDAV
routing only when the URL is a WebDAV backend resource.
| val temp = File.createTempFile(file.name, ".tmp", file.parentFile) | ||
| val connection = URL(remoteUrl).openConnection().apply { | ||
| connectTimeout = 10_000 | ||
| readTimeout = 20_000 | ||
| } | ||
| try { | ||
| if (connection is HttpURLConnection && connection.responseCode !in 200..299) { | ||
| throw IOException("HTTP ${connection.responseCode}") | ||
| val download = { | ||
| val connection = (URL(remoteUrl).openConnection() as HttpURLConnection).apply { | ||
| instanceFollowRedirects = false | ||
| connectTimeout = 10_000 | ||
| readTimeout = 20_000 | ||
| } | ||
| connection.getInputStream().use { input -> | ||
| temp.outputStream().use { output -> | ||
| input.copyTo(output) | ||
| try { | ||
| if (connection.responseCode !in 200..299) { | ||
| throw WebDavHttpStatusException(connection.responseCode) | ||
| } | ||
| connection.getInputStream().use { input -> | ||
| temp.outputStream().use { output -> input.copyTo(output) } | ||
| } | ||
| } finally { | ||
| connection.disconnect() | ||
| } | ||
| if (!temp.renameTo(file)) { | ||
| temp.copyTo(file, overwrite = true) | ||
| } | ||
| } | ||
| WebDavRequestCoordinator.execute( | ||
| WebDavRequest( | ||
| method = "GET", | ||
| url = remoteUrl, | ||
| kind = WebDavRequestKind.ARTWORK, | ||
| ), | ||
| ) { | ||
| download() | ||
| WebDavTransportResult(Unit, 200) | ||
| }.close() | ||
| try { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Temp file leaks on every failed download. temp is created before the coordinated download, but cleanup lives only in the finally around the rename. Any throw from download()/execute (non-2xx, timeout, circuit open) is caught by the outer runCatching, leaving the .tmp file behind in the image cache — and failures are exactly the repeated case here.
🐛 Wrap the whole download in the cleanup scope
val temp = File.createTempFile(file.name, ".tmp", file.parentFile)
- val download = {
+ try {
+ val download = {
...
- }.close()
- try {
- if (!temp.renameTo(file)) temp.copyTo(file, overwrite = true)
+ }
+ WebDavRequestCoordinator.execute(
+ WebDavRequest(method = "GET", url = remoteUrl, kind = WebDavRequestKind.ARTWORK),
+ ) {
+ download()
+ WebDavTransportResult(Unit, 200)
+ }.close()
+ if (!temp.renameTo(file)) temp.copyTo(file, overwrite = true)
} finally {
temp.delete()
}📝 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.
| val temp = File.createTempFile(file.name, ".tmp", file.parentFile) | |
| val connection = URL(remoteUrl).openConnection().apply { | |
| connectTimeout = 10_000 | |
| readTimeout = 20_000 | |
| } | |
| try { | |
| if (connection is HttpURLConnection && connection.responseCode !in 200..299) { | |
| throw IOException("HTTP ${connection.responseCode}") | |
| val download = { | |
| val connection = (URL(remoteUrl).openConnection() as HttpURLConnection).apply { | |
| instanceFollowRedirects = false | |
| connectTimeout = 10_000 | |
| readTimeout = 20_000 | |
| } | |
| connection.getInputStream().use { input -> | |
| temp.outputStream().use { output -> | |
| input.copyTo(output) | |
| try { | |
| if (connection.responseCode !in 200..299) { | |
| throw WebDavHttpStatusException(connection.responseCode) | |
| } | |
| connection.getInputStream().use { input -> | |
| temp.outputStream().use { output -> input.copyTo(output) } | |
| } | |
| } finally { | |
| connection.disconnect() | |
| } | |
| if (!temp.renameTo(file)) { | |
| temp.copyTo(file, overwrite = true) | |
| } | |
| } | |
| WebDavRequestCoordinator.execute( | |
| WebDavRequest( | |
| method = "GET", | |
| url = remoteUrl, | |
| kind = WebDavRequestKind.ARTWORK, | |
| ), | |
| ) { | |
| download() | |
| WebDavTransportResult(Unit, 200) | |
| }.close() | |
| try { | |
| val temp = File.createTempFile(file.name, ".tmp", file.parentFile) | |
| try { | |
| val download = { | |
| val connection = (URL(remoteUrl).openConnection() as HttpURLConnection).apply { | |
| instanceFollowRedirects = false | |
| connectTimeout = 10_000 | |
| readTimeout = 20_000 | |
| } | |
| try { | |
| if (connection.responseCode !in 200..299) { | |
| throw WebDavHttpStatusException(connection.responseCode) | |
| } | |
| connection.getInputStream().use { input -> | |
| temp.outputStream().use { output -> input.copyTo(output) } | |
| } | |
| } finally { | |
| connection.disconnect() | |
| } | |
| } | |
| WebDavRequestCoordinator.execute( | |
| WebDavRequest( | |
| method = "GET", | |
| url = remoteUrl, | |
| kind = WebDavRequestKind.ARTWORK, | |
| ), | |
| ) { | |
| download() | |
| WebDavTransportResult(Unit, 200) | |
| }.close() | |
| if (!temp.renameTo(file)) temp.copyTo(file, overwrite = true) | |
| } finally { | |
| temp.delete() | |
| } |
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 94-94: An outbound HTTP request is built from a non-literal URL: URL(...).openConnection(), OkHttp's Request.Builder().url(...), or a Spring RestTemplate call (getForObject/getForEntity/postForObject/postForEntity/exchange/execute). When the URL is derived from a variable, method call, concatenation, or interpolation, an attacker can point the request at internal services, cloud metadata endpoints (e.g. 169.254.169.254), or arbitrary hosts (Server-Side Request Forgery). Do not build request targets from untrusted input: validate the URL against a strict allowlist of permitted hosts/schemes, resolve and re-check the host after DNS resolution, and reject internal/link-local/loopback addresses before connecting.
Context: URL(remoteUrl).openConnection()
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-outbound-request-tainted-url-kotlin)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@ui-tv/src/main/kotlin/com/miruplay/tv/ui/components/RemoteImage.kt` around
lines 93 - 121, Move the temporary-file cleanup scope to encompass the entire
coordinated download flow in the RemoteImage download logic, including
WebDavRequestCoordinator.execute and download(). Ensure temp is deleted whenever
any download, HTTP-status, timeout, or coordination failure occurs, while
preserving the existing rename behavior for successful downloads.
1 participant
Summary
Validation
Summary by CodeRabbit
New Features
Bug Fixes
Improvements