Conversation
The wait for a free HTTP runspace before a request is shed with 503 was a hardcoded TimeSpan.FromSeconds(30) at both checkout sites in PowerShellRunnerService. Surface it as Worker:HttpQueueTimeoutSeconds with a CRAFT_HTTP_QUEUE_TIMEOUT env override, resolved once via CraftHostBuilderExtensions.ResolveHttpQueueTimeout (env > setting > built-in 30s default), mirroring how MinThreads resolves. This is a load-shedding bound, not a capacity knob.
The rate limiter set Retry-After on a throttled request but logged nothing, so every 429 went out invisible to operators. Emit a warning from OnRejected naming the partition the limit fired for (authenticated principal, else client address) plus the method, path and Retry-After. The logger is resolved per rejection (service registration has no built provider yet) and any logging failure is swallowed so it can never turn a throttle into a 500.
…limiter Add an optional per-client concurrency cap on app-only API callers so one automation cannot hold every runspace at once and starve the interactive UI, which is never capped. Config RateLimit:ApiConcurrencyLimit (env override CRAFT_API_CONCURRENCY_LIMIT), 0 = off by default. Because the limiter's lease spans the whole downstream pipeline, a permit covers both the wait for a runspace and execution — so the cap counts in-flight and queued-for-a-worker requests alike. Over-limit is rejected immediately with 429 (QueueLimit 0), reusing the existing Retry-After + log path. Callers are classified by a new, tested CallerClassifier (idp=aad plus a GUID AppId principal). The per-client rate limiter and the concurrency cap are built as chained partitioned limiters, and the middleware now runs whenever either is active.
The image bakes a DOTNET_GCHeapHardLimit sized for the smallest tier, and the CLR consumes it before any managed code runs - so larger tiers were stuck with the small tier's heap cap. Let a SkuProfile carry an optional GCHeapHardLimitMB and apply it on the matched profile at startup through AppContext.SetData + GC.RefreshMemoryLimit (.NET 8), under the same best-effort contract as pool sizing: any refusal logs and keeps the baseline rather than failing startup.
…alse-failing jobs
Under a pegged GC hard limit, an OutOfMemoryException thrown while logging inside
the JobQueuePump and JobManager catch-all blocks escaped ExecuteAsync. With the
host's default BackgroundServiceExceptionBehavior.StopHost that faults the
service and restarts the container mid-run: dispatch stops with work still
queued, and the pending backlog waits for the restart to reclaim its leases.
Guard those two log calls the way BackgroundTaskLimiter already guards its own
("logging is never worth the loop") so an allocation failure in logging can no
longer take the host down.
Separately, jobs still in flight when a shutdown began finished their work and
persisted their data, then threw ObjectDisposedException enqueueing their
terminal status because OrchestratorStatusWriter._signal was already disposed --
so a task that fully succeeded was recorded as Failed. Route every drain-loop
wake through a guarded Signal() that drops the wake once disposed. A lost
coalesced status wake on the way down is harmless; a succeeded task marked Failed
is not.
Introduce opt-in startup telemetry with configurable endpoint, app ID, timeouts, and jittered delays. Register a hosted service that emits one guarded boot report per instance using persisted storage state, and add version stamping plus native catalog injection so reports include meaningful build and surface metadata.
…ing the host A System.Threading.Timer callback that throws takes the whole process down. The per-run 60s maintenance tick (LogRunStatus / RedrivePendingTasks / CheckRunCompletion) had no guard, so a transient error — a dependency disposed during shutdown, a race on run state — would crash the host instead of being logged and retried on the next tick. Wrap the callback in try/catch (null-safe logger).
Contributor
|
Heads up that this removes a capability we currently rely on: today we can raise the heap on a specific instance by setting DOTNET_GCHeapHardLimit as an app setting and that always wins because env is the only source. With this change, a matched profile's GCHeapHardLimitMB overrides the env var via AppContext and since the code can't distinguish a deliberate per-instance app setting from the image-baked default, manual overrides silently stop working the moment we put GC values in the fleet profiles. Can we add an explicit escape hatch, e.g. CRAFT_GC_HEAP_LIMIT_MB that wins over the profile value, following the same pattern as CRAFT_API_CONCURRENCY_LIMIT and CRAFT_HTTP_QUEUE_TIMEOUT? That keeps per-instance tuning possible without editing fleet-wide SkuProfiles config. |
The SkuProfile GC limit lands through AppContext + GC.RefreshMemoryLimit, which the runtime treats as precedence over DOTNET_GCHeapHardLimit (it won't re-read that env var on refresh). So a fleet-wide profile value silently countermands a heap limit an operator hand-set on a single instance. Add CRAFT_GC_HEAP_LIMIT_MB as the per-instance escape hatch, mirroring CRAFT_API_CONCURRENCY_LIMIT and CRAFT_HTTP_QUEUE_TIMEOUT: when set it wins over the matched profile. A positive value sets the cap; 0 disables it entirely, refreshing to the container's own memory allowance. Unset/negative/unparseable defers to the profile (unchanged behaviour). Disabling only ever raises the limit, so it cannot trip RefreshMemoryLimit's below-committed-heap guard; any refusal logs and keeps the baseline, same best-effort contract as pool sizing.
Contributor
Author
|
@KelvinTegelaar addressed with 363921f |
Add a shared PipelineExecutionContext helper that captures a clean execution-context baseline during worker initialization and restores it during per-invocation cleanup. This prevents AsyncLocal values from leaking between invocations on ReuseThread workers while preserving runspace session state, and adds focused tests covering worker behavior, thread-reuse leak reproduction, and reset strategy validation/benchmarking.
Introduce a second `SkuProfiles` matrix (`SkuProfilesAlt`) that is selected by configurable env-var presence, with fallback to the default matrix when the alt list is empty. Add per-instance `CRAFT_HTTP_POOL_SIZE` and `CRAFT_BG_POOL_SIZE` overrides that apply last and win over profile/baseline values, and align GC heap semantics so profile `GCHeapHardLimitMB=0` disables the cap (null/negative remains no-op). Update example config comments and expand tests for matrix selection, env overrides, and GC zero behavior.
…queue deletes Four in-process table-IO improvements to the background orchestrator; no durable schema change. - R1 (latency): JobQueuePump now waits on an enqueue-driven wake signal instead of only its Task.Delay poll tick, so a quiet-system orchestration no longer waits up to the pump's backed-off idle interval (<=10s) before its first task is claimed. The poll stays as the backstop. Measured cold time-to-first-work: ~2.5s median / 7s worst-case -> sub-second. - R5 (efficiency): renew a claim's lease only in its last third rather than re-reading every in-flight row every tick. The lease outlives a normal task, so most claims never renew at all. - R6a (efficiency): batch the pump's finished-row deletes into one transaction per partition (new ICraftTableStore.DeleteBatchAsync; default = per-row loop, AzureTableStore override batches), replacing two point deletes per task. - F5 (reliability): the batch-upsert per-entity fallback no longer silently swallows a poison row -- it logs the dropped row, so a task that vanishes at creation is visible rather than surfacing only later as a run that never finalizes. Verified: full suite 675/675 green against Azurite; adds a pump wake-signal test.
CompleteTaskAsync (atomic mark-terminal + counter-decrement in one conditional transaction) had no production callers: the live fan-out path decrements via the batched status writer + DecrementRemainingAsync, a deliberate throughput choice, with ReconcileRemainingAsync as the lost-decrement backstop. Keeping the unused primitive implied an atomicity guarantee the hot path does not actually use. - Delete CompleteTaskAsync. - Rewrite the counter-section comment to describe the real design: the partition is shared so the CANCEL path (CancelPendingTaskAsync, still live) can ride one atomic transaction, while the fan-out path decrements separately by design. - Drop the three tests that exercised only CompleteTaskAsync; repoint the two that used it incidentally onto DecrementRemainingAsync (the live method). Build + full counter/cancel suite green.
…ical path (R2) Per-task StoreResultAsync was a single-entity upsert awaited while the JobManager slot was held — the un-batched O(N) write that leaves the BG pool ~93% idle under child fan-out (gated by table write throughput, not workers). Small results (those that fit one Azure Table property, <=30k chars) now ride the coalescing status writer, which writes them BEFORE the run's terminal task markers in the same flush. So a result is durable before its task is counted done -> before the counter decrements -> before finalize/post-execution reads it. On a rare result-write failure the writer WITHHOLDS that run's terminal markers too, so a task can never be counted done with its result lost. Large/chunked results keep the directly-awaited StoreResultAsync path unchanged. - OrchestratorStatusWriter: new _pendingResults + TryQueueResult; FlushOnce writes results first and holds failed-result runs' task/run markers; Requeue restores results. - OrchestratorTableStore.WriteResultBatchAsync: batched single-property result rows. - OrchestratorService.BuildTaskWork: small -> TryQueueResult, else await StoreResultAsync. - App:Orchestrator:BatchResultWrites (default true) gates it; false = original per-task write. Verified: full suite 675/675 green (the end-to-end result/finalize/post-exec tests run through the coalesced path by default); adds durability + threshold tests.
…igration (R4)
The queue RowKey embedded the enqueue tick ({ticks}-{run}-{task}), so re-dispatching a
task (crash recovery, orphan re-drive) wrote a SECOND row instead of updating the first
-- the documented cause of a task being claimed and executed 2-6x.
Schema v2 keys each row deterministically as {run}|{task} (both components escaped so the
key stays legal and the separator unambiguous), and moves the enqueue time to a QueuedUtc
property for age/status reporting. Re-enqueuing a task now upserts its one row.
Migration: a single forward pass at startup (MigrateSchemaAsync, bumped from the v1 index
backfill) re-keys every legacy row -- preserving bucket, priority, claim state and time --
new-key-first then old-key-deleted, so a crash mid-pass re-runs and converges. No dual-read;
after the pass only the new scheme is used. The pump now awaits InitializeAsync before it
claims (as the enqueue paths already do), so no row is claimed while the migration is only
half-applied.
Trade: within a priority bucket Azure now returns rows in run|task order, not oldest-first.
Priority still orders across buckets, and a fan-out enqueues together, so only sub-priority
FIFO fairness is given up -- cheap next to never running a task twice.
Tests updated for the new guarantees (idempotent re-enqueue; priority-first; claim-order-
agnostic status view) + a migration re-key test. Full suite 676/676 green against Azurite.
…b endpoints
Adds a durable-queue clear as a bridge method and removes CRAFT's HTTP job/run
endpoints, whose data is already exposed via bridges for downstream apps to wrap.
- JobQueueStore.ClearAllAsync: empty the durable queue (queue + index rows, keeping the
schema marker), streamed in bounded windows. A maintenance/reset primitive -- drops the
backlog, not in-flight work; pair with cancel to actually stop work.
- OrchestratorService.ClearQueueAsync delegates to it.
- WorkerMetricsBridge.ClearQueue() exposes it to PowerShell, alongside the existing
CancelRun/CancelJob/ChangePriority/GetSnapshot/GetSummary/GetJobDetails/GetRunSummaries.
- Remove JobEndpoints.cs (GET /API/jobs/{summary,allocation,runs,list}, POST /API/runs/cancel)
and its registration: CRAFT is the framework and exposes these as bridge methods; a
downstream app wraps whichever it needs into its OWN endpoints.
- perf-harness becomes exactly such a downstream wrapper: PerfApi's Invoke-PerfAllocation
wraps WorkerMetricsBridge.GetSnapshot() into the allocation shape (+ memory, for the
upcoming OOM-resilience harness); run-orch.ps1/run-e2e.ps1 point at /API/PerfAllocation.
Full suite green.
…rvival under heap pressure run-oom.ps1 brings CRAFT up under an optional GC heap hard limit (CRAFT_GC_HEAP_LIMIT_MB), enqueues a massive fan-out of tasks that allocate a large-object (LOH) buffer and hold it, and tracks peak heap, task completions/failures, dispatch progress (stall detection), and process survival. PerfApi gains Invoke-PerfAllocation / Invoke-PerfRuns (downstream wrappers over WorkerMetricsBridge) and a large-object task mode (allocmb/holdms on Push-PerfBg). Findings (perf-harness/oom-analysis.md): - Memory is bounded by the buffer, not the backlog: a 20,000-task fan-out peaks at ~64 MB managed heap (baseline 15 MB) and drains fully — heap does not scale with N. - A GC heap hard limit is back-pressure, not a crash, while the live set fits: the GC collects/throttles to stay under the cap (even 90 MB/task under a 160 MB limit completes). - Catchable task OOMs are caught (task -> Failed) and the dispatch loop keeps dispatching. - A FATAL runtime OOM (GC hard limit exhausted for the runtime's own allocation) is uncatchable: .NET FailFasts, exit 139, and it takes the dispatch loop down with the process. The resilience boundary is restart + durable crash-recovery, which is proven: on restart the interrupted run resumes, 54 stale claims are released, and it reaches a terminal state. Harness/tooling only; no product code change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several new configuration options and enhancements to the system, focusing on operational flexibility, resource management, and telemetry. Key areas of improvement include the addition of startup telemetry reporting, enhanced API rate limiting and concurrency controls, more flexible worker and memory management options, and improved job queue and result batching controls.
Telemetry and Observability
TelemetrySettingsclass and correspondingCraftSettings.Telemetryproperty to enable opt-in startup phone-home telemetry, including configuration for endpoint, app ID, storm-guarding, and timeouts. Telemetry is off by default and requires explicit operator enablement. (Services/Configuration/TelemetrySettings.cs[1]Services/Configuration/CraftSettings.cs[2]StartupTelemetryServiceas a hosted service, ensuring telemetry is reported once per process start, with proper role and configuration gating. (Services/Hosting/CraftHostBuilderExtensions.csServices/Hosting/CraftHostBuilderExtensions.csR272-R280)API Rate Limiting and Concurrency Control
ApiConcurrencyLimitand related properties inRateLimitSettings, with environment variable overrides and logic to skip middleware when not needed. (Services/Configuration/RateLimitSettings.csServices/Configuration/RateLimitSettings.csR28-R67)Services/Hosting/CraftHostBuilderExtensions.csServices/Hosting/CraftHostBuilderExtensions.csL271-R326)CallerClassifierutility to reliably distinguish between app-only API clients and interactive callers for correct limiter application. (Services/Hosting/CallerClassifier.csServices/Hosting/CallerClassifier.csR1-R31)Worker and Resource Management
SkuProfilesAlt) and environment-variable-based selection, enabling deployments to switch between two resource sizing configurations without code changes. (Services/Configuration/WorkerSettings.csServices/Configuration/WorkerSettings.csR38-R54)HttpQueueTimeoutSecondsto control how long HTTP requests wait for a free runspace before being shed, with environment override and a default of 30 seconds. (Services/Configuration/WorkerSettings.cs[1]Services/Hosting/CraftHostBuilderExtensions.cs[2]GCHeapHardLimitMB) to SKU profiles, with three-way control (no opinion, disable, or set limit), and environment variable override for per-instance tuning. (Services/Configuration/SkuProfile.csServices/Configuration/SkuProfile.csR35-R55)Job Queue and Result Handling
ClearQueuemaintenance method toWorkerMetricsBridge, allowing administrators to clear the durable job queue for recovery scenarios. (Services/Bridges/WorkerMetricsBridge.csServices/Bridges/WorkerMetricsBridge.csR695-R715)BatchResultWritestoOrchestratorSettingsto control batching of small task results for performance, defaulting to true when status batching is enabled. (Services/Configuration/OrchestratorSettings.csServices/Configuration/OrchestratorSettings.csR22-R31)Build and Versioning
0.0.0-dev) for local builds to ensure meaningful version stamping in telemetry and diagnostics, with CI override. (Directory.Build.propsDirectory.Build.propsR58-R65)These changes collectively improve operational control, reliability, and observability of the system, while providing more robust mechanisms for resource and workload management.