PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

• Decision: Approve
• Rationale: Restores the ~100ms low-latency disconnect summarization that #12139 regressed to ~10min, without reintroducing the per-instance duplicate-drain problem #12139 removed — exactly the #12199 design (cheap idempotent marker from any instance; orchestrator as the sole lease-backed drainer). All five #12199 ACs are met. Approve.

Peer-Review Opening: This lands the design cleanly and the source-archaeology delta is a genuinely good catch — restoring the old writer's completed/in_progress skip so a late disconnect can't reopen a finalized session or steal an active lease. I verified all five ACs (including the AC4 concurrency guarantee) and ran the suite locally; notes below are non-blocking.

🕸️ Context & Graph Linking

• Target Issue: Resolves #12199 (leaf sub — enhancement/ai/architecture/performance, not epic)
• Related Graph Nodes: Epic #12065 (orchestrator-as-SSOT), #12139 / PR #12197 (the regression this corrects), #10693 (the lease concurrency test backing AC4)

🔬 Depth Floor

Challenge / verification (AC4 — the one that needed proof): #12199's AC4 asked for a falsifying concurrency test that two overlapping drains (pending + drift) never double-summarize one session. This PR doesn't add a new test that literally composes that race — but it doesn't need to: both drains funnel through summarizeSessions({sessionId}) → claimSummarizationJob(), and that lease is already proven single-winner by SessionService.spec.mjs:11 ("exclusive lease prevents concurrent claim of same session", #10693), which forks N real worker processes racing the same session and asserts successCount === 1 / failCount === N-1. So the no-double-summarize guarantee is inherited from a rigorously-tested mechanism, not asserted by hand. AC4 is substantively satisfied via reuse — I'd call a literal pending+drift overlap test a nice-to-have, not a gap.

Secondary (non-blocking):

• Two near-identical pending readers: getPendingSummarizationJobs (in scheduling/summary.mjs, against the orchestrator's bridge db handle) and getPendingSummarizationJobIds (in SessionService.mjs, the drain payload via the SDK). The duplication is across a process boundary (the orchestrator scheduler shouldn't import the memory-core SDK), so it's defensible — but a one-line JSDoc cross-reference between them would save a future reader the "why are there two?" moment.
• limit: 50 per child run: if >50 sessions disconnect within one 3000ms poll window, the overflow waits for the next poll (or the drift sweep). Reasonable for the poll cadence; noting the bound exists.

Rhetorical-Drift Audit: Pass. PR body ("writes only an idempotent SummarizationJobs.pending marker"; "the orchestrator summary scheduler sees pending markers ahead of the 10-minute drift sweep") matches the diff exactly. The ON CONFLICT ... WHERE status != 'completed' AND status != 'in_progress' guard is precisely what the prose claims.

🧠 Graph Ingestion Notes

• [RETROSPECTIVE]: The fix reactivates the orphaned SummarizationJobs.'pending' lane (writer = onSessionClosed, reader = orchestrator drain, lease = claimSummarizationJob) end-to-end — turning dead-code-since-#12139 back into a live single-drainer path. The "cheap idempotent marker from any instance, single lease-backed drainer" split is the correct shape for multi-instance MCP safety.
• [KB_GAP]: none.
• [TOOLING_GAP]: none.

N/A Audits — 📑 📡 🛂

N/A across listed dimensions: no Contract Ledger surface change beyond internal service methods; no openapi.yaml; no novel external abstraction (revives an existing lane + reuses the existing lease).

🎯 Close-Target Audit

• Resolves #12199 — leaf sub, syntax-exact on its own line.
• Whole-body prose scan (grep -iE "(close|fix|resolve)[sd]? +#?N"): only the intended Resolves #12199.
• closingIssuesReferences = [12199] — no epic auto-close.
• Commit 84cd6361b clean.
• Findings: Pass.

🧪 Test-Execution & Location Audit

• Checked out PR head (detached); ran all 3 specs → 23 passed (1.9s).
• New coverage is real, with real SQLite for the idempotency invariants: pending-write, no-steal-in_progress-lease, no-reopen-completed, plus the trigger-priority (handover > pending > sweep), onSessionClosed-writes-marker, and summarizePendingSessions-drains-via-summarizeSessions paths.
• AC4 backed by the pre-existing #10693 fork-based lease concurrency test (verified its successCount===1 assertion).
• Locations canonical (unit/ai/daemons/orchestrator/scheduling/, unit/ai/mcp/server/memory-core/, unit/ai/services/memory-core/).
• CI: green (unit, integration-unified, CodeQL, Analyze, check, lint-pr-body).
• Findings: Tests pass.

📋 Required Actions

No required actions — eligible for human merge.

(Optional, non-blocking: one-line JSDoc cross-reference between the two pending-readers noting the process-boundary rationale.)

📊 Evaluation Metrics

• [ARCH_ALIGNMENT]: 98 — revives the orphaned pending lane with orchestrator-as-sole-drainer + reuses the proven lease; idempotent marker with the correct finalized-session guard. 2 deducted: the two near-identical pending readers (defensible across the process boundary, but uncross-referenced).
• [CONTENT_COMPLETENESS]: 97 — @summary JSDoc on every new method explaining the design intent; Fat-Ticket body with source-archaeology rationale. 3 deducted: the two pending-readers don't reference each other.
• [EXECUTION_QUALITY]: 97 — 23/23 pass (ran locally); idempotency invariants tested against real SQLite; AC4 met via proven-lease reuse. 3 deducted: AC4's literal overlap-composition not separately tested (met by reuse); minor duplication.
• [PRODUCTIVITY]: 100 — all five #12199 ACs met: idempotent non-inline marker ✓, orchestrator drains pending lease-claimed on a short tick distinct from the sweep ✓, seconds-not-sweep latency ✓ (3000ms poll vs 600000ms drift), no-double-summarize via lease ✓ (#10693), no dead pending path — lane live writer→reader→lease ✓.
• [IMPACT]: 60 — restores low-latency disconnect summarization (≈100ms vs ≈10min regression) without the #12139 duplicate-drain; reinforces the #12065 orchestrator-SSOT.
• [COMPLEXITY]: 50 — moderate: spans 4 source files across the orchestrator/memory-core boundary + 3 test files; cross-process pending-lane coordination.
• [EFFORT_PROFILE]: Heavy Lift — careful cross-boundary lane revival with idempotency guards and concurrency-safety-by-reuse.

Clean restoration of the good idea #12139 dropped. Approving — over to @tobiu for merge.

— Reviewed by Opus 4.8 (Claude Code), session af888fcb-6a68-45bd-8a34-13be477733b1.