Conversation
There was a problem hiding this comment.
Pull request overview
Adds detection for potentially truncated background-sync protobuf downloads (primarily Watch background refresh) by comparing the downloaded byte count against the response’s expected content length, with unit tests for the completeness check.
Changes:
- Introduce
BackgroundSyncManager.isDownloadComplete(receivedBytes:expectedContentLength:)helper for validating download completeness. - Add a feature-flagged check in the URLSession download delegate to drop data when a Content-Length mismatch is detected (and log it).
- Add unit tests covering common completeness/mismatch scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Modules/Server/Sources/PocketCastsServer/Public/Sync/BackgroundSyncManager.swift | Adds the download completeness helper used by background sync. |
| Modules/Server/Sources/PocketCastsServer/Public/Sync/BackgroundSyncManager+URLSession.swift | Applies the completeness check after download completion, behind a feature flag, and logs/drops mismatching payloads. |
| Modules/Server/Tests/PocketCastsServerTests/BackgroundSyncManagerTests.swift | Adds unit tests for the completeness helper across expected/unknown/mismatch cases. |
You can also share your feedback on Copilot code review. Take the survey.
| if FeatureFlag.detectTruncatedBackgroundSyncDownloads.enabled { | ||
| let expectedLength = downloadTask.response?.expectedContentLength ?? -1 | ||
| if let receivedData = data, !BackgroundSyncManager.isDownloadComplete(receivedBytes: receivedData.count, expectedContentLength: expectedLength) { | ||
| FileLog.shared.addMessage("Background sync data truncated for task \(downloadTask.taskDescription ?? "unknown"): received \(receivedData.count) bytes, expected \(expectedLength)") |
There was a problem hiding this comment.
I don't think this is likely and we have no evidence. In theory, the URLSession background task is downloading an incomplete response (due to disconnect or something). It shouldn't receive a response greater than expected.
Modules/Server/Tests/PocketCastsServerTests/BackgroundSyncManagerTests.swift
Show resolved
Hide resolved
Modules/Server/Sources/PocketCastsServer/Public/Sync/BackgroundSyncManager+URLSession.swift
Show resolved
Hide resolved
Modules/Server/Sources/PocketCastsServer/Public/Sync/BackgroundSyncManager.swift
Outdated
Show resolved
Hide resolved
Modules/Server/Sources/PocketCastsServer/Public/Sync/BackgroundSyncManager.swift
Show resolved
Hide resolved
NSURLResponseUnknownLength is not available from Swift
There was a problem hiding this comment.
Pull request overview
Adds detection of potentially truncated protobuf downloads during background sync by comparing received bytes against the HTTP Content-Length, gated behind a feature flag.
Changes:
- Introduces
detectTruncatedBackgroundSyncDownloadsfeature flag. - Adds byte-count vs
expectedContentLengthvalidation helper inBackgroundSyncManager. - Adds unit tests for the completeness-check helper.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Modules/Utils/Sources/PocketCastsUtils/Feature Flags/FeatureFlag.swift | Adds a new feature flag and sets its default enabled behavior. |
| Modules/Server/Sources/PocketCastsServer/Public/Sync/BackgroundSyncManager.swift | Introduces isDownloadComplete and an “unknown length” constant used for validation. |
| Modules/Server/Sources/PocketCastsServer/Public/Sync/BackgroundSyncManager+URLSession.swift | Applies the truncation detection during background sync processing and drops data when mismatched. |
| Modules/Server/Tests/PocketCastsServerTests/BackgroundSyncManagerTests.swift | Adds unit tests for the byte-count validation helper. |
You can also share your feedback on Copilot code review. Take the survey.
| true | ||
| case .cleanUpTmpFiles: | ||
| true | ||
| case .detectTruncatedBackgroundSyncDownloads: | ||
| true |
| /// Returns `false` if the received byte count doesn't match the expected Content-Length, | ||
| /// indicating the download was truncated or corrupt. | ||
| /// Value returned by `URLResponse.expectedContentLength` when the length is unknown. | ||
| static let unknownContentLength: Int64 = -1 |
There was a problem hiding this comment.
This looks like a good suggestion.
There was a problem hiding this comment.
Unfortunately, NSURLResponseUnknownLength was not available in Swift. It's an Objective-C macro instead of a constant. This commit message mentions it.
| // Verify the download completed fully by comparing received bytes to Content-Length. | ||
| // A truncated download can produce valid-but-incomplete protobuf that silently drops fields. | ||
| if FeatureFlag.detectTruncatedBackgroundSyncDownloads.enabled { | ||
| let expectedLength = downloadTask.response?.expectedContentLength ?? BackgroundSyncManager.unknownContentLength | ||
| if let receivedData = data, !BackgroundSyncManager.isDownloadComplete(receivedBytes: receivedData.count, expectedContentLength: expectedLength) { | ||
| FileLog.shared.addMessage("Background sync data truncated for task \(downloadTask.taskDescription ?? "unknown"): received \(receivedData.count) bytes, expected \(expectedLength)") | ||
| data = nil | ||
| } | ||
| } |
SergioEstevao
left a comment
There was a problem hiding this comment.
Looking good, I tested this on my watch and phone and all worked correctly.
There is very small nitpick comment regarding the unknow size constant that I think it's a good suggestion, but up to you.
| /// Returns `false` if the received byte count doesn't match the expected Content-Length, | ||
| /// indicating the download was truncated or corrupt. | ||
| /// Value returned by `URLResponse.expectedContentLength` when the length is unknown. | ||
| static let unknownContentLength: Int64 = -1 |
There was a problem hiding this comment.
This looks like a good suggestion.
There are indications from our Up Next Sentry logging that users are receiving truncated protobuf responses. Theoretically, this could lead to "empty" Up Next responses where the response truncated is exactly between the
serverModifieddate and the list of episodes.The changes here verify that we received the full response by checking the content-length header and verifying the byte count we received.
Two things limit the scope of these changes:
detectTruncatedBackgroundSyncDownloadsFeature FlagBackgroundSyncManager.performBackgroundRefreshTo test
This is difficult to test because Watch ↔ iPhone syncing doesn't work well in the simulators and I cannot figure out how to proxy the watch connection to rewrite the response.
Overall, this can be smoke tested by running the Watch app in Watch mode for a while and making sure new episodes are synced.
Checklist
CHANGELOG.mdif necessary.