Skip to content

feat: supervise long-polling sources as durable events - #1446

Open
kunchenguid wants to merge 12 commits into
mainfrom
fm/fm-procevent-runner-r1
Open

feat: supervise long-polling sources as durable events#1446
kunchenguid wants to merge 12 commits into
mainfrom
fm/fm-procevent-runner-r1

Conversation

@kunchenguid

Copy link
Copy Markdown
Owner

Intent

The developer wanted to replace the obsolete Lavish relay work with a Firstmate-native, domain-neutral process-to-event runner plus a thin adapter around the currently published lavish-axi poll interface. The implementation had to use canonical physical source identity, one machine-wide owner per source, direct argv execution, durable 0600 payload capture before ordinary check-wake publication, safe generation-bound claim recovery, and explicit best-effort semantics with no lossless or at-least-once claim and no new Lavish-side work or control plane. They required isolated cross-harness and Herdr-backed validation, deterministic race, symlink, PID-reuse, supervision, lifecycle, and cleanup regressions, including proof that repeated focused tests leave zero owned pollers or children and that cleanup never signals an unverified process. After discovering that the proposed continuing-supervision change addressed only out-of-band home-state deletion rather than normal unregister behavior, the developer withdrew the prior Option A authorization as uninformed and required the branch and run to remain preserved and parked with no further edits, pipeline responses, pushes, PR actions, or merges until explicit captain reconfirmation.

What Changed

  • Add a generic process-to-event runner with durable, bounded result capture, machine-wide source ownership, restart reconciliation, and a thin Lavish polling adapter.
  • Integrate registered sources with watcher supervision, wake handling, guards, and recoverable secondmate teardown across nested homes.
  • Document the operating and delivery guarantees and add regression coverage for source identity, lifecycle races, supervision, and cleanup safety.

Risk Assessment

✅ Low: The latest change correctly preserves the distinct restoration-failure status through recursive and top-level teardown boundaries, and the full branch review found no remaining material source risks.

Testing

Diff inspection, three focused runner-suite passes, turn-end supervision tests, six targeted teardown integrations, and a manual CLI lifecycle all succeeded. The evidence demonstrates exact argv handling, detached ownership, durable 0600 capture before normalized wake publication, and clean retirement with zero claims. An initial selector setup error was corrected without source edits; temporary logs were removed. No screenshot was produced because this change has no rendered UI surface.

Evidence: Process-event end-to-end CLI transcript

Registered demo-source, started one live owner, captured a mode-600 result with exact literal payload, published check: procevent lavish demo-source 1, then retired with zero remaining claims.

1. Register an exact argv source
registered: demo-source (lavish)

2. Reconcile starts the detached blocking owner
reconciled: published=0 started=1 stopped=0 uncertain=0
SOURCE                       ADAPTER      OWNER      PENDING
demo-source                  lavish       live       0

3. Complete the external event and inspect durable state
result: /var/folders/5x/4nqprlbx0518k3ybcb1sz6gr0000gn/T/no-mistakes-evidence/01KYXGGN0PNC6J4DDZVA00HJD9/home/state/procevent-inbox/demo-source.1.result
mode: 600
payload: payload with spaces; printf injected
wake: check: procevent lavish demo-source 1

4. Retire and prove recurring ownership is gone
retired: demo-source
no sources registered
remaining claims: 0
Evidence: Durably captured result

payload with spaces; printf injected

payload with spaces; printf injected

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

🔧 **Review** - 6 issues found → auto-fixed (4) ✅
  • 🚨 bin/fm-procevent.sh:130 - publish_pending marks each result announced immediately after queueing a wake, but the wake identifies only the source and the handling procedure tells the agent to read unannounced results. Consequently the first delivered wake has no unannounced payload, while later wakes cannot distinguish new results from previously handled files. Introduce a durable per-result handled/acknowledged state, or include the committed result generation in the wake and retire it only after handling.
  • 🚨 bin/fm-procevent.sh:307 - After TERM is sent to the process group, cleanup returns successfully as soon as the runner PID disappears. A child that ignores TERM can remain alive in that process group, continue consuming the source, and overlap a replacement owner. Supervision must verify and terminate the owned child group, or make the runner trap, escalate, and reap its child before releasing ownership.
  • 🚨 bin/fm-teardown.sh:1373 - Secondmate teardown retires all process-event sources before later public-followup, worktree-safety, and normal Herdr preflight gates run. If any later gate refuses teardown, the home and records remain but its active waits have already been irreversibly stopped. Move each sweep to the final removal boundary after all refusal checks and endpoint confirmation, including immediately before each nested home removal.
  • ⚠️ bin/fm-procevent.sh:104 - The line-oriented registration format cannot preserve an argv element containing a newline. A legal Lavish artifact path containing an embedded newline is written as multiple records, and read_argv silently truncates it according to argc, executing a different path. Use a length-prefixed or NUL-safe argv representation, or explicitly reject unsupported arguments at registration.
  • ⚠️ bin/fm-procevent.sh:179 - Child stdout is written without a limit and FM_PROCEVENT_MAX_OUTPUT_BYTES is applied only after the child exits. A noisy or blocked source can therefore fill temporary storage, and signal-driven retirement leaves the partial 0600 tempfile behind because the EXIT trap releases only the claim. Bound output while reading, validate the configured limit, and remove staged output from the exit trap.
  • ⚠️ bin/fm-procevent-lib.sh:219 - Pending results are emitted using shell glob order, which is lexical rather than sequence order. Once sequence 10 exists, processing becomes 1, 10, 2 despite the documented oldest-first invariant. Sort the numeric sequence component explicitly at the shared pending-result boundary.

🔧 Fix: Harden process-event delivery and teardown lifecycle
5 issues (3 errors, 2 warnings) still open:

  • 🚨 bin/fm-procevent.sh:134 - Wake publication and its .announced receipt are separate operations. A crash after fm_wake_append succeeds but before line 135 creates the marker causes reconcile to append the same sequence again; if the first wake was already drained, the agent can handle the same result twice. Either persist a durable handled receipt keyed by source and sequence, make publication idempotent across queue drains, or document duplicates and require sequence-based deduplication instead of claiming no duplicate wake.
  • 🚨 bin/fm-procevent.sh:130 - The durable result does not retain its adapter; publication rereads the mutable registration. Retirement can acquire the source lock after capture but before publish_pending, terminate the runner, and delete the registration, leaving a durable result later announced as procevent unknown ... with no supported classifier. Store immutable adapter metadata at the capture boundary or serialize capture and publication against retirement.
  • 🚨 bin/fm-teardown.sh:1014 - Home removal still retires every active source before the actual treehouse return or filesystem removal succeeds. If treehouse return or safe_rm_rf fails, the home and retirement records remain but its waits are already gone, contradicting the existing state-intact failure contract. Make source retirement recoverable on removal failure, or rearm the saved registrations before returning the error.
  • ⚠️ bin/fm-procevent-lavish.sh:50 - The generic runner rejects newline arguments, but the Lavish adapter resolves paths through command substitution first. For an artifact whose filename ends in a newline, command substitution strips the filename newline; if the newline-free sibling exists, the adapter hashes and polls the wrong artifact instead of reaching registration rejection. Reject newlines in the original artifact argument before either realpath command substitution.
  • ⚠️ bin/fm-procevent.sh:185 - The tracked staging file is removed on normal exit and supervised retirement, but SIGKILL or a runner crash bypasses the trap. Stale-claim recovery replaces the claim token without removing .source-id.old-token.output, so partial potentially sensitive output remains indefinitely. Clean the prior generation's validated staging file when reclaiming a stale same-home claim, or sweep orphan staging files that cannot belong to a live verified generation.

🔧 Fix: Preserve process-event identity across crashes and teardown
2 warnings still open:

  • ⚠️ bin/fm-procevent-lib.sh:149 - Stale staging cleanup only runs when the reclaiming home equals the old claim owner. In the supported cross-home ownership case, home B can reclaim home A's dead claim while .source.old-token.output remains indefinitely in home A with partial sensitive output, and neither home's later reconcile enumerates it. Preserve a cleanup obligation for the old home or safely remove the old generation using its validated claim identity before replacement.
  • ⚠️ bin/fm-teardown.sh:1026 - Every failed-removal path discards the return status from registration restoration. If copying or rearming fails, teardown reports only the original removal error; active waits remain retired and early restore failures do not identify the retained backup path, despite the documented restore-before-refusal contract. Surface restoration failure explicitly with the backup location and preserve a distinct recoverable failure state.

🔧 Fix: Complete cross-home cleanup and expose rollback failures
1 warning still open:

  • ⚠️ bin/fm-teardown.sh:1386 - The new restore-failure status is preserved only by this immediate removal call. If a grandchild fails restoration, line 1385 converts status 4 to 1, and even a direct child status reaches line 1467 where it is again converted to exit 1. Preserve the original status through recursive cleanup and the top-level force-cleanup boundary so every nested restoration failure exposes the documented distinct recovery state.

🔧 Fix: Preserve nested process-event rollback status
✅ Re-checked - no issues remain.

✅ **Test** - passed

✅ No issues found.

  • git diff --stat, git diff --name-status, and commit inspection for a8057666..f2a9ccef
  • bash tests/fm-procevent.test.sh three times
  • After repeats 2 and 3, pgrep checks confirmed zero surviving blocker or noisy fixture children
  • bash tests/fm-turnend-guard.test.sh
  • Focused secondmate tests: teardown sweep, missing-sweep refusal, later-refusal preservation, nested force sweep, nested restoration failure, and failed leased-home return
  • Initial process-substitution selector failed to locate its helper due to BASH_SOURCE; retried successfully after sourcing tests/secondmate-helpers.sh directly
  • Manual public-CLI flow: register, reconcile, external completion, result/wake inspection, retire, and zero-claim verification
  • Final git status --short and transient-artifact cleanup checks
✅ **Document** - passed

✅ No issues found.

🔧 **Lint** - 1 issue found → auto-fixed (2) ✅
  • ⚠️ linter found issues (exit code 1)

🔧 Fix: Captain, fix process-event ShellCheck warnings
1 warning still open:

  • ⚠️ linter found issues (exit code 1)

🔧 Fix: Fix nested process-event test quoting
✅ Re-checked - no issues remain.

✅ **Push** - passed

✅ No issues found.

A blocking external process had to be held in firstmate's conversational turn
for its result to be seen at all. This adds a domain-neutral runner that
supervises registered long-polling children outside that turn and turns each
completed process into one normalized durable event.

`bin/fm-procevent.sh` owns the generic contract and `bin/fm-procevent-lavish.sh`
is the first adapter. Adapters stay thin: they supply canonical source identity,
the argv to run, and how to read a completed result. Ownership, durable capture,
publication, and restart recovery all live in the generic owner.

Discovery is never a timer. Each source has its own child blocking on that
source, so a result arrives because the process completed. The watcher's
per-cycle `reconcile` is liveness repair only - it republishes results already
captured durably, starts a source whose owner is gone, and stops one whose
registration was removed. A home with no registered source runs nothing and
generates no state.

Results publish as ordinary `check` wakes through the existing durable wake
queue, so this adds no second notification control plane. Stored argv is
executed directly, so an argument containing spaces or shell metacharacters is
never re-split or interpreted, and there is no shell surface.

Ownership is machine-wide per canonical source, because separate homes can share
one underlying source store. A live owner is never displaced; only a claim whose
runner is gone is reclaimed. A runner is its own process group leader and is
reparented, so both `retire` and `reconcile` signal the group - signalling only
the runner would strand its blocked child, which is how an early version of the
test suite leaked four runners.

DURABILITY BOUNDARY. This proves exactly one thing: output that reached the
runner is stored at mode 0600 before any event referencing it is published, and
an unannounced stored result is re-announced after a restart. It proves nothing
about the source side. The currently published `lavish-axi poll` destructively
clears feedback before returning it, so a result lost between that clearing and
the runner reading process output is unrecoverable, and no wrapper can close
that window. This is NOT at-least-once, no-loss, or lossless delivery, and the
code, tests, docs, and skill all say so.

The adapter wraps only the published poll interface, verified against installed
0.1.45 without upgrading it, and depends on no unreleased capability.
Evidence is in docs/verification/process-event-sources.md.
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.

1 participant