Skip to content

fix: SignalError.As reports success but never assigns the target - #42

Open
dualfroz wants to merge 1 commit into
oklog:mainfrom
dualfroz:dualfroz/fix-signalerror-as-assign
Open

fix: SignalError.As reports success but never assigns the target#42
dualfroz wants to merge 1 commit into
oklog:mainfrom
dualfroz:dualfroz/fix-signalerror-as-assign

Conversation

@dualfroz

@dualfroz dualfroz commented Sep 5, 2026

Copy link
Copy Markdown

Staleness note

This repository merges infrequently: the last code merge to main was
PR #27 ("Fix bug in SignalHandler", 2024-01-27), which is also the commit
that introduced the bug fixed here. A later commit (#38, 2025-06-27) only
touched .github/workflows/test.yaml and the README, no Go source. There is
no CONTRIBUTING.md, CODE_OF_CONDUCT.md, or any AI-authorship policy in the
repo or its .github/ directory as of this branch. This PR is submitted as
a draft against a fork; it has not been opened upstream.

Problem

SignalError.As(target interface{}) bool (actors.go) is the custom
implementation errors.As calls when it can't match SignalError by
concrete type directly (see the errors.As contract: "sets target to that
error value and returns true"). The current implementation only inspects
the target's type and returns true/false accordingly - it never
assigns anything to target:

// actors.go, upstream HEAD (eee6e04), lines 86-93
func (e SignalError) As(target interface{}) bool {
	switch target.(type) {
	case *SignalError, SignalError:
		return true
	default:
		return false
	}
}

So the idiomatic usage pattern

var target SignalError
ok := errors.As(err, &target) // ok == true
_ = target.Signal             // always nil - never populated

reports success (ok == true) while silently discarding the actual
os.Signal that triggered the error. Callers who trust the errors.As
contract get a zero-valued SignalError and no indication anything went
wrong. This is a regression in the maintainer's own prior fix (#27) for a
previously-documented design error in SignalError (see the type's doc
comment, which predates this change and is unrelated to the bug fixed here).

The project's own test suite did not catch this: TestSignalError in
actors_test.go calls errors.As(err, &(SignalError{})) (line 52) and
checks only the boolean return value, never the populated struct.

Root cause

actors.go:86-93 (upstream commit eee6e04, "Fix bug in SignalHandler",
#27) - SignalError.As matches on target's type but never performs the
assignment errors.As promises.

Reproduction (empirical)

Added TestSignalErrorAsAssignsTarget to actors_test.go and ran it
against the unmodified source:

errors.As returned true; target.Signal stayed nil. Confirms the bug
as described.

Fix

Replace the type-only switch with the standard errors.As
custom-implementation pattern: type-assert target to the only type that
can meaningfully be assigned through it, and assign into it.

// actors.go, after fix
func (e SignalError) As(target interface{}) bool {
	t, ok := target.(*SignalError)
	if !ok {
		return false
	}
	*t = e
	return true
}

Note on the dropped case SignalError (bare, non-pointer) branch: the
standard library's errors.As panics before calling any custom As method
unless its target argument is itself a non-nil pointer, so a bare
SignalError value can never reach this method through the public
errors.As API - and even if it did, there would be no addressable
location to write a result back into (an interface{} holding a value
type is a copy). That branch was dead code that could only return a
misleading true with no possibility of ever assigning; removing it does
not change any reachable behavior and removes a second copy of the same
class of bug this PR fixes.

The *SignalError case is the one real path: it's what errors.As calls
when a caller does var target SignalError; errors.As(err, &target), since
the concrete type returned by SignalHandler (*SignalError) is not
directly assignable to target's pointed-to type (SignalError), forcing
errors.As down the As(interface{}) bool path.

(The other common pattern, var target *SignalError; errors.As(err, &target),
already worked correctly before this fix and is untouched by it: errors.As
resolves it via a direct concrete-type match before ever calling As.)

errors.As calls SignalError.As when it cannot match the concrete type
directly, e.g. for `var target SignalError; errors.As(err, &target)`.
The previous implementation matched target's type and returned true
but never wrote to it, so callers got a successful errors.As result
with a zero-valued SignalError and no indication the signal was lost.

Add a regression test asserting the target is actually populated.
@dualfroz
dualfroz force-pushed the dualfroz/fix-signalerror-as-assign branch from 996a174 to bbbbc56 Compare September 5, 2026 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant