diff --git a/docs/FIRST-MAC-PLAYABLE-STATUS.md b/docs/FIRST-MAC-PLAYABLE-STATUS.md index 90403109..e3e7a583 100644 --- a/docs/FIRST-MAC-PLAYABLE-STATUS.md +++ b/docs/FIRST-MAC-PLAYABLE-STATUS.md @@ -2,6 +2,8 @@ 日期：2026-09-11。对应计划：`docs/FIRST-MAC-PLAYABLE-IMPLEMENTATION.md`。 +后续审查：提交 `f39a55fd` 的观察器缓存保留、运行取消清理、外部内存门禁与异常 JSON 处理问题，见 [审查与修复记录](REVIEW-f39a55fd.md)。以下环境数据是原实施时的快照。 + 本文只记录已核对的事实。**当前没有可放行的候选包，也没有任何实网证据**；所有依赖服务器、测试账号、环境负责人确认或人工签核的条目均为 BLOCKED。离线/假进程测试 PASS 只证明门禁和状态机按契约工作，不能代替联网验收。 ## 1. 结论 diff --git a/docs/REVIEW-f39a55fd.md b/docs/REVIEW-f39a55fd.md new file mode 100644 index 00000000..a440f138 --- /dev/null +++ b/docs/REVIEW-f39a55fd.md @@ -0,0 +1,38 @@ +# f39a55fd 审查与修复 + +审查对象：`f39a55fdd5f14cb2d14d67929cf45f438f572a06`。修复基于该提交，位于本地分支 `fix/playable-f39a55fd`。 + +## 已确认的问题 + +| 优先级 | 问题与触发条件 | 影响 | 修复 | +| --- | --- | --- | --- | +| P1 | `PlayableProbe._connections` 用强引用保存 `EffectRegistry`；重连或切图重建注册器 | 旧注册器及其解析缓存一直保留，观察器自身污染长跑内存结果 | 弱引用订阅源；重新绑定和统计时移除失效订阅 | +| P1 | 终止联网/地图运行器或批量入口时，没有完整的子进程清理 | 客户端可能继续联网执行操作，代理、日志处理器、采样器可能残留 | 共用进程清理模块；保留信号退出码，仅对仍持有的子 PID 执行 TERM、有界等待、KILL 和 wait；批量入口转发终止 | +| P1 | 外部 RSS 检查尚未写回时，已有干净退出的中间 `report.json`；之后直接聚合 | 缺失 `STB-MEMORY-01` 的 soak 仍可被发布汇总判 PASS | 汇总 soak PASS 时必须同时存在唯一的内存 PASS 用例和 PASS verdict | +| P2 | 客户端报告、事件行或发布清单包含合法 JSON `null` 等错误类型 | 校验器抛出异常，无法生成应有的失败报告 | 校验 JSON 根类型、事件类型和单调时间字段；错误输入写出 FAIL 报告 | + +## 复现证据与回归入口 + +- 注册器释放用例：模拟 30 次注册器替换。原实现出现 59 项断言失败；修复后 `playable_harness_test: failures=0`。 +- 取消运行用例：原联网和地图运行器均在收到 TERM 后留下客户端；回归检查终止入口后客户端已被回收，入口退出码为 143，并覆盖普通批量与 soak 入口。 +- 发布汇总用例：`node script/playable_release_test.mjs --baseline` 读取原提交的校验器，复现缺少内存检查仍然 PASS（该命令预期断言失败）。正常命令使用修复后的校验器。 +- 异常 JSON 用例：检查返回失败码的同时，要求实际生成失败报告，避免只根据异常退出码误判测试通过。 + +可重复运行： + +```bash +godot --headless --path project --script playable_harness_test.gd +bash script/playable_gate_test.sh +node script/playable_release_test.mjs +node script/playable_fault_proxy_test.mjs +node script/playable_soak_metrics_test.mjs +node script/audit_playable_maps_test.mjs +bash script/rendering_batch_test.sh +git diff --check +``` + +本轮修复后结果：门禁夹具 126 项检查通过；渲染批处理 53 项通过、`failures=0`；注册器/联网状态机、发布汇总、故障代理、内存指标、地图资源审计测试通过；Shell/Node 语法检查及 `git diff --check` 通过。 + +渲染批处理日志：`build/rendering/batch-20260911-211958-82865/`。 + +这些是离线、回环代理和假签名客户端测试。实网战斗、真实两小时长跑、最终新包与人工画面对照仍需独立验收；本次审查不会将其改成已通过。 diff --git a/project/playable_harness_test.gd b/project/playable_harness_test.gd index 9023af4a..9f197a37 100644 --- a/project/playable_harness_test.gd +++ b/project/playable_harness_test.gd @@ -203,10 +203,24 @@ func valid_config() -> Dictionary: "resolution": [1280, 720], "loops": 1, "timeout_seconds": 900, } +func test_probe_releases_old_registries() -> void: + var observer := Probe.new() + for i in range(30): + var registry := FakeFx.new() + var reference: WeakRef = weakref(registry) + observer.watch_local(registry, "fx_spawned", "fx_spawned") + observer.watch_local(registry, "fx_finished", "fx_finished") + check(observer.bound_count() == 2, "probe prunes old scene bindings") + registry = null + check(reference.get_ref() == null, "probe does not retain the old scene's effect registry") + observer.disconnect_all() + observer.free() + func run() -> void: MapCoord.set_base(Vector2.ZERO) test_config_contract() test_report_contract() + test_probe_releases_old_registries() test_happy_path() test_gameplay_not_allowed() test_absent_slot_fails_without_select() diff --git a/project/testing/playable_probe.gd b/project/testing/playable_probe.gd index 8bb16b65..97873b16 100644 --- a/project/testing/playable_probe.gd +++ b/project/testing/playable_probe.gd @@ -77,7 +77,7 @@ func snapshot() -> Dictionary: func disconnect_all() -> void: for entry in _connections: - var source: Object = entry.source + var source: Object = entry.source.get_ref() var signal_name: String = entry.signal var callback: Callable = entry.callback if source != null and is_instance_valid(source) and source.has_signal(signal_name) \ @@ -86,15 +86,24 @@ func disconnect_all() -> void: _connections.clear() func bound_count() -> int: + _prune_connections() return _connections.size() +func _prune_connections() -> void: + for i in range(_connections.size() - 1, -1, -1): + if _connections[i].source.get_ref() == null: + _connections.remove_at(i) + func _exit_tree() -> void: disconnect_all() func _bind(source: Object, signal_name: String, callback: Callable) -> void: + # Registries are RefCounted and own parsed effect caches. Observing a scene + # must not keep it alive after reconnect; discard dead bindings on rebind. + _prune_connections() if source.has_signal(signal_name) and not source.is_connected(signal_name, callback): source.connect(signal_name, callback) - _connections.append({"source": source, "signal": signal_name, "callback": callback}) + _connections.append({"source": weakref(source), "signal": signal_name, "callback": callback}) func _emit(kind: String, payload := {}, actor_vid := 0, target_vid := 0) -> void: var data: Dictionary = payload.duplicate(true) diff --git a/script/forest_map_render_test.sh b/script/forest_map_render_test.sh index 71394ed5..49705c5e 100755 --- a/script/forest_map_render_test.sh +++ b/script/forest_map_render_test.sh @@ -26,6 +26,7 @@ timeout_seconds=900 required_arch="${MT_PLAYABLE_REQUIRED_ARCH:-arm64}" kill_grace_seconds=10 suite="forest_render" +source script/playable_process_cleanup.sh usage() { cat <<'EOF' @@ -181,6 +182,7 @@ while kill -0 "$child_pid" 2>/dev/null; do done wait "$child_pid" process_code=$? +child_pid="" node script/validate_playable_report.mjs --report "$client_report" --output "$final_report" \ --log "$log" --process-code "$process_code" --timed-out "$timed_out" --run-id "$run_id" \ diff --git a/script/playable_gate_test.sh b/script/playable_gate_test.sh index ff465ed5..076f2f3b 100755 --- a/script/playable_gate_test.sh +++ b/script/playable_gate_test.sh @@ -27,6 +27,7 @@ fixture="$repo/test/playable" validate_fixture() { # report log process_code timed_out [extra...] local report="$1" log="$2" code="$3" timed="$4" shift 4 + rm -f "$tmp_dir/sealed.json" node script/validate_playable_report.mjs --report "$report" --output "$tmp_dir/sealed.json" --log "$log" \ --process-code "$code" --timed-out "$timed" --run-id fixture-run --suite playable \ --required-cases "$tmp_dir/v/required-cases.json" --build "$tmp_dir/v/build.json" \ @@ -54,6 +55,17 @@ cp "$fixture/report.blocked.json" "$tmp_dir/v/client-report.json" expect_code 2 "$(code_of validate_fixture "$tmp_dir/v/client-report.json" "$fixture/clean.log" 0 0)" "validator: BLOCKED case is not releasable" reset_fixture expect_code 1 "$(code_of validate_fixture "$tmp_dir/v/client-report.json" "$fixture/clean.log" 1 0)" "validator: nonzero child exit" +for root_value in null '[]' '"invalid"' 42; do + reset_fixture + printf '%s\n' "$root_value" >"$tmp_dir/v/client-report.json" + expect_code 1 "$(code_of validate_fixture "$tmp_dir/v/client-report.json" "$fixture/clean.log" 0 0)" "validator: invalid root $root_value" + if [ -f "$tmp_dir/sealed.json" ]; then pass "validator: invalid root sealed as FAIL"; else fail "validator: invalid root crashed before sealing"; fi +done +reset_fixture +printf 'null\n' >"$tmp_dir/v/events.jsonl" +expect_code 1 "$(code_of validate_fixture "$tmp_dir/v/client-report.json" "$fixture/clean.log" 0 0)" "validator: null event" +[ -f "$tmp_dir/sealed.json" ] && pass "validator: null event sealed as FAIL" || fail "validator: null event crashed before sealing" +reset_fixture expect_code 1 "$(code_of validate_fixture "$tmp_dir/v/client-report.json" "$fixture/clean.log" 0 1)" "validator: timed out" expect_code 1 "$(code_of validate_fixture "$tmp_dir/v/client-report.json" "$fixture/rid-warning.log" 0 0)" "validator: RID warning in log" expect_code 1 "$(code_of validate_fixture "$tmp_dir/v/client-report.json" "$tmp_dir/missing.log" 0 0)" "validator: missing log" @@ -171,6 +183,36 @@ run_gate() { # scenario output [extra args...] } report_status() { node -e 'try{process.stdout.write(require(process.argv[1]).status)}catch{process.stdout.write("MISSING")}' "$1/report.json"; } +cancel_gate() { # script output [runner args...] + local runner="$1" out="$2" owner child="" code pid_file + shift 2 + MT_FAKE_SCENARIO=cancel bash "$runner" --app "$app" --output "$out" "$@" >"$out.stdout" 2>&1 & + owner=$! + for ((attempt=0; attempt<100; attempt++)); do + pid_file="$(find "$out" -name fake-child.pid -print -quit 2>/dev/null || true)" + if [ -n "$pid_file" ] && [ -s "$pid_file" ]; then child="$(<"$pid_file")"; break; fi + kill -0 "$owner" 2>/dev/null || break + sleep 0.1 + done + kill -TERM "$owner" 2>/dev/null || true + set +e + wait "$owner" + code=$? + set -e + expect_code 143 "$code" "cancel: $runner preserves signal exit" + if [ -n "$child" ] && ! kill -0 "$child" 2>/dev/null; then + pass "cancel: $runner reaps its client" + else + fail "cancel: $runner left a client alive or never started one" + if [ -n "$child" ]; then kill -KILL "$child" 2>/dev/null || true; fi + fi +} +cancel_gate script/run_client_gate.sh "$tmp_dir/run-cancel" --config "$tmp_dir/scenario.local.json" \ + --assets "$tmp_dir/assets" --serverlist "$tmp_dir/serverlist.txt" +cancel_gate script/forest_map_render_test.sh "$tmp_dir/forest-cancel" --maps "$map_key" +cancel_gate script/playable_test.sh "$tmp_dir/batch-cancel" --config "$tmp_dir/scenario.local.json" \ + --assets "$tmp_dir/assets" --serverlist "$tmp_dir/serverlist.txt" + expect_code 0 "$(code_of run_gate pass "$tmp_dir/run-pass")" "runner: PASS" [ "$(report_status "$tmp_dir/run-pass")" = "PASS" ] && pass "runner: sealed report PASS" || fail "runner: sealed report not PASS" expect_code 0 "$(code_of run_gate pass "$tmp_dir/run-pass-2")" "runner: second independent PASS" @@ -365,6 +407,8 @@ expect_code 2 "$(code_of run_soak_gate soak-pass "$tmp_dir/soak-proxy-down" "$tm [ ! -e "$tmp_dir/soak-proxy-down/client.log" ] && pass "soak runner: client not started when the upstream is down" || fail "soak runner: client started with upstream down" # ---------- STB-01 public entry: script/playable_soak.sh ---------- +cancel_gate script/playable_soak.sh "$tmp_dir/soak-cancel" --config "$tmp_dir/soak.local.json" \ + --assets "$tmp_dir/assets" --serverlist "$tmp_dir/serverlist.txt" --allow-gameplay run_soak() { # scenario output [extra args...] local scenario="$1" out="$2" shift 2 diff --git a/script/playable_process_cleanup.sh b/script/playable_process_cleanup.sh new file mode 100644 index 00000000..0feaef27 --- /dev/null +++ b/script/playable_process_cleanup.sh @@ -0,0 +1,37 @@ +#!/usr/bin/env bash +# Sourced by runners. Only signal children whose PIDs this runner still owns. +# Clear each PID immediately after wait, so EXIT cannot signal a reused PID. +child_pid="" +redactor_pid="" +sampler_pid="" +proxy_pid="" + +cleanup_playable_processes() { + local status=$? pid round alive + trap - EXIT + trap '' HUP INT TERM + for pid in "$child_pid" "$redactor_pid" "$sampler_pid" "$proxy_pid"; do + if [ -n "$pid" ]; then kill -TERM "$pid" 2>/dev/null || true; fi + done + for ((round=0; round<${process_cleanup_grace_seconds:-10}; round++)); do + alive=0 + for pid in "$child_pid" "$redactor_pid" "$sampler_pid" "$proxy_pid"; do + if [ -n "$pid" ] && kill -0 "$pid" 2>/dev/null; then alive=1; fi + done + [ "$alive" -eq 1 ] || break + sleep 1 + done + for pid in "$child_pid" "$redactor_pid" "$sampler_pid" "$proxy_pid"; do + if [ -n "$pid" ]; then + kill -KILL "$pid" 2>/dev/null || true + wait "$pid" 2>/dev/null || true + fi + done + if [ -n "${fifo:-}" ] && [ -p "$fifo" ]; then rm -f "$fifo"; fi + return "$status" +} + +trap cleanup_playable_processes EXIT +trap 'exit 129' HUP +trap 'exit 130' INT +trap 'exit 143' TERM diff --git a/script/playable_release_test.mjs b/script/playable_release_test.mjs new file mode 100644 index 00000000..88f6cc83 --- /dev/null +++ b/script/playable_release_test.mjs @@ -0,0 +1,46 @@ +#!/usr/bin/env node +// Release aggregation must reject the intermediate report before the RSS seal. +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { spawnSync } from 'node:child_process'; +import { fileURLToPath } from 'node:url'; + +const repo = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const root = fs.mkdtempSync(path.join(os.tmpdir(), 'mt-playable-release-')); +const write = (name, value) => fs.writeFileSync(path.join(root, name), JSON.stringify(value)); +const fixture = JSON.parse(fs.readFileSync(path.join(repo, 'test/playable/report.valid.json'), 'utf8')); +const manifest = { schema_version: 1, candidate: fixture.build, required_runs: { soak: 1 }, + runs: [{ run_id: fixture.run_id, suite: 'soak', path: 'run-1' }] }; +const report = { ...fixture, suite: 'soak', exit_gate: { checked: true, process_code: 0, timed_out: false, errors: [] } }; +const baseline = process.argv.includes('--baseline') + ? spawnSync('git', ['show', 'f39a55fd:script/validate_playable_report.mjs'], { cwd: repo, encoding: 'utf8' }).stdout : null; +function check(expected, label) { + const args = baseline ? ['--input-type=module', '-'] : [path.join(repo, 'script/validate_playable_report.mjs')]; + const result = spawnSync(process.execPath, [...args, '--release-dir', root], { input: baseline, encoding: 'utf8' }); + assert.equal(result.status, expected, `${label}: ${result.stdout} ${result.stderr}`); + assert.equal(JSON.parse(fs.readFileSync(path.join(root, 'release-report.json'))).status, expected === 0 ? 'PASS' : 'FAIL'); + console.log(`ok - ${label}`); +} +try { + fs.mkdirSync(path.join(root, 'run-1')); + write('release-manifest.json', manifest); + write('run-1/report.json', report); + check(1, 'soak PASS without external memory seal is rejected'); + report.runner_cases = [{ id: 'STB-MEMORY-01', status: 'PASS' }]; + write('run-1/report.json', report); + check(1, 'memory case alone cannot replace the verdict'); + report.memory = { verdict: { status: 'FAIL' } }; + write('run-1/report.json', report); + check(1, 'contradictory memory verdict is rejected'); + report.memory.verdict.status = 'PASS'; + write('run-1/report.json', report); + check(0, 'clean exit and external memory PASS can be aggregated'); + write('run-1/report.json', null); + check(1, 'null run report produces a sealed release failure'); + write('release-manifest.json', null); + check(1, 'null manifest produces a sealed release failure'); +} finally { + fs.rmSync(root, { recursive: true, force: true }); +} diff --git a/script/playable_soak.sh b/script/playable_soak.sh index ac0c5665..49436781 100644 --- a/script/playable_soak.sh +++ b/script/playable_soak.sh @@ -22,6 +22,8 @@ set -euo pipefail set +x cd "$(dirname "$0")/.." repo="$PWD" +process_cleanup_grace_seconds=15 # Allow the child runner's 10-second cleanup to finish. +source script/playable_process_cleanup.sh app="${MT_PLAYABLE_APP:-}" config="" output="" @@ -152,8 +154,11 @@ run_one() { # name suite timeout if [ -n "$assets" ]; then args+=(--assets "$assets"); fi if [ -n "$serverlist" ]; then args+=(--serverlist "$serverlist"); fi set +e - bash script/run_client_gate.sh "${args[@]}" /dev/null; do done wait "$child_pid" process_code=$? +child_pid="" # The redactor ends at EOF; bound the wait in case a grandchild kept the FIFO open. redactor_wait=0 while kill -0 "$redactor_pid" 2>/dev/null && [ "$redactor_wait" -lt 10 ]; do @@ -267,6 +268,7 @@ if kill -0 "$redactor_pid" 2>/dev/null; then fi wait "$redactor_pid" redactor_code=$? +redactor_pid="" rm -f "$fifo" if [ -n "$sampler_pid" ]; then # The sampler stops by itself once the child is gone; bound the wait anyway. @@ -277,6 +279,7 @@ if [ -n "$sampler_pid" ]; then done if kill -0 "$sampler_pid" 2>/dev/null; then kill -TERM "$sampler_pid" 2>/dev/null; fi wait "$sampler_pid" + sampler_pid="" fi stop_proxy diff --git a/script/validate_playable_report.mjs b/script/validate_playable_report.mjs index d1d1ab05..ff02b301 100755 --- a/script/validate_playable_report.mjs +++ b/script/validate_playable_report.mjs @@ -56,10 +56,16 @@ function argsOf(argv) { } function readJson(file) { - try { return JSON.parse(fs.readFileSync(file, 'utf8')); } + try { + const value = JSON.parse(fs.readFileSync(file, 'utf8')); + if (!isObject(value)) return { __read_error: `${path.basename(file)}: root must be an object` }; + return value; + } catch (error) { return { __read_error: `${path.basename(file)}: ${error.code || 'invalid JSON'}` }; } } +function isObject(value) { return value !== null && typeof value === 'object' && !Array.isArray(value); } + function secretLiterals() { return ['MT_ACCOUNT', 'MT_PASSWORD'].map((name) => process.env[name] || '').filter((v) => v.length >= SECRET_MIN_SCAN); } @@ -160,7 +166,8 @@ function validateReport(report, options, errors) { for (const line of lines) { let event; try { event = JSON.parse(line); } catch { badEvents += 1; continue; } - if (!EVENT_KEYS.every((key) => Object.hasOwn(event, key)) || event.run_id !== options.run_id + if (!isObject(event) || !EVENT_KEYS.every((key) => Object.hasOwn(event, key)) || event.run_id !== options.run_id + || !Number.isSafeInteger(event.monotonic_us) || event.monotonic_us < 0 || typeof event.payload !== 'object' || event.payload === null || Array.isArray(event.payload) || !Number.isInteger(event.connection_epoch) || event.monotonic_us < lastUs) badEvents += 1; else lastUs = event.monotonic_us; @@ -273,6 +280,16 @@ function sealRelease(dir) { if (report.run_id !== run.run_id) errors.fail.push(`${label}: report run_id mismatch`); if (report.suite !== run.suite) errors.fail.push(`${label}: report suite mismatch`); if (report.exit_gate?.checked !== true || report.exit_gate?.process_code !== 0 || report.exit_gate?.timed_out !== false) errors.fail.push(`${label}: report not sealed by a clean exit gate`); + // The client exit gate runs before the external RSS seal. An interrupted + // or crashed memory sealer must not leave a releasable intermediate PASS. + if (run.suite === 'soak' && report.status === 'PASS') { + const memoryCases = Array.isArray(report.runner_cases) + ? report.runner_cases.filter((item) => item?.id === 'STB-MEMORY-01') : []; + if (memoryCases.length !== 1 || memoryCases[0].status !== 'PASS' + || report.memory?.verdict?.status !== 'PASS') { + errors.fail.push(`${label}: STB-MEMORY-01 external memory seal missing or not PASS`); + } + } checkBuild(report.build, manifest.candidate, `${label}: `, errors); if (report.status === 'PASS') passedBySuite[run.suite] = (passedBySuite[run.suite] || 0) + 1; else if (report.status === 'BLOCKED') { diff --git a/test/playable/fake_client.sh b/test/playable/fake_client.sh index 32c40eed..d84b8d02 100755 --- a/test/playable/fake_client.sh +++ b/test/playable/fake_client.sh @@ -34,6 +34,7 @@ fs.writeFileSync(process.env.MT_TEST_REPORT, JSON.stringify(report, null, 2)); } case "$scenario" in + cancel) exec node -e 'require("node:fs").writeFileSync(process.env.MT_TEST_OUTPUT + "/fake-child.pid", String(process.pid)); setInterval(() => {}, 1000)' ;; pass) write_report PASS; echo "PLAYABLE CLIENT: PASS" ;; soak-pass) write_report PASS; if [ "${MT_PLAYABLE_SUITE:-}" = "soak" ]; then sleep 3; fi; echo "PLAYABLE CLIENT: PASS" ;; exit3) write_report PASS; echo "PLAYABLE CLIENT: PASS"; exit 3 ;;