Skip to content

[Feature] No reusable failing-journal or failing-snapshot-store double exists, so a throwing append inside persistAll, a snapshot write that fails after a successful append, and recovery over a gapped stream are all untested #1024

Description

@pathosDev

Use case

Every persistence test in the repository runs against a store that works. There is no Journal or SnapshotStore test double whose operations can be made to fail, so no test in the suite has ever seen PersistentActor react to a store error.

The complete inventory of persistence doubles under tests/:

  • GatedJournal (tests/unit/fsm/PersistentFSM.test.ts:741) — parks read until the test releases it. Delays, never fails.
  • CountingStore (tests/integration/in-process/persistence/CachedSnapshotStore.test.ts:16) — counts calls.
  • noBusJournal (tests/integration/in-process/persistence/query/PushBasedQuery.test.ts:154) — a two-method stub for a query test.
  • FlakyBatchClient (tests/integration/in-process/persistence/CassandraJournalConcurrency.test.ts:160) — 14 lines, the only failure injector in the suite. It fails the Cassandra driver's batch(), one level below the Journal interface, and it exists to test CassandraJournal's claim-release logic. Nothing above the journal ever sees it.

The one test that does exercise a recovery failure gets there through data rather than through a store: it writes a snapshot claiming a sequence number ahead of the journal, so assertTrustworthySnapshot refuses it. Its helper's JSDoc records the choice — "No throwing-store fake needed." That is true for the case it covers and is why the gap has stayed invisible.

What is consequently untested, all of it reachable from ordinary framework code:

  • append throwing inside persistAll. There is no onPersistFailure hook; the rejection propagates out of persistAll, out of onCommand, and into supervision as an ordinary actor failure. Whether the actor restarts, what its recovered state is, and what happens to the messages stashed by persistAll's finally block are all unverified. persistAll has exactly one test, and it is the happy path.
  • A snapshot write failing while the journal append succeeded. saveSnapshotNow() is awaited inside persistAll, after this._state and this._seq have already advanced for every written event. A store that rejects there aborts the rest of persistAll — the user's persist(event, cb) callback never runs, and neither does the pending-callback drain — even though the event is durably written and the in-memory state already reflects it. No test covers this ordering.
  • Recovery over a gapped stream. A journal that returns events 1, 2, 4 (a partially-applied batch, a manual repair, a store that lost a row) exercises the replay fold's sequence tracking. Nothing produces one.
  • A read failing part-way through recovery. GatedJournal proves the harness can park a read; nothing makes one reject.

The absence also blocks tests filed elsewhere. The replay-mutation fuzzer's corruption property currently asserts against a local closure because there is no way to make a real journal hand back a bad event; the fix for that issue depends on this one.

Proposed shape

A single reusable decorator, in the testkit rather than in one test file, scripted rather than random.

  • FaultInjectingJournal and FaultInjectingSnapshotStore, each wrapping any real implementation and configured through the project's XOptions shape — FaultInjectingJournalOptions with withFailOn('append' | 'read' | 'highestSeq' | 'delete'), withFailAfterCalls(n), withFailOnce(), withError(() => new Error(…)), and a withGapAfterSeq(n) for the gapped-stream case. Scripted, deterministic, no probability — a persistence failure test must be reproducible.
  • Layer it over InMemoryJournal, so the failure is injected at the Journal seam the framework actually calls, not at a driver seam that only one backend has.
  • Add the scenarios to the shared contract suite (tests/integration/brokers/lib/persistence-contract/), whose 14 journal scenarios today all describe a working store. A failure scenario there runs against every backend at once, which is where it belongs — "an append that throws leaves the head unchanged" is a contract claim, not an in-memory one.
  • Write the four actor-level tests listed above against the decorator. They are the point of the exercise; the decorator on its own is scaffolding.

persist has no failure hook today. If these tests show that a store error should be surfaceable to user code rather than only to supervision, that is a follow-up feature issue — this one asks for the ability to find out.

Acceptance

  • A FaultInjectingJournal / FaultInjectingSnapshotStore decorator exists, is scripted (not probabilistic), and wraps any implementation.
  • It sits at the Journal / SnapshotStore interface, so every backend and every framework path above it is covered by one double.
  • The shared persistence contract suite gains failure scenarios, so all backends run them.
  • A test covers append throwing inside persistAll and asserts the resulting supervision outcome and recovered state.
  • A test covers a snapshot save failing after a successful journal append, and asserts what happens to the persist callback.
  • A test covers recovery over a stream with a sequence-number gap.
  • EN and DE docs describe the double.

Verification status

Found in the ten-lens production-readiness review of 2026-08-05 (v0.13.0) and re-verified before filing: confirmed by reading. The inventory came from grep -rn "implements Journal\|implements SnapshotStore\|: Journal\b\|extends InMemoryJournal\|extends InMemorySnapshotStore" tests/ plus a sweep for class *(Flaky|Failing|Throwing|Broken|Faulty)* across tests/ and src/; each of the four hits was read. grep for spyOn / mock( over the persistence and unit trees returns nothing, so there is no monkey-patched failure path either. The persistAll ordering above is read from the source at src/persistence/PersistentActor.ts:262-307; no probe was run, and the issue makes no claim about what the framework does in these cases — only that nothing observes it.

The only injector in the suite, in full:

tests/integration/in-process/persistence/CassandraJournalConcurrency.test.ts:158-173
describe('CassandraJournal — a failed event batch releases its claim', () => {
  /** Fails the Nth batch, so the claim is committed but the events never land. */
  class FlakyBatchClient extends FakeCassandraClient {
    failNextBatch = false;

    override async batch(
      queries: ReadonlyArray<CassandraBatchQuery>,
      options?: { prepare?: boolean; logged?: boolean; consistency?: number },
    ): Promise<void> {
      if (this.failNextBatch) {
        this.failNextBatch = false;
        throw new Error('simulated write timeout');
      }
      return super.batch(queries, options);
    }
  }

The recovery-failure suite's own note that a throwing store was not needed:

tests/integration/in-process/persistence/PersistentActorRecoveryFailure.test.ts:126-133
/**
 * Make recovery fail on data alone.  A snapshot claiming a sequence
 * number far ahead of the journal is refused by
 * `assertTrustworthySnapshot`, so `replayState` throws *before*
 * `PersistentActor` assigns `_state` — which is precisely the window
 * under test.  No throwing-store fake needed.
 */

The whole of persistAll's coverage:

tests/integration/in-process/persistence/PersistentActor.test.ts:190-209
describe('PersistentActor — persistAll atomic batch', () => {
  test('persistAll appends every event with sequential seqs', async () => {
    const { system, journal } = makeSystem();
    class Batch extends PersistentActor<'go', number, number[]> {
      readonly persistenceId = 'batch';
      initialState() { return []; }
      onEvent(s: number[], e: number): number[] { return [...s, e]; }
      async onCommand(_s: unknown, _command: 'go'): Promise<void> {
        await this.persistAll([1, 2, 3]);
      }
    }
    const ref = system.spawn(Batch, 'b');
    ref.tell('go');
    await sleep(30);
    const events = await journal.read<number>('batch', 1);
    expect(events.map(e => e.event)).toEqual([1, 2, 3]);
    expect(events.map(e => e.sequenceNr)).toEqual([1, 2, 3]);
    await system.terminate();
  });
});

and the snapshot call inside it, which is the ordering the second missing test is about:

src/persistence/PersistentActor.ts:286-296
      const policy = this.snapshotPolicy();
      let shouldSnapshot = false;
      for (let i = 0; i < written.length; i++) {
        const pe = written[i]!;
        const domainEvent = events[i]!;  // pre-envelope domain shape
        this._state = this.onEvent(this._state, domainEvent);
        this._seq = pe.sequenceNr;
        if (policy(pe.sequenceNr, this._state, domainEvent)) shouldSnapshot = true;
      }
      if (shouldSnapshot) await this.saveSnapshotNow();
      await cb?.(this._state);

Adjacent: #536 proposes a public persistence testkit — exporting the conformance suite and a FailureInjectingJournal so third-party backend authors can verify their own implementations. Its acceptance criteria are all about export and documentation; none of them requires the framework's own tests to use the injector, so #536 could ship complete and every gap listed above would remain. This issue is the internal-coverage half: build the double, and write the four actor-level tests it unblocks. They should be sequenced together, with this one first — the decorator is the shared dependency. #493 (persistence contract polish) touches the same contract suite. The replay-mutation-fuzz issue in this batch depends on the double for its corruption property.

Part of the production-readiness review batch — tracked in #913.

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or requestpriority: highTop priority — high impact, plan nextproduction-goalBlocks or defines the path to production readiness

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions