feat(local-media): harden download storage, recovery, and metadata - #396
Conversation
…load notifications
…over deduplication
…over deduplication
…, and remove legacy cancellation state
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c3509269f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| val frameData = bytes.copyOfRange(frameDataStart, frameDataEnd) | ||
| val value = decodeId3TextFrame(frameData) |
There was a problem hiding this comment.
When a tag contains a large binary frame such as the common APIC artwork frame, this copies the entire payload and passes it through decodeId3TextFrame before checking frameId. Since inspectImpl and inspectMetadataOnlyImpl run this parser for every directly accessible audio file, a folder scan can repeatedly turn multi-megabyte cover images into large temporary strings, multiplying memory and CPU use across parallel scans. Check whether the frame ID is one of the supported text fields before copying or decoding its payload.
Useful? React with 👍 / 👎.
| internal fun LocalMediaSupport.parseId3Metadata(bytes: ByteArray): ContainerMetadata? { | ||
| if (bytes.size < 10 || bytes.readAscii(0, 3) != "ID3") return null | ||
| val majorVersion = bytes[3].toInt() and 0xFF | ||
| val flags = bytes[5].toInt() and 0xFF |
There was a problem hiding this comment.
When an ID3v2 tag has the unsynchronization flag set, the tag payload contains inserted escape bytes that must be removed before frame boundaries and text are interpreted. This parser reads the flag but only uses the extended-header bit, so unsynchronized tags can produce corrupted or missing title, artist, and album fallback metadata when TagLib or retriever metadata is unavailable. De-unsynchronize the tag payload—and account for ID3v2.4 per-frame unsynchronization—before decoding frames.
Useful? React with 👍 / 👎.
| _scanPreviewState.value = preparedState.copy( | ||
| isScanning = false, | ||
| scanProgress = completedProgress | ||
| ) | ||
| if (result.metadataDeferred) { |
There was a problem hiding this comment.
For scans above the metadata-defer threshold, this marks scanning complete before starting the asynchronous metadata hydration job, so the preview immediately enables Import/Add-to-playlist while many rows still contain filename and unknown-artist placeholders. Dismissing the preview then cancels hydration, and all three scanned-song repository methods explicitly persist with hydrateLocalMetadata = false; the post-import refresh was also removed in this change. Keep the action busy until hydration settles, or hydrate the selected pending rows before persisting them.
Useful? React with 👍 / 👎.
| if (hydratedSong == null || targetIndex !in updatedSongs.indices) { | ||
| return@forEachIndexed |
There was a problem hiding this comment.
When metadata hydration returns null for an unreadable, corrupt, or temporarily inaccessible song, this exits without removing the target key from metadataPendingKeys. The metadata-only predicate treats every pending key as having metadata, so that row remains visible and selected indefinitely even after the hydration job has finished, defeating the filter and allowing a metadata-less entry to be imported. Mark the attempted key as no longer pending even when hydration fails.
Useful? React with 👍 / 👎.
| duplicateMetadataKeys = remapScanPreviewKeySet( | ||
| duplicateMetadataKeys, | ||
| previousSong, | ||
| hydratedSong | ||
| ) |
There was a problem hiding this comment.
When hydration changes a song's title, artist, or album, merely remapping an existing duplicate key cannot discover a new metadata collision or remove a collision that existed only in the quick scan. Consequently, on large deferred scans the “hide duplicate metadata songs” option uses stale results and may import duplicates or hide distinct tracks. Recompute duplicateScannedSongKeysByMetadata from updatedSongs after applying each hydrated batch.
Useful? React with 👍 / 👎.
| internal fun LocalAudioImportManager.copyIfExists(source: File, target: File) { | ||
| if (!source.exists() || target.exists()) { | ||
| return |
There was a problem hiding this comment.
When a previously imported document URI is replaced with different-sized audio, copyExternalAudioToTarget updates the cached audio, but every nearby lyric, cover, and metadata copy is skipped here because its old target already exists. Re-importing the URI therefore pairs the new audio with stale sidecars from the previous file. Track whether the audio was replaced and atomically refresh the corresponding imported sidecars in that case rather than unconditionally preserving them.
Useful? React with 👍 / 👎.
| return when (val result = target.backend.read(target.reference) { input -> | ||
| input.bufferedReader(Charsets.UTF_8).use { it.readText() } |
There was a problem hiding this comment.
When the selected download directory contains an oversized or provider-controlled metadata or lyric sidecar, this reads the entire stream into a String with no byte limit. Managed catalog scans, metadata reconciliation, and migration all use this helper, so one malformed multi-megabyte or unbounded document can cause excessive allocation or an OutOfMemoryError during startup or refresh. Apply explicit metadata and lyric limits while streaming instead of calling readText() to EOF.
Useful? React with 👍 / 👎.
…actions, and refine deletion safety
…peration tracking
…te from stale SAF cache
…x snapshot commit races
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8978517f51
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (uri.scheme.equals("content", ignoreCase = true) && uri.authority == MediaStore.AUTHORITY) { | ||
| return StabilizedExternalAudio(uri) | ||
| } |
There was a problem hiding this comment.
When the user has denied READ_MEDIA_AUDIO but opens a MediaStore item through ACTION_VIEW or ACTION_SEND, access may exist only through the intent's temporary URI grant. Returning the original URI here means the item plays during the current task, but the persisted player queue cannot reopen it after the task or process is gone. Copy these imports like other externally granted content URIs unless durable read access is already available.
Useful? React with 👍 / 👎.
| ?: resolver.getType(uri) | ||
| ?.substringAfterLast('/') | ||
| ?.substringAfter('+') | ||
| ?.takeIf { it.isNotBlank() } | ||
| ?: "audio" |
There was a problem hiding this comment.
When a content provider omits OpenableColumns.DISPLAY_NAME, deriving the suffix directly from the MIME subtype turns standard types such as audio/mpeg and audio/mp4 into .mpeg and .mp4. Neither suffix is in LOCAL_AUDIO_FILE_EXTENSIONS, so the initial import can play but later scans of the persisted Imports directory omit the file. Use an explicit MIME-to-extension mapping (for example, audio/mpeg to mp3 and audio/mp4 to m4a) and sanitize the result.
Useful? React with 👍 / 👎.
| val stat = Os.fstat(fd.fileDescriptor) | ||
| check(OsConstants.S_ISREG(stat.st_mode)) { "伴随文件不支持稳定普通文件身份" } | ||
| val bytes = ParcelFileDescriptor.AutoCloseInputStream(fd.dup()).use { it.readBytes() } |
There was a problem hiding this comment.
When an existing lyric, cover, or metadata companion is unusually large or provider-controlled, companionSnapshot reads the entire descriptor into one ByteArray before it can create a recovery backup. Editing or deleting that sidecar can therefore allocate an unbounded amount of memory and crash the process, despite normal lyric and cover reads elsewhere being capped. Stream the snapshot into the durable backup while enforcing the appropriate sidecar size limit instead of calling readBytes().
Useful? React with 👍 / 👎.
| }.onFailure { error -> | ||
| isMigrating = false | ||
| onInlineMessageChange( | ||
| resources.getString( | ||
| R.string.settings_download_directory_pick_failed, | ||
| error.message ?: "" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
If WorkManager rejects or throws while the user confirms a directory migration, this failure branch clears the pending change but leaves the newly acquired persistable permission for change.targetUri in place. The directory was never configured and the UI no longer retains a retry object, so repeated enqueue failures leak persisted URI grants until the platform's finite grant quota is exhausted. Release the target permission here when releaseTargetPermissionOnCancel is true.
Useful? React with 👍 / 👎.
| job = serviceScope.launch(start = CoroutineStart.LAZY) { | ||
| var wantsReschedule = false | ||
| try { | ||
| val result = DownloadExecutionHosts.default.execute(applicationContext, operationId) | ||
| wantsReschedule = shouldRescheduleUidtExecution(result) |
There was a problem hiding this comment.
When the app enters safe mode with a previously scheduled Android 14 UIDT job, NeriPlayerApplication deliberately skips initializeNormalComponents, but the system can still start this exported JobService and this path immediately executes the download host. Source resolution then reaches lazy AppContainer dependencies whose application was never initialized, causing the operation to fail/retry and allowing background download work to mutate state while safe mode is supposed to isolate normal components. Detect safe mode before host execution and leave the durable operation queued until normal startup is restored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a4a45cf29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| progress.emit( | ||
| phase = LocalAudioScanPhase.COMPLETED, | ||
| processed = maxOf(mediaStoreQueryTotalCount, acceptedRows), | ||
| total = maxOf(mediaStoreQueryTotalCount, acceptedRows), | ||
| discoveredSongs = completedSongs.size, |
There was a problem hiding this comment.
For an Android 10+ scan of a non-managed external-storage subfolder, this MediaStore helper emits COMPLETED, but scanFolderSongsInternal then performs the full DocumentsContract traversal and emits additional TRAVERSING, BUILDING_ENTRIES, and hydration updates before completing again. The scan preview consequently announces that the scan completed and then regresses to earlier phases, potentially for the duration of a slow provider traversal; reserve COMPLETED for the outer scan after both result sets are merged.
Useful? React with 👍 / 👎.
| val completion = completeScannedSongs( | ||
| context = context, | ||
| songs = rawResult.songs, | ||
| progress = progress, | ||
| visitedDirectories = 0, | ||
| knownSidecarReferences = knownSidecarReferences |
There was a problem hiding this comment.
When the MediaStore fast path is available, this hydrates all indexed songs before returning to scanFolderSongsInternal, which subsequently traverses the same folder and invokes completeScannedSongs again for the SAF candidates. For folders at or below the 256-song threshold, duplicate entries therefore undergo expensive embedded metadata, duration, and sidecar probing twice, doubling provider/file opens and delaying the scan; merge the quick MediaStore and SAF entries first and hydrate the merged set once.
Useful? React with 👍 / 👎.
| internal fun shouldProbeMediaStoreContentReference( | ||
| rowOrdinal: Int, | ||
| hasResolvedFile: Boolean, | ||
| probeLimit: Int = 256 | ||
| ): Boolean { | ||
| return hasResolvedFile || rowOrdinal in 1..probeLimit |
There was a problem hiding this comment.
For every MediaStore row with a valid resolved _data file, this returns true regardless of rowOrdinal, so both device and folder scans call openAssetFileDescriptor for every song even though shouldKeepMediaStoreAudioRow already accepts the resolved file and the probe result is unused. A large directly accessible library therefore performs thousands of unnecessary provider opens, defeating the stated bounded-probe policy and substantially slowing scans; probe only unresolved rows within the configured limit.
Useful? React with 👍 / 👎.
… SAF deletion performance
…pping and batching
…d non-blocking enrichment
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be83aa12aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (retainedFrame?.decodedBitmap != null) { | ||
| return retainedFrame |
There was a problem hiding this comment.
When playback moves from a song with a decoded cover to another song whose cover is not cached, displayedFrame still contains the first song's bitmap and this unconditional return selects it over the new requestedFrame. The composable consequently renders the decoded Image branch rather than the AsyncImage branch, so no request for the new artwork is ever started and the old cover can remain displayed indefinitely. Retain the old bitmap only as a placeholder while still composing/loading the requested frame, or reject retained frames whose source/owner differs.
Useful? React with 👍 / 👎.
| if (restoreBaseCover) { | ||
| existingOriginalCoverUrl |
There was a problem hiding this comment.
When restoring a local song whose originalCoverUrl is a legacy backup-directory reference, existingOriginalCoverUrl has already been cleared by the directory filter, while legacyOriginalCoverUrl contains the matching file found inside that directory. This branch nevertheless keeps the filtered value and discards the resolved file, so resolveRestoredBaseCoverUrl receives no original baseline and can clear the cover or reuse the current custom artwork instead of restoring the saved original. Prefer legacyOriginalCoverUrl here when it is available.
Useful? React with 👍 / 👎.
| ioScope.launch { | ||
| runSongMetadataMutation { | ||
| runCatching { | ||
| runLocalPlaylistMutationSafely("updateSongLyricsAndTranslation") { |
There was a problem hiding this comment.
If the local playlist repository write fails during the combined lyrics editor flow, runLocalPlaylistMutationSafely returns a failed Result rather than throwing, but that result is ignored here. The enclosing runCatching therefore continues updating the download catalog, history, usage data, and persisted queue and logs that synchronization succeeded while the playlist still contains the old lyrics; after reloading, users can see the edit revert or stores disagree. Inspect the returned result and stop the remaining fanout on failure.
Useful? React with 👍 / 👎.
…r and refine delete recovery
… and app sessions
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c131cf1e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (songChanged || sourceChanged) { | ||
| currentMediaArtwork = null | ||
| currentNotificationLargeIcon = null | ||
| coverResolutionGeneration += 1L |
There was a problem hiding this comment.
When playback moves to a song with no usable cover, this branch now retains currentNotificationLargeIcon; the new ownership check only marks the snapshot as not ready, while buildNotification() still passes that bitmap unconditionally to setLargeIcon. If asynchronous cover resolution returns no replacement, it never clears or refreshes the notification, so the previous song's artwork remains displayed indefinitely. Clear the retained bitmap here or gate the bitmap passed to the notification by the same owner/source check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2da7b4dfa1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| GlobalDownloadManager.syncDownloadedSongMetadataNow( | ||
| song = latestSong, | ||
| clearRestorableOverrides = RestorableMetadataClearPolicy( | ||
| userLyricOffset = true | ||
| ) |
There was a problem hiding this comment.
When the playlist database write throws, runLocalPlaylistMutationSafely returns a failed Result, but the newly added call still writes the new offset to the downloaded-song metadata and clears its restorable offset override. A transient Room failure therefore leaves the playlist with the old offset while the managed download records the new one, producing inconsistent values after reload; inspect the mutation result and only perform this sync after a successful playlist update.
Useful? React with 👍 / 👎.
| "UPDATE download_batch SET state_bits = " + | ||
| "(state_bits & ~1) | 8, updated_at_ms = :nowMs " + |
There was a problem hiding this comment.
Completing a batch only changes its state bits, and cancellation does the same; there is no deletion path anywhere for either download_batch or download_batch_member. Although terminal operations are pruned after seven days, every batch therefore permanently leaves one batch row and one member row per requested song, so repeated downloads make the user database and its backups grow without bound. Retain any batch needed for the current presentation, then add bounded retention that deletes terminal batches together with their members.
Useful? React with 👍 / 👎.
| }.onFailure { error -> | ||
| roomStorageEnabled = false | ||
| NPLogger.e( | ||
| "LocalPlaylistRepo", | ||
| "Failed to read Room playlists; falling back to legacy storage", |
There was a problem hiding this comment.
After Room cutover, the cleanup scheduler deletes both local_playlists.json and its backup, so a transient exception from readIfRoomPrimary() reaches this branch, disables Room, and makes readStoredPlaylists() initialize the library from missing files as an empty/system-only collection. If the user then performs a playlist mutation, the fallback path writes that incomplete collection and marks legacy JSON primary, making the intact Room playlists unreachable on later launches. Distinguish a legitimate non-primary null result from a Room read failure and retry or fail initialization when Room is authoritative instead of falling back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 04182028de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!targetFile.exists()) return true | ||
| if (!targetFile.isFile) return true | ||
| if (targetFile.length() <= 0L) return true | ||
| return expectedBytes != null && targetFile.length() != expectedBytes |
There was a problem hiding this comment.
When a document keeps the same URI and byte length but its contents are replaced, this returns false and reuses the old cached audio. The subsequent code even stamps that stale target with the source's new modification time, so re-importing cannot recover until the size changes or the cache is removed. Compare the source modification time or durable content identity in addition to length before skipping the copy.
Useful? React with 👍 / 👎.
| pendingChange = null | ||
| return | ||
| } | ||
| pendingChange = null |
There was a problem hiding this comment.
When the user chooses the no-migration path and another managed-library operation wins the race after guardDirectoryChange, this clears the pending change before tryBeginExclusive rejects the apply. The catch only displays an error, leaving the newly persisted target URI grant unused and removing the UI object that could release or retry it; repeated races can consume the platform's persisted-grant quota. Clear the pending state only after the target is adopted, or release its grant on failures that occur before configuration changes.
Useful? React with 👍 / 👎.
| if (belongsToRoot && marker.exists() && !marker.delete()) { | ||
| NPLogger.w(loggerTag, "清理已确认空目录标记失败: ${marker.name}") |
There was a problem hiding this comment.
After a confirmed full-library deletion, a transient failure to delete the marker here is only logged while the nonempty Room rows and backup are still committed. On the next restore, isConfirmedEmpty is checked before either source and returns an empty catalog solely because this stale file still matches the root, hiding every subsequently downloaded song. Make nonempty persistence fail or retry when the marker cannot be removed, or only honor the marker when the durable catalog is actually empty.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83cf6a301d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 元信息只能识别历史上丢失来源引用的条目, 不能把两个真实文件合并 | ||
| if (includeMetadataFallback && isEmpty()) { | ||
| addMetadataFallbackKeys(song) |
There was a problem hiding this comment.
When a historical local entry has lost all source references, it produces only metadata fallback keys, while the newly scanned copy now produces only its path/content/audio keys because this isEmpty() condition suppresses its metadata keys. Consequently, callers that explicitly enable fallback—such as LocalScanDuplicateIndex and SystemPlaylistSongDeduper—cannot match the legacy entry to the current file and may import or display it twice. Avoid comparing metadata between two referenced files, but still expose it when one side is a reference-less legacy entry.
Useful? React with 👍 / 👎.
| seed = resolveLyricsEditorSeed( | ||
| song = actualSong, | ||
| preparedLyrics = displayedLyricsText, | ||
| preparedTranslatedLyrics = displayedTranslatedLyricsText |
There was a problem hiding this comment.
For a non-local song whose romanized/phonetic lyrics were fetched for the current display but have not been persisted into actualSong, this branch snapshots displayedRomanizedLyricsText but passes only the displayed original and translated text to the editor seed. The romanized field therefore opens empty, and saving the combined draft can replace the currently displayed romanization with an empty value. Pass the displayed romanized snapshot through resolveLyricsEditorSeed alongside the other two lyric variants.
Useful? React with 👍 / 👎.
| selectedSongKeys = selectedSongKeys | ||
| ) | ||
| if (songsPendingDelete.isNotEmpty()) { | ||
| deleteEntireLibraryPending = fullLibrarySelectionRequested | ||
| showMultiDeleteDialog = true |
There was a problem hiding this comment.
If the downloaded-song list changes after the user presses Select All—for example, an active download completes—selectedSongKeys no longer equals the current library, but fullLibrarySelectionRequested remains true. Pressing Delete then copies this stale flag even though the UI's allSelected value is false, and the backend takes the full-library path, scans the entire managed directory, and deletes the newly added unselected song too. Derive this flag from the captured selection and current list when deletion is requested, or clear it whenever downloadedSongs changes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: beff210f94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pendingChange = PendingDownloadDirectoryChange( | ||
| previousUri = previousUri, | ||
| targetUri = targetUri, | ||
| targetSummary = targetSummary, | ||
| releaseTargetPermissionOnCancel = releaseTargetPermissionOnCancel, |
There was a problem hiding this comment.
If the process is killed while this migration-confirmation dialog is open, the persistable URI permission acquired during preflight survives restart, but pendingChange exists only in Compose state and is lost. Since neither the configured directory nor the migration journal references the target yet, startup cannot release or adopt that grant, and repeated interruptions can exhaust Android's persisted-grant quota. Persist the pending target for startup reconciliation, or defer/recover the grant independently of this UI state.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| internal fun LocalMediaSupport.encodeEditableCoverAsJpeg(sourceBytes: ByteArray): ByteArray? { | ||
| val bitmap = BitmapFactory.decodeByteArray(sourceBytes, 0, sourceBytes.size) ?: return null |
There was a problem hiding this comment.
When an unsupported MP4 cover format must be transcoded, a compressed image can remain below the 16 MiB byte limit while declaring dimensions large enough to require hundreds of megabytes when decoded. This call decodes the full-resolution bitmap without first checking MAX_COVER_PIXELS or setting inSampleSize, so a malformed remote cover or oversized selected image can crash metadata saving with an OutOfMemoryError. Probe the bounds and reject or downsample before allocating the bitmap.
Useful? React with 👍 / 👎.
| child.isDirectory -> pendingDirectories.add( | ||
| PendingDirectory( | ||
| uri = child.documentUri, | ||
| isInsideManagedRoot = directory.isInsideManagedRoot || |
There was a problem hiding this comment.
When a DocumentsProvider returns the selected directory itself, an ancestor, or the same directory through multiple aliases as a child, this unconditionally queues it again and the traversal has no visited-document set. A buggy or provider-controlled tree can therefore make folder scanning loop indefinitely while growing the queue and repeatedly querying the provider. Deduplicate directory document IDs or canonical URIs before enqueueing them, and apply the same protection to the DocumentFile fallback.
Useful? React with 👍 / 👎.
| context.applicationContext.filesDir, | ||
| "$UNCONFIRMED_FILE_PREFIX${epoch}_${UUID.randomUUID()}.json" | ||
| ) |
There was a problem hiding this comment.
Every unconfirmed full-library deletion moves its intent to a new UUID-named archive, but the only other reference to these files is an epoch-specific existence probe; no path removes archives after that epoch's clear fence is released. Repeated provider failures can therefore permanently retain one potentially large target list per attempt in filesDir. Delete the archive after successful fence release, while retaining only the current crash-recovery marker.
Useful? React with 👍 / 👎.
| val matchedRomanizedLyric: String? = null, | ||
| val originalRomanizedLyric: String? = null, |
There was a problem hiding this comment.
When the current song or a synced playlist contains fetched or user-edited romanized lyrics, these new fields are absent from PersistedSongItem, PlaybackQueueSongEntity, and SyncSong. Even when queue persistence explicitly retains the current song's embedded lyrics, a process restart drops both romanized values, and GitHub/WebDAV synchronization also omits them while preserving the original and translated variants. Add the fields to the queue and sync schemas and their bidirectional mappers.
Useful? React with 👍 / 👎.
| val descriptor = context.contentResolver.openFileDescriptor(reference.toUri(), "rwt") | ||
| ?: throw IOException("伴随文件不可写") | ||
| descriptor.use { fd -> | ||
| val stat = Os.fstat(fd.fileDescriptor) | ||
| check(OsConstants.S_ISREG(stat.st_mode) && "${stat.st_dev}:${stat.st_ino}" == identity) { "伴随写入对象已改变" } |
There was a problem hiding this comment.
When a sidecar is replaced between the transaction snapshot and this write, opening it with mode rwt can truncate the newly mapped file before fstat compares its inode with the recorded identity. The subsequent check then correctly rejects the replacement, but its contents may already be zeroed, defeating the transaction's conflict protection and losing a concurrent lyric, cover, or metadata update. Open with rw, verify the identity, and only then call ftruncate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 616ca001c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // queue/current-song state is published immediately; catalog fanout stays off the editor's critical path | ||
| ioScope.launch { |
There was a problem hiding this comment.
For a remote song, or whenever local sidecar persistence is disabled, this detached ioScope.launch is the only path that writes the edited lyrics to the playlist repository and persisted queue, yet the method immediately returns true and callers display success or close the editor. If the process is killed or this scope is cancelled before the coroutine reaches persistState, the acknowledged edit disappears on restart; persist the queue/repository state before returning success and defer only the nonessential fanout.
Useful? React with 👍 / 👎.
| AudioDownloadTransferPolicy.resolveResumeValidatorHeader(resumeFingerprint).isNullOrBlank() | ||
| ) { | ||
| AudioDownloadLog.d("续传缺少 If-Range 校验符，回退整文件重下: ${destFile.name}") | ||
| resumedBytes = 0L |
There was a problem hiding this comment.
When a working file has bytes but no strong resume validator, this only resets the counter and leaves the old payload on disk. downloadResponse then persists the new response fingerprint before opening the file in truncating mode, so a process interruption or file-open failure in that interval leaves the stale bytes paired with the new validator; the next retry treats those bytes as belonging to the new response and can append a 206 suffix to them. Delete or durably truncate the working file in this branch, as is already done when the source key changes.
Useful? React with 👍 / 👎.
| "WHERE batch_id = :batchId AND generation = :generation " + | ||
| "AND state_bits & ${DownloadBatchState.OPEN} != 0 " + | ||
| "AND state_bits & ${DownloadBatchState.TERMINAL_MASK} = 0 " + | ||
| "AND state_bits & ${DownloadBatchState.CLEARING} = 0 " + | ||
| "AND network_generation >= :expectedNetworkGeneration" |
There was a problem hiding this comment.
If the network generation changes while a batch confirmation dialog is open, the old request can race with publication of the replacement request. The batch path in continueDownloadsOnMobileDataAndWake does not verify that the request ID or generation is still current, and this >= predicate accepts the old generation against the newer batch row, clears its Wi-Fi fence, and rewrites its operations with requiresWifiNetwork=false. This can apply consent from an earlier prompt to a later cellular connection; require an exact generation match (and reject a non-current request) before granting the override.
Useful? React with 👍 / 👎.
1 participant
No description provided.