Skip to content

fix(cron): avoid heartbeat deadlock behind fire fence - #109392

Open
bludiesel wants to merge 2 commits into
NousResearch:mainfrom
bludiesel:fix/cron-fire-claim-heartbeat
Open

bludiesel wants to merge 2 commits into
NousResearch:mainfrom
bludiesel:fix/cron-fire-claim-heartbeat

Conversation

@bludiesel

Copy link
Copy Markdown

Summary

  • let claim heartbeat use the jobs lock and owner CAS directly while the worker holds its fire fence
  • add a regression test for the cross-thread heartbeat path
  • update audited npm dependencies, including Electron, Vitest, and the WhatsApp bridge

Verification

  • scripts/run_tests.sh tests/cron/test_claim_job_for_fire.py (18 passed)
  • web tests: 298 passed
  • TUI tests: 1769 passed, 3 skipped
  • npm audits: 0 vulnerabilities across root, web, TUI, and WhatsApp bridge

Known baseline

  • desktop UI suite retains 2 pre-existing voice preference assertion failures on main; reproduced before this branch.

@bludiesel
bludiesel requested a review from a team September 12, 2026 20:09

@gaoanze888 gaoanze888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The heartbeat change itself addresses the same already-open issue as #109079 (Fix cron heartbeat starvation during long deliveries). #109079 predates this PR and changes the same heartbeat_fire_claim() path, preserving the fire fence except when this process proves it already owns it. Please coordinate/close one implementation so there is a single review thread and the earlier author's credit is preserved.

There is also substantial unrelated scope in commit 0f14a616: Electron/Vitest upgrades and an Express 4→5 major upgrade across root/desktop/TUI/web/WhatsApp lockfiles. That is not needed for the cron deadlock fix, makes the concurrency change much harder to review, and introduces independent runtime/packaging compatibility risk. Please split the dependency remediation into its own PR (or drop it here).

For the cron fix, I would also prefer retaining cross-process fire-fence semantics when no same-process holder is known (as #109079 does), rather than bypassing _under_fire_fence unconditionally. The owner CAS under _jobs_lock protects the current claim field, but the fire fence is documented as serializing owner mutations and external side effects across processes. A focused regression should cover both cases: same-process worker + heartbeat does not deadlock, while an unrelated process that does not hold the fire fence cannot use this helper to bypass that fence.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management dependencies Pull requests that update a dependency file duplicate This issue or pull request already exists labels Sep 12, 2026
@alt-glitch

Copy link
Copy Markdown

This was generated by AI during triage.

Duplicate of #109310 (merged 2026-09-12): heartbeat_fire_claim on main already bypasses _under_fire_fence and relies on the jobs-lock CAS, which is the same change this PR makes. The remaining diff is unrelated npm dependency bumps; if those are wanted they should be a separate PR.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management dependencies Pull requests that update a dependency file duplicate This issue or pull request already exists P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants