Compactor.Compact discards DecodeHeader errors and returns nil

[corylanou]

Compactor.Compact discards DecodeHeader errors and returns nil

Summary

Compactor.Compact uses a naked return when an input's DecodeHeader() fails. The named return retErr is never assigned, so the function returns nil, reporting success, after writing no output header.

Present in v0.5.1, unchanged since 1e93519 (2022-08-09).

The code

compactor.go:78-88:

func (c *Compactor) Compact(ctx context.Context) (retErr error) {
	if len(c.inputs) == 0 {
		return fmt.Errorf("at least one input reader required")
	}

	// Read headers from all inputs.
	for _, input := range c.inputs {
		if err := input.dec.DecodeHeader(); err != nil {
			return
		}
	}
	...

err is scoped to the if statement. retErr appears once in the function, in the signature, and is never assigned. There is no deferred assignment. So the naked return yields retErr's zero value.

Every other error path in the function returns explicitly, eight in total. This is the only naked return.

Effect

A failure reading any input header reaches the caller as success. Compact returns nil having encoded no output header.

Callers that stream Compact's output into a Decoder then fail at the decode step with decode header: EOF, because the pipe closed cleanly with nothing written.

Every distinct input failure collapses into that one symptom. All of these produce an identical decode header: EOF downstream:

• a transport error or timeout while reading an input
• an input object that is missing, empty, or shorter than a header
• structurally invalid header bytes
• an input deleted between planning and reading

We hit this in a downstream consumer that calls Compact on a restore path. The reported error pointed at header decoding, which read as data corruption. The actual cause was a provider-side failure fetching an input. The original error is unrecoverable from logs, since it is discarded at the point of failure.

Reproduction

Build a Compactor whose input decoder fails on DecodeHeader, for example an input reader that returns an error or hits EOF immediately, and call Compact. It returns nil.

Suggested fix

if err := input.dec.DecodeHeader(); err != nil {
	return err
}

Wrapping it with the input's identity would help callers more, since Compact knows which input failed and the caller does not:

if err := input.dec.DecodeHeader(); err != nil {
	return fmt.Errorf("decode header for input %d: %w", i, err)
}

If retErr has no other purpose, the named return could go too. It currently only makes the naked return look intentional.

Test coverage

compactor_test.go has three error-path subtests: ErrInputReaderRequired, ErrPageSizeMismatch, and ErrNonContiguousTXID. All three exercise errors raised after the header loop, and each of those paths returns explicitly. No test covers a failing DecodeHeader, which is the one path with the naked return.

A test asserting Compact returns non-nil when an input header fails would cover it.