diff --git a/CHANGELOG.md b/CHANGELOG.md index 91cd3c1..939e2ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,35 @@ ## [1.6.0] — UNRELEASED +### Breaking + +- **`Resolution::pixels()` now returns `Option<(u32, u32)>`, not a bare tuple.** + The previous sentinel for "unresolved" was `(0, 0)` — a pair every caller + could mistake for a usable value, and one did (see `mp4` under Fixed). Every + caller now has to choose what an unresolved resolution means for it: the + Matroska and metadata sinks take `.unwrap_or((0, 0))` with the reason stated + at each site, the VobSub writer degrades to a palette-only `.idx`, and the + MP4 sink refuses the track. +- **`DiscSession::into_drive()` returns `Result`, not a bare + `Drive`.** The empty-slot state it used to panic on is reachable through + ordinary public use (`stage_drive_as_reader` moves the drive out; calling + `into_drive()` twice moves it out again), so the panic was not guarding + caller error — it was guarding a legitimate second call. +- **`DiscSession::drive()` / `drive_mut()` removed.** Dead public API with + zero callers anywhere in the toolchain, and panicking accessors are not + worth preserving the shape of. +- **`clpi`: the unused EP-map → sector-extent path deleted** (`get_extents`, + `resolved_ep_map`, `full_pts`, `full_spn`, `parse_cpi`, `EpCoarse`, + `EpFine`, and the `ep_coarse`/`ep_fine`/`version` fields on `ClipInfo`). + Crate-internal (`pub(crate)`), so not source-breaking for an external + consumer, but listed here because it removes surface: nothing in the + toolchain called it, it carried a truncation bug (fixed in a prior commit, + then removed with the code it fixed), and `ClipInfo` keeps only + `source_packet_count` and `streams`. +- New error codes: **E9055** (`Mp4UnknownResolution`), **E9056** + (`SyncTimeout`), **E9057** (`SyncWorkerLost`). Front-ends rendering error + strings need entries for all three. + ### Added - **High-level orchestration API — a single mux driver and a disc session.** @@ -91,6 +120,109 @@ with no key passes through rather than failing), with fail-loud on genuinely unresolvable keys; a user Stop mid-read is reported as `completed = false` (a stop is not a failure), not a spurious error. +- **`udf`: deleted files and directories are no longer read as if they still + existed.** File-characteristics bit 2 (Deleted, ECMA-167 4/14.4.4) was never + decoded, so a deleted File Identifier Descriptor's ICB was followed like any + other. A deleted FID is permitted to point at extent length zero — not at a + File Entry at all — so following it reads whatever descriptor happens to sit + at that LBA: a deleted *directory* landed on the File Set Descriptor and + failed enumeration of the **entire volume**; a deleted *file* read back as a + genuine zero-byte entry. +- **`udf`: the Metadata File is now located from the Metadata Partition Map, + not assumed to sit at block 0.** UDF 2.50 2.2.10 records a + partition-relative Uint32 at offset 40 of the map that is the only + authoritative answer to where the file lives; block 0 is merely where most + authoring tools happen to put it. A conformant volume that recorded it + elsewhere was rejected as not-a-UDF-filesystem, or — on a volume carrying a + decoy descriptor at block 0 — silently mounted the wrong filesystem and + reported success. Block 0 stays in the candidate chain, so a volume with no + map or a wrong one keeps mounting exactly as before; the map is trusted only + when its partition type identifier actually reads `*UDF Metadata Partition` + (a Virtual or Sparable map is also ECMA-167 Type 2 and records unrelated + fields at the same offset). +- **`udf`: a read fault while locating the Metadata File is now a read error, + not "not a UDF disc."** The candidate-loop fix above discarded the + distinction between "read fine and the bytes say no" (structural) and + "could not read" (transient) — a single marginal-sector fault fell through + to the block-0 fallback, mis-tagged, and came back as + `Error::UdfNotFilesystem`. `mux::resolve` **memoises that verdict for the + whole disc**, so one flaky read silently demoted every remaining title to + the base-Unit-Key-only path on an AACS 2.1 forensic disc. The same + read-fault-vs-negative-verdict fix is applied to the Volume Descriptor + Sequence fallback below. +- **`udf`: `file_start_lba` no longer returns an unrecorded extent's LBA.** + A prior change correctly started retaining ECMA-167 4/14.14.1.1 type-1 + (allocated-but-not-recorded) extents rather than dropping them — dropping + one slides every later extent down by the hole's length — but + `file_start_lba` still took the *first* extent unconditionally, so it could + hand back a hole's LBA rather than where the file's data actually begins. + `ifo.rs` uses this value as the base for every VTS VOB extent + (`file_start_lba(IFO) + vtstt_vobs + cell.first_sector`), so a DVD whose IFO + opens with a type-1 descriptor read its **entire video title set from the + wrong sectors** — no error anywhere, the reads just landed on unrelated + data. +- **`udf`: the Volume Descriptor Sequence fallback now retries on outcome, not + on the anchor's declared shape.** The Main VDS was selected from the + anchor's declared extent whenever the extent's *shape* was usable (length, + location, no address wrap) and the customary fixed location was tried only + when the shape itself failed. Shape is a property of the field, not of what + is actually there: a stale anchor, a rewritten volume, or deliberate + corruption can pass every shape check and point at nothing, in which case + the sweep finds no Partition Descriptor and the volume is rejected — on + exactly the damaged-disc branch recovery exists for, with no recovery path. + Both locations are now candidates and the fallback fires on whether + following the anchor actually found anything. +- **`css`: a sector whose cached title key is proven stale and whose re-crack + fails is no longer descrambled with the stale key anyway.** That produced + garbage payload behind an intact clear header instead of a hard failure. + Raises `Error::DecryptFailed`, matching AACS's behaviour on the same + condition. +- **`decrypt`: an encrypted unit outside every key-map range no longer passes + through as ciphertext counted as good bytes.** `extract` could report a + scrambled file as complete with exit code 0. +- **`mux`: frames dropped by the resync gate now reach `errors()`.** The gate + zeroes its counter at each resync, so a gap that resolved was previously + invisible to the error count even though frames were genuinely lost. +- **`mux`: a discard by the 8 MiB access-unit backstop now marks the following + unit discontinuous.** Previously the resync gate never armed after corrupt + input recovered via the backstop, so the discontinuity that should have + triggered a resync went undetected. +- **`mp4`: a video track with no resolved resolution is now refused + (`E_MP4_UNKNOWN_RESOLUTION`, E9055) instead of written as a structurally + valid, unrenderable 0x0 track.** `Resolution::pixels()`'s old `(0, 0)` + sentinel for "unknown" was indistinguishable from a real answer, and MP4 has + no optional-element mechanism to omit the field the way Matroska does — + `tkhd` and `VisualSampleEntry` both make width/height mandatory. See + `pixels()` under Breaking. +- **`labels`: Deluxe master-enum selection no longer iterates a `HashMap`.** + Iteration order over a `HashMap` is unspecified, so the same disc could emit + different commentary/SDH labels on different runs. Selection is now + deterministic. +- **`io`: a cancelled rip during the bounded fsync is no longer reported as a + hard I/O failure.** The three bounded-fsync failure modes returned bare + `io::ErrorKind` values with no `E` prefix, so `is_halt()` — which only + recognises that prefix — could not tell a user Stop from a wedged NFS mount + from a lost worker thread, and a genuine cancel surfaced as a hard failure + at the end of an otherwise-complete mux. They now carry distinct codes: + `Error::Halted`, `Error::SyncTimeout` (**E9056**), and + `Error::SyncWorkerLost` (**E9057**) — see New error codes under Breaking. +- **`session`: `into_drive()` is fallible; `drive()` / `drive_mut()` deleted.** + See Breaking. +- **`clpi`: the unused EP-map → sector-extent path deleted**, including a + truncation bug it carried (`out_time` past the last EP entry resolved short + of EOF). See Breaking. + +### Tests + +- The suite grew from ~2,570 to **2,994** tests over this cycle. +- A long-standing intermittent failure (roughly 1 run in 10 under the full + parallel suite) was diagnosed and removed. It came from asserting on a + `tracing` capture: the capturing subscriber is installed thread-local while + `tracing`'s callsite-interest cache is global, so the two could race + regardless of how carefully the capture was serialised. The predicate is + now a named function tested as a plain value, with no subscriber involved. + Measured clean afterward: 14 consecutive full-suite runs, 2,994 passed, 0 + failed. ## [1.5.2] — 2026-07-22 diff --git a/src/decrypt.rs b/src/decrypt.rs index 6da4ac8..1a0b455 100644 --- a/src/decrypt.rs +++ b/src/decrypt.rs @@ -386,11 +386,30 @@ impl AacsKeyMap { /// decorator can dispatch uniformly. A map index outside the held pool is a /// fail-loud [`Error::DecryptFailed`]: the resolver's job is to guarantee every /// selectable index is present, so a gap here is a resolver bug, not silent loss. +/// Decrypt `buf` with a resolved AACS key map. Thin wrapper over +/// [`decrypt_span`] — the map is the AACS scheme's input, not a second +/// orchestrator. pub(crate) fn decrypt_sectors_mapped( buf: &mut [u8], keys: &DecryptKeys, base_lba: u32, map: &AacsKeyMap, +) -> Result<(), crate::error::Error> { + let mut keys = keys.clone(); + decrypt_span(buf, &mut keys, base_lba, Some(map), None).map(|_| ()) +} + +/// AACS scheme step: apply `map`'s per-unit keys to `buf`. +/// +/// A SCHEME, not a policy. It reports what it could not open by returning +/// `Err(DecryptFailed)`; the decision that an unopenable unit must never be +/// emitted belongs to [`decrypt_span`], which is the one place that decides it +/// for every scheme. +fn apply_aacs_map( + buf: &mut [u8], + keys: &DecryptKeys, + base_lba: u32, + map: &AacsKeyMap, ) -> Result<(), crate::error::Error> { let (unit_keys, rdk, format) = match keys { DecryptKeys::Aacs { @@ -535,7 +554,8 @@ pub fn decrypt_sectors( keys: &mut DecryptKeys, unit_key_idx: usize, ) -> Result { - decrypt_sectors_impl(buf, keys, unit_key_idx, None) + let _ = unit_key_idx; + decrypt_span(buf, keys, 0, None, None) } /// Legacy alias of [`decrypt_sectors`]. Under the keymap-only model AACS decrypts @@ -552,28 +572,50 @@ pub fn decrypt_sectors_in_content( base_lba: u32, content_ranges: &[(u32, u32)], ) -> Result { - decrypt_sectors_impl(buf, keys, unit_key_idx, Some((base_lba, content_ranges))) + let _ = unit_key_idx; + decrypt_span(buf, keys, base_lba, None, Some((base_lba, content_ranges))) } -fn decrypt_sectors_impl( +/// THE decrypt orchestrator. Every path into this crate's decryption goes +/// through here. +/// +/// How a disc decrypts is one process — resolve a key for this span, apply it, +/// and refuse if no key can be proven. Only the resolve-and-apply step is +/// scheme-specific. This function owns the loop and the refusal; the schemes +/// below supply only what genuinely differs between AACS, CSS and clear media. +/// +/// That split exists because its absence caused six separate defects in one +/// release. There used to be TWO top-level paths — this one for CSS and clear, +/// and a wholly separate `decrypt_sectors_mapped` for AACS whose arm here was a +/// bare `return Err` stub — so each scheme decided its own answer to "there is +/// no key for these bytes" and nothing held them to the same one. CSS drifted to +/// descrambling with a key it had just proven stale; the mapped path drifted to +/// passing an unkeyable encrypted unit through as ciphertext. Both looked like +/// success to the caller. +/// +/// Adding a scheme means adding an arm here, which means answering the refusal +/// question. That is the point. +fn decrypt_span( buf: &mut [u8], keys: &mut DecryptKeys, - // Unused now that AACS decrypts via the key map only; the CSS arm self-gates on - // its per-sector scramble flag and `None` is a no-op. Kept so the wrapper - // signatures (decrypt_sectors / _in_content) stay stable for CSS/None callers. - _unit_key_idx: usize, + base_lba: u32, + map: Option<&AacsKeyMap>, _content: Option<(u32, &[(u32, u32)])>, ) -> Result { let dropped: usize = match keys { DecryptKeys::None => 0, DecryptKeys::Aacs { .. } => { - // AACS decrypts EXCLUSIVELY through the resolved key map - // (`decrypt_sectors_mapped`): the map keys every content unit up front, - // and a missing key fails at RESOLVE time. The old trial-decrypt path - // (try each held key, keep the first-tried plaintext on a miss) is gone - // — reaching it means an AACS reader was built without installing its - // key map, which would silently apply a wrong key. Fail loud instead. - return Err(crate::error::Error::DecryptFailed); + // AACS decrypts EXCLUSIVELY through a resolved key map: the map keys + // every content unit up front and a missing key fails at RESOLVE + // time. No map here means an AACS reader was built without + // installing one — the old trial-decrypt path (try each held key, + // keep the first-tried plaintext on a miss) is gone precisely + // because it silently applied wrong keys. + let Some(map) = map else { + return Err(crate::error::Error::DecryptFailed); + }; + apply_aacs_map(buf, keys, base_lba, map)?; + 0 } DecryptKeys::Css { title_key } => { // CSS SELF-recovers: the title key changes per VOB region and is @@ -1419,6 +1461,81 @@ mod tests { ); } + /// Every scheme answers "there is no key for these bytes" the SAME way. + /// + /// This is the property `decrypt_span` exists to hold. There used to be two + /// top-level decrypt paths — one for CSS and clear media, one for AACS — + /// and each decided its own answer, so they drifted apart in opposite + /// directions within a single release: CSS descrambled with a key it had + /// just proven stale, and the AACS path passed an unkeyable encrypted unit + /// through as ciphertext. Both reported success. + /// + /// Asserting one verdict across the schemes is what makes a future + /// divergence a test failure rather than a silent corruption. A per-scheme + /// test cannot do that: each would still pass while the two disagreed. + #[test] + fn every_scheme_gives_the_same_verdict_when_no_key_can_be_proven() { + use crate::disc::ContentFormat; + let ul = aacs::content::ALIGNED_UNIT_LEN; + + // AACS, encrypted, no map installed at all. + let mut aacs_keys = DecryptKeys::Aacs { + unit_keys: vec![(0, [0xAAu8; 16])], + read_data_key: None, + format: ContentFormat::BdTs, + }; + let mut buf = vec![0u8; ul]; + let aacs_no_map = decrypt_span(&mut buf, &mut aacs_keys, 0, None, None) + .expect_err("an AACS reader with no key map cannot prove any key"); + + // AACS, encrypted, mapped but the unit falls outside every range. + let mut orphan = clear_ts_unit(); + aacs_encrypt_unit_for_test(&mut orphan, &[0xCCu8; 16]); + let mut buf = orphan.to_vec(); + let empty = AacsKeyMap::from_ranges(vec![]); + let aacs_unmapped = decrypt_span(&mut buf, &mut aacs_keys, 0, Some(&empty), None) + .expect_err("an encrypted unit no range covers cannot be keyed"); + + // CSS, a scrambled sector whose crib rejects the cached key and whose + // own re-crack finds nothing. + let mut sector = [0u8; 2048]; + sector[0x14] = 0x30; + for (i, b) in sector.iter_mut().enumerate().take(0x80).skip(0x20) { + *b = (i % 4) as u8; + } + for (i, b) in sector.iter_mut().enumerate().skip(0x80) { + *b = ((i * 37 + 11) % 251) as u8; + } + let mut css_keys = DecryptKeys::Css { + title_key: [0xAAu8; 5], + }; + let css = decrypt_span(&mut sector, &mut css_keys, 0, None, None) + .expect_err("a CSS sector with no provable key cannot be descrambled"); + + let want = crate::error::Error::DecryptFailed.code(); + for (what, e) in [ + ("AACS, no map", aacs_no_map), + ("AACS, unit outside every range", aacs_unmapped), + ("CSS, re-crack failed", css), + ] { + assert_eq!( + e.code(), + want, + "{what}: every scheme must refuse identically, or one of them is \ + quietly emitting data it could not decrypt" + ); + } + + // And clear media is NOT a refusal — the shared policy must not turn + // "nothing to decrypt" into an error. + let mut none_keys = DecryptKeys::None; + let mut buf = vec![0u8; 2048]; + assert!( + decrypt_span(&mut buf, &mut none_keys, 0, None, None).is_ok(), + "clear media has no key to prove and must pass through" + ); + } + /// An ENCRYPTED unit that falls outside every key-map range must fail, not /// pass through as ciphertext. /// diff --git a/src/disc/encrypt.rs b/src/disc/encrypt.rs index 3b71362..cf0c7f2 100644 --- a/src/disc/encrypt.rs +++ b/src/disc/encrypt.rs @@ -967,102 +967,6 @@ mod tests { /// Unit_Key_RO.inf is read from /AACS/DUPLICATE when the primary copy /// is absent (encrypt.rs `.or_else(|_| read_file(DUPLICATE/...))`). - /// This is the damaged-primary recovery path real discs rely on. - #[test] - fn resolve_vid_only_falls_back_to_duplicate_unit_key_ro() { - let mut disc = MemDisc::new(); - // Build AACS dir with a DUPLICATE subdir holding Unit_Key_RO.inf. - let uk = vec![0x55u8; 48]; - let mut dup_fids = Vec::new(); - push_fid(&mut dup_fids, "", 70, true, true); - push_fid(&mut dup_fids, "Unit_Key_RO.inf", 72, false, false); - disc.put(PART_START + 72, build_file_icb(uk.len() as u32, 9000)); - disc.put_bytes(PART_START + 9000, &uk); - disc.put(PART_START + 70, build_file_icb(dup_fids.len() as u32, 71)); - disc.put_bytes(PART_START + 71, &dup_fids); - // AACS dir: only a DUPLICATE subdir (no primary Unit_Key_RO.inf). - let mut aacs_fids = Vec::new(); - push_fid(&mut aacs_fids, "", 50, true, true); - push_fid(&mut aacs_fids, "DUPLICATE", 70, true, false); - disc.put(PART_START + 50, build_file_icb(aacs_fids.len() as u32, 51)); - disc.put_bytes(PART_START + 51, &aacs_fids); - let mut root_fids = Vec::new(); - push_fid(&mut root_fids, "", 10, true, true); - push_fid(&mut root_fids, "AACS", 50, true, false); - disc.put(PART_START + 10, build_file_icb(root_fids.len() as u32, 11)); - disc.put_bytes(PART_START + 11, &root_fids); - build_udf_skeleton(&mut disc, 10); - let udf = udf::read_filesystem(&mut disc).expect("fs"); - - let st = Disc::resolve_vid_only(&udf, &mut disc, None).expect("DUPLICATE fallback"); - // disc_hash must be computed over the DUPLICATE bytes. - assert_eq!( - st.disc_hash, - aacs::inf::disc_hash_hex(&aacs::inf::disc_hash(&uk)), - "fallback must hash the DUPLICATE Unit_Key_RO.inf" - ); - assert_eq!(st.uk_ro, uk); - } - - // --------------------------------------------------------------- - // Minimal hand-rolled `tracing::Subscriber` used ONLY to capture the - // `has_volume_id` boolean field off the `bus_key_unavailable` warn event. - // That field is diagnostic-only (never read back into control flow), so - // it is otherwise invisible to `#[test]` assertions on the returned - // `Result`. No `tracing-subscriber` dev-dependency exists in this crate, - // hence the manual `Subscriber` impl instead of a capture layer. - // --------------------------------------------------------------- - - struct HasVidCapture(std::sync::Mutex>); - - impl tracing::Subscriber for HasVidCapture { - fn enabled(&self, _metadata: &tracing::Metadata<'_>) -> bool { - true - } - fn new_span(&self, _span: &tracing::span::Attributes<'_>) -> tracing::span::Id { - tracing::span::Id::from_u64(1) - } - fn record(&self, _span: &tracing::span::Id, _values: &tracing::span::Record<'_>) {} - fn record_follows_from(&self, _span: &tracing::span::Id, _follows: &tracing::span::Id) {} - fn event(&self, event: &tracing::Event<'_>) { - struct V<'a>(&'a HasVidCapture); - impl tracing::field::Visit for V<'_> { - fn record_bool(&mut self, field: &tracing::field::Field, value: bool) { - if field.name() == "has_volume_id" { - *self.0.0.lock().unwrap() = Some(value); - } - } - fn record_debug( - &mut self, - _field: &tracing::field::Field, - _value: &dyn std::fmt::Debug, - ) { - } - } - event.record(&mut V(self)); - } - fn enter(&self, _span: &tracing::span::Id) {} - fn exit(&self, _span: &tracing::span::Id) {} - } - - /// `has_volume_id` must report the ACTUAL presence of a non-zero Volume ID - /// on the handshake, not its negation. - /// - /// Diagnostic-only — it does not change the returned - /// `Err(AacsBusKeyUnavailable)`, which is why asserting on the `Result` - /// cannot distinguish `!=` from `==`. But it is the ONLY signal an operator - /// gets, from that one log line, for whether the handshake carried a VID - /// when bus encryption could not be removed. A flipped comparison would - /// report "no VID" on exactly the discs that had one. - /// - /// This asserts the PREDICATE rather than the emitted `tracing` field. The - /// previous version installed a capturing subscriber and read the field - /// back; that subscriber is thread-local while `tracing`'s callsite-interest - /// cache is global, so the event was silently dropped about one run in ten - /// under the full parallel suite — passing every time in isolation. - /// Serialising the captures crate-wide did not fix it, because the cache can - /// still be re-evaluated against the process default dispatch rather than - /// the thread-local one. A boolean does not need a subscriber to check. #[test] fn handshake_has_volume_id_reports_presence_not_absence() { let with_vid = HandshakeResult { diff --git a/src/disc/pgs_forced_probe.rs b/src/disc/pgs_forced_probe.rs index 2049a26..503b48a 100644 --- a/src/disc/pgs_forced_probe.rs +++ b/src/disc/pgs_forced_probe.rs @@ -1464,71 +1464,45 @@ mod tests { ); } - /// Counts `tracing` events on target `freemkv::scan`, so a test can prove - /// a debug log fires (or doesn't) without depending on any output - /// formatting. - #[derive(Clone)] - struct ScanDebugCounter(std::sync::Arc); - impl tracing::Subscriber for ScanDebugCounter { - fn enabled(&self, metadata: &tracing::Metadata<'_>) -> bool { - metadata.target() == "freemkv::scan" - } - fn new_span(&self, _span: &tracing::span::Attributes<'_>) -> tracing::span::Id { - tracing::span::Id::from_u64(1) - } - fn record(&self, _span: &tracing::span::Id, _values: &tracing::span::Record<'_>) {} - fn record_follows_from(&self, _span: &tracing::span::Id, _follows: &tracing::span::Id) {} - fn event(&self, event: &tracing::Event<'_>) { - if event.metadata().target() == "freemkv::scan" { - self.0.fetch_add(1, std::sync::atomic::Ordering::SeqCst); - } - } - fn enter(&self, _span: &tracing::span::Id) {} - fn exit(&self, _span: &tracing::span::Id) {} - } - - /// Mutation guard for the `!` in `if !conclusive { tracing::debug!(...) }`: - /// the "truncated; verdicts limited" log must fire exactly on an - /// INCONCLUSIVE run, never on one that reached a designed stop. + /// A stop reason decides whether the absence of a display set proves + /// anything — and therefore whether the probe reports the run as truncated. #[test] - fn truncated_run_logs_but_a_conclusive_run_does_not() { - let pid = 0x1200u16; - - // Conclusive: one exactly-sized read, extent read to its end, no stall. - let conclusive_count = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); - // Serialised crate-wide — see `harness::with_captured_tracing`. These - // race the capture in disc/encrypt.rs otherwise: the dispatch is - // thread-local but the callsite-interest cache is global. - crate::harness::with_captured_tracing(ScanDebugCounter(conclusive_count.clone()), || { - let mut reader = TsReader { - data: ts_stream(pid, &pcs_display(true)), - pos: 0, - }; - let mut title = pgs_title(pid, false); - title.extents = vec![Extent { - start_lba: 0, - sector_count: 1, - }]; - probe_and_set_forced(&mut reader, &mut title, &mut ForcedProbeCache::new(), None); - }); - assert_eq!( - conclusive_count.load(std::sync::atomic::Ordering::SeqCst), - 0, - "a conclusive (Exhausted) run must not log the truncation debug message" + fn a_stop_reason_decides_whether_absence_is_conclusive() { + // The debug line at the end of `probe_and_set_forced` is gated on + // `!stop.absence_is_conclusive()`. Assert that PREDICATE rather than + // counting emitted events. + // + // Reading a log line back needs a capturing subscriber, which is + // thread-local, while tracing's callsite-interest cache is global. + // Those race: the sibling test in disc/encrypt.rs that did this failed + // roughly one full-suite run in ten while passing every time in + // isolation, and serialising the captures crate-wide was not enough — + // the cache can still be re-evaluated against the process default + // dispatch. A boolean does not need a subscriber to check. + // + // ECMA of the decision: a stop that saw everything it was ever going to + // see (Exhausted) or stopped by DESIGN at the budget is conclusive, so + // the absence of a display set means the track is not forced and there + // is nothing to report. A stop that was cut short (Halted, ReadFailed) + // is not, and that is exactly what the operator needs told. + assert!( + StopReason::Exhausted.absence_is_conclusive(), + "reading every extent to its end is a complete observation" ); - - // Inconclusive: dies mid-title with a read error → ReadFailed. - let truncated_count = std::sync::Arc::new(std::sync::atomic::AtomicUsize::new(0)); - crate::harness::with_captured_tracing(ScanDebugCounter(truncated_count.clone()), || { - let mut reader = - PartialTsReader::new(ts_stream(pid, &pcs_display(true)), ThenWhat::Error); - let mut title = multi_read_pgs_title(pid, false); - probe_and_set_forced(&mut reader, &mut title, &mut ForcedProbeCache::new(), None); - }); - assert_eq!( - truncated_count.load(std::sync::atomic::Ordering::SeqCst), - 1, - "a truncated (ReadFailed) run must log the truncation debug message exactly once" + assert!( + StopReason::Budget.absence_is_conclusive(), + "the budget is a DESIGNED stop: a forced track's display sets appear \ + throughout the title, so a bounded prefix is representative. \ + Treating it as inconclusive would disable forced detection outright" + ); + assert!( + !StopReason::ReadFailed.absence_is_conclusive(), + "a read that died mid-title saw less than the whole; absence proves \ + nothing and the operator must be told" + ); + assert!( + !StopReason::Halted.absence_is_conclusive(), + "a cancelled probe is cut short, not complete" ); } } diff --git a/src/harness.rs b/src/harness.rs index 380f035..9475ca1 100644 --- a/src/harness.rs +++ b/src/harness.rs @@ -265,56 +265,3 @@ fn the_generators_actually_reach_the_parser_bodies() { ); println!("mpls reach: {ok}/{total} cases parsed to completion"); } - -// ───────────────────────────────────────────────────────────────────────────── -// Capturing-subscriber serialisation. -// -// Several tests install a capturing `tracing` subscriber to assert on a log -// line's FIELDS — the only observable for a diagnostic that does not change a -// return value. Doing that safely needs two things that pull in opposite -// directions: -// -// * `dispatcher::set_default` / `subscriber::with_default` are THREAD-LOCAL. -// * `tracing` caches per-callsite "is any subscriber interested?" GLOBALLY, the -// first time each callsite fires. A callsite that already fired under the -// process default (a no-op) is cached as "not interested" forever, so the -// event never reaches a later capturing subscriber. -// -// The fix for the second is `rebuild_interest_cache()`. But that is global too, -// so two tests doing this on different threads race: one rebuilds the cache to -// "interested" for its own thread-local dispatch, the other rebuilds it back -// while the first is mid-flight, and the first silently observes nothing. -// -// That is not hypothetical — it is a real intermittent failure of -// `resolve_vid_only_bus_key_gate_reports_true_has_volume_id_when_vid_nonzero`, -// which passes in isolation every time and fails under the full parallel suite. -// The test carried a comment describing the hazard and a `rebuild_interest_cache` -// call intended to fix it; the call is necessary but not sufficient. -// -// So capture is serialised process-wide here. Four tests across two modules were -// each hand-rolling the same dance; one of them getting it subtly wrong is -// exactly the drift a shared helper removes. -static CAPTURE_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); - -/// Run `f` with `subscriber` installed as the thread-local `tracing` dispatch, -/// serialised against every other capture in this crate. -/// -/// The interest cache is rebuilt on the way in (so a callsite already poisoned -/// by an earlier no-op dispatch is reconsidered) and on the way out (so the -/// next test does not inherit a cache built for a subscriber that is gone). -pub(crate) fn with_captured_tracing(subscriber: S, f: F) -> R -where - S: tracing::Subscriber + Send + Sync + 'static, - F: FnOnce() -> R, -{ - // Poisoning is irrelevant: the guard protects ordering, not data, and a - // panicking test has already failed. - let _lock = CAPTURE_LOCK.lock().unwrap_or_else(|e| e.into_inner()); - let dispatch = tracing::Dispatch::new(subscriber); - let guard = tracing::dispatcher::set_default(&dispatch); - tracing::callsite::rebuild_interest_cache(); - let out = f(); - drop(guard); - tracing::callsite::rebuild_interest_cache(); - out -} diff --git a/src/io/pipeline.rs b/src/io/pipeline.rs index f05655a..4310a0c 100644 --- a/src/io/pipeline.rs +++ b/src/io/pipeline.rs @@ -1223,20 +1223,6 @@ mod tests { /// A sink that records the exact order of items it receives, so we /// can prove the channel is FIFO (no reordering). `close` returns /// the recorded vector. - struct OrderSink { - seen: Vec, - } - impl Sink for OrderSink { - type Output = Vec; - fn apply(&mut self, item: u64) -> Result { - self.seen.push(item); - Ok(Flow::Continue) - } - fn close(self) -> Result, Error> { - Ok(self.seen) - } - } - /// Zero items sent: closing the pipeline immediately must still /// call `close()` exactly once and return its Output. The consumer /// loop's `while let Ok = rx.recv()` exits on the dropped tx with