Skip to content

importadded: preserve_write_mtimes targets the written file, not the source - #6992

Open
Huang-404-Q wants to merge 7 commits into
beetbox:masterfrom
Huang-404-Q:fix/importadded-readonly-source-6954
Open

Huang-404-Q wants to merge 7 commits into
beetbox:masterfrom
Huang-404-Q:fix/importadded-readonly-source-6954

Conversation

@Huang-404-Q

Copy link
Copy Markdown

importadded: preserve_write_mtimes targets the written file, not the source

What this fixes

With importadded and preserve_write_mtimes enabled, beet convert
crashes when the source file is read-only (in my case, files with the
immutable bit set):

PermissionError: [Errno 1] Operation not permitted: '.../01 somename.flac'

The conversion itself never needs to write the source file — but the
plugin does.

Why it happens

update_after_write_time(item, path) is invoked from the after_write
event that Item.write() sends at the end of every write
(beets/library/models.py), where path is the file that was just
written. Under beet convert that is the converted file
(item.try_write(path=converted)), not the source. The plugin currently
ignores path and unconditionally calls write_item_mtime(item, ...),
which does os.utime on item.path — the source file — and stomps
item.mtime even though the source was not the file being written.

The fix

Point the mtime write at path (the file actually written), and only
sync item.mtime when path is the item's own file:

self.write_file_mtime(util.syspath(path), item.added)
if path == item.path:
    item.mtime = int(item.added)

This mirrors what Item.write() itself does
(if path == self.path: self.mtime = self.current_mtime()), so the DB
field stays consistent with the same rule the library already applies.
Ordinary writes (beet write, tagging after import) pass
path == item.path, so their behavior is unchanged — all existing
importadded tests pass as-is.

Tests

New test/plugins/test_importadded_convert.py (2 tests, convert +
importadded with both preserve options on):

  • test_convert_does_not_touch_source_mtime: the source file's mtime is
    left alone and the converted file carries the added mtime.
  • test_convert_with_readonly_source: a source whose os.utime raises
    PermissionError no longer crashes the convert.

Red without the fix (2 failed: mtime assert + the PermissionError from
the issue), green with it. pytest -k "importadded or convert": 48
passed, 6 skipped (optional dependencies). ruff check and
ruff format --check clean.

Fixes #6954

…source

The after_write listener passed the written file's path to
write_item_mtime, which ignored it and always rewrote the mtime of the
item's own file. When beet convert wrote tags to a converted file, the
plugin instead reset the mtime of the original source file, and crashed
with a PermissionError when that file was read-only (e.g. immutable).

Write item.added to the file that was actually written, and update the
item's mtime field only when that file is the item's own.

Fixes beetbox#6954
@Huang-404-Q
Huang-404-Q requested a review from a team as a code owner September 5, 2026 06:16
@github-actions github-actions Bot added the importadded importadded plugin label Sep 5, 2026
@codecov

codecov Bot commented Sep 5, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.27%. Comparing base (998f5c5) to head (bfbf102).
⚠️ Report is 8 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
beetsplug/importadded.py 33.33% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6992   +/-   ##
=======================================
  Coverage   77.27%   77.27%           
=======================================
  Files         163      163           
  Lines       21845    21847    +2     
  Branches     3370     3371    +1     
=======================================
+ Hits        16881    16883    +2     
  Misses       4147     4147           
  Partials      817      817           
Files with missing lines Coverage Δ
beetsplug/importadded.py 94.25% <33.33%> (+0.13%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@snejus snejus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just move the changelog under the Unreleased section please.

snejus and others added 3 commits September 8, 2026 19:24
Merging master (b07203f) dropped master's four Unreleased "Bug fixes"
entries, commented the heading out, and left this branch's beetbox#6954 entry
inside the released 2.14.0 section — which is what the docs check
rejects:

  docs/changelog.rst:69: Changelog entry must be added under Unreleased
  section above. (changelog-unreleased)

Take master's changelog and append the entry to the bottom of the
Unreleased Bug fixes list, as the original commit had it. The four
dropped entries come back with it.

Verified against the check's own command: it reports the violation on the
previous head and passes here.
@Huang-404-Q

Copy link
Copy Markdown
Author

The Check docs failure was real and is now fixed.

docs/changelog.rst:69: Changelog entry must be added under Unreleased section
above. (changelog-unreleased)

Root cause was the merge, not the original commit. 76d09f1 had this entry correctly at the bottom of the Unreleased → Bug fixes list. The later Merge branch 'master' (b07203f) resolved the changelog conflict the wrong way: it commented the Unreleased Bug fixes heading out, moved this branch's entry down into the released 2.14.0 section, and dropped master's four Unreleased entries (#7001, #7002, discogs, #4889) along with it. Those would have gone missing had this merged as it stood.

Fixed by taking master's changelog and appending the :bug:6954`` entry to the bottom of the Unreleased Bug fixes list, exactly as the original commit had it — so the four entries come back with it. I also merged current master in, which cleared the conflict this had picked up on `docs/changelog.rst` and the BEHIND state.

Verified with the check's own command from lint.yaml:

$ git diff --word-diff=plain -U1000 origin/master -- docs/changelog.rst | awk '...'
# previous head: docs/changelog.rst:69: Changelog entry must be added under Unreleased section above.
# this head:     (no output)

Net diff against master is now just this PR:

beetsplug/importadded.py                 |  8 +++-
docs/changelog.rst                       |  4 ++
test/plugins/test_importadded_convert.py | 73 ++++++++++++++++++++++++++++++++
test/plugins/test_importadded.py test/plugins/test_importadded_convert.py
9 passed

@semohr

semohr commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

@snejus

snejus commented Sep 17, 2026

Copy link
Copy Markdown
Member

I'll allow this one as the policy had not been present when this PR was opened.

@snejus
snejus added this pull request to the merge queue Sep 17, 2026
@snejus
snejus removed this pull request from the merge queue due to a manual request Sep 17, 2026
@snejus
snejus enabled auto-merge September 17, 2026 16:11
@snejus
snejus added this pull request to the merge queue Sep 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

importadded importadded plugin

Projects

None yet

Development

Successfully merging this pull request may close these issues.

importadded: PermissionError thrown when converting from a readonly source

3 participants