From 82fe35a24dad8d8b336a3ceec450b40155dd98ae Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Sat, 10 Oct 2026 01:43:43 +0000 Subject: [PATCH 1/2] Port reviewed capability-gated open metadata --- docs/open-setup-metadata.md | 28 +++++ src/abi/ace_exports.cpp | 15 ++- src/network/client.cpp | 17 ++- src/network/client.h | 5 + src/network/session.cpp | 33 ++++++ src/network/session.h | 1 + src/network/wire.h | 4 + tests/unit/network_teardown_batch_test.cpp | 2 +- tests/unit/network_use_budget_test.cpp | 114 ++++++++++++++++++++- 9 files changed, 212 insertions(+), 7 deletions(-) create mode 100644 docs/open-setup-metadata.md diff --git a/docs/open-setup-metadata.md b/docs/open-setup-metadata.md new file mode 100644 index 00000000..0c797c0f --- /dev/null +++ b/docs/open-setup-metadata.md @@ -0,0 +1,28 @@ +# Capability-gated remote open metadata + +New clients advertise kCapOpenSetupMetadata. A supporting server appends the +immutable record length and, when a production bag opens successfully, its +ordinary OpenIndexAck payload to the existing OpenTableAck TLV sections. +Clients without that capability get the existing reply format unchanged. + +The server reuses its existing OpenIndex handler, including session-local +bindings and index-open rollback. It restores natural order after the setup. +The client parses index metadata through the same parser as an ordinary index +reply, registers it through its existing AdsOpenIndex path and still checks the +caller's output-array capacity. Record length warms the existing handle cache. +Missing or rejected metadata uses the existing wire fallback. + +Before consuming the warm index reply, the client checks the requested bag's +case-insensitive stem against the confirmed production bag. This follows the +existing production-bag dedup convention, including CDX/Z01 spellings. The +production path is set before the first auto-open, so the guard can run there. + +Tests cover CDX and Z01 production bags, initial natural position, explicit +same-bag open, record-length cache, independent login cursors, CloseAll/reopen, +legacy capability gating, and short mandatory index payloads. The per-USE +opcode counters show no separate GetRecordLength or OpenIndex request for a +valid warmed production bag. Server-side setup work still occurs within open; +this is an RTT saving, not proof that all CPU or elapsed startup time improves. + +No count-cache policy, OPENADS_FRESH_COUNTS, daemon-hardening exemption, +Harbour RDD-core change, lock change, data-jail change or CI change is included. diff --git a/src/abi/ace_exports.cpp b/src/abi/ace_exports.cpp index 8d2b5306..0877f31e 100644 --- a/src/abi/ace_exports.cpp +++ b/src/abi/ace_exports.cpp @@ -8524,6 +8524,12 @@ UNSIGNED32 ENTRYPOINT AdsOpenTable(ADSHANDLE hConnect, rt->name = name; rt->alias = std::move(alias); rt->close_counted = true; + rt->prod_bag_path = ot.prod_bag_path; + rt->production_index_reply = std::move(ot.production_index_reply); + if (ot.has_record_length) { + rt->cached_record_length = ot.record_length; + rt->record_length_cached = true; + } rt->open_mode_raw = usMode; rt->open_exclusive = (map_open_mode(usMode) == openads::engine::OpenMode::Exclusive); @@ -15939,7 +15945,14 @@ UNSIGNED32 ENTRYPOINT AdsOpenIndex(ADSHANDLE hTable, UNSIGNED8* pucName, return ok(); } } - auto r = rt->conn->open_index(rt->id, path); + const bool use_open_metadata = !rt->production_index_reply.empty() && + !bag_stem_ci(path).empty() && + bag_stem_ci(path) == bag_stem_ci(rt->prod_bag_path); + auto r = use_open_metadata + ? openads::network::RemoteConnection::parse_open_index_reply( + rt->production_index_reply, path) + : rt->conn->open_index(rt->id, path); + if (use_open_metadata) rt->production_index_reply.clear(); if (!r) return fail(r.error()); auto& s = state(); std::lock_guard lk(s.mu); diff --git a/src/network/client.cpp b/src/network/client.cpp index de97f632..3d859d61 100644 --- a/src/network/client.cpp +++ b/src/network/client.cpp @@ -420,7 +420,7 @@ void connect_pack_payload(std::vector& payload, std::uint32_t caps = kCapPrefetchConsume | kCapPrefetchBackward | kCapOpenTableMode | kCapSetFieldsBatch | kCapFlushInCloseAll | kCapNavOrderFuse - | kCapNavBoundaryPair | kCapLockedRow; + | kCapNavBoundaryPair | kCapOpenSetupMetadata | kCapLockedRow; for (int i = 0; i < 4; ++i) payload.push_back(static_cast((caps >> (8 * i)) & 0xFFu)); } @@ -684,6 +684,14 @@ RemoteConnection::open_table(const std::string& rel, std::uint16_t mode) { result.fields = std::move(sr).value(); result.has_schema = true; } + } else if (tag == OpenTableAckSections::kRecordLength && slen == 4) { + result.record_length = read_u32_le(pl.data() + off); + result.has_record_length = true; + } else if (tag == OpenTableAckSections::kProductionIndex) { + std::vector bytes(pl.begin() + static_cast(off), pl.begin() + static_cast(off+slen)); + if (parse_open_index_reply(bytes, result.prod_bag_path)) { + result.production_index_reply = std::move(bytes); + } } else if (tag == OpenTableAckSections::kFirstRow) { result.first_row.assign( pl.begin() + static_cast(off), @@ -2199,7 +2207,12 @@ RemoteConnection::open_index(std::uint32_t table_id, rep.value().payload.size() < 2) { return util::Error{5000, 0, "OpenIndex: server error", path}; } - const auto& pl = rep.value().payload; + return parse_open_index_reply(rep.value().payload, path); +} + +util::Result> +RemoteConnection::parse_open_index_reply(const std::vector& pl, const std::string& path) { + if (pl.size() < 2) return util::Error{5000, 0, "OpenIndex: short payload", path}; std::uint16_t n = read_u16_le(pl.data()); if (pl.size() < 2u + 4u * n) { return util::Error{5000, 0, diff --git a/src/network/client.h b/src/network/client.h index 3bf5fe04..67715ebb 100644 --- a/src/network/client.h +++ b/src/network/client.h @@ -194,6 +194,9 @@ class RemoteConnection { bool has_schema = false; std::vector first_row; // nav-ack trailer layout bool has_first_row = false; + bool has_record_length = false; + std::uint32_t record_length = 0; + std::vector production_index_reply; }; util::Result open_table(const std::string& rel, std::uint16_t mode = 0); util::Result close_table(std::uint32_t id); @@ -266,6 +269,7 @@ class RemoteConnection { std::uint16_t option, const std::vector& recnos); util::Result get_aof_opt_level(std::uint32_t id); + static util::Result> parse_open_index_reply(const std::vector& pl, const std::string& path); util::Result> open_index(std::uint32_t table_id, const std::string& path); @@ -1014,6 +1018,7 @@ struct RemoteTable { // closes/reopens). Cached on first hit; rddads asks both per USE. bool table_type_cached = false; std::uint16_t cached_table_type = 0; + std::vector production_index_reply; bool record_length_cached = false; std::uint32_t cached_record_length = 0; // Set by a wire AdsGotoRecord that landed on a row: the connection diff --git a/src/network/session.cpp b/src/network/session.cpp index f313260d..ade93452 100644 --- a/src/network/session.cpp +++ b/src/network/session.cpp @@ -1858,6 +1858,7 @@ DispatchResult Session::dispatch(const Frame& f) { client_prefetch_back_ok_ = (caps & openads::network::kCapPrefetchBackward) != 0; // M12.x — client sends [u16 mode] prefix on OpenTable. + client_open_setup_metadata_ok_ = (caps & kCapOpenSetupMetadata) != 0; client_locked_row_ok_ = (caps & kCapLockedRow) != 0 && std::getenv("OPENADS_NO_LOCKED_ROW_CAP") == nullptr; client_open_table_mode_ok_ = @@ -2325,6 +2326,38 @@ DispatchResult Session::dispatch(const Frame& f) { bag.begin(), bag.end()); } append_open_warm_sections(reply.payload, id, tbl); + if (client_open_setup_metadata_ok_ && tbl != nullptr) { + // Reuse the exact OpenIndex handler: per-session bindings, + // rollback on failed index open and unchanged tag metadata. + // Client releases its ABI mutex during OpenTable. + std::vector extra; + unsigned added = 0; + auto add = [&](std::uint8_t tag, const std::vector& bytes) { + extra.push_back(tag); + write_u32_le(static_cast(bytes.size()), extra); + extra.insert(extra.end(), bytes.begin(), bytes.end()); + ++added; + }; + std::vector len; + write_u32_le(tbl->driver() ? tbl->driver()->record_length() : 0, len); + add(OpenTableAckSections::kRecordLength, len); + if (!bag.empty()) { + Frame index_req; index_req.opcode = Opcode::OpenIndex; + write_u32_le(id, index_req.payload); + index_req.payload.insert(index_req.payload.end(), bag.begin(), bag.end()); + auto index_reply = dispatch(index_req); + if (index_reply.reply && index_reply.reply->opcode == Opcode::OpenIndexAck) { + add(OpenTableAckSections::kProductionIndex, index_reply.reply->payload); + if (auto hit = tbls_h_.find(id); hit != tbls_h_.end()) { + (void)install_table_order(id, 0); + (void)AdsGotoTop(hit->second); + } + } + } + const std::size_t count_offset = 6u + bag.size(); + reply.payload[count_offset] = static_cast(reply.payload[count_offset] + added); + reply.payload.insert(reply.payload.end(), extra.begin(), extra.end()); + } } break; } diff --git a/src/network/session.h b/src/network/session.h index 5834621e..7997b45c 100644 --- a/src/network/session.h +++ b/src/network/session.h @@ -154,6 +154,7 @@ class Session { bool client_prefetch_back_ok_ = false; // M12.x — client sends [u16 mode] prefix on OpenTable payloads. bool client_open_table_mode_ok_ = false; + bool client_open_setup_metadata_ok_ = false; bool client_locked_row_ok_ = false; // M12.32 — last ShowDeleted state received; abi_conn_ is created // lazily, so ensure_abi_conn must re-apply it or the ABI connection diff --git a/src/network/wire.h b/src/network/wire.h index 9f4bf102..a8a2b689 100644 --- a/src/network/wire.h +++ b/src/network/wire.h @@ -700,7 +700,11 @@ inline constexpr std::uint32_t kCapLockedRow = 0x00000400u; // clients skip both round-trips. Unknown tags are skipped by length, // so the section list stays extensible in both directions. +inline constexpr std::uint32_t kCapOpenSetupMetadata = 0x00000200u; + namespace OpenTableAckSections { + constexpr std::uint8_t kRecordLength = 3; + constexpr std::uint8_t kProductionIndex = 4; constexpr std::uint8_t kSchema = 1; constexpr std::uint8_t kFirstRow = 2; } diff --git a/tests/unit/network_teardown_batch_test.cpp b/tests/unit/network_teardown_batch_test.cpp index cc0c2144..97d6d834 100644 --- a/tests/unit/network_teardown_batch_test.cpp +++ b/tests/unit/network_teardown_batch_test.cpp @@ -361,7 +361,7 @@ TEST_CASE("Teardown batching: immutable metadata caches per handle") { // Table type: answered from the opened name's extension (0 frames) // when it has one; at most one frame otherwise. CHECK(tb_op(0x6A) <= tt0 + 1); - CHECK(tb_op(0x6C) == rl0 + 1); + CHECK(tb_op(0x6C) == rl0); // immutable length rode OpenTableAck REQUIRE(AdsCloseTable(hTable) == AE_SUCCESS); REQUIRE(AdsDisconnect(hConn) == AE_SUCCESS); diff --git a/tests/unit/network_use_budget_test.cpp b/tests/unit/network_use_budget_test.cpp index b3eca3e4..765b16b1 100644 --- a/tests/unit/network_use_budget_test.cpp +++ b/tests/unit/network_use_budget_test.cpp @@ -10,6 +10,8 @@ #include "doctest.h" #include "mgmt/mg_stats.h" #include "network/server.h" +#include "network/session.h" +#include "network/client.h" #include "openads/ace.h" #include @@ -53,6 +55,8 @@ TEST_CASE("Nav batching: full USE cycle stays within wire budget") { } ADSHANDLE hI0 = 0; UNSIGNED8 bag[] = "UB.CDX"; + SUBCASE("production CDX") {} + SUBCASE("production Z01") { std::memcpy(bag, "UB.Z01", sizeof(bag)); } UNSIGNED8 tag[] = "BYID"; UNSIGNED8 exp[] = "ID"; REQUIRE(AdsCreateIndex61(hT0, bag, tag, exp, nullptr, nullptr, @@ -72,6 +76,7 @@ TEST_CASE("Nav batching: full USE cycle stays within wire budget") { REQUIRE(AdsConnect60(sb, ADS_REMOTE_SERVER, nullptr, nullptr, 0, &hC) == AE_SUCCESS); + const auto open_index_before = ub_op(0x88); // Snapshot AFTER connect: the session pool (one Connect per lane, // OPENADS_POOL_SIZE) is established once per logical connection and // amortised over every USE — the budget below guards the per-USE @@ -83,6 +88,16 @@ TEST_CASE("Nav batching: full USE cycle stays within wire budget") { UNSIGNED8 tn[] = "UB.DBF"; ADSHANDLE hT = 0; REQUIRE(AdsOpenTable(hC, tn, nullptr, ADS_CDX, 0, 0, 0, 0, &hT) == AE_SUCCESS); + UNSIGNED32 record_len = 0; const auto rl_before = ub_op(0x6C); + REQUIRE(AdsGetRecordLength(hT, &record_len) == AE_SUCCESS); + CHECK(record_len == 9); CHECK(ub_op(0x6C) == rl_before); + const auto oi_after_open = ub_op(0x88); + ADSHANDLE duplicated[64]{}; UNSIGNED16 duplicated_count = 64; + REQUIRE(AdsOpenIndex(hT, bag, duplicated, &duplicated_count) == AE_SUCCESS); + CHECK(duplicated_count == 1); CHECK(ub_op(0x88) == oi_after_open); + UNSIGNED32 natural_rec = 0; + REQUIRE(AdsGetRecordNum(hT, 0, &natural_rec) == AE_SUCCESS); + CHECK(natural_rec == 1); ADSHANDLE hOrd = 0; UNSIGNED8 want[] = "BYID"; REQUIRE(AdsGetIndexHandle(hT, want, &hOrd) == AE_SUCCESS); @@ -99,9 +114,7 @@ TEST_CASE("Nav batching: full USE cycle stays within wire budget") { UNSIGNED32 kc = 0; REQUIRE(AdsGetKeyCount(hOrd, 0, &kc) == AE_SUCCESS); CHECK(kc == 50u); - REQUIRE(AdsCloseTable(hT) == AE_SUCCESS); - REQUIRE(AdsDisconnect(hC) == AE_SUCCESS); - + CHECK(ub_op(0x88) == open_index_before); // production binding rode OpenTable auto delta = [&](std::uint8_t op) { return ub_op(op) - before[op]; }; std::uint64_t total = 0; for (int o = 0; o < 256; ++o) @@ -116,6 +129,101 @@ TEST_CASE("Nav batching: full USE cycle stays within wire budget") { CHECK(delta(0x48) == 0u); // AtEOF: served locally CHECK(delta(0x4C) == 0u); // AtBOF: served locally + // A separate login gets independent server index IDs and cursor state. + ADSHANDLE second_conn = 0, second_table = 0; + REQUIRE(AdsConnect60(sb, ADS_REMOTE_SERVER, nullptr, nullptr, 0, &second_conn) == AE_SUCCESS); + REQUIRE(AdsOpenTable(second_conn, tn, nullptr, ADS_CDX, 0, 0, 0, 0, &second_table) == AE_SUCCESS); + ADSHANDLE second_order = 0; REQUIRE(AdsGetIndexHandle(second_table, want, &second_order) == AE_SUCCESS); + CHECK(second_order != hOrd); + REQUIRE(AdsSetIndexOrderByHandle(second_table, second_order) == AE_SUCCESS); + REQUIRE(AdsGotoTop(second_order) == AE_SUCCESS); + UNSIGNED32 first_rec = 0, second_rec = 0; + REQUIRE(AdsGetRecordNum(hT, 0, &first_rec) == AE_SUCCESS); + REQUIRE(AdsGetRecordNum(second_table, 0, &second_rec) == AE_SUCCESS); + CHECK(first_rec == 50); CHECK(second_rec == 1); + // CloseAll / same-bag reopen keeps its existing validated park path. + REQUIRE(AdsCloseAllIndexes(second_table) == AE_SUCCESS); + duplicated_count = 64; + REQUIRE(AdsOpenIndex(second_table, bag, duplicated, &duplicated_count) == AE_SUCCESS); + CHECK(duplicated_count == 1); + REQUIRE(AdsCloseTable(second_table) == AE_SUCCESS); REQUIRE(AdsDisconnect(second_conn) == AE_SUCCESS); + REQUIRE(AdsCloseTable(hT) == AE_SUCCESS); + REQUIRE(AdsDisconnect(hC) == AE_SUCCESS); + s.stop(); fs::remove_all(dir, ec); } + +TEST_CASE("Open metadata: legacy clients do not receive setup sections") { + // Reuse the table created by the USE fixture only after creating our own + // independent small DBF, so this test never depends on test order. + auto dir = fs::temp_directory_path() / "openads_metadata_cap"; + std::error_code ec; fs::remove_all(dir, ec); fs::create_directories(dir); + UNSIGNED8 local[512]{}; std::memcpy(local, dir.string().c_str(), dir.string().size()); + ADSHANDLE c = 0, t = 0; + REQUIRE(AdsConnect60(local, ADS_LOCAL_SERVER, nullptr, nullptr, 0, &c) == AE_SUCCESS); + UNSIGNED8 name[] = "cap.dbf", def[] = "ID,N,8,0"; + REQUIRE(AdsCreateTable(c, name, nullptr, ADS_CDX, 0, 0, 0, 0, def, &t) == AE_SUCCESS); + ADSHANDLE index = 0; UNSIGNED8 bag[] = "cap.cdx", tag[] = "BYID", expression[] = "ID"; + REQUIRE(AdsCreateIndex61(t, bag, tag, expression, nullptr, nullptr, + ADS_COMPOUND, 512, &index) == AE_SUCCESS); + REQUIRE(AdsCloseTable(t) == AE_SUCCESS); REQUIRE(AdsDisconnect(c) == AE_SUCCESS); + openads::network::Server server; + for (bool supports_metadata : {false, true}) { + openads::network::Session session(server, openads::network::Socket{}, "", 0); + openads::network::Frame connect; connect.opcode = openads::network::Opcode::Connect; + auto text = [&](const std::string& value) { + connect.payload.push_back(static_cast(value.size() & 255)); + connect.payload.push_back(static_cast(value.size() >> 8)); + connect.payload.insert(connect.payload.end(), value.begin(), value.end()); + }; + text(dir.string()); text(""); text(""); + const std::uint32_t caps = supports_metadata ? openads::network::kCapOpenSetupMetadata : 0; + for (int i = 0; i < 4; ++i) connect.payload.push_back(static_cast(caps >> (8 * i))); + auto login = session.dispatch(connect); REQUIRE(login.reply); + REQUIRE(login.reply->opcode == openads::network::Opcode::ConnectAck); + openads::network::Frame open; open.opcode = openads::network::Opcode::OpenTable; + open.payload.assign(name, name + std::strlen(reinterpret_cast(name))); + auto result = session.dispatch(open); REQUIRE(result.reply); + REQUIRE(result.reply->opcode == openads::network::Opcode::OpenTableAck); + const auto& pl = result.reply->payload; REQUIRE(pl.size() >= 7); + const auto baglen = static_cast(pl[4] | (pl[5] << 8)); + std::size_t off = 6 + baglen; REQUIRE(off < pl.size()); + const unsigned count = pl[off++]; bool found_length = false, found_index = false; + for (unsigned i = 0; i < count; ++i) { + REQUIRE(off + 5 <= pl.size()); const auto tag = pl[off++]; + const std::size_t length = static_cast(pl[off]) | + (static_cast(pl[off+1]) << 8) | + (static_cast(pl[off+2]) << 16) | + (static_cast(pl[off+3]) << 24); + off += 4; REQUIRE(off + length <= pl.size()); + if (tag == openads::network::OpenTableAckSections::kRecordLength) { + found_length = true; CHECK(length == 4); CHECK(pl[off] == 9); + } + if (tag == openads::network::OpenTableAckSections::kProductionIndex) { + found_index = true; + std::vector bytes(pl.begin() + off, pl.begin() + off + length); + auto parsed = openads::network::RemoteConnection::parse_open_index_reply(bytes, "cap.cdx"); + REQUIRE(parsed); REQUIRE(parsed.value().size() == 1); + CHECK(parsed.value()[0].tag == "BYID"); + } + off += length; + } + CHECK(found_index == supports_metadata); + CHECK(found_length == supports_metadata); + CHECK(off == pl.size()); + } + fs::remove_all(dir, ec); +} + +TEST_CASE("Open metadata: shared index parser rejects short mandatory payloads") { + using C = openads::network::RemoteConnection; + CHECK_FALSE(C::parse_open_index_reply({}, "bag.cdx")); + CHECK_FALSE(C::parse_open_index_reply({1}, "bag.cdx")); + CHECK_FALSE(C::parse_open_index_reply({1, 0, 1, 0, 0}, "bag.cdx")); + CHECK_FALSE(C::parse_open_index_reply({1, 0, 1, 0, 0, 0, 3, 0, 'x'}, "bag.cdx")); + auto legacy = C::parse_open_index_reply({1, 0, 42, 0, 0, 0}, "bag.cdx"); + REQUIRE(legacy); REQUIRE(legacy.value().size() == 1); + CHECK(legacy.value()[0].id == 42); + CHECK(legacy.value()[0].tag.empty()); +} From fd34b08dfb078c713a210a49cb895d8dccba2ec1 Mon Sep 17 00:00:00 2001 From: Pritpal Bedi Date: Fri, 9 Oct 2026 21:18:55 -0500 Subject: [PATCH 2/2] Fix shadowed TLV tag name in open-metadata test --- tests/unit/network_use_budget_test.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/network_use_budget_test.cpp b/tests/unit/network_use_budget_test.cpp index 765b16b1..cc978c3c 100644 --- a/tests/unit/network_use_budget_test.cpp +++ b/tests/unit/network_use_budget_test.cpp @@ -191,16 +191,16 @@ TEST_CASE("Open metadata: legacy clients do not receive setup sections") { std::size_t off = 6 + baglen; REQUIRE(off < pl.size()); const unsigned count = pl[off++]; bool found_length = false, found_index = false; for (unsigned i = 0; i < count; ++i) { - REQUIRE(off + 5 <= pl.size()); const auto tag = pl[off++]; + REQUIRE(off + 5 <= pl.size()); const auto tlv_tag = pl[off++]; const std::size_t length = static_cast(pl[off]) | (static_cast(pl[off+1]) << 8) | (static_cast(pl[off+2]) << 16) | (static_cast(pl[off+3]) << 24); off += 4; REQUIRE(off + length <= pl.size()); - if (tag == openads::network::OpenTableAckSections::kRecordLength) { + if (tlv_tag == openads::network::OpenTableAckSections::kRecordLength) { found_length = true; CHECK(length == 4); CHECK(pl[off] == 9); } - if (tag == openads::network::OpenTableAckSections::kProductionIndex) { + if (tlv_tag == openads::network::OpenTableAckSections::kProductionIndex) { found_index = true; std::vector bytes(pl.begin() + off, pl.begin() + off + length); auto parsed = openads::network::RemoteConnection::parse_open_index_reply(bytes, "cap.cdx");