Repository navigation
TSI-3766: fix(scheduler): starvation sweep WORKER_CRASH 判定时刻跨 run 抖动，(120s,150s] 窗口边界敏感 - #128
Open
multica-tsic404[bot] wants to merge 1 commit into
Open
multica-tsic404[bot] wants to merge 1 commit into
multica-tsic404[bot] wants to merge 1 commit into
Conversation
Reviewer's GuideThe PR introduces a one-shot scheduler wake based on the oldest unassigned pending job, causing starvation judgments to occur just after the no-worker deadline instead of being quantized by inspection-tick phase, while retaining tick fallback and existing sweep semantics. Sequence diagram for starvation judgment wake-upsequenceDiagram
participant Scheduler
participant Database
participant WakeTimer
participant StarvationSweep
Scheduler->>Database: EarliestPendingCreatedAt()
Database-->>Scheduler: anchor
Scheduler->>WakeTimer: Reset(anchor + NoWorkerJobTimeout + starvationExpiryMargin)
WakeTimer-->>Scheduler: expiry wake
Scheduler->>StarvationSweep: checkNoWorkerStarvation()
StarvationSweep-->>Scheduler: classify and persist verdict
Scheduler->>Database: EarliestPendingCreatedAt()
Scheduler->>WakeTimer: Reset(next anchor deadline)
Flow diagram for starvation wake and tick fallbackflowchart TD
A[Scheduler loop starts] --> B[Read oldest pending unassigned created_at]
B --> C{Future deadline exists?}
C -- Yes --> D[Arm one-shot wake]
C -- No --> E[Wait for timeout tick]
D --> F{Wake or timeout tick?}
E --> F
F --> G["checkNoWorkerStarvation()"]
G --> H{Live schedulable worker exists?}
H -- Yes --> I[No starvation verdict]
H -- No --> J[Write terminal failure and release capacity]
I --> B
J --> B
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="pkg/server/scheduler/scheduler.go" line_range="192" />
<code_context>
case <-timeoutTicker.C:
s.checkTimeouts()
s.checkNoWorkerStarvation()
+ s.armStarvationWake(starvationWake)
+ case <-starvationWake.C:
+ // A pending job's no-worker clock just expired: judge it now rather
</code_context>
<issue_to_address>
**Starvation verdict arrives late**
When a job becomes pending after the initial arm found no pending jobs, and its deadline passes before the next timeout tick, `run` schedules or reschedules the job without re-arming `starvationWake`, so `checkNoWorkerStarvation` does not judge it until the next timeout tick. The verdict is delayed past the no-worker deadline.
Re-arm `starvationWake` when scheduling or rescheduling makes a job pending, not only at startup, timeout ticks, or wakes.
Also at `pkg/server/scheduler/scheduler.go:177`, `pkg/server/scheduler/scheduler.go:213`.
</issue_to_address>
### Comment 2
<location path="pkg/server/scheduler/scheduler.go" line_range="31" />
<code_context>
+// three orders of magnitude below the default 30s check interval, so the
+// verdict stays far inside the (120s, 150s] acceptance window (AC-17) and well
+// before the CLI's own no-worker give-up at deadline+check interval.
+const starvationExpiryMargin = time.Second
+
type Config struct {
</code_context>
<issue_to_address>
**CLI deadline precedes server verdict**
When `TimeoutCheckInterval` is less than one second and a timeout tick checks the job at the strict cutoff, the one-second margin places the expiry wake after the deadline advertised to the CLI when `TimeoutCheckInterval` is shorter than one second. If a tick lands exactly on the strict cutoff, it skips the job and the CLI can give up before the next server verdict.
Keep the wake margin below the configured client slack (or reject configurations where that cannot be guaranteed) while still ensuring the sweep runs strictly after its cutoff.
</issue_to_address>
### Comment 3
<location path="pkg/server/db/db.go" line_range="1920-1925" />
<code_context>
+// not pull the anchor earlier.
+func (d *Database) EarliestPendingCreatedAt() (time.Time, bool, error) {
+ var anchor time.Time
+ err := d.db.QueryRow(`
+ SELECT created_at FROM jobs
+ WHERE status = ? AND worker_id IS NULL
+ ORDER BY created_at ASC
+ LIMIT 1
+ `, protocol.JobStatusPending).Scan(&anchor)
+ if errors.Is(err, sql.ErrNoRows) {
+ return time.Time{}, false, nil
</code_context>
<issue_to_address>
**Anchor lookup scans pending backlog**
When the database has a large pending backlog and the scheduler repeatedly re-arms the wake, `EarliestPendingCreatedAt` filters by `status` and `worker_id` while ordering by `created_at`, but the jobs table has no index covering that combination. With a large pending backlog, each startup, timeout tick, or wake can scan and sort many pending rows, adding recurring database work to the scheduler.
Add an index that supports the pending, unassigned `created_at` lookup, and verify the query plan uses it.
</issue_to_address>
### Comment 4
<location path="pkg/server/scheduler/scheduler_starvation_wake_test.go" line_range="86-87" />
<code_context>
+
+ // Anchor the pending clock so the deadline is a second away, and pass the
+ // deadline before the verdict can be observed.
+ deadline := time.Now().Add(time.Second)
+ backdatePendingAnchor(t, database, job.ID, deadline.Add(-noWorkerTimeout))
+
+ notifyCh, cancel := database.JobNotifier().Subscribe(job.ID)
</code_context>
<issue_to_address>
**Wake test can time out**
When test setup or scheduling takes more than one second before the scheduler arms the wake, `armStarvationWake` sees the test deadline has passed and leaves judgment to the one-hour timeout tick, so the test's 30-second wait expires despite the fallback being intentional.
Set the test deadline relative to a synchronized scheduler start, or otherwise ensure the scheduler arms the wake before the deadline passes.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 4 findings to address first, and the expiry wake can mark an unassigned pending job failed earlier than the next timeout tick; if that verdict is wrong, the failed status remains after a revert, but the job can be retried or resubmitted.
Blocking findings: pkg/server/scheduler/scheduler.go:192, pkg/server/scheduler/scheduler.go:31, pkg/server/db/db.go:1925, pkg/server/scheduler/scheduler_starvation_wake_test.go:87
| case <-timeoutTicker.C: | ||
| s.checkTimeouts() | ||
| s.checkNoWorkerStarvation() | ||
| s.armStarvationWake(starvationWake) |
There was a problem hiding this comment.
🟡 Medium · Starvation verdict arrives late
When a job becomes pending after the initial arm found no pending jobs, and its deadline passes before the next timeout tick, run schedules or reschedules the job without re-arming starvationWake, so checkNoWorkerStarvation does not judge it until the next timeout tick. The verdict is delayed past the no-worker deadline.
Re-arm starvationWake when scheduling or rescheduling makes a job pending, not only at startup, timeout ticks, or wakes.
Also at pkg/server/scheduler/scheduler.go:177, pkg/server/scheduler/scheduler.go:213.
Prompt for AI agents
In `pkg/server/scheduler/scheduler.go` at line 192:
**Starvation verdict arrives late**
When a job becomes pending after the initial arm found no pending jobs, and its deadline passes before the next timeout tick, `run` schedules or reschedules the job without re-arming `starvationWake`, so `checkNoWorkerStarvation` does not judge it until the next timeout tick. The verdict is delayed past the no-worker deadline.
Re-arm `starvationWake` when scheduling or rescheduling makes a job pending, not only at startup, timeout ticks, or wakes.
Also at `pkg/server/scheduler/scheduler.go:177`, `pkg/server/scheduler/scheduler.go:213`.| // three orders of magnitude below the default 30s check interval, so the | ||
| // verdict stays far inside the (120s, 150s] acceptance window (AC-17) and well | ||
| // before the CLI's own no-worker give-up at deadline+check interval. | ||
| const starvationExpiryMargin = time.Second |
There was a problem hiding this comment.
🟡 Medium · CLI deadline precedes server verdict
When TimeoutCheckInterval is less than one second and a timeout tick checks the job at the strict cutoff, the one-second margin places the expiry wake after the deadline advertised to the CLI when TimeoutCheckInterval is shorter than one second. If a tick lands exactly on the strict cutoff, it skips the job and the CLI can give up before the next server verdict.
Keep the wake margin below the configured client slack (or reject configurations where that cannot be guaranteed) while still ensuring the sweep runs strictly after its cutoff.
Prompt for AI agents
In `pkg/server/scheduler/scheduler.go` at line 31:
**CLI deadline precedes server verdict**
When `TimeoutCheckInterval` is less than one second and a timeout tick checks the job at the strict cutoff, the one-second margin places the expiry wake after the deadline advertised to the CLI when `TimeoutCheckInterval` is shorter than one second. If a tick lands exactly on the strict cutoff, it skips the job and the CLI can give up before the next server verdict.
Keep the wake margin below the configured client slack (or reject configurations where that cannot be guaranteed) while still ensuring the sweep runs strictly after its cutoff.
Comment on lines
+1920
to
+1925
| err := d.db.QueryRow(` | ||
| SELECT created_at FROM jobs | ||
| WHERE status = ? AND worker_id IS NULL | ||
| ORDER BY created_at ASC | ||
| LIMIT 1 | ||
| `, protocol.JobStatusPending).Scan(&anchor) |
There was a problem hiding this comment.
🟡 Medium · Anchor lookup scans pending backlog
When the database has a large pending backlog and the scheduler repeatedly re-arms the wake, EarliestPendingCreatedAt filters by status and worker_id while ordering by created_at, but the jobs table has no index covering that combination. With a large pending backlog, each startup, timeout tick, or wake can scan and sort many pending rows, adding recurring database work to the scheduler.
Add an index that supports the pending, unassigned created_at lookup, and verify the query plan uses it.
Prompt for AI agents
In `pkg/server/db/db.go` at lines 1920-1925:
**Anchor lookup scans pending backlog**
When the database has a large pending backlog and the scheduler repeatedly re-arms the wake, `EarliestPendingCreatedAt` filters by `status` and `worker_id` while ordering by `created_at`, but the jobs table has no index covering that combination. With a large pending backlog, each startup, timeout tick, or wake can scan and sort many pending rows, adding recurring database work to the scheduler.
Add an index that supports the pending, unassigned `created_at` lookup, and verify the query plan uses it.
Comment on lines
+86
to
+87
| deadline := time.Now().Add(time.Second) | ||
| backdatePendingAnchor(t, database, job.ID, deadline.Add(-noWorkerTimeout)) |
There was a problem hiding this comment.
🟡 Medium · Wake test can time out
When test setup or scheduling takes more than one second before the scheduler arms the wake, armStarvationWake sees the test deadline has passed and leaves judgment to the one-hour timeout tick, so the test's 30-second wait expires despite the fallback being intentional.
Set the test deadline relative to a synchronized scheduler start, or otherwise ensure the scheduler arms the wake before the deadline passes.
Prompt for AI agents
In `pkg/server/scheduler/scheduler_starvation_wake_test.go` at lines 86-87:
**Wake test can time out**
When test setup or scheduling takes more than one second before the scheduler arms the wake, `armStarvationWake` sees the test deadline has passed and leaves judgment to the one-hour timeout tick, so the test's 30-second wait expires despite the fallback being intentional.
Set the test deadline relative to a synchronized scheduler start, or otherwise ensure the scheduler arms the wake before the deadline passes.
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
1 participant
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.
Closes TSI-3766
Issue: https://multica.ai/issues/01a133b5-b046-7e37-999e-559094cb13fd
改动
WORKER_CRASH的 Server 侧分支由 starvation sweep 判定，而 sweep 此前只挂在巡检 tick 上：判定点 = 「pending 时钟到期」之后的第一个 tick，落在 (120s, 150s] 里的哪一点完全由 tick 相位决定。生产默认配置下 tick 周期 30s 整除 no-worker 超时 120s，到期时刻正好压在 tick 边界上，亚毫秒级相位差就决定判定落在 +120s 还是 +150s —— 这正是 TSI-3765 首跑（CLI 240s 包裹内只看到cancelled）与重跑（232s 才拿到[WORKER_CRASH]）两种结果的来源，也是「CLI 到点先放弃」错序的来源。pkg/server/db/db.go：新增EarliestPendingCreatedAt()—— 最早的「pending 且未认领」作业的created_at，即 starvation sweep 的 pending 时钟锚点（迁移、超时重排、重启恢复都会刷新该字段）。过滤条件与GetStarvedPendingJobs对齐：已被认领（worker_id IS NOT NULL）的作业永不被 sweep，不得把锚点提前。pkg/server/scheduler/scheduler.go：新增一次性到期唤醒starvationWake。循环启动时与每次巡检 tick 后按锚点重新瞄准，唤醒点 = 锚点 +--no-worker-job-timeout+starvationExpiryMargin(1s，用于越过 sweep 的严格比较——pending 时长必须严格大于阈值)；唤醒后照常执行同一个checkNoWorkerStarvation()（幂等，保留「存在存活可调度 worker 则短路不判」的语义），再重新瞄准下一个锚点。判定点因此收敛到 (120s, 121s]，与 tick 相位无关；巡检 tick 仍是兜底路径（锚点已过期、或唤醒来不及瞄准时由 tick 判定，例如作业一直卡在有存活 worker 的守卫后面）。契约语义未变：
03-contracts.md§3.1 的分类口径、§5 的巡检 tick 配置项、05-modules/server.md的 sweep 伪代码（cutoff 比较、短路条件、终态写入 + 限流释放 + 广播）全部保持原样，改的只是判定时刻不再被 tick 相位量化；NoWorkerDeadline（created_at + timeout + interval，客户端放弃点上界）不动，保证 CLI 永远不会先于服务端判定放弃。验证
TestStarvationWakeJudgesJustPastDeadline：tick 设为 1h（唯一可能的触发就是到期唤醒），锚点使到期点在 1s 后；断言判定严格晚于到期点、且距到期点不超过 margin+4s，分类为NO_WORKER_AVAILABLE。修复前该测试失败（无唤醒 → 30s 内无任何判定）。TestStarvationWakeFallsBackToTickForAnExpiredDeadline：锚点已过期时没有未来唤醒点，tick（500ms）必须照常判定 —— 唤醒只是加速器，不是唯一路径。TestEarliestPendingCreatedAt：空表返回「无锚点」；更早但已被认领（queued）的作业不得把锚点提前。kill -9worker），判定点相对迁移锚点：--timeout-check-interval 45s（tick 不再整除 120s 超时，使相位效应可见）：修复后 +121.0013s，修复前 +150.0011s（落在下一个 tick 上，晚了 30s）status=failed、failure_type=WORKER_CRASH、stderr 精确Job failed: [WORKER_CRASH] worker crashed (heartbeat timeout) and no surviving worker to take over the job、CLI rc=1，均未出现cancelled错序。gofmt、go vet干净；go test ./...：本次改动的包（pkg/server/db、pkg/server/scheduler、pkg/server/handlers、pkg/server/workerhealth）全绿。pkg/worker的 16 个用例在 clean HEAD26e0159上以完全相同的集合失败（本沙箱 ffmpeg fixture 不兼容；pkg/worker/cmd/worker不依赖本次改动的任何包），与本 PR 无关。Summary by Sourcery
Stabilize starvation-sweep timing so worker-crash verdicts are produced just after the no-worker deadline regardless of inspection tick phase.
Bug Fixes:
Enhancements:
Tests: