Fix #12774: skip consumer POM re-attachment on repeated task segments - #12916
Conversation
…gments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Correct and minimal fix that makes injectTransformedArtifacts() idempotent by guarding against duplicate consumer POM attachment.
One low-severity optimization opportunity noted inline.
Positive observations:
TransformedArtifactalready handles POM-change detection lazily via SHA1 checksumming inmayUpdate(), so skipping re-attachment is safe — the already-attached artifact will still pick up source POM changes on the nextgetFile()call.- The matching criteria (
getClassifier()+getType()) on Maven'sArtifactinterface is correct for this context.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
| deferDeleteFile(consumer); | ||
|
|
||
| project.addAttachedArtifact(createConsumerPomArtifact(project, consumer, session)); | ||
| boolean alreadyAttached = project.getAttachedArtifacts().stream() |
There was a problem hiding this comment.
Low: The idempotency check here is placed after the temp file creation and deferDeleteFile() registration (lines 87-90). On repeated invocations where the consumer POM is already attached, a new temp file is created on disk, registered for deferred deletion, but never used.
Consider hoisting the alreadyAttached check to the top of the if (Features.consumerPom(...)) block, before creating the temp file. This would avoid unnecessary I/O on repeated invocations. The files are cleaned up by @PreDestroy doDeleteFiles() so this is not a leak, just wasteful work.
There was a problem hiding this comment.
Do you want to apply this improvement before merging as we had some performance issue with local repository in rc6 or do you want to apply it in a separate PR/later version?
Avoid unnecessary I/O on repeated invocations by checking whether the consumer POM is already attached before creating a temp file and registering it for deferred deletion. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Delta review — checking new commit 9515783 ("Hoist alreadyAttached check before temp file creation").
✅ Previous finding resolved: The alreadyAttached check is now the first statement inside the if (Features.consumerPom(...)) block, and all temp file creation / deferDeleteFile() / addAttachedArtifact calls are wrapped inside if (!alreadyAttached). No unnecessary I/O on repeated invocations.
A new regression test injectTransformedArtifactsTwiceShouldNotDuplicate verifies the idempotency behavior — clean, well-structured test.
No new issues introduced.
This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.
Claude Code on behalf of Guillaume Nodet
…#12916) (#12956) * Fix #12774: skip consumer POM re-attachment on repeated task segments * Hoist alreadyAttached check before temp file creation Avoid unnecessary I/O on repeated invocations by checking whether the consumer POM is already attached before creating a temp file and registering it for deferred deletion. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
ConsumerPomArtifactTransformer.injectTransformedArtifacts()idempotent by checking if a consumer POM artifact (classifier=consumer, type=pom) is already attached before callingaddAttachedArtifact()artifact '...:pom:consumer:...' already attached, replacing previous instancewarning when running multiple task segments (e.g.mvn package compiler:help)Fixes #12774
Test plan
injectTransformedArtifactsTwiceShouldNotDuplicateverifies idempotencymvn package compiler:helpno longer produces the warning🤖 Generated with Claude Code