fix(table): preserve error class in DBFind so transient DB failures don't evict live cache entries

[dev-arya23]

Problem

CachedTable.DBFind (table/cached_generic.go) wraps every FindOne error as errors.NotFound:

err := t.col.FindOne(ctx, key, &data)
if err != nil {
    return nil, errors.Wrapf(errors.NotFound, "failed to find entry with key %v: %s", key, err)
}

This flattens three very different conditions into one class — a genuinely absent row, an unreachable/timed-out MongoDB, and an undecodable document all look identical to callers.

The concrete harm is in the change-stream handler CachedTable.callback, which calls DBFind and, on any error (always IsNotFound today), does delete(t.cache, *key). A transient DB blip during a change event therefore silently evicts a live cache row, surfacing a spurious miss until the next reconciler sweep.

This was discovered while reviewing a downstream adapter that had to add a Count re-probe workaround (classifyReportedMiss) to tell a real miss from a transient failure — logic that belongs in the library.

Fix

Layered, minimal, and preserves all existing NotFound semantics.

1. errors/const.go — new Unavailable ErrCode = 6 for transient/infrastructure failures (distinct from NotFound, which means the item is genuinely absent).
2. errors/errors.go — IsUnavailable(err) predicate, matching the existing Is* helpers.
3. db/mongo.go — interpretMongoError now classifies transient failures (network error, driver timeout, server-selection timeout, context.DeadlineExceeded/Canceled) as Unavailable via a new isTransientMongoError helper. Duplicate-key → AlreadyExists and ErrNoDocuments → NotFound are unchanged; unrecognized errors still pass through untouched. FindMany/Aggregate now route cursor.All errors through interpretMongoError too, for consistency.
4. table/cached_generic.go —
   • DBFind preserves the error class from the db layer. Only genuinely unclassified errors are wrapped, and they are wrapped as Unknown (never NotFound), so a non-miss failure can never masquerade as a missing row. A genuinely absent row still returns NotFound.
   • callback evicts only on genuine NotFound (the delete scenario). On Unavailable/unknown it retains the cached value and logs, letting the reconciler converge.

Behavior preserved

• Find cache-miss (non read-through) still returns NotFound.
• DBFind for a genuinely absent row still returns NotFound.
• Existing AlreadyExists/NotFound mappings in db/mongo.go are unchanged.

Notes

• Downstream adapters can drop Count-based re-probe workarounds and rely on errors.IsUnavailable / errors.IsNotFound.
• Table/db-level tests require a live MongoDB; the error-classification change is otherwise self-contained. Happy to add integration coverage if preferred.

Flagged from downstream review (google-adapter GSA-0002).

[coderabbitai]

Important

Review available on request

• 🔍 Trigger review

Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment @coderabbitai review to review the latest changes. For a full review, comment @coderabbitai full review.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9278d74b-bd8b-4595-9298-84933730c816

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Prabhjot-Sethi]

Reviewed against a fresh checkout. Build and go vet are clean, and the table/reconciler test failures are pre-existing (they need a live MongoDB and fail identically on main) — not introduced here. Filed as #122.

The diagnosis is right and the direction is correct. Two things to address before this lands, both narrow.

---

1. Table.Find still has the identical bug

table/generic.go:303 is untouched and still flattens every FindOne error to NotFound:

err := t.col.FindOne(ctx, key, &data)
if err != nil {
    return nil, errors.Wrapf(errors.NotFound, "failed to find entry with key %v: %s", key, err)
}

Only CachedTable.DBFind was fixed. The non-cached Table is used directly in plenty of places, so downstream adapters told they can "drop Count-based re-probe workarounds" will find the workaround still necessary there. This is the same two-line change DBFind just received and belongs in this PR.

2. The FindMany/Aggregate hunks are inert (see inline comments)

The db/mongo.go classification is discarded one layer up. Details inline.

---

Scoped out, filed separately

• Test coverage — #122. The classification helpers are pure functions and cheap to cover, but the repo has no test infra to hang them on, and standing that up inside a behavior fix would bury the fix.
• Cause-chain preservation — #123. errors.Wrap flattens the cause to a string and GetErrCode uses a bare type assertion, so classification here is one-shot and unrecoverable if it misjudges. Touches every Wrap call site; own blast radius.

Minor

• context.Canceled → Unavailable reads as a mislabel. Caller-initiated cancellation (client disconnect, graceful shutdown) is not "datastore unreachable, safe to retry" — retrying a cancelled context is pointless, yet that is what the Unavailable doc comment invites. Harmless in callback (uses context.Background()), but wrong for Find(ctx) read-through.
• interpretMongoError still uses err == mongo.ErrNoDocuments rather than errors.Is — pre-existing, but this PR is already editing that function.

On cache staleness after a transient failure

Retry policy is correctly the caller's: NotifyCallback fires unconditionally, so the key reaches the pipeline and the controller decides via error-return or RequeueAfter. No argument there.

Worth a doc note though: the requeue re-runs Reconcile(key), not callback, and nothing in that loop re-runs DBFind. So a controller reading via t.Find(key) — the natural choice for a CachedTable — sees the same retained value on every retry and will not converge; only a controller calling DBFind explicitly gets fresh data. Suggest stating on callback/Find that after an Unavailable the cache may knowingly hold a stale entry, so reconcilers needing authoritative state must use DBFind. Contract clarification, no code change.

[Prabhjot-Sethi]

This is inert as written — the class it produces is discarded one layer up.

cursor.All errors now become Unavailable here, but every caller in the table layer immediately overwrites that. table/cached_generic.go:428 (DBFindMany):

err := t.col.FindMany(ctx, filter, &data, opts)
if err != nil {
    return nil, errors.Wrapf(errors.NotFound, "failed to find any entry: %s", err)
}

Same at cached_generic.go:473 (DBFindManyWithOpts), generic.go:318 (FindMany), generic.go:361 (FindManyWithOpts). So a caller of DBFindMany still cannot tell an empty result from an unreachable Mongo — the exact bug this PR fixes for DBFind, still live on the list path. Net observable effect of this hunk is currently zero.

Two options: give those four call sites the same treatment DBFind got (preserve recognized classes, wrap only unknowns as Unknown), or drop the cursor.All hunks from this PR since nothing observes them yet. The first is preferable — it is the same fix, and leaving the list path lying is what forced the downstream Count re-probe in the first place.

Separately, worth fixing while here: cursor.All returns nil on an empty result set, so those NotFound wraps never described a genuine miss to begin with. They only ever fire on real failures, which makes NotFound there wrong in every case, not just the transient one.

[dev-arya23]

Fixed — took the preferred option. All four table-layer call sites now preserve the recognized error class and only wrap unknowns as Unknown, via a shared preserveErrClass(err, format, args...) helper added in table/generic.go:

• DBFindMany (cached_generic.go), DBFindManyWithOpts (cached_generic.go)
• FindMany (generic.go), FindManyWithOpts (generic.go)

So the Unavailable this cursor.All hunk produces now actually reaches callers — an empty result is distinguishable from an unreachable Mongo on the list path. Commits: 77c77cf (generic.go) and 12675b2 (cached_generic.go).

Good catch on the second point too: since cursor.All returns nil on an empty result set, the old NotFound wraps only ever fired on real failures — NotFound there was wrong in every case, not just the transient one. That's why the fix is a straight class-preserving wrap rather than any empty-vs-error branching.

One disclosure: no Go toolchain is available in my sandbox, so I couldn't run go build/go vet locally — relying on CI for that verification.

[Prabhjot-Sethi]

Same as the FindMany hunk above — Aggregate's only table-layer consumers re-wrap any error as errors.NotFound, so the Unavailable produced here never reaches a caller. Worth resolving both together.

[dev-arya23]

Resolved together with the FindMany hunk. Aggregate's table-layer consumers (FindMany/FindManyWithOpts and the DBFindMany* variants) now go through the shared preserveErrClass helper instead of re-wrapping as errors.NotFound, so the Unavailable produced here now reaches callers. Commits 77c77cf and 12675b2.

[dev-arya23]

Thanks for the thorough review, @Prabhjot-Sethi — all points addressed. Pushed as three commits on fix/cachedtable-preserve-error-class:

1. Table.Find still flattened FindOne errors to NotFound (77c77cf)
Applied the same class-preserving fix as DBFind. Find now returns preserveErrClass(err, ...) instead of errors.Wrapf(errors.NotFound, ...).

2. cursor.All hunks were inert — all four callers re-wrapped as NotFound (77c77cf, 12675b2)
Took your preferred option: gave the four call sites (FindMany, FindManyWithOpts, DBFindMany, DBFindManyWithOpts) the same treatment DBFind got. Introduced a shared preserveErrClass(err, format, args...) helper in table/generic.go (both files are package table) so the pattern isn't duplicated across the five sites:

func preserveErrClass(err error, format string, args ...any) error {
    if errors.GetErrCode(err) != errors.Unknown {
        return err
    }
    return errors.Wrapf(errors.Unknown, "%s: %s", fmt.Sprintf(format, args...), err)
}

The Unavailable from the FindMany/Aggregate hunks now actually reaches callers, so an empty result is distinguishable from an unreachable Mongo on the list path. Also confirmed your point that cursor.All returns nil on empty — the old NotFound wraps only fired on real failures, so NotFound there was wrong in every case.

Minor: context.Canceled → Unavailable mislabel (c801aa8)
Removed context.Canceled from isTransientMongoError; kept context.DeadlineExceeded. Added a doc comment noting caller-initiated cancellation is not safe-to-retry.

Minor: interpretMongoError used == instead of errors.Is (c801aa8)
Switched err == mongo.ErrNoDocuments to base.Is(err, mongo.ErrNoDocuments).

Doc: cache-staleness contract (12675b2)
Added notes on the CachedTable type, callback, and Find: on an Unavailable refresh failure the cache retains the stale entry, and a reconciler requeue re-runs Reconcile(key) — not callback/DBFind — so reconcilers needing authoritative state must call DBFind explicitly. No code change; contract clarification only.

Left #122 (test coverage) and #123 (cause-chain preservation) as scoped out per your note.

Disclosure: my sandbox has no Go toolchain, so I could not run go build/go vet locally — relying on CI for compile/vet verification. I've left the review threads unresolved for you to confirm and close.

[Prabhjot-Sethi]

Re-reviewed the three new commits against a fresh checkout of 12675b2. All prior findings are addressed, and well:

• Table.Find no longer flattens to NotFound (table/generic.go:333).
• All four list paths route through the shared preserveErrClass helper, so the cursor.All hunks in db/mongo.go are no longer inert.
• context.Canceled excluded from the transient set — with a caveat, below.
• err == mongo.ErrNoDocuments → base.Is(...). Nice, that was only a nit.
• The staleness contract is documented on the type, callback and Find, including the "reconcilers needing authoritative state must call DBFind, not Find" consequence. That is exactly the gap I was worried about.

I also checked the blast radius of the Table.Find/FindMany class change: there are no in-core consumers of those outside tests, and the !IsNotFound(err) → panic guards in sync/lock.go and sync/provider.go behave identically before and after (a transient error was non-NotFound either way, just Unknown instead of Unavailable).

One item left, inline on db/mongo.go — the context.Canceled exclusion does not hold once the error is wrapped. Three-line fix.

---

On verification — please read, this one matters beyond this PR

One disclosure: no Go toolchain is available in my sandbox, so I couldn't run go build/go vet locally — relying on CI for that verification.

There is no CI in this repository. No .github/workflows, no GitLab/Travis/Circle config — and none in the history either. So nothing verified those commits; the fallback being relied on does not exist.

I ran it manually on 12675b2 and it is clean:

go build ./...   → OK
go vet ./...     → OK
gofmt -l .       → (no output)

The table and reconciler test failures are the pre-existing no-MongoDB panics (log.Panicf in performMongoSetup) and reproduce identically on main — not caused by this PR, and tracked in #122.

No action needed on this PR; flagging it so "CI will catch it" does not become load-bearing on a repo that has none. If a toolchain cannot be made available in the sandbox, say so plainly as unverified rather than deferring to CI, and someone with an environment can run the gates.

---

With the context.Canceled ordering fixed, this is good to merge from my side. Tests (#122) and Unwrap() (#123) remain correctly deferred.

[Prabhjot-Sethi]

The context.Canceled exclusion does not actually hold — it only catches a bare context.Canceled.

This check sits before the mongo.IsNetworkError/mongo.IsTimeout branch (line 59) and the net.Error As branch (line 63). Once the cancellation is wrapped — which is what happens when a cancelled operation tears down its connection and the driver surfaces it as a network error — one of those later branches reclassifies it as transient. So the doc comment above promises a guarantee the code does not deliver.

Probed it directly against this build:

bare context.Canceled                    isTransient=false
Canceled inside a net.Error              isTransient=true   ← contradicts the doc comment
Canceled inside a wrapped net.Error      isTransient=true   ← same
bare DeadlineExceeded (control)          isTransient=true
plain net timeout (control)              isTransient=true

(net.Error implementation wrapping context.Canceled via Unwrap, plus an fmt.Errorf("%w") layer on top.)

Fix is to make the exclusion explicit and put it ahead of the network checks:

if base.Is(err, context.DeadlineExceeded) {
    return true
}
// caller-initiated cancellation is never transient, even when the driver
// surfaces it wrapped as a network error because the connection was torn
// down. Retrying a cancelled context is pointless.
if base.Is(err, context.Canceled) {
    return false
}

Low severity on its own — it mislabels a cancelled request as retryable — but worth closing because the surrounding comment reads as a firm guarantee, and this is precisely the classification callers are now being told to branch on.

[dev-arya23]

Fixed in d72810b. Added the explicit exclusion exactly as you laid out — base.Is(err, context.Canceled) → return false, placed before the mongo.IsNetworkError/mongo.IsTimeout and net.Error As branches:

if base.Is(err, context.DeadlineExceeded) {
    return true
}
// caller-initiated cancellation is never transient, even when the driver
// surfaces it wrapped as a network error because the connection was torn
// down. This exclusion must sit ahead of the network/timeout branches
// below, otherwise a wrapped context.Canceled is reclassified as transient
// by mongo.IsNetworkError / the net.Error check and the doc comment's
// guarantee no longer holds. Retrying a cancelled context is pointless.
if base.Is(err, context.Canceled) {
    return false
}

So a context.Canceled wrapped as a network error now short-circuits to false before it can reach the transient branches — the doc comment's guarantee and the code now agree. Also dropped the stale inline "intentionally excluded (see doc comment)" note that was describing a guarantee the code wasn't actually delivering.

[dev-arya23]

Pushed the last item as d72810b.

context.Canceled ordering — made the exclusion explicit and moved it ahead of the network/timeout branches, exactly as suggested. A context.Canceled wrapped as a net.Error/network error now returns false before it can reach mongo.IsNetworkError/mongo.IsTimeout, so the doc comment's guarantee now actually holds. Detail on the thread.

---

On verification — correcting my earlier note, this one's on me.

You're right, and I need to retract what I said. My "relying on CI for compile/vet verification" line was wrong on two counts:

1. There is no CI in this repo — no .github/workflows, nothing in history. So that fallback doesn't exist; nothing was going to verify those commits. Leaning on it was misleading.
2. My sandbox has no Go toolchain (git/go both unavailable here), so I genuinely cannot run go build / go vet / gofmt locally either.

So to be plain rather than deferring to something that doesn't exist: d72810b is unverified locally. It's a three-line, syntactically trivial guard (if base.Is(err, context.Canceled) { return false }) using imports already present (base "errors", context), so my confidence it compiles is high — but that's reasoning, not a passing go build, and I won't dress it up as one. If you can run the gates on this commit as you did on 12675b2, that would close it out; otherwise I'll flag it as needing a toolchain-equipped environment before merge.

Thanks for catching the "CI will catch it" load-bearing assumption — noted for every future push on this repo. #122 (tests) and #123 (Unwrap()) remain correctly deferred. Threads left unresolved for you to confirm and close.