fix(runtime): a reclaim that loses to a concurrent remover confirms absence instead of failing - #1579
fix(runtime): a reclaim that loses to a concurrent remover confirms absence instead of failing#1579jfw-ppi wants to merge 3 commits into
Conversation
|
👋 Thanks for opening your first PR to Vexa, @jfw-ppi! Highly recommended (not required): hop into our Discord Your PR is judged on its evidence — the observation bundle + the diff — not on whether you |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06c8a70b84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| while time.monotonic() < deadline: | ||
| if self._req("GET", f"/containers/{h._impl}/json").status_code == 404: # type: ignore[attr-defined] | ||
| return | ||
| time.sleep(_RECLAIM_POLL_SEC) |
There was a problem hiding this comment.
If the concurrent removal finishes after the last 200 ms poll but before the 10-second deadline, this loop sleeps past deadline and raises without issuing another GET. For example, a 200 at 9.8 s followed by removal at 9.9 s produces a false failed destroy even though the container disappeared within the advertised confirmation window. Perform a final inspection after the loop (or cap the final sleep and poll at the deadline) before declaring the reclaim failed.
Useful? React with 👍 / 👎.
| if r.status_code == 409: | ||
| deadline = time.monotonic() + _RECLAIM_CONFIRM_SEC | ||
| while time.monotonic() < deadline: | ||
| if self._req("GET", f"/containers/{h._impl}/json").status_code == 404: # type: ignore[attr-defined] |
There was a problem hiding this comment.
Each poll uses _req's default 30-second timeout, so if Docker responds to the initial 409 but then stalls on inspect, the first GET can block for 30 seconds before the loop can observe its nominal 10-second deadline. This makes the documented bounded reclaim wait substantially exceed _RECLAIM_CONFIRM_SEC (and may surface a transport timeout instead of the intended reclaim failure); pass the remaining deadline duration as the inspect timeout.
Useful? React with 👍 / 👎.
06c8a70 to
4f23181
Compare
🃏 Merge card — #1579
Not mergeable yet — every row above must be accepted before merge (choke point 1). Fill in what's ❌ above, then this clears automatically. How a PR reaches merge: the merge bar. |
4f23181 to
735484f
Compare
…bsence instead of failing The docker backend's cleanup accepted only 204/404 from DELETE ?force=true, so the daemon's 409 "removal of container ... is already in progress" - another remover finishing the same reclaim (an operator's rm -f, a GC, another deployment sharing the daemon) - made destroy raise although the container was already going away. Truthfulness needs the container GONE, not our DELETE to be the one that won: on 409 the reclaim is now judged by confirmed absence (inspect until 404, bounded by RUNTIME_RECLAIM_CONFIRM_SEC, default 10s, clamped to 1..120s so no bad value can hold a destroy slot forever), and a container still present at the deadline fails exactly like any other unconfirmed reclaim - a destroy that would lie over a live bot still cannot. The poll keys on the status code alone, never the message; each inspect carries its own short timeout so one stalled request cannot stretch the bound; a transient inspect failure keeps polling instead of aborting the window, and a window in which NO inspect succeeded says "absence could not be confirmed", never "still present". Both ends of the race leave an operator breadcrumb. Proven both ways: scripted-daemon tests stage the concurrent remover deterministically (loser-confirms-absence with the poll timeout pinned, status-not-message, blip tolerance, daemon-gone wording, never-leaves raises after provably polling, the 204/404/other-error paths unchanged), and test_docker_reclaim_races_a_real_concurrent_remover stages the same race against the real daemon - a fat writable layer holds the removal window open while a competing rm -f gets a head start, so cleanup's DELETE reliably draws the 409; a run where the race does not overlap skips visibly instead of passing vacuously. Closes Vexa-ai#1384 Signed-off-by: Jacob Weinhold <29459386+jfw-ppi@users.noreply.github.com>
735484f to
b0a7eb2
Compare
Reviewer finding on the confirm loop: a removal completing between the last 200ms poll and the deadline was misreported as a failed reclaim - the loop slept past the deadline and raised without another look. The loop now inspects FIRST and checks the deadline AFTER, so it can never raise without a terminal inspect; even a zero-width window gets one, and a container that vanishes at the window's edge is confirmed instead of failed. Also scoped in: only a verdict-bearing inspect answer (200/404) counts as having observed the container, so a window of nothing but inspect-500s ends as "absence could not be confirmed" instead of claiming the container is still present. The sibling finding (polls inheriting _req's default 30s timeout) was already addressed: every inspect carries its own 2s timeout, and the failure message reports the measured elapsed time. Signed-off-by: Jacob Weinhold <29459386+jfw-ppi@users.noreply.github.com>
Signed-off-by: Jacob Weinhold <29459386+jfw-ppi@users.noreply.github.com>
1 participant
Delivers issue: #1384
Contribution rights
under Apache-2.0, and it is not owned or controlled by an employer, client, or other entity.
The commit carries my DCO
Signed-off-by.What & why
DockerBackend.cleanup()accepted only 204/404 fromDELETE ?force=true, so the daemon's 409"removal of container … is already in progress"— another remover finishing the same reclaim (an operator'sdocker rm -f, a GC, another deployment sharing the daemon) — madedestroy()raise although the container was already going away (#1384 carries the full evidence chain).The invariant the code states is kept, not weakened: truthfulness needs the container gone, not our DELETE to be the one that won. On 409 the reclaim is judged by confirmed absence — inspect until a literal 404, bounded by
RUNTIME_RECLAIM_CONFIRM_SEC(default 10 s, floored at 1, mirroringRUNTIME_STOP_GRACE_SEC; declared in the runtime's config contract). The branch keys on the status code alone (no message parsing); each inspect carries a short timeout so one stalled request cannot stretch the bound; a transient inspect failure keeps polling instead of aborting the window; both ends of the race leave an operator breadcrumb (warningon entry,infowith elapsed time on the confirming 404). A container still present at the deadline raises exactly like any other unconfirmed reclaim.Observation bundle
uv run pytest tests/test_docker_reclaim.py -q· saw: 6 passed — loser-confirms-absence (exact request sequence pinned), status-code-not-message (bare-body 409 takes the same path), never-leaves raises"still present"and provably polled, daemon-blip mid-confirm keeps polling to the 404, 204/404 fast paths and other-error raises unchanged · concluded: the contract holds without a daemon, in any CI; a message-parsing or no-poll mutant dies.tests/test_docker_backend.py::test_docker_reclaim_races_a_real_concurrent_remover(new; docker-gated like its siblings) 7× against a real daemon (docker 29.7.2) · saw: 7/7 passed, ~2–7 s each · concluded: the race is repeatable — a fat writable layer holds the removal window open, a competingrm -fgets a 0.5 s head start, cleanup must succeed with the container provably gone.maineach of these runs raises.uv run pytest -q -k "not real_pod"· saw: 184 passed (incl. both real-docker tests) · concluded: no adjacent regression. (test_k8s_backend_real_pod_lifecycleis a cold-cluster environment flake, untouched by this diff.)gate:config-contractafter declaringRUNTIME_RECLAIM_CONFIRM_SEC(the gate caught the undeclared read — the system working).Acceptance floor
test_cleanup_409_confirms_the_concurrent_removers_reclaim— exact request sequence assertedtest_cleanup_409_keys_on_the_status_code_not_the_message— bare-body 409test_cleanup_409_with_a_container_that_never_leaves_raisestest_cleanup_409_keeps_polling_through_a_daemon_bliptest_cleanup_returns_on_204_and_404,test_cleanup_other_errors_still_raisetest_docker_reclaim_races_a_real_concurrent_remover+ the C3 request logDocs diff (D6c)
docs/changelog.d/1579-docker-reclaim-409.md— the operator-visible change in the fragment convention. The new env knob is declared incore/runtime/src/runtime_kernel/config.v1.jsonbeside its sibling grace knob.Security checks
No new dependencies; no lockfile change. The 409 path adds only read-only inspects against the same daemon socket; failure modes stay fail-loud; the destroy route notes it may now hold a threadpool slot for a few seconds (still never the event loop).
Validation request
Any maintainer with docker:
uv run pytest tests/test_docker_backend.py -q— the new staged-race test is red onmain(409 → RuntimeError) and green here, no manual setup needed. Deployment validated: none (backend-internal); the live evidence above ran against a real daemon. Lite / compose / k8s / hosted honestly unclaimed.Authorship
Sole author: the human submitting this. No agent co-author trailers (D13).
Tooling disclosure: investigation, fix, and verification assisted by Claude Code; the request logs are from live runs against a real daemon.