Skip to content

feat(guard): hand-woven concurrency gates are held at zero in production and ratcheted in tests - #2782

Merged
rbuergi merged 3 commits into
mainfrom
feat/hand-woven-gate-ratchet
Aug 30, 2026
Merged

rbuergi merged 3 commits into
mainfrom
feat/hand-woven-gate-ratchet

Conversation

@rbuergi

@rbuergi rbuergi commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

AGENTS.md states the rule — a SemaphoreSlim or any hand-rolled gate is forbidden outside the one sealed inside IoPool — and nothing enforced it. This is the conservative half of the sweep: freeze the debt, so the wave that clears it can be written without the count moving underneath.

Two tiers, the shape #2762 established

  • ProductionRoots (src, tools, samples, clients, memex) — ZERO, no allow file. The only permitted gates are SanctionedGates entries, verified not listed: each must still exist and still contain a gate, so an entry cannot outlive its subject.
  • RatchetedRoots (test) — a seeded inventory that may only shrink. Measured on main's tip: 79 sites across 24 files, every one a ManualResetEventSlim, 30 of them in IoPoolTest.cs.

🚨 It found a production violation on its first run

tools/MeshWeaver.ThumbnailGenerator — which my earlier src/-only grep had missed, and which I had reported to the maintainer as "production is clean". It is not; that report was scoped too narrowly.

Registered rather than converted, with the premise verified rather than asserted in prose. Its csproj is an Exe referencing Microsoft.Playwright, SixLabors.ImageSharp and System.CommandLine — not one MeshWeaver assembly. There is no hub, no grain turn and no IIoPool in that process, so the defect the rule prevents cannot occur; its semaphore bounds concurrent browser pages against an external server.

The register re-reads that csproj on every run and fails if a MeshWeaver ProjectReference ever appears. An exemption whose premise silently stops holding is worse than one whose subject moved — the subject's absence at least fails loudly. And tools stays in scope rather than being dropped, so a gate added to a tool that does reference the mesh still fails.

Why the test tree is in scope at all

In a test the same primitive fails more subtly: it strands a blocked worker when an assertion throws before the release runs. Measured today on #2770 — IoPoolResidualNamesItsPoolTest put release.Set() after an await of the method-timeout token, so on timeout the release never ran and the leaf held a pool thread for two minutes, into the next test.

Proven by breaking it, not by passing

The self-test plants a real directory tree and runs the real scan over it — .cs, .csx, a prose mention, a string literal and bin/ — rather than matching the regex against strings. That distinction is exactly the defect found in three separate guards in this repo today: a self-test that never calls its own scanner can lose half its scope and stay green.

Then each arm was mutated:

probe result
a new gate in src/ 🔴 production zero
a new file in test/ outside the allow list 🔴 NEW SITE
a listed test file's count grows 🔴 may only shrink
all restored 🟢 4/4

What this deliberately does NOT do

It does not convert the 79 test sites. That is a 24-file change with real deadlock-shaped subtleties in each one, and it wants its own review — the .ToTask() programme worked precisely because the ratchet landed first and the sweep followed against a frozen number.

🤖 Generated with Claude Code

…ion and ratcheted in tests

A SemaphoreSlim / ManualResetEventSlim / AutoResetEvent / CountdownEvent /
Monitor.Wait parks a thread. On a hub action block or a grain turn that is the
deadlock the mesh cannot recover from — the class /async names alongside await
on a hub. AGENTS.md states the rule; nothing enforced it.

Two tiers, the shape #2762 established:

- ProductionRoots (src, tools, samples, clients, memex) — ZERO, no allow file.
  The only permitted gates are SanctionedGates entries, VERIFIED not listed:
  each must still exist and still contain a gate, so an entry cannot outlive
  its subject.
- RatchetedRoots (test) — a seeded inventory that may only shrink. Measured on
  main's tip: 79 sites across 24 files, every one a ManualResetEventSlim, 30 of
  them in IoPoolTest.cs. TotalBudget 79.

🚨 The guard found a production violation on its first run that a src/-only
grep had missed: tools/MeshWeaver.ThumbnailGenerator. Registered rather than
converted, with the premise VERIFIED rather than asserted in prose — its csproj
is an Exe referencing Microsoft.Playwright, SixLabors.ImageSharp and
System.CommandLine and NOT ONE MeshWeaver assembly, so there is no hub, no
grain turn and no IIoPool in that process and the defect cannot occur. The
register re-reads that csproj on every run and FAILS if a MeshWeaver
ProjectReference ever appears: an exemption whose PREMISE silently stops
holding is worse than one whose subject moved, because the subject's absence at
least fails loudly. `tools` stays in scope rather than being dropped, so a gate
added to a tool that DOES reference the mesh still fails.

The self-test plants a real tree and runs the REAL scan over it — .cs, .csx,
prose, a string literal and bin/ — rather than matching the regex against
strings. That distinction is the defect this repo found in three separate
guards today.

Verified by MUTATION, each arm proven able to fail and then restored:
  A new gate in src/                        -> RED (production zero)
  A new file in test/ not in the allow list -> RED (NEW SITE)
  A listed test file's count grows          -> RED (may only shrink)
  restored                                  -> GREEN 4/4

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI lite review requested due to automatic review settings August 30, 2026 13:54

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a governance ratchet to enforce the “no hand-woven concurrency gates” rule: production roots must stay at zero (except a verified sanctioned register), while the test tree is held to a seeded inventory that may only shrink.

Changes:

  • Introduces HandWovenGateRatchetGuard to scan production/test roots for gate primitives and enforce the zero/ratchet policy.
  • Adds a seeded allow file enumerating current test-tree gate sites/counts.
  • Adds a “What’s New” documentation entry describing the new enforcement.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
test/MeshWeaver.Documentation.Test/HandWovenGateRatchetGuard.cs New ratchet guard test enforcing zero-in-production and shrink-only-in-tests scanning policy.
test/HandWovenGateSites.allow Seeded inventory of current test-tree gate sites and counts for the ratchet to enforce.
src/MeshWeaver.Documentation/Data/WhatsNew/2026-08-30-hand-woven-concurrency-gates-cannot-come-back.md User-facing documentation entry announcing the new guard/ratchet behavior.
Suppressed comments (1)

test/MeshWeaver.Documentation.Test/HandWovenGateRatchetGuard.cs:146

  • Like the production scan, this test relies on SourceScan.SourceFiles, which silently ignores missing roots. If the "test" root is renamed/moved, the guard will stop scanning and can pass vacuously. Add an explicit existence assertion for RatchetedRoots.
        var root = SourceScan.FindRepoRoot();
        var allowed = SourceScan.ReadAllowFile(
            Path.Combine(root, "test", AllowFileName), AllowFileName);

        var actual = SourceScan.SourceFiles(root, RatchetedRoots)

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment on lines +29 to +32
/// <item><see cref="RatchetedRoots"/> — <c>test/</c> carries a seeded inventory that may only
/// SHRINK. Measured 2026-08-30: 79 sites across 27 files, every one a
/// <c>ManualResetEventSlim</c>, 30 of them in <c>IoPoolTest.cs</c>. The sweep lands in a later
/// wave; this freezes the debt so it cannot grow while that wave is written. When it empties,
Comment on lines +92 to +94
var root = SourceScan.FindRepoRoot();
var sanctioned = SanctionedGates.Select(s => s.File).ToHashSet(StringComparer.Ordinal);

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 3)

734 tests   542 ✅  4m 52s ⏱️
  5 suites  192 💤
  5 files      0 ❌

Results for commit c36feaa.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 0)

951 tests   951 ✅  5m 26s ⏱️
  6 suites    0 💤
  6 files      0 ❌

Results for commit c36feaa.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 5)

1 547 tests   1 547 ✅  6m 2s ⏱️
    8 suites      0 💤
    8 files        0 ❌

Results for commit c36feaa.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 1)

971 tests   967 ✅  6m 17s ⏱️
  6 suites    4 💤
  6 files      0 ❌

Results for commit c36feaa.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 4)

   10 files     10 suites   5m 31s ⏱️
3 481 tests 3 481 ✅ 0 💤 0 ❌
4 146 runs  4 146 ✅ 0 💤 0 ❌

Results for commit c36feaa.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Test Results (shard 2)

803 tests   803 ✅  6m 28s ⏱️
  6 suites    0 💤
  6 files      0 ❌

Results for commit c36feaa.

♻️ This comment has been updated with latest results.

@github-actions

github-actions Bot commented Aug 30, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

   41 files     41 suites   34m 38s ⏱️
8 487 tests 8 291 ✅ 196 💤 0 ❌
9 152 runs  8 956 ✅ 196 💤 0 ❌

Results for commit c36feaa.

♻️ This comment has been updated with latest results.

rbuergi added a commit that referenced this pull request Aug 30, 2026
 merged (#2783)

`scripts/type-forwards.allow` still lists

    MeshWeaver.Fixture:MeshWeaver.Fixture.ObservableAwait

but #2771 has merged, so that move is IN main. The gate compares against the
MERGE BASE, so relative to any branch cut after #2771 nothing of that name
moves — and the entry is reported as STALE:

    STALE ALLOW ENTRY: scripts/type-forwards.allow lists
    `MeshWeaver.Fixture:MeshWeaver.Fixture.ObservableAwait`,
    but nothing of that name moved in this diff

🚨 That reddens EVERY subsequent PR, not one. Measured: both currently open PRs
fail `Public surface (binary compatibility)` on it — #2782 (a test guard plus an
allow file) and #2781 (a documentation-only Homebrew change). Neither can reach
a public type; both are blocked by main.

This is the failure mode the allow file's own rule names and that #2678 -> #2688
already paid for once: an allow entry goes stale FLEET-WIDE the instant its move
merges, so it must be deleted in the VERY NEXT change. It was flagged on #2771
before that merge and landed with it anyway, which is exactly how the previous
seven stale entries accumulated.

The file now carries no live entries — header only, which is the correct state:
nothing is currently exempt.

Verified:
  scripts/check-type-forwards.py --self-test  -> All 28 self-tests passed
  scripts/check-type-forwards.py --base <merge-base>
                                             -> OK, no unguarded public-type move

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi and others added 2 commits August 30, 2026 16:17
The ratchet caught real drift on its first exposure to a moving main —
IoPoolTest.cs 30 -> 32, from #2770 (`a pooled-I/O residual NAMES the leaf that
would not unwind`) landing after this branch took its seed.

Re-seeded rather than argued with: the allow file's own header says to seed
against main's TIP, not the branch point, precisely because CI checks out the
merge ref. TotalBudget 79 -> 81, with the reason recorded in the guard's
remarks so the next reader does not read 81 as the original measurement.

🚨 This is the ratchet working, not a concession. Those two sites come back out
in the IoPool drain-budget change, which lowers the budget to 79 again — the
mechanism the file documents (delete a line, lower the budget by exactly that
amount, in the same change).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi added a commit that referenced this pull request Aug 30, 2026
…leaking a pool thread

#2770 merged without these. On main the test now:

  * cannot pass at all — a residual is only observable by letting IoPool's
    drain budget EXPIRE, DrainTimeout is 30s and Drain() spends it THREE times
    (cancel join, each gate slot, blocking-idle join), while
    test/xunit.runner.json caps every method at methodTimeout: 30000. CI
    recorded it as `[took 30s]`, killed at exactly the budget with no assertion
    message;
  * LEAKS on that path — `release.Set()` sits after
    `await drain.WaitAsync(..., TestContext.Current.CancellationToken)`, and
    that token IS the method-timeout token. On timeout the await throws, the
    release never runs, the deliberately-blocked leaf holds a pool thread for
    its full two minutes, and `using` disposes over live work. The test written
    to prove residuals are reported was leaking one into the next test.

Fixes, in the order they matter:

1. IoPool gains an `(int maxConcurrency, TimeSpan drainTimeout)` overload and
   IoPoolOptions a `DrainTimeout`, threaded through IoPoolRegistry.Get. The
   default is UNCHANGED at 30s and is now pinned by its own test, so making it
   injectable cannot quietly move the teardown contract.
2. The test drives a 250ms budget and still calls the REAL
   registry.DrainAll(out byPool) — the subject is "the residual is reported and
   NAMES its pool", not the numeric budget.
3. The release is UNCONDITIONAL, in a `finally`.
4. Both ManualResetEventSlim gates become AsyncSubject<Unit>, per /async: a
   hand-woven gate is the same defect class as await on a turn-based scheduler.
   The one remaining block IS the subject (a leaf ignoring its token) and is
   now bounded at 30s so a failing run cannot wedge the shard.

Measured from the trx, where per-test duration actually lives:

  DrainAll_reportsTheResidual…    00:00:00.279   (was: killed at 30s)
  TheDefaultDrainBudget…          00:00:00.0004
  DrainAll_reportsNoPool…         00:00:00.0007

This also removes 2 of the 81 hand-woven gates the ratchet in #2782 seeds; the
one merged second should lower TotalBudget 81 -> 79 and drop IoPoolTest.cs's
count accordingly. A seed HIGHER than actual only leaves slack, so either merge
order is safe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@rbuergi
rbuergi merged commit 62d5af7 into main Aug 30, 2026
25 checks passed
@rbuergi
rbuergi deleted the feat/hand-woven-gate-ratchet branch August 30, 2026 14:56
rbuergi added a commit that referenced this pull request Aug 30, 2026
…oven event (#2792)

🚨 MAIN IS RED and this is my break. Two PRs each green alone, red together:
`tools/MeshWeaver.PluginTester/StaticTestRunner.cs` landed with a
ManualResetEventSlim, and #2782 landed the ratchet that holds tools/ at zero.
Neither run could see the other; the merged tree fails.

The fix is to delete the gate, not exempt it, because the standard library
already has this exact primitive. The code ran each case on a dedicated
background Thread and waited on an event the thread Set() in a `finally`:

    var done = new ManualResetEventSlim(false);
    ... finally { done.Set(); }
    thread.Start();
    if (!done.Wait(timeout)) { /* hung case */ }

`Thread.Join(timeout)` answers exactly that question — "did that thread finish
within the budget" — and is STRICTER: the event fired from `finally` signalled
before the thread had actually terminated, whereas Join returns only on real
termination, which is also what gives the `failure` field its happens-before.

Note this is NOT the ThumbnailGenerator case: PluginTester DOES reference
MeshWeaver assemblies (Hosting.Monolith, Hosting, Mesh.Operations,
ContentCollections.Indexing.Graph), so the "no mesh in this process" premise
that justifies that entry does not apply here and an exemption would have been
wrong.

Verified: PluginTester builds -c Release -warnaserror clean;
HandWovenGateRatchetGuard 4/4 green.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi added a commit that referenced this pull request Aug 30, 2026
…leaking a pool thread (#2786)

* test(iopool): the residual test costs 0.28s instead of 30s+ — and now shows its real failure

IoPoolResidualNamesItsPoolTest was killed at exactly 30s in CI with no
assertion message. It could not have passed: a residual is only observable by
letting IoPool's drain budget EXPIRE, DrainTimeout is 30s, and Drain() spends
it THREE times (the cancel join, each gate slot, the blocking-idle join) —
while test/xunit.runner.json caps every method at methodTimeout: 30000.

Worse, `release.Set()` sat after `await drain.WaitAsync(..., TestContext.
Current.CancellationToken)`. That token IS the method-timeout token, so on the
timeout the await threw and the release never ran: the deliberately-blocked
leaf held a pool thread for its full 2 minutes and `using` disposed over live
work. The test written to prove residuals are reported was leaking one.

- IoPool gains an `(int maxConcurrency, TimeSpan drainTimeout)` overload;
  IoPoolOptions gains `DrainTimeout`, threaded through IoPoolRegistry.Get.
  The default is UNCHANGED at 30s and now pinned by its own test, so making it
  injectable cannot quietly move the teardown contract.
- The test drives a 250ms budget and calls the REAL registry.DrainAll(out
  byPool) — the subject is "the residual is reported and NAMES its pool", not
  the numeric budget.
- `release.Set()` moves into a `finally`.

Measured from the trx, which is where per-test duration actually lives:
  DrainAll_reportsTheResidual…   00:00:00.28   (was: killed at 30s)
  TheDefaultDrainBudget…         00:00:00.0006
  DrainAll_reportsNoPool…        00:00:00.0013
  whole class: 300 ms.

🚨 And the speed-up EXPOSED a real defect the timeout was hiding. The test now
fails in 0.28s on a genuine mismatch in this PR's own diff:

  expected "Query=1"
  found    "Query=1 [MeshWeaver.Hosting.Test.IoPoolResidual…b__0]"

PoolResidual.ToString() appends the pending leaf SITES, but the test asserts
the older `{pool}={n}` and calls it "the wire format the teardown traces
embed, so it is part of the contract". One of the two is wrong — left failing
deliberately rather than papered over, because which one is the author's call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(iopool): the residual test costs 0.28s instead of 30s, and stops leaking a pool thread

#2770 merged without these. On main the test now:

  * cannot pass at all — a residual is only observable by letting IoPool's
    drain budget EXPIRE, DrainTimeout is 30s and Drain() spends it THREE times
    (cancel join, each gate slot, blocking-idle join), while
    test/xunit.runner.json caps every method at methodTimeout: 30000. CI
    recorded it as `[took 30s]`, killed at exactly the budget with no assertion
    message;
  * LEAKS on that path — `release.Set()` sits after
    `await drain.WaitAsync(..., TestContext.Current.CancellationToken)`, and
    that token IS the method-timeout token. On timeout the await throws, the
    release never runs, the deliberately-blocked leaf holds a pool thread for
    its full two minutes, and `using` disposes over live work. The test written
    to prove residuals are reported was leaking one into the next test.

Fixes, in the order they matter:

1. IoPool gains an `(int maxConcurrency, TimeSpan drainTimeout)` overload and
   IoPoolOptions a `DrainTimeout`, threaded through IoPoolRegistry.Get. The
   default is UNCHANGED at 30s and is now pinned by its own test, so making it
   injectable cannot quietly move the teardown contract.
2. The test drives a 250ms budget and still calls the REAL
   registry.DrainAll(out byPool) — the subject is "the residual is reported and
   NAMES its pool", not the numeric budget.
3. The release is UNCONDITIONAL, in a `finally`.
4. Both ManualResetEventSlim gates become AsyncSubject<Unit>, per /async: a
   hand-woven gate is the same defect class as await on a turn-based scheduler.
   The one remaining block IS the subject (a leaf ignoring its token) and is
   now bounded at 30s so a failing run cannot wedge the shard.

Measured from the trx, where per-test duration actually lives:

  DrainAll_reportsTheResidual…    00:00:00.279   (was: killed at 30s)
  TheDefaultDrainBudget…          00:00:00.0004
  DrainAll_reportsNoPool…         00:00:00.0007

This also removes 2 of the 81 hand-woven gates the ratchet in #2782 seeds; the
one merged second should lower TotalBudget 81 -> 79 and drop IoPoolTest.cs's
count accordingly. A seed HIGHER than actual only leaves slack, so either merge
order is safe.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* review: satisfying one ratchet by violating another is not a fix

My own conversion traded a hand-woven gate for a blocking bridge:
ManualResetEventSlim -> AsyncSubject + .Wait(). That cleared
HandWovenGateRatchetGuard and tripped BlockingBridgeInTestRatchetGuard
(`NoNewTestBridgesAnObservableToABlockingWait`), which is not progress — it is
moving the same defect between two guards' jurisdictions.

What the test actually needs is (a) to know the leaf started and (b) for the
leaf to outlive the drain budget. Neither needs a signal back INTO the leaf, so
the whole release channel is gone:

  * the precondition goes through the sanctioned test bridge —
    `await entered.Timeout(10s).Await(ct)`; no .Wait(), no gate;
  * the leaf sleeps. The SUBJECT of this test is a leaf that ignores its
    cancellation token, and sleeping IS ignoring it. With a 250 ms budget a 1 s
    sleep outlives it four times over, and the drain returns without waiting for
    the leaf, so the suite pays nothing for it.

Also lowers the gate inventory by exactly what this deletes: the
IoPoolResidualNamesItsPoolTest entry is removed from HandWovenGateSites.allow
and TotalBudget goes 81 -> 79, which is the mechanism that file documents.

Verified:
  IoPoolResidualNamesItsPoolTest      3/3, residual case 00:00:00.279
  BlockingBridgeInTestRatchetGuard +
  HandWovenGateRatchetGuard           6/6 together

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
rbuergi added a commit that referenced this pull request Aug 30, 2026
…led (#2412) (#2799)

Toward #2412 — the structural fix, not a third partial one.

With strict:false every PR is tested against the main it branched from, so a
burst of merges lands a COMBINATION no run ever compiled. Measured twice:
`CS0246: MeshOperations` on 2026-08-26, and today a new guard (#2782) plus an
independently-landed gate primitive (#2792) that were each green alone and
red together — five consecutive red main runs and a documentation-only PR
caught in the blast. #2447 and #2767 each changed WHEN you find out; neither
builds the combination before it lands. A merge queue does.

This repo is PUBLIC, so GitHub's merge queue is available on the org's Team
plan (private repos need Enterprise Cloud — MeshWeaver.Plugins carries the
same trigger dormant for exactly that reason, and its own comment says so).
`mergeQueue(branch:"main")` currently resolves to null: available, not enabled.

What this change does:
- `merge_group:` trigger, so the queue's temporary gh-readonly-queue/main/…
  ref gets the required context. Without it an entry queues, times out and
  is ejected with no failing check to point at.
- The two DIFF gates (record signatures, type forwards) run on merge_group
  too, with base resolved to main — github.base_ref is EMPTY on merge_group,
  and a queue entry's base IS main by construction. Resolved inline at all
  four uses so the two gates cannot drift.
- Nothing else changes: concurrency groups the queue ref per entry (a
  superseded entry gets a new ref), the fork gates read null as "not a fork",
  HEAD_SHA falls through to github.sha, and the executed-and-green marker
  stays push-only.

🚨 Order matters: this must be ON MAIN before the `merge_queue` rule is added
to the `main pr protection` ruleset, or the first queued PR sits forever.
Enabling the rule is the next step, done via the ruleset API once this lands.

Verified: YAML parses with triggers [push, pull_request, merge_group,
workflow_dispatch]; MainRunsAreNeverCancelledGuard 11/11 (the concurrency
expressions are untouched).

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants