fix: refuse an unplaceable hunk, and resize a pool without stopping it - #139
Merged
Merged
Conversation
added 2 commits
August 3, 2026 23:15
`@@ -0,0 +...` says the old file has no lines at this point. Against a file that exists with content in it that is false about the tree, and the second rung of the ladder acted on it anyway: `git apply` refuses the header, `--unidiff-zero` takes the absent context at face value and inserts the lines at line 1. Live, that put `def multiply` above `calc.py`'s module docstring, which then stopped being a docstring at all -- `calc.__doc__` was None -- and put two tests above `from calc import add, subtract`. `pytest -q` passed throughout, because Python does not care where a string literal sits, so the item reached the reviewer as a success carrying damage the harness introduced rather than the model. `apply_diff`'s own docstring already had the principle: fuzzy matching is off by default "because a misplaced hunk that reports success is worse than a failure". This is the same trade one rung higher up, and it is now refused before anything runs rather than after the ladder, because the patch is checkable against the tree without applying it. The rescue the ladder exists for is untouched. A hunk header that understates its context still names a position, and `--unidiff-zero` still forgives it -- the existing tests for that rung pass unchanged. What is refused is the one header that names no position at all. It costs one honest case, measured: `git diff -U0` emits exactly this header for a line prepended to a file that has content. Nothing in the patch tells that apart from a model that meant "somewhere in this file" and wrote the emptiest header it knew, and one of the two is damage no gate can see. Both are refused, the failure says what a usable header would look like, and a prepend carrying one line of context is applied by the first rung as before.
`max_workers` was persisted per project and then ignored: a pool kept the thread count it was started with until the project was stopped and started again. So buying capacity for a busy project cost a drain/restart cycle -- tearing down agents that are mid-item, to apply an integer -- and reducing it could not be done at all without coordinating around in-flight work. The obstacle was one stop event per pool. With a single switch the only answers available were "keep every worker" and "stop the project", so each worker now carries its own, and the pool-wide halt sets all of them. `Fleet.resize` computes the delta from the workers that are not already leaving: - growing starts only the shortfall; - shrinking sets the excess workers' switches and joins them off the caller's thread, so an in-flight item finishes at its own boundary and nothing is killed -- the same reason `stop` drains rather than interrupts; - the counts stay truthful: `running()` reports threads that are alive, which after a shrink is still the old number until those items land, and the project stays `running` throughout; - concurrent resizes serialise on the fleet lock and are idempotent, so two callers asking for three workers get three, not six; - a worker the OS refuses to start is recorded as a worker failure and leaves its healthy siblings working, rather than failing the whole call. A draining or stopped pool is not resized. Adding a thread underneath a drain outlives the stop that was supposed to have finished, because the finalizer joining the pool was handed the old list. Registering a project update reconciles its live pool, which is the surface operators already use for `max_workers`; on a stopped project it changes nothing until the next start, as registering anything does.
thedancingdeveloper
force-pushed
the
fix/apply-safety-and-pool-resize
branch
from
August 3, 2026 23:23
f0a4c63 to
0ee17c6
Compare
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
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.
Two defects, both about a gate telling the truth.
A hunk with no placement is refused before the ladder runs
@@ -0,0 +...says the old file has no lines at this point. Against a filethat exists with content in it, that is false about the tree — and the second
rung acted on it anyway:
git applyrefuses the header,--unidiff-zerotakes the absent context at face value and inserts at line 1.
Live, that put
def multiplyabovecalc.py's module docstring, which thenstopped being a docstring at all, and put two tests above their imports.
pytest -qpassed throughout, because Python does not care where a stringliteral sits, so the item reached the reviewer as a success carrying damage
the harness introduced rather than the model.
apply_diff's docstring already carried the principle — fuzzy is off bydefault "because a misplaced hunk that reports success is worse than a
failure". This is the same trade one rung higher, refused before anything
runs because the patch is checkable against the tree without applying it.
The rescue the ladder exists for is untouched. A header that understates
its context still names a position and
--unidiff-zerostill forgives it;test_a_hand_written_hunk_header_is_rescuedand its strict-apply guard passunchanged. What is refused is the one header that names no position at all.
It costs one honest case, measured:
git diff -U0emits exactly this headerfor a line prepended to a non-empty file. Nothing in the patch tells that
apart from a model that meant "somewhere in this file", and one of the two is
damage no gate can see — so both are refused, the failure says what a usable
header looks like, and a prepend with one line of context applies as before.
The two tests from #134 that used a
-0,0diff as their fixture now use acontext-free hunk that names a real position — still rescued by
--unidiff-zero, still proposed-text ≠ applied-change, which is what theywere about.
A running pool can be resized
max_workerswas persisted and then ignored: a pool kept its original threadcount until the project was stopped and started again. The obstacle was one
stop event per pool — the only answers available were "keep every worker" and
"stop the project" — so each worker now carries its own switch.
Fleet.resizecomputes the delta from the workers not already leaving:growing starts only the shortfall; shrinking sets the excess workers'
switches and joins them off the caller's thread, so an in-flight item
finishes at its own boundary and nothing is killed.
running()keepsreporting threads that are alive, which after a shrink is still the old
number until those items land, and the project stays
runningthroughout.Concurrent resizes serialise on the fleet lock and are idempotent; a worker
the OS refuses to start is recorded as a worker failure and leaves its
siblings working. A draining or stopped pool is not resized.
Registering a project update reconciles its live pool — the surface operators
already use for
max_workers— anddocs/USAGE.mdsays so.Verification
Every new test was run against the unfixed code first and fails there:
-0,0tests fail withapplied == True, how == "git apply --unidiff-zero"— the bug, reproduced;resizereduced to "return the current size" (the old behaviour), thefour fleet tests and the API integration test fail.
Threading tests use events and predicates, no bare sleeps: a
HoldingExecutorholds an item until released, so the shrink demonstrably lands mid-item.
uv run pytest -q,ruff check .,ruff format --check .andmypy .allpass.
Closes #133. Closes #102.