Repository navigation
fix(frida): drain admitted operations and honor queued cancellation - #2305
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
TL;DR: Fixed, with real Frida on Linux: a cancelled queued load, unload or resume is never sent to the target. Shutdown now waits for an attach that is still running, and a failed cleanup there gets another try. Not tested for real: remote discovery cleanup. I ran base 0365454 and head 344fc71 with real Frida, 3 times per case. Each run had a fresh MCP server and a fresh target, a small C program of my own. ResultsThe table counts how often the old bug showed up.
Not run
Thanks, rohitg00, for the fix and for tests that cancel through the real MCP client. How the runs workedThe key repro for the first row is short. A script that loops for 2 seconds holds the session. I queue a second load and cancel it with the SDK abort signal. On base, the second script writes its marker in the target. On head, it never does. The first script keeps running on both trees. For the shutdown rows I ran REA's server in one process. The SDK client used an in-memory link, and Frida was real. On base, The shutdown row used natural timing. A rerun with a 1.5 second pause before the real attach gave the same result as the natural timing, on both trees. The attach cleanup row used that pause and a first real The open session row used the throwing CommandsEvery run used its own server and its own target. I checked the target's log for the script markers. I checked its memory map for the Frida agent. # per tree, Node 24.18.0, HOME set to an empty folder
npm ci && npm run build && npm run test:prepare # exit 0, 0, 0 in both trees
# the target: calls named_function every 100 ms
gcc -O1 -rdynamic -o target target.c # exit 0
# 7 cases x 2 trees x 3 runs, then 2 more cases x 2 trees x 3 runs
node cases.mjs <case> <tree> <label> <run> # 54 runs, all exit 0
# the PR's test file, one Node at a time
npm run test:focused -- tests/boundary/mcp/fridaLifecycleMcp.test.tsThe test file exited 0 on head with Node 22.23.1, 24.18.0 and 26.11.1. It exited 1 on base with Node 24.18.0. Test setup
🤖 Generated with Claude Code (Sonnet 5.5, verified by Opus 5.5) 🦊 |
|
@greptileai review |
|
This PR is set for a Plus review. TREX isn't supported with Plus or Apex reviews yet, so Greptile reviews this PR at Base while TREX is on. |
|
Source-reviewed changes. Tests, builds and check reruns were skipped at maintainer request.
Node timers can fire while performance.now() is still a fraction of a millisecond short of the monotonic deadline. ProviderStartupDeadline then reported interruption "timeout" and an aborted signal while remainingMs() returned ceil(0.x) = 1, so polling loops guarded by remainingMs() > 0 (Hopper and Ghidra startup) could run past an elapsed deadline, and the real-timer test "aborts a bounded wait when its real monotonic deadline expires" failed intermittently in CI (expected 1 to be 0). Once the deadline's own timer has classified a timeout, report zero remaining time. A deterministic regression test fires the timer with the monotonic clock 0.4 ms short of the deadline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix(comparison): preserve incomplete function call evidence Keep missing, unknown, partial and unresolved call relationships unknown in function comparisons and reconstruction claims. Aggregate IDA caller and callee coverage without overwriting uncertainty. Validate with 144 focused tests, typecheck/lint, docs checks and test discovery. Real providers were not verified; the full headless IDA session suite requires unavailable Windows native controls. * fix(comparison): recheck historical calls-only changes --------- Co-authored-by: morluto <76467478+morluto@users.noreply.github.com> (cherry picked from commit 688defc)
(cherry picked from commit 8cac51e)
(cherry picked from commit 1335bcd)
…2895) * fix: repair the static-check failures on the default branch The default branch fails its own required `Static checks` job three ways, so every pull request reports that job as failed regardless of its own change. - `npm run format:check` reports `src/domain/functionComparison.test.ts` and `src/ida/IdaOperationRunner.ts`. - `npm run lint` reports a duplicate `import { join } from "node:path"` in `tests/boundary/mcp/ghidraInventoryMcp.test.ts` (lines 4 and 17). - `npm run typecheck` reports `format: "devin"` missing from the client format union in `src/application/SupportedClients.ts`, and an unchecked index in `tests/boundary/cli/clientProfileLifecycle.test.ts`, whose pair array is inferred as `string[][]`. Apply the pinned formatter, drop the duplicate import, list the `"devin"` format that its own definition already uses, and mark the pair array `as const`. Validation: `npm run check:fast` and `npm run format:check` pass. * fix(setup): keep Devin on the JSON configuration dialect --------- Co-authored-by: 吴佳浩 <wujiahao@supremind.com> Co-authored-by: morluto <76467478+morluto@users.noreply.github.com> (cherry picked from commit df3d96d)
Summary
Make Frida shutdown drain admitted operations and settle script-load cancellation before a queued resume can run. Cancelled operations no longer dispatch native effects after waiting for the session lock. Failed cleanup remains owned and retryable.
Related: #2535 and #2544.
Problem and expected behavior
A cancelled queued load, resume or unload could still execute, and shutdown could finish while attach or discovery was acquiring resources. Cancellation during native script loading also allowed a waiting resume to observe the cancelled script before service-level rollback ran.
The manager now owns admission, queue reservation, final load outcome and cleanup. An earlier operation reserves its session position synchronously, so a later close cannot overtake it. Cancellation during creation or loading releases the new script before returning. A failed unload retains the script identity and prevents premature resume. Once a load commits successfully, later cancellation cannot reclassify it; explicit unload releases it.
Change and scope
Drain admitted work, share close attempts, forward request cancellation and preserve cleanup ownership. Remove the service's superseded post-success rollback. Keep existing FIFO message capture, earlier scripts and caller-owned persistent sessions intact.
The Apple archive regression fixture also has a bounded 90-second compiler timeout within a 120-second test budget, preserving its assertions when a cold Swift cache exceeds the previous 30-second budget. Landed upstream CI and dyld fixture corrections are integrated without changing their implementation.
Contract and boundary impact
Expected existing effects:
Evidence and regression coverage
Real MCP registration, Client/InMemoryTransport, service and manager tests with controlled native bindings cover queued cancellation, attach/discovery shutdown, synchronous admission, reentrant shutdown, creation/loading cancellation, failed cleanup and explicit retry. Ordering regressions fail before their producing-boundary fixes. Retained Evidence is checked against what queued native resume actually observed.
Cancelled-load results retain script identity and messages with
script_cleanup: "released"or"incomplete_or_unknown"; committed successful loads remain explicitly unloadable.Validation performed
After integrating upstream
fb28ec1, using Node 24.18.0/npm 11.16.0 on macOS arm64:npm run test:focused -- tests/boundary/mcp/fridaLifecycleMcp.test.ts src/application/frida/FridaInstrumentationManager.test.ts src/application/frida/FridaInstrumentationService.test.ts tests/boundary/mcp/fridaMcp.test.ts tests/boundary/cli/fridaCli.test.ts tests/boundary/cli/dylibResolutionCli.test.ts: 82 tests passed across six files, including the upstream dyld correction.npm run check:ci: all eight tasks and architecture guards passed.npm run docs:check: passed, including both README consistency tests.git diff --check: passed. The upstream merge required no conflict resolution or additional source changes.Earlier native Frida verification passed at
5792141in a disposable Linux arm64 Debian Bookworm container with Frida 17.23.2: local spawn, marker message observed and successful cleanup. That receipt does not establish native verification of the final revision; later cancellation paths are verified with controlled bindings. The Apple fixture previously reproduced its timeout with a fresh Swift cache and passed with another fresh cache after the budget correction.Compatibility, safety, and release
No breaking schema, dependency, migration or additional authority. Fresh final-revision native Frida verification, remote devices, Android/iOS and other native hosts remain unverified locally. Hosted CI is separate from local checks.
The previous hosted shard 3 failure is the existing
ProviderDeadline.remainingMs()contradiction (expected zero after timeout, observed one millisecond). It remains tracked in open PR #2720; this PR does not duplicate that contributor's fix.Before merge