Skip to content

fix: check job ownership on every progress interval - #720

Open
jia-gao wants to merge 3 commits into
llm-d:mainfrom
jia-gao:fix/ownership-fence-every-interval-713
Open

jia-gao wants to merge 3 commits into
llm-d:mainfrom
jia-gao:fix/ownership-fence-every-interval-713

Conversation

@jia-gao

@jia-gao jia-gao commented Oct 5, 2026

Copy link
Copy Markdown

Why is this PR needed?

The epoch fence on a running job only runs through ProgressTracker, and the tracker calls the updater only when counts are dirty. A job whose requests run long without completing can go indefinitely without checking its epoch or cancelling status: a reclaimed job keeps running on the old owner until its next completion, and a missed cancel event isn't noticed until finalization. A failed progress write also clears the dirty flag until another result arrives.

This addresses @lioraron's review point on #676. cc @wseaton

What does this PR do?

  • ProgressTracker calls a new CheckJobStatus on every tick, after the push when counts were dirty (skipped if that push already stopped the job).
  • The worker implements it as a read of the job's row by ID, fenced like DBUpdateProgress:
    • a missing row, a different epoch or a terminal status aborts the job as lost ownership, without a terminal write, the same as a fenced-out progress write;
    • cancelling aborts it as a user cancel, a backstop for a missed cancel event.
  • It reads instead of writing, using the existing DBGet. There are no storage interface changes, and a quiet interval adds no row version or WAL (a fenced no-op UPDATE would add one per job per interval).
  • Read errors are logged and the job keeps running, the same as progress write errors, so a transient DB error never aborts a job. Only a definite answer (row missing, other epoch, terminal, cancelling) stops it.
  • A failed progress write now leaves the counts dirty, so the next tick retries it.

Open questions:

  1. Each running job polls its own row (about 5 lookups by ID per processor every 15s at the defaults). A single processor-wide cancelling query would need a registry of running jobs. Is per-job OK?
  2. DBGet logs "DBGet: succeeded" at V(1) and the chart's default verbosity is 2, so this adds one log line per running job per interval. A dedicated fenced-read method on BatchProgressDBClient would avoid that at the cost of an interface change. Happy to switch if you prefer.
  3. The check runs while the tracker runs (the execution phase); ingestion and finalization still rely on the cancel event and the write fences.
  4. With Reconciler treats pod readiness as processor liveness #712 still open, a false-positive reclaim (an owner that is unready but still running) now aborts the original owner within one interval rather than at its next completion.

How was this tested?

  • Unit tests added/updated/verified

  • Integration/e2e tests added/updated/verified (PostgreSQL-backed test; e2e not run)

  • Manual testing performed

  • TestExecuteJob_QuietIntervalStatusChange: a request runs with no completions; after the first progress write the row's epoch is bumped (or set to cancelling) and the job must abort with the matching cause. Both cases time out on main.

  • TestJobProgressUpdater_CheckJobStatus: owned, epoch bumped, missing row, terminal, cancelling, cancelling under a newer epoch, transient read error.

  • TestProgressTracker_Tick: quiet ticks check without writing; a failed push is retried on the next tick with no new results.

  • TestProgressTrackerQuietIntervalPostgres (make test-postgres): the same scenarios against PostgreSQL, plus the row's xmin stays unchanged across quiet ticks.

  • make test (with -race), make lint, make test-regression, make test-integration, make test-postgres and make pre-commit pass locally.

Checklist

  • Commits are signed off (git commit -s) per DCO
  • Code follows project contributing guidelines
  • CI checks pass (make ci)
  • E2E tests pass (make test-e2e)

Related Issues

Fixes #713
Related to #676. Lease-based liveness (#712) is out of scope.

@github-actions github-actions Bot added the bug Fixes incorrect behavior label Oct 5, 2026
@zdtsw
zdtsw requested a review from aneeshkp October 5, 2026 15:55
BaseQuery: db.BaseQuery{IDs: []string{jobID}},
},
false, 0, 1)
if err != nil {

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.

is continuing on DB failure acceptable even though ownership cannot currently be verified?
How long it will continue if the error persists ?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yes, deliberately. It's the same contract the progress write already had before this PR: a failed UpdateProgressCounts is logged and the job keeps going.

How long it continues: until the DB is reachable again (the next successful check aborts the job if the epoch moved in the meantime), or until the job reaches finalization, whose write is epoch-fenced and can't commit for a non-owner. So any extra work is bounded by the outage. A non-owner never gets to commit terminal state.

Why I didn't make it abort after N failures: aborting here goes through the neutral "lost ownership" path, which makes no terminal write. If we are actually still the owner, which is the common case for a transient error, the job would be left in_progress with nobody working on it until something reclaims it. Two more points:

Duplicate work therefore needs both a reclaim and this processor being cut off from the DB at the same time.

The principled bound is the lease from #712: a processor that can't renew its lease stops at lease expiry, which is exactly when a reclaimer may take over. I'd rather add the bound there than invent a second timeout here. If it would help in the meantime, I can raise the log level or add a counter for consecutive failed checks, so a persistent failure is visible.

return u.fenced(u.inner.UpdateProgressCounts(ctx, u.jobID, u.epoch, counts))
}

// CheckJobStatus runs on every progress interval, so a job with no new results

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit on the comment style:

// CheckJobStatus calls onFencedOut when epoch no longer owns the job and
// onCancelling when the job is cancelling. Other errors are returned.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Applied verbatim in c6a3b0e, thanks.

@wseaton wseaton left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM. Ran the consistency sim harness (rebased past #676: https://github.com/wseaton/llm-d-batch-gateway/tree/sim-post-676) against main and against this PR on compose, with a 2s progress interval. Two new scenarios hit the quiet interval directly.

scenario main this PR
quiet_interval_cancel_lost: cancelling written, event never inserted, every request still generating cancelling ignored for 40s, then cancelling -> completed cancelled 3s after the cancelling write
quiet_interval_reclaim: epoch bumped with 5 requests in flight and 3 waiting old owner started all 3 waiting requests 0 started after the bump
cancel_event_lost (existing) cancelling -> completed cancelled
cancel_racing_completion (existing) holds holds

Not for this PR: the finalizing write is fenced on epoch only, so a cancelling that lands after the last tick but before finalization can still get overwritten. The window is one progress interval now instead of the rest of the job.

@jia-gao
jia-gao force-pushed the fix/ownership-fence-every-interval-713 branch from f39d8a4 to c6a3b0e Compare October 7, 2026 03:30
@acardace

acardace commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@zdtsw I think we can safely merge this.

@jia-gao
jia-gao force-pushed the fix/ownership-fence-every-interval-713 branch from c6a3b0e to 8872316 Compare October 8, 2026 05:13
@zdtsw
zdtsw enabled auto-merge (squash) October 8, 2026 05:55
jia-gao and others added 3 commits October 10, 2026 11:34
The epoch fence on a running job only ran through ProgressTracker, and
the tracker called the updater only when counts were dirty. A job whose
requests run long without completing could go indefinitely without
checking its epoch or cancelling status. A failed progress write also
cleared the dirty flag until the next result arrived.

The tracker now calls CheckJobStatus on every tick, whether or not
counts changed. The worker implements it as a read of the job's row by
primary key, fenced like the progress write, so quiet intervals add no
writes. A missing row, a different epoch or a terminal status aborts
the job as lost ownership, the same as a fenced-out progress write. A
cancelling status aborts it as a user cancel, which backstops a missed
cancel event. Read errors are logged and the job keeps running, the
same as progress write errors. A failed progress write now leaves the
counts dirty so the next tick retries it.

Fixes llm-d#713

Signed-off-by: Jiazhou Gao <gjz140103@gmail.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jiazhou Gao <gjz140103@gmail.com>
…ick case

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Jiazhou Gao <gjz140103@gmail.com>
@jia-gao

jia-gao commented Oct 10, 2026

Copy link
Copy Markdown
Author

Rebased onto main to resolve the conflict with #725 (initial push in ProgressTracker.Run):

  • Kept fix(processor): publish batch request total when the job starts #725's initial push and merged the two Run doc comments.
  • The two changes compose: if the initial push fails, this PR keeps the counts dirty, so the next tick retries it.
  • TestProgressTracker_Tick's quiet-tick case now expects exactly that one initial push and no writes from quiet ticks (separate commit).

make test (1317 passed), make lint, and make test-postgres-local (including TestProgressTrackerQuietIntervalPostgres) are green.

auto-merge was automatically disabled October 10, 2026 18:36

Head branch was pushed to by a user without write access

@jia-gao
jia-gao force-pushed the fix/ownership-fence-every-interval-713 branch from 8872316 to 381eef4 Compare October 10, 2026 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Fixes incorrect behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ownership fence only runs when progress counts change

4 participants