test(vk): make recorder destruction checks independent of scheduling #101

Open

mooki wants to merge 1 commit from mooki/Longshot:test/recorder-sync-observer into main

pull from: mooki/Longshot:test/recorder-sync-observer

merge into: MagicBeanDev:main

MagicBeanDev:main

Conversation 1 Commits 1 Files changed 3 +32 -3

mooki commented 2026-11-13 02:56:16 +00:00

Contributor

• Copy link
• Copy Markdown

Why

The recorder regression can report NOT CAUGHT for a deliberately missing destruction wait when the recorder finishes before the observer sees the bad order. This repairs the test, not the production recorder; Windows execution still needs a repeated run.

What changed

• tests/recorder_sync_test.cpp, destroy_recorded: drain the preceding frame, restart the empty ring at a packet boundary, and keep the recorder awake with its existing thread-start hook until the next publication. The final copy remains unpublished until an ordered boundary flushes it.
• With the real destruction boundary, the copy is published and completed before destruction. With the unchanged broken variant, destruction happens first and the following pool reset publishes the copy: the existing destroyed-buffer and complete call-order checks both fail, however long the producer is paused.
• Assert the fixture's pending, unpublished-copy precondition. Clear the hook only after joining the recorder. No sleep, retry, longer timeout or source-text assertion was added.
• Explain the ordering guarantee in tests/test_recorder_sync.py and the existing development guide. All broken variants remain. No production source changed.

Flags and settings

None.

Measurements

Branch state: on main 5eaaf7fb9c548782a3dfabcf89ad37363ee76b1e; head ea5c10c2eb1266fc3ca1864979e410a3b484ef5c; whole game-free suite on this head: 510 passed, 0 failed, 19 skipped (main 5eaaf7fb9c548782a3dfabcf89ad37363ee76b1e: 510 passed, 0 failed, 19 skipped); the commits are unchanged by rebases apart from the results table.

main: f41db065cf (2026-11-12)
this PR: move/test/recorder-sync-observer @ 755c6ac3ffd0b5c394d349a3005a66d6791d0400
Hardware: Intel Core i9-13900K; Arch Linux, kernel 7.2.15-1-omarchy; clang 23.1.3. No GPU or Vulkan device used.
Scene: the fake Vulkan recorder fixture, not a game scene.
Method: repeated destruction-negative executions; deliberate CPU contention; temporary producer delays; and two interleaved batches per version of all eight positive environment cases, after one warm-up each.
How to reproduce: python -I -B tests/test_recorder_sync.py --cxx clang++ --headers path/to/Vulkan-Headers/include.

The escaping schedule

The original destroy_recorded queues a frame and a final copy naming a buffer, then destroys that buffer. The fake destruction and copy share the log mutex. If the producer is descheduled long enough after packing the copy, the recorder reaches the fake first. Both the resource check and call-order comparison then see the valid order, although the destruction boundary is broken. This scenario had no hold() call; even a fixed-duration hold would not guarantee the producer runs before it expires.

To force that schedule, a temporary fixture copy inserted std::this_thread::sleep_for(std::chrono::milliseconds(10)); on the producer immediately before the final copy_runs(kUp, 0) and immediately before vkDestroyBuffer, only in THREAD mode. The first pause lets the recorder sleep, causing the next packet to publish immediately; the second lets that packet finish. These two sleeps are not in this PR. The same pauses were applied to both versions.

The isolated runs select the existing destruction scenario and its unchanged check_run observer. The complete --only thread checks were also run with the forced schedule, including both million-packet stress runs.

Negative variant caught / escaped	main @ f41db065c	this PR @ 755c6ac3
300 plain destruction-only runs, two at once	300 / 0	300 / 0
300 destruction-only runs, two fixtures and a busy process sharing one CPU	300 / 0	300 / 0
100 destruction-only runs with the two producer pauses	0 / 100	100 / 0
Two complete thread-check runs with the same pauses	0 / 2	2 / 0

Every repaired negative failure above was the intended destroyed-buffer violation and copy/destruction mismatch, not a timeout, setup failure or crash. The repaired positive fixture still passed 100/100 isolated runs with the same pauses.

Original full thread-check escape:

---- broken: thread: vkDestroyBuffer made without waiting (added to unordered()) -> NOT CAUGHT (exit code 0)
PASS thread: a buffer destroyed while a recording names it (the commands naming it run first): 279 calls, 16502 argument bytes identical
ALL PASS

With the repaired observer and the same broken recorder and producer pauses:

FAIL thread: a buffer destroyed while a recording names it (the commands naming it run first): 1 lifecycle violation(s), the first: vkCmdCopyBuffer on command buffer 0x200 names a destroyed buffer (driver call 273)
MISMATCH thread: a buffer destroyed while a recording names it (the commands naming it run first): call 271 of 279/279: expected vkCmdCopyBuffer, got vkDestroyBuffer (bytes differ from 0)
FAILED (2)

Test duration

All eight positive environment cases passed in each batch, in main / PR / main / PR order. These are fixture execution times, excluding compilation, on the same shared PC; no game speed claim is made.

Eight positive cases	main @ f41db065c	this PR @ 755c6ac3
First batch	12.770 s	12.502 s
Second batch	12.278 s	11.956 s
Median	12.524 s	12.229 s

No material increase was observed. The small difference is not claimed as a speedup; the machine was shared. The repair adds no timer wait.

Testing

Tested: On Linux, the complete tests/test_recorder_sync.py passed: all eight positive environment cases and all 23 applicable broken variants. The full named test took 83.072 seconds, including compilation. The repeated before/after checks and 32 interleaved positive executions are above. The final fixture compiled to x86-64 Windows COFF objects with both real and destruction-negative recorder headers, using clang's Windows target, llvm-mingw headers and libc++, C++20, and -Wall -Wextra -Werror. That was compilation only, without Windows execution.

The complete game-free suite on the exact head also passed: python -I -B tests/run_all.py — 509 passed, 0 failed, 19 skipped. No failure was retried.

---- broken: thread: vkDestroyBuffer made without waiting (added to unordered()) -> caught (exit code 1)
PASS recorder sync: identical streams, poisoned caller memory, coverage, fallback order; broken variants caught

Not tested: Windows execution of the changed fixture and its existing Windows-only process-exit/DLL checks: no Windows runtime was available. The cross-compile does not test Microsoft's C++ library. No game or GPU run, because production code is unchanged. The precise scheduling of the historical Windows failure was not captured; Linux reproduces a concrete cause of the same result, not a trace of that Windows run.

Needs other testers:

• Windows: repeat python -I -B tests/test_recorder_sync.py --headers path/to/Vulkan-Headers/include at least 200 times, including under CPU load, expecting every positive case to pass and every broken variant, especially vkDestroyBuffer made without waiting, to be caught on every run (test request T-3FRA)

Screenshots

Nothing on screen.

Risk and rollback

The fixture depends on the recorder's publication counters, batch size and existing start hook. A future packet/batching change may require updating this setup; the explicit unpublished-copy check prevents a silently ineffective negative test. The hook changes only while the recorder is joined. Reverting this commit restores the previous test. No player setting or production behavior changes.

Reviewer notes

Start with destroy_recorded and the unchanged destruction mutation in test_recorder_sync.py. The test now exercises a pending unpublished copy; it does not claim to make every other timing-dependent negative case deterministic, or to test a different flush-without-wait mutation.

The original Windows output is in https://github.com/mooki000/Hookshot/actions/runs/43280308588/job/131399531380 . The separate investigation was promised at https://github.com/NotRexed/Hookshot/pull/2830#issuecomment-6832639364 . Its preceding 30-second timeout concerned the deliberately swallowed exception, not the destruction escape.

The measured base is retained while this remains WIP; it has not been refreshed onto the newer main.

Issues

See #90

Checklist

For everyone

• One topic.
• The title is a Conventional Commit (type(scope): description).
• Rebased on current main.
• python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above).
• Tested in the game, or what wasn't is named under Testing.
• Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
• Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported.
• No game files, dumps, build output or generated files.
• third_party/ untouched (a new folder with a PROVENANCE.md instead).
• The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

• AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
• A game-free test added for the fix, failing on main and passing here.
• A check under tests/ that reads changed source updated in this PR, not weakened.
• Docs updated.

---

New on this forge; not opened on GitHub.

Assisted-by: GPT-6 Astra
Co-Authored-By: GPT-6 Astra [email protected]

## Why The recorder regression can report `NOT CAUGHT` for a deliberately missing destruction wait when the recorder finishes before the observer sees the bad order. This repairs the test, not the production recorder; Windows execution still needs a repeated run. ## What changed - `tests/recorder_sync_test.cpp`, `destroy_recorded`: drain the preceding frame, restart the empty ring at a packet boundary, and keep the recorder awake with its existing thread-start hook until the next publication. The final copy remains unpublished until an ordered boundary flushes it. - With the real destruction boundary, the copy is published and completed before destruction. With the unchanged broken variant, destruction happens first and the following pool reset publishes the copy: the existing destroyed-buffer and complete call-order checks both fail, however long the producer is paused. - Assert the fixture's pending, unpublished-copy precondition. Clear the hook only after joining the recorder. No sleep, retry, longer timeout or source-text assertion was added. - Explain the ordering guarantee in `tests/test_recorder_sync.py` and the existing development guide. All broken variants remain. No production source changed. ## Flags and settings None. ## Measurements <!-- branch-state --> Branch state: on main `5eaaf7fb9c548782a3dfabcf89ad37363ee76b1e`; head `ea5c10c2eb1266fc3ca1864979e410a3b484ef5c`; whole game-free suite on this head: `510 passed, 0 failed, 19 skipped` (main `5eaaf7fb9c548782a3dfabcf89ad37363ee76b1e`: `510 passed, 0 failed, 19 skipped`); the commits are unchanged by rebases apart from the results table. <!-- /branch-state --> main: f41db065cf6667ec638a10aa6990099392d10dca (2026-11-12) this PR: move/test/recorder-sync-observer @ 755c6ac3ffd0b5c394d349a3005a66d6791d0400 Hardware: Intel Core i9-13900K; Arch Linux, kernel 7.2.15-1-omarchy; clang 23.1.3. No GPU or Vulkan device used. Scene: the fake Vulkan recorder fixture, not a game scene. Method: repeated destruction-negative executions; deliberate CPU contention; temporary producer delays; and two interleaved batches per version of all eight positive environment cases, after one warm-up each. How to reproduce: `python -I -B tests/test_recorder_sync.py --cxx clang++ --headers path/to/Vulkan-Headers/include`. ### The escaping schedule The original `destroy_recorded` queues a frame and a final copy naming a buffer, then destroys that buffer. The fake destruction and copy share the log mutex. If the producer is descheduled long enough after packing the copy, the recorder reaches the fake first. Both the resource check and call-order comparison then see the valid order, although the destruction boundary is broken. This scenario had no `hold()` call; even a fixed-duration hold would not guarantee the producer runs before it expires. To force that schedule, a temporary fixture copy inserted `std::this_thread::sleep_for(std::chrono::milliseconds(10));` on the producer immediately before the final `copy_runs(kUp, 0)` and immediately before `vkDestroyBuffer`, only in THREAD mode. The first pause lets the recorder sleep, causing the next packet to publish immediately; the second lets that packet finish. These two sleeps are **not in this PR**. The same pauses were applied to both versions. The isolated runs select the existing destruction scenario and its unchanged `check_run` observer. The complete `--only thread` checks were also run with the forced schedule, including both million-packet stress runs. | Negative variant caught / escaped | main @ f41db065c | this PR @ 755c6ac3 | |---|---:|---:| | 300 plain destruction-only runs, two at once | 300 / 0 | 300 / 0 | | 300 destruction-only runs, two fixtures and a busy process sharing one CPU | 300 / 0 | 300 / 0 | | 100 destruction-only runs with the two producer pauses | 0 / 100 | 100 / 0 | | Two complete thread-check runs with the same pauses | 0 / 2 | 2 / 0 | Every repaired negative failure above was the intended destroyed-buffer violation **and** copy/destruction mismatch, not a timeout, setup failure or crash. The repaired positive fixture still passed 100/100 isolated runs with the same pauses. Original full thread-check escape: ```text ---- broken: thread: vkDestroyBuffer made without waiting (added to unordered()) -> NOT CAUGHT (exit code 0) PASS thread: a buffer destroyed while a recording names it (the commands naming it run first): 279 calls, 16502 argument bytes identical ALL PASS ``` With the repaired observer and the same broken recorder and producer pauses: ```text FAIL thread: a buffer destroyed while a recording names it (the commands naming it run first): 1 lifecycle violation(s), the first: vkCmdCopyBuffer on command buffer 0x200 names a destroyed buffer (driver call 273) MISMATCH thread: a buffer destroyed while a recording names it (the commands naming it run first): call 271 of 279/279: expected vkCmdCopyBuffer, got vkDestroyBuffer (bytes differ from 0) FAILED (2) ``` ### Test duration All eight positive environment cases passed in each batch, in main / PR / main / PR order. These are fixture execution times, excluding compilation, on the same shared PC; no game speed claim is made. | Eight positive cases | main @ f41db065c | this PR @ 755c6ac3 | |---|---:|---:| | First batch | 12.770 s | 12.502 s | | Second batch | 12.278 s | 11.956 s | | Median | 12.524 s | 12.229 s | No material increase was observed. The small difference is not claimed as a speedup; the machine was shared. The repair adds no timer wait. ## Testing **Tested:** On Linux, the complete `tests/test_recorder_sync.py` passed: all eight positive environment cases and all 23 applicable broken variants. The full named test took 83.072 seconds, including compilation. The repeated before/after checks and 32 interleaved positive executions are above. The final fixture compiled to x86-64 Windows COFF objects with both real and destruction-negative recorder headers, using clang's Windows target, llvm-mingw headers and libc++, C++20, and `-Wall -Wextra -Werror`. That was compilation only, without Windows execution. The complete game-free suite on the exact head also passed: `python -I -B tests/run_all.py` — **509 passed, 0 failed, 19 skipped**. No failure was retried. ```text ---- broken: thread: vkDestroyBuffer made without waiting (added to unordered()) -> caught (exit code 1) PASS recorder sync: identical streams, poisoned caller memory, coverage, fallback order; broken variants caught ``` **Not tested:** Windows execution of the changed fixture and its existing Windows-only process-exit/DLL checks: no Windows runtime was available. The cross-compile does not test Microsoft's C++ library. No game or GPU run, because production code is unchanged. The precise scheduling of the historical Windows failure was not captured; Linux reproduces a concrete cause of the same result, not a trace of that Windows run. **Needs other testers:** - Windows: repeat `python -I -B tests/test_recorder_sync.py --headers path/to/Vulkan-Headers/include` at least 200 times, including under CPU load, expecting every positive case to pass and every broken variant, especially `vkDestroyBuffer made without waiting`, to be caught on every run (test request T-3FRA) ## Screenshots Nothing on screen. ## Risk and rollback The fixture depends on the recorder's publication counters, batch size and existing start hook. A future packet/batching change may require updating this setup; the explicit unpublished-copy check prevents a silently ineffective negative test. The hook changes only while the recorder is joined. Reverting this commit restores the previous test. No player setting or production behavior changes. ## Reviewer notes Start with `destroy_recorded` and the unchanged destruction mutation in `test_recorder_sync.py`. The test now exercises a pending **unpublished** copy; it does not claim to make every other timing-dependent negative case deterministic, or to test a different flush-without-wait mutation. The original Windows output is in https://github.com/mooki000/Hookshot/actions/runs/43280308588/job/131399531380 . The separate investigation was promised at https://github.com/NotRexed/Hookshot/pull/2830#issuecomment-6832639364 . Its preceding 30-second timeout concerned the deliberately swallowed exception, not the destruction escape. The measured base is retained while this remains WIP; it has not been refreshed onto the newer main. ## Issues See #90 ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. - [x] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). - [x] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [x] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] A game-free test added for the fix, failing on `main` and passing here. - [x] A check under `tests/` that reads changed source updated in this PR, not weakened. - [x] Docs updated. --- New on this forge; not opened on GitHub. Assisted-by: GPT-6 Astra Co-Authored-By: GPT-6 Astra <[email protected]>

mooki commented 2026-11-13 03:03:34 +00:00

Author

Contributor

• Copy link
• Copy Markdown

The test request for a repeated Windows run is open (T-3FRA). Rebased onto main 5eaaf7fb9 with the patch unchanged; whole suite on the new head ea5c10c2e: 510 passed, 0 failed, 19 skipped. Ready for review.

The test request for a repeated Windows run is open (T-3FRA). Rebased onto main 5eaaf7fb9 with the patch unchanged; whole suite on the new head ea5c10c2e: 510 passed, 0 failed, 19 skipped. Ready for review.

mooki changed title from WIP: test(vk): make recorder destruction checks independent of scheduling to test(vk): make recorder destruction checks independent of scheduling 2026-11-13 03:03:34 +00:00

mooki force-pushed test/recorder-sync-observer from 755c6ac3ff to ea5c10c2eb 2026-11-13 03:03:35 +00:00 Compare

klimovich008 referenced this pull request 2026-11-13 08:10:07 +00:00

perf(vk): the recorder thread's slot resets without the wait and an idle that waits on its ring (RX_VK_RECORDER_OWN, RX_VK_RECORDER_IDLE=wait; opt-in) #162

Some checks are pending

Hide checks

Linux AppImage / appimage (pull_request) Blocked by required conditions

Details

Windows builder / Longshot Builder zip (pull_request) Blocked by required conditions

Details

Windows builder / Windows Vulkan back end (llvm-mingw) (pull_request) Blocked by required conditions

Details

Windows builder / dav1d for Windows (llvm-mingw) (pull_request) Blocked by required conditions

Details

CI / Conventional Commit title (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions

Details

CI / Python source checks (windows-2022, part 1/2) (pull_request) Blocked by required conditions

Details

CI / Python source checks (windows-2022, part 2/2) (pull_request) Blocked by required conditions

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions

Details

CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions

Details

Third-party parts / As their notes say (pull_request) Blocked by required conditions

Details

Third-party parts / The same as upstream (pull_request) Blocked by required conditions

Details

PR description / Description follows the template (pull_request_target) Successful in 2s

Details

This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.

Some workflows are waiting to be reviewed.

This branch is out-of-date with the base branch

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u 'test/recorder-sync-observer:mooki-test/recorder-sync-observer'

git switch 'mooki-test/recorder-sync-observer'

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

area/accessibility

Barriers for people with disabilities

  

area/audio

Sound and music

  

area/input

Controllers, keyboard, mouse

  

area/install

Installing, updating, building from your game

  

area/menu

In-game menu and settings

  

area/renderer

Vulkan renderer, shaders, effects

  

area/saves

Save files

  

area/upscaling

DLSS, FSR, frame generation

  

good first issue

A good start for a new contributor

  

gpu/amd

AMD GPUs

  

gpu/igpu

Integrated GPUs

  

gpu/intel

Intel GPUs

  

gpu/nvidia

NVIDIA GPUs

  

help wanted

Extra hands or knowledge wanted

  

kind

bug

Something doesn't work as it should

  

kind

chore

Build, CI, dependencies, tidying

  

kind

docs

Documentation

  

kind

feature

A new feature or an improvement

  

kind

performance

Frame rate, frame time, loading, memory

  

kind

question

A question rather than a change

  

needs measurements

Waiting for before/after measurements

  

platform/linux

Linux only

  

platform/steam-deck

Steam Deck

  

platform/windows

Windows only

  

preview

Maintainers: deploy a preview of this PR's website (read its changes first). Removed after the run.

  

priority

critical

Crashes, data loss, or blocks most players

  

priority

high

Important: next in line

  

priority

low

Nice to have

  

status

blocked

Waits on something else first

  

status

confirmed

Reproduced or agreed on

  

status

duplicate

Already reported or proposed

  

status

invalid

Not a problem in this project

  

status

needs-info

Waiting for more information from the reporter

  

status

wontfix

Will not be worked on

  

testing wanted

Wanted: a test on other hardware (posts a test request in #test-requests)

No labels

area/accessibility

area/audio

area/input

area/install

area/menu

area/renderer

area/saves

area/upscaling

good first issue

gpu/amd

gpu/igpu

gpu/intel

gpu/nvidia

help wanted

kind

bug

kind

chore

kind

docs

kind

feature

kind

performance

kind

question

needs measurements

platform/linux

platform/steam-deck

platform/windows

preview

priority

critical

priority

high

priority

low

status

blocked

status

confirmed

status

duplicate

status

invalid

status

needs-info

status

wontfix

testing wanted

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

MagicBeanDev/Longshot!101

No description provided.