fix(batch): survive compute-node loss after a task completes - #124
Open
Joaquín Rivero (jQuinRivero) wants to merge 2 commits into
Open
fix(batch): survive compute-node loss after a task completes#124Joaquín Rivero (jQuinRivero) wants to merge 2 commits into
Joaquín Rivero (jQuinRivero) wants to merge 2 commits into
Conversation
Processing new imagery on dev1 failed with the raw Azure SDK error dump as the
image layer's entire status history:
Request encountered an exception.
Code: NodeNotReady
Message: {'additional_properties': {}, 'lang': 'en-US', 'value': 'Node is
not able to perform the requested operations in its current state...
The Batch task had succeeded and its outputs were already in blob storage. The
layer was still marked FAILED.
ImageryPostProcessor reacts to a task reaching a terminal state by reading files
back off the node that ran it -- imagery_manifest.json, imagery_friendly.log,
then a working-directory delete. Those file APIs (file.list/get/delete_from_task)
are served *by the compute node*, and the node is being torn down at exactly that
moment: infra/modules/batchPool.bicep sets `$NodeDeallocationOption =
taskcompletion`, and the shared-dev pools dev1 targets are autoscale with
minNodes=0 on low-priority nodes, which can also be preempted. A deallocating,
rebooting or preempted node answers NodeNotReady.
Three properties then turned a transient race into a permanent failure:
- NodeNotReady is HTTP 409, and is_server_error only retried 5xx, so it was
never retried.
- The imagery queue trigger did `statusMessage = str(e)`, replacing the whole
progress history with the SDK repr -- the unreadable dialog above.
- host.json sets maxDequeueCount: 1, so the message is never redelivered.
Classify node errors and retry the transient ones. NodeNotReady and
NodeStateInvalid describe a node that is mid-transition and may still answer, so
they join the retry predicate; NodeNotFound describes a node that is gone, so it
is surfaced as unavailable instead. The retry budget is unchanged (5 attempts,
exponential 4-10s) and reraise=True was added so an exhausted budget still yields
the BatchErrorException rather than tenacity's RetryError, keeping it
classifiable by callers.
Degrade instead of failing. get_filecontent_from_task now returns None when the
node cannot serve a file -- matching its existing "file not found" contract, which
every caller already branches on -- and cleanup_task skips the working-directory
delete while still disabling the job (task retention_time=P2D reclaims the disk).
Every other Batch error still propagates. This covers all five workloads:
imagery, training, inference, artifacts and embedding.
Recover the outputs from blob. Batch already uploads outputs/*.* on task
completion, so the manifest was sitting at <projectHash>/<taskId>/ the whole
time -- the same path _generate_imagery_url already resolves in production.
ImageryPostProcessor._read_task_output reads the node copy first and falls back
to that blob copy. The manifest stays required; the progress log is best-effort.
A failing fallback returns None rather than raising, so it can never mask the
original reason the node read failed.
To make the log recoverable at all, add_task now accepts file_pattern as a string
or a list, emitting one OutputFile per pattern, and imagery submits both
outputs/*.* and logs/*.* -- the log is written to logs/, which no pattern
previously covered, so it only ever existed on the node. A single blanket **/*
was rejected: it would also upload the raw downloaded imagery.
Stop destroying the status history. The imagery trigger appends via
append_status_message instead of assigning, and renders the cause through a new
hastegeo.core.utils.errors.describe_exception, which reduces an Azure-style error
to "NodeNotReady: Node is not able to perform the requested operations in its
current state" and strips the RequestId/Time trailer. It matches the error shape
(.error.code / .error.message.value) rather than importing azure.batch, so
utils stays a leaf package, and it lives in hastegeo rather than function_app.py
per the repository's function-app boundary rule. The train, embedding and
inference triggers already appended; they now use the same formatter.
The pool configuration is deliberately untouched. This makes the race survivable,
not impossible -- preemption of a spot node can happen regardless of the
deallocation policy. Whether the pool should hold a node floor is an
infrastructure decision that needs an ADR and a quota discussion; it is recorded
as an open question in the spec.
Note the fix is not retroactive: the logs/ upload only applies to newly submitted
tasks, so image layers that already failed this way must be re-run.
Adds 31 unit tests across error classification, retry policy, runner degradation,
the imagery blob fallback, upload patterns and message formatting. Also repairs a
pre-existing fixture gap in test_imagery_preprocess_config.py, which covers the
same _execute_image_preprocess method this change touches and had been failing
since clipBbox was added to the submitted config -- MagicMock(spec=ImageLayer)
does not expose pydantic field names. Suite goes from 155 passed / 2 failed to
188 passed / 0 failed.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 158a23ee-bb84-40cf-9974-98b097e7c6a1
Record the root cause, the design, and the alternatives that were rejected, per the repository rule that specs are the source of truth. Two documentation claims were also wrong once the read path changed. Both api/hastefuncqueues/README.md and docs/api/hastefuncqueues.md stated that imagery_friendly.log and imagery_manifest.json are read "from the task working directory", which is now only the first of two sources -- and, on an autoscale pool, usually the one that no longer exists. They now describe the node-then-blob order, which of the two files is required, and why the node copy disappears. The spec set follows spec/_templates/feature/ and includes the Agent Assignment Map required by the templates. It records the pool configuration as an explicit non-goal: this change makes node loss survivable rather than impossible, and whether the pool should hold a node floor is left as an open question needing an ADR and a quota discussion. The CHANGELOG entry sits alongside the other Batch fixes under Unreleased, and carries the operator note that the logs/ upload is not retroactive -- image layers that already failed this way have to be re-run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 158a23ee-bb84-40cf-9974-98b097e7c6a1
Copilot started reviewing on behalf of
Joaquín Rivero (jQuinRivero)
August 12, 2026 21:09
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Improves Azure Batch resilience when compute nodes disappear after task completion.
Changes:
- Adds node-error classification, retries, and tolerant cleanup.
- Adds imagery blob fallback and readable status errors.
- Adds tests and supporting specifications.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 15 comments.
Show a summary per file
| File | Description |
|---|---|
CHANGELOG.md |
Records the resilience fix. |
api/hastefuncqueues/README.md |
Documents output fallback. |
api/hastefuncqueues/function_app.py |
Preserves and formats failure history. |
docs/api/hastefuncqueues.md |
Documents queue behavior. |
hastelib/src/hastegeo/core/processors/imagery.py |
Recovers task outputs from blob. |
hastelib/src/hastegeo/core/runners/azure_batch.py |
Handles node-loss errors and output patterns. |
hastelib/src/hastegeo/core/runners/local.py |
Adds multiple-pattern normalization. |
hastelib/src/hastegeo/core/utils/blob.py |
Adds text fetching helper. |
hastelib/src/hastegeo/core/utils/errors.py |
Adds readable exception formatting. |
hastelib/tests/core/processors/test_imagery_output_fallback.py |
Tests imagery recovery. |
hastelib/tests/core/processors/test_imagery_preprocess_config.py |
Repairs test fixture. |
hastelib/tests/core/runners/test_azure_batch_node_errors.py |
Tests node-error handling. |
hastelib/tests/core/utils/test_errors.py |
Tests exception formatting. |
spec/features/batch-node-loss-resilience/README.md |
Defines the fix and acceptance criteria. |
spec/features/batch-node-loss-resilience/design.md |
Documents technical design. |
spec/features/batch-node-loss-resilience/impact-analysis.md |
Assesses operational impact. |
spec/features/batch-node-loss-resilience/plan.md |
Tracks implementation phases. |
spec/features/batch-node-loss-resilience/rollout.md |
Defines deployment and rollback. |
spec/features/batch-node-loss-resilience/test-plan.md |
Defines validation coverage. |
spec/features/batch-node-loss-resilience/user-stories.md |
Defines stories and agent assignments. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+318
to
+320
| # Surface the underlying BatchErrorException once the budget is spent, | ||
| # instead of tenacity's RetryError, so callers can still classify it. | ||
| reraise=True, |
| batch_error_code(e), | ||
| filename, | ||
| ) | ||
| return None |
| return parts[1], "/".join(parts[2:]) | ||
|
|
||
|
|
||
| def fetch_url_text(url: str, timeout: int = 30) -> Optional[str]: |
| return self.image_data | ||
|
|
||
| def _get_image_preprocess_logs(self): | ||
| def _read_task_output(self, filename: str): |
| output_prefix: str, | ||
| resource_files: list, | ||
| file_pattern: str, | ||
| file_pattern, |
| @@ -0,0 +1,98 @@ | |||
| # Rollout Plan: Batch node-loss resilience | |||
| @@ -0,0 +1,93 @@ | |||
| # Execution Plan: Batch node-loss resilience | |||
| @@ -0,0 +1,90 @@ | |||
| # Impact Analysis: Batch node-loss resilience | |||
| @@ -0,0 +1,144 @@ | |||
| # Design: Batch node-loss resilience | |||
| | Method | Contract | | ||
| |---|---| | ||
| | `get_filecontent_from_task` | returns file text, or `None` when the file is missing **or** the node cannot serve it. Unrelated `BatchErrorException`s propagate. | | ||
| | `cleanup_task` | best-effort working-directory delete; always disables the job. Unrelated `BatchErrorException`s propagate. | |
RC artifacts readyAll branch deployment references use the same RC tag:
|
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
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.
Description
Processing new imagery on dev1 failed, and the status dialog showed a raw Azure SDK dump as the image layer's entire status history:
The Batch task had succeeded, and the imagery it produced was already in blob storage. The layer was still marked FAILED.
Root cause
ImageryPostProcessorreacts to a task reaching a terminal state by reading files back off the compute node that ran it:_get_image_preprocess_logsfile.list/get_from_taskimagery_friendly.log_update_results_from_jobfile.list/get_from_taskimagery_manifest.jsonrunner.cleanup_taskfile.delete_from_taskThose APIs are served by the node, and the node is being torn down at exactly that moment —
infra/modules/batchPool.bicepsets$NodeDeallocationOption = taskcompletion, and the shared-dev pools dev1 targets are autoscale withminNodes = 0on low-priority (preemptible) nodes.Three properties then turned a transient race into a permanent failure:
NodeNotReadyis HTTP 409, andis_server_erroronly retried 5xxstatusMessage = str(e)host.jsonsetsmaxDequeueCount: 1The outputs were never lost. Batch already uploads
outputs/*.*on task completion, so the manifest was sitting at<projectHash>/<taskId>/— the same path_generate_imagery_urlalready resolves. The processor simply never looked there.What this changes
1. Classify node errors, retry the transient ones —
NodeNotReady/NodeStateInvaliddescribe a node mid-transition that may still answer, so they join the retry predicate;NodeNotFounddescribes a node that is gone, so it is surfaced as unavailable instead. Budget unchanged (5 attempts, exponential 4–10s). Addedreraise=Trueso an exhausted budget yields theBatchErrorExceptionrather than tenacity'sRetryError, keeping it classifiable.2. Degrade instead of failing —
get_filecontent_from_taskreturnsNonewhen the node cannot serve a file (matching its existing "file not found" contract, which every caller already branches on), andcleanup_taskskips the working-directory delete while still disabling the job. Every other Batch error still propagates. Applies to all five workloads — imagery, training, inference, artifacts, embedding.3. Recover outputs from blob (imagery) —
_read_task_outputreads the node copy first, then falls back to the blob copy. The manifest stays required; the progress log is best-effort. A failing fallback returnsNonerather than raising, so it can never mask the original reason the node read failed.4. Upload the progress log —
add_tasknow acceptsfile_patternas a string or a list, and imagery submits bothoutputs/*.*andlogs/*.*. The log is written tologs/, which no pattern previously covered, so it only ever existed on the node. A blanket**/*was rejected — it would also upload the raw downloaded imagery.5. Stop destroying the status history — the trigger appends via
append_status_messageand renders the cause through a newdescribe_exception, givingNodeNotReady: Node is not able to perform the requested operations in its current stateinstead of the object dump. It matches the error shape (.error.code/.error.message.value) rather than importingazure.batch, and lives inhastegeorather thanfunction_app.pyper the repo's function-app boundary rule.Deliberately out of scope
The pool configuration is untouched. This makes the race survivable, not impossible — a spot node can be preempted regardless of the deallocation policy. Whether the pool should hold a node floor is an infrastructure decision that needs an ADR and a quota discussion; it is recorded as an open question in
plan.md.Also out of scope: raising
maxDequeueCount(redelivery would re-run whole tasks), and a blob fallback for the non-imagery workloads (they inherit the runner-level fixes; only imagery reads a required output file back).Full design, rejected alternatives and risk analysis:
spec/features/batch-node-loss-resilience/.Type of change
Checklist
Testing
31 new unit tests across three files:
tests/core/runners/test_azure_batch_node_errors.pytests/core/processors/test_imagery_output_fallback.pytests/core/utils/test_errors.pyRetry tests use
retry_with(wait=wait_none()), so the real predicate and stop policy are exercised without the 4–10s sleeps.BatchErrorExceptions are constructed directly with the codes the service returns; the Batch client is mocked.Regression-checked against a clean worktree at
HEAD, excluding two modules that cannot run outside the conda test env (test_prepare_imagery.pyneedsosgeo;test_artifacts.pyneedspytest-mock):c03c6ea)The two baseline failures were in
test_imagery_preprocess_config.py, which exercises_execute_image_preprocess— the same method this PR touches. They were a fixture gap, not a product bug:MagicMock(spec=ImageLayer)does not expose pydantic field names, so the mock had noclipBboxonce that field was added to the submitted config. Fixed here with a one-line fixture addition, so reviewers aren't left wondering whether this PR broke them.black,isort,flake8anddetect-secretsclean on every touched file (pinned pre-commit hooks).Not yet verified live
Recovery has not been observed against a real deallocating node — only against constructed error codes. The manual dev1 verification steps are in
test-plan.md.Additional context
logs/upload only applies to newly submitted tasks, so image layers that already failed this way on dev1 must be re-run.Reviewer attention is most useful on:
get_filecontent_from_taskreturningNonewhere it previously raised — I checked that all five processors already branch on falsy content, but a second pair of eyes on the non-imagery callers is worthwhile.TRANSIENTvsTERMINALsplit — if there are other node-state codes worth treating as transient, they're a one-line addition to the frozenset.is unavailablewarning should be promoted to a metric, so we can tell how often the race actually fires and decide if the pool itself needs the follow-up ADR.Deployment:
hastelibwheel →hastefuncqueues/hastefuncapiviadeploy-apps.yml. No infrastructure, Bicep, app-setting, UI or dependency changes; fully reversible by revert + redeploy.