Repository navigation
fix(bin): wait for the Herdr presentation lock during recovery - #6496
Open
cloud-practitioner wants to merge 5 commits into
Open
cloud-practitioner wants to merge 5 commits into
cloud-practitioner wants to merge 5 commits into
Conversation
Herdr presentation recovery took the session-scoped presentation-order lock with the same five-second bounded try that clean projected creates use. A create can fall back to the flat layout when the lock is contended, but recovery has no fallback, so a concurrent spawn or recovery that held the lock past five seconds made it refuse with "refusing a concurrent resume". On a loaded runner that hold can exceed five seconds, so the outcome depended only on machine speed. spawn_herdr_presentation_order_lock_acquire now takes an optional wait in seconds (default 5, unchanged for ordinary spawns and the abort path). Recovery waits up to 120 seconds, still bounded so a stuck holder produces a clear refusal before any Herdr mutation. FM_TEST_HERDR_RECOVERY_LOCK_WAIT shortens that bound for tests. The real-Herdr presentation e2e now stretches each concurrent recovery's husk replacement past the former budget and asserts the recoveries serialize, and asserts a stuck holder is refused without mutating Herdr. Closes kunchenguid#6454
…contention regression
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…e.test.sh by canonicalizing the expected presentation-lock path at its shared producer using fm_lock_abs_path(), matching the resolver used before mktemp. This fixes attempt logging for both recovery tests when /tmp is symlinked, without changing production behavior. The behavioral probe failed before the fix and passed afterward; physical and symlinked paths passed acquisition, contention, reacquisition, and unrelated-mktemp checks. Bash syntax, canonical lint, and git diff --check passed. The real Herdr E2E run stopped at its safety preflight because the isolated environment lacks the required running default session, so full E2E verification remains blocked
Owner
|
Speaking as Kun's firstmate: RELOOK vs tip VISION (per rule):
Blockers: Fork CI + Require no-mistakes approved this pass ( Does close #6454 when CI is green. |
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
Closes #6454.
On the Herdr runtime, when two session recoveries run at once, the second one gives up after about 5 seconds with "refusing a concurrent resume" instead of waiting for the first recovery's presentation-order lock.
Recovery now waits for that lock for a bounded time (up to 120 seconds), while ordinary new spawns keep the existing short bounded try and flat-layout fallback.
Why the wait is bounded and recovery-only
A clean projected create can fall back to the flat layout when the session lock is contended, so its five-second try stays as is.
Recovery has no such fallback, so a holder that keeps the lock past five seconds (for example a concurrent spawn on a loaded runner) made recovery refuse purely on timing.
The wait is deliberately bounded by an elapsed-time deadline rather than unbounded: a holder that keeps the lock for the whole 120 seconds is treated as stuck, and recovery refuses clearly before any Herdr mutation.
It is also not a new default for other paths: only recovery of an existing presentation journal waits longer, so the earlier unbounded-wait attempts (#5405, #5962) and the opt-in flag proposed in #5989 address a different trade-off, and this change does not add a flag.
Test note
tests/fm-backend-herdr-presentation-e2e.test.shhas a pre-existing failure in the duplicate-token agent check when run against Herdr 0.9.3 (CI pins an older Herdr); it is unrelated to this change, which does not touch that code.What Changed
Risk Assessment
✅ Low: The change is narrowly scoped, enforces the recovery deadline at acquisition, retains ordinary spawn fallback, and preserves identity checks and mutation safeguards after waiting.
Testing
After correcting disposable-driver setup issues, real Herdr/Treehouse checks passed for concurrent recovery, ordinary fallback, the production deadline, and expired acquisition refusal. The baseline failure was reproduced; CLI/API transcripts and timing evidence were saved. Labs were torn down. No full suite, lint, or delivery phase ran; screenshots were unnecessary for this CLI wait-policy change.
Evidence: Live-validation results, isolation, and cleanup
Source: Live-validation results, isolation, and cleanup
Evidence: Target product CLI and API transcript
Source: Target product CLI and API transcript
Evidence: Serialized recovery replacement events
Source: Serialized recovery replacement events
Evidence: Second recovery acquisition attempts
Source: Second recovery acquisition attempts
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 2 issues found → auto-fixed (2) ✅
bin/fm-spawn.sh:1401- The intent requires a bounded wait "up to 120 seconds", butlimit=$(( ${2:-5} * 10 ))bounds sleep iterations, not elapsed time. With a stuck live holder, recovery performs 1,200 sleeps totaling 120 seconds plus 1,201 lock attempts, including filesystem operations and subprocesses. On the loaded runners this fix targets, it can therefore wait materially longer and even acquire a lock released after the promised deadline. Related changed sites: bin/fm-spawn.sh:1405-1408, iteration-based timeout; bin/fm-spawn.sh:3665-3666, recovery call and claimed deadline; docs/herdr-backend.md:366-367, upper-bound guarantee. Correct the recovery path to enforce an elapsed-time deadline at the acquisition boundary while preserving ordinary spawns' existing short try/fallback.tests/fm-backend-herdr-presentation-e2e.test.sh:1390- The regression can pass without exercising the formerly failing wait. If the second recovery is delayed during the serialized Treehouse calls or other preparation until the first releases the presentation lock, the original five-second implementation succeeds too. Both timing assertions pass: the tab creates are non-overlapping and the second starts at least eight seconds after suite launch. Related changed sites: tests/fm-backend-herdr-presentation-e2e.test.sh:153 and :161, tab-create timestamps; :1377-1380, unsynchronized recovery launches; :1388-1390, ordering and elapsed-time assertions. Coordinate the slow holder with the second recovery reaching lock acquisition, then verify that recovery remains pending beyond the former budget before releasing the holder.🔧 Fix applied.
2 warnings still open:
tests/fm-backend-herdr-presentation-e2e.test.sh:1390- The regression can pass without exercising the formerly failing wait. If the second recovery is delayed during the serialized Treehouse calls or other preparation until the first releases the presentation lock, the original five-second implementation succeeds too. Both timing assertions pass: the tab creates are non-overlapping and the second starts at least eight seconds after suite launch. Related changed sites: tests/fm-backend-herdr-presentation-e2e.test.sh:153 and :161, tab-create timestamps; :1377-1380, unsynchronized recovery launches; :1388-1390, ordering and elapsed-time assertions. Coordinate the slow holder with the second recovery reaching lock acquisition, then verify that recovery remains pending beyond the former budget before releasing the holder.tests/fm-backend-herdr-presentation-e2e.test.sh:1380- Round 1’s deadline fix introduces a false failure in the stuck-holder regression. The one-second budget at :1366 now starts before lock-path resolution (bin/fm-spawn.sh:1399-1403). If the real session-list, hashing, and filesystem work crosses the next SECONDS tick, acquisition correctly refuses without attempting the lock. The mktemp wrapper at tests/fm-backend-herdr-presentation-e2e.test.sh:277 therefore writes no attempt log, but :1378-1380 require nonempty first/last timestamps and fail despite correct bounded refusal. This is especially reachable on the loaded runners targeted by the fix. Give the shortened timeout and injected delay enough headroom for real resolution, and handle expiry before the first attempt without interpreting a missing log as a timeout overrun.🔧 Fix applied.
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
python3 .gate6454/live.py: real Herdr/Treehouse checks of ordinary-spawn fallback, the production 120-second recovery deadline, and synchronized cross-home recovery.python3 .gate6454/regression.py: late-acquisition refusal and reproduction using the unmodified baselinefm-spawn.sh.bin/fm-spawn.sh primary-recover <disposable-project> "sh -c 'while :; do sleep 60; done'" --mode no-mistakes --yolo off --backend herdr, with equivalent launches forbravo-recoverandordinary-new, against marked lab homes.Inspected real Herdr workspace and pane API responses, acquisition timestamps, mutation logs, metadata identity, and preserved focus.Ran guardedbin/fm-herdr-lab.shprovision, restart, and teardown operations; teardown verified the default-session tripwire.Corrected disposable-driver setup issues and re-drove the affected checks; removed transient fixtures and the baseline executable, then confirmed a clean worktree.✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.