fix(nomad): bound control requests with durable registration recovery - #1916
Conversation
Nomad used a pooled HTTP client with no request deadline. Finite calls such as job inspect and regions.list could hang forever when the API accepted TCP but never finished the JSON body. Give those control calls a 2m request context, pass context through Regions, and set ResponseHeaderTimeout on the default transport. Leave Client.Timeout at 0 so AllocationExec streams can outlive the control budget. Signed-off-by: Sebastien Tardif <sebtardif@ncf.ca>
|
🦞👀 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. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 12, 2026, 3:40 AM ET / 07:40 UTC (Revision 2). ClawSweeper reviewWhat this changesBounds Nomad control requests and records durable recovery claims so interrupted job registration remains discoverable and safely recoverable. Merge readiness✅ Ready for maintainer review This repair remains necessary on current main and v0.57.0. The revised head addresses the previous registration-recovery finding and both requested follow-ups; no new blocking defect was found. Priority: P2 Review scores
Verification
How this fits togetherCrabbox’s Nomad adapter translates CLI lifecycle commands into scheduler requests and stores local lease claims. Those claims connect registration, readiness, command execution, and ownership-checked cleanup. flowchart TD
A[Run or warmup] --> B[Persist registration identity]
B --> C[Bounded Nomad request]
C --> D{Registration outcome}
D -->|Acknowledged| E[Check allocation readiness]
D -->|Uncertain| F[Retain recovery claim]
F --> G[Ownership checked cleanup]
E --> H[Execute or retain lease]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep request bounds and registration reconciliation inside the Nomad adapter, using shared durable claim transactions while preserving legacy claims and established exec streams. Do we have a high-confidence way to reproduce the issue? Yes, source establishes a concrete path: a stalled JSON response can leave main’s finite Nomad calls unbounded, and Regions ignores caller cancellation. This review did not execute a current-main reproduction. Is this the best way to solve the issue? Yes. Adapter-local request contexts preserve stream behavior, and durable identity plus guarded reconciliation addresses ambiguous registration without introducing a global HTTP timeout or competing claim store. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 159d54e4499d. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Bound finite Nomad JSON requests without imposing a whole-request timeout on established exec streams. Persist registration identity before submission, retain uncertain outcomes for ownership-checked recovery, and preserve primary failure codes and existing setup rollback semantics. Reuse the shared claim transaction and run-session owners. Cover accepted-but-delayed registration with a permanent HTTP regression and actual CLI timeout/cancellation proof; keep provider docs and Unreleased notes current. Co-authored-by: Sebastien Tardif <sebtardif@ncf.ca>
steipete
changed the title
clawsweeper
Bot
added
rating: 🐚 platinum hermit
|
Merge verification update for 94058fa: The fresh, actual-merge focused race/static/docs/build gates passed, followed by actual CLI runs against a synthetic Nomad HTTP service. The normal registration timeout and interrupted-run cases both preserved exit 1; registration recovery, pending-claim retention, later owned cleanup, and timing/session output passed. All eleven HTTP requests/handlers and five CLI processes finished, with no remaining fixture claims. This is real CLI/HTTP transport proof, not a hosted Nomad scheduler or cloud-driver smoke. Post-merge CI was not entirely green: https://github.com/openclaw/crabbox/actions/runs/34682158837 failed in the existing POSIX workspace-owner renewal fixture under coverage. Its three-second wall-clock lease reported EXPIRED; the log does not identify the exact scheduling or lock delay. That fixture does not call Nomad or the shared acquisition finalizer. I preserved the failed run and prepared the test-only timing repair at #2138. The follow-up merged at 751f6a4 after full CI passed, and fresh actual-merge normal/coverage/race fixture tests also passed. Production renewal/expiry policy is unchanged; this does not claim to fix #1712. CI closeout is complete through that follow-up: all 12 main CI jobs, including coverage, passed at https://github.com/openclaw/crabbox/actions/runs/34685602834; the five connector and four CodeQL analysis jobs also passed. Source comparison from the Nomad merge to that verified main commit confirms the Nomad/shared provider code and CLI production code are unchanged; the only change in those components is the repaired test fixture. The original failed run has not been rerun or presented as successful. No hosted Nomad proof or serving-version/deployment claim is implied. |
Control only the positive fixture's protocol clock and synchronize on actual child startup. Verify renewal extends persisted expiry past the initial deadline while the real witness continues streaming and preserves exit 23. Bound test subprocess cleanup and identify the task-owned detached daemon before signaling it. Production owner policy and negative coverage are unchanged. This repairs the timing-sensitive fixture observed after #1916; it does not claim to resolve #1712. Repeated focused normal, coverage, and race tests passed locally; full CI, connector, CodeQL, and release checks passed for the exact PR head. Managed Codex review was P0 scoped-clean.
2 participants
Summary
Complete the Nomad control-request timeout repair with durable registration recovery. All eight finite JSON operations have a two-minute ceiling while retaining earlier caller deadlines. Regions discovery now carries cancellation. Default transports bound response-header waits to 30 seconds without adding a whole-request client timeout to established exec streams; injected client settings stay unchanged.
The original write-timeout concern is addressed, not omitted: a registration can be accepted even when its response is lost. Crabbox now durably records the exact job, lease, scope, and registration phase before dispatch. An unresolved submission is not discarded on the first 404. Matching owned observations use the existing guarded cleanup path, and setup failure still rolls back regardless of
--keep.Ownership and compatibility
The provider uses the existing atomic claim writer, durable compare-and-swap transactions, and shared run finalizer. A small explicit recovery carrier lets acquisition errors report a kept session without authorizing a second cleanup, running a command, or applying normal Keep policy to failed acquisition. Ordinary ID-only acquisition errors and other providers retain their behavior.
Primary failure codes and causes are preserved even when cleanup adds another typed error. Warmup diagnostics retain job/lease identity; Run session output and its single timing record retain recovery identity. Pending status is not readiness. Existing claims without registration markers keep their confirmed-job behavior, and unknown new marker versions fail closed.
The original complete setup-metadata check is retained using a key list and fingerprint, without storing arbitrary custom metadata values in the new claim. Normal ready-lease cleanup retains its prior reserved-ownership checks. Registration is not automatically retried, and elapsed time alone does not prove that an uncertain submission was rejected.
Head
ab884aee666ffddda115286ea2991e1ee0d05b0a, tree1f8181b15b40c1ea41906eab4f91d3302c2e7254, integrates main159d54e4499d2a2661d6dc08aedb5b2168f59711and preserves the original contributor commit. Thanks @SebTardif; contributor credit and the Unreleased entry are included.Verification
Actual CLI over real HTTP
A separately frozen CLI harness used isolated configuration/state and an owned loopback service. It stored the originally submitted job and ownership metadata unchanged; no credentials, real Nomad scheduler, driver, remote job, or command were used.
The five CLI calls made 11 HTTP requests. All 11 request-handler threads joined, all CLI processes were reaped, both listeners stopped, and no claims remained. The final test-only addition rebuilt a byte-identical CLI, so this execution remains applicable by binary identity; it is not presented as an additional run.
Earlier evidence is preserved: the first package run hit a proof-profile Unix-socket restriction, corrected only with a task-owned socket allowance and outside-prefix denial controls; an earlier CLI run met its nonzero-exit checks but exposed exit-code precedence 5 instead of 1. A regression reproduced nine failures before the correction, and the strengthened CLI assertions then passed.
Limits and remaining gate
This proves actual CLI/client transport and local durable recovery behavior against a synthetic service, not live scheduler or default-Docker behavior. No multi-minute real allocation exec session is claimed; its unchanged established-stream path is supported by source and configuration checks, with startup HTTP discovery documented separately. Current-head GitHub CI and review disposition remain to be checked before landing. No release, fleet update, or production Nomad operation is included.