From 32a09938d898746b75c51e1d38cc5b4c6ae0ed3d Mon Sep 17 00:00:00 2001 From: Torlando <281092095+torlando-agent[bot]@users.noreply.github.com> Date: Tue, 15 Sep 2026 06:27:14 +0000 Subject: [PATCH] fix(nomadnet): address Greploop round 1 on the cache transient-stall guard Three findings on the transient-stall bail, all valid: - Reload invalidation: a bail recorded BYPASS, but NomadNetCacheFlow::service() accepts only MISS as a successful invalidation, so a bail during an admitted reload reported 'Page cache invalidation failed' instead of falling through to a live fetch. The bail now records MISS. - Open-resource leak: the bail cleared read_open_/write_open_ (and abandoned an open directory enumeration) without calling endRead()/abortWrite()/endList(), leaking SD handles. The bail now releases each in-flight resource via the seam's own teardown (bounded best-effort; a still-transient close is accepted rather than re-pinning the op). - Tick-vs-time: the not-ready UNAVAILABLE path returns immediately (no bus wait), so a pure 500-tick budget could expire during a legitimate SD mount window and disable caching for the whole session. The bail is now gated on BOTH the tick floor AND a 10s wall-time window (service() takes a monotonic ms clock; production passes millis(), 0 is a safe default for tests). Regression tests: reload-invalidation bail -> NEED_LIVE (flow), flat-clock does not bail, healable transient keeps authority, list-open (RECOVERY_END) stall bails and releases the handle (cache). --- lib/tdeck_ui/UI/LXMF/NomadNetCache.cpp | 69 +++++++++++++++++----- lib/tdeck_ui/UI/LXMF/NomadNetCache.h | 26 ++++++-- lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.cpp | 4 +- lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.h | 2 +- lib/tdeck_ui/UI/LXMF/UIManager.cpp | 2 +- tests/native/test_nomadnet_cache.cpp | 37 +++++++++++- tests/native/test_nomadnet_cache_flow.cpp | 27 ++++++++- 7 files changed, 137 insertions(+), 30 deletions(-) diff --git a/lib/tdeck_ui/UI/LXMF/NomadNetCache.cpp b/lib/tdeck_ui/UI/LXMF/NomadNetCache.cpp index 3826c8d3..6b9ea245 100644 --- a/lib/tdeck_ui/UI/LXMF/NomadNetCache.cpp +++ b/lib/tdeck_ui/UI/LXMF/NomadNetCache.cpp @@ -928,15 +928,19 @@ CacheResult NomadNetCache::invalidate(const CacheKey& key) { return result_; } -void NomadNetCache::service() { +void NomadNetCache::service(std::uint64_t now_ms) { // Transient-stall guard (cross-call): compare this call's entry state to // the previous call's. A transient (BUSY/UNAVAILABLE) retry changes none // of the tracked bits, so an unchanged entry across consecutive service() - // calls is a stall; any advance resets the counter. Past the bounded - // budget, bail so a persistently unhealthy SD seam can never pin the cache - // (and the NomadNet UI at "Checking SD page cache..."). The baseline is - // refreshed before the switch so every path — including early returns — - // leaves a correct entry for the next call. + // calls is a stall; any advance resets the counter. The bail is gated on + // BOTH a tick floor and an elapsed-time window: a not-ready SD seam + // returns UNAVAILABLE immediately (no bus wait), so a pure tick budget + // could expire during a legitimate mount window and disable caching for + // the whole session. Past both bounds, bail so a persistently unhealthy + // SD seam can never pin the cache (and the NomadNet UI at "Checking SD + // page cache..."). The baseline is refreshed before the switch so every + // path — including early returns — leaves a correct entry for the next + // call. if (operation_ != Operation::NONE && operation_ == transient_prev_op_ && offset_ == transient_prev_offset_ && @@ -947,7 +951,9 @@ void NomadNetCache::service() { write_open_ == transient_prev_write_open_) { if (transient_stall_count_ < MAX_TRANSIENT_STALL_TICKS) { ++transient_stall_count_; - } else { + if (transient_stall_count_ == 1) transient_stall_start_ms_ = now_ms; + } else if (now_ms >= transient_stall_start_ms_ && + now_ms - transient_stall_start_ms_ >= MAX_TRANSIENT_STALL_MS) { transient_stall_count_ = 0; transient_bail(); } @@ -1462,14 +1468,46 @@ void NomadNetCache::service() { void NomadNetCache::transient_bail() { // The storage seam has been stalled for far longer than any real SPI - // contention or SD mount window. Stop retrying: drop all namespace - // authority (lookups and commits now bypass), and clear the in-flight op - // so the caller's flow falls through to a live fetch, restoring the - // pre-cache page-load behavior instead of a frozen UI. + // contention or SD mount window. Stop retrying: release any in-flight + // storage resources (read handle, partial write, directory enumeration) + // so nothing leaks, drop all namespace authority (lookups and commits now + // bypass), and clear the in-flight op so the caller's flow falls through + // to a live fetch, restoring the pre-cache page-load behavior instead of + // a frozen UI. Each release is a bounded best effort: transient + // (BUSY/UNAVAILABLE) retries are re-tried a few times, and a still-open + // handle is accepted rather than re-pinning the op — the storage seam + // stays abandoned for the rest of the session, and its destructor closes + // whatever is left. transient_stall_count_ = 0; namespace_authoritative_ = false; - read_open_ = false; - write_open_ = false; + const bool list_was_active = + operation_ == Operation::RECOVERY_BEGIN || + operation_ == Operation::RECOVERY_NEXT || + operation_ == Operation::RECOVERY_END; + if (list_was_active) { + for (int attempt = 0; attempt < 3; ++attempt) { + const auto list_result = storage_.endList(); + if (!storage_result_is_transient(list_result)) break; + } + } + if (read_open_) { + for (int attempt = 0; attempt < 3; ++attempt) { + const auto read_result = storage_.endRead(); + if (!storage_result_is_transient(read_result)) { + read_open_ = false; + break; + } + } + } + if (write_open_) { + for (int attempt = 0; attempt < 3; ++attempt) { + const auto write_result = storage_.abortWrite(); + if (!storage_result_is_transient(write_result)) { + write_open_ = false; + break; + } + } + } commit_job_ = false; cleanup_stages_after_failure_ = false; eviction_pending_ = false; @@ -1481,7 +1519,10 @@ void NomadNetCache::transient_bail() { metadata_bytes_.clear(); ExternalVector().swap(body_); operation_ = Operation::NONE; - result_ = CacheResult::BYPASS; + // MISS (not BYPASS): for an admitted reload invalidation the flow accepts + // MISS as "the stale generation will not be served", which correctly falls + // through to a live fetch; a failed bail must never report a hard error. + result_ = CacheResult::MISS; } void NomadNetCache::cancel() { diff --git a/lib/tdeck_ui/UI/LXMF/NomadNetCache.h b/lib/tdeck_ui/UI/LXMF/NomadNetCache.h index c3d7f46c..6adc3d86 100644 --- a/lib/tdeck_ui/UI/LXMF/NomadNetCache.h +++ b/lib/tdeck_ui/UI/LXMF/NomadNetCache.h @@ -93,7 +93,11 @@ public: CacheResult beginRecovery(std::uint64_t now, bool cleanup_stages = false); CacheResult invalidate(const CacheKey& key); - void service(); + // now_ms is a monotonic millisecond clock used to bound the transient-stall + // bail in wall-time (see the stall-guard note below). Production passes + // millis(); 0 is a safe default for tests that never expect a stall (the + // time window is only reached after 500 consecutive no-progress ticks). + void service(std::uint64_t now_ms = 0); void cancel(); bool busy() const { return operation_ != Operation::NONE; } bool recoveryComplete() const { return recovery_complete_; } @@ -234,12 +238,21 @@ private: // offset, scan/cleanup index, scan count, open-flag) resets the stall // counter, so slow-but-progressing steps (chunked reads/writes, directory // scans) never false-trip; a tick with no advance is a transient stall. - // Past the bounded budget the cache bails: mark the namespace - // non-authoritative for the session (lookups/commits then bypass) and - // clear the op, so the flow falls through to a live fetch (the pre-cache - // behavior). 500 no-progress ticks far exceeds any real SPI contention or - // SD mount window (each op already waits only 100 ms on the bus mutex). + // + // The bail is time-bounded, not tick-bounded: a not-ready SD seam returns + // UNAVAILABLE immediately (no bus wait), so the main loop can spin many + // ticks in a second and a pure tick budget would expire during a + // legitimate mount window and disable caching for the whole session. + // Both a tick floor (protects a pathological fast tick loop) AND an elapsed + // wall-time window must pass before the cache bails. 10s comfortably + // exceeds the one-time boot mount window (~2.5s, and recovery starts after + // UI-ready with the card already mounted) and any SPI contention burst. + // On bail the namespace is marked non-authoritative for the session + // (lookups/commits then bypass) and the in-flight op is cleared, so the + // flow falls through to a live fetch (the pre-cache behavior) instead of + // a frozen UI. static constexpr std::uint32_t MAX_TRANSIENT_STALL_TICKS = 500; + static constexpr std::uint64_t MAX_TRANSIENT_STALL_MS = 10000; Operation transient_prev_op_ = Operation::NONE; std::size_t transient_prev_offset_ = 0; std::size_t transient_prev_scan_index_ = 0; @@ -248,6 +261,7 @@ private: bool transient_prev_read_open_ = false; bool transient_prev_write_open_ = false; std::uint32_t transient_stall_count_ = 0; + std::uint64_t transient_stall_start_ms_ = 0; void transient_bail(); static constexpr std::size_t VERIFY_SCRATCH_BYTES = 1024; diff --git a/lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.cpp b/lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.cpp index 00a40140..8d822ac1 100644 --- a/lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.cpp +++ b/lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.cpp @@ -31,10 +31,10 @@ CacheFlowState NomadNetCacheFlow::begin(const CacheKey& key, std::uint64_t now, return state_; } -void NomadNetCacheFlow::service() { +void NomadNetCacheFlow::service(std::uint64_t now_ms) { if (state_ == CacheFlowState::CANCELLED || state_ == CacheFlowState::FAILED) return; - if (cache_.busy()) cache_.service(); + if (cache_.busy()) cache_.service(now_ms); if (state_ == CacheFlowState::INVALIDATE) { if (cache_.busy()) return; if (!invalidation_admitted_) { diff --git a/lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.h b/lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.h index 90c171aa..a4baec15 100644 --- a/lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.h +++ b/lib/tdeck_ui/UI/LXMF/NomadNetCacheFlow.h @@ -22,7 +22,7 @@ public: explicit NomadNetCacheFlow(NomadNetCache& cache) : cache_(cache) {} CacheFlowState begin(const CacheKey&, std::uint64_t now, bool reload); - void service(); + void service(std::uint64_t now_ms = 0); bool acceptLive(const std::vector&, const CacheEligibility&, std::uint64_t now); void cancel(); diff --git a/lib/tdeck_ui/UI/LXMF/UIManager.cpp b/lib/tdeck_ui/UI/LXMF/UIManager.cpp index e7d44467..1350ba5b 100644 --- a/lib/tdeck_ui/UI/LXMF/UIManager.cpp +++ b/lib/tdeck_ui/UI/LXMF/UIManager.cpp @@ -3101,7 +3101,7 @@ void UIManager::nomad_update() { _nomad_cache_pending_now = 0; _nomad_cache_pending_ttl = 0; } - _nomad_cache_flow.service(); + _nomad_cache_flow.service(millis()); if (_nomad_state == NomadState::CACHE) { if (_nomad_cache_generation != _nomad_navigation_generation) { _nomad_cache_flow.cancel(); diff --git a/tests/native/test_nomadnet_cache.cpp b/tests/native/test_nomadnet_cache.cpp index 24e58729..db240f6b 100644 --- a/tests/native/test_nomadnet_cache.cpp +++ b/tests/native/test_nomadnet_cache.cpp @@ -23,7 +23,7 @@ struct MemoryStorage final:NomadNetStorage{ StorageResult stat(const char*n,uint32_t&s)override{++operations;if(n==stat_fail_path&&stat_fail_result!=StorageResult::OK)return stat_fail_result;auto i=files.find(n);if(i==files.end())return StorageResult::MISS;s=i->second.size();return StorageResult::OK;} StorageResult beginList(const char*d)override{++operations;list.clear();for(auto&f:files)if(f.first.rfind(std::string(d)+"/",0)==0)list.push_back(f.first);li=0;return available?list_result:StorageResult::UNAVAILABLE;} StorageResult nextList(char*n,size_t c,bool&done)override{++operations;if(next_list_fail_at&&static_cast(li+1)==next_list_fail_at)return StorageResult::IO_ERROR;if(li==list.size()){done=true;return StorageResult::OK;}done=false;if(list[li].size()+1>c){++li;return StorageResult::TOO_LARGE;}std::memcpy(n,list[li].c_str(),list[li].size()+1);++li;return StorageResult::OK;} - StorageResult endList()override{++operations;return StorageResult::OK;}std::vectorlist;size_t li=0; + StorageResult endList()override{++operations;if(list_close_busy)return StorageResult::BUSY;return StorageResult::OK;}std::vectorlist;size_t li=0;bool list_close_busy=false; }; static CacheKey key(const char*path="/page/index.mu"){return CacheKey{"0123456789abcdef0123456789abcdef",path,RequestDataClass::NIL};} // A seam whose directory enumeration is permanently BUSY (SPI mutex starved @@ -68,12 +68,43 @@ int main(){int f=0;auto ck=[&](bool x,const char*n){if(!x){++f;std::cerr<<"FAIL // A persistently transient seam during boot-time recovery must not pin the // cache (and the UI). Before this fix the recovery retried beginList forever; // now the stall budget expires, the namespace is marked non-authoritative, - // and the op clears so the flow can fall through to a live fetch. + // and the op clears so the flow can fall through to a live fetch. The bail is + // gated on wall-time, so the driver advances a fake monotonic clock (1ms per + // tick) so the time window elapses in a bounded number of ticks. + auto stall_run=[](NomadNetCache&c){uint64_t now=0;for(int i=0;i<200000&&c.busy();++i){now+=1;c.service(now);}}; MemoryStorage busy_boot;busy_boot.available=false;NomadNetCache bc(busy_boot,cfg); - for(int i=0;i<2000&&bc.busy();++i)bc.service(); + stall_run(bc); ck(!bc.busy(),"persistent transient recovery bails instead of pinning"); ck(!bc.recoveryComplete(),"bailed recovery is not authoritative"); ck(bc.beginLookup(key(),100)==CacheResult::BYPASS,"post-bail lookup bypasses to live"); + ck(busy_boot.active.empty(),"bail does not leak a read handle"); + // F3: 500 fast no-progress ticks are NOT enough to bail; the wall-time window + // (10s) must also elapse. With a flat (slow) clock the stall must survive well + // past the tick floor so a healthy SD that stays briefly quiet keeps its cache + // authority for the session. + MemoryStorage slow_boot;slow_boot.available=false;NomadNetCache sc(slow_boot,cfg); + uint64_t flat=0;for(int i=0;i<600&&sc.busy();++i){flat+=1;sc.service(flat);} + ck(sc.busy(),"fast ticks alone do not disable the session cache"); + // A seam that heals within the window terminates recovery cleanly. Boot + // recovery transients do not degrade the namespace (the RECOVERY_BEGIN/END + // paths keep it authoritative), so a healable transient still completes + // recovery authoritatively and the session cache remains usable. + MemoryStorage recover;recover.available=false;NomadNetCache hr(recover,cfg); + uint64_t hrnow=0;int guard=0;for(;hr.busy()&&guard<100000;++guard,++hrnow){if(guard==500)recover.available=true;hr.service(hrnow);} + ck(!hr.busy(),"seam that heals within the window terminates recovery"); + ck(hr.recoveryComplete(),"healable transient keeps the namespace authoritative"); + ck(hr.beginLookup(key(),100)==CacheResult::PENDING,"post-heal lookup uses the cache, not a bypass"); + // F2: a bail while the directory enumeration handle is open (RECOVERY_END + // endList stuck BUSY) must release it rather than leak it. A fresh boot + // recovery opens the list in RECOVERY_BEGIN and closes it in RECOVERY_END, so + // a seam whose endList is persistently BUSY reaches the guard with the list + // handle open — exactly the device case a stale SPI mutex can produce. + MemoryStorage list_seam;list_seam.list_close_busy=true; + NomadNetCache list_recover(list_seam,cfg); + uint64_t lnow=0;for(int i=0;i<200000&&list_recover.busy();++i){lnow+=1;list_recover.service(lnow);} + ck(!list_recover.busy(),"list-open stall bails instead of pinning"); + ck(!list_recover.recoveryComplete(),"bailed recovery is not authoritative"); + ck(list_recover.beginLookup(key(),100)==CacheResult::BYPASS,"post list-bail lookup bypasses to live"); // Deterministic expired-first then oldest quota eviction. MemoryStorage s3;NomadNetCache c3(s3,cfg);drain(c3,&s3);for(int i=0;i<3;i++){auto k=key((std::string("/page/")+char('a'+i)).c_str());ck(c3.beginCommit(k,body,400+i,i==0?1:100)==CacheResult::PENDING,"quota commit");drain(c3);}ck(c3.entryCount()<=2&&c3.totalBytes()<=cfg.max_bytes,"quotas bounded");ck(c3.beginLookup(key("/page/a"),500)==CacheResult::PENDING,"evicted lookup");drain(c3);ck(c3.lastResult()!=CacheResult::HIT,"expired evicted first"); diff --git a/tests/native/test_nomadnet_cache_flow.cpp b/tests/native/test_nomadnet_cache_flow.cpp index 27d4d92f..17aaa66c 100644 --- a/tests/native/test_nomadnet_cache_flow.cpp +++ b/tests/native/test_nomadnet_cache_flow.cpp @@ -36,9 +36,30 @@ int main(){int f=0;auto ck=[&](bool x,const char*n){if(!x){f++;std::cerr<<"FAIL Mem hang;hang.list_busy=true;NomadNetCache hc(hang);NomadNetCacheFlow hf(hc); CacheKey hk{"fedcba9876543210fedcba9876543210","/page/hang.mu",RequestDataClass::NIL}; ck(hf.begin(hk,1000,false)==CacheFlowState::LOOKUP,"hang lookup admitted"); - int serviced=0; - while(hf.state()==CacheFlowState::LOOKUP&&serviced<10000){hf.service();++serviced;} + uint64_t hclock=0;int serviced=0; + while(hf.state()==CacheFlowState::LOOKUP&&serviced<200000){hf.service(++hclock);++serviced;} ck(hf.state()==CacheFlowState::NEED_LIVE,"pinned recovery no longer freezes the lookup"); - ck(serviced<10000,"lookup reached live in bounded service ticks"); + ck(serviced<200000,"lookup reached live in bounded service ticks"); + ck(hc.recoveryComplete()==false,"bailed recovery is not authoritative"); + // F3: a flat (non-advancing) clock must NOT bail — the wall-time window has to + // elapse, so 500 fast no-progress ticks alone keep the cache (session) alive. + { + Mem hangf;hangf.list_busy=true;NomadNetCache hcf(hangf);NomadNetCacheFlow hff(hcf); + CacheKey hkf{"fedcba9876543210fedcba9876543210","/page/hangf.mu",RequestDataClass::NIL}; + hff.begin(hkf,1000,false); + uint64_t flat=0;for(int i=0;i<600&&hff.state()==CacheFlowState::LOOKUP;++i){hff.service(flat);++flat;} + ck(hff.state()==CacheFlowState::LOOKUP,"flat clock keeps a fast-ticking cache from bailing"); + } + // F1: a bail during an admitted RELOAD invalidation must fall through to a + // live fetch (NEED_LIVE), not report a hard "Page cache invalidation failed". + { + Mem hangr;hangr.list_busy=true;NomadNetCache hcr(hangr);NomadNetCacheFlow hfr(hcr); + CacheKey hkr{"fedcba9876543210fedcba9876543210","/page/hangr.mu",RequestDataClass::NIL}; + hfr.begin(hkr,1000,true); + uint64_t rclock=0;int svc=0; + while((hfr.state()==CacheFlowState::INVALIDATE)&&svc<200000){hfr.service(++rclock);++svc;} + ck(hfr.state()==CacheFlowState::NEED_LIVE,"reload invalidation bail falls through to live"); + ck(hfr.state()!=CacheFlowState::FAILED,"reload invalidation bail is not a hard failure"); + } } std::cout<<(f?"failed":"passed")<<"\n";return f?1:0;}