When Success Doesn't Mean It Worked

Birdwatcher is the tool you reach for when Milvus metadata in etcd goes bad. It reads a broken entry, fixes it in memory, marshals it back to protobuf and saves it. I found a case where that last step could fail and the command would still say "done." The fix is PR #516, closing #515, and it's merged.

The code

This is what writeRepairedIndex looked like:

bs, err := proto.Marshal(index)
if err != nil {
    fmt.Println("failed to marshal segment info", err.Error())
}
err = cli.Save(context.Background(), p, string(bs))
return err

It prints the error and then saves anyway. If Save succeeds, the function returns nil and repair index_metric_type reports a successful repair.

It's not an empty value

The issue described the bad write as an empty value. I wanted to see for myself what bs holds after a marshal failure, so I triggered one with invalid UTF-8 in a string field:

marshal err: string field contains invalid UTF-8 bytes: [10 17 8 42 26 11 255 254 0 128 105 110 118 97 108 105 100]
unmarshal err: proto: cannot parse invalid wire-format data

It's a non-empty buffer that won't round-trip. That's worse than empty. A health check can spot a zero-byte key cheaply, but this one only shows up when something actually tries to decode it, and by then nobody connects it to the repair run that wrote it.

The error message also says "segment info" inside a function that marshals an index. That's a copy-paste leftover from writeRepairedSegment, which had the same bug. Its caller is commented out right now, but I fixed it too so the pattern doesn't come back if someone finishes that command.

The fix

Return early:

bs, err := proto.Marshal(index)
if err != nil {
    return fmt.Errorf("failed to marshal repaired index %d: %w", index.GetIndexInfo().GetIndexID(), err)
}
return cli.Save(context.Background(), p, string(bs))

The error now carries the index ID, and Save only ever sees bytes that marshaled cleanly.

Testing it without mocking protobuf

The neat part was making proto.Marshal fail for real. Go strings can hold any bytes, but protobuf string fields must be valid UTF-8, and Go's protobuf checks that at marshal time. So I put "\xff\xfe\x00\x80invalid" in IndexName.

I wrote a small fake kv.MetaKV that records every Save call. The failure test asserts an error comes back and that Save was never called. An error plus a write would still be the original bug. A second test checks that a valid index saves and unmarshals back to the same values. I did the same for segments and ran the full repair package, go vet and gofmt.

Why I care about this kind of bug

A crash is honest. This wasn't: the tool said it worked when it didn't, so you can't trust its output in the middle of an incident, and that's exactly when you need to. Logging an error isn't handling it. You're only handling it if the code stops.

Thanks to Congqi Xia for the quick review and merge.