fix: clean up AFK launcher lock on signal - #517
ViktorTsvetkov wants to merge 3 commits into
Conversation
|
Some extra evidence for this one, in case it helps prioritise it: while validating #506 I saw this race reproduce on a clean
That is the existing To check it on your side: git checkout main
for i in $(seq 1 20); do
./tests/fm-afk-launch.test.sh 2>&1 | grep '^not ok - launcher signal' || true
doneI measured roughly 40% failures before this fix and 0/150 after — but that was on my hardware, and the window is timing-dependent, so the rate is likely to differ on yours. Worth confirming rather than taking my number at face value; the failure mode itself should reproduce. |
|
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#517 at |
kunchenguid
added
the
wheelhouse:pending-contributor-action
|
Automated reminder: this PR still looks blocked on a rebase or merge conflict fix. If you are still interested, please rebase onto the current base branch, resolve the conflict, and push. If I do not hear back, I may close this as inactive. |
|
I am closing this because it has been waiting on a rebase or merge-conflict fix since 2026-07-24, and I have not seen a comment or push since then. If you still want to keep working on this, please reopen it or open a new PR and mention this one. Happy to take another look when there is an update. |
|
Scheduling approved by David, 2026-09-09. The overnight-campaign session that owns this card is approved to start, which supersedes the earlier intake-only hold recorded on this issue. Ownership and overlap fences for that session come from the campaign's bundling and overlap audit completed before launch. Item-level acceptance stays per card, and a review finding on any card in a bundle holds the whole bundle. Not released by this scheduling approval: specific design selections, protected-region decisions, and any recurring production cost increase. Those remain reserved for David. |
3 participants
Intent
Fix a signal-handling race in the away-mode launcher. fm_afk_launch_main armed its EXIT/INT/TERM traps AFTER calling fm_afk_launch_lock_acquire, so a TERM/INT delivered in that window killed the process by default disposition, the EXIT trap never ran, and the lifecycle lock was orphaned. Move the three trap lines before the lock acquisition; fm_afk_launch_lock_release is already pid-guarded ([ pid = $$ ] || return 0), so arming the EXIT trap before acquisition is a safe no-op if a signal fires pre-acquire and a correct release otherwise. Pure cross-platform bug fix, affects POSIX and Windows equally, not gated. Deflakes tests/fm-afk-launch.test.sh unit_signal_exits_with_lock_cleanup (measured ~40 percent to ~0.4 percent failure).
What Changed
.afk-launch.lockand preserve signal exit status.Risk Assessment
✅ Low: Captain, the change is tightly scoped to AFK launcher signal cleanup, preserves the existing lock ownership guard, and adds a focused regression case for the previously reported publication-window race.
Testing
The pre-run full baseline was already green; I then ran the relevant AFK launcher suite and a focused signal-window check for TERM and INT, both of which demonstrated correct signal exit and lock cleanup with evidence saved under
/tmp/no-mistakes-evidence/01KXD28SMG8QQMACD1NKHAA3N9.Evidence: AFK launcher test log
Evidence: Signal-window transcript
signal=TERM exit_code=143 expected=143 lock_exists_after_exit=no verdict=pass signal=INT exit_code=130 expected=130 lock_exists_after_exit=no verdict=passPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-afk-launch.sh:601- The remaining lock-acquisition critical section can still orphan.afk-launch.lock: the traps now run beforefm_afk_launch_lock_acquire, butfm_afk_launch_lock_releaseonly removes the directory whenpidalready equals$$. If TERM/INT lands aftermkdir "$FM_AFK_LAUNCH_LOCK"succeeds but beforepid/pid-identityare fully written, the EXIT trap sees no matching pid and leaves the just-created lock behind. Make acquisition publish ownership in a signal-safe cleanup path, or let the release path remove an incomplete lock known to be created by this process.🔧 Fix: Captain, harden AFK lock signal cleanup
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"Baseline already provided as passing:command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; exit "$rc"bash tests/fm-afk-launch.test.sh | tee /tmp/no-mistakes-evidence/01KXD28SMG8QQMACD1NKHAA3N9/fm-afk-launch-test.logManual evidence check invokingfm_afk_launch_main startwith injectedTERMandINTduringmkdir "$FM_AFK_LAUNCH_LOCK", recorded in/tmp/no-mistakes-evidence/01KXD28SMG8QQMACD1NKHAA3N9/afk-launch-signal-window-transcript.log✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.