Reclaim stale runtime locks after Docker container restarts - #1363
Open
eutialia wants to merge 5 commits into
Open
Reclaim stale runtime locks after Docker container restarts#1363eutialia wants to merge 5 commits into
eutialia wants to merge 5 commits into
Conversation
…tarts After a host reboot or OOM kill Docker restarts the openalice container in place and Guardian crash-looped with RuntimeAlreadyRunningError until the lock was removed by hand. Four independent defects, all in how the recorded owner is compared with the current process and machine: - isSameProcess asked `ps -p <pid> -o lstart=` for the start-time identity check, but the slim server image ships no ps, so the lookup threw, returned null, and the check failed open. Process start time is now read from procfs on Linux (/proc/<pid>/stat field 22 after the last ')', /proc/stat btime, USER_HZ 100), falling back to ps/PowerShell elsewhere. compareProcessIdentity resolves our own pid without any external lookup, so an owner wearing this process's pid is definitively different, and an unverifiable same-machine owner with a stale heartbeat is treated as stale instead of failing open. - `currentProcessStartedAt()` derived the value written into the lock from `Date.now() - process.uptime()`, which starts counting at Node's `main()` rather than at `exec`, so it ran a systematic ~850ms behind the procfs value that every later verification reads back through `ProcessController.startedAt()`. That gap is spent out of the 2s pid-reuse tolerance before pid reuse is considered, and it is widest exactly where the new procfs lookup made the comparison reachable for the first time: slim containers with no `ps`, where a slow cold start stretches exec-to-main. A live owner whose recorded start drifts past the tolerance inspects as pid-reused, and its lock is reclaimed underneath it. The kernel start time is now read directly for our own pid where procfs exists; the uptime estimate stays as the fallback for the other platforms. - The node:22-trixie-slim runtime has no /etc/machine-id, so readMachineId() fell back to hostname:<container-id>, which changes on every recreate. The image now pins OPENALICE_MACHINE_ID=openalice-docker so the identity is stable across recreates; OPENALICE_TAKEOVER=1 remains for operators who need to force a takeover. - Pinning only prevents future records. A runtime.lock written by an older image carries machineId hostname:<old container id>, so every upgraded container reported a foreign owner forever. The cross-machine refusal now applies only when both sides carry strong ids that differ; a mismatch involving a hostname-scheme id falls through to the ordinary same-machine liveness checks, so duplicate detection on one host is unchanged.
`recoverRuntimeOwner` threw on any cross-machine owner before considering anything else, so `OPENALICE_TAKEOVER=1` could not clear a lock recorded against a different machine id. A container started against a volume carrying such a record crash-looped on "belongs to another machine; refusing to signal it" with no way out, while the docs offered takeover as the escape hatch. Signalling a cross-machine owner stays forbidden: its pid addresses a process table that is not ours. Under takeover its record is now claimed instead, but only once its heartbeat has aged out. A cross-machine owner that is still heartbeating keeps refusing takeover, and without takeover a stale one still surfaces as RuntimeAlreadyRunningError. `recoverRuntimeOwner` now reports whether it signalled anything, so `acquireRuntimeLock` can reclaim the record itself when no owner will release it. That also covers the owner that dies between inspection and recovery. It takes the inspection it acts on rather than the bare owner, and refuses a live cross-machine owner with CrossMachineOwnerError, a RuntimeAlreadyRunningError carrying that inspection, so launchers handle the refusal like any other occupied lock. The message text is unchanged.
acquireRuntimeLock now fires onOwnerReclaimed with the inspection that justified removing a stale, invalid, or stale cross-machine owner, and the dev and desktop Guardians log that reclaim instead of dropping it silently.
… sources Each packages/uta-broker-<engine>/src/index.ts imports its broker implementation from services/uta/src via a relative path, and tsup's noExternal bundles that source directly into dist/index.js. Turbo only tracks package.json dependency edges by default, so it can't see this relative-path import and reports a cache hit after editing UTA broker source, restoring a stale dist/index.js into the broker pack. Add a per-package turbo.json build override for each uta-broker-* package that extends $TURBO_DEFAULT$ with services/uta/src/**, so the cache key changes whenever the bundled sources change.
|
@eutialia is attempting to deploy a commit to the luokerenx4's Team Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
🟡 Changes recommended
One updated error-path still emits a misleading default message for cross-machine locks (it drops the inspection reason), which undermines the PR’s stated diagnostic behavior and makes the failure mode harder to interpret.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens Guardian/OpenAlice runtime lock ownership recovery so stale runtime.lock/guardian.lock records can be safely reclaimed after Docker container restarts and other unclean shutdowns, and fixes Turbo caching for broker pack builds that bundle out-of-package UTA sources.
Changes:
- Improve process identity checks (Linux
/proc/<pid>/statparsing, same-clock start time capture) and machine-id handling (weak hostname ids, typed cross-machine refusal, takeover behavior). - Add
onOwnerReclaimedcallback so launchers can log when a stale lock record is claimed. - Fix Turbo cache invalidation for
uta-broker-*builds by declaringservices/uta/src/**as an explicit build input.
File summaries
| File | Description |
|---|---|
| turbo.json | Adds per-broker-pack Turbo inputs overrides so bundled services/uta/src/** invalidates cache correctly. |
| src/main.ts | Logs reclaimed lock inspections via onOwnerReclaimed during runtime lock acquisition. |
| scripts/guardian/prod.mjs | Logs reclaimed lock inspections in the production Guardian launcher. |
| scripts/guardian/dev.ts | Logs reclaimed lock inspections in the dev Guardian launcher. |
| apps/desktop/src/main.ts | Logs reclaimed lock inspections in the Electron Guardian launcher. |
| packages/guardian-runtime/src/runtime-lock.ts | Implements cross-machine/heartbeat-aware lock inspection & recovery; adds onOwnerReclaimed hook and typed cross-machine error. |
| packages/guardian-runtime/src/runtime-lock.spec.ts | Adds coverage for reclaim reporting and cross-machine/takeover semantics. |
| packages/guardian-runtime/src/process-control.ts | Adds procfs-based Linux process start-time reading and process-identity comparison helpers. |
| packages/guardian-runtime/src/process-control.spec.ts | Adds tests for procfs parsing and identity comparison behavior. |
| Dockerfile | Pins OPENALICE_MACHINE_ID=openalice-docker in the server image to stabilize identity across container recreates. |
| docs/docker-deployment.md | Documents the Docker machine-id pin and takeover behavior for cross-machine locks. |
| docs/broker-packs.md | Documents why Turbo inputs must include out-of-package bundled sources for broker packs. |
Review details
- Files reviewed: 12/12 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…or message The default RuntimeAlreadyRunningError message dropped inspection.reason whenever an owner was recorded, so a cross-machine refusal logged as a plain "already running" and the foreign-volume case could not be told apart from a live local owner without reading the lock files.
eutialia
force-pushed
the
feat/docker-deployment-reliability
branch
from
September 5, 2026 03:47
fafa697 to
52a5558
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.
Summary
docker compose up -drecreate, Guardian in the server image crash-looped withRuntimeAlreadyRunningErroruntil someone deletedruntime.lockandguardian.lockby hand. The recorded owner was compared with the wrong tools: the slim image has nops, so the pid start-time check failed open; the image has no/etc/machine-id, so the machine id fell back to the container hostname, which changes on every recreate and made every old lock look like another machine's; and the value written into the lock came fromprocess.uptime(), about 850 ms behind the procfs value it was later compared against./proc/<pid>/staton Linux withps/PowerShell as the fallback, the image pinsOPENALICE_MACHINE_ID=openalice-docker, and a hostname-derived machine id is never treated as evidence of a different machine, so locks written by older images are reclaimed too.OPENALICE_TAKEOVER=1could not clear a lock recorded against a genuinely different machine id:recoverRuntimeOwnerrefused before checking anything. It now claims such a record once its heartbeat is stale, still never signals a pid it cannot address, and still refuses while the remote owner is heartbeating. The refusal is a typedCrossMachineOwnerErrorcarrying the inspection, so the dev launcher renders it as an owner report instead of a raw throw.RuntimeLockOptions.onOwnerReclaimedreports the inspection that justified the claim, and every launcher (scripts/guardian/prod.mjs,scripts/guardian/dev.ts,apps/desktop/src/main.ts,src/main.ts) logs one line, for example[guardian/prod] reclaimed guardian-docker lock from pid 7 (stale: owner process is not running).pnpm broker-packs:buildcould ship a stale pack: eachpackages/uta-broker-*bundle inlinesservices/uta/srcthrough a relative import that turbo does not track, so a cache hit restored an olddist/index.jsafter a broker source edit. Each of those packages now declares$TURBO_ROOT$/services/uta/src/**as a build input.Same root cause as #1177 (draft) for the lock reclaim. That branch persists a random UUID under the state directory and gates foreign reclaim behind an opt-in flag; this one fixes the identity comparison itself so the common Docker case needs no flag, and keeps the flag-free cross-machine refusal for strong ids.
Included increments
fix(guardian): reclaim stale runtime locks after Docker container restarts(procfs start time,compareProcessIdentity, machine id pin, weak hostname ids, same-clock start time, typed cross-machine error)fix(guardian): let takeover clear a stale cross-machine lockfeat(guardian): report reclaimed runtime lock owners to the launcherfix(build): key broker pack turbo cache on the bundled out-of-package sourcesVerification
Automated:
node scripts/run-tests.mjs --package @traderalice/guardian-runtime: 9 files, 71 tests passedcd packages/guardian-runtime && npx tsc --noEmit: cleannpx tsc --noEmit: cleannpx turbo run build --filter=@traderalice/uta-broker-ibkr --dry-run=json: on this branch the task hash changes after editingservices/uta/src/domain/trading/brokers/ibkr/IbkrBroker.ts(44ee180cc0c6ef19to9dc4c7f0aad5b274); withorigin/dev'sturbo.jsonthe same edit leaves the hash at98cb131f58cafd28.Manual, in a disposable compose stack (
oa-infra-test, fresh volume, lite mode, no ports), each scenario waiting for the container healthcheck:docker killthenup -d --force-recreate(new container id and hostname): healthy in 6 s, lock rewritten to the new container, no manual removal.machineId: hostname:deadbeefcafeand a heartbeat 5 h old (the shape every pre-pin image leaves behind): reclaimed, healthy in 6 s.machineId: env:openalice-docker, stale heartbeat, and pid 7 (the pid Guardian actually gets in the container, so pid reuse): reclaimed, healthy,RestartCount=0.env:other-host) and a stale heartbeat, no takeover: refused withRuntimeAlreadyRunningError("owner belongs to another machine and its heartbeat is stale; refusing automatic takeover"), container stays unhealthy.OPENALICE_TAKEOVER=1: healthy in 6 s, log line[guardian/prod] takeover -> previous OpenAlice runtime stopped, owner records rewritten; before the takeover commit this case crash-looped on "belongs to another machine; refusing to signal it".OPENALICE_TAKEOVER=1: 10 refusals, 0 takeovers over 140 s; a live remote owner is never displaced.OPENALICE_MACHINE_IDbaked in,/etc/machine-idabsent (confirming why the pin exists).Not run:
pnpm test:system:guardianspawns the realpnpm devstack on 47331-47334, which a live container on this host holds. Theps/PowerShell fallback path for non-Linux hosts is covered by unit tests only.Boundary touch
Non-goals
restart: unless-stoppedandthe reason is only in the logs.identity === 'unverified'and stale-heak without terminating an owner; it only triggers whenprocfs,ps, and PowerShell all fail and is unchanged here./datavolume across hosts; documented as unsupported, the machine id must be overridden per host.