mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
fix(codex-probe): bash-native watchdog when no timeout binary exists; negative-cache the deterministic model 400
Stock macOS ships neither coreutils gtimeout nor timeout(1); the wrapper's fallback ran the command unwrapped, so a hung codex exec blocked the probe and the calling workflow indefinitely. The fallback now backgrounds the command, TERMs it at the deadline, and mirrors timeout(1)'s exit-124 contract — with the watchdog's stdout detached so an early finish never blocks a caller's $(...) capture on the orphaned sleep. MODEL_UNUSABLE is now negative-cached for 15 minutes (same exit-1 + hints from cache). The deterministic 400 is config-driven, so re-probing every preflight charged the affected user a 30s round trip plus real tokens per review section, forever. Editing config.toml — the fix — changes the cache signature and re-probes immediately; MODEL_PROBE_INCONCLUSIVE stays uncached. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
c10a9736b8
commit
998aeb835f
+39
-4
@@ -48,9 +48,14 @@ _gstack_codex_model_probe() {
|
||||
# Contract:
|
||||
# MODEL_OK (exit 0) — round trip succeeded; cached 1h.
|
||||
# MODEL_UNUSABLE (exit 1) — deterministic model 400; hints printed.
|
||||
# Cached 15 min: the 400 is config-driven, so re-probing every preflight
|
||||
# charged the affected user a 30s round trip + real tokens per review
|
||||
# section, forever. Editing config.toml (the fix) changes the cache
|
||||
# signature and re-probes immediately; the short TTL covers server-side
|
||||
# entitlement recovery the signature can't see.
|
||||
# MODEL_PROBE_INCONCLUSIVE (exit 0) — timeout/transient; FAIL-OPEN so a
|
||||
# slow network never wedges codex mode (the per-invocation Error
|
||||
# Handling entry still covers a later 400).
|
||||
# Handling entry still covers a later 400). Never cached.
|
||||
#
|
||||
# Only call this AFTER _gstack_codex_auth_probe passes — probing without
|
||||
# auth just measures the auth failure again.
|
||||
@@ -76,6 +81,12 @@ _gstack_codex_model_probe() {
|
||||
echo "MODEL_OK (cached)"
|
||||
return 0
|
||||
fi
|
||||
if [ "$_c_status" = "MODEL_UNUSABLE" ] && [ "$_c_sig" = "$_sig" ] && [ $((_now - _c_ts)) -lt 900 ]; then
|
||||
echo "MODEL_UNUSABLE (cached)"
|
||||
echo "HINT: the rejected model comes from the 'model = ' line in $_codex_home/config.toml."
|
||||
echo "HINT: check its [notice.model_migrations] table — Codex records the intended replacement there."
|
||||
return 1
|
||||
fi
|
||||
fi
|
||||
local _out _code
|
||||
_out=$(_gstack_codex_timeout_wrapper 30 codex exec --skip-git-repo-check -s read-only "reply OK" </dev/null 2>&1)
|
||||
@@ -87,6 +98,8 @@ _gstack_codex_model_probe() {
|
||||
return 0
|
||||
fi
|
||||
if printf '%s' "$_out" | grep -qiE 'model.{0,40}is not supported|"status":[[:space:]]*400'; then
|
||||
mkdir -p "$_gstack_home" 2>/dev/null || true
|
||||
printf 'MODEL_UNUSABLE %s %s\n' "$_now" "$_sig" > "$_cache" 2>/dev/null || true
|
||||
echo "MODEL_UNUSABLE"
|
||||
printf '%s\n' "$_out" | grep -i "model" | head -3
|
||||
echo "HINT: the rejected model comes from the 'model = ' line in $_codex_home/config.toml."
|
||||
@@ -120,8 +133,8 @@ _gstack_codex_version_check() {
|
||||
|
||||
_gstack_codex_timeout_wrapper() {
|
||||
# Resolve wrapper binary: prefer gtimeout (Homebrew coreutils on macOS),
|
||||
# fall back to timeout (Linux), else run unwrapped. Arguments: $1 is the
|
||||
# duration in seconds; rest is the command to run.
|
||||
# fall back to timeout (Linux), else a bash-native watchdog. Arguments:
|
||||
# $1 is the duration in seconds; rest is the command to run.
|
||||
local _duration="$1"
|
||||
shift
|
||||
local _to
|
||||
@@ -129,7 +142,29 @@ _gstack_codex_timeout_wrapper() {
|
||||
if [ -n "$_to" ]; then
|
||||
"$_to" "$_duration" "$@"
|
||||
else
|
||||
"$@"
|
||||
# Stock macOS ships neither coreutils gtimeout nor timeout(1); running
|
||||
# unwrapped let a hung `codex exec` block the probe — and the calling
|
||||
# workflow — indefinitely. Emulate: background the command, TERM it at
|
||||
# the deadline, mirror timeout(1)'s exit-124 contract. The watchdog's
|
||||
# stdout is detached so an early finish never blocks a caller's $(...)
|
||||
# capture on the orphaned sleep.
|
||||
"$@" &
|
||||
local _cmd_pid=$!
|
||||
( sleep "$_duration" && kill -TERM "$_cmd_pid" 2>/dev/null ) >/dev/null 2>&1 &
|
||||
local _watch_pid=$!
|
||||
local _rc
|
||||
wait "$_cmd_pid"
|
||||
_rc=$?
|
||||
if kill -0 "$_watch_pid" 2>/dev/null; then
|
||||
# Command finished before the deadline. Retiring the watchdog subshell
|
||||
# also defuses its pending kill (the `&& kill` lives in the subshell);
|
||||
# its detached sleep expires harmlessly.
|
||||
kill "$_watch_pid" 2>/dev/null
|
||||
wait "$_watch_pid" 2>/dev/null
|
||||
elif [ "$_rc" -ge 128 ]; then
|
||||
_rc=124 # killed by the watchdog: report timeout(1)'s code
|
||||
fi
|
||||
return "$_rc"
|
||||
fi
|
||||
}
|
||||
|
||||
|
||||
@@ -287,6 +287,29 @@ describe('gstack-codex-probe: timeout wrapper + namespace hygiene', () => {
|
||||
}
|
||||
});
|
||||
|
||||
test('bash-native watchdog kills a hung command at the deadline (exit 124, no timeout binary)', () => {
|
||||
// Stock macOS ships neither gtimeout nor timeout(1) — the old fallback ran
|
||||
// the command unwrapped, so a hung `codex exec` blocked the calling
|
||||
// workflow forever. Force the fallback everywhere (Linux /bin has timeout
|
||||
// via usrmerge) with a PATH holding ONLY bash and sleep, then prove a
|
||||
// 30s sleep dies at the 1s deadline with timeout(1)'s exit code. The
|
||||
// runProbe 5s spawnSync cap doubles as the "actually killed fast" bound.
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-watchdog-'));
|
||||
try {
|
||||
const which = (tool: string) =>
|
||||
spawnSync('bash', ['-c', `command -v ${tool}`]).stdout.toString().trim() || `/bin/${tool}`;
|
||||
fs.symlinkSync(which('bash'), path.join(dir, 'bash'));
|
||||
fs.symlinkSync(which('sleep'), path.join(dir, 'sleep'));
|
||||
const r = runProbe({
|
||||
snippet: `_gstack_codex_timeout_wrapper 1 sleep 30; echo "rc=$?"`,
|
||||
env: { PATH: dir },
|
||||
});
|
||||
expect(r.stdout).toContain('rc=124');
|
||||
} finally {
|
||||
fs.rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('sourcing probe does NOT set errexit/trap/IFS in caller shell (namespace hygiene)', () => {
|
||||
// Capture `set -o` output before and after sourcing. Any drift means the
|
||||
// probe polluted the caller.
|
||||
|
||||
@@ -10,9 +10,13 @@
|
||||
* - exit 0 -> MODEL_OK, result cached (1h TTL + config/auth
|
||||
* mtime signature), second call does NOT re-invoke
|
||||
* - model 400 output -> MODEL_UNUSABLE (exit 1) + config.toml HINT lines,
|
||||
* nothing cached
|
||||
* - transient failure -> MODEL_PROBE_INCONCLUSIVE, FAIL-OPEN (exit 0)
|
||||
* - config.toml mtime change invalidates a cached MODEL_OK
|
||||
* negative-cached 15 min (same exit-1 + hints from
|
||||
* cache; re-probing every preflight charged the
|
||||
* affected user 30s + real tokens per section)
|
||||
* - transient failure -> MODEL_PROBE_INCONCLUSIVE, FAIL-OPEN (exit 0),
|
||||
* never cached
|
||||
* - config.toml mtime change invalidates a cached MODEL_OK and a cached
|
||||
* MODEL_UNUSABLE (editing the pin IS the fix)
|
||||
*/
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { spawnSync } from 'child_process';
|
||||
@@ -105,7 +109,7 @@ describe('codex model probe (#2477)', () => {
|
||||
}
|
||||
});
|
||||
|
||||
test('model 400 -> MODEL_UNUSABLE with config.toml hints, exit 1, nothing cached', () => {
|
||||
test('model 400 -> MODEL_UNUSABLE with config.toml hints, exit 1, negative-cached', () => {
|
||||
const f = makeFixture();
|
||||
try {
|
||||
const r = runProbe(f, 'model400');
|
||||
@@ -115,7 +119,35 @@ describe('codex model probe (#2477)', () => {
|
||||
// Surfaces the actual rejection so the user sees WHICH model.
|
||||
expect(r.stdout).toContain('gpt-5.4');
|
||||
expect(r.status).toBe(1);
|
||||
expect(fs.existsSync(path.join(f.gstackHome, '.codex-model-probe'))).toBe(false);
|
||||
// The deterministic 400 is config-driven: re-probing every preflight
|
||||
// charged the user a 30s round trip + real tokens per review section.
|
||||
// A second run within the 15-min TTL must NOT re-invoke codex, and must
|
||||
// keep the exit-1 + hints contract so callers can't tell the difference.
|
||||
expect(invocations(f)).toBe(1);
|
||||
const second = runProbe(f, 'model400');
|
||||
expect(second.stdout).toContain('MODEL_UNUSABLE (cached)');
|
||||
expect(second.stdout).toContain('config.toml');
|
||||
expect(second.status).toBe(1);
|
||||
expect(invocations(f)).toBe(1);
|
||||
} finally {
|
||||
fs.rmSync(f.home, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
test('config.toml change re-probes past a cached MODEL_UNUSABLE (the recovery path)', () => {
|
||||
const f = makeFixture();
|
||||
try {
|
||||
runProbe(f, 'model400');
|
||||
expect(invocations(f)).toBe(1);
|
||||
// Fixing the model pin changes the mtime signature — the negative cache
|
||||
// must not outlive the config it condemned.
|
||||
fs.writeFileSync(path.join(f.codexHome, 'config.toml'), 'model = "gpt-5.5"\n');
|
||||
const future = Date.now() / 1000 + 10;
|
||||
fs.utimesSync(path.join(f.codexHome, 'config.toml'), future, future);
|
||||
const r = runProbe(f, 'ok');
|
||||
expect(r.stdout.trim()).toBe('MODEL_OK');
|
||||
expect(r.status).toBe(0);
|
||||
expect(invocations(f)).toBe(2);
|
||||
} finally {
|
||||
fs.rmSync(f.home, { recursive: true, force: true });
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user