From 786c28863f5925dd0467c1750867d3dee26fbb14 Mon Sep 17 00:00:00 2001 From: tall-1997 <278631348+tall-1997@users.noreply.github.com> Date: Tue, 13 Oct 2026 19:33:11 +0800 Subject: [PATCH] fix(sandbox): honor the caller timeout and stop cd-prefixed reads arming 1200s MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Field report: shell commands 'always hang at 1200s'. Reproduced from this session's own tool log — a grep requested with timeout=60 ran the full 1200s and died with exit 124. Three stacked root causes: [T-cd-readonly] 'cd' was missing from READ_ONLY_TOKENS (and from INTERACTIVE_TOKENS), so the most natural agent command shape — 'cd repo && grep … | head' — failed the read-only check, isHeavy escalated it to forced BATCH, and the wall clock armed at 1200s. cd/pushd/popd are pure navigation builtins; they now classify exactly like the pipeline without the cd prefix (INTERACTIVE, fail fast). [T-timeout-honored] All three execution paths (ExecutionCoordinator, PersistentShell, ShellExecutor) explicitly IGNORED the caller's timeout — each logged 'caller timeout ignored' and armed the class wall. The tool schema promised 'Timeout in seconds (default: 900)' while the implementation discarded it. The class ceiling stays the authority (the old maxOf(requested, minimum) bug must not return), but the caller may now SHORTEN the wall: armed = max(setup floor, min(requested, class wall)) — the floor survives because a minis-dev-setup-full killed by a short request leaves a broken rootfs (setupFloorSurvivesAShortRequest). The guest watchdog gets the same effective wall so the in-guest sleep and the host withTimeoutOrNull agree on when to fire. [T-quote-aware-split] Segment splitting was quote-blind in both GuestWorkloadPolicy (isObviouslyReadOnly / isDiskRelief) and BudgetClassifier.classify: a '|' inside quotes ('grep -n "a\|b"') cut a phantom segment whose garbage leading token failed the whitelists — read-only pipelines classified as heavy/NORMAL. Quoted spans are blanked before splitting, the same technique hasWriteSyntax already used. Verified: 43-point logic simulation (new behavior + every existing test case kept green, including the setup floor); new unit tests pin the reported command shape (INTERACTIVE, not heavy, 60s wall, 30s request honored), the shorten-never-extend contract, the floor, and the quote-aware split. findIsNormalAndACallerTimeoutDoesNotChangeIt is updated to the new contract (find stays NORMAL; a caller may shorten). --- .../app/sandbox/ExecutionCoordinator.kt | 18 +++++-- .../app/sandbox/GuestWorkloadPolicy.kt | 26 +++++++++- .../openminis/app/sandbox/PersistentShell.kt | 17 ++++--- .../openminis/app/sandbox/ShellExecutor.kt | 15 ++++-- .../app/sandbox/ShellTimeoutPolicy.kt | 13 +++-- .../app/sandbox/kernel/BudgetClassifier.kt | 43 +++++++++++++++-- .../com/openminis/app/tools/AgentTools.kt | 2 +- .../app/sandbox/GuestWorkloadPolicyTest.kt | 44 +++++++++++++++-- .../app/sandbox/HeavyTaskAdmissionTest.kt | 5 ++ .../app/sandbox/KernelContractTest.kt | 47 ++++++++++++++++++- 10 files changed, 197 insertions(+), 33 deletions(-) diff --git a/src/android/app/src/main/java/com/openminis/app/sandbox/ExecutionCoordinator.kt b/src/android/app/src/main/java/com/openminis/app/sandbox/ExecutionCoordinator.kt index 48fed50c5f..5c78938ef5 100644 --- a/src/android/app/src/main/java/com/openminis/app/sandbox/ExecutionCoordinator.kt +++ b/src/android/app/src/main/java/com/openminis/app/sandbox/ExecutionCoordinator.kt @@ -119,7 +119,9 @@ object ExecutionCoordinator { suspend fun execute( sessionId: String, command: String, - // Ignored. Armed timeout is BudgetClassifier.classify(command).wallMs. + // [T-timeout-honored] May SHORTEN the armed wall clock, never + // extend it: armed = min(timeout, class wall). The class ceiling + // stays the authority; the caller gets to bound its own wait. timeout: Long = 600_000L, lineCallback: ((String) -> Unit)? = null, resourceClass: SandboxResourceGate.ResourceClass = SandboxResourceGate.ResourceClass.AUTO, @@ -174,11 +176,17 @@ object ExecutionCoordinator { onWaiting = { lineCallback?.invoke(it) }, ) { val budget = BudgetClassifier.classify(command, resourceClass) - val armed = budget.wallMs - if (timeout != armed) { - Log.w( + // [T-timeout-honored] The caller may shorten the class wall but + // never extend it, and the setup floor always survives (a killed + // minis-dev-setup-full leaves a broken rootfs). The old code + // logged "caller timeout ignored" — a 60s request then rode the + // full 1200s BATCH wall while the user watched the shell "hang". + val armed = BudgetClassifier.armMs(budget, command, timeout) + if (timeout > 0L && armed < budget.wallMs) { + Log.i( TAG, - "[$sessionId] caller timeout ${timeout}ms ignored; armed ${armed}ms class=${budget.workClass}", + "[$sessionId] caller timeout ${timeout}ms shortens class " + + "${budget.workClass} wall ${budget.wallMs}ms → ${armed}ms", ) } val startTime = System.currentTimeMillis() diff --git a/src/android/app/src/main/java/com/openminis/app/sandbox/GuestWorkloadPolicy.kt b/src/android/app/src/main/java/com/openminis/app/sandbox/GuestWorkloadPolicy.kt index d02a382005..00010b8e24 100644 --- a/src/android/app/src/main/java/com/openminis/app/sandbox/GuestWorkloadPolicy.kt +++ b/src/android/app/src/main/java/com/openminis/app/sandbox/GuestWorkloadPolicy.kt @@ -117,7 +117,7 @@ internal object GuestWorkloadPolicy { */ fun isDiskRelief(command: String): Boolean { if (hasWriteSyntax(command)) return false - val segments = command.split(Regex("""&&|\|\||[;|]""")).map { it.trim() }.filter { it.isNotEmpty() } + val segments = splitCommandSegments(command) if (segments.isEmpty()) return false return segments.all { segment -> val token = commandToken(segment) ?: return@all false @@ -192,7 +192,7 @@ internal object GuestWorkloadPolicy { fun isObviouslyReadOnly(command: String): Boolean { if (command.isBlank()) return true if (hasWriteSyntax(command)) return false - val segments = command.split(Regex("""&&|\|\||[;|]""")).map { it.trim() }.filter { it.isNotEmpty() } + val segments = splitCommandSegments(command) if (segments.isEmpty()) return true return segments.all { segment -> val token = commandToken(segment) ?: return@all false @@ -203,6 +203,23 @@ internal object GuestWorkloadPolicy { } } + /** + * [T-quote-aware-split] Split a compound command into segments WITHOUT + * letting quoted spans create phantom segments. `grep -n "a\|b" file` + * contains a `|` inside quotes: the old raw split cut it into `grep -n "a\` + * and `b" file`, whose garbage leading token failed the read-only + * whitelist — so a plainly read-only pipeline classified as heavy work, + * escalated to BATCH and armed a 20-minute wall clock (the field report: + * every `cd repo && grep …` hung the full 1200s). Quoted spans are + * blanked first, exactly like [hasWriteSyntax] already does; only the + * leading token of each segment is inspected, so losing the quoted text + * costs nothing. + */ + internal fun splitCommandSegments(command: String): List { + val stripped = command.replace(Regex(""""[^"]*"|'[^']*'"""), " ") + return stripped.split(Regex("""&&|\|\||[;|]""")).map { it.trim() }.filter { it.isNotEmpty() } + } + /** * The ceiling may reduce log sampling. It must not drop the counter: * a gap that is only "too long to be a hang" is exactly the freeze the @@ -319,6 +336,10 @@ internal object GuestWorkloadPolicy { // when the disk IS the constraint — it was missing while `df` was // present. The rest are pure reporters with no write mode, added for // the same reason: they diagnose a stressed system without growing it. + // [T-cd-readonly] `cd`/`pushd`/`popd` are pure navigation builtins: they + // write nothing to disk, yet their absence made every + // `cd repo && grep …` fail the read-only check → isHeavy → forced BATCH + // → the 1200s wall clock the field report hit on every repo command. private val READ_ONLY_TOKENS = setOf( "ls", "cat", "head", "tail", "pwd", "echo", "printf", "df", "du", "stat", "file", "wc", "true", "false", "whoami", "id", "uname", "date", "env", "printenv", @@ -327,6 +348,7 @@ internal object GuestWorkloadPolicy { "nl", "od", "hexdump", "fold", "grep", "egrep", "fgrep", "tree", "free", "lsblk", "findmnt", "vmstat", "iostat", "uptime", "ps", "nproc", "arch", "lscpu", "lsof", "groups", "hostname", "tty", + "cd", "pushd", "popd", ) fun clampHostTimeout(requestedMs: Long): Long = diff --git a/src/android/app/src/main/java/com/openminis/app/sandbox/PersistentShell.kt b/src/android/app/src/main/java/com/openminis/app/sandbox/PersistentShell.kt index 0d90813f57..4ff5c160df 100644 --- a/src/android/app/src/main/java/com/openminis/app/sandbox/PersistentShell.kt +++ b/src/android/app/src/main/java/com/openminis/app/sandbox/PersistentShell.kt @@ -544,8 +544,9 @@ class PersistentShell( suspend fun executeCommand( command: String, - // Ignored. Armed timeout comes from [callerBudget] (or, when absent, - // from BudgetClassifier.classify(command)). + // [T-timeout-honored] May SHORTEN the armed wall clock, never + // extend it: armed = min(timeout, class wall). The class ceiling + // stays the authority; the caller gets to bound its own wait. timeout: Long = 600_000L, lineCallback: ((String) -> Unit)? = null, // [T-resource-class-threaded] The budget the CALLER already computed, @@ -580,9 +581,13 @@ class PersistentShell( } val budget = callerBudget ?: BudgetClassifier.classify(command) - val armed = budget.wallMs - if (timeout != armed) { - Log.w(TAG, "caller timeout ${timeout}ms ignored; armed ${armed}ms class=${budget.workClass}") + // [T-timeout-honored] The caller may shorten the class wall but never + // extend it, and the setup floor survives. The guest watchdog below + // gets the SAME effective wall so the in-guest `sleep` and the host + // withTimeoutOrNull agree on when to fire. + val armed = BudgetClassifier.armMs(budget, command, timeout) + if (timeout > 0L && armed < budget.wallMs) { + Log.i(TAG, "caller timeout ${timeout}ms shortens class ${budget.workClass} wall ${budget.wallMs}ms → ${armed}ms") } val marker = UUID.randomUUID().toString().take(8) // [T-android-ghost-cwd] Heal the shell's cwd before handing control back @@ -610,7 +615,7 @@ class PersistentShell( append("echo \"__MINIS_GO_${marker}__\"\n") // Supervisor re-arm. Not a subshell around the command, not a trap, // not a second ulimit. On expiry `kill -TERM -$$` kills the group. - append(GuardianScript.persistentCommand(command, budget.wallSeconds)) + append(GuardianScript.persistentCommand(command, (armed / 1000L).toInt().coerceAtLeast(1))) append("__minis_rc=\$?\n") append("kill -KILL \${__minis_cmd_wd:-} 2>/dev/null\n") append("if [ ! -d \"\$PWD\" ]; then cd /var/minis/workspace 2>/dev/null || cd / ; fi\n") diff --git a/src/android/app/src/main/java/com/openminis/app/sandbox/ShellExecutor.kt b/src/android/app/src/main/java/com/openminis/app/sandbox/ShellExecutor.kt index e4fc215a69..d2c9ec1286 100644 --- a/src/android/app/src/main/java/com/openminis/app/sandbox/ShellExecutor.kt +++ b/src/android/app/src/main/java/com/openminis/app/sandbox/ShellExecutor.kt @@ -40,8 +40,11 @@ object ShellExecutor { * * @param context Android context for resolving PROOT_TMP_DIR * @param command Shell command string to execute - * @param timeout Timeout in milliseconds (default 10 minutes). IGNORED — - * the armed wall clock comes from [BudgetClassifier]. + * @param timeout Timeout in milliseconds (default 10 minutes). May + * SHORTEN the armed wall clock, never extend it: armed = + * min(timeout, class wall). [T-timeout-honored] The old code ignored + * this parameter outright and logged "caller timeout ignored" — a + * 60s request then rode the full 1200s BATCH wall. * @param environment Additional environment variables * @param lineCallback Called for each line of output (on IO dispatcher) * @param resourceClass Caller's `resource_class`. [T-resource-class-threaded] @@ -62,9 +65,11 @@ object ShellExecutor { check(PRootKernel.isBooted) { "PRootKernel must be booted before executing commands" } val budget = BudgetClassifier.classify(command, resourceClass) - val armed = budget.wallMs - if (timeout != armed) { - Log.w(TAG, "caller timeout ${timeout}ms ignored; armed ${armed}ms class=${budget.workClass}") + // [T-timeout-honored] The caller may shorten the class wall but never + // extend it, and the setup floor survives. + val armed = BudgetClassifier.armMs(budget, command, timeout) + if (timeout > 0L && armed < budget.wallMs) { + Log.i(TAG, "caller timeout ${timeout}ms shortens class ${budget.workClass} wall ${budget.wallMs}ms → ${armed}ms") } val first = runOnce( context, command, armed, environment, lineCallback, noSeccomp = false, diff --git a/src/android/app/src/main/java/com/openminis/app/sandbox/ShellTimeoutPolicy.kt b/src/android/app/src/main/java/com/openminis/app/sandbox/ShellTimeoutPolicy.kt index 97e84a9c74..673919a691 100644 --- a/src/android/app/src/main/java/com/openminis/app/sandbox/ShellTimeoutPolicy.kt +++ b/src/android/app/src/main/java/com/openminis/app/sandbox/ShellTimeoutPolicy.kt @@ -65,10 +65,15 @@ object ShellTimeoutPolicy { } /** - * Class wall clock. [requestedMs] is ignored. The live callers are - * [com.openminis.app.sandbox.ExecutionCoordinator.execute] and - * [com.openminis.app.sandbox.ShellExecutor.execute], which call - * [com.openminis.app.sandbox.kernel.BudgetClassifier.classify] themselves. + * Armed wall clock. [requestedMs] may SHORTEN the class wall, never + * extend it ([T-timeout-honored]): the class ceiling stays the authority + * — the old `maxOf(requested, minimumMs)` path is how a 10-minute + * default outlived the table — while a caller that asks for 60s gets + * 60s instead of riding the full 20-minute BATCH wall. The live callers + * ([com.openminis.app.sandbox.ExecutionCoordinator.execute], + * [com.openminis.app.sandbox.ShellExecutor.execute], + * [com.openminis.app.sandbox.PersistentShell.executeCommand]) apply the + * same min() themselves. */ fun effectiveMs(command: String, requestedMs: Long): Long = com.openminis.app.sandbox.kernel.BudgetClassifier.armMs(command, requestedMs) diff --git a/src/android/app/src/main/java/com/openminis/app/sandbox/kernel/BudgetClassifier.kt b/src/android/app/src/main/java/com/openminis/app/sandbox/kernel/BudgetClassifier.kt index b943f605bc..579f118b16 100644 --- a/src/android/app/src/main/java/com/openminis/app/sandbox/kernel/BudgetClassifier.kt +++ b/src/android/app/src/main/java/com/openminis/app/sandbox/kernel/BudgetClassifier.kt @@ -31,7 +31,13 @@ object BudgetClassifier { // command. Otherwise `sleep 100 &` becomes NORMAL and inherits a // CPU limit a daemon must not have. val background = isBackground(text) - val segments = text.split(Regex("""&&|\|\||[;&|]|\$\(|`""")) + // [T-quote-aware-split] Blank quoted spans BEFORE splitting: a `|` + // inside quotes (`grep -n "a\|b" file`) used to cut a phantom + // segment whose garbage token classified as NORMAL, inflating the + // wall clock for a quick utility. Only each segment's leading token + // is inspected, so losing the quoted text costs nothing. + val stripped = text.replace(Regex(""""[^"]*"|'[^']*'"""), " ") + val segments = stripped.split(Regex("""&&|\|\||[;&|]|\$\(|`""")) .map { it.trim() } .filter { it.isNotEmpty() } val detected = if (segments.isEmpty()) interactive() @@ -54,10 +60,34 @@ object BudgetClassifier { return Regex("""(?/dev/null\"")) } + /** + * [T-cd-readonly] `cd` is pure navigation — it writes nothing. Its + * absence from READ_ONLY_TOKENS made every `cd repo && grep …` classify + * as heavy work and arm the 1200s BATCH wall (the field report). + * [T-quote-aware-split] A `|` inside quotes must not create a phantom + * segment either: `grep -n "a\|b"` stayed read-only in truth but failed + * the whitelist on its garbage split half. + */ + @Test + fun cdPrefixedAndQuotedPipePipelinesAreReadOnly() { + assertTrue(GuestWorkloadPolicy.isObviouslyReadOnly("cd /var/minis/workspace/repo && grep -rn foo src/ | head")) + assertTrue(GuestWorkloadPolicy.isObviouslyReadOnly("cd /tmp && ls -la")) + assertTrue(GuestWorkloadPolicy.isObviouslyReadOnly("grep -n \"a\\|b\" file")) + assertTrue(GuestWorkloadPolicy.isObviouslyReadOnly("echo \"a ; b\"")) + // Real writes inside quotes stay writes (hasWriteSyntax already + // stripped quotes); real pipes still segment. + assertFalse(GuestWorkloadPolicy.isObviouslyReadOnly("cd /tmp && rm -rf x")) + assertFalse(GuestWorkloadPolicy.isObviouslyReadOnly("echo \"x\" > /tmp/out")) + // The split helper itself: quoted spans blanked, no phantom halves. + // Whitespace is collapsed for the assertion — the replacement leaves + // the surrounding spaces, which is irrelevant to token extraction. + assertEquals( + listOf("grep -n file"), + GuestWorkloadPolicy.splitCommandSegments("grep -n \"a\\|b\" file") + .map { it.replace(Regex("\\s+"), " ") }, + ) + assertEquals( + listOf("cd /x", "ls"), + GuestWorkloadPolicy.splitCommandSegments("cd /x && ls"), + ) + } + /** * [T-disk-percent-arm] The refusal message says "先清理空间再重试", but * the gate used to refuse `rm` and `apt-get clean` under the same diff --git a/src/android/app/src/test/java/com/openminis/app/sandbox/HeavyTaskAdmissionTest.kt b/src/android/app/src/test/java/com/openminis/app/sandbox/HeavyTaskAdmissionTest.kt index ef8967ea4f..89f016ba2a 100644 --- a/src/android/app/src/test/java/com/openminis/app/sandbox/HeavyTaskAdmissionTest.kt +++ b/src/android/app/src/test/java/com/openminis/app/sandbox/HeavyTaskAdmissionTest.kt @@ -18,6 +18,11 @@ class HeavyTaskAdmissionTest { assertFalse(SandboxResourceGate.isHeavy("cat report.txt")) assertFalse(SandboxResourceGate.isHeavy("./gradlew --stop")) assertFalse(SandboxResourceGate.isHeavy("pkill java")) + // [T-cd-readonly] the field-report shape: a cd-prefixed read pipeline + // must not consume the heavy budget — it used to, arming the 1200s + // BATCH wall for a plain grep. + assertFalse(SandboxResourceGate.isHeavy("cd /var/minis/workspace/repo && grep -rn foo src/ | head -5")) + assertFalse(SandboxResourceGate.isHeavy("grep -n \"a\\|b\" file")) } @Test fun differentHeavyToolsShareOneBudgetAndCancellationReleasesIt() = runTest { diff --git a/src/android/app/src/test/java/com/openminis/app/sandbox/KernelContractTest.kt b/src/android/app/src/test/java/com/openminis/app/sandbox/KernelContractTest.kt index d8b8bb7128..ec9aa0543e 100644 --- a/src/android/app/src/test/java/com/openminis/app/sandbox/KernelContractTest.kt +++ b/src/android/app/src/test/java/com/openminis/app/sandbox/KernelContractTest.kt @@ -48,12 +48,55 @@ class KernelContractTest { } @Test - fun callerTimeoutIsNotAnInput() { + fun callerTimeoutShortensButNeverExtends() { + // Extension is still blocked: the class ceiling stays the authority + // (the old maxOf(requested, minimum) bug). assertEquals(60_000L, BudgetClassifier.armMs("ls /tmp", 10_000_000L)) - assertEquals(1_200_000L, BudgetClassifier.armMs("apt-get install curl", 1_000L)) + // [T-timeout-honored] Shortening is honored: a 1s request no longer + // rides the full 1200s BATCH wall while the log says "ignored". + assertEquals(1_000L, BudgetClassifier.armMs("apt-get install curl", 1_000L)) + // No request → the class wall. + assertEquals(1_200_000L, BudgetClassifier.armMs("apt-get install curl")) + // Zero/negative request → the class wall (not a zero-second kill). + assertEquals(1_200_000L, BudgetClassifier.armMs("apt-get install curl", 0L)) + // The setup floor survives a short request: a killed + // minis-dev-setup-full leaves a broken rootfs. + assertEquals( + ShellTimeoutPolicy.LONG_RUNNING_TIMEOUT_MS, + BudgetClassifier.armMs("minis-dev-setup-full", 60_000L), + ) assertEquals(WorkClass.BATCH, BudgetClassifier.classify("apt-get install curl").workClass) } + /** + * [T-cd-readonly] The exact field-report shape: `cd repo && grep …` hung + * the full 1200s BATCH wall because `cd` failed the read-only check → + // isHeavy escalated it to forced BATCH, and the caller's 60s request was ignored. + */ + @Test + fun cdPrefixedReadPipelineIsInteractiveAndNotHeavy() { + val reported = "cd /var/minis/workspace/OpenMinis-Linux && " + + "grep -rn \"fun LargeContentGuard\" --include=\"*.kt\" src/ | head" + assertEquals(WorkClass.INTERACTIVE, BudgetClassifier.classify(reported).workClass) + assertEquals(60_000L, BudgetClassifier.classify(reported).wallMs) + assertFalse(SandboxResourceGate.isHeavy(reported)) + // …and a caller asking for even less gets it. + assertEquals(30_000L, BudgetClassifier.armMs(reported, 30_000L)) + } + + /** + * [T-quote-aware-split] A `|` inside quotes must not create a phantom + * segment: `grep -n "a\|b" file` is one INTERACTIVE command, not a + * garbage-segmented NORMAL one. + */ + @Test + fun quotedPipeDoesNotCreatePhantomSegments() { + assertEquals(WorkClass.INTERACTIVE, BudgetClassifier.classify("grep -n \"a\\|b\" file").workClass) + assertEquals(WorkClass.INTERACTIVE, BudgetClassifier.classify("echo \"a && b\"").workClass) + // Real pipes still segment — the max rank wins. + assertEquals(WorkClass.NORMAL, BudgetClassifier.classify("grep -n \"a\\|b\" file | sort").workClass) + } + @Test fun unsetQueueWaitIsTwoMinutes() { assertEquals(120_000L, SessionConcurrencyManager.queueWaitMs(0))