fix(bin): honor shutdown after remote job worker ownership loss - #1980
Open
tiago-peixoto wants to merge 3 commits into
Open
fix(bin): honor shutdown after remote job worker ownership loss#1980tiago-peixoto wants to merge 3 commits into
tiago-peixoto wants to merge 3 commits into
Conversation
Owner
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for firstmate#1980 at |
4 tasks
kunchenguid
removed
the
wheelhouse:pending-contributor-action
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
2 participants
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.
Intent
Fix the pre-existing remote job worker ownership-loss shutdown defect as its own focused change and pull request, separate from unrelated PR 1716. The worker's stale-job sweep can recreate the state root, jobs, and logs without worker.lock; quarantine publication then has no ownership directory, and TERM must not be swallowed so the same unowned serving worker continues publishing ready heartbeats. Choose the safe ownership or shutdown behavior from the existing code rather than guessing, keep the product change small, and preserve existing active-execution cleanup. The regression must first fail against the unfixed product and then pass with the fix, and must deterministically construct the state by watching the atomic heartbeat inode replacement, immediately sending STOP, removing the state root while stopped, sending CONT so the stale-job sweep recreates lock-free state, then sending TERM and observing the serving process. Do not replace that synchronization with timing luck or weaken the regression. Publish this branch with one upstream push and one pull request body publication because each extra push or body edit spends another approval round-trip.
What Changed
worker_shutdownno longer swallows the signal whenworker_publish_quarantinefails. Previously it re-armed theHUP INT TERMtrap and returned 0, so a worker whose ownership directory was gone resumed serving and kept publishing ready heartbeats; it now exits 125, and because the EXIT cleanup trap is registered before the shutdown trap, active-execution cleanup still stops the command tree first.tests/fm-remote-job-orphan-reap.test.shto construct the unowned state deterministically instead of by timing: a hard link pins the publishedworker.readyinode, a fork-free builtin-only loop watches for the atomic replacement and STOPs the serving child in that window, the state root is removed while it is stopped, CONT lets the stale-job sweep recreatejobs/logs/worker.readywithoutworker.lock, and TERM must then kill the child rather than let it advance the heartbeat. If a loaded host deschedules the watcher past the window, the child dies on its next heartbeat and the whole construction retries (bounded at 8) against the next serving child the supervisor spawns.serving_child/next_serving_childmatch onfm-remote-job-worker.sh --serveso a supervisor's restart-backoffsleepor a transient state-preparation subshell is never mistaken for the serving child, andinode_ofresolves the platform-specificstatselector once at startup instead of forkingunameon every poll. The file header now documents this second leak alongside the pruned-code-root one.Risk Assessment
✅ Low: The round-2 commit is test-only and well-bounded: it replaces the racy fork-based watcher with a fork-free hard-link/
-efloop plus a retry that can only trigger on a proven miss, preserving every assertion and the fail-against-unfixed-product property, while the reviewed three-line product fix is unchanged.Testing
I confirmed the regression is a real first-fail/then-pass one by reverting only the product hunk to base 833a9a2 - the test then fails deterministically with the serving child surviving TERM and advancing its heartbeat inode - and restoring the fix, after which it passes 5/5 and also 3/3 under 2x CPU oversubscription, so the STOP-in-the-heartbeat-window construction and its retry are not relying on timing luck. Because a passing assertion does not show what an operator sees, I drove the real worker through the same construction and captured
psand heartbeat state on both sides: before the fix the TERMed pid stays inpswith no worker.lock and its heartbeat inode keeps advancing, after the fix that pid is gone and the supervisor's replacement holds genuine ownership with worker.lock/pid and worker.ready naming the new pid. I separately exercised the changed exit-125 path with a real in-flight job to check the intent's preserve-active-execution-cleanup constraint: on the fix the recorded command group is killed and its side effect never lands, while on the base the worker ignores TERM and the job runs to completion and mutates. tests/fm-remote-job.test.sh passes in full, covering the unchanged shutdown and quarantine paths. No product failures; the only note is pre-existing gpg-signing noise in the fixture's git commit that does not affect any assertion. Transient scratch roots and worker processes were cleaned up and the worktree is clean.Evidence: Operator transcript - BEFORE the fix: unowned worker swallows TERM and keeps publishing ready heartbeats
--- the worker's own state root, recreated by its stale-job sweep ------- $ ls -la .../remote-jobs drwx------ jobs drwx------ logs -rw------- worker.ready worker.lock (the ownership directory) is absent: ls: .../remote-jobs/worker.lock: No such file or directory --- the serving process before TERM ------------------------------------ $ ps -o pid,ppid,state,command -p 12346 PID PPID STAT COMMAND 12346 12299 S /bin/bash .../bin/fm-remote-job-worker.sh --serve published heartbeat: inode 194257032 contents: 12346 --- operator sends TERM to the unowned serving worker ------------------- $ kill -TERM 12346 --- six seconds later -------------------------------------------------- RESULT: the process is STILL RUNNING and still owns no lock. PID PPID STAT COMMAND 12346 12299 S /bin/bash .../bin/fm-remote-job-worker.sh --serve heartbeat inode 194257032 -> 194257110 it is STILL PUBLISHING ready heartbeats: the fleet believes an unowned worker is healthy and will hand it jobs. VERDICT: DEFECT REPRODUCED - TERM was swallowed.Evidence: Operator transcript - AFTER the fix: unowned worker honors TERM, replacement reacquires real ownership
--- the serving process before TERM ------------------------------------ $ ps -o pid,ppid,state,command -p 63757 PID PPID STAT COMMAND 63757 63324 R /bin/bash .../bin/fm-remote-job-worker.sh --serve published heartbeat: inode 194234270 contents: 63757 --- operator sends TERM to the unowned serving worker ------------------- $ kill -TERM 63757 --- six seconds later -------------------------------------------------- RESULT: the process is GONE. $ ps -o pid,ppid,state,command -p 63757 PID PPID STAT COMMAND VERDICT: FIXED - the unowned worker honored TERM and stopped serving. --- what the supervisor put in its place -------------------------------- $ ps -o pid,ppid,state,command -p 66573 PID PPID STAT COMMAND 66573 63324 S /bin/bash .../bin/fm-remote-job-worker.sh --serve $ ls -la .../remote-jobs drwx------ jobs drwx------ logs -rw------- worker.identity drwx------ worker.lock -rw------- worker.pid -rw------- worker.ready ownership is genuine again - worker.lock/pid says 66573 heartbeat says 66573, and the TERMed pid 63757 is not either of them.Evidence: Active-execution cleanup on the changed exit-125 path - AFTER the fix (mid-flight job stopped, no side effect)
--- a real job is mid-flight on an owned worker ------------------------- PID PPID PGID STAT COMMAND 2963 2647 2647 S .../fm-remote-job-worker.sh --serve 4291 2963 4291 S .../fm-shutdown-job.sh .../job-started .../job-side-effect job job-pjUJVn state: running its command tree is recorded at jobs/job-pjUJVn/.claim/group = 4291 --- remove the ownership directory, so shutdown cannot quarantine ------- $ rm -rf .../remote-jobs/worker.lock --- TERM the serving worker -------------------------------------------- $ kill -TERM 2963 the serving worker honored TERM and exited. --- did exit cleanup still stop the active command tree? ---------------- $ ps -o pid,ppid,pgid,state,command -p 4291 PID PPID PGID STAT COMMAND the recorded command tree and the job command itself are both gone. waiting past the job's own sleep to prove it never completed its work... $ ls .../job-side-effect ls: .../job-side-effect: No such file or directory VERDICT: active execution cleanup PRESERVED - the mid-flight command tree was stopped and never mutated anything after shutdown.Evidence: Active-execution cleanup - BEFORE the fix (worker ignores TERM, job runs on and mutates)
--- TERM the serving worker -------------------------------------------- $ kill -TERM 18787 the serving worker IGNORED TERM and is still running: PID PPID STAT COMMAND 18787 18701 S /bin/bash .../fm-remote-job-worker.sh --serve --- did exit cleanup still stop the active command tree? ---------------- VERDICT: the job still produced its side effect at .../job-side-effect - cleanup failed.Evidence: First-fail proof: regression against the unfixed worker (product hunk reverted to base 833a9a2)
Evidence: Repro harness: unowned-worker TERM construction (heartbeat-inode watch -> STOP -> state removal -> CONT -> TERM)
Evidence: Repro harness: active-execution cleanup on the exit-125 shutdown path
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
tests/fm-remote-job-orphan-reap.test.sh:137- The STOP has to land in the ~7.4ms between the heartbeat mv and prepare_state re-creating the state root, but the detection loop's granularity is ~14ms, so the regression fails roughly half the time. main() writes the heartbeat and then immediately runs fm_remote_job_reap_stale -> fm_remote_job_prepare_state; I measured that prefix (validate_settings, canonical_existing_dir on the account home, normalize/dirname/basename, canonical_existing_dir on the parent) at ~7.4ms, and one iteration of this loop (inode_of subshell + uname fork + stat fork + sleep 0.001) at ~14ms. A synthetic replay of the same cadence detected the inode change within 7.4ms in only 54% of heartbeats, with p90 at 294ms. When the STOP lands late,rm -rf+ CONT leaves prepare_state to chmod/mkdir a removed root, it returns 1, and the next worker_write_heartbeat mktemp fails, so main() exits 1 and line 163 fails with "the serving child exited before recreating the lock-free state". Fix additively rather than by weakening the sync: wrap the whole construct-and-TERM sequence in a retry that, when the serving child dies before recreating the lock-free state, waits for the supervisor to respawn a new child (pgrep -P "$WORKER") and tries again; and hoist theuname -scheck out of inode_of (compute the stat flag once at line 44) so each poll costs one fork instead of three.bin/fm-remote-job-worker.sh:300- The fix makes TERM authoritative for a worker that lost its ownership directory, which is the defect the intent scopes. The adjacent exposure remains and is reachable: an unowned worker that is never signalled keeps publishing worker.ready and processing jobs from a state root it no longer owns, and because worker.lock is gone a second worker can mkdir it and serve the same queue concurrently. The serving loop never re-validates that it still holds the lock. Noting as the natural follow-up; closing it would need a per-iteration ownership check in main(), which is outside the intent's explicit "keep the product change small".bin/fm-remote-job-worker.sh:300- Exiting 125 rather than 0 makes worker_supervise_linux treat this shutdown as a child failure: it incrementsfailures, applies backoff, and counts toward FM_REMOTE_JOB_SUPERVISOR_MAX_RESTARTS. That is the right semantic here (the respawned child re-acquires ownership cleanly, and the test asserts the respawn), and a child that stayed up FM_REMOTE_JOB_SUPERVISOR_HEALTHY_SECONDS resets the counter, so it is self-limiting. Recording the tradeoff, not asking for a change.🔧 Fix: make heartbeat-window watcher fork-free and retryable
2 infos still open:
tests/fm-remote-job-orphan-reap.test.sh:190- The inode watcher spins with no sleep and no yield, which is the point (it is what shrinks detection latency to syscall speed) but is worth recording as a tradeoff: in the normal case it burns one core for the ~65ms between heartbeats, and on the pathological path where the heartbeat stops advancing it burns one core for the full 30s deadline before failing. Under a cgroup CPU quota the spin can get throttled, which makes a miss more likely - but that is exactly what the bounded retry at line 222 covers, so the outcome is a slower test rather than a wrong one. No action; noting it so the cost is a known property rather than a surprise.tests/fm-remote-job-orphan-reap.test.sh:225- The retry-exhaustion message says "the STOP never landed between the heartbeat and the stale-job sweep in N attempts", but the only condition that consumes an attempt is line 208 - the serving child died before recreating the lock-free state. Eight consecutive deaths are far more likely to mean the product started dying on a lock-free sweep than that scheduling missed the window eight times in a row, so the message points a maintainer at a scheduling ghost. Appending the observed cause ("the serving child died before recreating the lock-free state on every attempt") keeps the diagnosis honest without touching the contract.tests/fm-remote-job-orphan-reap.test.sh:110- tests/fm-remote-job-orphan-reap.test.sh (and tests/fm-remote-job.test.sh) build their fixture code root withgit commitbut never setcommit.gpgsign false, so on a machine with global commit signing enabled the fixture commit fails and printsgpg: signing failed: No secret key/fatal: failed to write commit objectinto the test output. The assertions still pass because the worker only needs the git index, not a commit, so this is cosmetic - but it makes passing runs look broken and could hide a genuine failure. Pre-existing on the base commit, not introduced by this change.bash tests/fm-remote-job-orphan-reap.test.shwith the fix - 5 consecutive runs, exit 0 eachbash tests/fm-remote-job-orphan-reap.test.shwith bin/fm-remote-job-worker.sh reverted to base 833a9a2 - 4 runs, allnot ok - the lock-free serving child survived TERM and advanced heartbeat ..., exit 1bash tests/fm-remote-job-orphan-reap.test.shunder 2x CPU oversubscription (28 spinner processes on 14 cores) - 3 runs, exit 0, retry construction never exhausted its 8 attemptsbash tests/fm-remote-job.test.sh- 22 assertions pass, coveringworker shutdown terminates the active command tree before replacement,failed shutdown quarantines ownership against replacement workers,quarantine clears only after recorded execution has stoppedManual end-to-end reprorepro-unowned-worker-term.shrun against both the base and fixed worker: starts a real worker via fm_remote_job_start_linux_worker, pins the published heartbeat inode with a hard link, STOPs the serving child on atomic inode replacement, removes the state root while stopped, CONTs so the stale-job sweep recreates jobs/logs/worker.ready without worker.lock, then TERMs and recordspsplus heartbeat inode movementManual verificationrepro-active-execution-cleanup.shagainst both workers: stages a real job whose command ignores TERM, removes only worker.lock so worker_publish_quarantine fails, TERMs the serving worker, then checks the recorded.claim/groupcommand tree and whether the job's post-sleep side effect ever appearsCode trace confirmingtrap worker_exit_cleanup EXIT(bin/fm-remote-job-worker.sh:678) is registered beforetrap worker_shutdown HUP INT TERM(line 687), so the newexit 125still runs active-execution cleanupbin/fm-remote-job-worker.sh:18- The remote job worker's ownership state machine has no authoritative owner document: worker.lock, its quarantine marker, and the distinct shutdown outcomes (exit 75 when ownership is quarantined after an unconfirmed shutdown, exit 125 for a shutdown that could not guard or stop cleanly, and how the Linux supervisor treats each) exist only as scattered code in bin/fm-remote-job-worker.sh and bin/fm-remote-job-lib.sh. This change's own invariant is correctly captured as a call-site comment, so nothing here is stale or wrong today. A follow-up consolidating that state machine into the bin/fm-remote-job-worker.sh header (the tier-7 mechanics owner, alongside the abandonment and supervisor-backoff paragraphs already there) would give the contract one owner, but writing it means restating behaviour this change did not touch and is out of scope for a focused fix.✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.