fix(core): re-read the fence before the queue-empty assert in Device::maintain

[AdrianEddy]

Connections

None.

Description

Device::maintain can panic on the defensive queue-empty assert when more than one thread polls the same device:

If the queue is empty, the current submission index (20) should be at least the wait submission index (21)

current_finished_submission is read from the fence before Queue::maintain, but queue_empty is observed inside it. Device::poll takes &self, so another thread can retire and triage submissions between those two observations — leaving the sampled fence value stale while the queue is genuinely empty.

Nothing spans the two observations: the lifetime-tracker mutex is only held inside Queue::maintain, and the command_indices read guard is dropped before it. That guard closes a different window — a poller racing a submitter between last_successful_submission_index advancing and track_submission landing — but it does nothing against a concurrent triager, which drains under lock_life.

The window is easy to hit with a zero-timeout Wait, the idiomatic "has this submission retired yet?" probe: the wait returns Ok(false), that bool is deliberately ignored, so current_finished_submission stays below wait_submission_index — and any concurrent poller that then drains the lifetime tracker makes queue_empty true. The racing thread need not be another Wait: a plain poll(Poll) drain loop triages too.

The fix (per review) captures the previously ignored Ok(bool) wait outcome and reports QueueEmpty only when the poll itself proved it: the branch is gated on wait_succeeded != Some(false), so a timed-out wait falls through to the existing fence-value check and resolves to WaitSucceeded or Timeout instead; the next poll observes the empty queue. This also makes two documented contracts actually hold: PollStatus::QueueEmpty ("this implies that the given Wait was satisfied") and PollType::Wait::timeout ("the poll will return PollError::Timeout").

The gate excludes only the timed-out case rather than requiring a successful wait, so PollType::Poll keeps its ability to report QueueEmpty — poll_all(force_wait: false) computes all_queue_empty from exactly that status, and non-blocking is_queue_empty() drain loops rely on it. With the gate in place the assert is sound with the existing fence sample: when it runs with a wait index, the wait must have succeeded, the sample was taken after that wait, and the fence is monotonic. No extra fence read, no new fallible call.

Testing

Reproduced on unmodified trunk with a small stress harness — one thread submitting copies and draining with poll(Poll), four threads spamming poll(Wait { submission_index: None, timeout: Some(ZERO) }). The drain loop additionally asserts that non-blocking Poll still observes QueueEmpty (guarding the poll_all(false) semantics):

• Unmodified trunk: panics in under 1 s on both Vulkan and DX12 (RTX 5090).
• With this change: Vulkan, 30 s — 814,080 submissions, 12,720 full non-blocking drains, 4.40 M wait polls (408 k QueueEmpty, 470 k WaitSucceeded, 3.52 M Timeout); DX12, 30 s — 515,200 submissions, 8,050 drains, 9.52 M wait polls. No panic, every drain loop terminated.

cargo xtask test poll — 72/72 pass across all GPU entries. cargo test -p wgpu-core --all-features — 65 pass. Clippy clean.

I have not added this as a #[gpu_test]; both racing roles are plain public API so it could ship as one, skipping the noop backend (which executes submissions synchronously, so the fence never lags and the race is unreachable there). Happy to add it if wanted.

Squash or Rebase?

Squash (the history reflects the review iteration).

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.

[andyleiserson]

It's possible this is a regression introduced by #9475, but I haven't gone back and studied the old code to assess that, and it doesn't really matter. Mentioning it so the link is here for archeological purposes.

[andyleiserson]

I guess reading the fence value is fairly cheap, but even so, I'm not sure we want to be doing extra bookkeeping for asserts without a compelling reason, which I'm not sure I see here.

I thought about removing the assert entirely, but I think another possibility is:

• Change if queue_empty on line 1074 to if queue_empty && wait_result == Some(true) (plus changes to save wait_result)
• Change the assertion to: assert!(wait_submission_index.is_some_and(|w| finished_submission >= w)

The idea being, if the poll was successful, then:

1. wait_submission_index was not None
2. On a successful (not timed out) wait, any fence value read after that really should exceed wait_submission_index
3. If the poll was not successful but we happened to empty the queue after that, give up on this assertion.

I guess a consequence of the change to the queue_empty condition on line 1074, is that some racy cases will return Err(WaitIdleError::Timeout) instead of Ok(PollStats::QueueEmpty). That seems like it should be okay? Anything that is sensitive to the difference is probably racy and broken anyways. And it gives back the protection the assert would have offered: if the caller really wants to see empty, they can call again, and see that. We'll only return QueueEmpty in cases where we're sure.

Or an entirely different approach, declare that QueueEmpty is a footgun (since the queue can immediately go non-empty again) and stop reporting it entirely, but that would be a breaking change.

[AdrianEddy]

Good call — done in a981c28, with one adjustment: the gate is wait_succeeded != Some(false) rather than == Some(true), i.e. it only excludes the timed-out case. Requiring a successful wait would make PollType::Poll (where the wait outcome is None) unable to report QueueEmpty — poll_all(force_wait: false) computes all_queue_empty from exactly that status, and non-blocking is_queue_empty() drain loops would never terminate.

With that gate the assert keeps its current shape and is sound with the existing sample: when it runs with a wait index, the wait must have succeeded, current_finished_submission was read after that wait, and the fence is monotonic — so no re-read, no extra bookkeeping. It is admittedly close to tautological now (it only catches a backend whose wait lies about success or whose fence values regress), so I'm equally happy to drop it entirely if you'd prefer.

On the consequence: agreed it's fine, and arguably overdue — PollStatus::QueueEmpty is documented as "this implies that the given Wait was satisfied", which the racy path violated, and Wait::timeout already promises PollError::Timeout on a timed-out wait. The changelog entry now spells out the behavior change.

On dropping QueueEmpty entirely: I'd push back a little. For a non-blocking Poll there is no other signal for "all in-flight work has retired", and poll_all / drain loops depend on it. The "queue can immediately go non-empty again" caveat is inherent to any point-in-time status and the docs already state it.

Re-verified with the same stress harness, with the drain loop now asserting that Poll observes QueueEmpty: unmodified trunk still panics in under 1 s on Vulkan and DX12; this branch runs clean for 30 s on each (814 k / 515 k submissions, 4.4 M / 9.5 M wait polls, 12,720 / 8,050 full drains). cargo xtask test poll is 72/72 across all GPU entries.