vrr/qwen-code
2
0
Fork 0
mirror of https://github.com/QwenLM/qwen-code.git synced 2026-07-09 17:19:02 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 10

refactor(core): F2 PR A R2 — wenshao followup (visited set + dedup predicate)

Browse source 
Two Suggestions from wenshao's first PR #4411 review pass (07:15Z),
both small and worth folding before merge:

PR-A-R2 #1 (pid-descendants.ts:309 — walkDescendants visited set):
  `walkDescendants`'s BFS lacked a `visited` set. If the snapshot
  captures a PID-reuse cycle — rare but possible on busy hosts with
  rapid pid churn between `ps -A`'s start and parse, where Linux
  wraparound can show a freed pid in a different parent's children
  list creating an A→B / B→A cycle — pre-fix BFS would revisit nodes
  and fill the MAX_DESCENDANTS=256 quota with duplicate entries,
  starving legitimate descendants. Pre-PR-A the per-pid `pgrep` BFS
  had the same theoretical issue but was less exposed (each
  `pgrep -P pid` call returns only DIRECT children; snapshot captures
  the whole tree at once, making cycles instantly visible).

  Fix: 3-LOC `Set<number>` add. `root` seeded into `visited` so a
  malformed snapshot listing root as a descendant of its own child
  doesn't re-enqueue root either.

PR-A-R2 #2 (session-mcp-view.ts:117 — predicate dedup):
  After W12, the exported `passesSessionFilter` /
  `passesSessionPromptFilter` still called `passesNameFilter` (the
  pre-W12 array-based implementation), while `applyTools` /
  `applyPrompts` used `compiledFilterAccepts(compileNameFilter(...))`.
  Two parallel implementations of the same predicate — future change
  to one without the other would silently diverge:
    - the exported function's tests (passesSessionFilter unit tests)
      would still pass
    - the production filter path in applyTools/applyPrompts would
      behave differently

  Reviewer also noted `passesSessionPromptFilter` had zero callers
  in production code or tests after W12 — `applyPrompts` no longer
  references it. Kept the export rather than deleting it (matches
  the `passesSessionFilter` shape for symmetry + the F3 audit-path
  comment block earmarks both as the replay predicates), but routed
  both through `compiledFilterAccepts(compileNameFilter(...))` so
  there is a single source of truth. Set construction is per-call
  for these exports (negligible for unit-test / one-off probes);
  the bulk paths in `applyTools` / `applyPrompts` still construct
  ONE filter per pass via the original W12 code path.

`passesNameFilter` (the standalone array-based helper) deleted —
its only callers were the two exports, which now use the compiled
path. Public-API surface unchanged: the two exported functions
keep their signatures and semantics.

Test sweep: 19/19 pid-descendants + session-mcp-view tests pass;
typecheck + ESLint clean.

Continues commit chain: f05917071 (R9) → 20d2f1b90 (W11) →
6cf18f641 (W12) → 2a41c6fae (R10) → this (R2 followups).
This commit is contained in:
doudouOUC 2026-05-22 22:15:35 +08:00
parent 2a41c6faee
commit ced5d62b00
2 changed files with 51 additions and 29 deletions
Download patch file Download diff file Expand all files Collapse all files

17
packages/core/src/tools/pid-descendants.ts
View file

@ -305,9 +305,24 @@ async function listDescendantPidsWinPerPidFallback(
* platforms' per-node subprocess forks once the snapshot has been
* obtained. Same MAX_DESCENDANTS / MAX_DEPTH caps as the legacy
* fallback path. Returns BFS order — children before grandchildren.
*
* F2 (#4175 commit 6 review fix — wenshao PR-A-R2 #1): `visited`
* set prevents BFS revisits when the snapshot captures a PID-reuse
* cycle (rare but possible on busy hosts with rapid pid churn
* between snapshot start and parse — Linux pid wraparound can make
* `ps -A` show a freed pid in a different parent's children list,
* producing an A→B / B→A cycle). Pre-fix the cycle would fill the
* MAX_DESCENDANTS=256 quota with duplicate entries and starve
* legitimate descendants. The per-pid `pgrep` BFS fallback had the
* same theoretical issue but was less exposed because each
* `pgrep -P pid` call only returns DIRECT children; the snapshot
* captures the whole tree at once. `root` is seeded into `visited`
* so a malformed snapshot listing root as a descendant of one of
* its own children doesn't re-enqueue root.
*/
function walkDescendants(tree: Map<number, number[]>, root: number): number[] {
const all: number[] = [];
const visited = new Set<number>([root]);
const queue: Array<{ pid: number; depth: number }> = [
{ pid: root, depth: 0 },
];
@ -317,6 +332,8 @@ function walkDescendants(tree: Map<number, number[]>, root: number): number[] {
const children = tree.get(pid);
if (!children) continue;
for (const child of children) {
if (visited.has(child)) continue;
visited.add(child);
if (all.length >= MAX_DESCENDANTS) break;
all.push(child);
queue.push({ pid: child, depth: depth + 1 });

63
packages/core/src/tools/session-mcp-view.ts
View file

@ -13,35 +13,26 @@ import type { ToolRegistry } from './tool-registry.js';
const debugLogger = createDebugLogger('McpPool:View');
function passesNameFilter(
name: string,
includeTools?: readonly string[],
excludeTools?: readonly string[],
): boolean {
if (excludeTools?.includes(name)) return false;
if (!includeTools) return true;
return includeTools.some((entry) => {
const stripped = entry.includes('(')
? entry.slice(0, entry.indexOf('('))
: entry;
return stripped === name;
});
}
/**
* F2 (#4175 commit 6 review fix — wenshao W12 / PR A): precompute
* lookup `Set`s once per `applyTools` / `applyPrompts` pass so the
* per-tool predicate is O(1) instead of repeating the array scan
* inside `passesNameFilter` for every snapshot entry. Same semantics:
* `excludeTools` is direct-equality match (parens form not stripped —
* intentional pre-F2 behavior preserved); `includeTools` strips the
* first `(...)` suffix so `toolName(args)` matches `toolName`.
* F2 (#4175 commit 6 review fix — wenshao W12 / PR A; PR-A-R2 #2
* folded the exports to delegate here): precompute lookup `Set`s
* once per `applyTools` / `applyPrompts` pass so the per-tool
* predicate is O(1) instead of repeating an array scan for every
* snapshot entry. Same semantics: `excludeTools` is direct-equality
* match (parens form not stripped — intentional pre-F2 behavior
* preserved); `includeTools` strips the first `(...)` suffix so
* `toolName(args)` matches `toolName`.
*
* `passesSessionFilter` / `passesSessionPromptFilter` (the array-
* based predicates exported above) stay unchanged for unit tests
* and any caller that wants to test a single name without paying
* the Set-construction cost. The Sets live on `applyTools` /
* `applyPrompts`'s stack frame.
* PR-A-R2 #2: `passesSessionFilter` / `passesSessionPromptFilter`
* (exported below for unit-testability) now route THROUGH
* `compiledFilterAccepts(compileNameFilter(...))` so there is a
* single source of truth for the predicate. Pre-fix the exports
* called a separate `passesNameFilter` array-based implementation
* with the same semantics, creating a drift risk where a future
* change to one impl wouldn't be caught by tests of the other.
* The Set construction is per-call for these exports (cheap for
* tests / one-off probes); the bulk paths in
* `applyTools`/`applyPrompts` still construct ONE filter per pass.
*/
interface CompiledNameFilter {
excludeSet?: ReadonlySet<string>;
@ -91,13 +82,21 @@ function compiledFilterAccepts(
* support, intentionally matching the existing pre-F2 behavior so
* operators don't see semantic divergence between the two filter
* lists when migrating sessions through pool mode.
*
* PR-A-R2 #2: routes through `compiledFilterAccepts(compileNameFilter(...))`
* so the bulk-path predicate and the exported per-name predicate
* share one implementation. Set construction is paid per call here
* (negligible for unit tests / one-off audit-path probes).
*/
export function passesSessionFilter(
tool: DiscoveredMCPTool,
includeTools?: readonly string[],
excludeTools?: readonly string[],
): boolean {
return passesNameFilter(tool.serverToolName, includeTools, excludeTools);
return compiledFilterAccepts(
compileNameFilter(includeTools, excludeTools),
tool.serverToolName,
);
}
/**
@ -113,13 +112,19 @@ export function passesSessionFilter(
* parens form `excludeTools: ['toolName(args)']` which only matches
* tools (the parens-stripping in `passesSessionFilter` matches
* `toolName` in the include list, not the exclude list).
*
* PR-A-R2 #2: same delegation to the compiled path as
* `passesSessionFilter`.
*/
export function passesSessionPromptFilter(
promptName: string,
includeTools?: readonly string[],
excludeTools?: readonly string[],
): boolean {
return passesNameFilter(promptName, includeTools, excludeTools);
return compiledFilterAccepts(
compileNameFilter(includeTools, excludeTools),
promptName,
);
}
/**