From fb068a94809eede1a8e16e1cbfe0e098faa34f87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gerg=C5=91=20Magyar?= Date: Sat, 13 Jun 2026 20:11:15 +0100 Subject: [PATCH] fix(group): pin repos during sync so large groups resolve cross-links (#2191) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(lbug): pin repos to exempt them from automatic pool eviction [#2189] Add a pinnedRepos set and pinRepo/unpinRepo to the LadybugDB pool adapter. evictLRU and the idle-timeout sweep skip pinned repos; closeOne clears the pin on teardown so explicit close always wins and pins never leak across operations. Behavior is byte-identical when nothing is pinned. Bounded multi-repo callers (group sync) can now keep more than MAX_POOL_SIZE repos resident through deferred cross-repo resolution. * fix(group): pin repos during sync so >MAX_POOL_SIZE groups resolve [#2189] syncGroup now pins each repo immediately after initLbug and releases the pin (unpin then close) in the finally. This keeps every group member resident through the deferred manifest/workspace resolution that runs after the init loop, so cross-links anchor to real graph symbols instead of falling back to synthetic UIDs when a group has more than MAX_POOL_SIZE repos. Release is unpin-before-close plus closeOne's own pin-clear, so pins never leak across syncs in the long-lived MCP server even on error. * style(test): apply prettier formatting to #2189 test files * fix(review): apply autofix feedback Clarify the pinRepo docstring: the pin does not survive teardown (closeOne clears it) and the repoId must match the key passed to initLbug. Addresses a code-review finding that the prior 'or later holds' wording contradicted closeOne's unconditional pin-clear. * refactor(lbug): reference-count pool pins so overlapping holders are safe [#2189] Change pinnedRepos from Set to Map. pinRepo increments the lease count; unpinRepo decrements and deletes the key at 0 (flooring at zero, unknown-id no-op). evictLRU, the idle sweep, and closeOne are transparent to the swap (has()/delete() keep their semantics: skip while count>=1, force-clear on teardown). A boolean Set could not represent two simultaneous holders, so the first release wrongly cleared a pin another holder still needed — the concurrent overlapping group_sync teardown race from the PR #2191 review (Finding 1). Reference counts let two windows of one sync, or two concurrent syncs sharing a repo, coexist safely: the repo stays exempt until the last lease releases. * refactor(lbug): pinRepo returns a leak-proof release disposer [#2189] pinRepo now returns a release() disposer (mirroring addPoolCloseListener) that releases its own lease exactly once — a double-call is a guarded no-op, so it can never over-decrement a sibling holder's reference count. Callers can use the leak-proof pattern `const release = pinRepo(id); try { … } finally { release(); }`. unpinRepo stays exported for explicit pairing. Addresses the PR #2191 review's P3: the exported pin primitive had no built-in pairing, so a caller that forgot to unpin would disable eviction for a repo permanently. * refactor(group): windowed manifest resolution bounds sync pool residency [#2189] Replace whole-sync pinning with windowed deferred resolution. The init loop extracts contracts without pinning (repos evict naturally); manifest links are pre-sorted and partitioned into windows whose referenced in-group repos number <= getMaxResidentRepos(), and each window re-inits + leases only its own repos, resolves, then RELEASES the leases (not closeLbug — released repos stay evictable for the LRU, which avoids stomping a concurrent MCP reader). Peak per-sync pool residency is now bounded by getMaxResidentRepos() distinct repos regardless of group size, removing the unbounded-mmap crash risk the PR #2191 review flagged (Finding 3) — without a new magic-number threshold (it reuses MAX_POOL_SIZE via an intent-named accessor). #2189 stays fixed: each window resolves against live, freshly-leased pools, so cross-links anchor to real graph symbols. partitionManifestWindows is a pure, unit-tested function (every link in exactly one window — the contract-dedup invariant). New sync-windowed-resolution.test.ts asserts the partition bound and, through the real pool, that concurrently-open Databases never exceed the resident cap for a group larger than it. Rewrote the sync.test.ts pinning block (init loop no longer pins; per-window lease/release; release-not-close). --- gitnexus/src/core/group/sync.ts | Bin 11940 -> 17612 bytes gitnexus/src/core/lbug/pool-adapter.ts | 91 ++++++- .../group/sync-windowed-resolution.test.ts | 243 ++++++++++++++++++ gitnexus/test/unit/group/sync.test.ts | 232 +++++++++++++++-- gitnexus/test/unit/lbug-pool-pinning.test.ts | 223 ++++++++++++++++ 5 files changed, 764 insertions(+), 25 deletions(-) create mode 100644 gitnexus/test/unit/group/sync-windowed-resolution.test.ts create mode 100644 gitnexus/test/unit/lbug-pool-pinning.test.ts diff --git a/gitnexus/src/core/group/sync.ts b/gitnexus/src/core/group/sync.ts index 5c535f29f4ccd18cef47e2414eee822370f4623a..a329500be5a0edb4a4588be0960b53d4c79b71aa 100644 GIT binary patch literal 17612 zcmeHP-EJI5cFwh)qJ$V`)8;fSZ({6@RuW{1UMrC-fs~;Hf)Msh*UYrl(>>eOBZ{F3 zL;W+>Uaxiqnn$?mFCr_TRhr&w?5rZo#^7EQf2 z2UVTh>B1dv@K@oiJGgtR&?TF!UG*37@9yoD`Fgup*v4G<*UaQuZ`CbhimGUz&bG@V z{BGad`L?w$vL;*G);7gI+MLffMRjU7HGf^&_F4A+)VdMcm?yf?t-fvB&1ntNoxB%_bw~L0*oJ5Bc!hNnAllkVd^!U4hh{VX z2$Ut3+_?Ycswo!jMU`j>f}8{=~>p! zSMuj8EShIcE>C(#t+}G8mLs<3(6@PeW}A1#+yAw3T(PYDiyj=OOD8j3 zdpS`E^*;45k1jVW+nk7c4`g%hRp7?UUNl8(gQ5gY?W3UzI=YSQ`noCdrR}Yq$%9Eg z8?06hHXk^nr*XmhU7508!WMMUVS40=!nW_ljusUx&;krMXP4Fd`KB%E${CuAsD95O z7r@ItnBu81Kbj<=v6+g^Pz!Qs?%g+FI4$t+ho-1o1 zaGu)3?_hPO29^=xab%V>XfI&Z!=nNC4gyfFwq<#r;N9C|^NJh;*cptl%_=??P>+iS zPZc^JW@TAloV8gwS`2#3YBzdlDcsMi@^T26=bfx)HqU{w>%Y6cb!#sV;5oKsD=Y}I z=cXfj{Q3>?n+J=G_t84*t8FD(Wb^$y!6yfNNFwaj8|y}LHuwR&I<@n<$$uXx&JE!$ z$k|BsQV;O-~*dW0#Xi&3xm#nWzL#= zMqs5CgdlC}r}c$x9^plDD8zu=wn2JwET+3py)-EnDIdw=1yno%b+Myvq4%a<^iVpK zli~OmCXv^Bjx_h~-BYwa{d9Pg?vSI*Z7lLAD_k@-sQL49m&yKO3S{>c>=UQa%`BIN6H62+Kn{LBCItGGn? z78#b?MFx3;)KTqJos$o-M~LIWWe!H;=d0&5+K64{qy@PWl$AW>cZWv0D5^XGfkee^ zB^(hR^i6}O3Hf&09)>$p_mvn5nX%UsO31FP_65@e^EXx0AQR9i1;8+fKjieZvqj-C z73qb%vLbikxsTVvtKOfw;+m-9AL!QcXP@0NpP9!t-y#SUZJ(6lm)lj=0u2~%4PGI` z#z8Gaw6<#qw5@V|-_|C)B`}1^;+4Av)P=Bu`z1U}!6{1zY7u7_jcX^)+RDHd0Jam5 zgf<6<@IZFer2}+3w+MUTP7<3g(<4yFITOMUqu2|;!xR9HZ}sGmw~S`YC9-y4*kBpr z``2%J=0R|~XDWMP&TQ+g)&0I*KtMU%e9x%8>sbOGNJAh82W?f6d225rFHkYQ6J3R6 z+0@t5Y5$SCT+izA`y#*o(LZkM(Kmgn+PAK|{~X6sf$MUmv55x~nJU*;i<7{V>yd&H%S6?`0Y2ssiNnj;UIh?~SS zxDAK}jAz9an(3GesAzqWlB+w8EySs>WR0-S^LHncP`*I+&6DAQiJne|NuKnVFyd<-c&Nn2QN&G=<{zdXf^TL6Jsk*B0W+M)ZKP)&4+76|fn|rg!eB z{OZ>J(xLsOkWW69jC8&@pd2wp~^21!}5J%Wb_45(TVr=Xt%J6&2uL6sROL7Qv#kld5hf^Afs2fOG`_2)|)g z+nGXzTe$?FtXZ2(jJB9p<(b3pU{}P`K#3CEp0EVGfmHjG@*{(@jGTqv)#}2?y>C6G zAON8@VHO!|w5SF^3Pa3EM_^u*i_OX3J$&@?>EHLH$Rv=RW(#@-!H1cJ9wRnFC1Op) za&bV>^g92*4P4S}+bX)=gCH>j#+fo0C$|?hVFfZ=)Y~#Qc8&YP?jx#!tg9bE80`p& zDWJ(_VDZe+S8(^KSRPUZ5YOLMKvN!_nc`Ivni1j<*>Xpd30IW5)?$ZgOx4P`tP4S?$A9v!Wu&SVT*{8 zE(1Q8mjElh@~OA;b}>4=Nb_I=UWt8l3GBKE?R2-&^7;(}h3@W%@H?N!S3RpUMdE|x zqdB?-rVh^-I8bVXd%l2=OUgY*#*iGF?%fxk^STr_cw|aMs&5dUm9SMv3?)A6kQ1NP z9Et~0S4x%rTXCl!K8o?6M=L2q;EVradvcimfY>KF_=|%PU@#1hJxx;~-Wy$=WUz~Z zmp?nn4%4zm?1Hp?4Hj51S&&L*heQ<@R9wBV?jc2?Cgy3^eoWVmW zq8eK2i7*TfyW+(ml5#~&MsQMC9#DjQi?_;3*|u;aR@}y(+Y*Ghz>BoB)v{e_pb!E3 zHR23Dr`iOt_xl3X9dCjvYn(yU0z}}pZF+ZSjKc6E{44Rf+(p-+R6(mgMM#Fi5JeGk zJC5ZT6B7isD0fHkjWInEOF~ClB2>b9ftfNwQY48Ckq{g_lZV&vQNMpD@qyP}O?jdQ zGXatihUp|Kt6qB^X_?j3o0ds`ueJhRiiHyFJ(X-hAoVrZ6ozD@uERf7yirdq`C{NAfra+nkmI9(O3}zsp4!mMeHM6Ma zFk79_ZJN)$$K4=<(S3)hfoa0-*`(_8lEaO zuQbvw>v|I-3LhoGYwFcK(UKT7Mt6017i;MY!CCx=?va6CU=W+Cz1UjzPhk> zLno>v^T7+lT`00`5ZEDD16R?MX;HwDupqCus1hMYi!o1z{B>dBGAH$>BO77?9C)hB z0_EuL60zs+>LM4q;AnP6`j*tzAhf7ox>LziOF|2PRjA+C+S^2_D16Zwxq-R>UUrc& zV8gD=8O0$g^PpkEs;B0kFnY$85FAC0=|@)yL;Wj4?e~4c)*wDCeCgWJZoNrZCbA9& zg^T;YPowae2M@@{-GYxlAewNsGHk}2yazZ6>~05Vj3v7+arvN2GhV6PmAH1)k=a?T z%@&Wx%R%d9yw*2NBIt3Oh3g?L{OS3N*P^pjI69yu%gk?>5>`~uXS^7E>=Zp#X`l4i20|PrM+4}@>fGYVVLWc zuqMdW@EdZw4cr#M&MX9hz+in~DTV;iC}=Wf(bY+}B$^S3sCj(q7pQWGN6u8?^KNRwi{-c}aA=y6E3gU=a46YbV= z0WyuUuGsBqtk4R#fyoI3({vV=%ey`oeBTH^DQO4|JD(`Er_QhT4i(lQ^KlN@BWee~ z{T=hp_=Edu_drNXL7*3?P+XCmJbLOLL=x7c*oV|}o+%uX)`3)6gXcsZ;ytFh?lhdZ zs0sv1AbuG(`jGJmuUM%>PagrJPjJFLbmy?+AH{mGmB#gI@9Dk+*|+vLbTbpLbi6We zIJ}F*tP)wKx<93b9@>rW#-dA-5vfwDH4ok}2_!i(qcg*UfvyhB<(#(Bd(|$32^2%) ztRH2n@#<`4ScWWZ9*W}OnHJ~vRMWc!zNdXZbpNyI?jzZsP5Bu4{!05_NPCrK%=2aR8Xu2=xe`M(2nq-0Y#a&)Puye|F(*KM zj(OfU@K8xO@W>o&+r{L!Lug_58hiid?D=XbF0D$S$fE9o0HSL36a!;)OB^Qn1#`T^n|GachtEeGZ@nH%A z?Krl_vWpDzvD=rSV-ZpZv}@7$(`f;$d213TJP7kAnDB9zuC-|O;0M5Bxda`fWyRbm zpPl%@IEPk;c0?*`0Un75BYReuv0SIpJ$*Y)+H5q%Z8-S zEu@H9mQSS(sI`8mFAA@|=d=Rn>u;nHf!`h# zVmNK`OX@oR@5KjJeMTt!5}lD^7%!oDhZ`qaI>Zu{EJnymz6*D%Q){6_7Q`?LrZ8+! zK+#e^EiPJxu{0b{AVLAGFZgTUf?0Dv8bJ}VFAd?(BZCBE2;fZ@@e{fh2mLT#AL1a_ zVXxdoIOy%RJ>j7=pi`$~l|{9v*|_WfoTE(=$7Lya=&gDUMP~NNm6NMILf5{9Rp2}y z=C@oLb#gf8cCC#P)ncy0G!F|@e*a+SHAj8;(wz@&b)+!y3L58R!Ep zrB9oMy4dj|gF)jI0xCWjjG-k`cv9c#z{!BC10AHDyvzy?{V8;g2cS_eooF6_E^#Pa zp)AwMq}IedNEP_OrG2kTreeR98lX!FI?=u->D*Iy_6;agfv$bYQ{b_c4g1E`q0`D= zcbl!ZJ6uACZz$&h++GF}IWNDMSg3~bd#>@pG5ZQ{I#(h$2op}6T?$iBKwt8KYp}() zMZ%1V@7XKiwSqc@{72lhp^H%H@LFdtL>G7xXEM-_XYU1gz-yuXZqR-o%5_hwgG`TX zRPcncH2lmD#h-+)55~1*Em6@W+}NiNOg!qT0SdjlpMZ!gbW#a($#Kj697detnSy%5 z4wVf4&|fP?VelvDHIzo2EZ}jFMu>(lM>s-sdh+z-;n|5v;v@+tr!eclQ5JNGd}~GZ zC{zOh58bXB zYcGAe0V!4bUN|7--VR>Iz7N&8wU}pOH;d|2>3A@V3xo`vT}-XD=Gmhcz%%32;yOd! z5%WKo+mRpK^2VVjXYQC!&2M5ov!}kw0&xn6Nb8G%H&Ff6C829IH4mF6yX?2DAW_=P zRZPZ-Lmx^Eys&57HXN2RdjSf;DiGmw{sK&Uf@n=a^u|RxiRYHyEn}~tyQpt_>OQ5lzIJ#9RPGN`c_@?r#1g|6N@?`AFL-p+xWN?7{je0)vZ0%g=XxZ5lqvybm8NwU^(nF++9=qQn4?Y-b-w-;VbBawrS!0wC ze(zC9bAb>uYn&h-gnn`re6o*sLeQLias?j5OfkOA>BczF`Z-P-?KKl0Dh@J`>s}0x zVvL0C*sCN)L@(o2*Pi5wJM0DEk!T^z2Ej&Lgau^dPe(0exB|Tn4=gYWnK>MNH^+gv zRi1cx&>X!IQwVU#;n$cCTX6a%3|68^y241KbY4^QlCvIChe3@1Gn-<8bq-QifrnXu zrq@Ii(O@XRH&2O8ki{meVq8H&-8cmj0PY(G(K!%=Crl^69i|9=5>_G&FM~dA;ferl z7_#ifDTzc6obp5mSaO1Xyr8jy_xt-bX{Z!TnT1@cJ0kUJagcs>-#@}X?tT=R49vz+ z5Vi3AqsWTFx`H~ONQQ5Xk(KygAvkcl{jVy=@CF!s2s{F8`4UqB4!+Aw;(0K?{Tl&0 zkX(dy{CP@O1LBp`>S+i)FE*U>g|rS3*HK{w7jzw8YkIwubb~%#+UGJdj5Lt+c!}g= zdLUok9dw_DKvz)3g)>)x@?A{$90nmTK5Rj5B2jQS%OK@+oHpRpx3tL@B{OHt53&7^ z4`i4{R-6sMg=Kso!zYo@l^z-a*a4>#P|)mVs7VD*1Y;J8K_~=#bXZlH-Z^klWjHPJZ-7kac6Ck1z1S{CP zhJP8|BL7BR8UJ?vMxs1?)X5RA+pPAa@$1hJv}vfe_Y+>J52XQy%mv9e`QC&>5c=Qn zvK+C6ol0Qw-WpCRLa3p!>&)PDiMoOI*hX##F~b-{!M^z#2gJI>TP3XMygT z3I_^E&jA6d(>Ze<52-Z$$;beWGV15{pgDDg`T7!t+iipR8PNa+{xRi)kAG0lLUIQI z$Dlp)CH(+7K*{a0LGA=^gE47*xx#S=NdMUE=czf{>LzD8@xg+_|L+Jx`@nBx1V|_(?X3`9aUZ%pE_{j&Rh!DlWz-sT-CgM5r~mlR z|NWnT9k4))cy~KDp&|}Bg2AQ4k=H7dk~&eQMPwOaOyEPz`LbvsS(u*^O%-0AQ$R7n x-CU-zPZdZ6NfIAD_^k7T3>q^hIz&j5_nSzK*C;H4YI@yWChc$&w(9!U{{hUcIw=4E delta 737 zcmYjP&ubGw6lQafko2ZCjqTw<5AI@|q*7`cjfg?2P*56gA;SK6=}t49S$1YkLM_&t zJz1HP=%ELXg6M)*!L#@$=wBdSJ&Cj1reRs3Z7*8pIYx;-1R`h9~DPcOw}JRfDb)F$M;wV%IlyCl*p} zQa*(Awe_j2Cl`Me80wu0nG3wTWAqc#AY1zH>e3uMg@gwS_xjL=UOzp5sZr1`7ta*}Pyb%6r=6u2R%HfHqC|#vlsD^RxS-cyoMGQy zuNLu?JUxcP(lfIR?7Ex|&@kdSS5F0pIN`<_Ndp8GQVAka7=wx@B`{yT4jvQ!5Tt;a zb#e!j7$ulruqO!awri#+VAK?u9tk#;GC^2gX*RAo8)Q1~QHWq1a2cC`hfIu4;*h6Z z?jYySr1)YtVyFy@EsUfv79*Mdx$?s*&qzssUj1O{gT~^*18)a?MTR($wlUV^NMaMC zOy4)YSjFk?y4@rThCGW*)@GM-Zj|)FJ44`Mma~wezl|2)r(|2Gmw+O%;xm5jsq&5v4;=9TZ WZQFtNE!f*PQnPgS(>FI)ivIv^t_Cdt diff --git a/gitnexus/src/core/lbug/pool-adapter.ts b/gitnexus/src/core/lbug/pool-adapter.ts index e5338e3d0..11b1f7690 100644 --- a/gitnexus/src/core/lbug/pool-adapter.ts +++ b/gitnexus/src/core/lbug/pool-adapter.ts @@ -103,6 +103,31 @@ const IDLE_TIMEOUT_MS = 5 * 60 * 1000; // 5 minutes /** Max connections per repo (caps concurrent queries per repo) */ const MAX_CONNS_PER_REPO = 8; +/** + * Repos exempt from AUTOMATIC eviction (LRU + idle timeout) until explicitly + * unpinned. Used by bounded multi-repo operations like `group sync`, which + * initializes one pool per repo and then resolves cross-repo manifest/workspace + * links against ALL of those pools after the init loop. Without pinning, a + * group larger than MAX_POOL_SIZE would LRU-evict the earliest repos before + * resolution runs, leaving the deferred executor closures pointing at dead pool + * entries (issue #2189). + * + * Pins are REFERENCE-COUNTED: the map holds repoId → active lease count. This + * lets overlapping holders (two windows of one sync, or two concurrent + * `group sync` calls sharing a repo) coexist safely — the repo stays exempt + * until the LAST holder releases. A boolean Set could not represent "two + * holders," so the first release would wrongly clear a pin another holder still + * needs (PR #2191 review, Finding 1). + * + * Pins block only automatic eviction (LRU + idle). Explicit teardown + * (closeOne / closeLbug) always closes the entry and force-clears its count — + * teardown is authoritative. A present key always means count ≥ 1. While every + * pooled repo is pinned, evictLRU finds no eligible victim and the pool may + * transiently exceed MAX_POOL_SIZE — the same soft-cap behavior that already + * occurs when every entry is checked out. + */ +const pinnedRepos = new Map(); + // Behavior-neutral RSS tracing for the FTS evict→reload memory repro // (gitnexus/scripts/bench/fts-evict-reload-rss.mjs). Two invariants keep it safe // in the pool init/close hot path: it writes ONLY to stderr (stdout is the MCP @@ -145,6 +170,7 @@ function ensureIdleTimer(): void { idleTimer = setInterval(() => { const now = Date.now(); for (const [repoId, entry] of pool) { + if (pinnedRepos.has(repoId)) continue; if (now - entry.lastUsed > IDLE_TIMEOUT_MS && entry.checkedOut === 0) { closeOne(repoId); } @@ -167,7 +193,64 @@ export const touchRepo = (repoId: string): void => { }; /** - * Evict the least-recently-used repo if pool is at capacity + * Acquire one eviction-exemption lease on a repo (LRU + idle timeout) by + * incrementing its reference count. The repoId must match the key passed to + * initLbug (e.g. group sync leases by handle.id — the same id it inits with). + * Leasing a repoId before it enters the pool is allowed and protects the entry + * once it is created, but the lease does NOT survive a teardown: closeOne + * force-clears the count, so a later re-init of the same repoId starts + * unpinned. Each pinRepo MUST be balanced by exactly one release (the repo + * stays exempt until the last lease is released). See the pinnedRepos docstring + * for the full contract. + * + * Returns a `release` disposer (mirroring addPoolCloseListener) that releases + * THIS lease exactly once — calling it twice is a no-op, so it can never + * over-decrement a sibling holder's count. Prefer the disposer + * (`const release = pinRepo(id); try { … } finally { release(); }`) so the + * pin/release pair is leak-proof; unpinRepo remains available for callers that + * pair explicitly. + */ +export const pinRepo = (repoId: string): (() => void) => { + pinnedRepos.set(repoId, (pinnedRepos.get(repoId) ?? 0) + 1); + let released = false; + return () => { + if (released) return; + released = true; + unpinRepo(repoId); + }; +}; + +/** + * Release one eviction-exemption lease on a repo. The repo becomes eligible for + * automatic eviction again only once its count reaches 0 (the key is deleted). + * Idempotent at the floor: releasing a repo with no active lease is a no-op (no + * negative counts). Does NOT close the repo's pool. + */ +export const unpinRepo = (repoId: string): void => { + const count = pinnedRepos.get(repoId); + if (count === undefined) return; + if (count <= 1) { + pinnedRepos.delete(repoId); + } else { + pinnedRepos.set(repoId, count - 1); + } +}; + +/** + * Maximum number of repos a bounded multi-repo operation (e.g. group sync's + * windowed manifest resolution) should hold resident at once. Equals + * MAX_POOL_SIZE today, but exposed under an intent-named accessor so callers + * size their working set against "max repos a bounded op should hold" rather + * than coupling to the LRU eviction-cap constant, which may be tuned + * independently. + */ +export const getMaxResidentRepos = (): number => MAX_POOL_SIZE; + +/** + * Evict the least-recently-used repo if pool is at capacity. + * Pinned repos are never chosen as the eviction victim — when every eligible + * entry is pinned, no eviction occurs and the pool transiently exceeds + * MAX_POOL_SIZE (see the pinnedRepos docstring). */ function evictLRU(): void { if (pool.size < MAX_POOL_SIZE) return; @@ -175,6 +258,7 @@ function evictLRU(): void { let oldestId: string | null = null; let oldestTime = Infinity; for (const [id, entry] of pool) { + if (pinnedRepos.has(id)) continue; if (entry.checkedOut === 0 && entry.lastUsed < oldestTime) { oldestTime = entry.lastUsed; oldestId = id; @@ -244,6 +328,11 @@ function closeOne(repoId: string): void { pool.delete(repoId); + // Clear any eviction pin — the entry is gone, so the pin is meaningless and + // would otherwise leak across operations in a long-lived process. Teardown + // is authoritative: an explicit close always wins over a pin. + pinnedRepos.delete(repoId); + // Notify listeners AFTER the pool entry is gone so any cache-invalidation // they perform is consistent with `isLbugReady(repoId) === false`. for (const listener of poolCloseListeners) { diff --git a/gitnexus/test/unit/group/sync-windowed-resolution.test.ts b/gitnexus/test/unit/group/sync-windowed-resolution.test.ts new file mode 100644 index 000000000..d7cdb4090 --- /dev/null +++ b/gitnexus/test/unit/group/sync-windowed-resolution.test.ts @@ -0,0 +1,243 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { mkdtempSync, writeFileSync, mkdirSync, rmSync } from 'node:fs'; +import type { GroupConfig, GroupManifestLink } from '../../../src/core/group/types.js'; + +// Two test surfaces for the windowed manifest resolution (issue #2189 / PR #2191 +// review, Finding 3 — bound peak pool residency to MAX_POOL_SIZE regardless of +// group size): +// +// 1. partitionManifestWindows — a pure function; the bounded-residency logic +// lives here (every window references <= maxResident repos, every link in +// exactly one window). Tested directly, no pool. +// 2. A real-pool integration test that drives syncGroup through the actual +// pool (native LadybugDB layer mocked, as in lbug-pool-pinning.test.ts) and +// asserts the count of concurrently-open Databases never exceeds the +// resident cap — the end-to-end residency bound the review flagged as +// missing. + +// ── Surface 1: pure partition function ────────────────────────────────────── + +describe('partitionManifestWindows (issue #2189 windowed resolution)', () => { + const link = (from: string, to: string): GroupManifestLink => ({ + from, + to, + type: 'http', + contract: `GET::/${from}-${to}`, + role: 'consumer', + }); + + it('keeps every window within maxResident repos and places every link exactly once', async () => { + const { partitionManifestWindows } = await import('../../../src/core/group/sync.js'); + const repos = ['r1', 'r2', 'r3', 'r4', 'r5', 'r6', 'r7', 'r8']; + const known = new Set(repos); + // A star: every leaf links to the hub r1, plus a few leaf-leaf links. + const links = [ + link('r1', 'r2'), + link('r1', 'r3'), + link('r1', 'r4'), + link('r1', 'r5'), + link('r1', 'r6'), + link('r7', 'r8'), + link('r2', 'r3'), + ]; + const maxResident = 5; + const windows = partitionManifestWindows(links, known, maxResident); + + // Bounded residency: no window references more than maxResident repos. + for (const w of windows) expect(w.repos.size).toBeLessThanOrEqual(maxResident); + + // True partition: every link appears in exactly one window. + const placed = windows.flatMap((w) => w.links); + expect(placed).toHaveLength(links.length); + const placedKeys = placed.map((l) => `${l.from}->${l.to}`).sort(); + const inputKeys = links.map((l) => `${l.from}->${l.to}`).sort(); + expect(placedKeys).toEqual(inputKeys); + // No link appears twice (the contract-dedup invariant — KTD-4). + expect(new Set(placedKeys).size).toBe(placedKeys.length); + }); + + it('counts only in-group repos toward a window; dangling links consume no budget', async () => { + const { partitionManifestWindows } = await import('../../../src/core/group/sync.js'); + const known = new Set(['r1']); + const links = [ + link('r1', 'external-a'), // 1 in-group repo + link('external-b', 'external-c'), // 0 in-group repos (fully dangling) + ]; + const windows = partitionManifestWindows(links, known, 5); + // Both links are still placed (so they yield synthetic-UID contracts)... + expect(windows.flatMap((w) => w.links)).toHaveLength(2); + // ...but the only repo counted is r1. + const allRepos = new Set(windows.flatMap((w) => [...w.repos])); + expect(allRepos).toEqual(new Set(['r1'])); + }); + + it('returns no windows for an empty link set', async () => { + const { partitionManifestWindows } = await import('../../../src/core/group/sync.js'); + expect(partitionManifestWindows([], new Set(['r1']), 5)).toEqual([]); + }); + + it('splits links across multiple windows when referenced repos exceed maxResident', async () => { + const { partitionManifestWindows } = await import('../../../src/core/group/sync.js'); + const known = new Set(['r1', 'r2', 'r3', 'r4', 'r5', 'r6']); + // 3 disjoint repo-pairs = 6 distinct repos; maxResident 2 forces ≥3 windows. + const links = [link('r1', 'r2'), link('r3', 'r4'), link('r5', 'r6')]; + const windows = partitionManifestWindows(links, known, 2); + expect(windows.length).toBeGreaterThanOrEqual(3); + for (const w of windows) expect(w.repos.size).toBeLessThanOrEqual(2); + expect(windows.flatMap((w) => w.links)).toHaveLength(3); + }); +}); + +// ── Surface 2: real-pool residency bound through syncGroup ─────────────────── + +const { loadFTSExtensionMock, openCounter } = vi.hoisted(() => ({ + loadFTSExtensionMock: vi.fn(), + openCounter: { live: 0, peak: 0 }, +})); + +vi.mock('@ladybugdb/core', () => ({ + default: { + Database: vi.fn(), + Connection: vi.fn(function (this: any) { + this.query = vi.fn().mockResolvedValue({ + getAll: vi.fn().mockResolvedValue([]), + close: vi.fn(), + }); + // executeParameterized's prepare/execute path (manifest resolveSymbol). + this.prepare = vi.fn().mockResolvedValue({ + isSuccess: () => true, + getErrorMessage: vi.fn().mockResolvedValue(''), + }); + this.execute = vi.fn().mockResolvedValue({ + getAll: vi.fn().mockResolvedValue([]), + close: vi.fn(), + }); + this.close = vi.fn().mockResolvedValue(undefined); + }), + }, +})); + +vi.mock('../../../src/core/lbug/lbug-adapter.js', () => ({ + isReadOnlyDbError: vi.fn(() => false), + loadFTSExtension: loadFTSExtensionMock, +})); + +vi.mock('../../../src/core/lbug/lbug-config.js', () => ({ + // Track concurrently-open Databases: a fresh fake per open, decrement on close. + createLbugDatabase: vi.fn(() => { + openCounter.live += 1; + openCounter.peak = Math.max(openCounter.peak, openCounter.live); + return { + init: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockImplementation(async () => { + openCounter.live -= 1; + }), + }; + }), + toNativeSafePath: vi.fn((p: string) => p), + isWalCorruptionError: vi.fn(() => false), + WAL_RECOVERY_SUGGESTION: '', +})); + +vi.mock('../../../src/core/lbug/sidecar-recovery.js', () => ({ + preflightLbugSidecars: vi.fn().mockResolvedValue(undefined), + isMissingFsError: vi.fn(() => false), + isMissingShadowSidecarError: vi.fn(() => false), + isReadOnlyShadowReplayError: vi.fn(() => false), + quarantineWalForMissingShadow: vi.fn().mockResolvedValue(''), + renameFailureMessage: vi.fn((p: string) => `rename failed for ${p}`), + statIfExists: vi.fn().mockResolvedValue(null), +})); + +// readRegistry is called in syncGroup's else branch; resolveRepoHandle is +// supplied, so an empty registry is fine (only the meta.json fallback reads it). +vi.mock('../../../src/storage/repo-manager.js', () => ({ + readRegistry: vi.fn().mockResolvedValue([]), +})); + +const { syncGroup } = await import('../../../src/core/group/sync.js'); +const { closeLbug, getMaxResidentRepos } = await import('../../../src/core/lbug/pool-adapter.js'); + +describe('syncGroup windowed resolution bounds pool residency (real pool, #2189)', () => { + let tmpRoot: string; + + beforeEach(() => { + tmpRoot = mkdtempSync(path.join(os.tmpdir(), 'gn-window-resid-')); + loadFTSExtensionMock.mockResolvedValue(true); + openCounter.live = 0; + openCounter.peak = 0; + }); + + afterEach(async () => { + await closeLbug().catch(() => {}); + rmSync(tmpRoot, { recursive: true, force: true }); + }); + + it('never holds more than getMaxResidentRepos() Databases open for a large group', async () => { + const maxResident = getMaxResidentRepos(); + const repoCount = maxResident + 4; // exceed the cap so windowing must split + + const repos: Record = {}; + const links: GroupManifestLink[] = []; + for (let i = 1; i <= repoCount; i++) { + const gp = `app/repo-${i}`; + repos[gp] = `repo-${i}`; + // Star topology: every repo links to repo-1 → many windows reference repo-1. + if (i > 1) { + links.push({ + from: gp, + to: 'app/repo-1', + type: 'http', + contract: `GET::/api/${i}`, + role: 'consumer', + }); + } + } + + const config: GroupConfig = { + version: 1, + name: 'test', + description: '', + repos, + links, + packages: {}, + // All detection off → init loop just opens pools (no extractor file reads). + detect: { + http: false, + grpc: false, + thrift: false, + topics: false, + shared_libs: false, + embedding_fallback: false, + workspace_deps: false, + }, + matching: { bm25_threshold: 0.7, embedding_threshold: 0.65, max_candidates_per_step: 3 }, + }; + + await syncGroup(config, { + resolveRepoHandle: async (_name, groupPath) => { + // Each repo gets a real storage dir with a fake lbug file so fs.stat in + // doInitLbug succeeds; distinct paths → distinct Databases. + const storagePath = path.join(tmpRoot, groupPath); + mkdirSync(storagePath, { recursive: true }); + writeFileSync(path.join(storagePath, 'lbug'), ''); + return { + id: groupPath.replace(/\//g, '-'), + path: groupPath, + repoPath: storagePath, + storagePath, + }; + }, + skipWrite: true, + }); + + // The init loop (no pin) keeps the pool at the LRU cap; windowed resolution + // leases <= maxResident repos per window and releases them. Peak concurrent + // open Databases must stay within the resident cap (+ at most one transient + // overshoot at an init boundary — the pool's documented soft cap). + expect(openCounter.peak).toBeGreaterThan(0); + expect(openCounter.peak).toBeLessThanOrEqual(maxResident + 1); + }); +}); diff --git a/gitnexus/test/unit/group/sync.test.ts b/gitnexus/test/unit/group/sync.test.ts index 4fc320076..061f5cfc4 100644 --- a/gitnexus/test/unit/group/sync.test.ts +++ b/gitnexus/test/unit/group/sync.test.ts @@ -166,20 +166,19 @@ describe('syncGroup', () => { expect(result).toBeDefined(); }); - it('test_syncGroup_closes_only_opened_pools', async () => { + it('test_syncGroup_does_not_force_close_pools (release-not-close, #2191 review)', async () => { + // Post windowed-resolution refactor, syncGroup releases its eviction leases + // and lets the pool's LRU reclaim repos — it does NOT call closeLbug. This + // avoids tearing down a pool entry a concurrent MCP reader may share. const config = makeConfig({ 'app/backend': 'backend-repo', 'app/frontend': 'frontend-repo', }); - const closedIds: string[] = []; - const { vi } = await import('vitest'); const poolAdapter = await import('../../../src/core/lbug/pool-adapter.js'); const initSpy = vi.spyOn(poolAdapter, 'initLbug').mockResolvedValue(undefined); - const closeSpy = vi.spyOn(poolAdapter, 'closeLbug').mockImplementation(async (id?: string) => { - if (id) closedIds.push(id); - }); + const closeSpy = vi.spyOn(poolAdapter, 'closeLbug').mockResolvedValue(undefined); try { await syncGroup(config, { @@ -192,19 +191,8 @@ describe('syncGroup', () => { skipWrite: true, }).catch(() => {}); - // closeLbug must have been called at least once with specific pool ids - expect(closeSpy.mock.calls.length).toBeGreaterThan(0); - expect(closedIds).toContain('app-backend'); - expect(closedIds).toContain('app-frontend'); - - // Every call must have a truthy string id - for (const id of closedIds) { - expect(id).toBeTruthy(); - expect(typeof id).toBe('string'); - } - // No blanket close (no-arg or empty-string or undefined) - const blanketCalls = closeSpy.mock.calls.filter((args) => args.length === 0 || !args[0]); - expect(blanketCalls).toHaveLength(0); + // No closeLbug — repos are left evictable for the LRU to reclaim. + expect(closeSpy.mock.calls.length).toBe(0); } finally { initSpy.mockRestore(); closeSpy.mockRestore(); @@ -534,7 +522,9 @@ service OrderService { }, }); expect(initSpy).toHaveBeenCalledWith('billing-repo', path.join(storageDir, 'lbug')); - expect(closeSpy).toHaveBeenCalledWith('billing-repo'); + // syncGroup no longer force-closes pools (release-not-close, #2191 review); + // repos are left evictable for the LRU. Assert no teardown call here. + expect(closeSpy).not.toHaveBeenCalled(); } finally { initSpy.mockRestore(); closeSpy.mockRestore(); @@ -1044,18 +1034,20 @@ service OrderService { skipWrite: true, }); - // Manifest symbol resolution must run while pools are still open + // Manifest symbol resolution runs against live (leased) pools. expect(manifestResolvedWhilePoolOpen).toBe(true); - expect(closeLbugCalled).toBe(true); // The manifest cross-link must use the real UID from the DB, not synthetic + // — the #2189 fix, now via windowed resolution (the svc/orders↔svc/payments + // link forms one window whose repos are re-inited + leased for resolution). const manifestLinks = result.crossLinks.filter((cl) => cl.matchType === 'manifest'); expect(manifestLinks).toHaveLength(1); expect(manifestLinks[0].to.symbolUid).toBe('real-uid-checkout'); expect(manifestLinks[0].to.symbolUid).not.toContain('manifest::'); - // closeLbug must fire exactly twice (one per repo) - expect(closeSpy).toHaveBeenCalledTimes(2); + // syncGroup no longer force-closes pools (release-not-close, #2191 review). + expect(closeLbugCalled).toBe(false); + expect(closeSpy).not.toHaveBeenCalled(); } finally { initSpy.mockRestore(); closeSpy.mockRestore(); @@ -1105,6 +1097,198 @@ service OrderService { }); }); +// Lifecycle wiring for issue #2189: syncGroup must pin every repo it +// initializes (so a group larger than MAX_POOL_SIZE survives deferred +// manifest/workspace resolution) and release those pins on completion AND on +// error. The eviction-survival MECHANISM itself is proven against real +// evictLRU in test/unit/lbug-pool-pinning.test.ts; these tests prove the sync +// loop drives that mechanism correctly. (A full end-to-end proof through the +// real pool — real symbolUid instead of synthetic after >5 repos — would +// require a real or fully-native-mocked LadybugDB stack; mechanism + wiring +// coverage stands in for it here.) +describe('syncGroup windowed manifest resolution (issue #2189 / PR #2191 review)', () => { + const groupConfig = (count: number, links: GroupManifestLink[] = []): GroupConfig => { + const repos: Record = {}; + for (let i = 1; i <= count; i++) repos[`app/repo-${i}`] = `repo-${i}`; + return { + version: 1, + name: 'test', + description: '', + repos, + links, + packages: {}, + detect: { + http: true, + grpc: false, + thrift: false, + topics: false, + shared_libs: false, + embedding_fallback: false, + workspace_deps: false, + }, + matching: { bm25_threshold: 0.7, embedding_threshold: 0.65, max_candidates_per_step: 3 }, + }; + }; + + const okHandle = async (_name: string, groupPath: string): Promise => ({ + id: groupPath.replace(/\//g, '-'), + path: groupPath, + repoPath: '/tmp/' + groupPath, + storagePath: '/tmp/' + groupPath + '/.gitnexus', + }); + + const httpLink = (from: string, to: string): GroupManifestLink => ({ + from, + to, + type: 'http', + contract: 'GET::/api/x', + role: 'consumer', + }); + + // pinRepo now returns a release disposer; the spy returns a tracked spy fn so + // tests can assert every acquired lease was released. + const setupPoolSpies = async () => { + const poolAdapter = await import('../../../src/core/lbug/pool-adapter.js'); + const releaseSpies: Array> = []; + const initSpy = vi.spyOn(poolAdapter, 'initLbug').mockResolvedValue(undefined); + const execSpy = vi.spyOn(poolAdapter, 'executeParameterized').mockResolvedValue([]); + const pinSpy = vi.spyOn(poolAdapter, 'pinRepo').mockImplementation(() => { + const release = vi.fn(); + releaseSpies.push(release); + return release; + }); + const restore = () => { + initSpy.mockRestore(); + execSpy.mockRestore(); + pinSpy.mockRestore(); + }; + return { releaseSpies, initSpy, execSpy, pinSpy, restore }; + }; + + it('pins only the repos referenced by manifest links, not the whole group', async () => { + const { pinSpy, restore } = await setupPoolSpies(); + try { + await syncGroup(groupConfig(8, [httpLink('app/repo-1', 'app/repo-2')]), { + resolveRepoHandle: okHandle, + skipWrite: true, + }); + const pinnedIds = pinSpy.mock.calls.map((c) => c[0]).sort(); + // Only the windowed (link-referenced) repos are leased — bounded residency, + // not the whole 8-repo group. + expect(pinnedIds).toEqual(['app-repo-1', 'app-repo-2']); + expect(pinnedIds).not.toContain('app-repo-3'); + } finally { + restore(); + } + }); + + it('does not pin during the init loop when there are no manifest links', async () => { + const { pinSpy, restore } = await setupPoolSpies(); + try { + await syncGroup(groupConfig(8, []), { resolveRepoHandle: okHandle, skipWrite: true }); + // The init loop extracts contracts without pinning; with no links there + // are no resolution windows, so nothing is ever pinned. + expect(pinSpy.mock.calls.length).toBe(0); + } finally { + restore(); + } + }); + + it('releases every window lease on successful completion', async () => { + const { releaseSpies, restore } = await setupPoolSpies(); + try { + await syncGroup( + groupConfig(8, [ + httpLink('app/repo-1', 'app/repo-2'), + httpLink('app/repo-7', 'app/repo-8'), + ]), + { resolveRepoHandle: okHandle, skipWrite: true }, + ); + expect(releaseSpies.length).toBeGreaterThan(0); + for (const release of releaseSpies) expect(release).toHaveBeenCalled(); + } finally { + restore(); + } + }); + + it('releases the window leases even when resolution throws mid-window', async () => { + const { ManifestExtractor } = + await import('../../../src/core/group/extractors/manifest-extractor.js'); + const { releaseSpies, restore } = await setupPoolSpies(); + const manifestSpy = vi + .spyOn(ManifestExtractor.prototype, 'extractFromManifest') + .mockRejectedValue(new Error('resolution boom')); + try { + await expect( + syncGroup(groupConfig(8, [httpLink('app/repo-1', 'app/repo-2')]), { + resolveRepoHandle: okHandle, + skipWrite: true, + }), + ).rejects.toThrow('resolution boom'); + // The window's finally released its acquired leases despite the throw. + expect(releaseSpies.length).toBeGreaterThan(0); + for (const release of releaseSpies) expect(release).toHaveBeenCalled(); + } finally { + restore(); + manifestSpy.mockRestore(); + } + }); + + it('does not pin a repo that fails to resolve (no pool handle)', async () => { + const { pinSpy, restore } = await setupPoolSpies(); + try { + await syncGroup(groupConfig(3, [httpLink('app/repo-1', 'app/repo-2')]), { + resolveRepoHandle: async (_name, groupPath) => + groupPath === 'app/repo-2' ? null : okHandle(_name, groupPath), + skipWrite: true, + }); + const pinnedIds = pinSpy.mock.calls.map((c) => c[0]); + // repo-2 has no handle (resolve returned null) → not in knownRepos → + // never windowed, never leased; repo-1 (resolved) is. + expect(pinnedIds).toContain('app-repo-1'); + expect(pinnedIds).not.toContain('app-repo-2'); + } finally { + restore(); + } + }); + + it('releases an already-acquired lease when a later init in the same window throws', async () => { + const poolAdapter = await import('../../../src/core/lbug/pool-adapter.js'); + const releaseSpies: Array> = []; + // Throw on the SECOND init of app-repo-2 — the first is the init-loop + // extraction; the second is the window re-init. This isolates the failure + // to window setup, after app-repo-1's lease was already acquired. + const initCounts = new Map(); + const initSpy = vi.spyOn(poolAdapter, 'initLbug').mockImplementation(async (id: string) => { + const n = (initCounts.get(id) ?? 0) + 1; + initCounts.set(id, n); + if (id === 'app-repo-2' && n === 2) throw new Error('window init boom'); + }); + const execSpy = vi.spyOn(poolAdapter, 'executeParameterized').mockResolvedValue([]); + const pinSpy = vi.spyOn(poolAdapter, 'pinRepo').mockImplementation(() => { + const release = vi.fn(); + releaseSpies.push(release); + return release; + }); + try { + await expect( + syncGroup(groupConfig(2, [httpLink('app/repo-1', 'app/repo-2')]), { + resolveRepoHandle: okHandle, + skipWrite: true, + }), + ).rejects.toThrow('window init boom'); + // Exactly one lease was acquired (app-repo-1) before app-repo-2's init + // threw, and the window finally released it — no leaked lease. + expect(releaseSpies.length).toBe(1); + expect(releaseSpies[0]).toHaveBeenCalled(); + } finally { + initSpy.mockRestore(); + execSpy.mockRestore(); + pinSpy.mockRestore(); + } + }); +}); + describe('stableRepoPoolId', () => { it('returns lowercase name when no collision', () => { const entry: RegistryEntry = { diff --git a/gitnexus/test/unit/lbug-pool-pinning.test.ts b/gitnexus/test/unit/lbug-pool-pinning.test.ts new file mode 100644 index 000000000..914647b09 --- /dev/null +++ b/gitnexus/test/unit/lbug-pool-pinning.test.ts @@ -0,0 +1,223 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { mkdtempSync, writeFileSync, rmSync } from 'node:fs'; + +// Drive the REAL initLbug path (which calls evictLRU) rather than +// initLbugWithDb (which bypasses eviction entirely). The native LadybugDB +// open/connect/FTS stack is mocked exactly as in lbug-pool-fts-load.test.ts, +// plus sidecar-recovery so openReadOnlyDatabase's preflight is a no-op. fs is +// NOT mocked — each repo uses a real temp file so the fs.stat existence check +// in doInitLbug succeeds naturally. +// +// Covers issue #2189: a group sync larger than MAX_POOL_SIZE must keep every +// repo resident through deferred manifest/workspace resolution. Pinning makes +// that resident set survive automatic (LRU + idle) eviction. + +const { loadFTSExtensionMock } = vi.hoisted(() => ({ + loadFTSExtensionMock: vi.fn(), +})); + +vi.mock('@ladybugdb/core', () => ({ + default: { + Database: vi.fn(), + Connection: vi.fn(function (this: any) { + // probeDatabaseForShadowReplay() runs a probe query during the + // read-only open; the result must expose getAll()/close(). + this.query = vi.fn().mockResolvedValue({ + getAll: vi.fn().mockResolvedValue([]), + close: vi.fn(), + }); + this.close = vi.fn().mockResolvedValue(undefined); + }), + }, +})); + +vi.mock('../../src/core/lbug/lbug-adapter.js', () => ({ + isReadOnlyDbError: vi.fn(() => false), + loadFTSExtension: loadFTSExtensionMock, +})); + +vi.mock('../../src/core/lbug/lbug-config.js', () => ({ + // A fresh fake Database per call so distinct dbPaths get distinct entries + // and closeOne's db.close() resolves per repo. + createLbugDatabase: vi.fn(() => ({ + init: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + })), + toNativeSafePath: vi.fn((p: string) => p), + isWalCorruptionError: vi.fn(() => false), + WAL_RECOVERY_SUGGESTION: '', +})); + +vi.mock('../../src/core/lbug/sidecar-recovery.js', () => ({ + preflightLbugSidecars: vi.fn().mockResolvedValue(undefined), + isMissingFsError: vi.fn(() => false), + isMissingShadowSidecarError: vi.fn(() => false), + isReadOnlyShadowReplayError: vi.fn(() => false), + quarantineWalForMissingShadow: vi.fn().mockResolvedValue(''), + renameFailureMessage: vi.fn((p: string) => `rename failed for ${p}`), + statIfExists: vi.fn().mockResolvedValue(null), +})); + +const { initLbug, closeLbug, isLbugReady, pinRepo, unpinRepo } = + await import('../../src/core/lbug/pool-adapter.js'); + +describe('pool-adapter repo pinning (issue #2189)', () => { + let tmpDir: string; + // Track every repoId touched so afterEach can fully reset module-global state. + const touched = new Set(); + + const dbPathFor = (repoId: string): string => { + const p = path.join(tmpDir, `${repoId}.lbug`); + writeFileSync(p, ''); // real file so fs.stat() in doInitLbug succeeds + return p; + }; + + const init = async (repoId: string): Promise => { + touched.add(repoId); + await initLbug(repoId, dbPathFor(repoId)); + }; + + beforeEach(() => { + tmpDir = mkdtempSync(path.join(os.tmpdir(), 'gn-pin-test-')); + loadFTSExtensionMock.mockResolvedValue(true); + }); + + afterEach(async () => { + vi.useRealTimers(); + await closeLbug().catch(() => {}); + for (const id of touched) unpinRepo(id); + touched.clear(); + loadFTSExtensionMock.mockReset(); + rmSync(tmpDir, { recursive: true, force: true }); + }); + + // MAX_POOL_SIZE is 5; the 6th init triggers evictLRU. + it('characterization: WITHOUT pinning, the earliest repo is LRU-evicted past the cap', async () => { + for (let i = 1; i <= 6; i++) await init(`repo-${i}`); + + // repo-1 had the oldest lastUsed and nothing was checked out, so it is the + // eviction victim — exactly the stale-executor scenario from #2189. + expect(isLbugReady('repo-1')).toBe(false); + // The most recently initialized repo survives. + expect(isLbugReady('repo-6')).toBe(true); + }); + + it('FIX: pinning each repo keeps all of them resident past the cap', async () => { + for (let i = 1; i <= 6; i++) { + await init(`repo-${i}`); + pinRepo(`repo-${i}`); + } + + for (let i = 1; i <= 6; i++) { + expect(isLbugReady(`repo-${i}`)).toBe(true); + } + }); + + it('explicit close beats the pin AND clears it (no cross-operation leak)', async () => { + pinRepo('repo-x'); + await init('repo-x'); + expect(isLbugReady('repo-x')).toBe(true); + + // Explicit teardown closes a pinned repo without needing an unpin first. + await closeLbug('repo-x'); + expect(isLbugReady('repo-x')).toBe(false); + + // The pin must have been cleared on close: re-init repo-x FIRST (oldest + // lastUsed) and fill past the cap. If the pin had leaked, repo-x would be + // un-evictable; instead it is evicted as the LRU victim. + await init('repo-x'); + for (let i = 1; i <= 5; i++) await init(`fresh-${i}`); + expect(isLbugReady('repo-x')).toBe(false); + }); + + it('idle-timeout sweep skips pinned repos but still evicts idle unpinned ones', async () => { + vi.useFakeTimers(); + + await init('pinned-idle'); + pinRepo('pinned-idle'); + await init('unpinned-idle'); + + expect(isLbugReady('pinned-idle')).toBe(true); + expect(isLbugReady('unpinned-idle')).toBe(true); + + // The idle timer runs every 60s and closes entries idle past + // IDLE_TIMEOUT_MS (5 min) with no checked-out connections. advance past + // both thresholds, flushing microtasks between fires. + await vi.advanceTimersByTimeAsync(5 * 60 * 1000 + 60 * 1000); + + expect(isLbugReady('pinned-idle')).toBe(true); + expect(isLbugReady('unpinned-idle')).toBe(false); + }); + + it('unpinRepo re-enables eviction for that repo', async () => { + // Pin five repos and fill the pool; a sixth init evicts nothing (all pinned). + for (let i = 1; i <= 5; i++) { + await init(`p-${i}`); + pinRepo(`p-${i}`); + } + await init('p-6'); // unpinned; pool now holds 6 (soft-cap exceeded) + for (let i = 1; i <= 6; i++) expect(isLbugReady(`p-${i}`)).toBe(true); + + // Unpin the oldest, then init a 7th repo — the now-unpinned p-1 is the LRU + // victim. + unpinRepo('p-1'); + await init('p-7'); + expect(isLbugReady('p-1')).toBe(false); + expect(isLbugReady('p-7')).toBe(true); + }); + + it('reference-counts leases: two pins need two unpins before eviction (Finding 1)', async () => { + // Fill the pool to capacity, all leased. + for (let i = 1; i <= 4; i++) { + await init(`rc-${i}`); + pinRepo(`rc-${i}`); + } + await init('rc-shared'); + pinRepo('rc-shared'); // lease 1 + pinRepo('rc-shared'); // lease 2 (two holders) + + // Release ONE lease — a holder remains, so rc-shared stays exempt even + // under eviction pressure. + unpinRepo('rc-shared'); + await init('rc-extra'); // evictLRU finds no unpinned victim → pool grows + expect(isLbugReady('rc-shared')).toBe(true); + + // Release the LAST lease — now rc-shared (oldest unpinned) is evictable. + unpinRepo('rc-shared'); + await init('rc-extra2'); + expect(isLbugReady('rc-shared')).toBe(false); + }); + + it('unpinRepo floors at zero and tolerates unknown repoIds', () => { + expect(() => { + unpinRepo('never-touched'); // unknown repoId → no-op + pinRepo('floor-x'); + unpinRepo('floor-x'); // count 0 → key deleted + unpinRepo('floor-x'); // already gone → no-op, never a negative count + }).not.toThrow(); + }); + + it('pinRepo returns a disposer that releases exactly once and composes with refcount', async () => { + for (let i = 1; i <= 4; i++) { + await init(`d-${i}`); + pinRepo(`d-${i}`); + } + await init('d-shared'); + const release1 = pinRepo('d-shared'); // lease 1 + const release2 = pinRepo('d-shared'); // lease 2 + + release1(); + release1(); // double-call is a no-op — must NOT decrement lease 2 + + // lease 2 still held → d-shared survives eviction pressure. + await init('d-extra'); + expect(isLbugReady('d-shared')).toBe(true); + + // Release the last lease via its own disposer → now evictable. + release2(); + await init('d-extra2'); + expect(isLbugReady('d-shared')).toBe(false); + }); +});