fix(procevent): prove listener liveness before lavish arm reports ready - #2782
fix(procevent): prove listener liveness before lavish arm reports ready#2782Lifferado wants to merge 13 commits into
Conversation
Confidence Score: 4/5The PR is not yet safe to merge because a concurrent registration can supersede the generation that 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 |
e5b823c to
7b8c236
Compare
|
Speaking as Kun's firstmate: Corrective for ready-for-pr #2756. VISION:
Security: no. Overlap: Fork CI approved (runs 32555854121 CI, 32555854144 Require no-mistakes). no-mistakes/CI have not finished. Greptile is red: (1) concurrent same-artifact This is waiting on CI and the author, not the captain. |
|
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 This is waiting on the author, not the captain. |
Response to the residual P1 "Background reconciles remain unowned" (
|
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.
…rmation wait and env var
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.
3dd35d6 to
0512cfa
Compare
Closing evidence for the remaining findings on
|
…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).
|
Speaking as Kun's firstmate: Re-review of NEW HEAD VISION (per rule):
Security: no. Overlap: Prior Greptile P1s on this line of work look addressed on this HEAD: register now returns the locked generation identity so concurrent same-artifact 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 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. |
|
Speaking as Kun's firstmate: Newer HEAD 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. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
| return 1 | ||
| fi | ||
| if fm_procevent_claim_load_locked "$id" \ | ||
| && [ "$FM_PROCEVENT_CLAIM_REG_IDENTITY" = "$identity" ]; then |
There was a problem hiding this comment.
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.
What Changed
bin/fm-procevent-lavish.sh armnow kicks a single-flighted backgroundreconcilepass immediately after registering, then pollsfm_procevent_generation_livein a wall-clock-bounded loop (FM_PROCEVENT_LAVISH_ARM_WAIT_MS, default 5000ms) until a live owner holds exactly the freshly registered generation before printingarmed:; on timeout it exits nonzero with the registration left in place for later reconciliation. Addsarm_now_ms(EPOCHREALTIME with a perl fallback) andarm_probe_liveto bound each liveness probe viafm_run_bash_timeout.bin/fm-procevent-lib.shaddsfm_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_registernow captures the file identity of the just-published registration while still holding the source lock and includes it in theregistered: <id> (<adapter>) <identity>output line, so callers can later confirm that exact generation.docs/configuration.mdanddocs/verification/process-event-sources.mdwith the newFM_PROCEVENT_LAVISH_ARM_WAIT_MSsetting and verification evidence, and extendstests/fm-procevent.test.shwith 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 infm-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
armcommand 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)
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 <artifact>success path — printedarmed:only afterfm-procevent.sh listshowed a live owner, then the listener actually drained the source into the wake queueManual CLI transcript:fm-procevent-lavish.sh arm <artifact>refusal path — an older live registration generation caused arm to exit nonzero with noarmed:line and an explicitdid not confirm liveerror, leaving the registration in place✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.