HarperFast / HarperFast/harper-pro

Add fault-injection hook for in-replicateOverWS close() paths in integration tests

Open
#155 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
3
Forks
0
Avg merge
1d 21h
Merged PRs (30d)
80

Description

## Summary

The new tests in `integrationTests/cluster/replicationReconnect.test.mjs` (introduced by https://github.com/HarperFast/harper-pro/pull/154) cover the user-driven `unsubscribe()` teardown and the generic transient retry path (TCP close after `killHarper`). They do **not** cover the protocol-level `close(...)` paths inside `replicateOverWS`, which were the actual regression that #154 fixes:

- peer-initiated `DISCONNECT` (line 622-623)
- `close(1008, 'Unauthorized')` after WS open (500, 514, 1033, 1044)
- `close(1008, 'Node name mismatch')` (544)
- `close(1008, 'Invalid sequence id ...')` (1416)
- `close(1008, 'Error handling subscription to node ...')` (1550)
- the 60s empty-subscription delayed close (2113)

These all formerly set `ws.isFinished = true` and emit `'finished'`, killing the connection silently with no retry. The fix changes that for everything except the 60s delayed close. Without a way to trigger one of these closes from an integration test, the specific regression isn't directly guarded.

## What's needed

A way for an integration test to deterministically cause one of these closes on a live replication connection. Some options, in increasing order of intrusiveness:

1. **Test-only operation** that injects a fault on the next replication message from a specific peer (e.g., flips the next received command id to an unknown one, triggering `close()` at line 1389). Gated behind an env var so it can't ship in prod.
2. **Hook in `replicateOverWS`** that takes an optional `_testHooks` object on the options bag, allowing tests to override behavior (return a fake `setDatabase`, force a NODE_NAME mismatch, etc.). Slightly more invasive but more flexible.
3. **A bare WS client helper** that knows the `harperdb-replication-v1` subprotocol enough to perform the NODE_NAME handshake and then send a raw `[142]` (DISCONNECT) command. Use it as the "peer" in a test where the real Harper node is the outgoing client — verify the outgoing client retries instead of silently dying. Closest to a real-world repro and doesn't touch prod code.

Option 3 is probably the right call — it doesn't require any production-code changes and reproduces the exact bug shape.

## Acceptance

Once the helper exists, extend `replicationReconnect.test.mjs` (or add a sibling) with at least one test that:

1. Stands up a real Harper node.
2. Connects to its replication endpoint as a fake peer, completes the NODE_NAME handshake.
3. Sends a `DISCONNECT` command frame.
4. Asserts the real node logs a transient close, schedules a retry, and successfully re-handshakes within a bounded time window.

## Why now

Pro currently has no unit-test runner (`AGENTS.md` is explicit; only `npm run test:integration` exists), and the existing integration harness has no fault-injection. The bug fixed in #154 took live CDP debugging to find — having a repro in CI would have caught it years earlier.

Filed as follow-up from https://github.com/HarperFast/harper-pro/pull/154.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.