Destructive Reads Turn Parse Failures Into Permanent Hangs

A job failed at 18:28. The user saw "in progress" until 20:30. The failure notice had arrived on time, and the worker had read it. It just did not understand it.

The bug was one line of control flow. A worker waiting on an out-of-band result treated every fetch error the same way, so "the result is not there yet" and "the result says the job failed" both meant keep waiting. That alone would have been a slow bug. What made it permanent was the read itself: the worker used a destructive read, so the moment it misread the failure notice, that notice ceased to exist.

If you have an async worker that polls a key for a result, this post is about the two things that turn a small misclassification into a multi-hour hang, and how to structure the consumer so it cannot happen.

The two states a consumer must tell apart

The setup is common. A backend submits work to a compute service and gets an ID back immediately. The compute service does its thing and writes the outcome to a shared key, then publishes a nudge. The backend subscribes to the nudge and also polls the key on a ticker, because pub/sub delivery is not guaranteed.

The consumer loop looked roughly like this:

for {
    select {
    case <-ctx.Done():
        return rescueOrError(ctx.Err())

    case msg := <-notifyCh:
        result, err := fetchResult(ctx, key)
        if err != nil {
            slog.Warn("fetch after notify failed", "error", err)
            continue          // keep waiting
        }
        return result, nil

    case <-ticker.C:
        result, err := fetchResult(ctx, key)
        if err == nil {
            return result, nil
        }
        // no result yet, keep waiting
    }
}

Read that continue again. It is reached when the compute service has explicitly told us the job failed. The consumer logs a warning and goes back to sleep.

A waiting consumer has to distinguish at least three outcomes, and this loop collapsed them into two:

• Not ready yet. The key does not exist. Keep waiting. This is the only case that should keep waiting.
• Transient infrastructure error. The key store is briefly unreachable or loading. Keep waiting, because the result was probably not consumed.
• Terminal. The result arrived and it is either an explicit failure or something we cannot interpret. Stop now.

The fix is not "handle the error case." It is to make the third category a type the loop can recognize, and to route every branch through the same predicate:

type StepError struct{ Step, Server, Message string }      // service said: failed
type ProtocolError struct{ Step, Server, Reason, Raw string } // we cannot parse it

func isTerminal(err error) bool {
    var se *StepError
    if errors.As(err, &se) { return true }
    var pe *ProtocolError
    return errors.As(err, &pe)
}

Two details matter more than they look. Return these as pointers, because errors.As against a *StepError target will silently fail to match a value type. And wrap with %w, never %v, or the chain breaks at the first wrapper and every downstream check quietly returns false.

Why a destructive read raises the stakes

The consumer read the result with an atomic read-and-delete (Redis GETDEL). That choice is defensible on its own: it prevents two workers from consuming the same result, and it keeps the key space clean without relying on TTL.

But it converts the delivery guarantee to at-most-once, and that changes what a parse failure means.

With a plain GET, a malformed payload is annoying. You log it, leave the value in place, and decide later. With GETDEL, the value is consumed by the act of looking at it. So the rule becomes absolute:

Once a destructive read returns anything other than "key missing," every failure below that point is terminal.

Invalid JSON is terminal. A missing status field is terminal. A status field that is a number instead of a string is terminal. None of these will ever resolve by waiting, because there is nothing left to wait for. The original loop treated all of them as "keep waiting," so each one was a guaranteed multi-hour hang until some outer safety timer fired.

There is a real tradeoff here. If you keep the destructive read, you accept that a consumer crash between read and commit loses the result. If you switch to GET plus a delete after your own state is committed, you become idempotent but must handle duplicate consumption. Either is fine. What is not fine is using a destructive read while writing consumer code that assumes it can look again.

Positive allowlists, not negative checks

While adding the terminal types, I found a second bug hiding in the same function, older than the one I was fixing:

var status string
if s, ok := raw["status"]; ok {
    json.Unmarshal(s, &status)   // error ignored
}
if status == "error" {
    return nil, errors.New("service reported failure")
}
return &Result{Status: "done", Content: raw}, nil   // everything else

This checks for the one bad value and treats the entire remaining universe as success. Walk through what that means:

Three rows there are silent data corruption. An empty object was reported as a successful job, and whatever partial content came with it got written into the record as a result. Nobody had noticed because the producer happened to always send a well formed payload. The contract was being honored by luck, not enforcement.

Invert it. Name the values you accept, and make everything else an error you can see:

switch status {
case "done":
    return &Result{Status: "done", Content: raw}, nil
case "error":
    return nil, &StepError{Step: step, Message: sanitize(msg)}
default:
    return nil, &ProtocolError{Step: step, Reason: "unknown_status:" + status}
}

The same discipline applies to the unmarshal error you were about to ignore. If status is present but not a string, that is a producer contract violation, and you want it loud rather than converted into a fake success.

Retry budgets need their own key scope

Once failures surface immediately, you probably want a retry. A transient out-of-memory error on the compute side is worth one more attempt, and a bad input is not, but you often cannot tell them apart from the payload.

My first attempt reused an existing array of "servers we already tried" as the budget counter. Two reviewers killed it in sequence, and the second reason is the interesting one.

The first problem was an off-by-one. Appending the current server and then checking len(servers) >= 1 fails the job on the first attempt, so the retry never happens. Obvious once stated.

The second problem is what happens when you fix the first. Raise the cap to 2, and the array stops growing whenever the same worker picks the job back up, because the append was guarded against duplicates. No growth means no termination condition. The job requeues forever. Worse, the usual escape hatches were absent: this path did not increment the generic retry counter, and the job completed cleanly each cycle so the stale-job monitor never saw it as stuck.

The reason that same structure had been safe for a neighboring error class was the circuit breaker. Submit failures counted toward it, so after a few the breaker opened and the worker went idle, which broke the loop by accident. The change I was making explicitly removed that accident.

So the budget needs to be a monotonic counter, and its key scope needs thought:

// key: retry:{job_id}:{step}
const incrWithTTL = `
local v = redis.call("INCR", KEYS[1])
if v == 1 then redis.call("EXPIRE", KEYS[1], ARGV[1]) end
return v`

Three things I got wrong before settling on this:

• Scope by job, not by entity. Keying on the parent entity plus step name looks natural, but if a user can trigger several independent jobs against the same entity and step, they share one budget and starve each other. A per-enqueue ID keeps them separate, and internal requeues preserve it.
• Separate sub-steps. A pre-processing step that runs inside the same job record must not spend the main step's budget. Use the actual compute step name, not the job's declared step.
• Make INCR and EXPIRE atomic. INCR creates the key without a TTL. If the process dies before the EXPIRE, you have an immortal counter, and every future failure for that ID is instantly fatal with no natural recovery. One Lua script removes the window.

Set the TTL from the longest possible gap between attempts, not from the average. Mine ended up at twice the hard ceiling of a single wait. My first two guesses (one hour, then six) were both shorter than a single attempt could run, which would have let the counter expire mid-retry and reset the budget. That is the infinite requeue again, wearing a different hat.

Do not mix domain failures into your circuit breaker

The last piece is where these failures should count against the producer's health.

A circuit breaker exists to answer one question: is this endpoint sick? An explicit failure response is evidence that it is healthy. It accepted the request, did work, produced a structured answer, and delivered it through the result channel. The job failed. The server did not.

So explicit failures should not count as breaker failures. Otherwise a handful of bad inputs opens the breaker and takes a perfectly good node out of rotation, which pushes unrelated work into requeues.

Protocol errors are the opposite. A payload you cannot parse suggests version skew, a serialization bug, or a half finished deploy. That is a node problem, and it should count.

There is a subtlety that cost me a follow-up commit. If you simply skip the failure call, you may also skip the success call, and if your breaker tracks an in-flight probe slot during half-open state, that slot never gets released. The worker then sits behind a gate that never opens until the process restarts. I needed an explicit release:

func (cb *Breaker) ReleaseProbeAsSuccess() {
    cb.mu.Lock()
    defer cb.mu.Unlock()
    if cb.state == "half-open" {
        cb.state = "closed"     // the server answered, it is reachable
        cb.failures = 0
    }
    cb.inFlight = 0
}

Two guards on that method. Do not call it from a request that started while the breaker was closed, because breakers are usually shared across goroutines and you would be closing someone else's probe without a real probe having succeeded. And never call it on context cancellation, since a shutdown proves nothing about the remote host.

What is still not covered

Being honest about the edges, because they are the next bug:

• Concurrent consumers of one job. If two workers somehow process the same job, one may requeue at count 1 while the other gives up at count 2, and an already failed job goes back to the producer. Guarding that needs a compare-and-set on job state at requeue time, not just a counter.
• Retry without diversity. The counter caps total attempts but does not steer the retry to a different node. If the failure is node local, retrying blind wastes the budget.
• The producer contract itself. All of this is the consumer defending against ambiguity. The real fix is a payload that says whether a failure is retryable, so the consumer does not have to guess.

After deploying, the same failing job took two attempts across two nodes and settled in 2 minutes 19 seconds, with the producer's own error text preserved in the record. The previous path took over six hours and overwrote the cause with a generic timeout label.

FAQ

Is a destructive read a mistake?

No. It is a reasonable way to guarantee single consumption. It just moves responsibility: once you consume, parsing is your last chance, so every post-read failure has to be terminal. The mistake is pairing a destructive read with consumer code that assumes a second look is possible.

Why not simply retry the fetch on a parse error?

Because with a destructive read there is nothing to fetch. The retry sees an empty key, concludes "not ready yet," and waits. That is exactly how a parse error becomes a multi-hour hang.

How many attempts should a terminal failure get?

One retry was right for me because a meaningful share of failures were transient resource exhaustion on the compute side, and the worst case cost was one extra run of the step. If your producer reports whether an error is retryable, use that instead of a flat number. A flat number is a stand-in for information you do not have.

Should protocol errors and explicit failures both be retried?

I retry both, since the cheap uniform policy was easier to reason about than a taxonomy I could not verify. They differ in how they affect the breaker, not in how many attempts they get.

How do I keep the producer's error text safe to store?

Normalize before you truncate, not after. Fix invalid UTF-8, strip control characters and bidirectional overrides, redact internal paths and URLs, then cut to length. Cutting first leaves half of a path or URL that no longer matches your redaction patterns. And keep the raw unparsed payload out of any field that reaches a user, because it is the least trustworthy string in the whole system.