DBF: physical count refresh correctness and error forwarding

docs/physical-record-count-refresh.md

@@ -0,0 +1,26 @@
+# Physical record-count refresh
+
+For ordinary DBFs, a physical count refresh derives the row count from the
+file body length, matching Harbour DBFCDX's file-size rule. A stale high
+header no longer resurrects rows after a peer shrinks or empties the body;
+a stale low header does not hide complete rows already on disk. Optional
+EOF bytes are excluded by integer division for ordinary record lengths.
+
+Full-table encryption retains fixed-length records. Partial encryption has
+a trailing bitmap and keeps its existing header-count path instead. This
+patch does not change either encryption format.
+
+The driver refresh result now reaches local `AdsGetRecordCount` and the
+server's `GetRecordCount` reply. A failed read/size query or truncated DBF
+returns an error, not a successful stale count. Local caller output is not
+assigned on refresh failure. NTX, ADT and cache wrappers forward the result.
+Existing navigation refresh calls are not broadened into a new error policy.
+
+No remote count-cache policy, `OPENADS_FRESH_COUNTS` switch, ordered-position
+optimization, protocol change or Harbour RDD-core edit is included. A client
+cache hit still follows the existing policy and may miss a peer append;
+this patch corrects the value and error result when a physical refresh occurs.
+
+This work is carried in Pritpal Bedi's mtfix41 fork release. The upstream
+regressions cover stale headers, peer append/shrink, empty/EOF-only bodies,
+truncation errors on local and direct-wire paths, and encrypted counts.

src/abi/ace_exports.cpp

@@ -13141,7 +13141,8 @@ UNSIGNED32 ENTRYPOINT AdsGetRecordCount(ADSHANDLE hTable, UNSIGNED16 bFilterOpti
         // Multiuser: peer appends bump the on-disk header; re-read so
         // LastRec() / RecCount() match the other stations (and so the
         // browser's EOF fence is not stuck at open-time count).
-        t->refresh_record_count_from_disk();
+        if (auto fresh = t->refresh_record_count_from_disk(); !fresh)
+            return fail(fresh.error());
         *pulRecordCount = t->record_count();
     }
     return ok();

src/drivers/adt/adt_driver.h

@@ -62,8 +62,8 @@ class AdtDriver final : public IDriver {
     util::Result<std::uint32_t>
         bump_autoinc(std::uint16_t field_index) override;
 
-    void refresh_record_count_from_disk() noexcept override {
-        (void)refresh_record_count_();
+    util::Result<void> refresh_record_count_from_disk() override {
+        return refresh_record_count_();
     }
 
     // Drop the sequential read-ahead block (the engine calls this when a peer

src/drivers/cache/cached_driver.h

@@ -60,8 +60,9 @@ class CachedDriver final : public IDriver {
         bump_autoinc(std::uint16_t field_index) override;
 
     void invalidate_read_cache() noexcept override {}
-    void refresh_record_count_from_disk() noexcept override {
-        if (inner_) inner_->refresh_record_count_from_disk();
+    util::Result<void> refresh_record_count_from_disk() override {
+        if (inner_) return inner_->refresh_record_count_from_disk();
+        return {};
     }
 
     TableCacheMode mode() const noexcept { return mode_; }

src/drivers/cdx/cdx_driver.cpp

@@ -380,22 +380,19 @@ util::Result<void> CdxDriver::refresh_record_count_() {
                  (static_cast<std::uint32_t>(hdr_buf[5]) <<  8) |
                  (static_cast<std::uint32_t>(hdr_buf[6]) << 16) |
                  (static_cast<std::uint32_t>(hdr_buf[7]) << 24);
-    // Harbour DBFCDX derives the record count from the FILE SIZE
-    // (hb_dbfCalcRecCount) and flushes the header lazily — a peer's
-    // just-appended record can be on disk while the header still lags.
-    // Reading the header alone then hands the same recno to two writers
-    // (one record overwritten, both keys in the index: 299 rows / 300
-    // keys in the mixed stress test). Take the max of both views; zap()
-    // truncates the file, so the size-derived count can never resurrect
-    // zapped rows.
+    // Match Harbour hb_dbfCalcRecCount for ordinary DBFs: the physical
+    // file length, not the header count, is authoritative. Integer division
+    // discards the optional EOF byte for ordinary record lengths. A partial
+    // encryption bitmap follows the records, so that format retains its
+    // header-count path. Full-table encryption keeps fixed-length records.
     if (!partial_enc_ && rec_len_ != 0) {
-        // (a partially-encrypted table keeps a bitmap past the EOF record,
-        // so its file size does not track the record count)
-        if (auto sz = file_.size(); sz && sz.value() > hdr_len_) {
-            const std::uint32_t size_count = static_cast<std::uint32_t>(
-                (sz.value() - hdr_len_) / rec_len_);
-            if (size_count > rec_count_) rec_count_ = size_count;
+        auto sz = file_.size();
+        if (!sz) return sz.error();
+        if (sz.value() < hdr_len_) {
+            return util::Error{5103, 0, "DBF body shorter than header", ""};
         }
+        rec_count_ = static_cast<std::uint32_t>(
+            (sz.value() - hdr_len_) / rec_len_);
     }
     return {};
 }

src/drivers/cdx/cdx_driver.h

@@ -39,8 +39,8 @@ class CdxDriver final : public IDriver {
         bump_autoinc(std::uint16_t field_index) override;
 
     void invalidate_read_cache() noexcept override { invalidate_read_cache_(); }
-    void refresh_record_count_from_disk() noexcept override {
-        (void)refresh_count_shared_();
+    util::Result<void> refresh_record_count_from_disk() override {
+        return refresh_count_shared_();
     }
 
     // M11.2 — encrypted DBF support. `encrypted()` reflects the

src/drivers/driver_trait.h

@@ -98,7 +98,7 @@ class IDriver {
     // when answering GetRecordCount to keep the value fresh across
     // concurrent clients.  Default no-op (drivers that cache the count
     // in rec_count_ must override).
-    virtual void refresh_record_count_from_disk() noexcept {}
+    virtual util::Result<void> refresh_record_count_from_disk() { return {}; }
 };
 
 } // namespace openads::drivers

src/drivers/ntx/ntx_driver.h

@@ -15,8 +15,8 @@ class NtxDriver final : public IDriver {
 
     std::uint32_t record_count() const noexcept override
     { return inner_.record_count(); }
-    void refresh_record_count_from_disk() noexcept override
-    { inner_.refresh_record_count_from_disk(); }
+    util::Result<void> refresh_record_count_from_disk() override
+    { return inner_.refresh_record_count_from_disk(); }
     std::uint16_t record_length() const noexcept override
     { return inner_.record_length(); }
     std::uint16_t header_length() const noexcept override

src/engine/table.h

@@ -146,11 +146,11 @@ class Table {
     // Re-read the on-disk record count so concurrent appends by other
     // connections become visible.  Called by the server before answering
     // GetRecordCount and by AdsRefreshRecord.
-    void refresh_record_count_from_disk() noexcept {
+    util::Result<void> refresh_record_count_from_disk() {
         // A refresh is the moment another station's appends and deletes
         // become visible, so any cached live count taken before it is stale.
         bump_live_gen();
-        driver_->refresh_record_count_from_disk();
+        return driver_->refresh_record_count_from_disk();
     }
     // Clipper / SAP-ACE convention: phantom position past last
     // record reports recno()=LastRec()+1, so an empty table reads

src/network/session.cpp

@@ -2653,7 +2653,8 @@ DispatchResult Session::dispatch(const Frame& f) {
             std::uint32_t id = read_u32_le(f.payload.data());
             if (auto cit = cursor_tbls_.find(id); cit != cursor_tbls_.end()) {
                 UNSIGNED32 rc = 0;
-                AdsGetRecordCount(cit->second, 0, &rc);
+                auto count_status = AdsGetRecordCount(cit->second, 0, &rc);
+                if (count_status != 0) { reply = err("GetRecordCount: refresh failed", count_status); break; }
                 reply.opcode = Opcode::GetRecordCountAck;
                 write_u32_le(rc, reply.payload);
                 break;
@@ -2666,7 +2667,9 @@ DispatchResult Session::dispatch(const Frame& f) {
             if (!tbl) { reply = err("GetRecordCount: lookup failed"); break; }
             // Refresh the on-disk record count so concurrent appends by
             // other connections are visible (multiuser coherence).
-            tbl->refresh_record_count_from_disk();
+            if (auto fresh = tbl->refresh_record_count_from_disk(); !fresh) {
+                reply = err(fresh.error().message, static_cast<UNSIGNED32>(fresh.error().code)); break;
+            }
             std::uint32_t rc = tbl->record_count();
             reply.opcode = Opcode::GetRecordCountAck;
             write_u32_le(rc, reply.payload);

tests/unit/abi_encryption_test.cpp

@@ -76,6 +76,9 @@ TEST_CASE("M11.2 encrypt + reopen with correct password roundtrips") {
     REQUIRE(AdsOpenTable(hConn, leaf, nullptr, ADS_CDX,
                          0, 0, 0, 0, &hTable) == 0);
     REQUIRE(AdsEncryptTable(hTable) == 0);
+    UNSIGNED32 physical_count = 0;
+    REQUIRE(AdsGetRecordCount(hTable, ADS_IGNOREFILTERS, &physical_count) == 0);
+    CHECK(physical_count == 3); // full encryption retains fixed-length records
     REQUIRE(AdsCloseTable(hTable) == 0);
 
     // Header byte on disk must now be 0xC3.
@@ -169,6 +172,9 @@ TEST_CASE("M11.2 record-level encrypt: single record encrypted, others plain") {
 
     REQUIRE(AdsGotoRecord(hTable, 2) == 0);
     REQUIRE(AdsEncryptRecord(hTable) == 0);
+    UNSIGNED32 physical_count = 0;
+    REQUIRE(AdsGetRecordCount(hTable, ADS_IGNOREFILTERS, &physical_count) == 0);
+    CHECK(physical_count == 3); // trailing partial-encryption bitmap is not rows
 
     CHECK(AdsIsTableEncrypted(hTable, &enc) == 0);
     CHECK(enc == 1);
@@ -209,3 +215,33 @@ TEST_CASE("M11.2 record-level encrypt: single record encrypted, others plain") {
     REQUIRE(AdsDisconnect(hConn) == 0);
     fs::remove_all(dir, ec);
 }
+
+TEST_CASE("Physical count: partial encryption bitmap does not become extra rows") {
+    auto dir = fs::temp_directory_path() / "openads_physical_count_bitmap";
+    std::error_code ec;
+    fs::remove_all(dir, ec); fs::create_directories(dir);
+    write_plain_dbf(dir / "data.dbf", std::vector<std::string>(80, "AAAAA"));
+    auto path = dir.string();
+    ADSHANDLE connection = 0, table = 0;
+    REQUIRE(AdsConnect60(reinterpret_cast<UNSIGNED8*>(path.data()),
+        ADS_LOCAL_SERVER, nullptr, nullptr, 0, &connection) == AE_SUCCESS);
+    UNSIGNED8 password[] = "count-test", name[] = "data.dbf";
+    REQUIRE(AdsSetEncryptionPassword(connection, password) == AE_SUCCESS);
+    REQUIRE(AdsOpenTable(connection, name, nullptr, ADS_CDX,
+        0, 0, 0, 0, &table) == AE_SUCCESS);
+    REQUIRE(AdsGotoRecord(table, 80) == AE_SUCCESS);
+    REQUIRE(AdsEncryptRecord(table) == AE_SUCCESS);
+    // At 80 rows the bitmap is longer than one six-byte record. Treating
+    // this format like an ordinary DBF would report phantom extra rows.
+    UNSIGNED32 count = 0;
+    REQUIRE(AdsGetRecordCount(table, ADS_IGNOREFILTERS, &count) == AE_SUCCESS);
+    CHECK(count == 80);
+    REQUIRE(AdsCloseTable(table) == AE_SUCCESS);
+    REQUIRE(AdsOpenTable(connection, name, nullptr, ADS_CDX,
+        0, 0, 0, 0, &table) == AE_SUCCESS);
+    REQUIRE(AdsGetRecordCount(table, ADS_IGNOREFILTERS, &count) == AE_SUCCESS);
+    CHECK(count == 80);
+    REQUIRE(AdsCloseTable(table) == AE_SUCCESS);
+    REQUIRE(AdsDisconnect(connection) == AE_SUCCESS);
+    fs::remove_all(dir, ec);
+}

tests/unit/cdx_peer_append_5000_test.cpp

@@ -1,7 +1,11 @@
 #include "doctest.h"
 #include "openads/ace.h"
 #include "drivers/cdx/cdx_driver.h"
+#include "network/client.h"
+#include "network/server.h"
 
+#include <array>
+#include <fstream>
 #include <cstdint>
 #include <cstring>
 #include <filesystem>
@@ -88,3 +92,130 @@ TEST_CASE("CdxDriver fetch sees a peer's appended record (no spurious 5000)") {
 
     fs::remove_all(dir, ec);
 }
+
+// Focused physical-count regressions. Direct wire requests intentionally avoid
+// changing the separate ACE client count-cache policy.
+namespace {
+void count_seed(const fs::path& p, std::uint32_t header_count = 3) {
+    std::array<std::uint8_t, 65> h{};
+    h[0] = 3;
+    h[4] = static_cast<std::uint8_t>(header_count);
+    h[8] = 65;
+    h[10] = 6;
+    h[32] = 'V'; h[43] = 'C'; h[48] = 5; h[64] = 0x0D;
+    std::ofstream f(p, std::ios::binary);
+    f.write(reinterpret_cast<const char*>(h.data()), h.size());
+    for (int n = 0; n < 3; ++n) f.write(" AAAAA", 6);
+    f.put(0x1A);
+}
+void count_header(const fs::path& p, std::uint32_t n) {
+    std::fstream f(p, std::ios::in | std::ios::out | std::ios::binary);
+    std::array<std::uint8_t, 4> b{{static_cast<std::uint8_t>(n),
+        static_cast<std::uint8_t>(n >> 8), static_cast<std::uint8_t>(n >> 16),
+        static_cast<std::uint8_t>(n >> 24)}};
+    f.seekp(4); f.write(reinterpret_cast<const char*>(b.data()), b.size());
+}
+fs::path count_dir(const char* name) {
+    auto d = fs::temp_directory_path() / name;
+    std::error_code ec; fs::remove_all(d, ec); fs::create_directories(d);
+    return d;
+}
+}
+
+TEST_CASE("Physical count: ordinary DBF refresh follows body size, not stale header") {
+    const auto dir = count_dir("openads_physical_count_body");
+    const auto p = dir / "data.dbf";
+    count_seed(p);
+    {
+        CdxDriver driver;
+        REQUIRE(driver.open(p.string(), DriverOpenMode::Shared).has_value());
+        count_header(p, 99);
+        REQUIRE(driver.refresh_record_count_from_disk().has_value());
+        CHECK(driver.record_count() == 3);
+        count_header(p, 1);
+        REQUIRE(driver.refresh_record_count_from_disk().has_value());
+        CHECK(driver.record_count() == 3);
+        // A peer's shrink with a stale high header must not resurrect rows.
+        count_header(p, 99);
+        fs::resize_file(p, 65 + 6);
+        REQUIRE(driver.refresh_record_count_from_disk().has_value());
+        CHECK(driver.record_count() == 1);
+        fs::resize_file(p, 65);
+        REQUIRE(driver.refresh_record_count_from_disk().has_value());
+        CHECK(driver.record_count() == 0);
+        fs::resize_file(p, 65 + 1); // optional EOF byte, no records
+        REQUIRE(driver.refresh_record_count_from_disk().has_value());
+        CHECK(driver.record_count() == 0);
+        fs::resize_file(p, 64); // enough bytes to read count, short DBF header
+        auto short_body = driver.refresh_record_count_from_disk();
+        REQUIRE_FALSE(short_body.has_value());
+        CHECK(short_body.error().code == 5103);
+        fs::resize_file(p, 4);
+        auto short_header = driver.refresh_record_count_from_disk();
+        REQUIRE_FALSE(short_header.has_value());
+        CHECK(short_header.error().code == 5103);
+    }
+    fs::remove_all(dir);
+}
+
+TEST_CASE("Physical count: local ACE returns refresh errors without outputting stale count") {
+    const auto dir = count_dir("openads_physical_count_ace");
+    const auto p = dir / "data.dbf";
+    count_seed(p);
+    auto path = dir.string();
+    ADSHANDLE connection = 0, table = 0;
+    REQUIRE(AdsConnect60(reinterpret_cast<UNSIGNED8*>(path.data()),
+        ADS_LOCAL_SERVER, nullptr, nullptr, 0, &connection) == AE_SUCCESS);
+    UNSIGNED8 name[] = "data.dbf";
+    REQUIRE(AdsOpenTable(connection, name, nullptr, ADS_CDX, ADS_ANSI,
+        ADS_COMPATIBLE_LOCKING, ADS_IGNORERIGHTS, ADS_SHARED, &table) == AE_SUCCESS);
+    count_header(p, 99);
+    UNSIGNED32 count = 0;
+    REQUIRE(AdsGetRecordCount(table, ADS_IGNOREFILTERS, &count) == AE_SUCCESS);
+    CHECK(count == 3);
+    fs::resize_file(p, 4);
+    count = 0xDEADBEEFu;
+    CHECK(AdsGetRecordCount(table, ADS_IGNOREFILTERS, &count) == 5103);
+    CHECK(count == 0xDEADBEEFu);
+    CHECK(AdsCloseTable(table) == AE_SUCCESS);
+    CHECK(AdsDisconnect(connection) == AE_SUCCESS);
+    fs::remove_all(dir);
+}
+
+TEST_CASE("Physical count: direct remote refresh sees peer append and forwards disk errors") {
+    const auto dir = count_dir("openads_physical_count_wire");
+    const auto p = dir / "data.dbf";
+    count_seed(p);
+    openads::network::Server server;
+    REQUIRE(server.start("127.0.0.1", 0).has_value());
+    openads::network::RemoteConnection client;
+    REQUIRE(client.connect("127.0.0.1", server.port(), dir.string()).has_value());
+    auto opened = client.open_table("data.dbf");
+    REQUIRE(opened.has_value());
+    const auto id = opened.value().id;
+    REQUIRE(client.goto_top(id).has_value());
+    auto count = client.record_count(id);
+    REQUIRE(count.has_value()); CHECK(count.value() == 3);
+    {
+        CdxDriver peer;
+        REQUIRE(peer.open(p.string(), DriverOpenMode::Shared).has_value());
+        std::vector<std::uint8_t> row(peer.record_length(), ' ');
+        REQUIRE(peer.append_record_raw(row.data(), row.size()).has_value());
+    }
+    // Make the header lag behind the new physical row, then overstate it.
+    count_header(p, 3);
+    count = client.record_count(id);
+    REQUIRE(count.has_value()); CHECK(count.value() == 4);
+    count_header(p, 99);
+    count = client.record_count(id);
+    REQUIRE(count.has_value()); CHECK(count.value() == 4);
+    fs::resize_file(p, 65); // peer emptied the body without updating header
+    count = client.record_count(id);
+    REQUIRE(count.has_value()); CHECK(count.value() == 0);
+    fs::resize_file(p, 4);
+    count = client.record_count(id);
+    REQUIRE_FALSE(count.has_value()); CHECK(count.error().code == 5103);
+    CHECK(client.close_table(id).has_value());
+    client.disconnect(); server.stop();
+    fs::remove_all(dir);
+}