diff --git a/docs/physical-record-count-refresh.md b/docs/physical-record-count-refresh.md new file mode 100644 index 00000000..07dbb1b4 --- /dev/null +++ b/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. diff --git a/src/abi/ace_exports.cpp b/src/abi/ace_exports.cpp index 815c6c8c..d7c36efa 100644 --- a/src/abi/ace_exports.cpp +++ b/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(); diff --git a/src/drivers/adt/adt_driver.h b/src/drivers/adt/adt_driver.h index 50878b65..67187493 100644 --- a/src/drivers/adt/adt_driver.h +++ b/src/drivers/adt/adt_driver.h @@ -62,8 +62,8 @@ class AdtDriver final : public IDriver { util::Result bump_autoinc(std::uint16_t field_index) override; - void refresh_record_count_from_disk() noexcept override { - (void)refresh_record_count_(); + util::Result refresh_record_count_from_disk() override { + return refresh_record_count_(); } // Drop the sequential read-ahead block (the engine calls this when a peer diff --git a/src/drivers/cache/cached_driver.h b/src/drivers/cache/cached_driver.h index 601d6b88..b8553404 100644 --- a/src/drivers/cache/cached_driver.h +++ b/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 refresh_record_count_from_disk() override { + if (inner_) return inner_->refresh_record_count_from_disk(); + return {}; } TableCacheMode mode() const noexcept { return mode_; } diff --git a/src/drivers/cdx/cdx_driver.cpp b/src/drivers/cdx/cdx_driver.cpp index 60f2cf38..8a8af92e 100644 --- a/src/drivers/cdx/cdx_driver.cpp +++ b/src/drivers/cdx/cdx_driver.cpp @@ -380,22 +380,19 @@ util::Result CdxDriver::refresh_record_count_() { (static_cast(hdr_buf[5]) << 8) | (static_cast(hdr_buf[6]) << 16) | (static_cast(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( - (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( + (sz.value() - hdr_len_) / rec_len_); } return {}; } diff --git a/src/drivers/cdx/cdx_driver.h b/src/drivers/cdx/cdx_driver.h index a3a3b4bc..33f3930d 100644 --- a/src/drivers/cdx/cdx_driver.h +++ b/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 refresh_record_count_from_disk() override { + return refresh_count_shared_(); } // M11.2 — encrypted DBF support. `encrypted()` reflects the diff --git a/src/drivers/driver_trait.h b/src/drivers/driver_trait.h index 312845ea..283b8893 100644 --- a/src/drivers/driver_trait.h +++ b/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 refresh_record_count_from_disk() { return {}; } }; } // namespace openads::drivers diff --git a/src/drivers/ntx/ntx_driver.h b/src/drivers/ntx/ntx_driver.h index ca284991..f986e8aa 100644 --- a/src/drivers/ntx/ntx_driver.h +++ b/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 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 diff --git a/src/engine/table.h b/src/engine/table.h index aa174765..b65db8fc 100644 --- a/src/engine/table.h +++ b/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 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 diff --git a/src/network/session.cpp b/src/network/session.cpp index 69bf8085..f313260d 100644 --- a/src/network/session.cpp +++ b/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(fresh.error().code)); break; + } std::uint32_t rc = tbl->record_count(); reply.opcode = Opcode::GetRecordCountAck; write_u32_le(rc, reply.payload); diff --git a/tests/unit/abi_encryption_test.cpp b/tests/unit/abi_encryption_test.cpp index 54e9bdd9..2061fde8 100644 --- a/tests/unit/abi_encryption_test.cpp +++ b/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(80, "AAAAA")); + auto path = dir.string(); + ADSHANDLE connection = 0, table = 0; + REQUIRE(AdsConnect60(reinterpret_cast(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); +} diff --git a/tests/unit/cdx_peer_append_5000_test.cpp b/tests/unit/cdx_peer_append_5000_test.cpp index 873095f0..272deff6 100644 --- a/tests/unit/cdx_peer_append_5000_test.cpp +++ b/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 +#include #include #include #include @@ -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 h{}; + h[0] = 3; + h[4] = static_cast(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(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 b{{static_cast(n), + static_cast(n >> 8), static_cast(n >> 16), + static_cast(n >> 24)}}; + f.seekp(4); f.write(reinterpret_cast(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(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 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); +}