Fix disc rescan not updating after swapping discs

[Masterisk-F]

Summary

Both ScanDiscCdparanoia#scan() and ScanDiscCdinfo#scan() had an early-return guard (return true if @status == 'ok') that prevented re-scanning after the first successful scan. Once @status was set to "'ok'", it was never reset, so swapping discs and selecting "2) Scan drive for audio disc" from the CLI menu would still show the old disc's track information.

Root Cause

# scanDiscCdparanoia.rb, line 49:
return true if @status == 'ok'

# scanDiscCdinfo.rb, line 48:
return true if @status == 'ok'

@status is initialized to nil in the constructor and set to "'ok'" after a successful scan, but never reset afterward, causing every subsequent scan call to short-circuit immediately.

Call Flow

CLI "2) Scan drive for audio disc"
→ CliDisc#show() → Disc#scan()
    ├─ [1] ScanDiscCdparanoia#scan()  — called every time (main scanner)
    └─ [2] setMetadata()  →  metadata lookup
           ├─ CalcFreedbID → advancedTocScanner → ScanDiscCdinfo#scan()
           └─ CalcMusicbrainzID → advancedTocScanner → ScanDiscCdinfo#scan()

ScanDiscCdparanoia already had a restoreStatus method (@status = nil; @error = nil) used inside waitForDisc, but it was never called at the top of scan().

ScanDiscCdinfo had no such reset mechanism at all.

Changes

lib/rubyripper/disc/scanDiscCdparanoia.rb

• Call restoreStatus() at the start of scan(). This resets @status and @error before every scan, ensuring cd-paranoia -vQ actually runs each time.

lib/rubyripper/disc/scanDiscCdinfo.rb

• Add @status = nil at the start of scan(). Same effect: the cd-info command is executed every time instead of short-circuiting.

spec/disc/scanDiscCdparanoia_spec.rb

• Fix existing mock argument matching (the Execute#launch signature was extended with a stopPatterns parameter that the test mocks did not account for).
• Add a test verifying that a second scan() call re-executes the launch command.

spec/disc/scanDiscCdinfo_spec.rb

• Add a test verifying that a second scan() call reflects a different disc result.

Testing

• All 113 existing disc-suite examples + 2 new examples pass.
• The 18 pre-existing failures in freedb-related specs (brand-name string mismatch and Network mock return-value issues) are unrelated to this change.

[Masterisk-F]

Code Review: PR #13 (fix-disc-rescan-cache)

15 findings across 10 review angles — ranked by severity.

---

🔴 CRITICAL: Data Corruption on Rescan (3)

#	File	Line	Issue
1	scanDiscCdinfo.rb	49	No state reset — 12+ instance variables (@startSector, @dataTracks, @firstAudioTrack, @version, @vendor, @model, @revision, @discMode, @deviceName, @playtime, @totalSectors, @finalTrack) retain old disc data
2	scanDiscCdinfo.rb	122	@audiotracks calculated from stale @startSector.length
3	scanDiscCdcontrol.rb	46	Same bug unfixed — early return return true if @status == 'ok' still present

Failure: User scans disc A (10 tracks), ejects, inserts disc B (5 tracks), rescans → returns disc A's stale data.

---

🟠 HIGH: Breaking API Changes (2)

#	File	Line	Issue
4	scanDiscCdinfo.rb	49	scan() returns nil instead of true on success
5	scanDiscCdparanoia.rb	50	restoreStatus() returns nil, breaking return value contract

No internal callers use the return value, but external library consumers will break.

---

🟡 MEDIUM: Architecture & Maintenance (5)

#	File	Line	Issue
6	All scanners	—	4 different reset patterns — no common contract (cdinfo: @status=nil, cdparanoia: restoreStatus(), cdcontrol: early return, cdrdao: @error=nil)
7	scanDiscCdparanoia.rb	126	10-second blocking retry in waitForDisc() — blocks CLI on rescan
8	scanDiscCdinfo.rb	43	@status not explicitly initialized in initialize() (cdparanoia does)
9	scanDiscCdinfo.rb	49	Missing "why" comment (CLAUDE.md violation)
10	scanDiscCdparanoia.rb	211	Root cause: cdparanoia calls setupDisc() to clear state; cdinfo does not

---

🟢 LOW: Test Coverage & Consistency (5)

#	File	Issue
11	scanDiscCdcontrol_spec.rb	No rescan test for cdcontrol (which still has the bug)
12	scanDiscCdcontrol.rb	Fix incomplete — cdcontrol still broken
13	scanDiscCdrdao.rb	4th different reset pattern (@error=nil only)
14	scanDiscCdinfo_spec.rb	Mock argument count may need verification
15	All scanners	No shared abstraction for rescan behavior

---

Key Insight

The PR fixes rescan only for cdinfo and cdparanoia, but leaves cdcontrol with the exact same bug. This is an incomplete refactoring across similar implementations.

---

Recommendations

1. Apply the same fix to scanDiscCdcontrol.rb (remove early return, add state reset)
2. Extract a common RescannableScanner module/base class with reset_for_rescan() to eliminate the 4-pattern inconsistency
3. Add explicit return true at end of scan() to preserve API contract
4. Add rescan tests for cdcontrol and cdrdao

[Masterisk-F]

I have investigated and addressed the HIGH/CRITICAL issues regarding the rescan cache and return values.

Instead of manually resetting the state variables within each scanner class, I implemented a much safer approach: recreating the scanner instances upon each rescan from the caller side (Disc class). This guarantees that a fresh, stateless instance is always used for a new disc, entirely preventing any data corruption caused by leftover cache variables.

• Removed the faulty early return (return true if @status == 'ok') and incomplete status reset from scanDiscCdinfo.rb, scanDiscCdcontrol.rb, and scanDiscCdparanoia.rb.
• Modified Disc#scan to re-instantiate scanners.
• Updated and removed redundant RSpec tests to match the new lifecycle.

Regarding the return value contract (scan() returning nil instead of true), I noticed that the original implementation never returned true on success (it returned the result of addExtraInfo()). Therefore, I decided not to force a return true at the end of the method to avoid unnecessary API changes.

Please review the latest commits.

[Masterisk-F]

Code Review Results (max effort)

Scope: +33/-4 lines, 7 files
Method: 10 independent finder angles → 1-vote verify → sweep → 8 findings (all CONFIRMED)

---

🔴 1. CalcFreedbID / CalcMusicbrainzID caches are not cleared after rescan

lib/rubyripper/disc/disc.rb:42

Disc#scan resets @cdparanoia, @scanner, @cdrdao, and @cuesheet, but does not reset @calcFreedbID or @calcMusicbrainzID. These objects cache their results internally, so after a rescan they still return the old disc's values.

# calcFreedbID.rb:39 — only computes if @freedbString is nil
def freedbString
  getFreedbString() if @freedbString.nil?  # once set, never recomputed
  @freedbString
end

Failure scenario: Scan disc A → freedbString is called, caches disc A's ID → swap to disc B, rescan → calling freedbString again returns disc A's ID. Same issue for musicbrainzLookupPath and musicbrainzDiscid.

---

🔴 2. ScanDiscCdinfo#scan — isValidQuery returns false after a successful scan

lib/rubyripper/disc/scanDiscCdinfo.rb:86

isValidQuery uses return @status.nil? to determine success, but scan no longer resets @status. On a reused instance, a second call to scan causes isValidQuery to return false immediately — parseQuery and addExtraInfo are silently skipped, leaving all data from the previous scan.

# scanDiscCdinfo.rb:86
return @status.nil?  # @status='ok' from prior scan → false → parseQuery skipped

Commit 1 added @status = nil to reset state, but commit 2 removed it. The normal path is covered because Disc#scan creates a new instance, but the case where advancedTocScanner's memoized instance receives scan from multiple code paths within the same Disc lifecycle is not covered.

---

🔴 3. ScanDiscCdcontrol#scan — same stale @status bug

lib/rubyripper/disc/scanDiscCdcontrol.rb:83

Identical pattern.

---

🟠 4. ScanDiscCdinfo#scan — @dataTracks accumulates via <<

lib/rubyripper/disc/scanDiscCdinfo.rb:104

@dataTracks is initialized once in initialize and appended to with << in scan. It is never reset, so on a reused instance tracks from the previous disc remain.

Failure scenario: 12-track disc with 3 data tracks → swap to 8-track disc with 1 data track, rescan on the same instance → @dataTracks contains [old1, old2, old3, new1]. The tracks method (@audiotracks + @dataTracks.length) returns the wrong count.

---

🟠 5. ScanDiscCdcontrol#scan — same @dataTracks accumulation

lib/rubyripper/disc/scanDiscCdcontrol.rb:98

---

🟠 6. ScanDiscCdinfo — @firstAudioTrack frozen after first scan

lib/rubyripper/disc/scanDiscCdinfo.rb:105

@firstAudioTrack = tracknumber unless @firstAudioTrack || trackinfo[2] == "data"

The unless @firstAudioTrack guard prevents recalculation on any subsequent scan of the same instance.

---

🟠 7. ScanDiscCdcontrol — same @firstAudioTrack freeze

lib/rubyripper/disc/scanDiscCdcontrol.rb:99

---

🟡 8. ScanDiscCdparanoia#scan — restoreStatus() removal leaves stale @error

lib/rubyripper/disc/scanDiscCdparanoia.rb:49

Commit 2 removed the restoreStatus() call. The normal path is unaffected because Disc#scan creates a fresh ScanDiscCdparanoia.new() each time. However, if scan is called directly on a reused instance and @perm.problems? returns true, the method exits without reaching waitForDisc (which internally calls restoreStatus), leaving the old @error intact.

---

Recommended fix

# Add to Disc#scan:
@calcFreedbID = CalcFreedbID.new(self)
@calcMusicbrainzID = CalcMusicbrainzID.new(self)

# Add to the top of ScanDiscCdinfo#scan and ScanDiscCdcontrol#scan:
@dataTracks = []
@firstAudioTrack = nil
@status = nil

[Masterisk-F]

Reworked approach: recreate Disc in CliDisc#refreshDisc instead of per-variable reset

Problem with the previous approach

PR #13 patched 7 files (scanner early-return removal, @has_mock flag, scanner re-creation inside Disc#scan). However, review issues 1-3 revealed that this left multiple stale-state surfaces unfixed:

1. CalcFreedbID / CalcMusicbrainzID cache their results via nil? guards — never recomputed on rescan
2. ScanDiscCdinfo#isValidQuery depends on @status.nil? — returns false on a reused instance after the first successful scan
3. @dataTracks accumulates via <<, @firstAudioTrack freezes via unless @firstAudioTrack guard

Fixing each of these individually would scatter more per-variable resets across the codebase.

Simplified fix: recreate Disc on refresh

GTK (GtkDisc#refreshDisc) and TUI (TUIDisc#refresh_disc) already do @disc = Disc.new() on every rescan. Only CLI reused the same Disc instance across scans.

One-line change in CliDisc#refreshDisc:

# before
def refreshDisc ; @cd.scan() ; end

# after
def refreshDisc
  @cd = Disc.new()
  @cd.scan()
end

This guarantees all sub-objects (ScanDiscCdparanoia, CalcFreedbID, CalcMusicbrainzID, advancedTocScanner) start fresh on every rescan — no per-variable reset needed.

What this commit reverts vs preserves

Reverted (back to master state):

• disc.rb: @has_mock flag, scanner re-creation, @scanner/@cdrdao/@cuesheet=nil
• 3 scanner files: early-return guards (return true if @status == "ok") restored
• spec/disc/disc_spec.rb: "When scanning again" test block removed
• spec/disc/scanDiscCdinfo_spec.rb, scanDiscCdparanoia_spec.rb: trailing blank lines

Preserved:

• spec/disc/scanDiscCdparanoia_spec.rb: any_args fix (pre-existing bug from 21e86fc where launch got 4 params but mock only stubbed 1)

New:

• spec/cli/cliDisc_spec.rb: verifies Disc.new() is called on refresh

[Masterisk-F]

Code Review (max effort)

Summary: CliDisc#refreshDisc now creates a fresh Disc via Disc.new() instead of reusing the existing instance. Scanner test mock argument matching also fixed.

Tests: All 53 relevant tests PASS ✓

---

Findings

4. [MEDIUM] @error not cleared after successful rescan

In CliDisc#showDisc, @error is set in the else branch when discReady? is false. On a subsequent successful rescan, the if discReady? branch runs and @error is never cleared. It remains exposed via attr_reader :error.

No current production caller reads CliDisc#error, so this is a latent bug. However, for UI/TUI consistency: TUIDisc#refresh_disc explicitly resets @error = nil (L31), while CliDisc does not.

5. [LOW] Object allocation churn on every rescan

Each Disc.new() allocates new ScanDiscCdparanoia, CalcFreedbID, and CalcMusicbrainzID instances. Old objects are GC'd, but external tool calls (discid / cd-discid) and parsing run fresh every time. Frequent rescans incur unnecessary process spawning and allocation overhead.

6. [LOW] cliDisc_spec mock depends on exact Disc.new call count

expect(Disc).to receive(:new).and_return(first_disc, second_disc)

Constructor consumes first_disc, refreshDisc consumes second_disc. If another Disc.new is added to CliDisc, the mock returns the wrong double and the test still passes (both doubles are as_null_object).

7. [LOW] cdrdao background thread may be abandoned on rescan

Disc.startExtendedTocScan launches a cdrdao background thread. When refreshDisc discards the old Disc, the thread is not joined (it cleans up its own temp files). Rapid disc swaps could leave multiple abandoned threads running in parallel.

---

[INFO] Design trade-off — scanner early-return guard

The return true if @status == 'ok' guard remains in all three scanner classes (ScanDiscCdparanoia, ScanDiscCdinfo, ScanDiscCdcontrol). Only ScanDiscCdparanoia has a restoreStatus method; the other two do not.

This is safe as long as all rescans go through Disc.new(), which they currently do (both TUIDisc and CliDisc). No existing code path is affected. A future Disc#reset method could centralize this pattern and avoid full object reallocation, but it's not required for correctness.

[Masterisk-F]

以下は今回のPR変更範囲外

• 4 : PR以前から存在 — showDisc は変更されていない
• 7 : PR以前から存在

以下は意図した動作で問題なし

• 5 : 意図したインスタンス再生成
• 6 : 意図した動作。将来変更したらそのときにテストを修正すべき

結果としてレビューはOK