Refuse to serve when no HybridLog checkpoint is readable

[Ted Hart (TedHartMS)]

Fixes #2150

Problem

With --recover --fail-on-recovery-error true, a server whose HybridLog checkpoint tokens all failed to load would start anyway and serve only the retained AOF tail. In the reported reproduction that meant serving 5 keys instead of the 205 that were durably written, after printing Ready to accept connections.

Recovery.FindRecoveryInfo throws TsavoriteNoHybridLogException in two genuinely different situations: when no checkpoint was ever written, and when every token present on disk was rejected by the catch in GetClosestHybridLogCheckpointInfo (the "Skipping unreadable HybridLog checkpoint" path). SingleDatabaseManager and MultiDatabaseManager caught that exception, logged it as an informational fresh start, and continued — never consulting FailOnRecoveryError, which only the following general catch (Exception) does.

The issue also reports a second, destructive defect in cluster mode. CheckpointStore.GetLatestCheckpointEntryFromDisk returns a non-null entry with storeVersion: -1 and all-zero GUID tokens when nothing valid is found, so the if (entry == null) return; guard in PurgeAllCheckpointsExceptEntry was unreachable. It then treated those default tokens as the checkpoint to retain and deleted every real HybridLog and index token on disk — destroying the evidence, and any chance of manual recovery, before the operator could diagnose anything.

Approach

TsavoriteNoHybridLogException now reports what the scan saw: CandidateTokenCount (tokens enumerated) and UnreadableTokenCount (tokens whose metadata could not be read), populated where that catch already logs. The database managers record that outcome on the database, and a new VerifyRecoveryIsComplete runs after AOF recovery and before replay.

It refuses to serve when all of the following hold:

1. checkpoint tokens exist on disk and none of them yielded a usable checkpoint;
2. the surviving AOF cannot cover the resulting gap — it is disabled, or some physical sublog's BeginAddress is past LogAddress.FirstValidAddress;
3. FailOnRecoveryError is true.

The refusal decision deliberately lives in one place, after the AOF state is known, rather than rethrowing at the catch site. Rethrowing there would refuse the case where an unreadable checkpoint sits beside a complete AOF from the first valid address — which full replay reconstructs, so refusing would be a false positive.

Why a non-origin AOF begin address is not a trigger on its own

The issue's expected-behavior section proposes gating on "tokens unreadable and AOF begins past 64". Anchoring instead on the unreadable-token evidence avoids two false positives that the broader rule would introduce:

• A fresh directory without --aof recovers no checkpoint and has no AOF, so a rule phrased as "complete iff a checkpoint was recovered or the AOF begins at 64" would refuse a legitimate first start.
• A diskless-sync replica is the bigger one. ReplicaDisklessSync.TryReplicaDisklessRecovery initializes the replica's AOF at its primary's currentAofBeginAddress, normally far past 64, and deliberately writes no local checkpoint. "No tokens + AOF past 64" is therefore its normal, healthy steady state; gating on it would refuse a node that starts fine today.

That state is also what #2150's cluster defect used to leave behind, so it is fixed at the source rather than papered over with a startup gate.

Scoping

FastAofTruncate and AOF null-device configurations are exempt, since they discard AOF history by design. Both would otherwise satisfy the refusal conditions, so the exemption is load-bearing rather than defensive and is pinned by a test. In cluster mode only a PRIMARY refuses; a REPLICA reports the same error and continues, because a full sync from its primary reconciles it.

Cluster purge

PurgeAllCheckpointsExceptTokens now declines to purge when it has no valid token to retain — "keep nothing" must never mean "delete everything" — and a recovering node defers the orphan purge from the CheckpointStore constructor to Initialize(), so nothing is deleted before recovery has reported what it could read.

Client-visible behavior changes

Exhaustive. The trigger requires checkpoint tokens present with none readable, so nothing without that evidence changes.

1. Tokens present, none readable, AOF enabled but beginning past address 64, --fail-on-recovery-error true → now fails to start. Previously started and served partial data. This is the reported bug.
2. Tokens present, none readable, AOF disabled, --fail-on-recovery-error true → now fails to start. Previously started and served an empty database. This is beyond the issue's literal conjunction; it does not apply to a cluster replica.
3. --fail-on-recovery-error false (the default) → no functional change. The same conditions are now logged at Error instead of Information, so operators grepping for No Hybrid Log found for recovery at Information level in the unreadable case will see an Error-level message instead.
4. Cluster mode, any --fail-on-recovery-error → checkpoint artifacts are no longer deleted when no valid checkpoint can be selected. Anyone who unknowingly relied on that cleanup will now see orphan token directories retained. This is the issue's explicit requirement.

Explicitly unchanged, each pinned by a test: a fresh directory with or without --aof; an AOF-only database whose complete log begins at address 64; a valid checkpoint plus AOF tail; an unreadable checkpoint beside a complete AOF from address 64; no checkpoint tokens with an AOF beginning past 64; and --fast-aof-truncate / AOF-null-device configurations.

There is also one behavior change independent of the option: a replica recovering from a primary-supplied token now fails the sync when every checkpoint token is unreadable, rather than continuing with an incomplete store while still advertising the replication offset the primary sent it.

Interaction with #2149

#2149 reports transient false Invalid metadata length 0 results from concurrent metadata reads. That race requires two concurrent operations on one DeviceLogCommitCheckpointManager instance: the shared SemaphoreSlim is a counter with balanced per-operation accounting, so a strictly sequential caller cannot consume a foreign release.

During startup recovery there is no second reader. GarnetServer.Start() fully awaits Provider.RecoverAsync() before starting any listener or the metrics monitor, so neither a client INFO ALL nor background PopulateCheckpointInfo sampling can overlap the scan; GetClosestHybridLogCheckpointInfo iterates tokens sequentially; MultiDatabaseManager recovers databases one at a time; and the AOF uses a separate checkpoint manager instance from the store's.

A token rejected during recovery therefore reflects the on-disk state, and this change does not make a transient read failure fatal. Post-startup callers such as INFO ALL remain subject to #2149 and are unaffected here. Sequential iteration alone would not be enough — another caller can use the same checkpoint manager concurrently — so what the comment at the counting site and the UnreadableTokenCount documentation record is the startup exclusivity specifically, noting that callers such as GetLatestCheckpointTokens remain subject to #2149.

Tests

New test/standalone/Garnet.test/RespRecoveryFailOpenTests.cs covers the issue's matrix. Fault injection is pure C# — it zeroes info.dat in place, preserving length, matching the reproduction's dd conv=notrunc — so it works on Windows and Linux without shelling out.

Refusal / preservation:

• BadCheckpointCutAofRefuses — the reported bug; also asserts the artifacts survive
• BadCheckpointNoAofRefuses — unreadable checkpoint with AOF disabled
• ClusterBadCkptRefuses — cluster primary refuses and both token directories survive
• ClusterBadCkptKeepsFiles — the purge declines to run with no valid selection
• ClusterReplicaUnreadableCheckpointAbortsSyncTest — a replica aborts the sync instead of advertising the primary's offset over an empty store. The pre-existing replica test injects a general exception before the token scan and so only covers the unchanged general catch; removing the rethrow fails this new test while that one still passes

Must keep working:

• FreshStartStarts (with and without --aof), AofOnlyReplaysFully, CheckpointAndTailRecover, BadCheckpointCutAofOptOut (the FailOnRecoveryError=false opt-out), BadCheckpointFullAofStarts, NoCheckpointCutAofStarts, BadCheckpointTruncModesStart (both --fast-aof-truncate and AOF null-device)

New NoHybridLogTests in Tsavorite.test.recovery asserts the counters directly: 0/0 for an empty checkpoint location, 1/1 for a single zeroed token.

As a negative control, with the library changes reverted exactly the four refusal/preservation tests fail — one because no exception is thrown, one because the token directory was deleted — while the seven "must keep working" tests pass on both sides.

Validation

Suite	Result
Garnet.test (full)	1070 passed, 0 failed
Tsavorite.test.recovery (full)	212 passed, 0 failed
RespRecoveryFailOpenTests	13 cases passed (11 methods)
NoHybridLogTests	2 passed
Garnet.test.cluster (full)	161 passed, 4 failed
dotnet format (both solutions)	clean

Every log was grepped for error CS (0 in all runs) to rule out a stale assembly being run after a failed test-project compile.

The four cluster failures are three teardown-only Timed out DeleteDirectory (primary failure — test itself passed) and one ClusterCheckpointUpgradeFrom. The latter is pre-existing flaky rather than a regression: sampled four times on this branch and four times with the library changes reverted, it failed in two of four runs on both sides, with the same ~65 s timeout signature, and the failure also hits the Standalone case that never enters cluster mode. The teardown timeouts are the same path-length artifact described below.

Garnet.test.cluster.replication was also run, on a quiet machine, and paired with a run of the same suite in the same worktree with the library changes reverted to 277ea6c:

	this branch	libs reverted
passed / failed	65 / 43	68 / 40
Timed out DeleteDirectory (test body passed)	37	34
LASTSAVE did not advance within timeout	6	6
error CS	0	0

Same composition and magnitude on both sides, so none of it is attributable to this change. The teardown timeouts are an artifact of this worktree's path length rather than of load: DeleteDirectory(wait: true) retries forever while swallowing IOException, and PathTooLongException derives from IOException, so a path over MAX_PATH spins until the 60 s cap and reports "test itself passed". Files under this worktree already reach 265 characters against the 260 limit, because the checkout directory name is 39 characters longer than in the worktree where the same suite reports only 2 failures. #2147 is addressing extended-length test paths separately.

Merge note for #2145

Six files overlap with #2145 (tedhar-bgsave-reports-failed-checkpoint). That PR has grown considerably — 23 files at head bc528d8b9, against 6 when this note was first written — so the overlap is no longer "one comment block".

File	#2145 at bc528d8b9	This PR	Resolution
GarnetDatabase.cs	3 hunks: LastSaveSucceeded and related state	adds CheckpointRecovery field	keep both — independent fields
IDatabaseManager.cs	1 hunk: TakeCheckpointAsync returns a checkpoint status	adds VerifyRecoveryIsComplete with a default implementation	keep both — different members
StoreWrapper.cs	1 hunk: same return-type change	adds the verify call in RecoverAsync()	keep both — different methods
DatabaseManagerBase.cs	6 hunks: checkpoint outcome recording and return types	VerifyDatabaseRecoveryIsComplete, AofCanReconstructFromOrigin, and the db.CheckpointRecovery assignment	keep both — disjoint methods
SingleDatabaseManager.cs	4 hunks, of which one is comment-only on the catch (TsavoriteNoHybridLogException) block	rewrites that catch block; adds a DEBUG-only injection hook above it	keep both except the catch block — see below
MultiDatabaseManager.cs	11 hunks, of which one is comment-only on the same catch block	rewrites that catch block	keep both except the catch block — see below

The two manager files need a per-hunk resolution, not a per-file one. Most of #2145's hunks there are substantive checkpoint-status plumbing that must be kept. Only the comment-only hunk on the catch (TsavoriteNoHybridLogException) block conflicts with this PR.

On that one block, do not keep both sides. #2145's comment explains that the catch is deliberately not gated on FailOnRecoveryError because a never-checkpointed directory has no tokens to find and is therefore indistinguishable from a fresh start. After this PR that rationale holds only for the CandidateTokenCount == 0 branch — distinguishing those two situations is the entire point of #2150. Keeping both sides is the natural instinct when one side is "just a comment", but it would leave a comment asserting the opposite of the code directly beneath it, on a data-durability path. This PR's replacement comment states the post-change rationale, so take it and drop #2145's.

Note also that #2145's own rework of MultiDatabaseManager.RunPausedCheckpointsAndReleaseLocksAsync inverted the meaning of a pre-existing comment in that method; that is internal to #2145 and does not interact with this PR, but it is the same union-the-comments hazard in a second place.

Whoever lands second should also expect to need a fresh review approval: this repository sets require_last_push_approval, so a conflict-resolution commit drops an existing approval, while GitHub's native "Update branch" base-merge does not.

[Copilot]

🟡 Changes recommended

The public interface change breaks external implementations, and several newly claimed recovery branches lack targeted coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Hardens recovery so Garnet does not serve incomplete state when all checkpoint tokens are unreadable and the AOF cannot reconstruct the database.

Changes:

• Tracks unreadable checkpoint scan results and validates recovery completeness.
• Preserves cluster checkpoint artifacts when no valid checkpoint exists.
• Adds recovery tests and operational documentation.

File summaries

File	Description
website/docs/getting-started/configuration.md	Documents startup recovery behavior.
test/standalone/Garnet.test/RespRecoveryFailOpenTests.cs	Tests recovery scenarios.
libs/storage/Tsavorite/cs/test/test.recovery/NoHybridLogTests.cs	Tests token counters.
libs/storage/Tsavorite/cs/src/core/Utilities/TsavoriteException.cs	Exposes scan statistics.
libs/storage/Tsavorite/cs/src/core/Index/Recovery/Recovery.cs	Counts rejected checkpoints.
libs/server/StoreWrapper.cs	Invokes completeness verification.
libs/server/GarnetDatabase.cs	Stores recovery outcome.
libs/server/Databases/SingleDatabaseManager.cs	Handles single-database recovery.
libs/server/Databases/MultiDatabaseManager.cs	Handles multi-database recovery.
libs/server/Databases/IDatabaseManager.cs	Adds verification API.
libs/server/Databases/DatabaseManagerBase.cs	Implements completeness checks.
libs/server/Databases/CheckpointRecoveryOutcome.cs	Defines recovery state.
libs/cluster/Server/Replication/ReplicationManager.cs	Applies cluster role policy.
libs/cluster/Server/Replication/CheckpointStore.cs	Prevents destructive purging.

Review details

• Files reviewed: 14/14 changed files
• Comments generated: 5
• Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.