fix: run registration lifecycle probes serially - #61
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 8, 2026, 12:36 PM ET / 16:36 UTC. ClawSweeper reviewWhat this changesRuns captured registration callbacks sequentially so service startup finishes before stop and dispose, with regression coverage and a changelog entry. Merge readiness⛔ Blocked before merge - 2 items remain The fix remains necessary on current main and v0.3.24. No introduced correctness defect was found, and the supplied lifecycle trace convincingly demonstrates the repair. Priority: P2 Review scores
Verification
How this fits togetherPlugin Inspector captures plugin registrations and exercises their callbacks with synthetic inputs. The probe runner produces compatibility results consumed by reports and downstream tools such as Crabpot. flowchart TD
A[Captured plugin registrations] --> B[Execution profile and opt-in checks]
B --> C[Blocked or metadata result]
B --> D[Order registration callbacks]
D --> E[Await startup then teardown]
E --> F[Results grouped by capture index]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Retain sequential lifecycle execution and stable report output, supported by Crabpot source-mode smoke validation before landing. Do we have a high-confidence way to reproduce the issue? Yes: current-main source invokes startup and teardown concurrently, and the supplied public-API trace demonstrates the race with yielding startup. This read-only review did not execute a reproduction. Is this the best way to solve the issue? Yes: awaiting existing callbacks repairs the ordering defect without adding configuration or a competing implementation; invocation deadlines remain separate work. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 84ede904fd6e. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (18 earlier review cycles; latest 8 shown)
|
084f061 to
541de8e
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Run registerService start, then other callables, then stop/dispose instead of Promise.all so teardown cannot overlap startup. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
541de8e to
c840b7c
Compare
What Problem This Solves
Fixes an issue where users running synthetic probes with
includeLifecyclesee a registered service'sstopanddisposecallbacks run before asynchronousstartfinishes. This can report false failures or tear down partially initialized state.Why This Change Was Made
Await the existing registration invocations one at a time in the order owned by
profile.callableProperties. The service profile already declaresstart,stop, thendispose; the final repair uses that existing order directly.Invocation construction, arguments, opt-in guards, failure records, and
captureIndexgrouping are unchanged. Each callback settles before the next begins; recorded failures still allow later cleanup callbacks to run.Original fix and regression contributed by @SebTardif. The contributor branch and ancestry are preserved, with current main integrated through merge commits. The existing regression now yields during both startup and stop, covering disposal before asynchronous teardown finishes as well.
User Impact
Service lifecycle probes no longer overlap startup and teardown. This orders callbacks within each captured registration; it does not keep a service running around other registrations or establish a live Gateway lifecycle contract.
No new options, exports, dependencies, or timeout behavior are added.
Evidence
f7d1314d6723002fe0c499d3cb0d01be2e1a5205. The registration owner was byte-identical to pre-fix main. Of 359 tests, 358 passed and only the service regression failed. Its result records reportedstop ran before start finishedanddispose ran before start finished.04d7e65763c64ddd012092a65451692ee1cb09fe, integrated with main7cbdafa1a9c3a107c879088ffed98054cf11db5f.npm run checkpassed all 359 tests, including the asynchronous service regression and existing opt-in/sibling coverage, plus the package-contents check.Secret source: Nonewith read-only contents and metadata permissions.git diff --checkpasses.src/synthetic-probes.js,test/synthetic-probes.test.js, and one changelog entry. No profile, capture-runtime, process-boundary, or Gateway changes.The regression exercises the production capture/probe APIs with isolated callbacks. Contributor code was not executed locally, and no AWS lease was created. No live Gateway or hung-callback deadline proof is claimed. Crabpot consumer smoke has not been run in this preparation; that followthrough remains with the consolidated release integration.
Final head
04d7e65763c64ddd012092a65451692ee1cb09feis approved and authorized for squash landing, with the documented downstream consumer followthrough retained for release integration.