fix(par2repair): converge repairs on gapped, trimmed and taken-down releases - #876
Closed
javi11 wants to merge 20 commits into
Closed
fix(par2repair): converge repairs on gapped, trimmed and taken-down releases#876javi11 wants to merge 20 commits into
javi11 wants to merge 20 commits into
Conversation
…, and masked-credential fixes (#842)
…d optimize seek latency (#844)
… one slice A repair that fails IFSC MD5 verification reported a single, arbitrary slice number: verifyRecovered iterated a map and returned on the first mismatch, so the reported slice was whichever came up first rather than the lowest or the only one. That made the error impossible to reason about. Collect every mismatch, sort ascending, and report the lowest plus the count. Log a structured record alongside it carrying the facts needed to tell the possible causes apart: - how many of the recovered slices failed, and across how many files - each failing slice's file, local index and dead-article adjacency - how that file's article sizes were derived, via a new SetFile.SizeSource (metadata / probed / borrowed_hint / encoded_fallback) - whether the gf16 SIMD backend is active - whether the Main packet's FileIDs arrived in the expected order - a held-out-row check Solver.VerifyHeldOutRow re-derives a recovery row the solve never used from the recovered slices. A held-out row still satisfies the same equation but was not part of the inverted system, so agreement means the arithmetic and coefficients are self-consistent and the inputs or their numbering are wrong, while disagreement implicates the fold or the solve itself. Also close test gaps this work exposed: - TestSolverAwkwardSliceSizes covers 2380956 (a multiple of 4 but of no higher power of two) and other shipping sizes, exercising the SIMD prepared layout and the parallel fold's stride-aligned split at real dimensions. - TestRunJobAbsorbsManyMidSweepDeadArticles exercises the margin-row absorb path at production scale, where a dozen articles die mid-sweep across many members, rather than the single absorb previously covered. - TestRunJobCorruptOverflowFallsBackToReplan no longer names one article when asserting that replans re-swept; recovery-set order decides which member is swept first, and an attempt that overflows early never reaches the later files at all.
…aphically
Every recovered slice of a damaged release failed its IFSC MD5 verification
("41 of 41 recovered slices affected"), even though the release carried seven
times the recovery data it needed.
PAR2 orders FileIDs with byte 15 most significant -- par2cmdline's
MD5Hash::operator<, which compares from the last byte down. parseMainBody
sorted them with bytes.Compare, which is byte 0 most significant. The two
orderings differ, and the recovery-set file order defines the global input
slice numbering, which selects each slice's Vandermonde constant. A permuted
order therefore hands the solver the wrong coefficient for every slice.
Nothing else in the pipeline could see it. Present slices still passed their
IFSC CRC32, which is indexed per file and so unaffected by global numbering,
and recovery payloads still passed their own packet MD5. Only the final check
on reconstructed slices caught it.
Stop re-sorting: the Main packet's stored order is authoritative for slice
numbering, and preserving it is correct for any creator. Record whether that
order matched the convention in Index.MainIDsWereSorted for diagnostics, and
document fileIDLess so the comparator is not quietly "corrected" again.
Two independent blind spots had let this survive:
- par2gen used the same wrong comparator, writing Main packets
lexicographically and numbering slices to match, so every generated fixture
agreed with the bug. It now emits PAR2-conventional order.
- Every test reconstructing from real par2cmdline output used idx.Recovery[0],
whose exponent is 0. With g^0 = 1 all coefficients collapse to 1, the
equation degenerates to a plain XOR of all slices, and the result stops
depending on slice numbering -- so the external fixture could not detect the
bug even in principle. All such tests now take a non-zero exponent via
nonZeroExponentRef, which fails rather than silently picking exponent 0.
The existing fixtures also have only two members, where the two orderings
coincide about half the time. New fixture testdata/ordercheck holds ten real
par2cmdline files whose orderings provably differ, and its tests were verified
to fail with the bug present and pass with it fixed. testdata/realtool_large
adds a 131072-byte slice size so the parallel fold and the SIMD prepared
layout are checked against real tool output, which the 1024-byte fixture never
reached.
…are dead Two gaps in the article-sizing path could fail a repairable release. Neither corrupts a repair; both reject or degrade one that would have worked. sizePar2SetFiles had no fallback. A PAR2 volume whose non-final articles are all dead kept its yEnc-ENCODED NZB sizes, so its length and every BodyOffset inside it drifted by the yEnc overhead. Every recovery payload in that volume then failed its RecvSlic packet MD5 and was dropped as "unreachable or corrupt", silently costing the repair a whole volume's recovery rows. Size such volumes in a second pass instead, borrowing the release-wide decoded part size from a volume that could be probed -- usenet posts split every file of a release at one uniform decoded part size, so a sibling's probe is authoritative. This mirrors what sizeArticles already does for content members via its hint. A volume whose final article is also dead keeps that article's declared size, since a PAR2 volume has no FileDesc recording its true length, but every offset before it is corrected -- and those are what locating recovery payloads depends on. Only when no volume in the release has a probeable non-final article do sizes stay encoded. sizeArticles probed any live segment, including the last. The final article is the short remainder, so probing it yields a part size far below the real one and the uniformity check then rejects the file: a release whose only live article in some member was the last one failed as "part size inconsistent" even though a sibling's part size was available to borrow. Probe non-final articles only, so such a file reports errNoLiveArticle and the caller retries with the release-wide part size. Both paths now record SizeBorrowedHint, so a later verification failure names the guess in its diagnostics.
…kdir (#845) Co-authored-by: javi11 <javicuba11@gmail.com>
Co-authored-by: plumpygibbon <plumpygibbon@gmail.com>
…eleases - usenet: validate repaired patches against the article's full size (SegmentSize), not the trimmed range end — archive inner-file segments are article sub-ranges, so every straddling patch was silently rejected and successful repairs were thrown away in a repair loop - nzbgap (new): segment numbers absent from the NZB become deterministic synthetic placeholder articles; the importer defers such releases for repair, the NZB-mode planner marks them dead, and patches land under the synthetic IDs the read path looks up - par2repair: patch-aware fetcher (patched articles count alive and serve their patch) plus a plan-time terminal error when the only remaining dead articles are outside the PAR2 recovery set — ends the defer→repair→defer loop for partially coverable releases - par2repair: cap size-derivation probes per file, report the planning stage before the sizing fetches, and remember a 430 verdict inside the parse streams — a mostly-purged volume cost ~22 re-fetches per dead article while resync crawled its zeroed regions (hours of silent 18%) - par2: parse PAR2 streams concurrently with an ordered merge that keeps sequential semantics; planning progress now counts completed streams - par2repair: per-attempt Body timeout so a pool request orphaned by a dying connection (nntppool commit-then-die wedge) costs minutes, not the whole job deadline - cmd/par2diag (new): instrumented NZB-mode planning harness used to diagnose these failures against live providers
Resolve conflicts between the PAR2 streaming repair work and main's content-verification, metadata v3 migration and health-check changes: - serve.go/setup.go: health worker and filesystem take both the PAR2 repair service and the content-verify opener - api/server.go: keep par2Repair/par2RepairRepo alongside metadataMigrationWorker - stremio_addon_handlers.go: adopt main's findHealthyLibraryStreams helper - config/manager.go: keep both sets of defaults - health/worker.go: adopt main's activeCheck map - importer/service.go: keep both imports - HealthTableRow.tsx: keep repairReason and playbackImpact
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
4 participants
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.
Summary
Fixes a family of failure modes where PAR2 repair either threw away completed work or ground forever at a frozen progress percentage:
internal/usenet): repaired payloads are whole decoded articles, but archive inner-file segments map trimmed sub-ranges of an article — the oldEnd+1equality rejected every straddling patch, so a successful repair was discarded and the import re-parked in an endless repair loop.internal/nzbgap, new): segment numbers the NZB never listed (never-indexed articles with no message ID) become deterministic synthetic placeholder articles. The fast-fail gate defers such releases for repair, NZB-mode planning treats the placeholders as dead, and the recovered payloads are stored under the same synthetic IDs the read path resolves — repairing damage that previously failed imports with "incomplete NZB data, cannot auto-patch".ErrUnrepairablewhen the only remaining dead articles sit in files the PAR2 set does not cover — one real repair plus one cheap plan instead of re-downloading the release every cycle.ParseIndexWithProgress): every PAR2 file's packet chain is walked concurrently and merged in stream order (byte-identical semantics: metadata last-write-wins, recovery exponents first-seen-wins). Planning wall-clock divides by roughly the file count.PoolFetcher): defends against an observed nntppool wedge where a committed request orphaned by a dying connection never receives its response — the attempt now cancels after 2 minutes and retries on a fresh request instead of freezing until the job deadline.cmd/par2diag(new): instrumented NZB-mode planning harness (per-fetch IDs, durations, stage progress) used to diagnose these failures against live providers.Test plan
TestReaderServesFullArticlePatchOnTrimmedSegment,nzbgapsuite,TestResolveFromNzbRepairsSegmentMissingFromNzb,TestResolveFromNzbSecondCycleTerminatesOnUncoveredGaps,TestPatchAwareFetcher*,TestSizePar2SetFilesCapsProbesPerVolume,TestParseIndexReadsStreamsConcurrently,TestParseIndexParallelMatchesSequentialSemantics,TestPoolFetcherAttemptTimeout*,TestLazyFileReaderFetchesVanishedArticleOnce)go test ./internal/...— all packages passgo test -raceoninternal/par2repairandinternal/importer/parser/par2unrepairableverdict in minutes instead of hanging