Repository navigation
[3372 by Claude Opus 5.5] 0.9.0 release logic fix: an interrupted agent image build/pull leaves the Redis build slot to a dead holder, and the next start waits up to 45 minutes silently with no agents and no indexing. Check current upstream main first. - #3373
integry merged 2 commits into
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…w reclaimed, a stopping worker releases its own slot, and waiting is no longer silent. Upstream `main` was the same commit as this branch (`ba1e2724d`), so nothing there already covered it. All the checks the issue lists pass, but no test ran against a real Redis or a real `propr stop`. The tests use a fake Redis client and a fake owner probe. What changed 1. Holder identity and reclaim (`packages/core/src/agents/agentImageBuildLock.ts`): the slot now stores the build's token together with its owner (from `currentClaimOwner()`). When `SET NX` fails, the waiter reads the holder. If `claimOwnerGone` proves the holder's container or process is gone, the waiter deletes the slot only if it still holds that exact value, then tries again. It re-checks the same holder every 15 s rather than every 250 ms poll. A live holder is never touched, and the 45-minute lease still applies as a last resort. 2. Graceful shutdown: the worker's `close()` now calls the new `abortAgentImageBuildsForShutdown()` first, before it waits for jobs to finish. That stops any wait for the slot and aborts an in-flight build or pull, because the `docker build`/`pull` commands now get an abort signal. It then releases any slot this process holds, giving the build up to 5 s to stop first. Any later attempt to take the slot in that process is refused. 3. No more silent waits: a warning is logged 10 s into a wait and every 60 s after, naming the holder and the remaining lease. Reclaiming a slot is also logged. When the API finds the image not ready, its status now ends with `…has not been prepared by the worker; waiting for build slot held by <host> pid <n>, lease expires in 37m`. That text reaches `/status` warnings, agent health checks and the UI. The lookup uses a short-lived Redis connection that never retries and is cached for 5 s. With Redis down it returned nothing in 8 ms and the process exited cleanly. 4. Tests: - `test/agentImageBuildLock.test.ts` covers: the owner is recorded; a gone container and a gone pid are both reclaimed; a live holder waits with repeated warnings; three racing reclaimers delete the dead holder once and never overlap; and shutdown aborts the build, releases the slot and refuses new builds. - The new `test/agentImageSlotStatusHint.test.ts` (added to `test:unit`) covers the status text. 5. Changelog: added to the existing 0.9.0 Fixed bullet "Agent image recovery". Checks run - `npm run lint --workspaces --if-present -- --max-warnings 0`: clean. - Root lint and `tsc --noEmit`, plus core lint and typecheck: clean. - The lock, preparation, registry, runtime-package and API health/status test files: 191 + 35 passed. - `npm run test:unit`: 746 passed. - `EXPECTED_VERSION=0.9.0 RELEASE_TAG=v0.9.0 RELEASE_CANDIDATE=true npm run release:verify`: passed. Things to know - The install in the report won't be unstuck by this. Its slot was written by the unpatched 0.9.0, so it has no owner recorded. A slot like that can't be proven dead, so it is still only freed when its lease expires. It does now show up in logs and status as "an unidentified holder (token …)". That install needs to wait out the lease or have `propr:agent-image-build:slot` deleted by hand. - `src/worker.ts` was at the 400-line lint limit. To stay under it, I put the import on one line with `closeAgentImageBuildLock` and put `agentImageWatchdog.close(); await abortAgentImageBuildsForShutdown();` on a single line. - This changes status text only, not the UI, so I made no visual preview.
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR adds owner-aware slot recovery, shutdown cancellation, and visible wait diagnostics. It needs a localized correction before merge: moving callback invocation outside the cleanup block introduces a lock leak on synchronous exceptions. ✅ Ownership-safe reclamation — Reclamation compares the exact stored value before deleting it, protecting a replacement holder from stale cleanup. ✅ Bounded status diagnostics — Status inspection uses a timeout, disables reconnect retries, and caches results without acquiring the slot. ✅ Targeted recovery coverage — Added tests cover dead owners, live holders, legacy tokens, concurrent reclaimers, and cooperative shutdown. The supplied current-head checks report no failures. This review used static analysis only, as requested. The issue body is explicitly truncated, so assessment of its requirements is limited to the visible objective. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Keep callback invocation inside cleanup
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Exercise real shutdown and takeoverThe new shutdown test uses a callback that rejects immediately on abort. A follow-up integration test with Redis and an actual Docker operation would strengthen confidence in the five-second fallback release, especially when cancellation settles slowly and another worker is waiting. This is optional validation; the supplied excerpts do not establish that an actual Docker build remains running after that fallback. S2: 🟢 Clarify legacy recovery limitsLegacy bare-token holders are deliberately preserved until expiry, as the new test verifies. Qualifying the changelog would help operators understand that an already stranded slot from an older release can still require waiting. Preserving an unidentified holder is conservative and does not itself constitute a new regression. ScoreThe implementation is focused and current checks pass, but the newly introduced cleanup regression needs correction before merge. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
/fix F1 S2 |
|
✅ Applied the requested follow-up changes in commit 180a280
Summary of ChangesI fixed F1 and S2. All checks pass, including F1: a callback that throws immediately no longer leaves the slot held (
S2: changelog (
Other checks: core lint (zero warnings) plus core and root I found nothing else outside these two items. There's no visual change, so I made no preview. 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…:unit` (747 passed, 0 failed). I fixed F1 and S2. All checks pass, including `npm run test:unit` (747 passed, 0 failed). **F1: a callback that throws immediately no longer leaves the slot held** (`packages/core/src/agents/agentImageBuildLock.ts`) - `withAgentImageBuildSlot` now calls `work` inside the `try` block, through `Promise.resolve().then(...)`. An immediate throw becomes a rejected promise, and the `finally` cleanup handles it. - `finally` stops the renewal timer and releases the Redis slot on every exit after the slot is taken. It only removes the slot from the shutdown tracking set if it was added, so a failure before that point can't break cleanup. - Other callers: `runAgentImageBuild`, `agentRuntimeImageBuilder.ts` and `dockerImageBuilder.ts` all pass `async` callbacks, so they always returned a promise and weren't affected. The shutdown function `abortAgentImageBuildsForShutdown` only sees slots once they're tracked, which is the same as before. - New test in `test/agentImageBuildLock.test.ts`: a callback that throws `validation failed` immediately. It checks that the call rejects, the slot is released straight away, no renewal fires after one renewal interval, and a second build can then take the slot. I also ran this test against the previous version of the file, and it failed there as expected. **S2: changelog** (`CHANGELOG.md`, the "Agent image recovery" entry) - It now says that a slot taken by an earlier release records no holder. Such a slot can't be proven abandoned, so it is only freed when its lease expires, up to 45 minutes. Status shows it as "an unidentified holder". - It also gives the manual fix: stop ProPR and delete the `propr:agent-image-build:slot` Redis key. **Other checks:** core lint (zero warnings) plus core and root `tsc --noEmit` all pass, as do the 12 lock tests. These tests still use the fake Redis client, not a real Redis. I found nothing else outside these two items. There's no visual change, so I made no preview. PR: #3373 Comment by: @integry (ID: 6491802212) Model: Claude Opus 5.5
|
/review |
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR adds owner-aware slot recovery, shutdown cancellation, and visible waiting diagnostics. The supplied code supports merging within the visible scope, conditional on pending checks completing successfully. ✅ Safe conditional reclamation — Recovery requires evidence that the recorded owner is gone and compares the exact stored value before deletion, protecting a replacement holder. ✅ Cleanup on callback failure — Deferred callback invocation keeps synchronous exceptions inside the cleanup path; the added regression test checks immediate release and stopped renewal. ✅ Bounded status diagnostics — Status inspection uses a timeout, disables reconnection retries, disconnects its Redis client, and caches results. This was a static review; no commands or tests were run. The supplied issue body explicitly truncates its remaining requirements, so unseen acceptance criteria could not be assessed. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Exercise actual shutdown teardownAdd an integration test using real Redis and Docker that interrupts preparation and verifies prompt reacquisition after restart. The unit tests establish cancellation and release with cooperative callbacks, but do not establish how actual Docker teardown interacts with the five-second forced-release deadline in S4: 🟢 Cover shutdown acquisition interleavingConsider a regression test that pauses the Redis S5: 🟢 Broaden status hint coverageRuntime-package image resolution still reports a plain unavailable-image error when the base image exists but its runtime image needs preparation. Extending the holder hint to that path would make diagnostics more consistent. This is an optional extension beyond the reproduced missing-bundle path. ScoreThe implementation provides conservative recovery and useful focused tests, with no verified blocker in the supplied code. Current checks show no failures, with nine still pending; real stop/restart behavior remains untested in this review. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
AI Implementation Summary
Closes #3372
Branch:
3372/claude-opus-5-5-0-9-0-release-logic-fix-a-20261029-0736-r0wCommits: ✅ Changes committed (f56797c)
AI Processing Completed
Execution Details:
Summary:
I've fixed the issue: a build slot left behind by a dead worker is now reclaimed, a stopping worker releases its own slot, and waiting is no longer silent. Upstream
mainwas the same commit as this branch (ba1e2724d), so nothing there already covered it. All the checks the issue lists pass, but no test ran against a real Redis or a realpropr stop. The tests use a fake Redis client and a fake owner probe.What changed
packages/core/src/agents/agentImageBuildLock.ts): the slot now stores the build's token together with its owner (fromcurrentClaimOwner()). WhenSET NXfails, the waiter reads the holder. IfclaimOwnerGoneproves the holder's container or process is gone, the waiter deletes the slot only if it still holds that exact value, then tries again. It re-checks the same holder every 15 s rather than every 250 ms poll. A live holder is never touched, and the 45-minute lease still applies as a last resort.close()now calls the newabortAgentImageBuildsForShutdown()first, before it waits for jobs to finish. That stops any wait for the slot and aborts an in-flight build or pull, because thedocker build/pullcommands now get an abort signal. It then releases any slot this process holds, giving the build up to 5 s to stop first. Any later attempt to take the slot in that process is refused.…has not been prepared by the worker; waiting for build slot held by <host> pid <n>, lease expires in 37m. That text reaches/statuswarnings, agent health checks and the UI. The lookup uses a short-lived Redis connection that never retries and is cached for 5 s. With Redis down it returned nothing in 8 ms and the process exited cleanly.test/agentImageBuildLock.test.tscovers: the owner is recorded; a gone container and a gone pid are both reclaimed; a live holder waits with repeated warnings; three racing reclaimers delete the dead holder once and never overlap; and shutdown aborts the build, releases the slot and refuses new builds.test/agentImageSlotStatusHint.test.ts(added totest:unit) covers the status text.Checks run
npm run lint --workspaces --if-present -- --max-warnings 0: clean.tsc --noEmit, plus core lint and typecheck: clean.npm run test:unit: 746 passed.EXPECTED_VERSION=0.9.0 RELEASE_TAG=v0.9.0 RELEASE_CANDIDATE=true npm run release:verify: passed.Things to know
propr:agent-image-build:slotdeleted by hand.src/worker.tswas at the 400-line lint limit. To stay under it, I put the import on one line withcloseAgentImageBuildLockand putagentImageWatchdog.close(); await abortAgentImageBuildsForShutdown();on a single line.Detailed Logs:
dd90a571-8229-4a82-9f91-a61dcfe29d4bLog files stored at:
/usr/src/app/logs/executions/issue-3372-2026-10-29T07-53-48-035Z-conversation.json/usr/src/app/logs/executions/issue-3372-2026-10-29T07-53-48-035Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3372.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!