Skip to content

harmonytask: fence asynchronous admission and task completion - #1533

Open
hyunmoon wants to merge 3 commits into
filecoin-project:mainfrom
hyunmoon:codex/submission-q5-completion-20260914
Open

hyunmoon wants to merge 3 commits into
filecoin-project:mainfrom
hyunmoon:codex/submission-q5-completion-20260914

Conversation

@hyunmoon

@hyunmoon hyunmoon commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Prevent slow task preparation from blocking scheduling and stale acquisitions/completions from changing a newer attempt.

  • Preserve committed retry deadlines through claims and preemption.
  • Move bounded attempt preparation and failed-start cleanup off the scheduler loop; fence both with acquisition generation/token, including same-owner recovery.
  • Fence completion with the identity captured before dispatch. Stale results cannot delete/disown a newer task, spend its failure budget, change its retry clock or publish completion callbacks. Missing or superseded rows are terminal no-ops, not database outages. History and task mutations commit together; local resources release once when the execution returns.

Validation and limits

Normal/race and exact-tag vet/lint; PostgreSQL completion, same-owner recovery, missing-row, retry-clock, rollback and linked lock-wait tests, including a fence-removal negative control. The common completion code was also tested on disposable Yugabyte 2025.2.2.2-b11 with the actual migration runner, Read Committed and wait queues, including linked contention and stale rejection after lock release. Task/native bodies in these tests are substitutes.

The shared completion path has shown normal completion and subsequent task processing in production Indexing. This is not production validation of the entire PR or stale-completion races.

Participating workers must be updated: a WebRPC update or migration alone does not protect old workers. Stage UPDATE fencing inside Do, native termination, and exactly-once callbacks after ambiguous commits or process loss remain out of scope. The completion fence adds no cleanup API or additional migration.

Series overview

This is a navigation index, not a dependency for the independent PRs. The three Draft PRs depend only on this foundation and are independent of each other.

Scope PR Relationship State
PreCommit batch isolation #1534 Independent Ready for review
Assignment, transfers and CC quota #1535 Independent Ready for review
Queue limits and waiting release #1536 Independent Ready for review
Indexing readiness and piece offsets #1537 Independent Ready for review
Task admission and completion fences #1533 Independent Ready for review
Optional SDR start pacing #1540 Depends on #1533 Draft
Bounded task and PoRep monitoring #1541 Depends on #1533 Draft
Configuration editor preservation #1538 Independent Ready for review
Complete remote vanilla responses #1539 Independent Ready for review
Unavailable PoRep backend isolation #1542 Depends on #1533 Draft

@snadrus snadrus 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.

Please focus on comprehensive bug descriptions (with example scenarios that could become a test). It's unclear what problem(s) this PR is solving. This is a large PR in a complex code area handling many underspecified problems at once.

Failed bool `db:"failed"`
}

// Storage claims re-read the reference after advisory discovery/ownership

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.

What concerns me is that none of these situations should ever be a problem, so I'm unclear on the need for any of this code:

  • missing data: The watchers schedule tasks when the data is available.
  • ambiguous: I'm not sure what this refers to
  • completed/failed: these are task concepts already.

@@ -0,0 +1,30 @@
ALTER TABLE harmony_task ADD COLUMN IF NOT EXISTS work_start TIMESTAMPTZ;

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.

There are multiple 20260909-*.sql entries. This is not compatible with the loader.

@github-project-automation github-project-automation Bot moved this from 🔎 Awaiting review to ⌨️ In Progress in FOC Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. I combined distinct changes without explaining their failure scenarios clearly enough. I checked them separately against base 86e95967 and candidate 0c1f7cbd.

  • Retry clock: after a normal failure and its backoff, the peer notification drops the committed update time. The receiving scheduler uses receipt time and starts another wait. The regression fails on the base and passes on the candidate. DB polling can correct the old timestamp; this is not an unconditional doubling of retry delays.
  • Stale completion: a missed heartbeat lets CleanupMachines release a task for another worker to claim. When the old execution returns, its completion can delete or disown the new owner's task and publish stale history or a success callback. The candidate rejects those results while allowing the current execution to complete. Writes inside Do and native termination remain outside this protection.
  • SDR references: initial task creation and pipeline linking are atomic. The later success UPDATE clears task_id_sdr before the scheduler deletes the task in a separate transaction. Stopping between them leaves a task without a pipeline reference; SectorRemove can also leave one. Without pacing, the base wastes the first admission on that orphan but progresses on the next pass. The candidate skips it immediately. This establishes an admission mitigation, not orphan repair or a fleet-throughput result.
  • Completed SDR: a delayed poll snapshot can create a new task after SDR completion because pollStartSDR rechecks a NULL task ID but not after_sdr. The filter rejects that task but does not fix the producer race. I have not established equivalent normal producer cases for ambiguous references or an independently necessary failed-but-SDR-incomplete check.

The reproduction ZIP (README) contains the test overlays, commands and recorded results. Its SHA-256 is 8d7ee4c5daf3caa880cfbd41302fb025326af57f3f9634ac3eb1c31a0ab4af2c. Tests use disposable PostgreSQL and real task/SQL paths, with native work substituted and the SDR success-write block extracted unchanged. They do not establish the cause of the historical incident.

The duplicate migration-key concern is valid. Fresh startup executing both files and the forward reconciliation do not justify duplicate keys in a new upstream proposal.

I agree the proven bug fixes should be separated from the new attempt telemetry and its asynchronous preparation/cleanup design. My next step is to narrow that scope while preserving the acquisition identity needed for safe completion. The PR code is unchanged at this point.

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.

3 participants