hermes: extend scheduled wake timeout - #346
Conversation
|
A fresh concurrency review found a scheduler-level dependency: extending a wake adapter to two minutes is correct, but the current synchronous tick can hold unrelated due invocations behind that wake. #347 now tracks asynchronous, serialized dispatch and overlap protection. Keeping this PR draft; it should not merge until #347 is resolved or incorporated. |
|
Dependency update: draft PR #349 now implements the scheduler fairness boundary required before this timeout extension. Its Go and claw-api image checks are green, including focused race coverage for cross-target concurrency, strict per-target FIFO order, cancellation neutrality, and shutdown draining. Keep this PR draft until #349 is reviewed and landed. |
|
Independent review (claude, via talking-stick turn 3). Answering the question of whether #348 must land before this PR can honestly close #345. Determination: this PR does not, by itself, resolve the incident #345 actually reports — but the gap is closed by #349, not by this PR. #345's reported incident is a manual fire: "A production Hermes schedule fired manually through claw-api ... but claw-api recorded the invocation as
Both fire well before the 2-minute server budget this PR grants What each PR actually contributes to #345:
So: #348 is not a merge blocker for this PR, but the No objection to the code. Diff is minimal and correct, the test now asserts all three budget classes explicitly (openclaw / hermes / generic default) rather than only the two it covered before, and the changelog touches only One interaction worth recording: a 2-minute Hermes budget is longer than many cron cadences, which makes #349's overlap coalescing reachable in normal operation. I found that coalesced slots were audit-invisible and pushed a fix to the #349 branch ( |
Centralizing the longest supported wake budget only helps if something enforces that it is actually the longest. Nothing did: wakeExecTimeout() could hand any adapter a budget above schedulepkg.MaxWakeExecTimeout and the manual fire client, which sizes its request and transport budgets from that constant, would silently go back to cutting a healthy wake short -- the exact failure #348 exists to fix. The risk is immediate, not hypothetical: #346 adds hermes-exec as its own literal. Adds an invariant test over every adapter resolveWakeAdapter can emit, plus a nesting assertion so tuning the 5s margins cannot invert the transport > request > wake ordering. Verified the guard actually bites by temporarily giving hermes-exec a 3-minute budget, which fails it with the drift named explicitly. go test ./... , go vet ./... , git diff --check, and the integration-tag suite all pass.
f42af36 to
a2d54e6
Compare
Summary
hermes-execa dedicated two-minute claw-api wake budgetThis stays in claw-api's adapter-aware wake policy. It does not patch a downstream pod, modify Hermes images, or add cllama behavior.
Verification
go test ./cmd/claw-apigo vet ./...git diff --checkgo test ./...passed every package exceptcmd/clawandinternal/build; those two hit the repository's 10-minute timeout while the host Docker CLI was blocked indocker image inspectDOCKER_HOST=unix:///tmp/clawdapus-no-docker.sock go test ./cmd/claw ./internal/buildpassed both affected unit packages deterministicallyCloses #345