Skip to content

fix(batch): scope Batch job ids to the selected pool - #99

Merged
jQuinRivero merged 2 commits into
mainfrom
v-joaquinri/fix-batch-job-pool-binding
Jul 28, 2026
Merged

fix(batch): scope Batch job ids to the selected pool#99
jQuinRivero merged 2 commits into
mainfrom
v-joaquinri/fix-batch-job-pool-binding

Conversation

@jQuinRivero

@jQuinRivero jQuinRivero commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Problem

Two image layers submitted close together — the first ran, the second failed before reaching Batch:

Job <h100-pool> is bound to pool <h100-pool> but this task targets pool
<t4-pool>, and rebinding failed (OperationInvalidForCurrentState).

Capacity-aware spillover broke in exactly the concurrent case it exists to serve.

Root cause

Three properties collide:

  1. The Batch job id is static — it comes from config and defaults to the singular pool id (*_BATCH_JOB_ID).
  2. The pool is chosen per taskselect_pool returns the first candidate with an idle node.
  3. A Batch job is permanently bound to one pool — re-pointable only while it has no active tasks.
Step Result
Task A t4 has no idle node → routed to h100; job created bound to h100
Task B (while A runs) h100 busy, t4 idle → routed to t4
Same static job id, but that job is bound to h100
Rebind attempt OperationInvalidForCurrentState — the job has active tasks
Submission fails

The _rebind_job_pool I added in #98 was the wrong remedy: one static job id cannot span multiple pools, and the moment concurrency exists it cannot be rebound at all. This PR replaces that approach.

Fix — one job per pool

New resolve_job_id(base, selected_pool, candidates) derives the job id from the pool a task was routed to:

Case Result
base is one of the candidate pool ids (the default convention) the selected pool id
single candidate base unchanged — legacy environments are not renamed
multiple candidates + custom base "<base>-<selected_pool>", trimmed to 64

Truncation removes characters from base, never the pool, so two different pools can never collapse onto the same job id under the 64-char Batch limit.

create_job is now graceful and returns the job id it actually used — reuse when the binding matches, rebind when the job is idle, otherwise fall back to a pool-scoped job instead of failing the submission.

Callers capture the returned job id

This is the easy-to-miss part. add_task returns (job_id, task_id) and the returned id may now differ from the one passed in. All five processors persist jobId into metadata, which is later used for status polling and log retrieval — so each now captures the returned value (imagery, train, inference, artifacts, embedding). Previously none of them used the return value at all.

LocalRunner.add_task was verified to honour the same contract (returns the tuple, or raises).

Verification

  • New regression test reproduces the exact failure: a task routed to a spillover pool now uses a job scoped to that pool instead of colliding.
  • Tests for all four naming branches, 64-char overflow, and per-pool distinctness under truncation.
  • create_job covered for reuse / rebind / fallback-creates / fallback-reuses.
  • 108 passed, 7 skipped locally (modules needing geopandas/shapely/pytest-mock excluded — absent from my env, unrelated to this change).
  • pre-commit run clean; Config drift guard still passes (no application settings change here).

Blast radius

Limited to environments routing across multiple pools. Single-pool environments resolve to the identical job id they use today, so no jobs are renamed and no in-flight metadata is invalidated. Existing preprocessJob.jobId values keep pointing at jobs that still exist.

Affected environments self-heal — the spillover route simply starts using its own job. No manual job cleanup required.

Notes


Note on the second commit. fix(deploy): store Batch pool config as secrets... is replicated from #94, where it currently also lives. It is required here so this branch can be deployed to a test environment on its own: the pool config was moved from GitHub Environment variables to secrets (variables are not masked in this repo's public Actions logs), so a branch still reading vars.BATCH_* resolves them to empty and falls back to a derived pool name that no longer exists.

The content is identical to #94's copy, so whichever merges second should collapse cleanly.

Capacity-aware spillover failed whenever two tasks were in flight:

    Job <h100-pool> is bound to pool <h100-pool> but this task targets pool
    <t4-pool>, and rebinding failed (OperationInvalidForCurrentState).

Three properties collide. The Batch job id is static (it defaults to the
singular pool id), the pool is chosen per task by select_pool, and a Batch job
is permanently bound to the pool it was created against -- re-pointable only
while it has no active tasks. So the first task created a job on the preferred
pool, and a second task that spilled over to another pool collided with it.
Spillover broke in exactly the concurrent case it exists to serve.

Rebinding, added in #98, was the wrong remedy: one static job id cannot span
multiple pools, and once concurrency exists it cannot be rebound at all.

- new resolve_job_id(): derive the job id from the routed pool, so each pool
  gets its own job. Environments with a single candidate pool keep their
  existing job id, so no jobs are renamed there. Truncation to the 64-char
  Batch limit removes characters from the base, never the pool, so two pools
  can never collapse onto the same id.
- create_job() is now graceful and returns the job id it actually used: reuse
  when the binding matches, rebind when the job is idle, otherwise fall back to
  a pool-scoped job instead of failing the submission.
- processors capture the returned job id. All five persist jobId into metadata,
  which is later used for status polling and log retrieval, so the id the
  runner actually used has to be the one recorded.
- drop BatchJobPoolMismatchError, now unreachable.
@jQuinRivero
jQuinRivero requested a review from a team as a code owner July 28, 2026 12:18
… dump

Pool ids were wired through GitHub Environment *variables*, but this repo is
public and Actions logs are public with it. GitHub masks secret values in logs
and does not mask variables, and `az functionapp config appsettings set` was
printing the resulting settings JSON to stdout, so the values would have landed
in a public log.

Store them as environment secrets instead, matching how RESOURCE_PREFIX,
LOCATION, RESOURCE_SUFFIX and STATIC_APP_DOMAIN are already handled, and add
`--output none` so the app-settings dump is not emitted at all.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: dd98b384-7e26-47a9-8c03-6f9594eb611c
@github-actions

Copy link
Copy Markdown

RC artifacts ready

All branch deployment references use the same RC tag:

  • hastegeo_version: 1.0.27rc9
  • training_image_tag: 1.0.27rc9
  • imageprep_image_tag: 1.0.27rc9
  • wheel: hastegeo-1.0.27rc9-py3-none-any.whl

@jQuinRivero

Copy link
Copy Markdown
Contributor Author

Verified in a live environment

Deployed this branch (hastegeo 1.0.27rc9, component=funcqueue) to a dev environment and reproduced the original failure scenario.

Setup: the H100 pool had a warm node and was running an imageryprep task; the T4 pool had an idle node. A third image layer was then submitted while the first was still running — the exact sequence that previously failed at submit time with OperationInvalidForCurrentState.

Result:

Task Job Ran on pool State
img-928be853… <h100-pool> h100 completed
img-853ce679… <h100-pool> h100 running
img-178b2bce… <t4-pool> t4 running
  • The spillover submission was accepted — no OperationInvalidForCurrentState, no mismatch message in the UI status panel. Under the previous code this submission failed before reaching Batch.
  • Each job is bound 1:1 to its own pool (job.id == job.poolInfo.poolId for both), which is what resolve_job_id is meant to produce.
  • The spillover task went on to schedule and execute on the T4 node, so it is not merely accepted-then-stuck.
  • Deployed artifact confirmed as hasteimageryprep:1.0.27rc9, and the per-job user-delegation SAS path was active on all tasks.
  • Pool ids, AZURE_BATCH_USE_SAS and AZURE_BATCH_MANAGE_POOLS survived the deploy unchanged, and the deploy log contained no pool names or app-settings JSON.

Separate pre-existing issue found while testing

Spillover only engages once the secondary pool already has a node. The T4 pool autoscales on $ActiveTasks within its own job, evaluated every 5 minutes, and with AZURE_BATCH_MANAGE_POOLS=false the runner never resizes it. So while the pool sits at zero nodes, select_pool never sees an idle node there and never routes to it — a cold-start deadlock that only resolves if something else puts a task on that pool first.

I had to warm the T4 pool manually to exercise the spillover path here. This is a gap in the autoscale/routing interaction from the shared-pool work, not something introduced by this PR, so I have left it out of scope — but it is worth a follow-up, since in practice spillover will rarely trigger on its own.

@prbatero prbatero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jQuinRivero
jQuinRivero merged commit 6b5a02f into main Jul 28, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants