From 8ccefa6884d84a697bfd1146a41172d24cd3ee0d Mon Sep 17 00:00:00 2001 From: liu weikai Date: Thu, 1 Oct 2026 05:53:59 +0800 Subject: [PATCH] Keep Discover rows stable while query and publication status changes --- CHANGELOG.md | Bin 123724 -> 123752 bytes docs/geocaching-client-runtime.md | 7 ++++++ .../tests/run_browse_runtime_asan.sh | 1 + .../tests/test_browse_runtime.cpp | 15 +++++++++++++ .../core_geocaching/tests/test_sd_journal.cpp | 8 +++++-- .../geocaching/query_browse_source.h | 2 +- .../src/geocaching/browse_runtime.cpp | 21 ++++++++++++++---- 7 files changed, 47 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1c77c883621e925adb6ff2d3e47928651bddaecd..8c722c87b1560d1c1be6f9a28d39c9ca0f55077c 100644 GIT binary patch delta 37 tcmX?ejQzzi_6=W{CNbx67BZAFq%ssSR8D4$5N6bAPG;Vo%*^=mJ^&8X4Ke@# delta 32 ocmaEHjQz|p_6=W{ICB{C8Ilh5&!@I diff --git a/docs/geocaching-client-runtime.md b/docs/geocaching-client-runtime.md index f7aacb35..bf71f07e 100644 --- a/docs/geocaching-client-runtime.md +++ b/docs/geocaching-client-runtime.md @@ -179,6 +179,13 @@ a catalogue gives it a new lifetime token, so stale selections from a closed session cannot become valid when a new catalogue starts at generation one. Status messages and action buttons update independently of row rendering. +Discover rows use the committed query page's content generation. Query phase, +failure text, detail responses and unrelated publication progress do not change +that version. A separate source-lifetime token and saved-membership generation +invalidate selections when the browser session is replaced or Downloaded +membership changes. Pagination retains the existing rows while requesting the +next page; committing the replacement page invalidates the previous selection. + Background publication recovery filters completed, cancelled and no-intent requests before opening their parent tasks. Exact cache lookups still read confirmed receipts, and eligible unfinished requests retain task/reference diff --git a/modules/core_geocaching/tests/run_browse_runtime_asan.sh b/modules/core_geocaching/tests/run_browse_runtime_asan.sh index 61050b0e..1ee16cf1 100644 --- a/modules/core_geocaching/tests/run_browse_runtime_asan.sh +++ b/modules/core_geocaching/tests/run_browse_runtime_asan.sh @@ -35,3 +35,4 @@ g++ -std=c++17 -g -O1 -fsanitize=address -fno-omit-frame-pointer -DTRAIL_MATE_RE -o "$build_dir/browse_runtime" ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 "$build_dir/browse_runtime" modules/core_geocaching/tests/fixtures ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 "$build_dir/browse_runtime" modules/core_geocaching/tests/fixtures save-close-detail +ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 "$build_dir/browse_runtime" modules/core_geocaching/tests/fixtures legacy-index-upgrade diff --git a/modules/core_geocaching/tests/test_browse_runtime.cpp b/modules/core_geocaching/tests/test_browse_runtime.cpp index 6f1b8c44..01592a9a 100644 --- a/modules/core_geocaching/tests/test_browse_runtime.cpp +++ b/modules/core_geocaching/tests/test_browse_runtime.cpp @@ -537,6 +537,9 @@ int main(int argc, char** argv) "online detail not displayed"); require(detailText(item).description == expected_record.description && detailText(item).hint == expected_record.hint, "online detail omitted full description or hint"); + ui::geocaching::Item discover_probe; + require(snapshot(Section::Discover).generation == generation && test::source->item(Section::Discover, 0, generation, discover_probe), + "detail response invalidated unchanged discovery rows"); require(test::files.empty() && !test::blocked_io, "online detail required SD"); require(router.delivery({{reply_remote.data(), 16}, {router.local.data(), 16}, {}, {last_reply.data(), last_reply.size()}}, router.context), "verified detail duplicate was not acknowledged"); @@ -601,6 +604,8 @@ int main(int argc, char** argv) tick(); } require(router.sends == 4, "saving verified detail sent a redundant network request"); + require(snapshot(Section::Discover).generation != generation && !test::source->item(Section::Discover, 0, generation, discover_probe), + "downloaded membership retained stale discovery rows"); const auto released_response = test::allocations.find(open_response); require(released_response == test::allocations.end() || released_response->second.owner != "geocaching.rx", "saved detail retained its response buffer"); @@ -698,6 +703,7 @@ int main(int argc, char** argv) return view.count == 1 && test::source->item(Section::Published, 0, view.generation, item) && item.publication_confirmed && item.publication_revision == 1; }, "publication receipt not reflected in draft list"); require(router.identity.signs == 1, "publication signed more than once"); + const auto discovery_before_edit = snapshot(Section::Discover).generation; // Exercise the UI field limits and the confirmed-version edit path through // the same device owner, not a direct store call. const std::string long_name(96, 'N'), long_description(2048, 'D'), long_hint(512, 'H'); @@ -727,6 +733,9 @@ int main(int argc, char** argv) return test::source->item(Section::Published, 0, view.generation, item) && item.publication_confirmed && item.publication_revision == 2; }, "second publication receipt not reflected in list"); require(router.identity.signs == 2, "version update signed more than once"); + require(snapshot(Section::Discover).generation == discovery_before_edit && + test::source->item(Section::Discover, 0, discovery_before_edit, discover_probe), + "unrelated publication invalidated unchanged discovery rows"); // Isolate archive conformance from the following restart fixture, which // intentionally retains an active revision-2 draft for its map assertions. const auto active_fixture = test::files; @@ -1419,8 +1428,11 @@ int main(int argc, char** argv) if (found == test::files.end()) removed_staging = true; return removed_staging && snapshot(Section::Discover).has_more && found != test::files.end() && found->second.empty(); }, "pagination test did not enter maintenance"); + const auto rows_before_pagination = snapshot(Section::Discover).generation; require(test::source->loadMore(), "maintenance rejected pagination intent"); require(!test::source->loadMore() && !snapshot(Section::Discover).has_more, "duplicate pagination was accepted"); + require(snapshot(Section::Discover).generation == rows_before_pagination, + "pagination status invalidated the retained query page"); require(!test::ui_io, "pagination callback touched storage"); until([&] { return router.sends == paging_sends + 3; }, @@ -1431,6 +1443,9 @@ int main(int argc, char** argv) { const auto view = snapshot(Section::Discover); return view.can_refresh && !view.has_more && view.count == 0; }, "final pagination response was not displayed"); require(router.sends == paging_sends + 3, "pagination request sent twice"); + require(snapshot(Section::Discover).generation != rows_before_pagination && + !test::source->item(Section::Discover, 0, rows_before_pagination, discover_probe), + "replacement query page retained a stale selection"); closeRuntime(); const auto journalCount = [] { diff --git a/modules/core_geocaching/tests/test_sd_journal.cpp b/modules/core_geocaching/tests/test_sd_journal.cpp index 0356bb72..6f0bedad 100644 --- a/modules/core_geocaching/tests/test_sd_journal.cpp +++ b/modules/core_geocaching/tests/test_sd_journal.cpp @@ -456,8 +456,11 @@ int checkBrowseFlow(const char* capabilities_path, const char* query_path) Destination wrong; wrong.bytes[0] = 1; if (port.accepted(wrong, id, {original.data(), original.size()})) return 127; + const auto retained_rows = snapshot.generation; source.refresh(::ui::geocaching::Section::Discover); - if (source.item(::ui::geocaching::Section::Discover, 0, snapshot.generation, item)) return 128; + source.snapshot(::ui::geocaching::Section::Discover, snapshot); + if (snapshot.generation != retained_rows || snapshot.can_refresh || + !source.item(::ui::geocaching::Section::Discover, 0, retained_rows, item) || std::strcmp(item.name.data(), "Test")) return 128; if (!client->tick(10) || !advance() || client->phase() != QueryClientPhase::Querying) return 129; client->tick(10); if (!client->tick(10 + QueryClient::kReplyTimeoutMs) || client->phase() != QueryClientPhase::Cancelling) return 130; @@ -480,7 +483,8 @@ int checkBrowseFlow(const char* capabilities_path, const char* query_path) storage::PendingRequestView pending; if (storage::nextPendingRequest(state.view(), {}, {}, pending) != storage::PendingRequestResult::None) return 133; source.snapshot(::ui::geocaching::Section::Discover, snapshot); - if (!snapshot.can_refresh || !std::strstr(snapshot.status.data(), "did not reply")) return 134; + if (!snapshot.can_refresh || !std::strstr(snapshot.status.data(), "did not reply") || + !source.item(::ui::geocaching::Section::Discover, 0, snapshot.generation, item) || std::strcmp(item.name.data(), "Test")) return 134; fixture::files.clear(); return 0; } diff --git a/platform/esp/arduino_common/include/platform/esp/arduino_common/geocaching/query_browse_source.h b/platform/esp/arduino_common/include/platform/esp/arduino_common/geocaching/query_browse_source.h index a8f94bb5..78a944f4 100644 --- a/platform/esp/arduino_common/include/platform/esp/arduino_common/geocaching/query_browse_source.h +++ b/platform/esp/arduino_common/include/platform/esp/arduino_common/geocaching/query_browse_source.h @@ -84,7 +84,7 @@ class QueryBrowseSource final : public ::ui::geocaching::Source QueryBrowseSource& source; bool locked; }; - uint64_t generation() const { return (store_.generation() << 8) | static_cast(client_.phase()) | (static_cast(client_.failure()) << 4); } + uint64_t generation() const { return store_.generation(); } const char* status(::geocaching::QueryClientPhase phase) const { using Phase = ::geocaching::QueryClientPhase; diff --git a/platform/esp/arduino_common/src/geocaching/browse_runtime.cpp b/platform/esp/arduino_common/src/geocaching/browse_runtime.cpp index d75ae416..e931160a 100644 --- a/platform/esp/arduino_common/src/geocaching/browse_runtime.cpp +++ b/platform/esp/arduino_common/src/geocaching/browse_runtime.cpp @@ -86,6 +86,7 @@ struct Session { uint64_t local_map_revision = 0; uint64_t saved_catalog_epoch = 0, draft_catalog_epoch = 0; + uint64_t discover_catalog_epoch = 0, discover_saved_epoch = 0, discover_saved_generation = 0; struct Publication { enum class DraftStage : uint8_t @@ -326,6 +327,17 @@ std::atomic next_step{0}; std::atomic replies_seen{0}, replies_busy{0}, replies_queued{0}; uint32_t replies_processed = 0, replies_accepted = 0; uint64_t epoch = 0; +uint64_t discoverScope(Session& s) +{ + const auto saved_generation = s.saved ? s.saved->generation() : 0; + if (s.discover_saved_epoch != s.saved_catalog_epoch || s.discover_saved_generation != saved_generation) + { + s.discover_saved_epoch = s.saved_catalog_epoch; + s.discover_saved_generation = saved_generation; + s.discover_catalog_epoch = ++epoch; + } + return s.discover_catalog_epoch; +} void reportPublication(const char* event, const gc::PublishAttempt& attempt) { const auto& id = attempt.cacheId().bytes; @@ -888,7 +900,7 @@ void advanceBrowse(Session& s) return; } s.client->query(kWorld); - ++epoch; + s.discover_catalog_epoch = ++epoch; } drainReply(s); if (auto* pending = pending_announcement.exchange(nullptr, std::memory_order_acq_rel)) @@ -1661,9 +1673,10 @@ class Facade final : public ::ui::geocaching::Source out.can_create = out.has_more = false; } // List ownership and its own content version determine row validity. - // Transport progress and other sections must not invalidate local rows. + // Transport and operation progress must not invalidate unchanged rows. const auto scope = section == ::ui::geocaching::Section::Published && session ? session->draft_catalog_epoch : section == ::ui::geocaching::Section::Downloaded && session ? session->saved_catalog_epoch + : section == ::ui::geocaching::Section::Discover && session ? discoverScope(*session) : epoch; out.generation ^= scope << 32; } @@ -1768,7 +1781,7 @@ class Facade final : public ::ui::geocaching::Source } if (guard.locked && section == ::ui::geocaching::Section::Downloaded && session && session->saved && !session->needsRecovery()) return session->saved->item(index, generation ^ (session->saved_catalog_epoch << 32), out); - if (!guard.locked || !session || !session->source || !session->source->item(section, index, generation ^ (epoch << 32), out)) return false; + if (!guard.locked || !session || !session->source || !session->source->item(section, index, generation ^ (discoverScope(*session) << 32), out)) return false; out.downloaded = session->saved && session->saved->contains(out.id, out.revision_hash); out.can_download = !out.downloaded && !downloadActive() && !publicationActive() && !draftSaveActive() && (!session->store || !session->store->commitPending()) && !session->needsRecovery() && session->phase != Phase::Failed && storage::sd_card_ready() && !storage::sd_external_block_owner_active() && @@ -2091,7 +2104,7 @@ class Facade final : public ::ui::geocaching::Source if (session->saved && session->saved->contains(item.id, item.revision_hash)) return false; ::ui::geocaching::Snapshot snapshot; session->source->snapshot(::ui::geocaching::Section::Discover, snapshot); - if ((generation ^ (epoch << 32)) != snapshot.generation) return false; + if ((generation ^ (discoverScope(*session) << 32)) != snapshot.generation) return false; gc::protocol::SummaryView summary; bool found = false; for (size_t i = 0; i < snapshot.count; ++i)