WriteBatch::commit() does not poison the database on a write_batch() journal-write failure, unlike a persist() failure

[russfellows]

Summary

Database::batch().commit() performs two separate fallible steps against
the journal:

1. journal_writer.write_batch(...) — writes the batch's start marker,
   items, and end marker to the journal file.
2. journal_writer.persist(mode) — optional, only if a durability mode was
   requested; flushes/fsyncs.

In commit() (src/batch/mod.rs), if step 2 fails, the database is
poisoned (self.db.is_poisoned.poison()) and every future commit is
refused with Error::Poisoned. If step 1 fails, the error propagates
correctly (this is the WB-F02 fix, #304 — thank you for that one), but the
database is not poisoned.

This matters because a mid-batch failure in write_batch() can leave a
partial, unterminated record at the journal's current tail (e.g. the batch's
Start marker and some items written, but the End marker never reached
because a later write_all call in the same batch failed). Nothing
truncates or repairs that tail afterward. The next batch committed against
the same database — even with PersistMode::SyncAll — writes its own
Start/items/End sequence immediately after that leftover partial record,
and commit() correctly reports Ok for it, because that batch's own
writes genuinely succeeded.

On the next reopen, JournalBatchReader::next() (src/journal/batch_reader.rs)
replays the journal from the start. When it reaches the Start marker
belonging to the first (failed) batch's partial record, it recognizes a
second Entry::Start arriving before an Entry::End as corruption
("found batch start inside batch") and truncates the file back to
last_valid_pos — the position before the failed batch. This is a
reasonable response if the torn record were the last thing in the file (the
classic partial-write-before-a-crash case), but here it isn't: a
successfully committed, later batch is sitting right after it. That batch
is discarded along with the corruption, silently — no error is surfaced
anywhere in this path.

Net effect: commit() can return Ok(()) for a batch whose data does not
survive the next restart, with no diagnostic of any kind, for a batch that
had nothing wrong with it — the corruption belongs entirely to an earlier,
already-failed batch.

Why this is a different bug from #304 (WB-F02)

#304 was about a single batch's own Result being wrong. This is about the
database's state after a correctly-reported failure: persist() failures
already poison the database (undocumented publicly, but visible in
commit()'s own branch); write_batch() failures do not, even though both
can leave the same class of journal-tail damage behind. The fix, if it's
the right one from a maintainer's perspective, looks like: poison on any
error from journal_writer.write_batch(...)? in commit(), exactly as the
persist(mode) branch already does a few lines below it.

We are not asking for a rollback/repair mechanism — poisoning (refuse
further writes; let the caller restart/reopen) is the existing, already-
accepted contract for the sibling failure case, and would close this gap
with a one-line change in the same function.

Repro

Environment: Linux, fjall 3.1.8, a Database opened normally.

1. Commit Batch A with a value large enough to bypass the journal writer's
   internal BufWriter (>= JOURNAL_BUFFER_BYTES, 8 KiB) so
   write_batch() issues a real write(2) itself, and force that one
   write(2) call to fail (e.g. via an LD_PRELOAD interposer on write,
   scoped to the journal fd and to exactly one call). commit() correctly
   returns Err.
2. Immediately after, on the same Database handle, commit Batch B — an
   ordinary batch, e.g. with durability(Some(PersistMode::SyncAll)), no
   injected failure this time. commit() returns Ok(()).
3. Drop the Database and reopen it at the same path.
4. Batch B's key is absent.

We have a minimal, self-contained reproduction (two probe binaries plus an
LD_PRELOAD shim) and a passing/failing regression test built on it. If a
PR is welcome, we'd like to submit one: poison the database on the
write_batch(...)? error path in commit(), mirroring the existing
persist(mode) branch, plus the regression test (adapted from our own,
which currently lives against our downstream fork of the reproduction —
happy to rewrite it against fjall's own test harness/conventions instead).
Let us know and we'll open it.

How we're working around this in the meantime

Until this lands (or in case the fix takes a different shape than a plain
poison call), we've added an equivalent guard in our own application layer:
a shared Arc<AtomicBool> latch around every WriteBatch::commit() call
against a given Database, checked before the commit and set on any
failure — refusing all further commits against that database until the
process restarts. This gives us the same fail-closed behavior persist()
failures already get from fjall itself, just applied one layer up since we
can't set fjall's own internal poison flag from outside the crate. It's a
stopgap, not a substitute for the fix above — it only protects call sites
we've wrapped ourselves, and every other fjall consumer without an
equivalent layer is still exposed.

Version

fjall 3.1.8 (also present, by inspection, in any version since the #304 fix
landed — the poisoning gap is orthogonal to that fix).

[russfellows]

Following up with corrections to my own report above. Two things were wrong: the
workaround I described cannot actually work, and I under-reported the scope —
there are two affected paths, not one.

The workaround I suggested does not work — please don't copy it

I suggested guarding commit() behind an application-level poison latch. It
fails for two independent reasons:

1. It isn't on the only path to the journal. Keyspace::insert / remove /
   remove_weak consult is_poisoned and nothing else, so a direct write walks
   straight past a latch that only wraps commit(). This isn't a lesser case
   either: Writer::write_raw emits the same Start/item/End framing
   write_batch does, so JournalBatchReader discards it identically. I
   reproduced it — a Keyspace::insert issued after a failed batch returns
   Ok(()) and is gone after reopen, with the latch installed and active.
2. The ordering is wrong by construction. Any wrapper outside the crate
   necessarily checks its flag before commit() takes the journal writer
   mutex, which is the TOCTOU your own source warns about at each of those call
   sites (// IMPORTANT: Check the poisoned flag after getting journal mutex, otherwise TOCTOU). Two committers can both observe an unpoisoned latch, and
   the one still waiting on the mutex then proceeds after the other has already
   torn the tail.

So the ordering inside the call is load-bearing, and the fix has to live there.

There are two gaps, not one

I originally called commit() the lone outlier. That was wrong, and the way it
was wrong is worth mentioning: I found the paths by searching for
is_poisoned, which structurally can only return the ones that already poison.
Enumerating instead from the journal writer's own mutation methods gives five
tail-appending paths:

writer method	public entry point	poisons in 3.1.8
write_raw	Keyspace::insert	yes
write_raw	Keyspace::remove	yes
write_raw	Keyspace::remove_weak	yes
write_batch	WriteBatch::commit	no
write_clear	Keyspace::clear	no

(persist and rotate don't append records and can't tear the tail;
Database::persist poisons at its own call site.)

The fixes

Both are the same shape as the three that are already correct.

In WriteBatch::commit() (src/batch/mod.rs):

journal_writer
    .write_batch(self.data.iter(), self.data.len(), batch_seqno)
    .inspect_err(|_| {
        self.db.is_poisoned.poison();
    })?;

In Keyspace::clear() (src/keyspace/mod.rs):

journal_writer
    .write_clear(self.id, seqno)
    .inspect_err(|_| {
        self.is_poisoned.poison();
    })?;

Because the poison check is already inside the journal mutex, this closes the
direct-write case and the TOCTOU together.

Keyspace::clear's gap is much narrower than commit()'s, which may matter for
how you prioritize it. A clear record is tiny and fixed-size, so unlike a batch
carrying a large value it can never exceed JOURNAL_BUFFER_BYTES and force a
write(2) of its own. With default settings clear() therefore only reaches
disk through the trailing persist(Buffer) — which does poison — so the gap
is unreachable. It opens only under
CreateOptions::manual_journal_persist(true), where that persist is skipped and
write_clear's only route to disk is flushing an already-nearly-full buffer.

Reproduction

Both tests assert the contract rather than the mechanism — an operation that
returned Ok is still reflected after reopening — driven by an LD_PRELOAD
interposer on write(2) scoped to the journal fd.

• commit(): fail one write(2) during a batch carrying an oversized,
  incompressible value, then attempt a second write both ways (commit() and
  Keyspace::insert). Both report Ok on stock 3.1.8 and are gone after reopen.
• clear(): fill the journal buffer so clear() must flush, then short-write
  that flush so the tail tears mid-record. Failing the call outright proves
  nothing here — the file would just end on a clean record boundary. Then a
  following write reports Ok and is discarded by recovery.

Both fail against stock 3.1.8 and pass with the patches above. For the clear()
case the filler size that lands the flush inside clear() depends on entry
encoding, so the test sweeps sizes rather than hard-coding one.

One request beyond the fixes

A read-only accessor, something like:

#[must_use]
pub fn is_poisoned(&self) -> bool {
    self.is_poisoned.is_poisoned()
}

Right now an embedding application cannot distinguish "this one write failed"
from "the database is unusable until the process restarts" except by
pattern-matching Error::Poisoned at every call site and hoping none were
missed — the same fragility that sank the latch above. Having the signal
directly lets an application refuse further writes at its own boundary and
report its health accurately, rather than maintaining a parallel guess at
Fjall's state.

What we're doing in the meantime

The latch is gone. We now carry a patched copy of 3.1.8 in-tree via
[patch.crates-io] — the pristine published source as one commit, then the
changes above on top, so the divergence stays reviewable as a few small hunks.
We'll drop it as soon as a release carries the fixes.

Happy to open a PR with the two one-liners, the accessor, and the regression
tests rewritten against your harness and conventions. Just say the word — and
say so if you'd rather the tests land differently, or not at all.