Skip to content

compactv2: RelabelModifier ignores series set error and can write partial blocks #8933

Description

@aeron-gh

What happened:

RelabelModifier.Modify in pkg/compactv2/modifiers.go iterates the whole source
ChunkSeriesSet to build the relabeled result, but never checks set.Err() after the
loop:

for set.Next() {
    s := set.At()
    ...
    for chksIter.Next() {
        ...
    }
    if err := chksIter.Err(); err != nil {
        return errorOnlyStringIter{err}, nil
    }
    ...
}
// set.Err() is never checked here

symbolsSlice := make([]string, 0, len(symbols))
...
return index.NewStringListIter(symbolsSlice), newListChunkSeriesSet(chunkSeriesSet...)

storage.ChunkSeriesSet.Next() returns false both when iteration is complete and when it
fails. The interface documents this:

// The error that iteration has failed with.
// When an error occurs, set cannot continue to iterate.
Err() error

So if reading the source block fails partway through, the loop simply ends and Modify
returns a normal, successful looking result built only from the series it managed to read
before the failure. The caller then writes that as the new block.

Impact:

This is on the thanos tools bucket rewrite --rewrite.to-relabel-config path. A transient
or permanent read failure on the source block (corrupt chunk, object storage error) can
result in a new block being written that silently contains only part of the original data,
while the command reports success.

Why this looks like an oversight rather than intentional:

  1. The same function already checks the inner chunk iterator in two places with
    if err := chksIter.Err(); err != nil { return errorOnlyStringIter{err}, nil }.

  2. The other modifier in the same file propagates the source error correctly.
    delModifierSeriesSet.Err() forwards it:

    func (d *delModifierSeriesSet) Err() error {
        if d.err != nil {
            return d.err
        }
        return d.ChunkSeriesSet.Err()
    }
    

    RelabelModifier cannot do that because it drains the source set into maps and returns
    a brand new listChunkSeriesSet, so the source error has no path out unless it is
    checked explicitly.

  3. The dry run path in pkg/compactv2/compactor.go does check it:

    if err := set.Err(); err != nil {
        level.Error(w.logger).Log("msg", "error while iterating over set", "err", err)
    }
    

    so a dry run surfaces the failure but the actual rewrite does not.

What you expected to happen:

When the source series set fails, RelabelModifier.Modify should propagate that error
instead of returning a partial result, so the rewrite fails loudly rather than writing an
incomplete block.

Anything else:

Same class of swallowed iteration error as #8890 and #8925.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions