# Bug: snapshot writer produces truncated files silently — recovery dies on every restart

**Reported:** 2026-04-19
**Severity:** Critical (data loss potential — affected Zoe production instance)
**Affects:** smartbotic-database 1.6.0-1 (almost certainly older versions too — writer code is unchanged in HEAD)
**Files:** `service/src/persistence/snapshot.cpp` (writer + loader), `service/src/persistence/snapshot.hpp`

## Symptom on production

Zoe instance (`shadowman-zoe`) was restarted as part of a routine `shadowman` package upgrade. The smartbotic-database service was bounced via `systemctl restart smartbotic-database`. On startup it logged:

```
[2026-04-19 11:37:55.391] [error] [database] Recovery failed: Failed to read snapshot body
[2026-04-19 11:37:55.391] [info]  [database] ViewManager: loaded 0 views from _views
[2026-04-19 11:37:55.391] [info]  [database] CollectionConfigManager: loaded 0 configs from _collection_meta
[2026-04-19 11:37:55.391] [info]  [database] Database service initialized successfully
[2026-04-19 11:37:55.391] [info]  [database] Starting database service on 0.0.0.0:9004
```

The DB came up **empty** — zero documents — and started accepting writes against a blank store. All conversations, plugins, settings, KB articles, secrets, etc. silently disappeared from the live store. Data on disk in `wal/` and `snapshots/` was not modified by recovery, but no automatic fallback exists when the latest snapshot fails to load.

## Root cause

Every snapshot file on disk is truncated relative to what its own header claims. Header parsing (with proper struct alignment — `<8sI4xQQQI4xQQII` — total 72 bytes):

| File | Actual size | Header `compressedSize` + 72 hdr + 4 crc | Bytes missing |
|---|---:|---:|---:|
| snapshot-20260419-070625.dat | 70,856,704 | 73,146,794 | -2,290,090 |
| snapshot-20260419-080652.dat | 71,659,520 | 73,258,664 | -1,599,144 |
| snapshot-20260419-090719.dat | 71,770,112 | 73,369,107 | -1,598,995 |
| snapshot-20260419-100747.dat | 59,174,912 | 74,496,336 | **-15,321,424** |
| snapshot-20260419-110816.dat | 69,611,520 | (file moved aside; same pattern) | — |

Headers themselves parse correctly (magic, version, walSequence, documentCount, collectionCount, uncompressedSize, compressedSize, compressionType all sane). Body bytes are short. Reader fails in `loadSnapshot` at the body read:

```cpp
// service/src/persistence/snapshot.cpp ~ line 121
std::vector<uint8_t> bodyData(header.compressedSize);
file.read(reinterpret_cast<char*>(bodyData.data()),
          static_cast<std::streamsize>(bodyData.size()));
if (!file) {
    throw std::runtime_error("Failed to read snapshot body");   // <-- this fires
}
```

EOF before reading `compressedSize` bytes → failbit → throw.

## Bug in the writer

Looking at the snapshot writer (~ line 60 of the same file):

```cpp
std::ofstream file(snapshotPath, std::ios::binary);
if (!file) {
    throw std::runtime_error("Failed to create snapshot file: " + snapshotPath.string());
}

// Write header
file.write(reinterpret_cast<const char*>(&header), sizeof(header));

// Write body
file.write(reinterpret_cast<const char*>(bodyData.data()),
           static_cast<std::streamsize>(bodyData.size()));

// Write body checksum
uint32_t bodyChecksum = crc32::calculate(bodyData.data(), bodyData.size());
file.write(reinterpret_cast<const char*>(&bodyChecksum), sizeof(bodyChecksum));

file.close();
```

Three independent issues here, each contributing to the silent corruption:

1. **No error check after `write()`.** `std::ofstream::write` sets the failbit on partial write, but the code never checks it. A short write (disk pressure, OS-level buffering quirk, NFS, ENOSPC partway through) silently truncates the file — the writer happily moves on to the body checksum write, then `close()`.
2. **No `fsync()` / `flush()` before `close()`.** Even when `close()` returns success, there's no guarantee the bytes hit the platter before the file is considered "complete." A subsequent crash or power loss can leave a header-pointing-at-incomplete-body file.
3. **No "atomic rename" pattern.** The snapshot is written *in place* under its final name. If anything goes wrong mid-write — including the OS choosing to drop write-buffer pages on memory pressure — the partially-written file survives as a "valid" snapshot from the loader's POV. The standard pattern is: write to `<name>.tmp`, fsync, rename to `<name>`. That guarantees readers either see a complete file or no file.

## Bug in the loader (cascade)

When the latest snapshot fails to load:

```cpp
uint64_t SnapshotManager::loadLatestSnapshot(MemoryStore& store) {
    auto snapshots = listSnapshots();
    if (snapshots.empty()) return 0;
    return loadSnapshot(snapshots.front(), store);   // throws on failure, no fallback
}
```

The loader has zero fallback to the next-most-recent snapshot. One bad file means no recovery, ever. With `maxSnapshots = 5` and snapshot churn happening hourly in production, all reasonably-recent options are in the rotation — and apparently they're all corrupt.

Even better: loadLatestSnapshot doesn't even *try* successive snapshots — it bubbles the exception up to `database_service.cpp` which logs `"Recovery failed: ..."` and continues with an empty store. The user has no signal that data is on disk-but-unreachable; they just see an empty DB.

## Reproduction

To repro the writer truncation on a synthetic disk pressure scenario:

```bash
# Constrain the writer's filesystem to ~50% of expected snapshot size
# Run a workload that triggers snapshot creation
# Observe: header.compressedSize > st_size after the write
stat snapshot-*.dat
python3 -c "import struct; ..."  # parse header per the table above
```

Without disk pressure, the writer might not always truncate, but the point is: **there is no validation that it didn't.**

## Suggested fixes (priority order)

### 1. Fix the writer — three changes

```cpp
namespace fs = std::filesystem;
auto tmpPath = snapshotPath.string() + ".tmp";

{
    std::ofstream file(tmpPath, std::ios::binary);
    if (!file) throw std::runtime_error("snapshot create failed: " + tmpPath);

    file.write(reinterpret_cast<const char*>(&header), sizeof(header));
    if (!file) throw std::runtime_error("snapshot header write failed");

    file.write(reinterpret_cast<const char*>(bodyData.data()),
               static_cast<std::streamsize>(bodyData.size()));
    if (!file) throw std::runtime_error("snapshot body write failed");

    file.write(reinterpret_cast<const char*>(&bodyChecksum), sizeof(bodyChecksum));
    if (!file) throw std::runtime_error("snapshot trailer write failed");

    file.flush();
    if (!file) throw std::runtime_error("snapshot flush failed");

    // fsync the file descriptor for durability
    int fd = ::open(tmpPath.c_str(), O_RDONLY);
    if (fd >= 0) { ::fsync(fd); ::close(fd); }
}

// Atomic rename over the final name. Either the rename succeeds and a
// complete snapshot is now visible, or it fails and the partial .tmp
// stays out of the snapshot list.
std::error_code ec;
fs::rename(tmpPath, snapshotPath, ec);
if (ec) throw std::runtime_error("snapshot rename failed: " + ec.message());

// Optionally fsync the directory so the rename is durable
int dfd = ::open(config_.snapshotDir.c_str(), O_RDONLY);
if (dfd >= 0) { ::fsync(dfd); ::close(dfd); }
```

After this change, every file in the snapshots dir is either complete or absent.

### 2. Fix the loader — fallback chain

```cpp
uint64_t SnapshotManager::loadLatestSnapshot(MemoryStore& store) {
    auto snapshots = listSnapshots();   // already sorted newest-first
    for (const auto& path : snapshots) {
        try {
            return loadSnapshot(path, store);
        } catch (const std::exception& e) {
            spdlog::warn("snapshot load failed for {}: {} — trying older snapshot",
                         path.string(), e.what());
        }
    }
    spdlog::error("all {} snapshots failed to load — starting from empty store",
                  snapshots.size());
    return 0;
}
```

Cheap defense against corner cases that escape fix #1 (e.g. a snapshot that survives a failed disk).

### 3. Validate snapshots after write

After every successful snapshot write, immediately try to load it (open + read header + read body + verify CRC) into a temporary `MemoryStore` to confirm round-trip integrity. If validation fails, delete the corrupt file and log loudly so the operator notices BEFORE the next restart needs it.

This is more expensive (doubles snapshot creation cost) but it's the only way to guarantee recoverability. Could be gated by a config flag for hot paths.

### 4. Don't silently start with an empty store

When all snapshots fail to load AND the data directory is non-empty (snapshots exist on disk), refuse to start. Force the operator to acknowledge the data loss — either by passing `--allow-empty-recovery` or by manually clearing the snapshots dir. Currently the service silently masks data loss as a normal startup.

## Recovery for the affected instance

For Zoe specifically (and any other running instance that may have hit this), the workaround is:

1. Stop the database BEFORE the empty-DB writes overwhelm any chance of partial recovery.
2. Move corrupt snapshots aside (don't delete — keep for forensics).
3. If a manual snapshot-header rewrite tool is built (truncate `compressedSize` to actual file size, attempt decompression), some data may be partially recoverable — depends on whether the truncation cut into the LZ4 stream's frame boundary.
4. WAL alone is not enough — the DB documents/views/configs need a base snapshot to apply against.

We've snapshotted Zoe's `/var/lib/smartbotic-database/` to preserve forensic state.

## Bonus issue spotted

While reading `snapshot.cpp` I noticed `cleanupOldSnapshots()` runs after a successful write. This is fine, BUT: combined with the writer NOT verifying its own output, every successful-but-corrupt write deletes a potentially-good older snapshot. The cleanup should at least verify the new snapshot is loadable (per fix #3) before evicting the oldest.

## File references

- `service/src/persistence/snapshot.cpp` — writer (~line 30-90), loader (~line 90-160), listSnapshots (~line 160-185)
- `service/src/persistence/snapshot.hpp` — `SnapshotHeader` struct (line 60), `SnapshotManager` class (line 75+)
- `service/src/database_service.cpp` — recovery call site (line ~52, "Starting recovery")
- `service/src/persistence/persistence_manager.cpp` — wal+snapshot wiring