importsource: Catch importer crash when skipping

[JOJ0]

Catches a crash when "skip" is selected in the importer and task.imported_items() runs into a condition branch that supposedly should never be reached:

  File "beets/beets/importer/tasks.py", line 254, in imported_items
    assert False
           ^^^^^
AssertionError

To Do

• [x] ~Documentation.~
• [ ] Changelog.
• [ ] Tests.

[github-actions[bot]]

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 68.20%. Comparing base (c1904b1) to head (9ffae4b). :warning: Report is 4 commits behind head on master. :white_check_mark: All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6203      +/-   ##
==========================================
- Coverage   68.36%   68.20%   -0.17%     
==========================================
  Files         138      138              
  Lines       18773    18775       +2     
  Branches     3172     3173       +1     
==========================================
- Hits        12834    12805      -29     
- Misses       5263     5296      +33     
+ Partials      676      674       -2     

Files with missing lines	Coverage Δ
beetsplug/importsource.py	69.11% <100.00%> (+8.51%)	:arrow_up:

... and 1 file with indirect coverage changes

:rocket: New features to boost your workflow:

• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JOJ0]

I'm sure this is not the right way to fix this. Some eyes here @semohr or @snejus.

Here we assert False, I suppose because this should never happen right?

https://github.com/beetbox/beets/blob/2bd77b9895fa9779818ff1c0430b9f9738d1616b/beets/importer/tasks.py#L241-L254

[Serene-Arc]

Personally, I don't think we should have any assert False statements in the codebase at all, so we should phase them out when necessary. There are so many better ways to handle this. Clearly it is happening, so that assumption is false.

I think we should either raise a dedicated exception with debugging details so we can identify which assumption is false, or else have that function return a None or empty list and the calling functions can deal with it. But having the program crash is the worst thing for a user facing tool.

[semohr]

@JOJ0 Do you have the full trace-back here? How do we even run into this? This is a quite old part of the codebase, how was this never an issue beforehand?

Personally, I don't think we should have any assert False statements in the codebase at all, so we should phase them out when necessary. There are so many better ways to handle this. Clearly it is happening, so that assumption is false.

I think we should either raise a dedicated exception with debugging details so we can identify which assumption is false, or else have that function return a None or empty list and the calling functions can deal with it. But having the program crash is the worst thing for a user facing tool.

I agree fully here but I would like to figure out how and why we are now running into this issue now. We did not run into this for the previous 12 years afaik. (Empty list or none makes sense in my opinion)

[JOJ0]

@JOJ0 Do you have the full trace-back here? How do we even run into this? This is a quite old part of the codebase, how was this never an issue beforehand?

Personally, I don't think we should have any assert False statements in the codebase at all, so we should phase them out when necessary. There are so many better ways to handle this. Clearly it is happening, so that assumption is false.

I think we should either raise a dedicated exception with debugging details so we can identify which assumption is false, or else have that function return a None or empty list and the calling functions can deal with it. But having the program crash is the worst thing for a user facing tool.

I agree fully here but I would like to figure out how and why we are now running into this issue now. We did not run into this for the previous 12 years afaik. (Empty list or none makes sense in my opinion)

Sure. Here it is:

$ beet import --copy -t ~/Music/import-devbeets/

/Users/jojo/Music/import-devbeets (1 items)

  Match (42.8%):
  Dr. Ring-Ding & The Senior Allstars - Dandimite!
  ≠ missing tracks, id, data source, tracks
  Discogs, Vinyl, 1995, Germany, Grover Records, GRO-LP 004, None
  https://www.discogs.com/release/675198-Dr-Ring-Ding-The-Senior-Allstars-Dandimite
  * Artist: Dr. Ring-Ding & The Senior Allstars
  * Album: Dandimite!
     * (#2) Dandimite Ska (3:58)
Missing tracks (13/14 - 92.9%):
 ! Phone Talk (#1) (0:47)
 ! Big Man (#3) (3:31)
 ! Medley: Save A Bread / Save A Toast (#4) (4:42)
 ! (Want Me) Money Back (#5) (3:25)
 ! Got My Boogaloo (#6) (3:28)
 ! Bellevue Asylum (#7) (5:33)
 ! What A Day (#8) (2:03)
 ! Latin Goes Ska (#9) (3:41)
 ! Rudeboy Style (#10) (5:39)
 ! Stay Out Late (#11) (2:38)
 ! Knocking On My Door (#12) (4:12)
 ! One Scotch, One Bourbon, One Beer (#13) (4:03)
 ! Gloria (#14) (3:42)
➜ [A]pply, More candidates, Skip, Use as-is, as Tracks, Group albums,
Enter search, enter Id, aBort, eDit, edit Candidates, plaY? s
Traceback (most recent call last):
  File "/Users/jojo/.pyenv/versions/jtb311/bin/beet", line 6, in <module>
    sys.exit(main())
             ^^^^^^
  File "/Users/jojo/git/beets/beets/ui/__init__.py", line 1631, in main
    _raw_main(args)
  File "/Users/jojo/git/beets/beets/ui/__init__.py", line 1610, in _raw_main
    subcommand.func(lib, suboptions, subargs)
  File "/Users/jojo/git/beets/beets/ui/commands/import_/__init__.py", line 131, in import_func
    import_files(lib, byte_paths, query)
  File "/Users/jojo/git/beets/beets/ui/commands/import_/__init__.py", line 75, in import_files
    session.run()
  File "/Users/jojo/git/beets/beets/importer/session.py", line 236, in run
    pl.run_parallel(QUEUE_SIZE)
  File "/Users/jojo/git/beets/beets/util/pipeline.py", line 471, in run_parallel
    raise exc_info[1].with_traceback(exc_info[2])
  File "/Users/jojo/git/beets/beets/util/pipeline.py", line 336, in run
    out = self.coro.send(msg)
          ^^^^^^^^^^^^^^^^^^^
  File "/Users/jojo/git/beets/beets/util/pipeline.py", line 195, in coro
    task = func(*(args + (task,)))
           ^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jojo/git/beets/beets/importer/stages.py", line 171, in user_query
    plugins.send("import_task_choice", session=session, task=task)
  File "/Users/jojo/git/beets/beets/plugins.py", line 646, in send
    return [
           ^
  File "/Users/jojo/git/beets/beets/plugins.py", line 649, in <listcomp>
    if (r := handler(**arguments)) is not None
             ^^^^^^^^^^^^^^^^^^^^
  File "/Users/jojo/git/beets/beets/plugins.py", line 333, in wrapper
    return func(*args, **kwargs)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jojo/git/beets/beetsplug/importsource.py", line 42, in prevent_suggest_removal
    for item in task.imported_items():
                ^^^^^^^^^^^^^^^^^^^^^
  File "/Users/jojo/git/beets/beets/importer/tasks.py", line 254, in imported_items
    assert False
           ^^^^^
AssertionError
$

[JOJ0]

There might be some design-flaws that slipped through my initial review of this plugin. One thing is that the listener here is of no use if the configuration of the plugin does not want to use that feature anyway:

https://github.com/beetbox/beets/pull/6203/files#diff-2f4f7f8754e928450b8b006758b96ce931b391f184d586592fe7bfc499c27eedR37-R41

But still if we'd prevent registering the listener when not configured, it does not cure the problem that task.imported_items() can't be called safely. Even though it might be "the wrong moment" trying to ask for imported items and trying to loop through them, I agree that it would be an easy and elegant fix to simply return an empty list. That would fix the error at hand and also prevent any sanity checks on the callers end.

Should I try to include an "empty list or none fix" as you both suggested within this PR?

[semohr]

Have you seen #6211? Would that approach also fully fix this issue?

[JOJ0]

Have you seen #6211? Would that approach also fully fix this issue?

Yes definitely. Closed that one already. It's definitely correct to simply "not do prevent suggest stuff" if skip is chosen anyway.

But I thought we wanted to maybe change the assert false to allow empty list/none in the importer code? Or better not and leave it as is?

[JOJ0]

I applied the mentioned fix and rebased. Remains to be decided if we want to change things in the importer's imported_items() method or not @semohr @Serene-Arc

[JOJ0]

I applied the mentioned fix and rebased. Remains to be decided if we want to change things in the importer's imported_items() method or not @semohr @Serene-Arc

For now I would like to move on with getting this crash fixed and want to make the call here: Let's not touch the importer logic for now and move that decision to another time. Fixing this crash is my priority.

See my updated PR description bulletpoints. I fixed the original changelog and added two IMO essential test cases.

I'd like to request a final look here @semohr @Serene-Arc

Thanks!

[doronbehar]

Thanks!