Resolves neomjs/neo#17758
Refs neomjs/neo-agent-brain#46
TextEmbeddingService.retry.spec.mjs:1248 — "the queued repository stays protected even when the circuit-open is DEFERRED" — failed ~37% of the time in complete isolation (6 of 16 isolated runs, same assertion every time: requestCount expected 1, received 2). Not pollution, not order-dependent — the spec alone, nothing else in the run.
Evidence: L2 (25 consecutive isolated runs plus a mutation control on the surviving assertion) → L2 required (the close-target ACs are all run-observable). No residuals.
AC Evidence
| Acceptance criterion |
Evidence |
| AC-1 |
25/25 consecutive isolated runs clean with --retries=0, against 6/16 failing before. The rate is ~37%, so a single green would have been meaningless — the run count is the evidence. |
| AC-2 |
The surviving assertions are fixture-controlled: the deferred hook fires, and the queued repository fails rather than silently succeeding. Neither depends on which macrotask wins. The unsequenced requestCount pin is gone, and the fixed 50 ms sleep with it. |
| AC-3 |
The replacement comment records that daemons/orchestrator/services/TenantRepoSyncService.spec.mjs owns the deferred ordering proof, so the coverage is not read as lost. |
| AC-4 |
memory-core/ serial: 1883 passed. Full serial unit run receipt added as a comment below when it completes (~15 min). |
The mechanism
Deferring the abort to a macrotask (setTimeout(() => controller.abort(circuitError), 0)) puts two macrotasks in an unordered race after A's 400 ms timeout rejects: the drain selecting and dispatching B, and the queued abort removing it. Nothing the fixture can await sequences those two — the await new Promise(resolve => setTimeout(resolve, 50)) it used runs before A's timeout even fires, so it only shifted the odds.
The arm's own scope note already said this:
"this fixture CANNOT exercise the ordering guarantee. That is a property of the fixture, not of the lane: the tenant-sync production composition in TenantRepoSyncService.spec.mjs DOES exercise it."
expect(requestCount).toBe(1) therefore asserted precisely the property the paragraph directly above it disclaims.
Why no coverage is lost
- The non-deferred sibling immediately above still pins
requestCount to 1 — that path is sequenced, and it is stable across every run I made.
- The deferred ordering guarantee is exercised in production composition at
TenantRepoSyncService.spec.mjs:3288-3372 (run-scoped circuit sweep, KB_VECTOR_EMBED_PROVIDER_CIRCUIT_OPEN). Verified, not taken from the comment.
Deltas from ticket
The ticket left the fix shape open between "sequence it deterministically" and "narrow to the fixture's documented scope". Option 1 is not available: both racing steps are macrotasks scheduled by the code under test, so no external await orders them. Option 2 taken.
Test Evidence
- Before: 6 failures in 16 isolated runs (10-run batch → 5 failures; 6-run batch → 1), identical assertion and values each time.
- After: 25/25 isolated runs clean.
- Mutation control: inverting the surviving assertion to
toBeFalsy() turns it red (1 failed), so it is not vacuous — it still catches a queued repository that silently succeeds, which is the defect the sibling arm names.
memory-core/ directory, serial, --retries=0: 1883 passed.
Post-Merge Validation
None — the close-target effects are all observable in the unit suite.
Why this was urgent
PR neomjs/neo#17750 merged at 09:49Z and set failOnFlakyTests: isCI. CI runs workers: 4, retries: 2, so a ~37% per-attempt flake fails once and passes on retry — exactly the outcome that gate converts into a red run. The flake was not new; its consequence was. This is the gate working correctly on its first real subject, not a defect in neomjs/neo#17750.
Evolution
I first recorded this arm on neomjs/neo-agent-brain#46 as a fresh order-dependent pollution witness, on the strength of "passes alone, 54/54". That was two samples of a coin flip — at a 37% rate, two consecutive passes happen roughly 40% of the time by luck. Retracted there; neomjs/neo-agent-brain#46 is back to no live witness. The lesson that produced this PR is that an isolation control needs a repetition count chosen against the suspected rate, not a single clean run.
Authored by Ada (@neo-opus-ada, Claude Opus 5, Claude Code). Session be6b6eb4-dabe-4deb-9924-7c92335c69ff.
AC-4 receipt — not met, and I am not rounding it up
Promised in the body: the full serial receipt. Here it is, unflattering half first.
--workers=1 --retries=0, whole unit suite, branch head 7670b73e69
15170 passed · 11 skipped · 1 failed (13.9m)
AC-4 says "reports zero failures." It reported one, so the AC is unmet.
What the failure is — and is not
The failure is not this PR's arm. TextEmbeddingService.retry.spec.mjs:1248 passed, and the string flaky appears zero times in the run. The repair holds under the fullest ordering pressure available.
The one failure is knowledgeBaseArtifact.spec.mjs:775 — "pack streams: peak RSS stays bounded well below the JSONL it rewrites."
What I have established so far, stated as measurements rather than conclusions:
- It passed on the pre-change baseline serial run (current
dev, 2bc6907ed4), where the only failure was the arm this PR fixes.
- It passes 6/6 in isolation on this branch.
- Its assertion reads
process.memoryUsage().rss — a process-level measurement, so it is load- and environment-sensitive by construction, and this machine has been running heavy suites back to back all session.
Why I am not calling it "unrelated"
Because I cannot yet, and "pre-existing / environmental / flaky" is exactly the label a red gets when nobody has named its cause. The honest position is narrower: this diff cannot plausibly reach it — it edits one assertion and deletes a setTimeout in a different spec file, with no production code touched, and RSS in an unrelated spec is not a surface a test-only change reaches. But "implausible" is not "measured", and one run on each side is not a discriminator.
A second full serial run on this branch is in flight to see whether it recurs. If it recurs, this becomes a real finding and gets its own ticket rather than a footnote. If it does not, it is an intermittent under load and I will say exactly that, with both run receipts, and no stronger.
What this does not change
The approval and the merge gate are @tobiu's call with these facts in hand. What the PR claims on its own evidence is unaffected: 25/25 consecutive isolated runs clean (was 6/16 failing), a mutation control proving the surviving assertion is not vacuous, and memory-core/ serial at 1883 passed. AC-1 through AC-3 stand; AC-4 is open.
⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code
@neo-opus-ada commented on 2026-08-25T14:23:31Z
The case for this PR is stronger than the case I made for it
@neo-opus-vega checked the source rather than accepting my framing, and the result upgrades this change. Verified independently before writing it down.
My body argues the fixture cannot control the ordering — true, and sufficient. The sharper statement is that production never leaves that ordering to chance.
TextEmbeddingService.notifyProviderTimeout (:581-605) exists precisely to give both local providers one shared ordering promise, in its own words:
"if this attempt timed out, the caller's circuit is told before anything else can be dispatched into the provider that just failed"
The hook runs synchronously, before admission advances, so the drain cannot select the next span first. The two providers have genuinely different admission machinery — native Ollama's capped slot versus the OpenAI-compatible single-lane queue whose drain selects immediately after a rejection — and the helper exists because expressing that ordering twice is what let it exist in only one of them.
The helper names its own exception: a hook that throws before opening its circuit falls outside the guarantee. That exception is unreachable in production today — exactly one hook implements it (TenantRepoSyncService.mjs:1823), signal.aborted is a property read, and createProviderTimeoutCircuitError() is new Error(<literal>) plus a constant assignment and a return, so it cannot throw.
Why that matters for this diff
It changes what the removed assertion was. I described it as weak evidence for a real property. It was not evidence for anything: the race it sampled exists only inside the fixture, because every production caller is sequenced synchronously through the hook. So the choice was never "keep imperfect coverage or lose it" — it was "keep a coin flip that costs red dev CI under neomjs/neo#17750's gate, in exchange for nothing."
That is a better justification than the one in the body, and I would rather it be on the record than have a future reader reconstruct the weaker argument from the commit message.
Also corrected
The lane-guide entry I split into neomjs/neo#17760 said the opposite — that the lane does not order the abort against the drain. It was wrong, and a wrong contract fact in the lane's owning document is worse than a wrong test, because the next reader trusts it instead of re-measuring. Corrected there at 3b4dbab4dc before @neo-preview graded it; the entry now documents the guarantee and the single condition it rests on.
Nothing here changes this PR's head or its content. AC-4 remains open per the receipt above.
⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code
@neo-opus-ada commented on 2026-08-25T14:28:02Z
AC-4 — now met, and the earlier red has its cause named
Second full serial on this branch, same invocation and same head:
--workers=1 --retries=0, whole unit suite
run 1 → 15170 passed · 11 skipped · 1 failed (13.9m) ← knowledgeBaseArtifact:775
run 2 → 15172 passed · 11 skipped · 0 failed (13.6m) exit 0, zero `flaky`
AC-4 is satisfied by run 2. Both runs pass this PR's own arm, and flaky appears zero times in either.
The run-1 failure, named rather than labelled
I said I would not call it "unrelated" until it had a cause. Four measurements now:
| probe |
result |
pre-change baseline serial (dev @ 2bc6907ed4) |
passed |
| this branch, serial run 1 |
failed |
| this branch, serial run 2 |
passed |
| isolated, 6 consecutive runs |
6/6 passed |
So: an intermittent under full-suite load, not reproducible, and not reachable by this diff — which edits one assertion and deletes a setTimeout in a different spec file, touching no production code. process.memoryUsage().rss is a process-level reading, and its value depends on everything that ran before it in the same process plus the machine's state; this host has been running back-to-back suites all session. That is the cause I can evidence: load sensitivity, one occurrence in two runs.
What I am not claiming: that it is fine. It flaked once, and one non-recurrence does not prove an intermittent gone — the same standard I applied to my own arm, where the answer was 25 consecutive isolated runs rather than one green.
Worth someone's attention, and deliberately not a ticket from me
That arm is a load-sensitive assertion in a suite that now runs under neomjs/neo#17750's failOnFlakyTests. At workers: 4, retries: 2 a flake that passes on retry is disqualifying, so if this recurs in CI it will redden dev the same way my arm was about to. I am not filing it: one observation in two runs is thin, I have already filed three tickets today, and the flaky gate itself is the instrument that will surface it with better evidence than I have. Recorded here so that when it does, nobody re-derives the load-sensitivity from scratch.
All four ACs on neomjs/neo#17758 now stand. Nothing about this PR's head or content changed.
⚖️ Ada · @neo-opus-ada · Claude Opus 5 · Claude Code
Resolves neomjs/neo#17758
Refs neomjs/neo-agent-brain#46
TextEmbeddingService.retry.spec.mjs:1248— "the queued repository stays protected even when the circuit-open is DEFERRED" — failed ~37% of the time in complete isolation (6 of 16 isolated runs, same assertion every time:requestCountexpected 1, received 2). Not pollution, not order-dependent — the spec alone, nothing else in the run.Evidence: L2 (25 consecutive isolated runs plus a mutation control on the surviving assertion) → L2 required (the close-target ACs are all run-observable). No residuals.
AC Evidence
--retries=0, against 6/16 failing before. The rate is ~37%, so a single green would have been meaningless — the run count is the evidence.requestCountpin is gone, and the fixed 50 ms sleep with it.daemons/orchestrator/services/TenantRepoSyncService.spec.mjsowns the deferred ordering proof, so the coverage is not read as lost.memory-core/serial: 1883 passed. Full serial unit run receipt added as a comment below when it completes (~15 min).The mechanism
Deferring the abort to a macrotask (
setTimeout(() => controller.abort(circuitError), 0)) puts two macrotasks in an unordered race after A's 400 ms timeout rejects: the drain selecting and dispatching B, and the queued abort removing it. Nothing the fixture canawaitsequences those two — theawait new Promise(resolve => setTimeout(resolve, 50))it used runs before A's timeout even fires, so it only shifted the odds.The arm's own scope note already said this:
expect(requestCount).toBe(1)therefore asserted precisely the property the paragraph directly above it disclaims.Why no coverage is lost
requestCountto 1 — that path is sequenced, and it is stable across every run I made.TenantRepoSyncService.spec.mjs:3288-3372(run-scoped circuit sweep,KB_VECTOR_EMBED_PROVIDER_CIRCUIT_OPEN). Verified, not taken from the comment.Deltas from ticket
The ticket left the fix shape open between "sequence it deterministically" and "narrow to the fixture's documented scope". Option 1 is not available: both racing steps are macrotasks scheduled by the code under test, so no external await orders them. Option 2 taken.
Test Evidence
toBeFalsy()turns it red (1 failed), so it is not vacuous — it still catches a queued repository that silently succeeds, which is the defect the sibling arm names.memory-core/directory, serial,--retries=0: 1883 passed.Post-Merge Validation
None — the close-target effects are all observable in the unit suite.
Why this was urgent
PR neomjs/neo#17750 merged at 09:49Z and set
failOnFlakyTests: isCI. CI runsworkers: 4, retries: 2, so a ~37% per-attempt flake fails once and passes on retry — exactly the outcome that gate converts into a red run. The flake was not new; its consequence was. This is the gate working correctly on its first real subject, not a defect in neomjs/neo#17750.Evolution
I first recorded this arm on neomjs/neo-agent-brain#46 as a fresh order-dependent pollution witness, on the strength of "passes alone, 54/54". That was two samples of a coin flip — at a 37% rate, two consecutive passes happen roughly 40% of the time by luck. Retracted there; neomjs/neo-agent-brain#46 is back to no live witness. The lesson that produced this PR is that an isolation control needs a repetition count chosen against the suspected rate, not a single clean run.
Authored by Ada (@neo-opus-ada, Claude Opus 5, Claude Code). Session be6b6eb4-dabe-4deb-9924-7c92335c69ff.
AC-4 receipt — not met, and I am not rounding it up
Promised in the body: the full serial receipt. Here it is, unflattering half first.
AC-4 says "reports zero failures." It reported one, so the AC is unmet.
What the failure is — and is not
The failure is not this PR's arm.
TextEmbeddingService.retry.spec.mjs:1248passed, and the stringflakyappears zero times in the run. The repair holds under the fullest ordering pressure available.The one failure is
knowledgeBaseArtifact.spec.mjs:775— "pack streams: peak RSS stays bounded well below the JSONL it rewrites."What I have established so far, stated as measurements rather than conclusions:
dev,2bc6907ed4), where the only failure was the arm this PR fixes.process.memoryUsage().rss— a process-level measurement, so it is load- and environment-sensitive by construction, and this machine has been running heavy suites back to back all session.Why I am not calling it "unrelated"
Because I cannot yet, and "pre-existing / environmental / flaky" is exactly the label a red gets when nobody has named its cause. The honest position is narrower: this diff cannot plausibly reach it — it edits one assertion and deletes a
setTimeoutin a different spec file, with no production code touched, and RSS in an unrelated spec is not a surface a test-only change reaches. But "implausible" is not "measured", and one run on each side is not a discriminator.A second full serial run on this branch is in flight to see whether it recurs. If it recurs, this becomes a real finding and gets its own ticket rather than a footnote. If it does not, it is an intermittent under load and I will say exactly that, with both run receipts, and no stronger.
What this does not change
The approval and the merge gate are @tobiu's call with these facts in hand. What the PR claims on its own evidence is unaffected: 25/25 consecutive isolated runs clean (was 6/16 failing), a mutation control proving the surviving assertion is not vacuous, and
memory-core/serial at 1883 passed. AC-1 through AC-3 stand; AC-4 is open.⚖️ Ada ·
@neo-opus-ada· Claude Opus 5 · Claude Code@neo-opus-adacommented on 2026-08-25T14:23:31ZThe case for this PR is stronger than the case I made for it
@neo-opus-vega checked the source rather than accepting my framing, and the result upgrades this change. Verified independently before writing it down.
My body argues the fixture cannot control the ordering — true, and sufficient. The sharper statement is that production never leaves that ordering to chance.
TextEmbeddingService.notifyProviderTimeout(:581-605) exists precisely to give both local providers one shared ordering promise, in its own words:The hook runs synchronously, before admission advances, so the drain cannot select the next span first. The two providers have genuinely different admission machinery — native Ollama's capped slot versus the OpenAI-compatible single-lane queue whose drain selects immediately after a rejection — and the helper exists because expressing that ordering twice is what let it exist in only one of them.
The helper names its own exception: a hook that throws before opening its circuit falls outside the guarantee. That exception is unreachable in production today — exactly one hook implements it (
TenantRepoSyncService.mjs:1823),signal.abortedis a property read, andcreateProviderTimeoutCircuitError()isnew Error(<literal>)plus a constant assignment and areturn, so it cannot throw.Why that matters for this diff
It changes what the removed assertion was. I described it as weak evidence for a real property. It was not evidence for anything: the race it sampled exists only inside the fixture, because every production caller is sequenced synchronously through the hook. So the choice was never "keep imperfect coverage or lose it" — it was "keep a coin flip that costs red
devCI under neomjs/neo#17750's gate, in exchange for nothing."That is a better justification than the one in the body, and I would rather it be on the record than have a future reader reconstruct the weaker argument from the commit message.
Also corrected
The lane-guide entry I split into neomjs/neo#17760 said the opposite — that the lane does not order the abort against the drain. It was wrong, and a wrong contract fact in the lane's owning document is worse than a wrong test, because the next reader trusts it instead of re-measuring. Corrected there at
3b4dbab4dcbefore @neo-preview graded it; the entry now documents the guarantee and the single condition it rests on.Nothing here changes this PR's head or its content. AC-4 remains open per the receipt above.
⚖️ Ada ·
@neo-opus-ada· Claude Opus 5 · Claude Code@neo-opus-adacommented on 2026-08-25T14:28:02ZAC-4 — now met, and the earlier red has its cause named
Second full serial on this branch, same invocation and same head:
AC-4 is satisfied by run 2. Both runs pass this PR's own arm, and
flakyappears zero times in either.The run-1 failure, named rather than labelled
I said I would not call it "unrelated" until it had a cause. Four measurements now:
dev@2bc6907ed4)So: an intermittent under full-suite load, not reproducible, and not reachable by this diff — which edits one assertion and deletes a
setTimeoutin a different spec file, touching no production code.process.memoryUsage().rssis a process-level reading, and its value depends on everything that ran before it in the same process plus the machine's state; this host has been running back-to-back suites all session. That is the cause I can evidence: load sensitivity, one occurrence in two runs.What I am not claiming: that it is fine. It flaked once, and one non-recurrence does not prove an intermittent gone — the same standard I applied to my own arm, where the answer was 25 consecutive isolated runs rather than one green.
Worth someone's attention, and deliberately not a ticket from me
That arm is a load-sensitive assertion in a suite that now runs under neomjs/neo#17750's
failOnFlakyTests. Atworkers: 4, retries: 2a flake that passes on retry is disqualifying, so if this recurs in CI it will reddendevthe same way my arm was about to. I am not filing it: one observation in two runs is thin, I have already filed three tickets today, and the flaky gate itself is the instrument that will surface it with better evidence than I have. Recorded here so that when it does, nobody re-derives the load-sensitivity from scratch.All four ACs on neomjs/neo#17758 now stand. Nothing about this PR's head or content changed.
⚖️ Ada ·
@neo-opus-ada· Claude Opus 5 · Claude Code