mirror of
https://github.com/esengine/DeepSeek-Reasonix.git
synced 2026-11-07 10:45:59 -10:00
fix(jobs): keep recovered panics from becoming successful results
Problem: legacy nil-panic recovery can publish a successful job after its invocation or cleanup panics, and error-valued panics lose their cause identity. Root cause: two boundaries treat a non-nil recover value as the only panic signal and format panic errors without wrapping the original error. Fix: share a private normal-return guard across invocation and finalization, retain typed panic causes, preserve finalizer error classification and sibling cleanup, and keep the existing invocation stack message without adding stacks to finalizers. Move invocation recovery into its cohesive owning file. Verification: compiling preceding owners fail six test/subtest events with two controls passing. Actual compiled Manager child tests cover explicit panicnil modes zero/one, invocation, cleanup and no-launch. Frozen required2/rest237 partitions close all239 packages with23656 passes/215existing skips; complete jobs/delegation race549 passes and accepted14464 three-owner composition671 passes have zero skips. Independent required command has2349passes/66existing skips. Vet, complete lint, make check and formatting pass at unchanged2664baseline. Windows/Linuxamd64 owner builds are compilation only; exact-head hosted/native and independent review remain separate gates. No Goexit, other-goroutine panic, process-scope completion or crash recovery is claimed.
This commit is contained in:
1 parent
c1ffa46df2
commit
c8fcbc138e
7 files changed
+285
-17
No files matched your search
@@ -0,0 +1,108 @@
|
||||
---
|
||||
owner: @SivanCola
|
||||
backup: @esengine
|
||||
status: active
|
||||
reviewed: 2026-10-26
|
||||
---
|
||||
|
||||
# Job panic results / 任务 panic 结果
|
||||
|
||||
## Cause and boundary / 原因与边界
|
||||
|
||||
The invocation and finalizer boundaries treated a non-nil `recover()` value as
|
||||
the only evidence of panic. With explicit `GODEBUG=panicnil=1`, a recovered
|
||||
`panic(nil)` returns nil.
|
||||
|
||||
An invocation or cleanup callback could then appear successful, and an ordinary
|
||||
job could publish `Done`. Default nil-panic handling already produced an error.
|
||||
|
||||
Both boundaries now share a private normal-return guard. A panic is a failure
|
||||
unless the callback actually returned, independent of the recovered payload.
|
||||
Error-valued panic causes remain inspectable through `errors.Is`/`errors.As`.
|
||||
Finalizer failures continue to include `ErrFinalization`.
|
||||
|
||||
任务执行与清理边界原来只根据 `recover()` 是否非 nil 判断 panic。显式设置
|
||||
`GODEBUG=panicnil=1` 时,`panic(nil)` 恢复值为 nil,可能被误报为成功并发布
|
||||
`Done`。默认模式本来就能把 nil panic 表示成错误值。
|
||||
|
||||
两个入口现在共用私有的正常返回标记,不再从恢复值推断成功。错误值 panic 的
|
||||
cause 保留在错误链中,清理失败仍包含 `ErrFinalization`。
|
||||
|
||||
## Input, sink and authority / 输入、输出与权威
|
||||
|
||||
- Inputs remain trusted host job closures and finalizers. No new callback
|
||||
source, schema, executable, permission or setting is introduced.
|
||||
- The existing Manager owns terminal status, notification, artifact failure
|
||||
marking and finalizer ordering. Model text does not decide cleanup success.
|
||||
- Finalizers still run once, attempt siblings after an error or recovered panic,
|
||||
and cache their terminal error. No-launch keeps its original cause.
|
||||
- The guard adds no goroutine, timer or OS process. It does not release an
|
||||
external process scope or make logical cancellation physical completion.
|
||||
|
||||
输入仍是既有可信宿主回调,没有新增可执行来源、权限、配置项或持久协议。
|
||||
Manager 继续负责终态与通知;资源终结顺序、一次执行及错误缓存不变。
|
||||
|
||||
## Compatibility / 兼容性
|
||||
|
||||
| Path | Result |
|
||||
| --- | --- |
|
||||
| Normal return or returned error | Existing result and error semantics |
|
||||
| Default nil panic | Still a failure; typed cause remains inspectable |
|
||||
| Explicit legacy nil panic | Failure instead of false successful completion |
|
||||
| Error-valued panic | Existing message shape, preserved error identity |
|
||||
| Finalizer panic | `ErrFinalization`; siblings attempted and error cached |
|
||||
| Proven no-launch | Existing interrupted/failed state and original cause |
|
||||
|
||||
No persisted field/version, wire payload, tool schema, provider prefix, default
|
||||
policy or caller permission changes. Corrected failures use existing statuses
|
||||
and records, which older readers already understand.
|
||||
|
||||
Invocation errors retain their existing stack-bearing message contract.
|
||||
Finalizer errors do not gain a stack trace. Trusted error values can still
|
||||
contain details under the existing reporting contract; this adds no general
|
||||
redaction or host-only diagnostic policy.
|
||||
|
||||
没有新增持久字段、版本、wire、工具定义、稳定提示前缀或默认策略。普通返回与
|
||||
返回错误的行为不变;仅纠正 legacy nil panic 的假成功,并保留错误值的身份。
|
||||
执行错误保留原有堆栈消息形式,清理错误不新增堆栈。
|
||||
|
||||
## Evidence / 验证
|
||||
|
||||
Actual Manager tests run their compiled test executable with explicit
|
||||
`panicnil=0` and `panicnil=1`. Each child exercises invocation, finalization and
|
||||
proven no-launch finalization, recording actual terminal states and errors.
|
||||
|
||||
The parent logs child cases. The child helper is a no-op in ordinary runs.
|
||||
These settings affect only owned test subprocesses, not the parent environment.
|
||||
|
||||
Separate error-valued panic cases verify the original cause for all three
|
||||
paths, sibling cleanup, cached finalization failure and retained cancellation.
|
||||
Existing success, returned-error, cancellation and finalizer-ordering tests
|
||||
remain neighbours of this change.
|
||||
|
||||
Preceding production owners compile and fail the legacy nil-panic and error
|
||||
identity assertions. These are constructed trusted-callback regressions, not
|
||||
evidence of a production incident or remote exploit.
|
||||
|
||||
真实 Manager 测试通过自身编译出的测试程序分别设置两种 nil panic 模式,覆盖执行、
|
||||
清理和未启动清理。另验证错误身份、后续清理、缓存错误及原取消原因。旧生产 owner
|
||||
可编译后真实失败;不是编译失败对照,也不宣称线上事故或远程利用。
|
||||
|
||||
## Limits / 限制
|
||||
|
||||
Recovery covers only the current invocation stack. `runtime.Goexit`, another
|
||||
goroutine's panic, OS termination and process crashes remain outside it.
|
||||
Callback return or panic does not prove descendant process-scope completion.
|
||||
|
||||
A callback may partly change a resource before panicking. Error reporting does
|
||||
not roll those effects back or prove the resource safe. This adds no journal,
|
||||
durable Begin, recovery protocol, scope receipt or budget.
|
||||
|
||||
Automatic isolation/integration and the broader concurrency/recovery plan
|
||||
remain incomplete. Author analysis does not replace independent review.
|
||||
|
||||
恢复只覆盖当前调用栈,不覆盖 `Goexit`、其他 goroutine 的 panic、OS 终止或进程
|
||||
崩溃。回调停止也不等于后代进程 scope 完成。
|
||||
|
||||
错误报告不会回滚回调的部分副作用,不证明资源安全,也不建立日志、持久化 Begin、
|
||||
恢复协议、scope receipt 或自动隔离策略。整个并发与恢复方案仍未闭合。
|
||||
@@ -7,6 +7,11 @@ reviewed: 2026-10-25
|
||||
|
||||
# Job resource finalization / 后台任务资源终结
|
||||
|
||||
See [job panic results](JOB_PANIC_RESULT_SECURITY.md) for legacy nil-panic and
|
||||
typed-cause handling at invocation and finalization boundaries.
|
||||
|
||||
执行与清理边界的 legacy nil panic 和错误身份处理,另见上述说明。
|
||||
|
||||
## Cause and owner
|
||||
|
||||
Task and fleet prepared checkpoint writer registrations before calling the job
|
||||
|
||||
@@ -50,13 +50,11 @@ func (j *Job) finalize(state Finalization) error {
|
||||
return f.err
|
||||
}
|
||||
|
||||
func callFinalizer(fn func(Finalization) error, state Finalization) (err error) {
|
||||
defer func() {
|
||||
if p := recover(); p != nil {
|
||||
err = fmt.Errorf("%w: panic: %v", ErrFinalization, p)
|
||||
}
|
||||
}()
|
||||
if err := fn(state); err != nil {
|
||||
func callFinalizer(fn func(Finalization) error, state Finalization) error {
|
||||
err := withPanicRecovery(func() error { return fn(state) }, func(value any) error {
|
||||
return panicCause("panic", value)
|
||||
})
|
||||
if err != nil {
|
||||
return fmt.Errorf("%w: %w", ErrFinalization, err)
|
||||
}
|
||||
return nil
|
||||
|
||||
@@ -19,7 +19,6 @@ import (
|
||||
"io"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"runtime/debug"
|
||||
"slices"
|
||||
"sort"
|
||||
"strings"
|
||||
@@ -424,15 +423,6 @@ func (m *Manager) StartForSession(parentSession, kind, label string, run func(ct
|
||||
return j
|
||||
}
|
||||
|
||||
func runRecovered(ctx context.Context, out io.Writer, run func(context.Context, io.Writer) (string, error)) (result string, err error) {
|
||||
defer func() {
|
||||
if r := recover(); r != nil {
|
||||
err = fmt.Errorf("internal error: panic: %v\n%s", r, debug.Stack())
|
||||
}
|
||||
}()
|
||||
return run(ctx, out)
|
||||
}
|
||||
|
||||
func (m *Manager) openArtifactLocked(parentSession, id string) (logPath, metaPath string, file *os.File, artifactErr string) {
|
||||
dir := m.artifactDirLocked(parentSession)
|
||||
if dir == "" {
|
||||
|
||||
@@ -0,0 +1,41 @@
|
||||
package jobs
|
||||
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"io"
|
||||
"runtime/debug"
|
||||
)
|
||||
|
||||
func runRecovered(ctx context.Context, out io.Writer, run func(context.Context, io.Writer) (string, error)) (result string, err error) {
|
||||
err = withPanicRecovery(func() error {
|
||||
var runErr error
|
||||
result, runErr = run(ctx, out)
|
||||
return runErr
|
||||
}, func(value any) error {
|
||||
return fmt.Errorf("%w\n%s", panicCause("internal error: panic", value), debug.Stack())
|
||||
})
|
||||
return result, err
|
||||
}
|
||||
|
||||
// withPanicRecovery distinguishes a normal return from legacy nil-valued panic.
|
||||
// It does not recover Goexit or a panic on another goroutine.
|
||||
func withPanicRecovery(call func() error, onPanic func(any) error) (err error) {
|
||||
returned := false
|
||||
defer func() {
|
||||
value := recover()
|
||||
if !returned {
|
||||
err = onPanic(value)
|
||||
}
|
||||
}()
|
||||
err = call()
|
||||
returned = true
|
||||
return err
|
||||
}
|
||||
|
||||
func panicCause(prefix string, value any) error {
|
||||
if cause, ok := value.(error); ok {
|
||||
return fmt.Errorf("%s: %w", prefix, cause)
|
||||
}
|
||||
return fmt.Errorf("%s: %v", prefix, value)
|
||||
}
|
||||
@@ -0,0 +1,120 @@
|
||||
package jobs
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"io"
|
||||
"os"
|
||||
"os/exec"
|
||||
"strings"
|
||||
"sync/atomic"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"reasonix/internal/contract/event"
|
||||
)
|
||||
|
||||
func TestJobNilPanicCannotPublishSuccess(t *testing.T) {
|
||||
exe, err := os.Executable()
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
for _, mode := range []string{"0", "1"} {
|
||||
t.Run("panicnil="+mode, func(t *testing.T) {
|
||||
cmd := exec.CommandContext(t.Context(), exe, "-test.run=^TestJobNilPanicChild$", "-test.count=1", "-test.v")
|
||||
for _, entry := range os.Environ() {
|
||||
if !strings.HasPrefix(entry, "GODEBUG=") && !strings.HasPrefix(entry, "REASONIX_JOB_NIL_PANIC=") {
|
||||
cmd.Env = append(cmd.Env, entry)
|
||||
}
|
||||
}
|
||||
cmd.Env = append(cmd.Env, "GODEBUG=panicnil="+mode, "REASONIX_JOB_NIL_PANIC=1")
|
||||
out, err := cmd.CombinedOutput()
|
||||
t.Logf("explicit panicnil=%s child:\n%s", mode, out)
|
||||
if err != nil {
|
||||
t.Errorf("compiled job probe failed: %v", err)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestJobNilPanicChild(t *testing.T) {
|
||||
if os.Getenv("REASONIX_JOB_NIL_PANIC") != "1" {
|
||||
return
|
||||
}
|
||||
for _, phase := range []string{"run", "finalizer", "uninvoked_finalizer"} {
|
||||
t.Run(phase, func(t *testing.T) {
|
||||
runJobPanicCase(t, phase, nil)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestJobPanicPreservesErrorCause(t *testing.T) {
|
||||
for _, phase := range []string{"run", "finalizer", "uninvoked_finalizer"} {
|
||||
t.Run(phase, func(t *testing.T) {
|
||||
runJobPanicCase(t, phase, errors.New("host callback cause"))
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func runJobPanicCase(t *testing.T, phase string, panicValue any) {
|
||||
t.Helper()
|
||||
manager := NewManager(event.Discard)
|
||||
defer manager.Close()
|
||||
if phase == "uninvoked_finalizer" {
|
||||
manager.Close()
|
||||
}
|
||||
recorder := &finalizationRecorder{done: make(chan error, 1)}
|
||||
manager.SetTaskRecorder(recorder)
|
||||
var siblings atomic.Int32
|
||||
job := manager.StartForSession("session", "task", "panic result", func(context.Context, io.Writer) (string, error) {
|
||||
if phase == "run" {
|
||||
panic(panicValue)
|
||||
}
|
||||
return "done", nil
|
||||
}, WithFinalizer(func(Finalization) error {
|
||||
if phase != "run" {
|
||||
panic(panicValue)
|
||||
}
|
||||
return nil
|
||||
}), WithFinalizer(func(Finalization) error {
|
||||
siblings.Add(1)
|
||||
return nil
|
||||
}))
|
||||
select {
|
||||
case <-job.done:
|
||||
case <-time.After(5 * time.Second):
|
||||
t.Fatal("job did not complete")
|
||||
}
|
||||
cause := <-recorder.done
|
||||
if cause == nil {
|
||||
t.Error("panic became a successful completion")
|
||||
}
|
||||
if value, ok := panicValue.(error); ok && !errors.Is(cause, value) {
|
||||
t.Errorf("panic error identity lost: %v", cause)
|
||||
}
|
||||
if phase != "run" && !errors.Is(cause, ErrFinalization) {
|
||||
t.Errorf("cleanup panic lost ErrFinalization: %v", cause)
|
||||
}
|
||||
job.mu.Lock()
|
||||
status := job.status
|
||||
job.mu.Unlock()
|
||||
want := Failed
|
||||
if phase == "uninvoked_finalizer" {
|
||||
want = Interrupted
|
||||
if !errors.Is(cause, context.Canceled) {
|
||||
t.Errorf("no-launch cause lost: %v", cause)
|
||||
}
|
||||
}
|
||||
if status != want {
|
||||
t.Errorf("status=%s want %s", status, want)
|
||||
}
|
||||
if siblings.Load() != 1 {
|
||||
t.Errorf("sibling cleanup count=%d", siblings.Load())
|
||||
}
|
||||
if repeated := job.finalize(Finalization{}); phase != "run" && !errors.Is(repeated, ErrFinalization) {
|
||||
t.Errorf("repeated finalization lost cleanup failure: %v", repeated)
|
||||
}
|
||||
if siblings.Load() != 1 {
|
||||
t.Error("repeated finalization repeated sibling cleanup")
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,6 @@
|
||||
## 修复
|
||||
|
||||
- Recovered job and cleanup panics cannot publish success under legacy nil-panic
|
||||
handling; error-valued panic causes remain inspectable. Refs #14025.
|
||||
- 修复 legacy nil panic 被误报为任务或清理成功的问题,并保留错误值 panic 的原因链。
|
||||
关联 #14025。
|
||||
Reference in new issue
Block a user