Skip to content

fix(core): drop PendingSubmission's guards in reverse acquisition order - #9960

Merged
ErichDonGubler merged 3 commits into
gfx-rs:trunkfrom
AdrianEddy:fix-pending-submission-lock-drop-order
Jul 30, 2026
Merged

fix(core): drop PendingSubmission's guards in reverse acquisition order#9960
ErichDonGubler merged 3 commits into
gfx-rs:trunkfrom
AdrianEddy:fix-pending-submission-lock-drop-order

Conversation

@AdrianEddy

@AdrianEddy AdrianEddy commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Connections

Description

PendingSubmission holds a SnatchGuard acquired by its caller and a CommandIndices write guard acquired later, inside Queue::allocate_submission — but declares them in that same order. Struct fields drop in declaration order, so the guard acquired first is released first.

--cfg wgpu_validate_locks enforces stack-ordered release, so this fires on any submission reaching that path:

wgpu-core/src/lock/ranked.rs:182: assertion `left == right` failed: Lock not released in stacking order
released Device::command_indices  locked at wgpu-core/src/device/queue.rs:1654
when not expecting any locks to be held

Swapping the two declarations makes the release order match the acquisition order. Without the cfg there is no behavioural change — both guards are still released before the struct's other fields, and neither is observed in between.

Testing

With RUSTFLAGS="--cfg wgpu_validate_locks", cargo test -p wgpu-test --test wgpu-validation -- api::buffer_mapping:

  • trunk: aborts on the first test with the assertion above.
  • with this change: 10 passed.

Without the cfg, cargo test -p wgpu-core --all-features (65 pass) and cargo xtask test buffer are unaffected.

This does not make the whole suite pass under the validator. At least one unrelated violation remains on trunk — a rank ordering error rather than a release-order one:

last locked Device::command_indices  at wgpu-core/src/device/queue.rs:1658
now locking CommandBuffer::data      at wgpu-core/src/command/mod.rs:1418
Locking CommandBuffer::data after locking Device::command_indices is not permitted.

which still aborts the command-buffer tests. I have not investigated that one; it needs either a declared rank edge or a narrower guard scope, and I did not want to guess which.

Worth noting that nothing in CI or xtask passes --cfg wgpu_validate_locks, which is presumably how both of these accumulated.

Squash or Rebase?

Single commit; ready to rebase.

Checklist

  • I self-reviewed and fully understand this PR.
  • WebGPU implementations built with wgpu may be affected behaviorally.
  • Validation and feature gates are in place to confine behavioral changes.
  • Tests demonstrate the validation and altered logic works.
  • CHANGELOG.md entries for the user-facing effects of this change are present.
  • The PR is minimal, and doesn't make sense to land as multiple PRs.
  • Commits are logically scoped and individually reviewable.
  • The PR description has enough context to understand the motivation and solution implemented.

…rder

`PendingSubmission` holds a `SnatchGuard` acquired by its caller and a
`CommandIndices` write guard acquired later in `Queue::allocate_submission`,
but declares them in that same order. Struct fields drop in declaration
order, so the older guard was released first.

`--cfg wgpu_validate_locks` enforces stack-ordered release, so this trips
its assertion on the first submission that goes through this path — which
makes the validator unusable on the buffer-mapping path, since
`flush_writes_for_buffer` reaches it.

Swap the two declarations. No runtime behaviour changes without the cfg;
both guards are still released before the struct's other fields.

@ErichDonGubler ErichDonGubler 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.

Approved, conditional on current changes requested already being implemented. I'm leaving this open so you have a chance to object or suggest alternatives for the minor thing I wanted to see changed.

Comment thread wgpu-core/src/device/queue.rs Outdated
@ErichDonGubler ErichDonGubler self-assigned this Jul 30, 2026
@ErichDonGubler ErichDonGubler added type: bug Something isn't working area: correctness We're behaving incorrectly labels Jul 30, 2026
@ErichDonGubler
ErichDonGubler merged commit f9f55b7 into gfx-rs:trunk Jul 30, 2026
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: correctness We're behaving incorrectly type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants