feat: supervise long-polling sources as durable events - #1446
Open
kunchenguid wants to merge 12 commits into
Open
feat: supervise long-polling sources as durable events#1446kunchenguid wants to merge 12 commits into
kunchenguid wants to merge 12 commits into
Conversation
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.
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
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
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
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, publishedcheck: procevent lavish demo-source 1, then retired with zero remaining claims.Evidence: Durably captured result
payload with spaces; printf injectedPipeline
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_pendingmarks 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, andread_argvsilently truncates it according toargc, 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 andFM_PROCEVENT_MAX_OUTPUT_BYTESis 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.announcedreceipt are separate operations. A crash afterfm_wake_appendsucceeds but before line 135 creates the marker causesreconcileto 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 beforepublish_pending, terminate the runner, and delete the registration, leaving a durable result later announced asprocevent 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. Iftreehouse returnorsafe_rm_rffails, 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 eitherrealpathcommand 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.outputremains 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 fora8057666..f2a9ccefbash tests/fm-procevent.test.shthree timesAfter repeats 2 and 3,pgrepchecks confirmed zero surviving blocker or noisy fixture childrenbash tests/fm-turnend-guard.test.shFocused secondmate tests: teardown sweep, missing-sweep refusal, later-refusal preservation, nested force sweep, nested restoration failure, and failed leased-home returnInitial process-substitution selector failed to locate its helper due toBASH_SOURCE; retried successfully after sourcingtests/secondmate-helpers.shdirectlyManual public-CLI flow:register,reconcile, external completion, result/wake inspection,retire, and zero-claim verificationFinalgit status --shortand transient-artifact cleanup checks✅ **Document** - passed
✅ No issues found.
🔧 **Lint** - 1 issue found → auto-fixed (2) ✅
🔧 Fix: Captain, fix process-event ShellCheck warnings
1 warning still open:
🔧 Fix: Fix nested process-event test quoting
✅ Re-checked - no issues remain.
✅ **Push** - passed
✅ No issues found.