Separate rejected stream writes from transport failures

[derekste]

Distinguish stream writes rejected by Redis from writes whose transport outcome is unavailable. Single-item APIs return RA_REJECTED for known refusals and RA_NOT_CONNECTED for unavailable transport; !ok() covers both. Structured low-level write and trim results retain error text. This PR depends on the revised subscription layer in #108.

READONLY remains a known refusal and refreshes connections for later calls. Ordinary rejections do not reconnect. On standalone Redis, ambiguous writes are not replayed. RedisCluster's internal retry policy remains upstream behavior.

Batches retain accepted timestamps and stop at the first unavailable item or READONLY refusal so later explicit IDs are not sent past the failure. Exact final trimming is selectable; failed final trims preserve the accepted results. The legacy timestamp vector does not report trim success, while xtrimResult() exposes its status. Shared sentinels/equality compile in mock builds, and Python is optional for native test builds.

Validation: 75/75 Linux Release/C++20/Redis 7.4.2 CTest cases passed. The macOS suite also passed with Python optimization enabled, 150 ms proxy delays and unrelated health traffic; Redis 7.4-only checks ran on Linux. Proxy tests verify no replay after lost XADD/XTRIM replies, fail-fast mixed batches, rejected final trims without spurious reconnect, and READONLY recovery. Configuring native tests without Python succeeds.

[derekste]

Follow-up review, 30 September 2026: the incremental change is a justified, focused bugfix. Redis rejection should not launch reconnect, and partial batch acceptance should not conceal a different item's transport failure. No additional blocking defect was found in this diff.

Fresh native checks against private standalone Redis passed: lifecycle, single/batch rejection classification, and the proxy test that drops successful write replies. The proxy confirmed no replay of the ambiguously accepted commands.

The stack remains blocked by #108's callback-cleanup deadlock (#112). Retarget and revalidate after the corrected parent merges; independent Instrumentation approval is still required. This review does not establish the cause of the historical cache/allocator report.

[bigsamich]

Reviewed at 904e0ef and at the stack head, in the same isolated setup as #108. The write-path split is correct and very well tested. The fault-injecting proxy, which lets Redis accept an XADD and then drops the reply, makes an excellent regression test: mutation testing confirms it catches the replay mutant, the #105 "accepted item hides the failure" mutant, and a missing recovery.

#105's acceptance criteria all hold:

• A single rejected add returns RA_REJECTED with 0 new connections, and the connection stays usable. On main it returns RA_NOT_CONNECTED and opens 2 new connections.
• An all-rejected batch doesn't reconnect.
• A mixed batch returns the accepted IDs without claiming a transport failure.
• A genuine CLIENT KILL returns RA_NOT_CONNECTED, reconnects, and replays nothing.

Duplicate, backward and 0-0 IDs, WRONGTYPE, NOPERM, OOM and BUSY all classify as rejections: 1000 rejected writes cost 8 connections in total, against 462 on main. The change is source compatible (ok() still works, and the consumers I checked use ok()). ctest passes 17/17. WriteErrors and WriteTransportFailure pass 40/40 each (at 8 CPUs and 1 CPU) and are clean under ASan/UBSan and TSan.

Should fix

1. READONLY after a planned switchover is treated as permanent: 0 of 15 writes land, against 14/15 on main (RedisConnection.hpp:338).
2. A failed reconnect publishes null clients. Subscriptions stall and the first write after the server returns fails, so the adapter's own reconnect makes recovery worse than redis-plus-plus alone would. The mechanism is pre-existing (filed as #117), but this PR owns the reconnect policy (RedisAdapter.cpp:240).
3. WriteErrors asserts on a server-wide counter. CI's own healthcheck will make it flake about 1 run in 4 once this is on main (write_error_test.cpp:103).

Worth fixing

• Rejections aren't actionable: there is no error text, and transient refusals look like bad data (RedisConnection.hpp:338).
• The final-trim path is untested, and batch trims are always approximate (RedisAdapter.cpp:250).
• RA_REJECTED usability in mock builds and with == (RedisAdapter.hpp:67); CHANGELOG category and the "never replay" wording (CHANGELOG.md:13).
• Test robustness:
  • the 100 ms fault timeout, fixed sleeps, and PID-based keys that are never cleaned up (write_error_test.cpp:24);
  • assert-only verdicts in the Python driver (write_failure_test.py:87).
• Nits:
  • the xtrim out-parameter trap (RedisConnection.hpp:355);
  • Python 3 now REQUIRED at configure time (CMakeLists.txt:97);
  • rejected writes still log at LOG_ERR without the key or ID, one line per rejection (same volume as main); LOG_WARNING with the key and ID would be more useful;
  • a question about batches continuing after Unavailable (RedisAdapterTempl.hpp:353).

Scope note (not blocking this PR)

Reads still conflate rejection and transport failure. getSingleValue, getSingleList, getValues, getLists, #108's getStreamSnapshot, rename and connected() all launch a full reconnect on a server error reply. On a healthy server that costs 3 new connections plus a restart of every reader per call, and under MISCONF a single connected() probe blacks out all reads. Filed as #123 (the read-side counterpart of #105). The CHANGELOG should say that this fix covers writes only.

Issues

• Fixes #105 is in the description, but closing keywords only take effect when a PR targets the default branch. Today the base is dev/subscription-lifecycle, and closingIssuesReferences is empty. Merging bottom-up makes it work: once #108 merges, GitHub retargets this PR to main, because delete_branch_on_merge is on. Otherwise, close #105 manually after validation.
• CI: test.yml only runs for PRs into main, so this PR has no checks; the linked manual run is the only evidence. The strict required build-test check will force a fresh run after retargeting (#125 suggests running CI for stacked PRs).

[derekste]

@bigsamich Thanks for exercising the write outcomes and the fault proxy. The no-replay coverage is valuable, and I agree the remaining reconnect and test-isolation issues need attention before merge.

I'll treat READONLY as a known rejection that can refresh the connection for subsequent calls, while keeping ambiguously accepted writes out of automatic retry. A failed reconnect must not publish an unusable replacement client. The rejection result also needs enough diagnostic context to distinguish bad data, ACL denial, and a temporary refusal. The documentation will scope the no-replay claim to the supported standalone path and the write commands this change actually controls.

For explicit-ID batches, I propose stopping at the first Unavailable, preserving the accepted IDs and reporting an incomplete outcome. Continuing with a larger ID can make an earlier uncertain item impossible to submit later. That changes the current test contract, so it needs an explicit compatibility decision and regression coverage.

I'll remove the server-wide connection-counter assumption from WriteErrors, exercise the final-trim outcomes, and strengthen the fault driver's verdicts, timeouts, key cleanup, and mock/API compatibility checks. The Python configure requirement and xtrim argument trap also need to be resolved before the API ships.

#123 remains the separate read/admin/health rejection work. After the corrected #108 lands, this PR needs retargeting to main and a fresh required build-test run; #105 should close only after that validated merge.

The fixes above are in progress. I'll post the changed commits and validation before requesting another code review.

[derekste]

@bigsamich, the write-error revisions are pushed for another review, including the updated #108/#127 foundation.

READONLY now retains the known-rejection result and requests connection refresh for later writes. The shared reply policy retains server error text. Failed replacements preserve existing clients. Explicit-ID batches stop at the first unavailable item or READONLY refusal and return accepted timestamps preceding that point; ordinary rejected items can still be skipped. Exact batch trims are selectable, accepted timestamps survive final-trim failure, and xtrimResult() plus an exact status-pointer overload removes the pointer-to-bool trap.

The tests use unique keys/cleanup and their own proxy counters, so foreign health checks do not affect the verdict. They cover dropped successful XADD and XTRIM replies, a denied final trim, READONLY on old connections, and the mixed-batch prefix. Fixed reconnect sleeps and the 100 ms fault deadline are gone. Python and C++ verdicts remain active with optimization; native test configuration also works without Python. Sentinels and value equality are shared with mock builds. The API guide and changelog now qualify the standalone no-replay guarantee and explain transient rejections and the compatibility change.

Validation: 75/75 Linux Release/C++20/Redis 7.4.2 cases passed. The macOS run also passed with PYTHONOPTIMIZE=1, 150 ms response delays and concurrent foreign health traffic; the Redis 7.4-specific cases passed on Linux.