morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #3808] [MERGED] fix(apktool): own the version probe's process group and keep its timeout typed #5911

New issue

Closed

opened 2026-10-31 01:10:23 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:10:23 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3808
Author: @kaoru0822-kitauji
Created: 10/15/2026
Status: ✅ Merged
Merged: 10/15/2026
Merged by: @kaoru0822-kitauji

Base: main ← Head: fix/3342-apktool-owned-version-probe

📝 Commits (4)

45d1f6e fix(apktool): own the version probe's process group and keep its timeout typed
a642289 fix(apktool): carry unresolved probe cleanup through both provider entry points
eacc63b test(apktool): ignore zombies and always clean up the probe lifecycle fixture
0afdd97 test(apktool): treat a failed process inspection as unknown, not stopped

📊 Changes

5 files changed (+409 additions, -37 deletions)

View changed files

📝 src/apktool/ApktoolConfiguration.ts (+73 -21)
📝 src/apktool/ApktoolProvider.ts (+65 -4)
➕ src/apktool/ApktoolVersionProbe.lifecycle.test.ts (+107 -0)
➕ src/apktool/ApktoolVersionProbe.test.ts (+147 -0)
📝 tests/process-global/apktoolEnvironment.test.ts (+17 -12)

📄 Description

Fixes #3342
Fixes #3347
Fixes #3337

Root cause

#3342 (probe hangs past its deadline and on cancellation). inspectApktoolClient ran apktool --version through execFileOutput. execFile settles only after every inherited stdio pipe closes. A shell launcher's child that inherits those pipes (sleep 300 & wait) outlives the SIGTERM/SIGKILL sent to the shell, so the promise never settles. The 20 s deadline, the 5 s grace period and cancellation all wait on the orphaned child, which gets reparented to PID 1. The decode run already uses the owned process group (runOwnedCommand); the probe was the remaining unowned executor.

#3347 (timeout misclassified). An expired probe fell through to version_unresolved, which projects to a non-retryable capability_unavailable, instead of the typed timeout.

#3337 (decode launcher survives overflow and cancellation, and the CLI prints UNKNOWN). The owned decode run used the default expectedCommand, the launcher path. For an interpreter script, the process table shows /bin/sh <launcher> …, so owned cleanup rejected the identity ("owned launcher command identity did not match") and refused to signal. The retained owner then made close() throw an untyped AggregateError, which the CLI printed as UNKNOWN: Apktool process cleanup failed.

Change

The probe runs through runOwnedCommand, the same owner as the decode run. Its outcomes map as follows:
- cancellation → AnalysisCancelledError
- deadline → AnalysisTimeoutError("inspect_apktool_client", 20000) (provider_timeout, details.timeout_ms)
- ENOENT/EACCES → command_missing
- output limit / non-zero exit → version_unresolved (with the exit code and a stderr excerpt)
- an unresolved cleanup owner goes to an optional sink, which the provider retains and retries on close
Both the probe and decode pass expectedCommand: null. Apktool launchers are interpreter scripts by contract (docs/apktool-resource-analysis.md), so ownership is proved by the run token rather than the argv prefix. That is the same convention pcap, TShark, firmware, sqlite and pwntools use. Cleanup still requires ownership proof.
close() reports unresolved process cleanup as ProviderCleanupError (cleanup_incomplete, with the retained resources and reason), instead of an untyped AggregateError.

Validation (macOS 26 arm64, Node 26.5; controlled launchers, no real Apktool/JVM)

launcher / action	`main`	this PR
`#!/bin/sh` `sleep 300 & wait`, probe deadline	CLI still pending after the 20 s deadline, the 5 s grace and a SIGTERM at 60 s. It finished only after I killed the orphaned child by hand (198 s; child reparented to PID 1).	returns at 21 s
`#!/opt/homebrew/bin/node` launcher spawning an inheriting child, probe deadline	pending after 60 s (same hang)	21 s, `provider_timeout`, `details.timeout_ms: 20000`, child stopped
`#!/bin/sh` launcher, SIGINT at 3 s	hang	3 s, `cleanup_incomplete` naming `apktool version probe` (see note)
`#!/bin/sh` launcher printing `2.9.3`	available	available (2 s)

Note on the macOS /bin/sh case: /bin/sh and /bin/sleep are platform binaries whose environment, and therefore the REA run token, macOS withholds (platform_binary_environment_withheld). Owned cleanup therefore fails closed and keeps ownership instead of signalling unproven processes, and the CLI now says so with a typed result rather than hanging. On Linux the token is readable from /proc, so the same launcher's group is proved and stopped. I did not verify that Linux path locally; this host has no Linux runtime available. The node-launcher row exercises the full owned-cleanup path on macOS.

Output size: the probe result is unchanged on success (744 B). Failures are a single typed error envelope.

Tests:

New src/apktool/ApktoolVersionProbe.test.ts (with runOwnedCommand mocked) checks four things: the probe spawns an owned process with expectedCommand: null, timeoutMs: 20000 and a 64 KiB budget; a deadline projects to provider_timeout with timeout_ms: 20000; an unresolved cleanup owner is retained and the result projects to cleanup_incomplete; and cancellation is AnalysisCancelledError.
tests/process-global/apktoolEnvironment.test.ts now observes the owned spawn (command, hostEnvironment) instead of the removed execFileOutput call.
Apktool unit tests pass (54), as do tests/boundary/cli/apktoolCli.test.ts (including the missing-launcher message) and npm run check:fast.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3808 **Author:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Created:** 10/15/2026 **Status:** ✅ Merged **Merged:** 10/15/2026 **Merged by:** [@kaoru0822-kitauji](https://github.com/kaoru0822-kitauji) **Base:** `main` ← **Head:** `fix/3342-apktool-owned-version-probe` --- ### 📝 Commits (4) - [`45d1f6e`](https://github.com/morluto/rea/commit/45d1f6e293af962c99bf5da6a8556259f6035cad) fix(apktool): own the version probe's process group and keep its timeout typed - [`a642289`](https://github.com/morluto/rea/commit/a642289c0d295a4a3c14239b2914f14c0de82a78) fix(apktool): carry unresolved probe cleanup through both provider entry points - [`eacc63b`](https://github.com/morluto/rea/commit/eacc63b9824f41b6a00ed2b984c8dc33da20a641) test(apktool): ignore zombies and always clean up the probe lifecycle fixture - [`0afdd97`](https://github.com/morluto/rea/commit/0afdd97be6f4ae146a678c5c1e5ae52ed1ae8a44) test(apktool): treat a failed process inspection as unknown, not stopped ### 📊 Changes **5 files changed** (+409 additions, -37 deletions) <details> <summary>View changed files</summary> 📝 `src/apktool/ApktoolConfiguration.ts` (+73 -21) 📝 `src/apktool/ApktoolProvider.ts` (+65 -4) ➕ `src/apktool/ApktoolVersionProbe.lifecycle.test.ts` (+107 -0) ➕ `src/apktool/ApktoolVersionProbe.test.ts` (+147 -0) 📝 `tests/process-global/apktoolEnvironment.test.ts` (+17 -12) </details> ### 📄 Description Fixes #3342 Fixes #3347 Fixes #3337 ## Root cause **#3342 (probe hangs past its deadline and on cancellation).** `inspectApktoolClient` ran `apktool --version` through `execFileOutput`. `execFile` settles only after every inherited stdio pipe closes. A shell launcher's child that inherits those pipes (`sleep 300 & wait`) outlives the SIGTERM/SIGKILL sent to the shell, so the promise never settles. The 20 s deadline, the 5 s grace period and cancellation all wait on the orphaned child, which gets reparented to PID 1. The decode run already uses the owned process group (`runOwnedCommand`); the probe was the remaining unowned executor. **#3347 (timeout misclassified).** An expired probe fell through to `version_unresolved`, which projects to a non-retryable `capability_unavailable`, instead of the typed timeout. **#3337 (decode launcher survives overflow and cancellation, and the CLI prints `UNKNOWN`).** The owned decode run used the default `expectedCommand`, the launcher path. For an interpreter script, the process table shows `/bin/sh <launcher> …`, so owned cleanup rejected the identity ("owned launcher command identity did not match") and refused to signal. The retained owner then made `close()` throw an untyped `AggregateError`, which the CLI printed as `UNKNOWN: Apktool process cleanup failed`. ## Change - **The probe runs through `runOwnedCommand`**, the same owner as the decode run. Its outcomes map as follows: - cancellation → `AnalysisCancelledError` - deadline → `AnalysisTimeoutError("inspect_apktool_client", 20000)` (`provider_timeout`, `details.timeout_ms`) - `ENOENT`/`EACCES` → `command_missing` - output limit / non-zero exit → `version_unresolved` (with the exit code and a stderr excerpt) - an unresolved cleanup owner goes to an optional sink, which the provider retains and retries on close - **Both the probe and decode pass `expectedCommand: null`.** Apktool launchers are interpreter scripts by contract (`docs/apktool-resource-analysis.md`), so ownership is proved by the run token rather than the argv prefix. That is the same convention pcap, TShark, firmware, sqlite and pwntools use. Cleanup still requires ownership proof. - **`close()` reports unresolved process cleanup as `ProviderCleanupError`** (`cleanup_incomplete`, with the retained resources and reason), instead of an untyped `AggregateError`. ## Validation (macOS 26 arm64, Node 26.5; controlled launchers, no real Apktool/JVM) | launcher / action | `main` | this PR | |---|---|---| | `#!/bin/sh` `sleep 300 & wait`, probe deadline | **CLI still pending after the 20 s deadline, the 5 s grace and a SIGTERM at 60 s.** It finished only after I killed the orphaned child by hand (198 s; child reparented to PID 1). | returns at **21 s** | | `#!/opt/homebrew/bin/node` launcher spawning an inheriting child, probe deadline | **pending after 60 s** (same hang) | **21 s**, `provider_timeout`, `details.timeout_ms: 20000`, **child stopped** | | `#!/bin/sh` launcher, SIGINT at 3 s | hang | **3 s**, `cleanup_incomplete` naming `apktool version probe` (see note) | | `#!/bin/sh` launcher printing `2.9.3` | available | available (2 s) | Note on the macOS `/bin/sh` case: `/bin/sh` and `/bin/sleep` are platform binaries whose environment, and therefore the REA run token, macOS withholds (`platform_binary_environment_withheld`). Owned cleanup therefore fails closed and keeps ownership instead of signalling unproven processes, and the CLI now says so with a typed result rather than hanging. On Linux the token is readable from `/proc`, so the same launcher's group is proved and stopped. **I did not verify that Linux path locally**; this host has no Linux runtime available. The node-launcher row exercises the full owned-cleanup path on macOS. Output size: the probe result is unchanged on success (744 B). Failures are a single typed error envelope. **Tests:** - New `src/apktool/ApktoolVersionProbe.test.ts` (with `runOwnedCommand` mocked) checks four things: the probe spawns an owned process with `expectedCommand: null`, `timeoutMs: 20000` and a 64 KiB budget; a deadline projects to `provider_timeout` with `timeout_ms: 20000`; an unresolved cleanup owner is retained and the result projects to `cleanup_incomplete`; and cancellation is `AnalysisCancelledError`. - `tests/process-global/apktoolEnvironment.test.ts` now observes the owned spawn (`command`, `hostEnvironment`) instead of the removed `execFileOutput` call. - Apktool unit tests pass (54), as do `tests/boundary/cli/apktoolCli.test.ts` (including the missing-launcher message) and `npm run check:fast`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:10:23 +00:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

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

morluto/rea#5911

No description provided.

Rows
Columns