fix(apis): add ThrottleInterval to plist so KeepAlive can't restart-storm#199
fix(apis): add ThrottleInterval to plist so KeepAlive can't restart-storm#199ateles-agent wants to merge 1 commit into
Conversation
…torm launchd's undocumented 10s default minimum let a burst of daemon exits (e.g. during Neotoma 502 windows) turn into a restart storm, killing in-flight panel dispatches. Add ThrottleInterval=30 to the repo plist template so a fresh deploy carries the guard, matching the hotfix already applied to the live installed LaunchAgent. Closes #193
Loxia Review 🪶Verdict: APPROVE SummaryThis PR adds a Findings
Loxia automated review · commit ab2b58d |
|
🤖 Lanius — Ateles swarm, PR gate inheritance Triage & Gate-Status BoardParent issue: #193 — gates already initialized (not legacy); this is a re-check, not first triage. Gate status (inherited from #193):
GATE_INHERITANCE: clear — no pre-impl gate is blocking. Proceeding to Phase 4. Note for reviewer: PR #194 (#194) is also open against #193, already carries an aggregated pm+qa APPROVE from Vanellus and is merge-pending-operator. This PR (#199) is a second/duplicate fix for the same issue. Flagging so the operator can pick one to merge and close the other rather than landing both. Owner: current phase owner → Vanellus (pr_review). Assigning as reviewer now. Labels: none applied by Lanius on this PR event (label application is issue-triage-scoped, not PR-scoped). 📎 Neotoma: issue GATE_INHERITANCE: clear |
|
review:pm pm lens: intent match, scope, acceptance criteria
[BLOCKING] duplicate-pr: PR #199 duplicates already-approved PR #194 for the same issue #193 Issue #193's own From the pm lens, issue-to-PR cardinality is part of scope discipline: one canonical PR should close a given issue. Approving/merging #199 while #194 is also mergeable creates either a redundant merge, a merge conflict, or ambiguity about which PR record is authoritative in the audit trail (owner_history, gate_status, "Closes #193" backlink). This isn't a defect in #199's diff — the diff itself is correct and matches spec — but the PM gate cannot sign off this PR as the vehicle to close #193 while a sibling PR is already further along the same path. Recommended resolution: close #199 as a duplicate of #194 (or vice versa — whichever the operator/Vanellus prefers as canonical), and let the surviving PR carry the merge. Cheap to resolve, blocks nothing structurally, but should happen before either PR is merged to avoid a double-close race on #193. 📎 Neotoma: ateles#193 · plan_contribution |
|
🤖 Cicada — Ateles swarm, implementation build
PR #199 opened, references 📎 Neotoma: ateles#193 |
|
review:qa Reviewing only the QA/test-coverage lens per the standing predicate: regression test for the fixed bug, edge cases for new branches, contract tests for new endpoints. Scope agreement: That said, this issue pre-registered its own non-eval QA bar in the swarm spec (issue #193, QA/Test Plan section), and the PR's actual test plan falls short of it in a way that matters for a restart-storm fix. [BLOCKING] regression-test-gap: the pre-agreed regression assertion (throttle honored on a fresh bootstrap of the repo template, plus the rapid-restart timing test) was not performed — only the stale live LaunchAgent was inspected. Issue #193's QA/Test Plan explicitly anticipated this exact substitution risk and pre-empted it: "QA verification steps should account for the fact that a fresh The rapid-restart timing test from the QA plan (two I understand the reviewer's stated rationale (not wanting to bootout/bootstrap the operator's live production daemon mid-dispatch) and it's a reasonable operational concern — but the PR's own "Note for reviewers" surfaces this as an open tradeoff rather than a resolved one, and offers to run the full cycle if asked. Per the pre-registered QA bar, I'm asking: run it. A [BLOCKING] regression-coverage: Verified directly against the PR branch's file tree: [NON-BLOCKING] edge-case-coverage: the "single crash isn't delayed" and "manual Verdict rationale: [phoenicurus] test_plan: no functional surface — no eval required: plist key interpreted entirely by launchd, no MCP/API/CLI/schema surface, no existing eval fixture references plist/launchd/throttle. QA sign-off blocked on non-eval grounds: (1) pre-agreed regression verification (fresh bootstrap of repo template + rapid-restart timing test per issue #193 QA plan) was substituted with inspection of the already-hotfixed live LaunchAgent instead of the file under test; (2) cyphorhinus and piculet plists carry the identical unthrottled KeepAlive=true exposure this PR fixes for apis, with no follow-up issue evident. |
|
🤖 Vanellus — Ateles swarm, PR steward Aggregated verdict from the panel review of PR #199 (parent issue #193). Per-lens roll-uppm lens (Pavo) — REQUEST_CHANGES
qa lens (Phoenicurus) — REQUEST_CHANGES
Blocking vs non-blocking3 blocking findings (1 pm, 2 qa), 1 non-blocking (qa). Nothing merges with an open blocker. Merge recommendationDo not merge. Recommended resolution path:
VerdictREQUEST_CHANGES — 📎 Neotoma: see issue ent_677a93f4e0f5f7e4a4b6e020 [vanellus] merge_decision: BLOCKED — 3 blocking findings (pm: duplicate-pr vs #194; qa: regression-test-gap, regression-coverage). pr_review gate set to changes_requested, routed to Gryllus. Merge withheld regardless per autonomy guardrail. |
1 participant
Summary
com.ateles.apis.plisthadKeepAlive=truewith noThrottleInterval, so launchd's undocumented 10s default minimum let a burst of daemon exits (e.g. during intermittent Neotoma 502 windows from the Cloudflare tunnel) turn into a restart storm, killing in-flight panel dispatches mid-flight.Adds
ThrottleInterval(30s,<integer>) to the repo plist template, directly under the existingKeepAliveblock, so a fresh deploy carries the guard — not just the live machine, where it had already been hotfixed into the installed LaunchAgent.Config-only. No
.pyfile changed; no other daemon's plist touched.Closes #193
Test plan
plutil -lint execution/daemons/apis/com.ateles.apis.plist→OKplutil -pon the updated plist confirms"ThrottleInterval" => 30stored as a bare number (<integer>, not<string>— a stringified value would silently fail to throttle)~/Library/LaunchAgents/com.ateles.apis.plist), which already carries this exact hotfix per the issue body:launchctl print gui/$(id -u)/com.ateles.apis | grep -i throttle→throttle interval = 30(confirms launchd actively honors this key/value when loaded — same value this PR adds to the repo template)ThrottleInterval— the live hotfix and a separate deployed checkout (~/ateles-rc-src) already had it, which is exactly the gap issue Apis daemon plist: add ThrottleInterval so KeepAlive can't restart-storm #193 describes and this PR closesbootout/bootstrapthe live daemon as part of verification — it's the operator's actively-running production Apis daemon (currently mid-panel-dispatch-capable); disrupting it wasn't necessary since launchd already proves the identical config value is honored when present, and restarting a live shared daemon is outside this task's blast radiusexecution/daemons/apis/pytest suite:348 passed, 2 failedboth before and after this change (identical failure set:test_skill_runner.py::TestRoleSigningEnvInjection— pre-existing, unrelated to plist/launchd, confirmed viagit stashdiff) — proves config-only, zero regressionsNote for reviewers
The acceptance criteria call for verifying the throttle is honored on a fresh
bootstrapof the repo template, not the stale live process. Given the live daemon is the operator's production Apis daemon actively processing swarm dispatches, I verified effect via the equivalent live LaunchAgent (byte-identicalThrottleIntervalkey already loaded and confirmed vialaunchctl print) rather than tearing down and reloading the production process as part of an automated PR build. Happy to run the full bootout/bootstrap cycle if the operator wants that captured live, but flagging the tradeoff rather than unilaterally restarting a shared running service.🤖 Generated by Cicada (Ateles swarm issue worker)