Potential bug in dbshrink.cxx due to a typo

[JakeSays]

SHKPerformRootMove likely typo: fRecoveringRedo (enum constant) used where fRecoveryRedo (function parameter) was almost certainly intended

In dev/ese/src/ese/dbshrink.cxx, the function SHKPerformRootMove takes a BOOL fRecoveryRedo parameter. Two sites inside it reference an identifier that differs by a single letter — fRecoveringRedo — which is not the parameter but the second value of enum RECOVERING_MODE { fRecoveringNone, fRecoveringRedo, ... } defined in dev/ese/src/inc/daedef.hxx. That enum constant has integer value 1, so in a boolean context it is unconditionally true.

This appears to be a long-standing typo. clang-22 surfaces it as -Wconstant-logical-operand and -Wconstant-conversion-style diagnostics; MSVC issues no diagnostic for this idiom and the codebase has presumably been shipping with the typo's runtime semantics in place.

Affected code

File: dev/ese/src/ese/dbshrink.cxx

VOID SHKPerformRootMove(
    _In_ ROOTMOVE* const prm,
    _In_ PIB* const ppib,
    _In_ const IFMP ifmp,
    _In_ const BOOL fRecoveryRedo )       // <-- function parameter
{
    BOOL fPageFDPDelete = fFalse;

    // If we are in redo, get the PageFDPDelete flag so that we could set it on the new page.
    if ( prm->csrFDP.FLatched() && fRecoveringRedo )    // <-- Site 1: enum constant, not the parameter
    {
        fPageFDPDelete = prm->csrFDP.Cpage().FPageFDPRootDelete();
    }

    // ... later, inside an `if ( FBTIUpdatablePage( prm->csrNewFDP ) )` block:

        if ( fPageFDPDelete )
        {
            Assert( fRecoveringRedo );                   // <-- Site 2: enum constant, not the parameter
            prm->csrNewFDP.Cpage().SetPageFDPDelete( fTrue );
        }

The leading comment on Site 1 — "If we are in redo, get the PageFDPDelete flag so that we could set it on the new page" — strongly suggests the author intended to gate on the function parameter fRecoveryRedo (the redo/forward selector) rather than on a constant.

Call sites of SHKPerformRootMove

There are exactly two:

Call site	fRecoveryRedo value	Path
dev/ese/src/ese/dbshrink.cxx:~2216	fFalse	Forward shrink (do-time). Annotated /* fRecoveryRedo */ for clarity at the call site.
dev/ese/src/ese/_log/logredo.cxx:~10951	fTrue	Recovery redo. Annotated // fRecoveryRedo.

So the typo affects only the forward shrink path (the redo path takes the same branch either way because fRecoveryRedo == fTrue is true and the constant is also true).

Behavioral implications (as written)

prm->csrFDP.FLatched() returns a true BOOL (0/1) — BOOL CSR::FLatched() const { return m_latch != latchNone; } in dev/ese/src/inc/ccsr.hxx. Combined with && fRecoveringRedo (which is && 1), the clause at Site 1 reduces to:

if ( prm->csrFDP.FLatched() )       // effective behavior, both paths
    fPageFDPDelete = prm->csrFDP.Cpage().FPageFDPRootDelete();

So in both the redo path and the forward shrink path, if the FDP cursor is latched, the code reads FPageFDPRootDelete() from the existing FDP page and propagates it to the new FDP via SetPageFDPDelete( fTrue ) later in the function.

If the typo is genuine — i.e. the author meant to gate on the parameter — then the forward shrink path has been unintentionally copying the FPageFDPRootDelete flag onto the new FDP for as long as this code has existed. The Site 2 Assert( fRecoveringRedo ) would never have fired regardless, since it is Assert(1).

Questions for the ESE team

1. Was fRecoveringRedo at these two sites a typo for the parameter fRecoveryRedo? The leading comment ("If we are in redo, ...") and the parameter name suggest yes.
2. If yes, has the forward shrink path's silent propagation of FPageFDPRootDelete ever been observed in the wild? Is it a benign no-op (because forward shrink never encounters an FDP with that flag set) or a latent correctness issue?
3. If the author did intend the constant — i.e., the code was meant to run unconditionally and the && fRecoveringRedo is a stale "marker"-style remnant — should the line be cleaned up to drop the dead operand entirely?

References

• dev/ese/src/ese/dbshrink.cxx — SHKPerformRootMove, both sites and the call site at :~2216.
• dev/ese/src/ese/_log/logredo.cxx:~10951 — the redo call site.
• dev/ese/src/inc/daedef.hxx:~1018 — enum RECOVERING_MODE definition.
• dev/ese/src/inc/ccsr.hxx:~261 — BOOL CSR::FLatched() const.