fix: bounded concurrency and thread-safe digest for slow devices - #752
fix: bounded concurrency and thread-safe digest for slow devices#752amarnath-ac wants to merge 1 commit into
Conversation
a856b6e to
e55874d
Compare
|
Console working logs: {"level":"info","time":"2026-07-24T10:36:24+05:30","caller":"/home/ac/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.0.linux-amd64/src/fmt/print.go:263","message":"[GIN] 2026/07/24 - 10:36:24 | 200 | 923.98ms | 127.0.0.1 | GET "/api/v1/amt/power/state/e51c2a64-4cb7-4fb1-b55b-2ee4af0597c9""} {"level":"info","time":"2026-07-24T10:36:39+05:30","caller":"/home/ac/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.0.linux-amd64/src/fmt/print.go:263","message":"[GIN] 2026/07/24 - 10:36:39 | 200 | 93.79µs | 127.0.0.1 | GET "/api/v1/server/features""} {"level":"info","time":"2026-07-24T10:36:46+05:30","caller":"/home/ac/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.0.linux-amd64/src/fmt/print.go:263","message":"[GIN] 2026/07/24 - 10:36:46 | 200 | 632.229µs | 127.0.0.1 | GET "/api/v1/devices/e51c2a64-4cb7-4fb1-b55b-2ee4af0597c9""} {"level":"info","time":"2026-07-24T10:37:03+05:30","caller":"/home/ac/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.0.linux-amd64/src/fmt/print.go:263","message":"[GIN] 2026/07/24 - 10:37:03 | 200 | 233.62ms | 127.0.0.1 | POST "/api/v1/amt/power/action/e51c2a64-4cb7-4fb1-b55b-2ee4af0597c9""} {"level":"info","time":"2026-07-24T10:37:18+05:30","caller":"/home/ac/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.0.linux-amd64/src/fmt/print.go:263","message":"[GIN] 2026/07/24 - 10:37:18 | 200 | 794.89ms | 127.0.0.1 | GET "/api/v1/amt/power/state/e51c2a64-4cb7-4fb1-b55b-2ee4af0597c9""} {"level":"info","time":"2026-07-24T10:37:30+05:30","caller":"/home/ac/go/pkg/mod/golang.org/toolchain@v0.0.1-go1.25.0.linux-amd64/src/fmt/print.go:263","message":"[GIN] 2026/07/24 - 10:37:30 | 200 | 48.666µs | 127.0.0.1 | GET "/api/v1/server/features""} |
28d4d0e to
4f11c90
Compare
amarnath-ac
changed the title
b875897 to
51c4dbe
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses console timeouts and AMT auth lockouts on slow/offline devices by making the WSMAN HTTP client more resilient under parallel load (bounded concurrency, connection reuse, and thread-safe Digest state).
Changes:
- Introduces per-target bounded concurrency for
Postand aligns HTTP transport limits to reduce ME connection flooding. - Adds mutex-protected Digest challenge handling plus a “prime” single-flight to avoid parallel cold-wake handshakes.
- Adds transient transport retry logic and enables keep-alives to avoid repeated slow TLS handshakes.
d0c1ee1 to
d839cd7
Compare
1. Cap concurrent Posts per device at 7 (matches AMT ME capacity). 2. Single-flight the first authentication so only one Post wakes a sleeping device; concurrent siblings wait, then flow once primed. 3. Serialize digest challenge access under a mutex so concurrent Posts get monotonic nonce-counts, reset the counter when AMT issues a fresh nonce to avoid auth lockout. 4. Retry transient transport failures, rebuilding the digest header on each attempt. Signed-off-by: C, Amarnath <amarnath.c@intel.com>
|
@amarnath-ac : Can you add evidence ( in the form of logs ) to showcase the failures that you have mentioned occuring and post the fix how this is getting handled. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
pkg/wsman/client/wsman.go:86
hostSems/hostSemsMuintroduce package-level mutable global state to share semaphores across Targets. This makes concurrency limiting implicit and unbounded (every unique endpoint string is retained forever), which can grow memory over time in long-running processes and violates the repo guideline to avoid package-level mutable state.
Consider making the limiter explicit and lifecycle-managed (e.g., add an optional semaphore/limiter field to client.Parameters or a small Limiter interface, so callers that create multiple clients for the same device can share it without a global map; default to a per-Target limiter when not provided).
// hostSems limits concurrency per device, shared across all Targets for a host.
var (
hostSemsMu sync.Mutex
hostSems = make(map[string]chan struct{})
)
pkg/wsman/client/wsman.go:136
- The
deadlinebudget inretryOnTransientis only checked before each retry attempt. Since eacht.Do(...)call can still block up tot.Timeout(30s) independently, the total time holding the per-host semaphore can exceed the intendedtimeoutbudget (and potentially approachtimeout * (retries+1)).
To enforce a true overall budget, create a request context with the shared deadline/timeout in Post, apply it to the initial request, and reuse the same context for all retry requests (e.g., pass a context.Context into retryOnTransient and call req.WithContext(ctx) on each attempt).
for attempt := 1; attempt <= maxTransientRetries; attempt++ {
// All attempts share one budget so a Post cannot hold its slot for
// maxTransientRetries * timeout.
if time.Now().After(deadline) {
return nil, lastErr
}
sudhir-intc
left a comment
sudhir-intc
left a comment
There was a problem hiding this comment.
Changes LGTM with some comments to be updated.
| @@ -153,9 +254,9 @@ func NewWsman(cp Parameters) *Target { | |||
| } | |||
|
|
|||
| res.Transport = &http.Transport{ | |||
There was a problem hiding this comment.
Please add a comment here for the DisableKeepAlives: false so that in future it may not be turned to try.
| } | ||
|
|
||
| const timeout = 10 * time.Second | ||
| // hostSems limits concurrency per device, shared across all Targets for a host. |
There was a problem hiding this comment.
| // hostSems limits concurrency per device, shared across all Targets for a host. | |
| // hostSems stores process-wide semaphores keyed by endpoint host:port. | |
| // Each semaphore caps concurrent WSMAN Posts to that endpoint. |
| hostSems = make(map[string]chan struct{}) | ||
| ) | ||
|
|
||
| // hostSemaphore returns the shared concurrency limiter for an endpoint. |
There was a problem hiding this comment.
| // hostSemaphore returns the shared concurrency limiter for an endpoint. | |
| // hostSemaphore returns the shared per-endpoint concurrency limiter. | |
| // The same host:port gets the same semaphore across all Target instances. |
| WSManPath = "/wsman" | ||
| timeout = 30 * time.Second | ||
| idleConnTimeout = 90 * time.Second // keeps sockets warm as long as AMT holds them open | ||
| maxConcurrentPosts = 2 // caps simultaneous WSMAN Posts to match AMT firmware capacity |
There was a problem hiding this comment.
| maxConcurrentPosts = 2 // caps simultaneous WSMAN Posts to match AMT firmware capacity | |
| maxConcurrentPosts = 2 // Keep at 2 to avoid overload during powered-off wake paths; higher concurrency caused transient auth/transport failures. |
| lastErr := origErr | ||
|
|
||
| for attempt := 1; attempt <= maxTransientRetries; attempt++ { | ||
| // All attempts share one budget so a Post cannot hold its slot for |
There was a problem hiding this comment.
| // All attempts share one budget so a Post cannot hold its slot for | |
| // Soft overall budget guard: stop scheduling new retry attempts once the shared | |
| // deadline has passed. In-flight t.Do calls can still run until client/request | |
| // timeout, so this limits additional retries rather than hard-capping total wall time. |
There was a problem hiding this comment.
Correct the comment here.
|
Tested these changes along with the console PR#device-management-toolkit/console#1153 and here's my observations
With the fixes: Without fixes |
Issue 1082: Device Details Page Does Not Load When Device Is Powered Off or Sleeping.
Problem statement:
When an AMT device is in Soft Off or Sleep state (no CIRA), the Device Details page fails to load, resulting in an infinite spinner / 504 errors.
Root cause:
When the device is powered off or sleeping, AMT firmware can only service a couple of WSMAN requests at a time. The Device Details page fires ~11 at once, so most of them queue inside the firmware until they time out or their TLS connections get dropped. Powered on, the firmware handles the same burst fine, which is why the page only fails in the off state. Parallel requests also shared one digest auth object without locking, corrupting each other's auth state.
Fix:
protect shared digest auth with a mutex, reuse connections so the slow handshake happens only once, and limit concurrent requests per device.
Changes added: