Repository navigation
fix(bin): distinguish remote read deadlines from local ghost bounds - #6710
apfelstrudels wants to merge 4 commits into
Conversation
A poll whose remote end dies without closing the ssh link (observed after a herdr server restart on macmini) held the remote-reply source open indefinitely: the 55s window never closed, no relisten happened, and mirrored replies stalled until the ghost was killed by hand. Wrap the delta read in fm_run_timed with a bound of the wait window plus 120s of transport and queue slack (FM_REMOTE_REPLY_BOUND_SECONDS). A bound hit reports a closed window exactly like a job preemption - the cursor-anchored non-destructive read loses nothing, the runner relistens, and no caught-up watermark is published.
…s relisten-mapping header comment
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Review on kunchenguid#6710 found three holes in the poll bound: - A remote job worker execution deadline also surfaces as a timeout status, and was relistened as an empty window, hiding a failing read. cmd_source now separates the two by elapsed time: a timeout before the local bound could have fired is remote and exits as a failed read. - FM_REMOTE_REPLY_BOUND_SECONDS=0 silently disabled the bound on the timeout and perl mechanisms; a non-positive override now fails loudly. - The ghost regression could stall the suite for the fixture's hour-long sleep when the bound broke; an outer 20s deadline now bounds it, and a new case pins the remote-deadline distinction.
apfelstrudels
changed the title
|
Speaking as Kun's firstmate: reviewed against main What I checked: on main, CI: approved fork runs Heads-up: #6708 also edits this file ( Contract-class: restore. The remote-reply poll is a specified bounded window ( VISION.md, rule by rule
Next step: once CI is green on this head, it's eligible to merge as restore on the next pass. |
3 participants
Intent
The captain asked for a hard timeout on the remote-reply poll connection: a ghost ssh (remote end died without closing, observed after herdr server restarts on the Mac mini) held the remote-reply mirror open for ~55 minutes, stalling mirrored status replies until killed by hand, even though the designed window is 55s. Fix: wrap the delta read in cmd_source of bin/fm-procevent-remote-reply.sh in fm_run_timed with a bound of the wait window plus 120s slack (FM_REMOTE_REPLY_BOUND_SECONDS override); on the bound firing, report a closed window exactly like a job preemption - no caught-up watermark, runner relistens, cursor-anchored read loses nothing. Second commit addresses review findings on PR #6710: distinguish the remote job worker's own execution deadline (also a timeout status) from a local ghost bound by elapsed time - a remote deadline exits as a failed read, never relistens; reject a non-positive FM_REMOTE_REPLY_BOUND_SECONDS loudly; and bound the ghost regression test with an outer 20s deadline so a broken bound fails promptly instead of stalling on the fixture's hour-long sleep. Includes colocated regression tests for both distinctions. Scope is deliberately limited to this adapter; other fm-on callers keep their own timeout contracts.
What Changed
cmd_sourceinbin/fm-procevent-remote-reply.shnow wraps the delta read withfm_run_timedunder a hardBOUND_SECONDSwall-clock bound (WAIT_SECONDS + 120, overridable viaFM_REMOTE_REPLY_BOUND_SECONDS), killing a ghost transport that never closes the link and reporting a closed window so the runner relistens, same as a job preemption - with a louddieif the override is non-positive.BOUND_SECONDScould have elapsed is treated as a failed read and returned as-is instead of being relistened.fm-timeout-lib.shfor the timeout helpers.tests/fm-remote-reply.test.shcovering both cases: a hung "ghost" read that gets killed at its bound and relistens without publishing a caught-up watermark (itself bounded by an outer 20s deadline so a broken bound fails fast), and a simulated remote execution deadline that exits as a failed read without relistening.Risk Assessment
✅ Low: The change is scoped to one adapter, correctly wraps the delta read in fm_run_timed with a documented bound (WAIT_SECONDS+120, overridable), reuses the existing WINDOW_CLOSED_EMPTY/JOB_PREEMPTED relisten contract so a ghost bound behaves exactly like a preemption (no caught-up watermark), distinguishes a genuine remote-job execution deadline from the local ghost bound via an elapsed-time check with a correctly-reasoned 1-second truncation guard band, validates FM_REMOTE_REPLY_BOUND_SECONDS as a positive integer before use, and is covered by new tests that actually execute the adapter (not source-text assertions) for both the ghost-transport and remote-deadline cases, with an outer bound protecting the suite from stalling. I traced the remote worker's own timeout path (bin/fm-remote-job-worker.sh worker_run_with_timeout) and confirmed it does report 124 on its own deadline, validating the premise the elapsed-time disambiguation relies on.
Testing
Ran the colocated regression suite for bin/fm-procevent-remote-reply.sh end-to-end against the real script and fake-ssh fixtures; all 37 scenarios passed, including the two new ones for this change (ghost transport hits its hard bound and relistens without a watermark, in seconds thanks to the 20s outer test deadline; a remote job worker's own execution-deadline timeout is distinguished by elapsed time and exits as a failed read instead of relistening). The non-positive FM_REMOTE_REPLY_BOUND_SECONDS rejection had no existing test, so I drove it live with 0, -5, and abc and confirmed each exits 1 with the expected loud error message. No regressions, no missing evidence.
ok - a ghost reply poll dies at its hard bound and relistens without a watermark(fake ssh sleeps 3600s; fm_run_timed with BOUND_SECONDS=3 kills it, exits 75 in ~3s,…ok - a remote read deadline exits as a failed read instead of relistening(fixture exits 124 immediately; elapsed time is under BOUND_SECONDS-1, so cmd_source return…ok - a preempted reply poll reports a closed window without publishing channel freshnessand `ok - a preempted reply poll keeps its listener and reconcile launches n…FM_REMOTE_REPLY_BOUND_SECONDS=0|-5|abc bin/fm-procevent-remote-reply.sh source ioseach printed `error: FM_REMOTE_REPLY_BOUND_SECONDS must be a positive integer of seconds, got '<va…Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
ok - a ghost reply poll dies at its hard bound and relistens without a watermark(fake ssh sleeps 3600s; fm_run_timed with BOUND_SECONDS=3 kills it, exits 75 in ~3s,…ok - a remote read deadline exits as a failed read instead of relistening(fixture exits 124 immediately; elapsed time is under BOUND_SECONDS-1, so cmd_source return…ok - a preempted reply poll reports a closed window without publishing channel freshnessand `ok - a preempted reply poll keeps its listener and reconcile launches n…FM_REMOTE_REPLY_BOUND_SECONDS=0|-5|abc bin/fm-procevent-remote-reply.sh source ioseach printed `error: FM_REMOTE_REPLY_BOUND_SECONDS must be a positive integer of seconds, got '<va…bash tests/fm-remote-reply.test.sh (full adapter suite, 37 scenarios)manual live invocation: FM_REMOTE_REPLY_BOUND_SECONDS=0|-5|abc bin/fm-procevent-remote-reply.sh source ios✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.