fix(tests): make the stdin-write/unregister guard discover, and record two residuals (#2448)

[webmixgamer]

Summary

Follow-up to #2435. Its re-review arrived after the PR had already auto-merged, so two items never landed — and one of them is the item the reviewer said was the only thing standing between the fix and its own recurrence. All three changes here are corrections to claims that shipped wider than the code.

• The guard enumerated two files, so it did not guard. Proven by dropping a mistral_runtime.py into the tree with an unpaired stdin.write: 2 passed, 13 deselected. It now discovers — every agent_server/**/*.py that drives the process registry — and requires each stdin.write to be paired with unregister().
• _publish_calling_and_check_cancel_sync said "fails soft", which is true of the read half it replaced and not of the write half it adds. The residual is now documented at the helper, in architecture.md and in chat-turn-cancellation.md, all of which had inherited the too-wide "closed" claim.
• restamp_execution_dispatch records why the re-stamp reaches duration_ms — and why its sibling execution_time_ms still spans the park and is not a bug.

No behaviour change outside the docstrings. The guard is the substantive part.

The fourth site

A *_runtime.py glob would not have been sufficient either: it misses claude_code.py and headless_executor.py — and the latter is a real fourth site the enumeration had never counted. It is safe, but by a different shape: _run_headless_subprocess registers and writes stdin with no local guard, yet it is handed to run_in_executor inside execute_headless_task's try whose finally unregisters, so an exception propagates there.

So the guard models both accepted shapes:

shape	example
local — write inside a try that unregisters	claude_code.py, gemini_runtime.py ×2
caller-paired — writing function referenced inside a guarded try	headless_executor._run_headless_subprocess

A companion test pins which shape covers each known site, because caller-pairing is name-based (no call graph) and a name collision could otherwise mask a real offender — a false negative. Cross-module caller pairing is deliberately not modelled: it fails loudly rather than passing silently, which is the safe direction for a guard.

Two traps the reviewer flagged for exactly this move, both handled:

• _stdin_write_is_guarded returned total > 0 and guarded == total, so a discovered file with no stdin write would read as a FAILURE the moment enumeration stopped (codex_runtime.py registers but uses stdin=DEVNULL). It now returns an offender list, so "no writes" is vacuously clean.
• A discovery guard that discovers nothing passes vacuously, so the discovery floor is pinned — a broken walk (moved tree, renamed package) fails loudly.

The documented residual

If the owner's publish pipeline raises transiently while the other worker's connection is healthy, the marker keeps saying parked, so the remote can still finalize CANCELLED under a live POST — the original #378 symptom on the Redis-error path. Bounded by the 30s negative cache plus one tick. A hard outage is safe by construction: a process whose client is None never wrote a marker, so the remote's first read returns None and routes through the agent. Failing closed was rejected — every other Redis touch in this subsystem is fail-open, so a blip would fail every dispatch — and a retry cannot close it either.

Changes

• tests/unit/test_2433_review_fixes.py — 15 → 20 tests; the enumerating parametrize replaced with discovery + mechanism pinning
• src/backend/services/agent_call_limiter.py — residual docstring
• src/backend/db/schedules/executions.py — duration_ms vs execution_time_ms docstring
• docs/memory/architecture.md, feature-flows/chat-turn-cancellation.md, feature-flows/parallel-headless-execution.md — qualify the "closed" claim; document the fourth site and the discovering guard
• tests/registry.json

Test Plan

• Guard verified against the reviewer's PoC by putting a real file in the tree — caught by file:line, then removed
• tests/unit/test_2433_review_fixes.py → 20 passed
• Blast radius (2433 / cleanup / watchdog / slot / limiter / terminate / 679 / 1332 / 1094 / 2127 / headless) → 220 passed
• Full unit suite under CI conditions (clean dev worktree, no submodules, seed 20260831) → 13098 passed, 0 failed

Fixes #2448
Refs #2433, #2435

Generated with Claude Code

[dolho]

/review — #2450 (#2448, follow-up to my #2435 re-review)

Built a worktree off 87fd32c0 and tried to defeat the guard rather than read it. This does what I asked and one thing better — the discovery found a site my own suggestion would have missed.

It works, and the mutations prove it

mutation	result
a NEW fourth runtime that registers then writes stdin unpaired	FAIL — mistral_runtime.py:5, exact file:line
strip the local pairing from gemini_runtime.py	FAIL ×2 (offender + mechanism pin)
strip the local pairing from claude_code.py	FAIL ×2

20 pass on the real tree. That first row is the whole point of the PR — the enumerating predecessor could not produce it.

The correction I should record: my suggested fix was wrong

I said "glob agent_server/services/*_runtime.py plus claude_code.py". That misses headless_executor.py, which is a live fourth site (stdin.write at line 1085), and it is caller-paired rather than locally paired — a shape I had not considered at all. Discovering on "drives the process registry" instead of on filename is the right axis, and the four sites it finds are exactly right:

claude_code.py         stdin writes at [343]
codex_runtime.py       stdin writes at -        (vacuously clean, stdin=DEVNULL)
gemini_runtime.py      stdin writes at [264, 710]
headless_executor.py   stdin writes at [1085]   (caller-paired)

The total > 0 and guarded == total vacuous-failure trap I flagged is gone too — a module with no write returns [].

The residual docstrings are better than what I asked for

I asked only that the fail-soft claim stop covering both halves. This also quantifies the window, and I verified the numbers rather than taking them: INFLIGHT_REDIS_RETRY_SECONDS = 30.0, INFLIGHT_TICK_SECONDS = 15.0, _note_redis_failure arms _REDIS_UNAVAILABLE_UNTIL = now + 30s, and _get_client's default honours it — so the refresher genuinely cannot republish until it lapses plus one tick. The bound is real.

The duration_ms / execution_time_ms two-clocks note in executions.py is also correct — that matches what I traced end-to-end in the first review.

---

One finding

The caller-pairing exemption is name-based over ATTRIBUTES too, and that is reachable (Confidence 9/10)

_unguarded_stdin_writes builds covered_names from every identifier inside a guarded try:

if isinstance(n, ast.Name):
    covered_names.add(n.id)
elif isinstance(n, ast.Attribute):
    covered_names.add(n.attr)      # <-- this half

The docstring flags a possible collision, but the ast.Attribute half makes it ordinary rather than theoretical: any method call inside any guarded try exempts every same-named function in the module. Planted this — a plain shape, not a contrived one:

class Runtime:
    def execute(self, prompt, process, execution_id):        # unpaired: a real leak
        get_process_registry().register(execution_id, process, metadata={})
        process.stdin.write(prompt)
        process.stdin.close()

    def dispatch(self, runtime, execution_id, prompt, process):
        try:
            return runtime.execute(prompt, process, execution_id)   # <-- exempts `execute`
        finally:
            get_process_registry().unregister(execution_id)

→ 2 passed. The leak is invisible. execute / run / send / close are exactly the names that appear as attribute calls inside a dispatch try.

This matters more than a normal false negative because it lands on the case the PR exists for: test_each_known_site_is_pinned_to_its_pairing_mechanism protects the four known sites from silently switching path, but a new module — the thing discovery was added for — gets no such pin.

Fix, verified both directions: delete the ast.Attribute branch.

real tree with the branch removed          -> 20 passed   (unchanged)
the planted false negative, branch removed -> FAIL        (caught)

It stays green because the one legitimate caller-paired site is a bare Name — run_in_executor(_HEADLESS_EXECUTOR, _run_headless_subprocess, ctx) at headless_executor.py:1661. So the attribute half buys nothing today and costs the guard its teeth on new code. Two lines.

Minor

_is_registry_module keys on n.func.attr == "register", so a module that only calls register_pending and writes stdin is out of scope. unregister() discards pending too, so the pairing would be meaningful there. No such module exists today — worth a sentence in the docstring rather than a code change, so the next reader knows the boundary was chosen.

Clean

• Source changes are docstrings only — correct. I explicitly asked for no mechanism here, and none was added; the ordering fix from #2435 is untouched.
• Docs are honest, including the part that corrects me: the flow doc and architecture.md now name the fourth site and say why a glob misses it.
• tests/registry.json updated with the new static-guard tag and a description matching what the tests now do.
• Test-merges clean against current dev (13 behind).
• Suite: 2433/2435/limiter/registry/headless/cleanup → 578 passed, 1 skipped. The 4 failures in that run are test_ent14_registry_url_ssrf.py::test_the_boundaries_either_side_stay_public — pre-existing on dev (verified on 13272199 with the same four params), unrelated to this PR.

Recommendation

Approve after the two-line ast.Attribute removal. Everything else is done, and done more carefully than the review asked for.

[dolho]

.

[webmixgamer]

Fixed in 3171b5dc — you were right, and I verified it as a live false negative before touching the code rather than taking the report.

planted shape, guard as merged   -> []                     FALSE NEGATIVE
planted shape, attribute half gone -> [('planted.py', 7)]  caught
real tree, attribute half gone     -> 21 passed            unchanged

And your reasoning for why it's safe to delete holds structurally, not just empirically — the one legitimate caller-paired reference is a bare Name:

run_in_executor at line 1661: arg kinds=['Name', 'Name', 'Name']
                              names=['_HEADLESS_EXECUTOR', '_run_headless_subprocess', 'ctx']

So the attribute half genuinely bought nothing. Removed from both copies of the logic — _pairing_mechanisms had the same two lines, and leaving it there would have desynced the mechanism pin from the offender check.

Your mutation battery reproduces on my side after the change:

mutation	result
real tree	21 passed
new unpaired runtime	1 failed (mistral_runtime.py, file:line)
gemini_runtime.py local pairing stripped	2 failed (offender + mechanism pin)
claude_code.py local pairing stripped	2 failed

The planted shape is now pinned as its own test, so the hole can't reopen quietly.

I also wrote the boundary and its remedy into the docstring, because the next person to hit an attribute-paired site will be tempted to put the branch back: such a site is now reported, and the fix is to reference the function by name or add a justified allowlist entry — never to re-add the attribute half. That keeps the failure direction loud, which is the property the whole guard rests on.

Minor — taken as you suggested, docstring not code: scope keys on register( rather than register_pending(, and that is now recorded as a chosen boundary with the reason (every pending registration is promoted by register() at spawn and the write follows the spawn, so no such module exists today) plus a note to widen it if one appears.

On test_ent14_registry_url_ssrf — confirmed pre-existing, independently: 42 passed on this branch and 42 passed in a clean origin/dev worktree. Whatever produced the 4 failures in your run is environmental, not on either side of the diff.

Thanks for the correction on your own suggested fix, too — recording that the glob would have missed headless_executor.py is the part a future reader needs most, since that's what made "discover, don't enumerate" the right axis rather than just the thorough one.