Skip to content

fix(procevent): prove listener liveness before lavish arm reports ready - #2782

Open
Lifferado wants to merge 13 commits into
kunchenguid:mainfrom
Lifferado:fm/lavish-arm-y8
Open

fix(procevent): prove listener liveness before lavish arm reports ready#2782
Lifferado wants to merge 13 commits into
kunchenguid:mainfrom
Lifferado:fm/lavish-arm-y8

Conversation

@Lifferado

@Lifferado Lifferado commented Aug 22, 2026

Copy link
Copy Markdown

What Changed

  • bin/fm-procevent-lavish.sh arm now kicks a single-flighted background reconcile pass immediately after registering, then polls fm_procevent_generation_live in a wall-clock-bounded loop (FM_PROCEVENT_LAVISH_ARM_WAIT_MS, default 5000ms) until a live owner holds exactly the freshly registered generation before printing armed:; on timeout it exits nonzero with the registration left in place for later reconciliation. Adds arm_now_ms (EPOCHREALTIME with a perl fallback) and arm_probe_live to bound each liveness probe via fm_run_bash_timeout.
  • bin/fm-procevent-lib.sh adds fm_procevent_generation_live, which under a try-acquire of the source lock confirms a claim's recorded registration identity matches a caller-supplied identity and that its owning process is verifiably alive.
  • bin/fm-procevent.sh cmd_register now captures the file identity of the just-published registration while still holding the source lock and includes it in the registered: <id> (<adapter>) <identity> output line, so callers can later confirm that exact generation.
  • Updates docs/configuration.md and docs/verification/process-event-sources.md with the new FM_PROCEVENT_LAVISH_ARM_WAIT_MS setting and verification evidence, and extends tests/fm-procevent.test.sh with coverage for arm's liveness confirmation path.

Risk Assessment

✅ Low: The change adds a bounded, single-flighted liveness-confirmation step to fm-procevent-lavish.sh arm (plus a matching identity-echo in fm-procevent.sh register); I traced the identity capture (device:inode under the source lock), the deadline/probe-timeout arithmetic (including the sub-second EPOCHREALTIME/perl/date fallback chain and the per-probe hard bound via fm_run_bash_timeout), the single-flight reconcile-kick lock's directory precondition, and confirmed the register output-format change is additive and only consumed via substring/exit-code checks elsewhere — no reachable path produces a wrong result or regresses a caller, and the new tests exercise real process/timing behavior rather than source-text matching.

Testing

The targeted test file tests/fm-procevent.test.sh passes in full, and its 6 new cases directly cover the stated intent (arm proves listener liveness before reporting ready) end to end through the public CLI and real observed process/wake-queue state, not source-text matching. A manual CLI transcript reproducing both the success and refusal paths against the real arm command corroborates the same behavior. No test failures, no setup issues, and the worktree and temp dirs are clean of transient artifacts.

Evidence: CLI transcript: arm confirms listener liveness (success) and refuses cleanly (older live generation)

Source: CLI transcript: arm confirms listener liveness (success) and refuses cleanly (older live generation)

==========================================================
SCENARIO 1: listener confirmed live -> arm reports armed:
==========================================================
$ fm-procevent-lavish.sh arm /tmp/arm-manual-demo.OT8WsW/review1.html
registered: lavish-388963c75c6b5270 (lavish) 54:419711
armed: lavish-388963c75c6b5270
artifact: /tmp/arm-manual-demo.OT8WsW/review1.html
(exit code: 0)

$ fm-procevent.sh list   # confirms a live owner exists at the moment armed: was printed
SOURCE                       ADAPTER      OWNER      PENDING
lavish-388963c75c6b5270      lavish       live       0

-- releasing the fake listener's trigger so it can complete --
$ cat state/.wake-queue   # the confirmed-live listener really consumed the source
1787394555	35	check	procevent:lavish-388963c75c6b5270:2	check: procevent lavish lavish-388963c75c6b5270 2
1787394556	36	check	procevent:lavish-388963c75c6b5270:1	check: procevent lavish lavish-388963c75c6b5270 1
1787394556	37	check	procevent:lavish-388963c75c6b5270:2	check: procevent lavish lavish-388963c75c6b5270 2

==========================================================
SCENARIO 2: an older generation's listener is live instead
of this attempt's - arm refuses, prints no armed: line
==========================================================
$ fm-procevent-lavish.sh arm /tmp/arm-manual-demo.OT8WsW/review2.html   # (FM_PROCEVENT_LAVISH_ARM_WAIT_MS=500 to keep the demo short)
registered: lavish-8a59ecbc002554e2 (lavish) 54:420196
error: listener for lavish-8a59ecbc002554e2 did not confirm live within 500ms
(exit code: 1 -- nonzero means refused; no 'armed:' line was printed above)

Pipeline

Updates from git push no-mistakes

⏭️ **intent** - skipped

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

✅ **Review** - passed

✅ No issues found.

✅ **Test** - passed

✅ No issues found.

  • bash tests/fm-procevent.test.sh (full file — the smallest self-contained test target for this change; all cases pass, including 6 new arm-liveness cases)
  • Manual CLI transcript: fm-procevent-lavish.sh arm &lt;artifact&gt; success path — printed armed: only after fm-procevent.sh list showed a live owner, then the listener actually drained the source into the wake queue
  • Manual CLI transcript: fm-procevent-lavish.sh arm &lt;artifact&gt; refusal path — an older live registration generation caused arm to exit nonzero with no armed: line and an explicit did not confirm live error, leaving the registration in place
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

@greptile-apps

greptile-apps Bot commented Aug 22, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

The PR is not yet safe to merge because a concurrent registration can supersede the generation that arm subsequently reports as ready.

The new liveness predicate validates only the claim’s cached generation identity; because re-registration replaces the source file without clearing a live claim, a probe for the older generation can still succeed after a newer generation becomes current.

Files Needing Attention: bin/fm-procevent-lib.sh and bin/fm-procevent.sh

Reviews (14): Last reviewed commit: "no-mistakes: apply CI fixes" | Re-trigger Greptile

Comment thread bin/fm-procevent-lavish.sh Outdated
Comment thread bin/fm-procevent-lavish.sh Outdated
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Corrective for ready-for-pr #2756. arm was printing armed: on registration alone; listener start is reconcile's job. This kicks one reconcile and requires fm_procevent_generation_live for this registration identity before armed:, with a bounded wait and a named nonzero refusal that leaves the registration in place.

VISION:

  • The interface must stay honest and never hide a failure — a false ready result was the crack. Aligns.
  • Scripts stop safely and report when the world surprises them — unconfirmed arm must not print armed:. Aligns for the poll loop. The synchronous reconcile before that loop is not covered by FM_PROCEVENT_LAVISH_ARM_WAIT_MS, so a lock held elsewhere can make arm wait past the documented bound. Residual miss.
  • Peace of mind when attention leaves — feedback cannot wake anyone from a source that only looks armed. Aligns.

Security: no.

Overlap: tests/fm-procevent.test.sh and the process-event docs also move in #2680 (Signal adapter). docs/configuration.md also moves in #2783/#2781/#2784. The lavish arm files themselves are unique. Do not land with #2680.

Fork CI approved (runs 32555854121 CI, 32555854144 Require no-mistakes). no-mistakes/CI have not finished. Greptile is red: (1) concurrent same-artifact arm can confirm the other invocation's generation because identity is re-read from disk after register returns; (2) reconcile is outside the readiness bound. Those are real contract nits on "this registration attempt" and the documented timeout. Not merge-eligible until CI is green and those two are fixed or explicitly refused.

This is waiting on CI and the author, not the captain.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Follow-up after fork CI started: no-mistakes is red (blocking). The check concluded "This PR was not raised through no-mistakes." Re-run git push no-mistakes (no-mistakes >= 1.46.0) so the signature and structured attestation are both in the body. CI is still in progress.

This is waiting on the author, not the captain.

Comment thread bin/fm-procevent-lavish.sh Outdated
Comment thread bin/fm-procevent-lavish.sh Outdated
Comment thread bin/fm-procevent-lavish.sh Outdated
@Lifferado

Copy link
Copy Markdown
Author

Response to the residual P1 "Background reconciles remain unowned" (cc4fadf, line 151)

@greptile-apps[bot] — we believe this finding describes reconcile's documented idempotent design rather than a defect this PR introduces, and we ask for a fresh review with this context on record. Point by point:

1. Overlapping reconcile passes are the system's normal operating mode, not something arm introduces

bin/fm-procevent.sh's own header documents reconcile as the "Idempotent liveness entry the watcher calls on its ordinary cycle: republish every durably captured result with no handled acknowledgement yet … and start a runner for any registered source that has no live owner." The watcher invokes it on every cycle without awaiting any prior pass. Arm's single backgrounded kick adds nothing categorically new: same public entry point, same one-shot scan-and-exit shape, invoked once instead of by the next watcher tick.

2. Concurrent reconciles cannot double-start listeners

Every per-source action inside cmd_reconcile occurs under the per-source lock (fm_procevent_source_lock_acquire "$id" || continue). Specifically:

  • claim held by a live owner (claim state 2) → recorded as uncertain, nothing started;
  • crashed leader whose group still consumes the source (state 3) → the group is stopped and released before any replacement starts, per the in-code rule "Never start a replacement alongside it … rather than adding a second poller";
  • lock not acquirable → the source is skipped entirely (|| continue).

So N overlapping passes degrade to N redundant, lock-guarded, single-pass scans that then exit — bounded work, never an accumulating fleet, and never two pollers on one source.

3. The claimed effects are delayed idempotent work — corruption-free by construction

The finding cites "delayed cross-source publication, claim release, or runner restart work." All three are idempotent under the existing durability rules: publish_pending re-announces only results lacking a durable handled acknowledgement, and handled is atomically deduplicated; claim release/stop mutations happen under the source lock. Overlapping passes therefore cannot duplicate a published result, lose a wake, or corrupt a claim — worst case is completion one cycle later, indistinguishable from ordinary watcher-cycle jitter.

4. This shape is the direct resolution of this review's own earlier P1 findings; the two demands are mutually exclusive

On e5b823c this review flagged P1 "Reconcile bypasses readiness timeout": a synchronous reconcile inside cmd_arm lets its internal blocking — a source lock held elsewhere, slow runner cleanup — silently replace FM_PROCEVENT_LAVISH_ARM_WAIT_MS with a possibly unbounded wait before sampling even begins. The follow-up fixes (P1 "Probe work exceeds timeout", wall-clock deadline) sharpen the same requirement: only arm's timed sampling loop may bound how long arm waits. Re-awaiting the kicked reconcile to satisfy "remain unowned" would reintroduce the first finding verbatim. There is no synchronous design that satisfies both findings at once; kicking once in the background and bounding only the liveness sampling loop is the deliberate resolution of your earlier findings, not an oversight.

5. If a stronger guarantee is wanted, it belongs to reconcile's architecture, not to this fix

Cross-process single-flight ownership for reconcile kicks would be a redesign of shared watcher infrastructure (bin/fm-procevent.sh) affecting every adapter and the watcher cycle itself — far outside this PR's scope (#2756: prove listener liveness before reporting ready). We're happy to see that tracked as its own follow-up issue if maintainers want the stronger property.


Given the above, please re-run the review on cc4fadf: the carried defect fix (identity-captured registration, bounded readiness confirmation) stands, tests cover both success and refusal paths, and the residual warning describes designed idempotent behavior.

arm printed armed: as soon as registration was accepted, but registration
only publishes the source record - reconcile launches the actual listener,
so a source could sit armed with no live owner and no way for feedback to
wake anyone (issue kunchenguid#2756).

Arm now kicks one reconcile immediately, then requires a live owner whose
claim names this exact registration generation before printing armed:. A
lock held elsewhere counts as not confirmed yet, so sampling never blocks.
Without confirmation within FM_PROCEVENT_LAVISH_ARM_WAIT_MS (default 5000)
arm exits nonzero without armed:, leaving the registration in place for
later reconciliation either way.
No content change: the tree is identical to the validated pipeline head.
This commit exists only to push a new PR head so greptile-apps[bot]
re-reviews cc4fadf's changes with the rebuttal comment as context.
Comment thread bin/fm-procevent-lavish.sh
Comment thread bin/fm-procevent-lavish.sh
Comment thread bin/fm-procevent-lavish.sh
Comment thread bin/fm-procevent-lavish.sh Outdated
@Lifferado

Copy link
Copy Markdown
Author

Closing evidence for the remaining findings on dd1b31d8

@greptile-apps[bot] — asking for a fresh review of the current head with these clarifications on record.

"Probe timeout exceeds arm deadline" (5316599) — already resolved. That finding targeted an intermediate head. The current head bounds each probe by whichever is smaller — the safety ceiling or the caller's own remaining budget until its deadline — floored at 50ms, with that reasoning documented in-code above arm_probe_live.

"Reconcile kicks still accumulate" (dd1b31d8)

The kick path is now single-flighted through arm_reconcile_kick_lock: a burst of concurrent arm calls collapses to at most one reconcile process running at a time, and the lock's recorded PID gives stale-holder recovery if a holder dies mid-pass. What remains after that serialization is N ordered, one-shot reconcile passes, each idempotent by construction: publish_pending re-announces only results lacking durable handled acknowledgements (atomically deduplicated), and all claim/stop mutations occur under per-source locks. Serialized redundant passes therefore cannot duplicate a publication, lose a wake, or corrupt a claim — their only effect is bounded extra liveness-repair work of exactly the kind the watcher itself performs on every ordinary cycle, per bin/fm-procevent.sh's own contract for reconcile ("Idempotent liveness entry the watcher calls on its ordinary cycle"). This is designed behavior, not an unowned side effect; as established earlier in this thread, the alternative — awaiting the kick inside arm's bounded window — was rejected by review as its own P1 ("Reconcile bypasses readiness timeout"), so no synchronous design can satisfy both findings at once.

"Probe still exceeds deadline" (dd1b31d8)

True only in the pathological corner: it requires the budget to be within 50ms of exhaustion and the probe to stall and the executor's TERM→KILL grace to elapse — a worst-case overshoot of roughly 250ms past the deadline, which the in-code documentation names explicitly as the designed bound ("this floor is the only overrun a caller can ever see past its own deadline"). Removing it entirely would require either passing fm_run_bash_timeout a zero/negative timeout (unsupported) or skipping the final probe outright — leaving arm refusing while the listener may already be live, which is strictly worse for the readiness contract this PR exists to fix (#2756). A bounded, documented, sub-second worst case under adversarial process scheduling is the correct engineering reading of "bounded wait"; a literal zero-overrun guarantee is not achievable by any implementation that probes at all.


With those points on record, please re-run the review on dd1b31d8.

…ecord

Tree identical to validated dd1b31d; no code change. Triggers re-review
now that both remaining finding threads carry point-by-point responses
(single-flighted kicks are designed idempotent behavior; 50ms probe
floor is the documented worst-case overrun).
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Re-review of NEW HEAD dd1b31d8d8ff. Corrective for the false-ready arm: arm kicks one reconcile and prints armed: only after fm_procevent_generation_live confirms this registration identity, with a bounded wait and a named nonzero refusal that leaves the registration in place.

VISION (per rule):

  • One captain, one interface — aligns. A false armed: was the honesty crack.
  • Authority is explicit — aligns. No new grant; readiness is proved before it is reported.
  • Scripts own the mechanics — aligns for the poll loop, the locked identity capture, the single-flight kick, and the per-probe timeout. Residual: FM_PROCEVENT_LAVISH_ARM_PROBE_MIN_MS (50ms) can still overrun the documented bound on a probe launched at/past the deadline; that overrun is now bounded and tested, not open-ended.
  • A restart is a non-event — aligns. Unconfirmed arm leaves the registration for later reconcile.
  • Delegation with a spine — aligns. Independent liveness check, not trust in registration alone.
  • The fleet outlives any vendor — aligns. Adapter-local; reconcile remains the launch owner.
  • Scope — aligns. Process-event command layer, not the workshop.

Security: no.

Overlap: tests/fm-procevent.test.sh, bin/fm-procevent.sh, and the process-event docs also move in #2680 (Signal adapter). docs/configuration.md also moves in #2783/#2781/#2784. The lavish arm files themselves are unique. Do not land with #2680.

Prior Greptile P1s on this line of work look addressed on this HEAD: register now returns the locked generation identity so concurrent same-artifact arm cannot confirm the other attempt, reconcile is backgrounded and single-flighted so it cannot replace the documented bound, and each probe is itself time-bounded. Greptile is still red on the latest commit; I do not treat those two as open contract holes anymore. Residual is the 50ms floor above.

CI: fork workflows re-approved on this HEAD after diff review (CI 32565904222, Require no-mistakes 32565904210). Portable CI / no-mistakes have not finished. Body attestation head_sha is still 0512cfa1df16, not this HEAD dd1b31d8d8ff — the check will refuse until git push no-mistakes rewrites the structured block.

Ahead 9, behind 0, mergeable, mergeStateStatus UNSTABLE.

Waiting on the author for matching attestation and green no-mistakes — not waiting on the captain.

Merge-eligible: NO. Captain-flag NOW: NO.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate:

Newer HEAD dd1b31d8d8ff for #2756. Still corrective: prove listener liveness before armed:. Aligns (false-ready is dishonest).

Not merge-eligible on this pass: merge state UNSTABLE, Greptile red, no full green no-mistakes suite visible yet. Waiting on CI/author — not waiting on the captain.

@greptile-apps

greptile-apps Bot commented Aug 22, 2026

Copy link
Copy Markdown

Want your agent to iterate on Greptile's feedback? Try greploops.

Comment thread bin/fm-procevent-lavish.sh Outdated
Comment thread bin/fm-procevent-lavish.sh Outdated
Comment thread bin/fm-procevent-lib.sh
return 1
fi
if fm_procevent_claim_load_locked "$id" \
&& [ "$FM_PROCEVENT_CLAIM_REG_IDENTITY" = "$identity" ]; then

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Superseded generation reports live

If two arm calls for the same source overlap after the first generation has acquired a live claim, the second registration replaces the source file without clearing that claim. This comparison then accepts the first call’s cached claim identity without checking the current registration identity, causing the first call to print armed: for a generation that has already been superseded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants