מעקב אחרי סקירת PR #3429 (מאגר הקריאות של ה-MCP): שמונה הצעות שלא תוקנו ב-PR, וחסימת קריאת המראה במקור #3433

הקשר

סקירת קוד של PR #3429 (מאגר הקריאות של ה-MCP נגזר ממכסת הזיכרון, הממצא השלישי ב-#3391) בסקיל code-review של Han, עם שמונה סוכנים ומאמת יריב. הדוח המלא: code-review-3429-read-pool.md ב-CodeKeeper. כל ממצא שם אומת בהרצה לפני שנכנס לדוח; מספרי השורות למטה הם לפי הקומיט be33ca9 של ה-PR.

מה תוקן ב-PR עצמו (קומיט ההמשך שלו): WARN-001 עד WARN-005, SUGG-002, SUGG-003, SUGG-009, SUGG-011, SUGG-012, SUGG-013, ושלושת פריטי ה-YAGNI (התקרה ירדה מ-32 ל-12, הרוחב שהייצור כבר הריץ; הטסט הכפול של שורת הקיבולת קופל; הסקריפט מודד רק את המסמך הצפוף ביותר).

מה נשאר כאן, בסדר עדיפות שלי:

1. SUGG-006 — למאגר הקריאות אין שום סימן רוויה

mcp_server/server.py, _run_on_shared_pool (שורות 1056–1062): זו כל שליחת הקריאה — בלי חותמת זמן בשליחה, בלי מדידת המתנה בתור, בלי מונה עומק. מסלול הכתיבה ליד כבר עושה בדיוק את זה (_log_write_timing, _SLOW_WRITE_QUEUE_WAIT). כשהמאגר מתמלא — קריאת git grep תקועה, git show שממתין 30 שניות, פרץ קריאות של סוכן — כל קריאה נוספת ממתינה בתור בלי גבול, הלקוח מקבל timeout, /healthz נשאר ירוק, ושורת הקיבולת עדיין מדווחת את הרוחב שדיווחה לפני שעה. מאגר של 2 חוטים (ה-fallback) בלי שום סימן רוויה הוא בדיוק המצב שבו לא נדע.

טריגר לפתיחה: תלונה על זמני תגובה בקריאות שאין לה הסבר בלוגים. עד אז זה נדחה בכוונה, כי אין עדות לרוויה ו-YAGNI מזהיר מפני ניטור לכשל שמעולם לא קרה — אבל הדחייה רשומה כאן ולא שקטה.

הצורה המוצעת: לשכפל את מה שמסלול הכתיבה כבר עושה — time.perf_counter() לפני asyncio.to_thread ובתוך הגוף שנשלח, ו-WARNING כשהפער עובר סף, באותה הנמקה של _SLOW_WRITE_QUEUE_WAIT. הדרך הסלולה: _dispatch(fn, pool) אחד ששני היעדים עוברים דרכו, עם המדידה בפנים.

2. SUGG-005 — הרצפה נרשמת ב-INFO, ה-fallback ב-WARNING; ו"ללא מגבלה" מקבל את אותו גודל כמו "לא ניתן לקריאה"

mcp_server/server.py, attach_read_pool (שורות 807–816) מול _read_pool_size (700–703) ו-_memory_limit (562–577). כש-source == "floor" החשבון עצמו אומר שהתקציב לא מכסה שני חוטים, וזה נרשם רק בשורת ה-INFO של הקיבולת; כש-source == "fallback" (קובץ לא קריא) יש WARNING. ומכונה בלי מגבלה בכלל (מחשב פיתוח, שורש cgroup) מקבלת את המאגר הצר ביותר, 2, עם WARNING בכל עלייה. הצעד הקטן: if sizing.source in ("fallback", "floor") עם הטקסט מ-sizing.detail; הצעד הבא: להחזיר "unlimited" ו-"unreadable" כמצבים נפרדים מ-_memory_limit ולהחליט מה מגיע ל"ללא מגבלה".

3. SUGG-007 — שתי עטיפות ה-lifespan לעולם לא נבדקות יחד עם PostHog מוגדר

build_app קורא ל-attach_shutdown_drain ואז ל-attach_read_pool (שורות 2052–2066), וההערה מסבירה למה הסדר חשוב. הטסט היחיד שבונה את האפליקציה האמיתית רץ בלי POSTHOG_PROJECT_TOKEN/POSTHOG_HOST, ולכן _build_client() מחזיר None ו-attach_shutdown_drain יוצא לפני שהוא עוטף. טסט שמציב סטאב ב-mcp_server.analytics._CLIENT לפני build_app, נכנס ויוצא מה-lifespan, ומוודא שהמאגר עדיין ה-executor ברירת המחדל בזמן ש-_drain() רץ — יתפוס היפוך סדר עתידי.

4. SUGG-014 — אין טסט לקובץ memory.max שקיים אבל לא ניתן לפענוח

_memory_limit (שורות 537–577): הנפילה ל-v1 מוכחת רק לקובץ v2 חסר (OSError) או לא קריא. קובץ v2 ריק (IndexError) או לא-מספרי (ValueError) אמור ליפול ל-v1 ולהחזיר את הערך שלו, ואף טסט לא אומר זאת. כיסוי השורות מלא; התרחיש חסר. _fake_cgroup הקיים מספיק לזה.

5. SUGG-008 — לוגיקת התקציב יושבת בקובץ הגדול והתזזיתי ביותר בחבילה

כ-300 שורות של קריאת cgroup, חשבון תקציב, טיפוס תוצאה ומתקין lifespan נכנסו ל-mcp_server/server.py (2,085 שורות, 16 קומיטים ב-90 יום). mcp_server/analytics.py הוא התקדים לשמירת עניין שמותקן ב-lifespan במודול משלו עם נקודת כניסה אחת. העברה ל-mcp_server/read_pool.py היא ריפקטור נמוך-סיכון.

6. SUGG-004 — שורת הקיבולת קוראת מאפיין פרטי של CPython במסלול העלייה

read_pool._max_workers (שורות 740 ו-814): אם המאפיין ייעלם, השירות לא יעלה במקום להפסיד שורת לוג, בניגוד להבטחה בדוקסטרינג. כנראה לעולם לא יקרה (קיים מאז 3.2, נעוץ ב-3.11, הטסטים מקבעים). getattr(read_pool, "_max_workers", sizing.workers) שומר את קריאת המצב ומתדרדר לרוחב המבוקש; או להצהיר בדוקסטרינג שכשל עלייה כאן הוא מכוון.

7. SUGG-010 — הסקריפט קורא לפונקציה פרטית של md_parser

scripts/measure_md_parse_cost.py קורא ל-md_parser._build_parser() כי אין פונקציה ציבורית שמחזירה ספירת טוקנים גולמית, והפונקציה מודרת בכוונה מ-__all__. accessor ציבורי קטן (או ייצוא מסומן "למדידה בלבד") יהפוך את התלות לגלויה.

8. SUGG-001 — שורת assert שלא יכולה ליפול אחרי השורה שלפניה

tests/test_mcp_to_thread.py:1039: assert sizing.workers != min(32, cpu_count + 4) or sizing.workers == 2 — השורה שלפניה כבר מקבעת workers == 2, ולכן ה-or תמיד אמת. למחוק, או להחליף בבדיקה שיכולה ליפול (למשל os.cpu_count מזויף ל-16 וה-fallback נשאר 2).

9. השורש של WARN-003 — לחסום את קריאת המראה במקור

services/git_mirror_service.py, get_file_at_commit (שורות 1608–1638): git show נטען כולו לזיכרון (subprocess.run(..., capture_output=True)) ורק אז נבדק max_size. לכן max_size אינו תקרה על הזיכרון אלא בדיקה בדיעבד — קובץ של 11.7MB (md_preview.bundle.js.map בריפו הזה) נטען כולו לפני שנדחה, וקריאת lines= של md_preview.bundle.js (7MB) מחזיקה כ-38.5MiB לחוט, מעל 35.2MiB שהנוסחה מקצה. ב-PR הוחלט ותועד שהמחלק נשאר מתומחר לפי הפרסור (כלי הטווח הם אדמין בלבד, וההחזקה הגדולה ביותר על קבצים אמיתיים היום נכנסת בתוכנית: 92 + 10 × 38.5 = 477MiB מתוך 512MiB), והתקרה ירדה ל-12. הדרך הסלולה: בדיקת גודל (git cat-file -s <sha>:<path>) לפני git show, או קריאה בזרם עם תקרת בתים — ואז "קריאה אחת עולה לכל היותר X" הופך לתכונה של הקוד ולא להנחה בהערה, וכל נוסחת גודל עתידית יורשת אותה.