---
number: 12273
title: 'feat(orchestrator): restore pending summary markers (#12199)'
author: neo-gpt
state: MERGED
createdAt: '2026-05-31T20:16:35Z'
updatedAt: '2026-05-31T20:56:57Z'
closedAt: '2026-05-31T20:56:54Z'
mergedAt: '2026-05-31T20:56:54Z'
head: codex/12199-pending-summary-markers
base: dev
url: 'https://github.com/neomjs/neo/pull/12273'
---
Resolves #12199

Authored by GPT-5 (Codex Desktop). Session d9b4a887-57aa-43aa-8cec-a260ec35ce12.

FAIR-band: in-band [15/30 — current author count over last 30 merged]

Restores the low-latency disconnect marker path without resurrecting per-MCP-instance summarization drains. `Server.onSessionClosed()` now writes only an idempotent `SummarizationJobs.pending` marker; the orchestrator summary scheduler sees pending markers ahead of the 10-minute drift sweep; the summary child drains those markers through the existing `claimSummarizationJob()` lease path.

Evidence: L2 (source + focused unit proof: default orchestrator poll is 3000ms, pending markers bypass the 600000ms drift sweep, and pending/drift drains share the existing lease transition) -> L2 required (scheduler/lease contract; no browser-only or operator-only surface). No residuals.

## Deltas from ticket

- Preserved the pre-#12139 invariant that a late disconnect must not reopen a `completed` summarization job. The ticket asked for an idempotent marker writer; source archaeology showed the old writer intentionally skipped `completed` and `in_progress` rows, so the restored writer keeps that finalized-session boundary.
- The summary script drains pending markers first, then keeps the existing drift-detection sweep as a fallback. This keeps coverage intact when no pending marker exists.

## Test Evidence

- `node --check ai/services/memory-core/SessionService.mjs` -> passed.
- `node --check test/playwright/unit/ai/services/memory-core/SessionService.ResumeValidation.spec.mjs` -> passed.
- `git diff --check` -> passed.
- `npm run test-unit -- test/playwright/unit/ai/daemons/orchestrator/scheduling/summary.spec.mjs test/playwright/unit/ai/mcp/server/memory-core/Server.spec.mjs test/playwright/unit/ai/services/memory-core/SessionService.ResumeValidation.spec.mjs` -> 23 passed (1.2s after rebase onto `32806e65c`).

## Post-Merge Validation

- [ ] Observe the next live orchestrator disconnect cycle record a `pending-summarization:<n>` summary trigger before the periodic drift sweep would be due.

## Commit

- `84cd6361b` — `feat(orchestrator): restore pending summary markers (#12199)`

## Related

- Parent epic #12065
- Source-regression anchor #12139 / PR #12197


## Reviews

### `@neo-opus-ada` (APPROVED) reviewed on 2026-05-31T20:55:46Z

# 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.

---

