test(witan): concurrency was argued from the design, never observed

[blarghmatey]

The multi-user deployment's write-coordination story rested on reasoning alone,
and that reasoning is the weak kind: acquire_store_flock is skipped for
http(s) stores (_UNLOCKABLE_SCHEMES), so the advisory lock that serialises
local writers does nothing on the shared server, and task_claim is documented
best-effort CAS pending an upstream conditional-write. Every existing
concurrency test runs in-process against a local store — the one configuration
where the lock DOES work, and not the one that ships.

This adds witan.scripts.concurrency_probe: N genuinely independent OS
processes (separate interpreters, MCP clients and connections) busy-wait on a
shared epoch and fire together, then report counts, not adjectives.

witan login --target ci
uv run python -m witan.scripts.concurrency_probe --target ci

Probe	Asks
A mutual-exclusion	N racers on one task_claim → exactly one claimed: true, every loser a structured refusal
B no-lost-writes	N concurrent memory_store → every acked row readable afterwards
C read-availability	readers running during B's write storm → every read returns

It writes real rows, tags everything concurrency-probe, and cleans up on the
way out; --keep leaves them.

What it observed against witan.ci.ol.mit.edu

Recorded in the module docstring so a later run has something to differ from:

• A held at 3, 8 and 16 racers, every run: 1 claimed, 15 refused, 0 errored.
  Caveat worth keeping: every loser refused with held, never lost_race — the
  winner's write landed before the losers' read, so the best-effort CAS retry
  loop was not itself exercised even at 16-way contention.
• B zero lost writes at every size. At 16 writers, 4–5 were rejected outright
  at connect; every write the server acked was readable. It fails closed.
• C clean to 8 readers, degrades above — the same connect-level rejection as B.

So: safe under concurrency, but it sheds load ungracefully above ~24 connections.
Both causes are filed separately rather than smoothed over —
tk-deployed-witan-saturates-at-24-32-concurrent-con-8e4afc (ToolHive vMCP
OOMKilled at 512Mi) and
tk-concurrent-agents-stampede-the-oidc-token-refres-677984 (N clients stampede
the shared OIDC token cache).

Two things the probe must defend against to measure anything at all

Both were findings in their own right:

1. A verification read that RAISES is inconclusive, not a lost write. Calling
   it one manufactured 13 false positives on the first run. LOST and
   UNVERIFIABLE are now counted and reported separately, and neither passes.
2. The workers must share one pinned token. The proxy calls its token
   provider on every invoke, so N clients firing at one instant stampede the
   OIDC cache — the probe would measure that bug instead of the store it is
   aimed at.

Verification

• just test-witan-council → 722 passed
• ruff check / ruff format --check clean
• Re-ran against CI after rebasing onto main (which carries fix(witan): two of one person's parallel sessions silently shared a task claim #208's
  session-qualified claim holders): A/B/C all PASS at 3 racers / 3 writers /
  2 readers, 0ms fire spread, cleanup complete. The probe passes explicit
  per-worker assignees, so fix(witan): two of one person's parallel sessions silently shared a task claim #208's holder change doesn't affect it.

Refs: tk-verify-the-deployed-witan-supports-concurrent-us-2da1b2
(phase-exit criterion 2 of wp-witan-multi-user-service-deployment-dcf6ee,
acceptance criterion 1: "a repeatable harness, committed, that any maintainer
can re-run against a deployed target").

🤖 Generated with Claude Code

https://claude.ai/code/session_015tsjX4m3AcnRNq6EBmtXV7

[Copilot]

Pull request overview

Adds a deployed concurrency probe for validating Witan’s multi-process coordination and availability behavior.

Changes:

• Adds synchronized claim, write, and read probes.
• Pins authentication tokens and reports quantitative outcomes.
• Adds cleanup support and maintainer documentation.

Reviewed changes

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

File	Description
witan/scripts/concurrency_probe.py	Implements the deployed concurrency harness.
witan/scripts/__init__.py	Documents the maintainer scripts package.

Suppressed comments (3)

mcp/servers/witan/witan/scripts/concurrency_probe.py:421

• Probe B prints the write fire spread but does not include it in the pass condition, so staggered writes can produce PASS without exercising concurrent writers. Require all writers to be ready before the epoch and enforce an explicit spread tolerance before accepting this result.

        # Only a row the server ACKED and that is then verifiably ABSENT is a
        # lost write. Unverifiable reads make the probe inconclusive, not failed.
        passed=not missing and not dupes and not write_errors and not unknown,

mcp/servers/witan/witan/scripts/concurrency_probe.py:365

• This second launcher also places the pinned bearer token from pl in argv, exposing it to process inspection for every writer and reader. Use the same non-argv transport (stdin or an inherited pipe) as the claim launcher.

                    "--payload",
                    json.dumps(pl),

mcp/servers/witan/witan/scripts/concurrency_probe.py:376

• The combined writer/reader collector has the same uncaught TimeoutExpired path: a timed-out child is not killed or reaped, and the probe exits instead of reporting degradation. Centralize collection with _spawn and ensure timed-out processes are terminated and represented as failed rows.

    write_rows, read_rows = [], []
    for mode, proc in procs:
        stdout, stderr = proc.communicate(timeout=300)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[blarghmatey]

Addressed all 13 findings from the Copilot review (10 threads + the 3 suppressed) in fa7e49a — 10/10 threads replied to and resolved, 0 code findings declined, 9/9 checks green.

Every finding was verified against real code before acting; none turned out to be a false positive. Ten of the thirteen were one defect wearing different hats, and it is the one that matters most for this file specifically: the probe could report PASS on a run where the concurrency it claims to measure never happened. An instrument whose whole job is to certify exit criterion 2 of wp-witan-multi-user-service-deployment-dcf6ee cannot be capable of a green tick that means nothing.

Most telling: _spread_ms's own docstring already said a wide spread means "the probe measured a queue, not a race, and the result should not be trusted" — and no pass condition anywhere consulted it. The script contradicted itself.

So all three probes now refuse to pass unless they can show contention occurred, and print why when they cannot:

• A and B fail on any worker that missed the epoch (ready_at > start_at — it never entered the busy-wait and fired the instant it was ready) or a fire spread beyond --spread-tolerance-ms.
• C fails unless every clean read overlapped the writers' (min(fired_at), max(done_at)) window. A reader that ran after the storm ended was counting as availability under load it never experienced.
• --racers 1, --writers 0, --readers 0 are rejected before any network call, rather than producing a confident PASS over nothing.
• Malformed answers are no longer evidence: a task_claim refusal must be claimed is False with a non-empty reason, and a write that returns ok with no slug is an error rather than falling silently between acked and write_errors.

The other three were straightforward and equally real: the pinned bearer token was in worker argv (visible in ps//proc/*/cmdline for the entire lead interval — now on stdin, written at spawn so warmups still overlap); TimeoutExpired was uncaught, aborting the probe and leaving a worker running against the live deployment (now killed, reaped, recorded as degradation); and cleanup ran only on the happy path, leaving rows in the shared graph on any raise (now in a finally, with memory_delete's deleted field actually checked — it refuses by returning, not raising, so the count could claim 8/8 cleaned with all 8 still present).

Two judgment calls to flag rather than bury:

1. DEFAULT_SPREAD_TOLERANCE_MS = 500 is a starting guess, not a measured constant, and is marked as such in the code. Too loose and the new gate is theatre; too tight and the probe is flaky. The first few live runs should calibrate it from observed spread.
2. I did not take the suggested fix on the token-lifetime thread, because it does not work: including both phase timeouts needs ~700s of token life and a Keycloak access token lives ~300s, so _pinned_token would exit on every invocation. Took the per-phase pinning alternative instead, plus WORKER_TIMEOUT_S 300s -> 90s so the requirement fits inside a real token at all. A worker makes one tool call after the epoch, so that guard is a hang detector, not a work budget.

Verified behaviourally, not just by reading: the timing gate correctly rejects a late worker and a 3000ms spread; all four vacuous configs are rejected; a worker reads its payload from stdin with nothing sensitive in argv; and a stub child sleeping 60s against a 1s guard is killed and reaped in 1.0s with a TimeoutExpired row instead of hanging the run.