fix(torrent): address unresolved review findings from the v2 stack - #245
Merged
Merged
Conversation
Four automated review findings from PRs #232, #235, #236 and #237 were left unresolved when those branches were consolidated and merged. Metadata cache (#237, P1): pending fetches were keyed by info hash alone, so a TRACKER_ONLY caller arriving while a PUBLIC fetch was in flight joined that operation and inherited discovery it had opted out of, along with its rejection of private metadata. The privacy branch lives inside the fetch lambda, which only runs for the caller that creates the deferred, so the guard could never fire for the joining caller. Pending work is now keyed by hash and privacy. Completed entries stay shared because the hash authenticates them. Piece scheduler (#236): every HAVE frame rebuilt the peer's full availability as a BooleanArray and made the scheduler copy and rescan it, so a peer sending duplicate announcements forced repeated allocation and a full rarity scan per message. HAVE now applies incrementally; only the one-time bitfield rebuilds. Hash wire (#235): BEP 52 permits a request length of any positive power of two up to 512, but length 1 was rejected during decoding, closing otherwise valid v2 connections. The existing hashCount calculation already covers that case: a lone base hash covers no proof layer, so uncles is proofLayers + 1, which matches the invariant peerHashProofHeight enforces. Metainfo (#232): an unconditional requireNotNull rejected documents that omit "piece layers", which BEP 52 allows when no file exceeds the piece length. An absent key now parses as an empty dictionary; validatePieceLayers still rejects omission whenever the parsed files require external layers. The symlink race reported on #233 is not addressed here. Closing it needs a no-follow open plus handle-identity verification, which okio's commonMain FileSystem does not expose; validateOwned still rejects symlinks, checks file identity and revalidates after writing. The oversized-file finding on the same PR was already fixed before merge and only left unresolved on GitHub. Each fix carries a regression test. The two behavioural ones were confirmed to fail without their fix: reverting the pending key to ignore privacy fails restrictedCallersNeverJoinPendingPublicDiscovery, and removing the duplicate guard fails announce_buildsAvailabilityIncrementallyAndIgnoresRepeatedIndexes. Full suites pass on a forced rerun: 639 torrent and 203 core JVM tests, plus the iOS simulator target compiling clean.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four automated review findings from PRs #232, #235, #236 and #237 were left unresolved when those branches were consolidated into the merged stack. Each is still reproducible on
main, so this addresses them together.Metadata cache (#237, P1). Pending fetches were keyed by info hash alone, so a
TRACKER_ONLYcaller arriving while aPUBLICfetch was in flight joined that operation and inherited discovery it had explicitly opted out of, along with that operation's rejection of private metadata. The privacy branch sits inside the fetch lambda, which only runs for the caller that creates the deferred, so the guard could never fire for the joining caller. Pending work is now keyed by hash and privacy. Completed entries stay shared, since the hash authenticates them regardless of how they were discovered.Piece scheduler (#236). Every
HAVEframe rebuilt the peer's full availability as aBooleanArray, which the scheduler then copied and rescanned in full. A peer repeating announcements could force that allocation and scan per message, unbudgeted, at up to the one-million-piece limit.HAVEnow applies incrementally and only the one-time bitfield rebuilds availability.Hash wire (#235). BEP 52 permits a request
lengthof any positive power of two up to 512, butlength == 1was rejected during decoding, closing otherwise valid v2 connections. The existinghashCountcalculation already covers it: a lone base hash covers no proof layer, sounclesisproofLayers + 1, matching the invariantpeerHashProofHeightenforces.Metainfo (#232). An unconditional
requireNotNullrejected documents that omitpiece layers, which BEP 52 allows when no file exceeds the piece length. An absent key now parses as an empty dictionary;validatePieceLayersstill rejects omission whenever the parsed files genuinely require external layers.The symlink race reported on #233 is deliberately not addressed. Closing it needs a no-follow open plus handle-identity verification, which okio's
commonMainFileSystemdoes not expose;validateOwnedstill rejects symlinks, verifies file identity, and revalidates after writing. The oversized-file finding on the same PR was already fixed before merge and was only left unresolved on GitHub.Each fix carries a regression test. The two behavioural ones were confirmed to fail without their fix: reverting the pending key to ignore privacy fails
restrictedCallersNeverJoinPendingPublicDiscovery, and removing the duplicate guard failsannounce_buildsAvailabilityIncrementallyAndIgnoresRepeatedIndexes. Full suites pass on a forced rerun, 639 torrent and 203 core JVM tests, with the iOS simulator target compiling clean.