fix: use unique artifact names to avoid collisions in matrix builds#5132
fix: use unique artifact names to avoid collisions in matrix builds#5132
Conversation
Append a short job backend ID suffix to the artifact name so each matrix job produces a distinct artifact. Also track which artifact name was accepted by CreateArtifact and use it in FinalizeArtifact, fixing a bug where a conflict-deduped name was not carried forward.
There was a problem hiding this comment.
Code Review
This PR fixes two real issues with the GitHub artifact uploader:
- Matrix collision fix — appending the first 8 chars of
workflowJobRunBackendIdto the artifact name ensures each matrix job produces a unique name upfront, rather than relying solely on the 409 conflict retry loop. - FinalizeArtifact name sync bug — correctly tracks
acceptedArtifactNamewithin the loop and uses it for finalization, instead of always passing the originalfileName(which could differ if a conflict-deduplicated name like-2was used).
Both fixes are clean and well-reasoned. The logic is correct and the invariant around acceptedArtifactName! is properly upheld: the signedUploadUrl is null guard on line 80 ensures we never reach the finalize step unless acceptedArtifactName was set in the same branch.
Minor nit
BuildUniqueArtifactName uses Substring, but per the project's LangVersion: preview convention, the range/slice syntax is preferred:
// Current
var shortId = jobRunBackendId.Length > 8
? jobRunBackendId.Substring(0, 8)
: jobRunBackendId;
// Preferred (modern C#)
var shortId = jobRunBackendId.Length > 8
? jobRunBackendId[..8]
: jobRunBackendId;Observation on the conflict retry loop
Now that baseFileName is already unique per job (via the backend ID suffix), the 3-attempt name-conflict loop will almost never trigger in practice. It's harmless to keep as a safety net, but if you wanted to simplify, you could remove or reduce it without meaningful risk. That's a judgement call, not a blocker.
Overall: solid fix, the two-issue solution is well-scoped and the code reads clearly.
Keep the original artifact name for the first attempt so non-matrix builds get a clean name. Only fall back to the job backend ID suffix when a conflict is detected.
There was a problem hiding this comment.
Updated Code Review
The previous review nit about using Substring vs slice syntax has been addressed — GetShortJobId now correctly uses jobRunBackendId[..8]. Good.
Remaining Concern: Unnecessary 409s in Matrix Builds
The naming strategy still uses the raw fileName for the first attempt (nameAttempt == 0):
var artifactName = nameAttempt switch
{
0 => fileName, // base name — will collide in matrix!
1 => $"{nameWithoutExt}-{GetShortJobId(workflowJobRunBackendId)}{ext}",
_ => $"{nameWithoutExt}-{GetShortJobId(workflowJobRunBackendId)}-{nameAttempt}{ext}",
};In a matrix build, every job will start by requesting the same base name, guaranteeing a 409 before falling back to the unique name. The fix is correct but opts into a predictable failure on every matrix run. The 409 round-trip is harmless but wasteful.
Suggested alternative: include the job ID from attempt 0, and keep the numeric fallback for the astronomically-unlikely case of an 8-char prefix collision:
var artifactName = nameAttempt switch
{
0 => $"{nameWithoutExt}-{GetShortJobId(workflowJobRunBackendId)}{ext}",
_ => $"{nameWithoutExt}-{GetShortJobId(workflowJobRunBackendId)}-{nameAttempt}{ext}",
};This avoids the guaranteed first-attempt conflict in matrix builds while keeping the retry fallback.
Minor: Truncating to First 8 Chars May Not Be Unique Enough
GitHub Actions backend IDs are opaque strings, but if they share a common prefix (e.g., a timestamp or run-scoped prefix), taking [..8] could produce collisions between matrix jobs. Taking the last 8 chars or using a short hash (e.g., GetHashCode() mod hex) would be more robust:
private static string GetShortJobId(string jobRunBackendId) =>
jobRunBackendId.Length > 8 ? jobRunBackendId[^8..] : jobRunBackendId;Or even just drop the length guard since GitHub backend IDs are always long:
private static string GetShortJobId(string jobRunBackendId) =>
jobRunBackendId[^Math.Min(8, jobRunBackendId.Length)..];What's Good
- The FinalizeArtifact name sync bug is correctly fixed:
acceptedArtifactNameis set inside the success branch and used for finalization, with the null-forgiving operator!being safe sincesignedUploadUrl is nullreturns early before finalization. - The
GetShortJobIdhelper is clean and now uses modern slice syntax. - The switch expression is more readable than the ternary it replaced.
Overall this is a solid fix for a real bug. The concerns above are improvements rather than blockers.
There was a problem hiding this comment.
Follow-up Review
Both items raised in the previous review have been addressed:
Previous feedback - addressed:
Substring→[..8]range syntax: fixed in commit61a49303✅- Conflict retry loop observation: addressed in
709df332— the first attempt now uses the originalfileName(no suffix), so non-matrix builds retain clean names, and the job ID suffix only kicks in on a real 409 conflict ✅
Current code assessment
The logic is now well-structured. One subtle point worth noting:
RetryAsync + 409 interaction: When the inner lambda returns null on a 409, RetryAsync treats this as a successful-but-null result and doesn't retry — the outer nameAttempt loop advances instead. This is the correct behaviour. If a transient 5xx causes RetryAsync to exhaust its retries and also return null, the outer loop would advance to attempt 1 (with the job ID suffix) even though no name collision occurred. This is a pre-existing quirk and not a regression from this PR — just worth being aware of.
acceptedArtifactName! null-forgiving operator: Safe. The control flow guarantees that acceptedArtifactName is set whenever signedUploadUrl is not null, and the if (signedUploadUrl is null) { return null; } guard ensures we only reach FinalizeArtifact when both are set.
Overall
The three commits clean up the original fix nicely. The naming strategy (clean name → job-ID suffix on conflict → job-ID+numeric suffix) is pragmatic and correct. No further changes needed.
There was a problem hiding this comment.
Updated Code Review
The previous review nit about using Substring vs slice syntax has been addressed — GetShortJobId now correctly uses jobRunBackendId[..8]. Good.
Remaining Concern: Unnecessary 409s in Matrix Builds
The naming strategy still uses the raw fileName for the first attempt (nameAttempt == 0):
var artifactName = nameAttempt switch
{
0 => fileName, // base name — will collide in matrix!
1 => $"{nameWithoutExt}-{GetShortJobId(workflowJobRunBackendId)}{ext}",
_ => $"{nameWithoutExt}-{GetShortJobId(workflowJobRunBackendId)}-{nameAttempt}{ext}",
};In a matrix build, every job will start by requesting the same base name, guaranteeing a 409 before falling back to the unique name. The fix is correct but opts into a predictable failure on every matrix run. The 409 round-trip is harmless but wasteful.
Suggested alternative: include the job ID from attempt 0, and keep the numeric fallback for the rare case of an 8-char prefix collision:
var artifactName = nameAttempt switch
{
0 => $"{nameWithoutExt}-{GetShortJobId(workflowJobRunBackendId)}{ext}",
_ => $"{nameWithoutExt}-{GetShortJobId(workflowJobRunBackendId)}-{nameAttempt}{ext}",
};This avoids the guaranteed first-attempt conflict in matrix builds while keeping the retry fallback.
Minor: Truncating to First 8 Chars May Not Be Unique Enough
If GitHub Actions backend IDs share a common prefix (e.g., a timestamp or run-scoped prefix), taking [..8] could produce collisions between matrix jobs. Taking the last 8 chars would be more robust since suffixes tend to diverge more:
private static string GetShortJobId(string jobRunBackendId) =>
jobRunBackendId.Length > 8 ? jobRunBackendId[^8..] : jobRunBackendId;Not a blocker if the backend IDs are known to be random/opaque, but worth noting.
What Is Good
- FinalizeArtifact name sync bug is correctly fixed:
acceptedArtifactNameis set inside the success branch and used for finalization. The null-forgiving operator!is safe here since thesignedUploadUrl is nullguard returns early before finalization is reached. GetShortJobIdnow uses modern C# slice syntax[..8]— the previous review's nit has been addressed.- The switch expression is cleaner and more readable than the original ternary.
Overall: solid fix for a real bug. The concerns above are improvements, not blockers.
Updated [TUnit](https://github.com/thomhurst/TUnit) from 1.19.22 to 1.19.57. <details> <summary>Release notes</summary> _Sourced from [TUnit's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.19.57 <!-- Release notes generated using configuration in .github/release.yml at v1.19.57 --> ## What's Changed ### Other Changes * fix: use unique artifact names to avoid collisions in matrix builds by @thomhurst in thomhurst/TUnit#5132 * fix: resolve IndexOutOfRangeException with MethodDataSource<T> on class (#5118) by @thomhurst in thomhurst/TUnit#5137 * fix: prevent StringEqualsAssertion from matching non-string types by @thomhurst in thomhurst/TUnit#5156 ### Dependencies * chore(deps): update tunit to 1.19.22 by @thomhurst in thomhurst/TUnit#5117 * chore(deps): update dependency fsharp.core to 10.0.104 by @thomhurst in thomhurst/TUnit#5119 * chore(deps): update dependency microsoft.entityframeworkcore to 10.0.4 by @thomhurst in thomhurst/TUnit#5120 * chore(deps): update dependency fsharp.core to v11 by @thomhurst in thomhurst/TUnit#5128 * chore(deps): update dependency microsoft.templateengine.authoring.cli to v10.0.200 by @thomhurst in thomhurst/TUnit#5122 * chore(deps): update dependency dotnet-sdk to v10.0.200 by @thomhurst in thomhurst/TUnit#5123 * chore(deps): update dependency microsoft.sourcelink.github to 10.0.200 by @thomhurst in thomhurst/TUnit#5121 * chore(deps): update dependency system.commandline to 2.0.4 by @thomhurst in thomhurst/TUnit#5125 * chore(deps): update microsoft.extensions to 10.0.4 by @thomhurst in thomhurst/TUnit#5127 * chore(deps): update microsoft.build to 18.4.0 by @thomhurst in thomhurst/TUnit#5129 * chore(deps): update microsoft.aspnetcore to 10.0.4 by @thomhurst in thomhurst/TUnit#5126 * chore(deps): update dependency microsoft.templateengine.authoring.templateverifier to 10.0.200 by @thomhurst in thomhurst/TUnit#5124 * chore(deps): update microsoft.extensions to 10.4.0 by @thomhurst in thomhurst/TUnit#5130 * chore(deps): update dependency opentelemetry.instrumentation.aspnetcore to 1.15.1 by @thomhurst in thomhurst/TUnit#5136 * chore(deps): update dependency vogen to 8.0.5 by @thomhurst in thomhurst/TUnit#5133 * chore(deps): update dependency npgsql to 10.0.2 by @thomhurst in thomhurst/TUnit#5139 * chore(deps): update dependency microsoft.sourcelink.github to 10.0.201 by @thomhurst in thomhurst/TUnit#5141 * chore(deps): update dependency microsoft.entityframeworkcore to 10.0.5 by @thomhurst in thomhurst/TUnit#5140 * chore(deps): update dependency microsoft.templateengine.authoring.cli to v10.0.201 by @thomhurst in thomhurst/TUnit#5142 * chore(deps): update dependency microsoft.templateengine.authoring.templateverifier to 10.0.201 by @thomhurst in thomhurst/TUnit#5143 * chore(deps): update dependency npgsql.entityframeworkcore.postgresql to 10.0.1 by @thomhurst in thomhurst/TUnit#5145 * chore(deps): update dependency dotnet-sdk to v10.0.201 by @thomhurst in thomhurst/TUnit#5144 * chore(deps): update dependency system.commandline to 2.0.5 by @thomhurst in thomhurst/TUnit#5146 * chore(deps): update microsoft.aspnetcore to 10.0.5 by @thomhurst in thomhurst/TUnit#5147 * chore(deps): update dependency testcontainers.kafka to 4.11.0 by @thomhurst in thomhurst/TUnit#5149 * chore(deps): update microsoft.extensions to 10.0.5 by @thomhurst in thomhurst/TUnit#5148 * chore(deps): update dependency testcontainers.postgresql to 4.11.0 by @thomhurst in thomhurst/TUnit#5150 * chore(deps): update dependency testcontainers.redis to 4.11.0 by @thomhurst in thomhurst/TUnit#5151 * chore(deps): update dependency stackexchange.redis to 2.12.1 by @thomhurst in thomhurst/TUnit#5153 **Full Changelog**: thomhurst/TUnit@v1.19.22...v1.19.57 Commits viewable in [compare view](thomhurst/TUnit@v1.19.22...v1.19.57). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
[//]: # (dependabot-start)⚠️ **Dependabot is rebasing this PR**⚠️ Rebasing might not happen immediately, so don't worry if this takes some time. Note: if you make any changes to this PR yourself, they will take precedence over the rebase. --- [//]: # (dependabot-end) Updated [TUnit.Core](https://github.com/thomhurst/TUnit) from 1.19.22 to 1.19.57. <details> <summary>Release notes</summary> _Sourced from [TUnit.Core's releases](https://github.com/thomhurst/TUnit/releases)._ ## 1.19.57 <!-- Release notes generated using configuration in .github/release.yml at v1.19.57 --> ## What's Changed ### Other Changes * fix: use unique artifact names to avoid collisions in matrix builds by @thomhurst in thomhurst/TUnit#5132 * fix: resolve IndexOutOfRangeException with MethodDataSource<T> on class (#5118) by @thomhurst in thomhurst/TUnit#5137 * fix: prevent StringEqualsAssertion from matching non-string types by @thomhurst in thomhurst/TUnit#5156 ### Dependencies * chore(deps): update tunit to 1.19.22 by @thomhurst in thomhurst/TUnit#5117 * chore(deps): update dependency fsharp.core to 10.0.104 by @thomhurst in thomhurst/TUnit#5119 * chore(deps): update dependency microsoft.entityframeworkcore to 10.0.4 by @thomhurst in thomhurst/TUnit#5120 * chore(deps): update dependency fsharp.core to v11 by @thomhurst in thomhurst/TUnit#5128 * chore(deps): update dependency microsoft.templateengine.authoring.cli to v10.0.200 by @thomhurst in thomhurst/TUnit#5122 * chore(deps): update dependency dotnet-sdk to v10.0.200 by @thomhurst in thomhurst/TUnit#5123 * chore(deps): update dependency microsoft.sourcelink.github to 10.0.200 by @thomhurst in thomhurst/TUnit#5121 * chore(deps): update dependency system.commandline to 2.0.4 by @thomhurst in thomhurst/TUnit#5125 * chore(deps): update microsoft.extensions to 10.0.4 by @thomhurst in thomhurst/TUnit#5127 * chore(deps): update microsoft.build to 18.4.0 by @thomhurst in thomhurst/TUnit#5129 * chore(deps): update microsoft.aspnetcore to 10.0.4 by @thomhurst in thomhurst/TUnit#5126 * chore(deps): update dependency microsoft.templateengine.authoring.templateverifier to 10.0.200 by @thomhurst in thomhurst/TUnit#5124 * chore(deps): update microsoft.extensions to 10.4.0 by @thomhurst in thomhurst/TUnit#5130 * chore(deps): update dependency opentelemetry.instrumentation.aspnetcore to 1.15.1 by @thomhurst in thomhurst/TUnit#5136 * chore(deps): update dependency vogen to 8.0.5 by @thomhurst in thomhurst/TUnit#5133 * chore(deps): update dependency npgsql to 10.0.2 by @thomhurst in thomhurst/TUnit#5139 * chore(deps): update dependency microsoft.sourcelink.github to 10.0.201 by @thomhurst in thomhurst/TUnit#5141 * chore(deps): update dependency microsoft.entityframeworkcore to 10.0.5 by @thomhurst in thomhurst/TUnit#5140 * chore(deps): update dependency microsoft.templateengine.authoring.cli to v10.0.201 by @thomhurst in thomhurst/TUnit#5142 * chore(deps): update dependency microsoft.templateengine.authoring.templateverifier to 10.0.201 by @thomhurst in thomhurst/TUnit#5143 * chore(deps): update dependency npgsql.entityframeworkcore.postgresql to 10.0.1 by @thomhurst in thomhurst/TUnit#5145 * chore(deps): update dependency dotnet-sdk to v10.0.201 by @thomhurst in thomhurst/TUnit#5144 * chore(deps): update dependency system.commandline to 2.0.5 by @thomhurst in thomhurst/TUnit#5146 * chore(deps): update microsoft.aspnetcore to 10.0.5 by @thomhurst in thomhurst/TUnit#5147 * chore(deps): update dependency testcontainers.kafka to 4.11.0 by @thomhurst in thomhurst/TUnit#5149 * chore(deps): update microsoft.extensions to 10.0.5 by @thomhurst in thomhurst/TUnit#5148 * chore(deps): update dependency testcontainers.postgresql to 4.11.0 by @thomhurst in thomhurst/TUnit#5150 * chore(deps): update dependency testcontainers.redis to 4.11.0 by @thomhurst in thomhurst/TUnit#5151 * chore(deps): update dependency stackexchange.redis to 2.12.1 by @thomhurst in thomhurst/TUnit#5153 **Full Changelog**: thomhurst/TUnit@v1.19.22...v1.19.57 Commits viewable in [compare view](thomhurst/TUnit@v1.19.22...v1.19.57). </details> [](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot show <dependency name> ignore conditions` will show all of the ignore conditions of the specified dependency - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details> Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Summary
workflowJobRunBackendId) to the HTML report artifact name, ensuring each matrix job uploads a distinct artifact-2) was not carried forward to finalizationTest plan
MyProject-linux-net10.0-report-a1b2c3d4.html)