fix: reasoning-model flakiness, thinking-mode support, and high-volume-ci tuning#46
Merged
Conversation
TGPSKI
force-pushed
the
fix/curing-collect-fanin-staleness
branch
2 times, most recently
from
July 3, 2026 07:36
cbdc1da to
8deb88c
Compare
TGPSKI
force-pushed
the
fix/high-volume-ci-load-test-fixes
branch
from
July 3, 2026 07:40
c9d91c9 to
da7db81
Compare
TGPSKI
marked this pull request as draft
July 3, 2026 08:26
5 tasks
TGPSKI
marked this pull request as ready for review
July 3, 2026 16:49
http.Client{Timeout: ...} fired independently of the run's context
deadline, producing spurious "context deadline exceeded" errors well
before the real timeout under load. The context deadline (set once in
runner.Run / the curing's timeout_seconds) is now the single source of
truth for request timeouts.
New per-agent override that will send chat_template_kwargs.enable_thinking = false to the model. Zero value (false) leaves default model behavior untouched. Wired up to frontmatter parsing and the runner in following commits.
Adds "thinking: false" as a recognized *.agent.md frontmatter key, parsed into frontMatter.DisableThinking (inverted so the zero value matches "no override, model default").
LoadFile now copies frontMatter.DisableThinking into the constructed model.Agent, completing the thinking: false frontmatter feature.
Verifies "thinking: false" sets DisableThinking, and that it defaults to false (model default, no override) when unset.
Three related fixes found while load-testing a fan-out/fan-in pipeline against a real reasoning model (Qwen3 via vLLM, tool-call-parser qwen3_xml). Confirmed via direct replay against the LLM endpoint outside leather that this is model/parser flakiness under load, not a request- construction bug in the runner (~25% of isolated replays reproduced it; an explicit "don't do this" prompt instruction had zero effect). - Duplicate tool-call guard: the model would occasionally re-issue an already-succeeded tool call verbatim instead of progressing, spinning until max tool rounds was hit. A tool call with the same name and arguments that already succeeded this run is no longer re-executed — the model is told it already happened and the run proceeds. Also guards against duplicating a real side effect (e.g. posting a comment twice). Scoped to non-hide tools; hide pagination tools legitimately expect repeated calls. - Self-healing retry on empty final answers: a related manifestation of the same flakiness — the model would sometimes stop naturally (finish_reason "stop", not "length") with zero output tokens instead of the expected answer, propagating downstream as blank/N/A fields. Extended the existing length-truncation retry to also retry once, bare, on this case. - DisableThinking wiring: merges chat_template_kwargs.enable_thinking = false into the request when Agent.DisableThinking is set, alongside the existing parallel_tool_calls override. Also retains the tool-call-argument debug logging added during investigation (agent/tool/args at DEBUG) — the byte-count-only logging that existed before didn't reveal repeated-argument loops.
- TestRunner_DuplicateToolCallSkipsReExecution: a repeated identical tool call hits the real endpoint once, not twice. - TestRun_SelfHealingRetry_OnEmptyStopCompletion / TestRun_EmptyStopRetry_IsSingleShot: empty finish_reason=stop retries exactly once and passes through if the retry is also empty. - TestRun_DisableThinkingMergesChatTemplateKwargs: DisableThinking merges chat_template_kwargs without clobbering parallel_tool_calls.
…ents - decision.agent.md: the prompt said "copy PR_NUMBER, REPO, SHA verbatim from ANALYSIS 1", assuming the 3 parallel analysis agents always arrive in submission order. Only pr-metadata emits those fields, but pr-context (zero tool calls) often won the race for slot 1, leaving nothing to copy and producing "PR_NUMBER: N/A". Now references the block by its "(from: pr-metadata)" tag instead of position. - All 5 agents: added "thinking: false" and "completion_reserve: 768" (down from the reasoning-model default of 8192). Disabling the hidden reasoning trace was the most effective fix for both the tool-call-loop and empty-final-answer flakiness (see internal/runner fixes), and measured a 5.2x speedup on a 40-webhook burst load test (323s -> 62s) with equal-or-better correctness. See README.md for full numbers. - pr-metadata.agent.md / pr-diff.agent.md: tool_rounds 2 -> 3, giving one extra round of headroom before the max-rounds guard trips. - pr-comments.agent.md: tool_rounds 3 -> 5, and added an explicit "do not call post_pr_comment more than once per PR" instruction (kept as defense in depth alongside the runner-level dedupe guard, even though the instruction alone measurably had no effect on the underlying flakiness).
…t_jobs llm_timeout no longer applies now that the HTTP client timeout race fix (internal/session/http_client.go) makes the context deadline the single timeout source — leaving it here was dead config that could confuse a reader into thinking it still did something. max_concurrent_jobs raised 8 -> 16 to give the higher webhook volumes in this example room to run without saturating the scheduler ahead of the per-queue concurrency limits in tannery.yaml.
get_pr_files/get_pr_diff (20s/30s) and post_pr_comment/add_pr_label (15s) were too tight once the pipeline was actually driving burst concurrency against a real backend — bumped all four to 60s to match the per-agent LLM call timeout, so a slow shell-mcp round trip under load fails via the same timeout budget as everything else instead of tripping first.
comments-in was pinned to concurrency: 1 (serial, ordered delivery) — under real burst load this made pr-comments the pipeline's bottleneck, queueing up behind every decision even though comment order across different PRs doesn't need to be preserved. Raised to 8 to match the other queues.
- Wait/drain reordering: the script waited for `decision` artifact count to reach WEBHOOK_COUNT, but decision isn't the pipeline's last stage — pr-comments is. The results/tally/done summary printed while pr-comments runs (and the server's own --pretty live trace, unredirected and sharing this terminal) were still active, making it look like output kept streaming after the script said it was done. Now waits on pr-comments artifact count (tracking decision count alongside it for visibility), and moved the queue-drain + server-shutdown step to run before the results/tally/done section instead of after. - .env sourcing for LLM endpoint/model overrides without exporting them in the shell first. - Shell robustness: `[ "$WEBHOOK_COUNT" -lt 25 ] && WEBHOOK_COUNT=25` style short-circuit clamping replaced with explicit if/then (the `&&` form exits non-zero under `set -e` when the condition is false, intermittently aborting the script); `read -r f || [ -n "$f" ]` guards against a last line without a trailing newline; `|| true` after find pipelines so a transient empty result doesn't trip `set -e`; capture() regex fixed to standard `(?<name>...)` syntax.
Adds a "Reliability & performance fixes" section covering all issues found and fixed while load-testing this pipeline against a real vLLM endpoint: the 8 framework-level and example-level bugs fixed across the preceding commits, and the measured 5.2x speedup (323s -> 62s) from disabling thinking mode and right-sizing completion_reserve.
TGPSKI
force-pushed
the
fix/high-volume-ci-load-test-fixes
branch
from
July 4, 2026 06:22
50bfb3d to
a22f660
Compare
…nal answer Load-testing examples/11-high-volume-ci surfaced another qwen3-under-load flakiness mode: the model sometimes emits the head of its final answer (e.g. the PR_NUMBER/REPO/SHA header) in the same completion as a tool call, then — seeing that text in the session as already said — continues from where it stopped after the tool result. The runner recorded only the last round's content as the answer, silently head-truncating the artifact (6/40 pr-metadata artifacts in a 40-webhook burst lost their identifier header, cascading into blank or literal-placeholder decision reports and garbage post_pr_comment calls downstream). Answer fragments emitted in tool-call rounds are now banked and spliced ahead of the final round's text, so the recorded turn, cache write, and output routing all see the full answer. Scoped to non-hide rounds — text next to hide pagination calls is navigation narration, not answer. The bare empty-stop self-healing retry is skipped when fragments are banked: an empty stop then just means the model had nothing left to add.
- TestRun_SplitAnswerAcrossToolRounds_IsReassembled: content emitted alongside a tool call is spliced ahead of the post-tool-result continuation in LastResponse. - TestRun_SplitAnswer_EmptyFinalStopSkipsBareRetry: when the fragments already carry the whole answer and the final completion is an empty stop, the banked fragment is the answer and the empty-stop retry does not burn an extra completion.
New optional per-tool "patterns" map in shell-tools.json: argument key -> RE2 regexp that the substituted value must match before the command runs. A missing argument validates as the empty string, so anchored patterns also reject absent values — catching a flaky model passing blanks or literal prompt-template placeholders like "<number>" instead of real values. Patterns are validated at startup (bad regexps fail fast) and advertised in the tool's inputSchema so schema-aware clients see the constraint too.
A 40-webhook burst produced 6 decision reports with blank or literal
placeholder ("<number>") PR identifiers when the pr-metadata artifact
lost its header (see the runner split-answer fix). Downstream, five
pr-comments runs dutifully posted comments to PR #<number> and one burned
its whole completion budget reasoning about the malformed input. Three
layers of defense:
- shell-tools.json: pattern-constrain pr_number (^[0-9]+$) and repo
(owner/name) on all four tools, so garbage identifiers now fail loudly
at the shell-mcp boundary instead of executing with placeholder text.
- decision.agent.md: on a missing/non-numeric pr-metadata block, emit
Decision: ERROR with blank identifier fields rather than guessing or
echoing template placeholders.
- pr-comments.agent.md: on Decision: ERROR or a blank/placeholder
PR_NUMBER, make no tool calls and report the skip — preventing both
the garbage posts and the completion-budget reasoning spiral.
Adds Unreleased entries that were missing for work already on this branch and its base: the four curing-worker concurrency fixes (#45), the HTTP client timeout race, the reasoning-model tool-call-loop flakiness fixes (duplicate-call guard, empty-stop retry), the split-answer head-truncation fix, the thinking: front-matter field, and shell-mcp per-argument pattern validation.
… groups Root cause of a 99/100 completion in a 100-webhook burst of examples/11-high-volume-ci: ids.TimestampHex drew its uniqueness suffix from mathrand.Int31n(0x10000) — 65,536 values per (prefix, minute) bucket, ~1% birthday-collision odds at ~40 IDs/minute. Two concurrent pr-context legs drew the same output hide ID (hide_pr_context_..._c461); hide.Store.Put's os.MkdirAll silently merged them; PR #1019's decision group collected the shared hide, succeeded, and deleted it; PR #1043's group then found its leg's hide missing 6 seconds later and DLQ'd after exhausting retries (the #45 requeueOrDLQGroup machinery surfaced the loss exactly as designed). With the opposite race order the failure would have been worse: a silently wrong decision built from another PR's analysis content. Two layers: - ids.TimestampHex suffix widened to 32 bits (%08x): collision odds at hundreds of IDs per bucket drop to ~1 in a million. - hide.Store.Put now creates the entry directory exclusively (os.Mkdir instead of MkdirAll) and regenerates the ID on collision, bounded at 5 attempts — a drawn collision can no longer overwrite another hide's content out from under its queue references. Note: the NamesContaining reference-check used by the prefix-scan GC path was considered for handleCollected's cleanup as well, but it matches hide IDs embedded in queue *names* (fan-out input hides) and cannot see leg-output hides referenced only by queue item contents — exclusive creation at the write point is the guard that actually closes this class.
…rror - TestStore_Put_RegeneratesIDOnCollision: a Put whose generated ID collides with an existing hide regenerates instead of reusing it, and the existing hide's content survives untouched. - TestStore_Put_PersistentCollisionErrors: a permanently colliding ID generator makes Put fail loudly after bounded attempts rather than overwrite or loop forever. Also tightens TestTimestampHexUniqueness: with a 32-bit suffix, any collision in 1000 draws now fails the test instead of tolerating up to 100.
scripts/profile-run.sh wraps any command with 1-2 Hz samplers: host CPU/memory/disk (vmstat, plus iostat/mpstat/pidstat when sysstat is installed), kernel pressure-stall information, per-process attribution, GPU telemetry via nvidia-smi, lm-sensors temperatures, and vLLM's /metrics endpoint (running/waiting requests, KV-cache usage, token throughput, prefix-cache hit rate). scripts/profile-summary.py renders an avg/peak summary from any capture directory; sysstat and GPU samplers degrade gracefully when absent. examples/11-high-volume-ci/README.md gains a 'Measured at scale' section with the profiled 100-webhook burst on a single RTX PRO 4500: ~500 LLM jobs, 972k tokens, 3.2k prompt tok/s sustained — orchestrated for ~6% of one host's CPU, zero PSI stalls, and 2% peak disk util. The pipeline is GPU-bound end to end; leather's orchestration overhead is effectively free at this scale.
…/100 run Replaces the interim numbers (from a run on the pre-ID-fix binary that finished 99/100) with the canonical post-fix run: 100/100 through every stage in 190s, 500 jobs / 965k tokens, 4.6k prompt tok/s sustained, host at 6.5% CPU with zero PSI stalls, GPU 79.7% avg.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A grab-bag of fixes found while load-testing
examples/11-high-volume-ciend-to-end against a real local vLLM endpoint (Qwen3,
--tool-call-parser qwen3_xml --reasoning-parser qwen3, concurrency 8+, bursts of 40-100webhooks). Split out from #45, which covers a closely related but distinct
set of fan-in/collect fixes in
internal/curing— this PR is based on topof that branch.
Fixes
HTTP client timeout races the context deadline
(
internal/session/http_client.go):http.Client{Timeout: ...}firedindependently of the run's context deadline, producing spurious "context
deadline exceeded" errors well before the real timeout. The context
deadline is now the single timeout source.
Reasoning-model flakiness in the tool-call loop
(
internal/runner/runner.go): confirmed via direct replay against theLLM endpoint outside leather that the model would occasionally re-issue
an already-succeeded tool call verbatim (looping until max tool rounds),
and separately would sometimes stop naturally with a completely empty
final answer (
finish_reason: "stop", not truncation). Not a request-construction bug — an explicit "don't do this" prompt instruction had
zero effect; ~25% of isolated replays reproduced the loop. Added a
duplicate-tool-call guard (an identical call that already succeeded this
run is not re-executed) and extended the existing length-truncation
self-healing retry to also cover the empty-stop case.
thinking:agent frontmatter field(
internal/agent/frontmatter.go,internal/agent/agent.go,internal/model/model.go,internal/runner/runner.go): new per-agentoverride (
Agent.DisableThinking) sendingchat_template_kwargs.enable_thinking=false. Disabling the hiddenreasoning trace turned out to be the most effective fix for both
flakiness modes above, and measured a 5.2x speedup on a 40-webhook
burst load test (323s -> 62s) once combined with right-sizing
completion_reserve. Zero value (false) leaves model default behavioruntouched everywhere else.
Answer text emitted alongside tool calls was dropped from the final
answer (
internal/runner/runner.go): a third member of the sameflakiness family, found in a follow-up 40-webhook burst — the model
sometimes emits the head of its final answer (e.g. the PR_NUMBER/REPO/SHA
header) in the same completion as a tool call, then continues from
where it stopped after seeing the tool result. The runner recorded only
the last round's content, silently head-truncating 6/40
pr-metadataartifacts and cascading into blank or literal-placeholder
(
PR_NUMBER: <number>) decision reports and garbagepost_pr_commentcalls. Fragments emitted in non-hide tool-call rounds are now banked and
spliced ahead of the final round's text, and the bare empty-stop retry is
skipped when banked fragments already carry the answer.
16-bit ID suffixes collide under burst load, cross-wiring fan-in
groups (
internal/ids/ids.go,internal/hide/store.go): root cause ofa 99/100 completion on a 100-webhook burst.
ids.TimestampHexdrew itsuniqueness suffix from
mathrand.Int31n(0x10000)— 65,536 values per(prefix, minute)bucket, ~1% birthday-collision odds at ~40 IDs/minute.Two concurrent
pr-contextlegs drew the same hide ID;Put'sos.MkdirAllsilently merged them; one PR's decision group collected theshared hide, succeeded, and deleted it; the other PR's group found its
leg's hide missing 6 seconds later and DLQ'd after retries (the fix: curing collect fan-in hangs forever and leaks state when a leg permanently DLQs #45
requeueOrDLQGroupmachinery surfaced the loss exactly as designed —with the opposite race order this would have been a silently wrong
decision built from another PR's analysis). Suffix widened to 32 bits,
and
hide.Store.Putnow creates the entry directory exclusively(
os.Mkdir), regenerating the ID on collision instead of overwriting anexisting hide.
shell-mcp per-argument
patternsvalidation (cmd/shell-mcp/main.go):new optional per-tool
patternsmap inshell-tools.json(argument key →RE2 regexp), validated before the command runs and advertised in the
tool's
inputSchema. A missing argument validates as the empty string, soanchored patterns also reject absent values — a flaky model passing blanks
or literal prompt-template placeholders like
<number>now fails loudlyat the MCP boundary instead of executing with garbage.
examples/11-high-volume-cifixes and tuning:shell-tools.jsonpattern-constrainspr_number/repoon all four GitHub tools;decision.agent.mdemits
Decision: ERRORinstead of guessing or echoing templateplaceholders when the
pr-metadatablock lacks a numeric PR_NUMBER;pr-comments.agent.mdmakes no tool calls on a malformed report andreports the skip.
decision.agent.md: fixed a positional fan-in bug — the promptassumed the 3 parallel analysis agents always arrive in submission
order ("copy from ANALYSIS 1"), but
pr-context(zero tool calls)often won that race, leaving nothing to copy and producing
PR_NUMBER: N/A. Now references the block by its(from: pr-metadata)tag instead of position.thinking: false+completion_reserve: 768(downfrom the reasoning-model default of 8192).
config.yaml/shell-tools.json/tannery.yaml: removed stalellm_timeout, raisedmax_concurrent_jobs, tool timeouts, andcomments-inconcurrency to match real burst-load latency.run-demo.sh: fixed the completion-wait to trackpr-comments(theactual last stage) instead of
decision, and moved the queue-drain +server-shutdown step before the results summary — previously the
summary printed while the server's own
--prettylive trace(unredirected, sharing the terminal) was still streaming, making it
look like output kept coming after the script said it was done.
performance table.
Full-system load-test profiler (
scripts/profile-run.sh,scripts/profile-summary.py): wraps any command with samplers for hostCPU/mem/disk, PSI pressure, per-process attribution, GPU telemetry, and
vLLM
/metrics, plus an avg/peak summarizer. Used to produce the100-webhook profile now documented in the example README ("Measured at
scale"): 100/100 through every stage in 190s — 500 LLM jobs / 965k tokens
orchestrated for 6.5% of one host's CPU, zero PSI stalls, GPU-bound end
to end at 4.6k prompt tok/s sustained.
CHANGELOG: backfilled Unreleased entries for everything above plus the
four curing-worker concurrency fixes that shipped via fix: curing collect fan-in hangs forever and leaks state when a leg permanently DLQs #45.
Test plan
go build ./...go vet ./...go test ./...TestRunner_DuplicateToolCallSkipsReExecution,TestRun_SelfHealingRetry_OnEmptyStopCompletion,TestRun_EmptyStopRetry_IsSingleShot,TestRun_DisableThinkingMergesChatTemplateKwargs,TestParseFrontMatter_ThinkingFalse,TestParseFrontMatter_ThinkingDefaultsToEnabled,TestRun_SplitAnswerAcrossToolRounds_IsReassembled,TestRun_SplitAnswer_EmptyFinalStopSkipsBareRetry,TestExecutePatternValidation,TestToolListAdvertisesPatterns,TestExampleShellToolsJSON_PatternsCompileAndAreDeclared,TestStore_Put_RegeneratesIDOnCollision,TestStore_Put_PersistentCollisionErrorstest, 0 max-tool-rounds failures, 0 DLQ events, 0 errors (vs.
intermittent failures on the pre-fix baseline), 323s -> 62s; a
follow-up burst surfaced the split-answer head-truncation (6/40
malformed reports), root-caused from
.state/runs/*.jsonltokenaccounting and fixed above