Windows: stop full process-table CPU sampling on every session (fixes DPC_WATCHDOG_VIOLATION) - #13405
Conversation
|
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have the users @npapano42 on file. In order for us to review and merge your code, each contributor must visit https://cla.warp.dev to read and agree to our CLA. Once you have done so, please comment |
github-actions
Bot
added
the
external-contributor
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Every PR must be linked to a same-repo issue before Oz can review it. This PR is linked to #13404, but no linked issue is marked See the contribution guidelines for the full readiness model. Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
This PR is linked to #13404, but no linked issue is marked ready-to-implement yet. Only repository maintainers apply that label, so please wait for a maintainer to mark the issue. Once it is marked, push a new commit or comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
aminya
left a comment
aminya
left a comment
There was a problem hiding this comment.
I faced this DPC issue as well. Thanks
|
Hi @npapano42 — a reviewer requested changes on this PR and it hasn't had activity from you in 7 days. When you get a chance, please push updates or reply to the review so a reviewer can take another look. Without activity, this PR will be automatically closed after 14 days of inactivity. |
|
Hi @npapano42 — final reminder: a reviewer requested changes on this PR and it has been inactive for 10 days. It will be automatically closed in about 4 day(s) unless you push updates or reply. Maintainers can apply the |
|
The reviewer says the maintainers need to add a tag. This doesn't look like something the contributors can address. |
|
Yeah, I messaged in the slack channel to see if I can get it added, otherwise when the attached issue gets some traction I'll reopen the PR |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
Oz no longer requests changes for this pull request after the latest automated review.
There was a problem hiding this comment.
Overview
This PR changes Windows process enumeration used for Kaspersky detection so the full process-table refresh no longer samples per-process CPU/memory, and caches the Kaspersky check once per process launch to avoid repeating the sweep for every session bootstrap. It also adds a regression test asserting the full-table refresh kind remains name-only while the single-PID self-poll still samples CPU and memory.
Concerns
- No blocking correctness, security, or spec-alignment concerns found in the annotated diff.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
Bumping for review @acarl005 |
|
@acarl005 bumping again for review |
|
Ah thanks for bumping. I'll have a look |
…NT (#14907) ## Description Companion to warpdotdev/common-skills#50. A minimal, repo-specific addition to `.agents/skills/review-pr-local/SKILL.md`: two short clauses appended to the existing testing bullet and the existing commenting bullet (the latter added by #14902) under `## Repo-specific style and recurring review patterns`, noting that a confirmed violation of either should be treated as `⚠️ [IMPORTANT]`, not a nit. Nothing else changed — no new sections, no changes to `.agents/skills/rust-unit-tests/SKILL.md`, and #14902's original comment-audit bullet is otherwise untouched. `git diff origin/master --stat`: 1 file changed, 2 insertions(+), 2 deletions(-). Prompted by two real issues that slipped through a `review-pr` + `review-pr-local` pass on PR #13405: a transformation-comment violation in `app/src/util/windows.rs` (plus other `AGENTS.md` comment issues), and a "regression test" in `app/src/system/info_tests.rs` that only exercised a private helper instead of the call site where the historical bug lived. Linear: QUALITY-1563 ## Linked Issue - [x] N/A — skill-only documentation change, not tied to a GitHub issue. - [x] Not user-visible; no screenshots/video applicable. ## Testing Doc-only change — no code changed, so no automated/manual app testing applies. Validated together with warpdotdev/common-skills#50's specialization-contract repair via fresh subagents with no prior knowledge of the expected findings, run repeatedly with no scripted loading of this companion skill (each reviewer had to discover it on its own). Result across 3 final independent runs: the companion was discovered unprompted in all 3, confirming the core-skill specialization mechanism works; of the three original PR #13405 findings, the transformation-comment violation surfaced in 2 of 3 runs (⚠️ IMPORTANT), while the redundant/duplicated comment and the regression-test call-site gap did not surface in any of the 3 runs at this final state. The regression-test gap specifically has no corresponding content anywhere in this file or in `rust-unit-tests/SKILL.md` right now, so it isn't expected to surface until/unless that content is added here — see warpdotdev/common-skills#50's description for the full tallies, verbatim `review.json`, and validator output. ### Screenshots / Videos N/A — skill documentation change only. ## Agent Mode - [x] Warp Agent Mode - This PR was created via Warp's AI Agent Mode _Conversation: https://staging.warp.dev/conversation/ed29a745-bc15-4653-8e60-7ae1d25ec9da_ _Run: https://oz.staging.warp.dev/runs/019fedeb-6452-7cb8-b1a5-08c41f3f37c6_ _This PR was generated with [Oz](https://warp.dev/oz)._ --------- Co-authored-by: Oz <oz-agent@warp.dev> Co-authored-by: Warp Agent <agent@warp.dev>
|
@npapano42 Sorry for the delays. We've changed our review bot guidelines a bit so I'm going to run that again. |
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR reduces Windows session-bootstrap process enumeration cost by separating the full process-table refresh from the current-process CPU/memory refresh and caching the Kaspersky process check for the lifetime of the Warp process.
Concerns
- No blocking correctness, security, comment-quality, test-quality, or spec-alignment concerns found.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
|
@oz-agent rebase this onto master so we can review with the latest version of the skills |
|
Oz run started. |
|
I rebased this onto current
Rebasing onto today's So I pushed the rebased commit to a branch in Verification on the rebased branch:
If you'd prefer the rebase to land on #13405 itself rather than a mirror PR, someone with the Pull Requests: Remote branches:
|
|
/oz-review |
|
I'm re-reviewing this pull request in response to a review request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR changes the Windows Kaspersky detection path so the full process-table sweep uses a name-only sysinfo::ProcessRefreshKind, then caches the Kaspersky result once per Warp process. The added regression test checks that the full-table sweep no longer requests CPU or memory while the single-PID self-poll still does.
Concerns
- No blocking concerns found.
- The added comments explain the non-obvious Windows syscall/IPI rationale and cache trade-off rather than narrating implementation mechanics.
- No approved spec context was provided, and I found no material spec drift to evaluate.
- No security findings.
- Screenshots or video are not meaningful for this non-visual Windows syscall/crash-prevention change; the described Windows build, GUI bring-up, regression test, and sysinfo probe are the relevant evidence.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
There was a problem hiding this comment.
The code comments don't follow the project guidelines. Please ask your agent to read the commenting guidelines in AGENTS.md and edit the comments on this PR according to those. Make sure you merge/rebase the latest master before doing so.
I'll approve after these changes.
| /// forcing an all-core `KeFlushProcessWriteBuffers` IPI. The single-PID | ||
| /// self-poll ([`SystemInfo::refresh_kind`]) legitimately still samples both. | ||
| #[test] | ||
| fn all_processes_refresh_kind_does_not_sample_cpu_or_memory() { |
There was a problem hiding this comment.
not a useful test. you can delete this.
There was a problem hiding this comment.
Sorry about the long comment - fixed and test removed
is_kaspersky_running() enumerated the entire process table with per-process CPU cycle-time sampling on every terminal session bootstrap, just to check whether "avp" (Kaspersky) is running. On Windows each per-process CPU sample issues NtQueryInformationProcess(ProcessCycleTime), which forces KeFlushProcessWriteBuffers -- a synchronous inter-processor interrupt to every logical core. Across the whole process table, in bursts of session inits, on high-core-count machines this keeps all cores spinning at DISPATCH_LEVEL long enough to trip the DPC watchdog and bugcheck the system (DPC_WATCHDOG_VIOLATION 0x133). Confirmed via a kernel dump on a 24-thread Ryzen 9 5900X with Memory Integrity/HVCI enabled. Fix: - refresh_all_processes() now uses a name-only ProcessRefreshKind (all_processes_refresh_kind) instead of the CPU+memory self-poll kind, eliminating the per-process ProcessCycleTime syscalls. Names are still populated, so the Kaspersky name check is unchanged. - Cache is_kaspersky_running() in a OnceLock so the full sweep runs at most once per launch instead of on every session bootstrap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3fdabdb to
5eecb87
Compare
4 participants
Description
On Windows,
is_kaspersky_running()enumerated the entire system process table with per-process CPU cycle-time sampling on every terminal session bootstrap (every tab/pane/subshell/SSH session), with no cache — solely to check whether a process namedavp(Kaspersky) exists.On Windows, per-process CPU sampling makes the kernel issue
NtQueryInformationProcess(ProcessCycleTime)for every process, and the full enumeration issuesNtQuerySystemInformation(SystemProcessInformation). Both forceKeFlushProcessWriteBuffers, which broadcasts a synchronous inter-processor interrupt to every logical core. Across the whole process table, in bursts of session inits, on high-core-count machines (worse with HVCI, where each IPI is a hypercall) this keeps all cores spinning atDISPATCH_LEVELlong enough to trip the DPC watchdog and bugcheck the system (DPC_WATCHDOG_VIOLATION 0x133). Confirmed via a full kernel dump on a 24-thread Ryzen 9 5900X with Memory Integrity/HVCI enabled — the faulting thread was inwarp.exe, in exactly this call path.Fix
Two small, behavior-preserving changes:
app/src/system/info.rs—refresh_all_processes()now uses a dedicated name-onlyProcessRefreshKind(all_processes_refresh_kind()→ProcessRefreshKind::nothing()) instead of the CPU+memoryrefresh_kind()used by the single-PID self-poll. This drops the per-processProcessCycleTimesyscalls; process names are still populated, soprocesses_by_name("avp")is unchanged. The 5-second self-poll still samples CPU/memory (cheap — one PID).app/src/util/windows.rs—is_kaspersky_running()is cached in aOnceLock<bool>, so the full sweep runs at most once per launch instead of on every session bootstrap.Net effect: an N-process cycle-time storm on every session → one names-only enumeration, once per launch.
Trade-off: antivirus started mid-session is no longer re-detected. Impact is negligible — the cached flag only decides whether to attempt an
async_fsfallback for reading PowerShell history, which already degrades gracefully on error — and it's well worth eliminating a whole-system crash. (If a reviewer prefers, the cache can instead live as anOption<bool>on theSystemInfosingleton rather than a module-levelOnceLock.)Linked Issue
Fixes #13404 — see also #7771 and #7561, which this likely also addresses on Windows.
ready-to-specorready-to-implement. (Requestingready-to-implementon Windows: full process-table enumeration (with per-process CPU sampling) on every session bootstrap → DPC_WATCHDOG_VIOLATION (0x133) on high-core-count / HVCI systems #13404 — it's a self-contained bug fix; opening as a draft in the meantime.)Testing
Built and verified on Windows 11, AMD Ryzen 9 5900X (24 logical cores), Memory Integrity/HVCI ON, Rust 1.92.0 (MSVC).
Regression test (
app/src/system/info_tests.rs::all_processes_refresh_kind_does_not_sample_cpu_or_memory) asserting the full-table sweep samples neither CPU nor memory, while the single-PID self-poll still samples both. Passes; would have failed on the previous code (theAllsweep used the CPU-samplingrefresh_kind()).cargo clippy -p warp --all-targets --tests -- -D warnings→ clean../script/format --checkand./script/check_no_inline_test_modules→ clean.Behavioral verification with a standalone probe using the same
sysinfo0.37 API/features Warp uses, reproducingrefresh_all_processes'sProcessesToUpdate::Allsweep (364 processes):nothing()) refresh — all 364 processes have non-empty names, identical to the old.with_cpu().with_memory()path — soprocesses_by_name("avp")is unaffected.processes_by_name("explorer")resolvestrueunder the name-only refresh.ProcessCycleTimeall-core IPI cost that scales with core count and HVCI, and (b) the per-session repetition (now cached to once per launch).I have manually tested my changes locally with
./script/runguibinary on Windows (cargo build --bin warp-oss --features gui, the target./script/runruns on this platform) — compiles clean, and launched it as a full GUI: the app comes up, spawns terminal sessions, and runs without panic. This exercises the modified path directly —is_kaspersky_running()→refresh_all_processes()runs during session bootstrap — confirming the name-only refresh +OnceLockcache work end-to-end in the real app, not just in tests.Screenshots / Videos
Agent Mode
CHANGELOG-BUG-FIX: Windows — stopped enumerating the full process table with per-process CPU sampling on every terminal session, which could cause DPC_WATCHDOG_VIOLATION / high CPU on machines with many cores.