Do not report a failed checkpoint as a successful save by TedHartMS · Pull Request #2145 · microsoft/garnet

Ted Hart (TedHartMS) · 2026-09-17T03:03:21Z

Problem

A checkpoint that fails is reported to clients as a successful save.

DatabaseManagerBase.TakeCheckpointAsync returns long? and uses null for two different outcomes — "this was an incremental checkpoint, so there is no tail address to record" and "the checkpoint threw". Every caller then advances LastSaveTime unconditionally. BGSAVE followed by the documented LASTSAVE-polling pattern therefore reports durability for data that was never written. The loss only surfaces as an empty store after the next restart, because recovery treats a missing hybrid log as a fresh start.

Two further paths reported the same false success:

InitiateCheckpointAsync captured the result of StateMachineDriver.RunAsync and never read it. RunAsync returns false when another state machine is already running (reachable in practice via IndexAutoGrowTask), meaning no checkpoint was taken — yet the code still truncated the AOF and registered a cluster checkpoint entry, discarding data the checkpoint did not cover.
The failure was logged through the optional logger parameter rather than the manager's own Logger, so it frequently left no trace at all.

Beyond LASTSAVE, this also affects TakeOnDemandCheckpointAsync, which ReplicaSyncSession uses to decide that a primary's checkpoint is fresh enough to seed a replica, and TaskCheckpointBasedOnAofSizeLimitAsync, which truncates the AOF after "checkpointing".

Fix

TakeCheckpointAsync now returns a CheckpointResult carrying an explicit IsSuccessful alongside the nullable tail address, and a single RecordCheckpointOutcome advances LastSaveTime only on success.

Failures are returned rather than thrown, because MultiDatabaseManager fans out over databases with Task.WhenAll — letting exceptions escape would abort sibling databases' bookkeeping — and because a background checkpoint must not tear down the server.

IDatabaseManager.TakeCheckpointAsync now returns a CheckpointStatus (Success / AlreadyInProgress / Failed), since its bool could not distinguish "another checkpoint is in progress" from "the checkpoint failed".

An aborted checkpoint left the store permanently broken

The Phase.REST handling that releases checkpoint state is never entered when a state machine aborts, so the next checkpoint failed its Debug.Assert(_indexCheckpoint.IsDefault) in PREPARE — one transient failure disabled checkpointing until restart. CompleteCheckpointAsync already performs this cleanup on failure, but it is not on the RunAsync path Garnet uses.

Added an OnAbort hook to IStateMachineTask (a default no-op interface method, so no existing implementor changes), invoked by the driver when it fast-forwards an aborted state machine. It releases three things that REST would have:

Tsavorite's checkpoint state — _indexCheckpoint.Reset() and _hybridLogCheckpoint.Dispose(), which also close any snapshot devices and flush buffers already created.
The pending checkpoint task. REST completes store.checkpointTcs and publishes a fresh one; leaving it pending hangs ClientSession.WaitForCommitAsync, which re-reads store.CheckpointTask in its loop. The replacement source is published before the old one is faulted, so a woken continuation picks up the next checkpoint's source rather than the one it just watched fail. OnAbort receives the aborting exception for this.
The range-index checkpoint barrier. CheckpointTrigger.VersionShift sets a barrier that only FlushBegin clears, and range index operations spin-wait on it, so a checkpoint aborting between IN_PROGRESS and WAIT_FLUSH blocked them indefinitely. A new CheckpointTrigger.CheckpointFailed is raised from the abort path and clears it. It deliberately does not run the vector manager's completion work, which reclaims deletions that are only safe to release once a checkpoint has made them recoverable. The trigger can fire without VersionShift having been reached, so handling is idempotent.

Client-visible changes

Command	Before	After
`SAVE`	`+OK` even when the checkpoint threw	Error reply when the checkpoint did not complete. Matches Redis, which reports a failed `SAVE` as an error.
`SAVE` across all databases	`+OK` even when a database was skipped because its own checkpoint was already running	`ERR checkpoint already in progress`. The request never attempted that database and cannot vouch for it. Per-database `SAVE <dbid>` is unchanged — that path already returned this error.
`BGSAVE`	`+Background saving started`	Unchanged — the reply is sent before the checkpoint runs
`LASTSAVE`	Advanced on failure	Advances only for a checkpoint that completed, so it can be polled after `BGSAVE` to confirm persistence as the docs describe
`INFO PERSISTENCE`	No save status	New `rdb_last_bgsave_status` (`ok`/`err`), which is how a background save's failure is reported in Redis

The PERSISTENCE section was previously suppressed entirely unless the append-only file was enabled, even though GetDatabasePersistenceStats already falls back to "N/A" for every AOF field in that case. It is now always emitted, so a checkpoint failure is observable without AOF — which is the default configuration. Note this widens a public embedded API: MetricsApi.GetInfoMetrics filters nulls, so PERSISTENCE now appears in that enumeration on a non-AOF server where it previously did not.

Not changed

RecoverCheckpointAsync swallows TsavoriteNoHybridLogException and deliberately does not honor FailOnRecoveryError, so that a --recover start against a never-checkpointed directory comes up. That is intended and is left alone; a comment records the intent and the consequence — when no checkpoint was ever written there are no tokens to find, so this is indistinguishable from a fresh start, which is why the save path must never report a failed checkpoint as a successful save.

That catch is also reached in a case this PR does not address: checkpoint tokens exist but none of them can be read, which is not a fresh start and is distinguishable at recovery time. #2150 reports that separately, with the fail-open startup it causes under --fail-on-recovery-error true and a cluster-mode purge that deletes the unreadable artifacts. This PR is the write-side fix — do not report a checkpoint as durable unless it completed — and deliberately leaves the read-side recovery policy to that issue.

A general BGSAVE that skips a database whose own checkpoint is already in flight still replies +Background saving started. That is the documented BGSAVE contract, is asserted by MultiDatabaseSaveInProgressTest, and is left alone — the reply is sent before the aggregate outcome exists. The equivalent foreground SAVE no longer reports success in that situation; see the client-visible table above. In both cases the skipped database's own in-flight checkpoint records its own LASTSAVE and rdb_last_bgsave_status, so the per-database durability signal was always truthful; what was untruthful was only the aggregate SAVE reply.

Tests

CheckpointFailureTests injects failures two ways, because neither alone is sufficient:

Device failure, through a factory that throws when creating store checkpoint devices. SnapshotLogDevicesOnly lets the index device through and fails only snapshot.dat / snapshot.obj.dat, which are requested at WAIT_FLUSH with both checkpoints live — the shape of the real failure that motivated this work, where the device layer rejected an over-long path. (An earlier revision failed every checkpoint device, which aborted in the index task before the hybrid-log checkpoint existed and so never exercised its cleanup.)
Phase abort, through an IStateMachineCallback that throws once on entry to a chosen phase. This is device-agnostic and reaches abort points no device failure can — notably IN_PROGRESS, which is the only window where the range-index barrier is set but not yet cleared.

Neither depends on path length, file permissions or platform, and neither needs a product or TestUtils change — GetGarnetServerOptions already exposes DeviceFactoryCreator.

Coverage:

a failed BGSAVE does not advance LASTSAVE, writes no checkpoint files, and reports rdb_last_bgsave_status:err
a failed SAVE returns an error and does not advance LASTSAVE
an aborted checkpoint leaks nothing — snapshot devices, object log flush buffers and the index hash table device are all null and the index checkpoint is default — asserted directly, since a leak would still let a later checkpoint succeed while leaking a handle each time
a failed checkpoint faults the pending checkpoint task and publishes a new, incomplete one
a checkpoint blocked by another state machine fails without advancing LASTSAVE or truncating the AOF, covering the RunAsync == false branch that a device failure cannot reach
after a failure is cleared, SAVE succeeds, LASTSAVE advances, the status returns to ok, and a restart with tryRecover sees the data
the multi-database path, covering MultiDatabaseManager
MultiDatabaseForegroundSaveSkippingBusyDatabaseDoesNotReportSuccess — a foreground SAVE that skips a busy database returns an error rather than +OK, while BGSAVE in the same state still reports started, and SAVE succeeds again once no database is busy. The skip is made deterministic by holding the per-database checkpoint lock directly, rather than depending on a real checkpoint still being in flight.

Reverting only the LastSaveTime guard fails the save-reporting tests; restoring it passes them.

Validation

Suite	Result	Re-run after merging `main` (`cf1db86df`)
`Garnet.test`	1063 passed, 0 logic failures	1096 passed, 0 failed
`Garnet.test.cluster`	161 passed, 0 failed	not re-run
`Garnet.test.scripting` (MultiDatabase + Aof)	59 passed, 0 failed	MultiDatabase 32 passed, 0 failed
`Tsavorite.test`	342 passed, 0 failed	not re-run
`Tsavorite.test.recovery`	208 passed, 0 failed	236 passed, 0 failed
`Garnet.test.cluster.replication`	106 passed, 2 failed (both environmental; see below)	not re-run

Every log was checked for error CS before the result was trusted — 0 in all runs. A test project that fails to compile causes dotnet test to run a stale assembly and report a pass.

dotnet format --verify-no-changes clean on both Garnet.slnx and Tsavorite.slnx, before and after the merge.

Garnet.test.cluster.replication was added late, after a reviewer of a related change observed LASTSAVE did not advance within timeout failures elsewhere. That suite is the heaviest consumer of the exact pattern this PR changes: ClusterTestUtils.WaitCheckpoint polls LASTSAVE for 30 s and fails if it does not advance, and ClusterReplicationBaseTests calls it on primaries and replicas in 13 places. Since this PR stops LASTSAVE advancing on a failed checkpoint, a genuinely failing checkpoint there would now turn a silent pass into a timeout.

It does not. The run shows zero LASTSAVE/WaitCheckpoint failures. The two failures are cluster-formation contention before any checkpoint is taken — ERR Slot 0 is already busy at 145 ms during setup, and a gossip WaitUntilNodeIsKnown timeout in SimplePrimaryReplicaSetup — and both pass in isolation (8/8, 27 s).

Merge with #2153 (resolved)

#2153 (refuse to serve when no HybridLog checkpoint is readable) has merged, and main is merged into this branch as of cf1db86df. The two PRs shared six files; three auto-merged and three conflicted.

shared file	this PR	#2153	outcome
`libs/server/Databases/DatabaseManagerBase.cs`	checkpoint outcome plumbing	recovery outcome plumbing	auto-merged
`libs/server/Databases/IDatabaseManager.cs`	`CheckpointStatus` on the save API	recovery-outcome member	auto-merged
`libs/server/StoreWrapper.cs`	checkpoint entry points	recovery entry points	auto-merged
`libs/server/GarnetDatabase.cs`	last-save status field	recovery bookkeeping	conflict — kept both fields
`libs/server/Databases/SingleDatabaseManager.cs`	save outcome recording	recovery path	conflict — took #2153
`libs/server/Databases/MultiDatabaseManager.cs`	save aggregation	recovery path	conflict — took #2153

The two comment hazards flagged before the merge were both real, and neither was resolved by keeping both sides:

In both database managers, this PR's comment asserted that a never-checkpointed directory is "indistinguishable from a fresh start". Refuse to serve when no HybridLog checkpoint is readable #2153's token counts make that false, so Refuse to serve when no HybridLog checkpoint is readable #2153's text was taken wholesale. Only the one clause of this PR's comment that remains true was preserved — the cross-reference explaining why a failed checkpoint must never be reported as a successful save — relocated into the CandidateTokenCount == 0 branch, which is the case where it still holds.
In MultiDatabaseManager's checkpoint aggregation, the pre-existing comment asserting that a skipped database "leaves this a success" is deleted by this PR, because the code there now does the opposite: a skipped database yields AlreadyInProgress. That deletion survived the merge intact.

GarnetDatabase.cs was a clean keep-both: LastSaveSucceeded and CheckpointRecovery are independent. CheckpointRecovery is deliberately not copied by copyLastSaveData, matching #2153's own handling — it describes what recovery found at startup, not what the last save wrote.

Post-merge validation is the second column of the table above.

A checkpoint that failed was reported to clients as a successful save. DatabaseManagerBase.TakeCheckpointAsync returned long?, using null both for "this was an incremental checkpoint, so there is no tail address to record" and for "the checkpoint threw", and every caller then advanced LastSaveTime unconditionally. BGSAVE followed by the documented LASTSAVE-polling pattern therefore reported durability for data that was never written; the loss only surfaced as an empty store after the next restart, because recovery treats a missing hybrid log as a fresh start. Two further paths reported the same false success: - InitiateCheckpointAsync captured the result of StateMachineDriver.RunAsync and never read it. RunAsync returns false when another state machine (an index resize, for instance) is already running, meaning no checkpoint was taken, yet the code still truncated the AOF and registered a cluster checkpoint entry - discarding data the checkpoint did not cover. - The failure was logged through the optional logger parameter rather than the manager's own logger, so it frequently left no trace at all. Replace the overloaded null with a CheckpointResult carrying an explicit IsSuccessful alongside the nullable tail address, and record the outcome through a single UpdateLastSaveData that advances LastSaveTime only on success. Failures are returned rather than thrown so that a background checkpoint cannot tear down the server and one database's failure does not abort the bookkeeping of the databases checkpointed alongside it. IDatabaseManager.TakeCheckpointAsync now returns a CheckpointStatus, since its bool could not distinguish "another checkpoint is in progress" from "the checkpoint failed". A failed checkpoint also left the store unable to take any later one: the REST phase that releases _indexCheckpoint and _hybridLogCheckpoint is never entered when the state machine aborts, so the next checkpoint failed its Debug.Assert(_indexCheckpoint.IsDefault) in PREPARE. CompleteCheckpointAsync already performs exactly this cleanup on failure, but is not on the RunAsync path Garnet uses. Add an OnAbort hook to IStateMachineTask, called by the driver when it fast-forwards an aborted state machine to REST, and perform the same cleanup there. Client-visible changes: - SAVE now returns an error when the checkpoint did not complete, instead of replying OK. This matches Redis, which reports a failed SAVE as an error. - BGSAVE's reply is unchanged, as it is sent before the checkpoint runs. - LASTSAVE advances only for a checkpoint that completed, so it can be polled after BGSAVE to confirm persistence, as the documentation says. - INFO PERSISTENCE gains rdb_last_bgsave_status (ok/err), which is how a background save's failure is reported in Redis. The section was previously suppressed entirely unless the append-only file was enabled, even though its fields already fall back to "N/A" in that case; it is now always emitted so a checkpoint failure is visible without AOF. The regression tests inject the failure through a device factory that throws when creating store checkpoint devices, reproducing the original GetSnapshotObjectLogDevice failure without depending on path length, file permissions or platform, and they cover the multi-database manager as well.

Copilot

🟡 Changes recommended

Abort cleanup can leave RangeIndex barriers active, and multi-database SAVE can still report success after skipping a busy database.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Prevents failed checkpoints from being reported as successful and adds observable persistence status. Metadata matches the implementation, though no GitHub issue is linked.

Changes:

Introduces explicit checkpoint result/status handling.
Cleans up aborted Tsavorite checkpoints.
Adds INFO metrics, documentation, and failure tests.

File summaries

File	Description
`website/docs/commands/server.md`	Documents checkpoint status metrics.
`website/docs/commands/checkpoint.md`	Documents SAVE/BGSAVE failure behavior.
`test/standalone/Garnet.test/RespAdminCommandsTests.cs`	Updates status assertions.
`test/standalone/Garnet.test/FailingCheckpointDeviceFactoryCreator.cs`	Adds checkpoint failure injection.
`test/standalone/Garnet.test/CheckpointFailureTests.cs`	Tests client-visible failure handling.
`libs/storage/Tsavorite/cs/src/core/Index/Checkpointing/StateMachineDriver.cs`	Invokes abort cleanup.
`libs/storage/Tsavorite/cs/src/core/Index/Checkpointing/StateMachineBase.cs`	Delegates abort callbacks.
`libs/storage/Tsavorite/cs/src/core/Index/Checkpointing/IStateMachineTask.cs`	Adds the abort hook.
`libs/storage/Tsavorite/cs/src/core/Index/Checkpointing/IndexCheckpointSMTask.cs`	Resets aborted index checkpoints.
`libs/storage/Tsavorite/cs/src/core/Index/Checkpointing/HybridLogCheckpointSMTask.cs`	Disposes aborted log checkpoints.
`libs/server/StoreWrapper.cs`	Exposes detailed checkpoint status.
`libs/server/Resp/CmdStrings.cs`	Adds checkpoint failure error text.
`libs/server/Resp/AdminCommands.cs`	Returns correct SAVE/BGSAVE responses.
`libs/server/Metrics/Info/GarnetInfoMetrics.cs`	Exposes persistence status without AOF.
`libs/server/GarnetDatabase.cs`	Tracks the latest checkpoint outcome.
`libs/server/Databases/SingleDatabaseManager.cs`	Records successful and failed outcomes.
`libs/server/Databases/MultiDatabaseManager.cs`	Aggregates per-database outcomes.
`libs/server/Databases/IDatabaseManager.cs`	Updates the checkpoint contract.
`libs/server/Databases/DatabaseManagerBase.cs`	Centralizes outcome handling.
`libs/server/Databases/CheckpointStatus.cs`	Defines checkpoint result types.

Review details

Files reviewed: 20/20 changed files
Comments generated: 4
Review effort level: Balanced

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

Review feedback on the abort path introduced by the previous commit, from GitHub Copilot on the pull request and from the session that reported the original bug. Both independently identified that the abort hook released Tsavorite's own checkpoint state but left lifecycle side effects published by earlier phases in place. Fault the checkpoint task so waiters cannot hang. The REST phase completes store.checkpointTcs and publishes a fresh one; an aborted state machine never reaches it, so ClientSession.WaitForCommitAsync - which re-reads store.CheckpointTask in its loop - parked forever. OnAbort now publishes the replacement source before faulting the old one, so a continuation that immediately re-reads the field picks up the source for the next checkpoint rather than the one it just watched fail. This requires the aborting exception, so OnAbort takes it alongside the driver. Release the range-index checkpoint barrier. VersionShift sets a barrier that only FlushBegin clears, and range index operations spin-wait on it, so a checkpoint aborting between IN_PROGRESS and WAIT_FLUSH blocked them indefinitely. Add CheckpointTrigger.CheckpointFailed, raise it from the hybrid-log abort path, and clear the barrier in response. It deliberately does not run the vector manager's completion work: that reclaims deletions, which is only safe once a checkpoint has made them recoverable. The trigger can fire without VersionShift having been reached, so handling is idempotent. The tests had two blind spots, both now closed. The device injector threw for every checkpoint device, so a full checkpoint failed in the index checkpoint task before the hybrid-log checkpoint was initialized. The tests therefore only ever exercised the index cleanup and never reproduced the snapshot-device failure that motivated the work. The injector now takes a failure mode; SnapshotLogDevicesOnly lets the index device through and fails only the snapshot log devices, which are requested at WAIT_FLUSH with both checkpoints live. No device failure can abort at IN_PROGRESS, because no device is created there - so the range-index window above was unreachable from a device-based test. A phase-based callback that throws once on entry to a chosen phase covers it, and pins down the index cleanup in isolation. Tests now assert the absence of leaks directly - snapshot devices, object log flush buffers and the index hash table device are all null, and the index checkpoint is default - rather than only that a later checkpoint succeeds, which a leak would still allow while leaking a handle each time. Also covered: that a failed checkpoint faults the pending checkpoint task and publishes a new incomplete one, and that a checkpoint blocked by another state machine fails without advancing LASTSAVE or truncating the AOF. The latter exercises the RunAsync == false branch, which the device injector could not reach because it makes RunAsync throw instead.

The comment claimed the save path is the only place a checkpoint that was never written can be told apart from a fresh start. That holds when no checkpoint was ever written, because there are no tokens to find, but this catch also covers the case where tokens exist and none of them could be read, which is not a fresh start and is distinguishable here.

A general SAVE skips any database whose per-DB checkpoint lock is already held, but the aggregate outcome only considered the databases it actually pause-locked, so a skipped database was invisible and SAVE replied +OK. Track how many databases the request was asked to checkpoint and return AlreadyInProgress when fewer were attempted, prioritizing Failed so the more actionable outcome still wins. Background requests reply before the aggregate is computed, so the BGSAVE contract of skipping busy databases and reporting started is unchanged.

Ted Hart (TedHartMS) · 2026-09-20T19:49:13Z

Blocker analysis: CI failure + open review verdict

Two things were blocking this PR. One is not mine; the other was, and I was wrong about it in my earlier reply.

1. The `Build Tsavorite` failures are inherited from `main`, not introduced here

All four failing checks (Build Tsavorite on ubuntu/windows × Debug/Release) fail with:

TestBase.cs(46,9): error CS0234: The type or namespace name 'TestUtils'
  does not exist in the namespace 'Garnet.test'  [Tsavorite.test.csproj]

Cause. libs/storage/Tsavorite/cs/test/Tsavorite.test.csproj links exactly one file out of the Garnet test project:

<Compile Include="..\..\..\..\..\test\standalone\Garnet.test\TestBase.cs" Link="TestBase.cs" />

TestUtils.cs is not linked. #2151 ("Give tests per-checkout port isolation via GARNET_TEST_PORT_SLOT", 5c6e59536) added a call to Garnet.test.TestUtils.EnsurePortSlotResolved() at TestBase.cs:46 but did not add TestUtils.cs to that csproj, so Tsavorite.test stopped compiling.

Reproduced independently on pristine main. I checked out origin/main at 5c6e59536 in a separate scratch worktree with none of this branch's changes present and built Tsavorite.test.csproj: same error, same file, same line 46, column 9, Build FAILED.

Control verified. At a3fa958db (the parent of #2151) TestBase.cs contains zero references to TestUtils, and git show --stat 5c6e59536 shows #2151 touched TestBase.cs (+4) and TestUtils.cs (+493) but never touched Tsavorite.test.csproj.

This branch was fully green before the base-branch update; the update is what introduced the failure. Fixing it belongs in a main-targeted change — either link TestUtils.cs into Tsavorite.test.csproj, or make the EnsurePortSlotResolved() call conditional on the type being available. I have deliberately not merged main again, and will not until that build is fixed.

2. Copilot review verdict (submitted against `daa432a6`)

Concern A — "abort cleanup can leave RangeIndex barriers active": already addressed, and I traced the rest

This was fixed in d408df845 ("Complete the checkpoint abort cleanup and cover it with tests"), which postdates the reviewed commit. Rather than assert that, here is the full inventory of state raised during a checkpoint and where each item is released on the abort path:

State raised during checkpoint	Raised at	Released on abort by	Origin
RangeIndex checkpoint barrier (`SetCheckpointBarrier`)	`IN_PROGRESS`, via `CheckpointTrigger.VersionShift`	`CheckpointTrigger.CheckpointFailed` → `ClearCheckpointBarrier()` (`GarnetRecordTriggers.cs:189-197`)	this PR
`store._indexCheckpoint`	`PREPARE`	`IndexCheckpointSMTask.OnAbort` → `Reset()`	this PR
`store._hybridLogCheckpoint` (snapshot devices, flush buffers)	`PREPARE` / `WAIT_FLUSH`	`HybridLogCheckpointSMTask.OnAbort` → `Dispose()`	this PR
`store.checkpointTcs`	continuous	`OnAbort` republishes a fresh TCS before faulting the old one, so no waiter observes a faulted-and-final TCS	this PR
`lastVersion` / `lastVersionTransactionsDone`	`IN_PROGRESS` (`TrackLastVersion`)	`FastForwardStateMachineToRest()` → `ResetLastVersion()`	pre-existing
`waitForTransitionIn`/`Out` semaphores, `waitingList`, `waitForTransitionInException`	per transition	`FastForwardStateMachineToRest()` releases and clears	pre-existing
`stateMachine` registration slot, `stateMachineCompleted`	`Register` / `RunAsync`	`RunStateMachine` `finally`: `Interlocked.Exchange(ref stateMachine, null)`, TCS faulted/cancelled	pre-existing
Epoch protection	`GlobalStateMachineStep`	try/finally `epoch.Suspend()`	pre-existing
Garnet per-DB `CheckpointingLock`	`TryPauseCheckpoints`	try/finally `ResumeCheckpoints` on every path, including `RunPausedCheckpointsAndReleaseLocksAsync`'s `finally`	mixed
`vectorManager` deletion reclamation	—	deliberately not run on failure — reclaiming is only safe once a checkpoint has made the deletions recoverable	this PR

Two details worth stating explicitly:

Ordering. In RunStateMachine's catch, FastForwardStateMachineToRest() runs before ReleaseAbortedStateMachineResources(e), so waiters are released before task-owned resources are disposed. The entire diff to StateMachineDriver.cs is one added call plus that method, which wraps stateMachine.OnAbort in try/catch so a cleanup failure cannot replace the actionable aborting exception.
Cluster mode. I chased GarnetClusterCheckpointManager.CheckpointVersionShiftStart/End to its delegate targets at ReplicationManager.cs:311 and :331. They acquire no lock and no barrier — they only enqueue an AOF marker (AofEntryType.CheckpointStartCommit / CheckpointEndCommit), and only on a primary. So there is nothing held between Start and End to leak. An abort in the fuzzy region does leave an unmatched start marker, but AofProcessor.cs:274-278 already handles exactly that case by design (a subsequent CheckpointStartCommit clears the stale fuzzy-region buffer, and :295 ignores an unmatched end marker). Pre-existing and unchanged by this PR.

No residual gap found.

Concern B — "multi-database SAVE can still report success after skipping a busy database": this was a real bug, and my earlier reply was wrong

I previously dismissed this on the grounds that making the change would break MultiDatabaseSaveInProgressTest. That was incorrect, and the mistake is worth recording because it is a specific conflation, not a judgment call. There are two different candidate changes and I evaluated the wrong one:

Changing the pause phase — returning AlreadyInProgress when pausedCount == 0. This happens before if (background) return Task.FromResult(CheckpointStatus.Success);, so it does affect BGSAVE and does break that test. This is what I tried, reverted, and then cited.
Changing the aggregation — which is consumed only on the foreground path (return checkpointTask; when !background) and therefore cannot affect BGSAVE at all.

Copilot's comment was about (2). MultiDatabaseSaveInProgressTest issues only BGSAVE / BGSAVE 0 / BGSAVE 1 and never a foreground SAVE, so it never observes the aggregate.

The bug. MultiDatabaseManager.TakeCheckpointAsync silently skips a database it cannot pause-lock:

if (TryPauseCheckpoints(id))
    pausedDbIds[pausedCount++] = id;

and RunPausedCheckpointsAndReleaseLocksAsync then aggregated over pausedCount only, so a skipped database was invisible and a partial SAVE returned Success → +OK.

Fix (bc528d8b9): thread an explicit requestedCount (activeDbIdsMapSize on the all-DBs path, 1 on the single-DB path) into the aggregation, and resolve as: any attempted database failed → Failed; else pausedCount < requestedCount → AlreadyInProgress; else Success. Failed is prioritized so the more actionable outcome wins. The stale comment that asserted the opposite ("an empty set therefore leaves this a success") is replaced.

Regression coverage: MultiDatabaseForegroundSaveSkippingBusyDatabaseDoesNotReportSuccess. It holds DB 1's per-DB checkpoint lock directly via TryPauseCheckpoints(1) — the same state an in-flight per-DB BGSAVE leaves — which makes the skip deterministic instead of depending on a real checkpoint still being in flight. It asserts that foreground SAVE errors, that BGSAVE still replies Background saving started in the same state, and that SAVE succeeds again once no database is busy.

Verified against a control. With MultiDatabaseManager.cs reverted and only the test present (confirmed by git diff --stat HEAD showing the test file alone), the new test fails with Expected: <RedisServerException> But was: null — i.e. SAVE really did answer +OK after skipping a busy database. With the fix, it passes. The two pre-existing tests pass on both sides, confirming the BGSAVE contract is untouched.

Client-visible behavior change: a foreground SAVE covering all databases now returns ERR checkpoint already in progress instead of +OK when it skipped a database whose checkpoint was already running. Per-database SAVE <dbid> is unchanged (that path already returned this error). BGSAVE in all forms is unchanged.

Note the per-database durability signal was always truthful: a skipped database's own in-flight checkpoint owns its LASTSAVE and rdb_last_bgsave_status and records its outcome through the same RecordCheckpointOutcome path. What was untruthful is the aggregate SAVE reply — which is exactly the class of bug this PR exists to fix, so it belongs here.

Validation

All runs through the shared test lock; every log grepped for error CS first (a failed compile silently reruns a stale assembly).

Suite	Result	`error CS`
`Garnet.test.scripting --filter MultiDatabase`	32 passed, 0 failed, 4 skipped	0
`Garnet.test --filter CheckpointFailureTests`	8 passed, 0 failed	0
New test against reverted libs (control)	fails as expected	0
`dotnet format Garnet.slnx --verify-no-changes`	exit 0	—

CI cannot go green until main's Tsavorite.test build is fixed; that failure is inherited and independent of everything above.

Ted Hart (TedHartMS) · 2026-09-20T19:58:43Z

Correction to my remedy suggestion above, and a merge-overlap note.

1. One of the two fixes I suggested for the inherited Build Tsavorite failure is wrong. I wrote that it could be fixed by "either link TestUtils.cs into Tsavorite.test.csproj, or make the EnsurePortSlotResolved() call conditional." The first option does not work, and I should not have offered it without testing it — the diagnosis above was verified by reproduction, but the remedy was not.

Tsavorite.test.csproj references only Tsavorite.core and Tsavorite.devices.AzureStorageDevice. TestUtils.cs needs Garnet.client, Garnet.common, Garnet.server, Garnet.server.Auth.Settings, Garnet.server.TLS, Roslyn and StackExchange.Redis. Linking it would give the storage engine's own test project a dependency on the Garnet server stack, inverting the layering that the separate Tsavorite.slnx exists to preserve. #2161 tested exactly this and measured 122 errors.

The second option is the validated one, and #2161 carries it with build results for all three affected projects. It is also semantically right rather than a compile workaround: Tsavorite.test never binds Garnet's per-sub-project test ports, so it has no port slot to claim. Nothing here is actionable on this branch — the fix belongs on main, tracked in #2161 — but the bad half of my suggestion should not be left standing where someone might act on it.

2. Merge overlap with #2153 is six files, not the one method I flagged earlier. Shared: DatabaseManagerBase.cs, IDatabaseManager.cs, MultiDatabaseManager.cs, SingleDatabaseManager.cs, GarnetDatabase.cs, StoreWrapper.cs. Neither PR's surface contains the other's — #2153 also touches 11 files this one does not. The PR body now carries the full map, including the one conflict that must not be resolved by keeping both sides: the MultiDatabaseManager comment asserting that a skipped database "leaves this a success", which this PR deletes because the code there now does the opposite.

…-failed-checkpoint # Conflicts: # libs/server/Databases/MultiDatabaseManager.cs # libs/server/Databases/SingleDatabaseManager.cs # libs/server/GarnetDatabase.cs

Ted Hart (TedHartMS) · 2026-09-21T18:31:12Z

Merged current main (cf1db86df), which now includes #2153. The PR body's merge section is updated to record what was resolved; summary for re-review:

Six files were shared with #2153. Three auto-merged (DatabaseManagerBase.cs, IDatabaseManager.cs, StoreWrapper.cs) and three conflicted.

GarnetDatabase.cs — kept both fields. LastSaveSucceeded (this PR) and CheckpointRecovery (Refuse to serve when no HybridLog checkpoint is readable #2153) are independent. CheckpointRecovery is deliberately left out of copyLastSaveData, matching Refuse to serve when no HybridLog checkpoint is readable #2153's own handling: it describes what recovery found at startup, not what the last save wrote.
SingleDatabaseManager.cs / MultiDatabaseManager.cs — took Refuse to serve when no HybridLog checkpoint is readable #2153's version of the TsavoriteNoHybridLogException handler in both.

The second one is worth a sentence, because keeping both sides would have been actively wrong rather than merely redundant. This PR's comment asserted that a never-checkpointed directory is "indistinguishable from a fresh start". #2153's token counts are precisely what makes that distinguishable, so retaining the comment would have left a durability path documented as doing the opposite of what the code does. #2153's text was taken wholesale, and only the one clause of mine that is still true was preserved — the cross-reference explaining why a failed checkpoint must never be reported as a successful save — moved into the CandidateTokenCount == 0 branch, which is the case it still describes.

This PR's own aggregation change survived intact, including the deletion of the stale "an empty set therefore leaves this a success" comment.

Validation after the merge, every log checked for error CS (0 in all):

Suite	Result
`Garnet.test`	1096 passed, 0 failed
`Tsavorite.test.recovery`	236 passed, 0 failed
`CheckpointFailureTests` + `RespRecoveryFailOpenTests`	21 passed, 0 failed
`Garnet.test.scripting` (MultiDatabase)	32 passed, 0 failed

dotnet format --verify-no-changes exit 0 on both solutions.

The Build Tsavorite failures are gone. All four jobs pass on this merge, along with all four Build Garnet jobs and both format checks. That break was never from this branch — it was inherited from main via #2151 and is now fixed on main by #2159 (GARNET_TEST_UTILS), which this merge picks up. #2161 is closed.

CI result on the merge: 224 pass, 1 fail, and the one failure is not a test failure.

Garnet Cluster (windows-latest, net8.0, Debug, Garnet.test.cluster.vectorsets) reports failed, but the step breakdown shows the tests themselves passed:

step 6: Run tests Garnet.test.cluster.vectorsets -> success
step 7: Upload test results                      -> failure

The job failed while uploading the results artifact, after the suite had already run green. It is the only failing step anywhere in the run (verified across all 225 checks), and the same suite passes on the other 15 matrix combinations of this exact commit, including windows-latest/net8.0/Release and windows-latest/net10.0/Debug. The suite also references none of LASTSAVE, BGSAVE, WaitCheckpoint, TakeCheckpoint or CheckpointStatus, so it does not exercise the path this PR changes.

No re-run is needed to interpret it, and it is not evidence of a regression here.

Ted Hart (TedHartMS) requested review from Badrish Chandramouli (badrishc) and Vasileios Zois (vazois) and a balanced review from Copilot September 17, 2026 03:03

Copilot started reviewing on behalf of Ted Hart (TedHartMS) September 17, 2026 03:04 View session

Copilot AI reviewed Sep 17, 2026

View reviewed changes

Ted Hart (TedHartMS) mentioned this pull request Sep 17, 2026

Normalize local-filesystem path composition and enable extended-length test paths #2147

Merged

Ted Hart (TedHartMS) added 2 commits September 16, 2026 20:16

Merge branch 'main' into tedhar-bgsave-reports-failed-checkpoint

75bcd19

This was referenced Sep 17, 2026

DeviceLogCommitCheckpointManager shares I/O completions across concurrent reads, causing valid checkpoints to be skipped #2149

Open

--fail-on-recovery-error true still serves an incomplete AOF tail when no HybridLog checkpoint is readable #2150

Closed

This was referenced Sep 17, 2026

Refuse to serve when no HybridLog checkpoint is readable #2153

Merged

Test teardown: TestUtils.DeleteDirectory retries forever on a permanent error, and the 60s cap abandons the spinning thread #2155

Open

Badrish Chandramouli (badrishc) approved these changes Sep 19, 2026

View reviewed changes

Ted Hart (TedHartMS) added 2 commits September 19, 2026 14:31

Merge branch 'main' into tedhar-bgsave-reports-failed-checkpoint

5f60244

Ted Hart (TedHartMS) added 2 commits September 21, 2026 10:17

Merge branch 'main' into tedhar-bgsave-reports-failed-checkpoint

85d0caa

Merge remote-tracking branch 'origin/main' into tedhar-bgsave-reports…

cf1db86

…-failed-checkpoint # Conflicts: # libs/server/Databases/MultiDatabaseManager.cs # libs/server/Databases/SingleDatabaseManager.cs # libs/server/GarnetDatabase.cs

Merge branch 'main' into tedhar-bgsave-reports-failed-checkpoint

8d0dbd8

Badrish Chandramouli (badrishc) approved these changes Sep 22, 2026

View reviewed changes

Navigation Menu

Do not report a failed checkpoint as a successful save - #2145

Do not report a failed checkpoint as a successful save#2145
Ted Hart (TedHartMS) wants to merge 9 commits into
mainfrom
tedhar-bgsave-reports-failed-checkpoint

Ted Hart (TedHartMS) commented Sep 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ted Hart (TedHartMS) commented Sep 20, 2026

Uh oh!

Ted Hart (TedHartMS) commented Sep 20, 2026

Uh oh!

Ted Hart (TedHartMS) commented Sep 21, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Ted Hart (TedHartMS) commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

An aborted checkpoint left the store permanently broken

Client-visible changes

Not changed

Tests

Validation

Merge with #2153 (resolved)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ted Hart (TedHartMS) commented Sep 20, 2026

Blocker analysis: CI failure + open review verdict

1. The Build Tsavorite failures are inherited from main, not introduced here

2. Copilot review verdict (submitted against daa432a6)

Concern A — "abort cleanup can leave RangeIndex barriers active": already addressed, and I traced the rest

Concern B — "multi-database SAVE can still report success after skipping a busy database": this was a real bug, and my earlier reply was wrong

Validation

Uh oh!

Ted Hart (TedHartMS) commented Sep 20, 2026

Uh oh!

Ted Hart (TedHartMS) commented Sep 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Ted Hart (TedHartMS) commented Sep 17, 2026 •

edited

Loading

1. The `Build Tsavorite` failures are inherited from `main`, not introduced here

2. Copilot review verdict (submitted against `daa432a6`)

Ted Hart (TedHartMS) commented Sep 21, 2026 •

edited

Loading