Repository navigation
atomesh: make the fan-out request timeout a CLI option (--worker-request-timeout-secs) - #478
Conversation
WorkerManager's /get_load and /flush_cache requests carried a compiled-in 5 s timeout. It is now --worker-request-timeout-secs, default 5, so a run whose workers answer on a slower clock can raise it. Routing policy code is untouched. The other constant the issue names, DEFAULT_WORKER_HTTP_TIMEOUT_SECS (30 s), is left alone: its client serves only the HTTP health check, and that request sets its own timeout from --health-check-timeout-secs, which replaces the client default. A 40 s health check with a 60 s health timeout succeeds, so the 30 s value bounds no request and an option for it would change nothing. The sync inventory rows that pin cliargs.rs lines move with the new field, the worker_manager.rs row now points at the option, and the worker.rs row states what that client actually bounds. Test: a stub worker that answers after 40 s. Without the option both requests time out at 5.0 s; with --worker-request-timeout-secs 60 both succeed at 40.0 s. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
| pub request_timeout_secs: u64, | ||
|
|
||
| /// Timeout in seconds for the router's own /get_load and /flush_cache requests to workers | ||
| #[arg(long, default_value_t = DEFAULT_WORKER_REQUEST_TIMEOUT_SECS, help_heading = "Request Handling")] |
There was a problem hiding this comment.
Required finding 1: --worker-request-timeout-secs 0 is accepted and silently turns off load reporting and cache flushes. Fix: add value_parser = clap::value_parser!(u64).range(1..) to this #[arg], and pin CliArgs::try_parse_from(["atomesh", "--worker-request-timeout-secs", "0"]).is_err(), which needs no stub and no wall time.
Probe at b28823d9d and why
A zero Duration makes reqwest's total-timeout future ready on its first poll, so every /get_load and /flush_cache request fails before the worker can answer. Scratch probe, not committed and removed: a stub /get_load that answers immediately.
| flag | parse_from |
RouterConfig::validate() |
load |
|---|---|---|---|
--worker-request-timeout-secs 5 |
ok | ok | 7 |
--worker-request-timeout-secs 0 |
ok | ok | -1 |
The option beside it, request_timeout_secs, refuses zero with "Must be > 0" (validate_server_settings in config/validation.rs); this option cannot reach that check, because it is not in RouterConfig. Zero is also the value a user of a simulated launch is most likely to try for "no bound", and it does the opposite (a refusal is due, not a fallback).
| ); | ||
| assert_eq!(loads.loads[0].load, if answered { 7 } else { -1 }); | ||
| assert_eq!(flush.successful.len(), answered as usize); | ||
| assert_eq!(elapsed >= slow, answered); |
There was a problem hiding this comment.
Required finding 2: "defaults unchanged" is pinned only for a default of 40 s or more, because the no-flag leg's only time bound is elapsed < 40 s; the body's M4 row ("a changed default") overstates it. Fix, one line: assert!(answered || elapsed < Duration::from_secs(10)); (the leg measured 5.002 s), or assert CliArgs::parse_from(["atomesh"]).worker_request_timeout_secs == 5. Either reddens on the 5 -> 30 mutant; please record that red.
The mutant
DEFAULT_WORKER_REQUEST_TIMEOUT_SECS: u64 = 5 -> 30, one line, nothing else changed, line counts 849/415:
- published:
1 passed, 45.05 s (no-flag leg 5.002 s) - mutant:
1 passed, 70.05 s (no-flag leg 30.002 s, load-1, 0 flushed)
|
|
||
| /// Timeout for the `/flush_cache` and `/get_load` requests below; set from | ||
| /// `--worker-request-timeout-secs` when the router config is built. | ||
| pub static WORKER_REQUEST_TIMEOUT_SECS: AtomicU64 = |
There was a problem hiding this comment.
Reservation, not blocking: the process-wide static is acceptable and the leaner choice. Suggestion: one comment line on the static naming the ceiling, e.g. "process-wide: the last router config built wins; move into RouterConfig if a process ever builds two."
What the alternative costs, and the static's costs, at this head
Moving the value into RouterConfig would touch config/types.rs, server.rs (two call sites) and app_context.rs (LoadMonitor::new), and change the signatures of flush_cache_all, get_all_worker_loads and LoadMonitor: about 20 lines in 3 more files, no difference in behaviour. Simplicity outweighs a cleaner abstraction at that price.
- It skips
RouterConfig::validate(); finding 1 fixes that at the clap layer. - The last
to_router_configin a process wins. Today there is one caller per process:maininmain.rs, andbuild_server_configinpython.rsonce per launch path inatom/entrypoints/atomesh/server.py. - The test leaves the static at 60 for the rest of the lib test process. No other lib test calls
to_router_configor the fan-out functions, so nothing reads it.
| @@ -2483,27 +2483,27 @@ | |||
| "anchor": "pub const DEFAULT_WORKER_HTTP_TIMEOUT_SECS", | |||
| "category": "C1", | |||
There was a problem hiding this comment.
Reservation, not blocking, and a landing-order note. This row keeps category C1 while its new, correct why says it bounds no request; changing the category changes the audit README counts, outside this PR, so it is recorded on #477. This file conflicts with #476: whichever lands second must keep this PR's file/line/anchor/why on all four rows and add #476's mechanism fields.
Detail
Category: the audit README defines C1 as "a bound that exists to declare something broken: raise it, or switch it off", and counts the row in "five router and server bounds". The why is correct: the client has one use (http_health_check), and reqwest 0.12.28 RequestConfig::fetch returns the request's timeout and falls back to the client default only when the request has none, so it replaces the default rather than taking the minimum.
Conflict: git merge-tree --write-tree 67117f738 b28823d9d reports CONFLICT (content) here. #476 (delivers #454) still has worker_manager.rs:24 const REQUEST_TIMEOUT and cliargs.rs:423/:398 on these rows, and gives this row mechanism: K8. #476 carries need human, so this PR will most likely land first, and #476's base-update merge must do the keeping. If it keeps #476's line numbers, test_anchor_lines_are_still_where_they_say fails with exactly the three messages recorded in this PR's body. K8 on this row needs a second look for the same reason as the category.
|
REQUEST CHANGES at
Checked: the narrowing to one constant, the named result, revert-red M1-M3, the inventory rows, gate 1, the Rust suite, design references and effort; all hold (details). ponytail-review: lean already, no findings (details). Verified
Reservations, in full
Generated with Claude Code |
…lt (#469) A zero timeout made every /get_load and /flush_cache request fail before the worker could answer, and nothing refused it. clap now rejects values below 1. A parse-only test pins the refusal and the default of 5, which the 40 s stub test held only against defaults of 40 s or more. The static's doc comment now says the last router config built in a process wins. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Round 2 pushed
Gate 5281 passed, rc 0, same as control. PR body updated. |
|
Review round 2. Verdict: APPROVE at
Checked: the CPU gate on the head's tree and on the tree that will land (head merged with the tip EvidenceDelta Revert-red at the head. One mutant at a time, serially, with line counts kept (849/429). Each runs the full
CPU gate on node 18
The merged tree's Flake: 🤖 Generated with Claude Code |
- D4 table loses its Sites column; the crosswalk and the count paragraphs go. Each site's mechanism is the `mechanism` field of its row in sync_sites.json (#476). No prose states a site count. - D4, D5 and D9 cite code by path and symbol, not by line; D5's cite-audit bullet is deleted. - D5's K8 table and D9 item 9 state one atomesh bound, the --worker-request-timeout-secs option (#478); the 30 s client default is never reached (#477). The D7 log row takes #448's decision. - D4's zero-lookahead and DP rules keep the decisions and link D3 for the sizing and the critical-path example. - "Mistakes fall on the loud side" is limited to sends and receives; the clock-source lint and validation catch the rest. - D4's TSO handler case drops the rejected-design history. - README: D4 headline row and the doc 01 index row rewritten from the log; the Atomesh and wall-clock Scope rows follow D5; the straggler is checked on receipt; the header states no decision count. - "simulated window" becomes "simulation window", "stall report" becomes "stall diagnostic". Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…ield Conflicts resolved, keeping both sides: - atom/compass/audit/sync_sites.json: the two router rows #478 rewrote take the tip's file, line, anchor and why, and keep the branch's mechanism K8. - atom/compass/audit/sync_scan.py: the tip deleted category_counts with the count test (#492); the branch's counts_by, which replaced it, goes with it. - tests/compass/test_sync_inventory.py: the tip deleted the stated-count test (#492); the branch's extension of it to mechanism tables goes with it. The crosstab test counts mechanisms with Counter instead of counts_by. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
…stale doc-01 statements (#584) Per the owner's ruling of 2026-10-02, doc 11 and doc 01 now say the metrics push and refresh run on simulated time as daemon timers and the traffic LP scrapes /metrics on simulated time; _last_refresh is stamped with simulated time. Doc 01 adds uvicorn's Server.main_loop tick and keep-alive to the daemon deadlines. The three metrics rows of sync_sites.json keep category "ignore" and their original why; mechanism_why carries the daemon wording (K7). Doc 01 (#577): only DEFAULT_WORKER_HTTP_TIMEOUT_SECS is compiled in (DEFAULT_WORKER_REQUEST_TIMEOUT_SECS is the default of a CLI option, #478), and the deleted test_kv_blob_doc_table.py reference now points to test_kv_blob_site.py. The design README summary of doc 11 is updated to match. CPU gate on node 18 at 9ac55a0: 5811 passed, 155 skipped, 3 xfailed, GATE_CPU_RC=0. The merged tree with b645537 (#558, clock files only) passes the inventory tests. Closes #535. Closes #577. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Closes #469. Ready for review. No blocking issues.
The router's bound on its own
/get_loadand/flush_cacherequests to workers was a compiled-in 5 s. It is now--worker-request-timeout-secs(default 5, at least 1). The 30 sDEFAULT_WORKER_HTTP_TIMEOUT_SECSbounds no request, so it stays a constant; the design text that says otherwise is #477.Dev record
WORKER_CLIENTmakes one request,http_health_check, which sets its own timeout; in reqwest 0.12 that replaces the client default.atom/compass/audit/sync_sites.jsonpins atomesh lines by text, so three of its rows move with this change (outside the issue's file set, mechanical).request_timeout(), not inRouterConfig, so the last router config built in a process wins. It skipsRouterConfig::validate(), so clap refuses 0 at parse time.policies::tree::tests::test_tree_structure_integrity_after_stressfailed once in three full lib runs at the head, passed 5 of 5 alone;policies/is untouched./engine_metricsscrape bound (observability, on design: doc 01 names a 30 s atomesh bound that bounds no request (found in #469) #477); an end-to-end launch.Named result
core::worker_manager::tests::worker_request_timeout_option_outlasts_a_40s_worker, a stub worker that answers after 40 s:/get_load/flush_cache--worker-request-timeout-secs 60worker_request_timeout_defaults_to_5s_and_refuses_zeropins the default of 5 and the refusal of 0.Gates
46ae9eb31: 5281 passed, 155 skipped, 3 xfailed,GATE_CPU_RC=0.f87413a7a: 5281 passed, 155 skipped, 3 xfailed,GATE_CPU_RC=0.cargo test --lib: 1110 passed at the head, 1108 at the tip. clippy 81 warnings on both, none in changed lines. rustfmt clean. No Python file changed, so ruff has nothing to check.Evidence
Revert-red at
46ae9eb31, one line-count-preserving mutant at a time, tree restored and compared after each. Node ids are undercore::worker_manager::tests.to_router_configstores the default, not the flagworker_request_timeout_option_outlasts_a_40s_worker,assert_eq!(loads.loads[0].load, ..), left -1 right 7 (option leg 5.001 s)fan_outsite back toDuration::from_secs(5)assert_eq!(flush.successful.len(), ..), left 0 right 1parse_load_responsesite back toDuration::from_secs(5)assert_eq!(loads.loads[0].load, ..), left -1 right 7 (flush 1 at 40.002 s)value_parserrange removed (the attribute as it was atb28823d9d)worker_request_timeout_defaults_to_5s_and_refuses_zero,assertion failed: parse(&["--worker-request-timeout-secs", "0"]).is_err()DEFAULT_WORKER_REQUEST_TIMEOUT_SECS5 to 30worker_request_timeout_defaults_to_5s_and_refuses_zero,assert_eq!(parse(&[]).unwrap().worker_request_timeout_secs, 5), left 30 right 5. The 40 s stub test alone passes this mutant (no-flag leg 30.002 s).Inventory: with the tip's
sync_sites.jsonin the branch tree,tests/compass/test_sync_inventory.py::test_anchor_lines_are_still_where_they_sayfails with "no longer holds" forconst REQUEST_TIMEOUTinworker_manager.rs,pub disable_health_checkandpub disable_circuit_breakerincliargs.rs; with the branch's json, 26 passed. Rows changed: theworker_manager.rsREQUEST_TIMEOUTrow now points atpub worker_request_timeout_secsincliargs.rs; the twocliargs.rsrows changed onlyline; theDEFAULT_WORKER_HTTP_TIMEOUT_SECSrow changed onlywhy. It keeps category C1, which now contradicts itswhy; recorded on #477.reqwest check: a scratch
/healthstub answering after 40 s withhealth_config.timeout_secs = 60gavehttp_health_check() -> ok=trueafter 40.047 s, so the 30 s client default did not apply.Stress-test flake: 3 full lib runs at
46ae9eb31gave 1109 passed / 1 failed (test_tree_structure_integrity_after_stress, "Tenant should have positive size"), then 1110 and 1110 passed. The tip gave 1108 passed twice.Cost vs an estimate of about 30 statements: about 33 statements (production: const, static, fn, CLI field and its
to_router_configstore; tests: 22 statements over two fns), counted by hand because the AST counter is Python-only. Physical lines +109/-13:worker_manager.rs+91/-4 (tests 70 of them),cliargs.rs+11/-2,sync_sites.json+7/-7. The 3.3x physical-to-statement ratio comes from rustfmt's multi-lineuseblocks and the axum builder chain in the stub.🤖 Generated with Claude Code