EloqDSS reports failed checkpoint writes as durable → WAL truncated over un-written data (data loss)

[liunyl]

Two spots in the EloqDSS write path return NO_ERROR even though the write did not durably land. Because PutAll success drives checkpoint-ts advancement and WAL truncation, a failed checkpoint batch is treated as durable and the redo log covering it is purged → silent committed-data loss. This affects the default RocksDB-backed builds, including single-node.

Defect 1 — RPC channel failure after retries leaves the result NO_ERROR

store_handler/data_store_service_client_closure.h:1884-1942, BatchWriteRecordsClosure::Run():

if (cntl_.Failed()) {
    if (cntl_.ErrorCode() != EOVERCROWDED && ... != ERPCTIMEDOUT) {
        ds_service_client_->UpdateOwnerNodeIndexOfShard(...);
        need_retry = true;                     // <-- no result_.set_error_code(...)
    } else {
        result_.set_error_code(NETWORK_ERROR); // only the timeout branch sets an error
    }
}
...
if (need_retry && retry_count_ < retry_limit_) { ...retry...; return; }
(*callback_)(callback_data_, this, *ds_service_client_, result_);  // result_ still NO_ERROR after retries exhausted

When retries are exhausted (retry_limit_ = 2), result_ is whatever it was cleared to (NO_ERROR). Every other closure sets an error on this path; only BatchWriteRecords forgets. The caller sees success → PutAll returns true.

Defect 2 — RocksDB write/flush error is unconditionally overwritten

store_handler/eloq_data_store_service/rocksdb_data_store_common.cpp:736-737:

if (!write_status.ok()) {
    result.set_error_code(WRITE_FAILED);  result.set_error_msg(...);
} else if (!batch_write_req->SkipWal()) {
    auto flush_status = db->Flush(flush_options);
    if (!flush_status.ok()) { result.set_error_code(FLUSH_FAILED); ... }
}
result.set_error_code(NO_ERROR);          // <-- clobbers WRITE_FAILED / FLUSH_FAILED
batch_write_req->SetFinish(result);

A disk-full / IO error on the write or the synchronous flush (note: these RocksDB DSS backends run with disableWAL=true, so !SkipWal() means "synchronous full Flush" — there is no DSS WAL) is reported as success. Also masks failures of skip_wal=false catalog/range metadata writes.

Impact

Checkpoint reports a ckpt ts ≥ data that was never persisted → UpdateCheckpointTs lets the log service purge the redo below it → crash/failover loses those committed writes with no error surfaced.

Fix: set the error code on the exhausted-retry path; make the trailing set_error_code(NO_ERROR) conditional on no prior error.

---

Found during a code audit (PR #493). Verified against source at the cited lines.

🤖 Found with Claude Code