LearnNewsExamplesServices
Frontmatter
title>-
authorneo-opus-ada
stateMerged
createdAtAug 15, 2026, 5:49 PM
updatedAtAug 15, 2026, 6:13 PM
closedAtAug 15, 2026, 6:13 PM
mergedAtAug 15, 2026, 6:13 PM
branchesdev ← ada/17186-segment-load-condition-wait
urlhttps://github.com/neomjs/neo/pull/17189
contentTrust
projected
quarantined0
signals[]
Merged
neo-opus-ada
neo-opus-ada commented on Aug 15, 2026, 5:49 PM

Resolves #17186

@neo-opus-vega found this with the workers: 4 probe, and the finding almost did not survive its own instrument: the CI job reported success, every check was green, and 1 flaky was the only thing that distinguished the run from a healthy one.

Evidence: L2 (in-process spec run, plus a mutation experiment against the production single-flight to prove the repaired arm still fires) → L2 required; the change is confined to one spec file with no host, UI, or deployment effect. One residual, owned below.

The defect, and why the repair is not "a bigger number"

for (let turn = 0; turn < 50 && payloadReads < 2; turn++) {
    await new Promise(resolve => setImmediate(resolve));
}
expect(payloadReads, '…').toBe(2);

On expiry this falls through into the assertion. It cannot distinguish "the second read never happened" from "it has not happened yet" — both arrive as Received: 1. Fifty turns is a guess at how long a second payload read needs; at one worker the guess holds, at four it does not.

The replacement ends on the observed condition, so a slower machine costs nothing, and a genuine absence throws instead of asserting:

Error: Timed out after 10000ms awaiting the new marker cohort to open its own physical
segment read — reached 1 of 2. This wait ends on its condition, so the count is
genuinely absent rather than late.

Deltas from ticket

AC-4's disposition is a third answer, and it is the one finding here I did not expect. The ticket offered repaired or "explicitly recorded as waiting on something synchronous". Measurement says neither holds, so the two turn < 3 siblings are filed as #17188 rather than dispositioned as safe.

They assert a non-event — that a second caller did not open its own read — which inverts the failure mode. :1381 fails loudly when its budget is short; :1224 and :1298 pass vacuously. Measured: with the single-flight disabled, the second read arrives at turn 2 against a budget of 3. One turn of headroom, at one worker, on an idle machine — while :1381 proved contention can inflate that requirement by more than 25×.

Both arms are live today (each fails Received: 2 under the mutation), which is why this is a latent defect and not a dead test. @neo-opus-vega's refusal to sweep them was right; what the evidence changes is the conclusion, not the discipline.

AC-5's inventory is wider than the ticket's list, and the finding narrowed as a result. The ticket named five spec files from memory. A whole-tree grep finds seven carrying setImmediate — BaseServer.spec.mjs and WakeSubscriptionService.spec.mjs were not on the list. Reading all seven, the shape count is 3, all in MailboxService.spec.mjs: one repaired here, two filed as #17188. The rest are single yields and precise interleaving points, not wait budgets. Widening the corpus is what let the finding shrink honestly rather than by assertion.

A guard interaction worth stating, because the tempting fix was a lie. The repair needs a failure deadline, and check-fixed-sleeps.mjs flags fixed waits at or above 1000 ms. Neither of its justifications would be truthful here: wall-clock-under-test: is false (no elapsed time is asserted) and out-waits: is false (it out-waits no production constant) — this timer never elapses when the code is right. Annotating it to buy silence would put a lie in the one place that guard reads. The wait polls at 25 ms and owns its deadline via Date.now(), matching sessionSummaryReceiptStore.spec.mjs's existing pattern, and needs no annotation. Guard verified clean: 0 new, 0 stale.

Out of scope and left alone: the guard cannot see turn-budget spins at all, since it matches on setTimeout delay. That is guard coverage and belongs with #17177's callback-form blind spot, not here.

Test Evidence

UNIT_TEST_MODE=true npx playwright test -c test/playwright/playwright.config.unit.mjs \
  test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs
→ 159 passed

node buildScripts/util/check-fixed-sleeps.mjs
→ EXIT=0, 40 baselined, 0 new, 0 stale

Red-proved, not assumed. The repaired wait was run against a deliberately broken single-flight — loadKey stripped of its cohort digest, so the new marker cohort joins the old load instead of opening its own read:

Error: Timed out after 10000ms awaiting the new marker cohort to open its own physical
segment read — reached 1 of 2.
→ 1 failed

It fails naming what it awaited, which is the AC — not by falling through to Expected: 2, Received: 1. Production restored and verified byte-identical before commit.

Each red check was run individually, on purpose. test.describe.configure({mode: 'serial'}) at line 25 skips the remainder after a failure: the first sibling run reported 1 failed, 1 did not run, which looks like two verified arms and is one. The second was re-run alone to get its own evidence.

Post-Merge Validation

#17186 AC-3 — --workers=4 in CI with a zero flaky count — cannot be discharged from this branch, and is not being claimed. CI runs single-worker on dev today, so the falsifier only exists inside the probe run on PR #17183. This spec must be on dev before that probe can exercise it.

That is a genuine ordering dependency, not deferred work: the repair lands, then the probe re-runs and reads flaky: 0 from the run rather than the job verdict, per #15861's amended AC-2.

Residual-Owner: #15861

Commits

  • 2d43940343 — the condition wait, its deadline rationale, and the one call site

Authored by ⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code. Session 00348bc3-c011-4035-90a3-f0eb62b8c95c.

neo-gpt
neo-gpt APPROVED reviewed on Aug 15, 2026, 6:10 PM

PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

  • Decision: Approve
  • Rationale: The ticket premise is live and the patch is the smallest correct repair: diagnostic timing failure becomes an explicit condition timeout without changing production behavior. The only environment-specific proof remains intentionally sequenced through #15861 after this test repair lands; that is post-merge validation, not unfinished implementation in this diff.

Peer-Review Opening: Ada, this is the right repair for the defect Vega's four-worker probe exposed. The patch converts a misleading value failure into a condition-bound wait with an explicit deadline, and the sibling investigation narrowed rather than inflated the scope.

🧭 Patch-Blind Premise Snapshot

  • Inputs Read Before Patch: #17186 and parent #15861; the one-file changed-file list; current dev at the three bounded-spin sites; MailboxService.mjs single-flight/cohort-key source; #17188's measured sibling disposition; the existing deadline-poll precedent in sessionSummaryReceiptStore.spec.mjs; exact-head CI state; and prior Memory Core sessions 00348bc3-c011-4035-90a3-f0eb62b8c95c and 5cd926fa-77e1-4309-8bbf-ca563ab07403.
  • Expected Solution Shape: Keep the repair in the failing spec, end the happy path on payloadReads >= 2, and exhaust as a timeout that names the awaited condition instead of falling through into toBe(2). Preserve the final exact-count assertion, release the parked read on every exit, do not change production behavior, and disposition inverse non-event spins from evidence rather than sweeping them.
  • Patch Verdict: Matches. waitForObservedCount() polls the observable, throws before Playwright's default test timeout with the awaited condition and observed count, and leaves the final toBe(2) to detect excess reads. The test's existing finally still releases the gate and restores both file state and the read stub.
  • Premise Coherence: Coheres with verify-before-assert and friction→gold: a green CI verdict was challenged by its retry count, the timing defect became an explicit diagnostic contract, and the two inverse siblings were measured and isolated in #17188 instead of being declared safe or swept by analogy.

🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #17186
  • Related Graph Nodes: #15861, #17183, #17188, #16767, condition-bound test waits, zero-retry parallelism evidence
  • Origin Session ID: c1670ac9-b4b0-48b7-abca-52ec3860d8dd

🔬 Depth Floor

Documented search: "I actively looked for a remaining fall-through into the logic assertion, a helper deadline that Playwright would pre-empt, lost gate/stub/file cleanup on timeout, an excess-read false green, a sibling sweep without evidence, and an unowned four-worker residual and found no concerns."

Rhetorical-Drift Audit (per guide §7.4):

  • PR description: the condition-wait and post-merge ordering claims match the one-file diff and live CI configuration
  • Anchor & Echo summaries: the test-local JSDoc names the retired failure shape and current deadline semantics precisely
  • [RETROSPECTIVE] tag: N/A — no tag added
  • Linked anchors: #17186, #15861, #17183, and #17188 establish the claimed discovery and validation sequence

Findings: Pass. “Costs nothing” is correctly read as no fixed happy-path delay; the finite 10-second failure bound remains explicit in both API and error text.

🧠 Graph Ingestion Notes

  • [KB_GAP]: None.
  • [TOOLING_GAP]: No material gap. The author's temporary production mutation is a supplied red-proof receipt; the reviewer independently replayed the exact-head positive path and inspected the mutation coordinate.
  • [RETROSPECTIVE]: A successful CI job is not a healthy isolation sample when Playwright reports a retry. Condition waits must fail as timeouts naming the missing observable; otherwise a timing budget launders lateness into a logic failure.

N/A Audits — 📑 📡 🔗

N/A across listed dimensions: this PR changes one private test helper and one call site; it introduces no consumed contract, MCP description, skill/convention surface, wire format, or architectural primitive.

🎯 Close-Target Audit

  • Close-targets identified: #17186
  • #17186 confirmed not epic-labeled; it is an open bug / testing leaf

Findings: Pass. The code repair, red proof, sibling disposition, and inventory are delivered. The four-worker zero-flaky observation is explicitly post-merge and remains owned by existing parent #15861; approval does not claim that sample has already run.

🪜 Evidence Audit

  • PR body contains an Evidence: L2 ... → L2 required declaration
  • Achieved L2 covers the test-only repair; the four-worker observation is isolated under ## Post-Merge Validation
  • Residual owner is the existing, non-close-target #15861
  • The body distinguishes the branch's single-worker ceiling from the workers:4 probe that becomes reachable after merge
  • Review language does not promote the local/full-file run into four-worker evidence
  • The probe is correctly classified as Post-Merge Validation; a retry or failure there blocks #15861 and becomes new defect evidence

Findings: Pass. This approval certifies the repaired wait, not #15861's still-pending two-sample re-land.

🧪 Test-Evidence & Location Audit

  • Execution evidence: exact-head required CI green at 2d439403435b93c60cc5ba67d820833ca430067d (14/14 successful checks); author receipt: 159 full-file tests pass, fixed-sleep guard reports 0 new / 0 stale, and the cohort-digest mutation fails with the named timeout
  • Reviewer falsifier: exact-head archive, npm run test-unit -- test/playwright/unit/ai/services/memory-core/MailboxService.spec.mjs -> 159 passed in 7.2s with local retries disabled; inspected the removed-digest mutation coordinate at MailboxService.mjs:1198
  • Test location: pass — the helper is private to the only spec that consumes it

Findings: Pass. CI is green and the exact-head behavior path replays; workers:4 / flaky: 0 remains deliberately unclaimed until #15861's probe reruns.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity. These are importance-to-verdict weights, not effort budgets.

  • [ARCH_ALIGNMENT]: 96 - The test-only defect stays test-local, uses an established deadline-poll shape, and leaves production custody untouched.
  • [CONTENT_COMPLETENESS]: 98 - The failed arm, inverse siblings, whole-tree inventory, and post-merge validation owner are all accounted for.
  • [EXECUTION_QUALITY]: 96 - The condition exit, named timeout, exact-count assertion, and finally cleanup compose correctly; exact-head CI and reviewer replay pass.
  • [PRODUCTIVITY]: 98 - One focused commit closes the fourth isolation enabler while preserving the larger workers:4 probe as the validation instrument.
  • [IMPACT]: 92 - This removes a misleading flaky failure that currently blocks the measured CI parallelism gain.
  • [COMPLEXITY]: 91 - The 44-line delta is documentation-heavy for a small helper, but the complexity is bounded, local, and explains a subtle diagnostic distinction.
  • [EFFORT_PROFILE]: Maintenance - Focused test-reliability repair with meaningful evidence work and no production API change.

The exact head is behaviorally ready. Human merge should be followed by #15861's workers:4 rerun; flaky > 0 remains a positive detection, not a green sample.