Skip to content

fix: deliver Tachometer shutdown to its Slurm step - #428

Closed
nv-yna wants to merge 1 commit into
NVIDIA:mainfrom
nv-yna:yna/tachometer-step-shutdown
Closed

nv-yna wants to merge 1 commit into
NVIDIA:mainfrom
nv-yna:yna/tachometer-step-shutdown

Conversation

@nv-yna

@nv-yna nv-yna commented Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Tachometer shutdown currently sends SIGTERM to the local srun client. Slurm25.11.8 aborts the remote task with SIGKILL in response, so the scraper cannot flush and compact its capture. SRT then reports the launcher as having stopped gracefully without checking its exit code. This occurred in Hecate run579709: the scraper step ended0:9 and produced no final artifact.

Record the scraper's exact Slurm job.step in a private file unique to its launch, then exec the scraper with its original arguments. A Tachometer-specific process class signals that recorded step, keeps srun alive for the configured compaction grace, and checks the launcher exit code. Timeout escalation targets only the same step. Missing or invalid identity and signal-command failures are reported without falling back to launcher or allocation signals. Registry cleanup uses the same shutdown behavior.

Validation:

  • make check: 2,178 passed, 2 skipped, 6 deselected. Final focused suite: 154 passed, including the added production-registration regression. Ruff and whitespace checks passed.
  • Real Bash wrapper tests verify exec PID, argument preservation, private identity, and allocation validation; shutdown tests cover configured grace, nonzero exit status, invalid/stale metadata, signal failures, registry cleanup, and scoped timeout escalation.
  • On Slurm25.11.8, two isolated CPU-only helper steps demonstrated the failure and delivery correction: launcher TERM produced task SIGKILL/exit137; exact-step TERM reached the handler and finished0 after cleanup.
  • An additional end-to-end Slurm smoke used the exact committed TachometerStep.command and TachometerProcess.terminate API. Step579710.37 completed0:0, and seven8-row shards reconciled to56 fully decoded rows in byte-identical local/uploaded final files.
  • Three small synthetic-endpoint runs with the exact scraper binary completed0 after exact-step TERM. Final local and uploaded Parquet files matched and fully decoded. The rotating case completed three periodic compactions and reconciled seven8-row shards to56 final rows.

The cluster tests validate the signal mechanism and bounded scraper finalization; they do not certify production-scale compaction or resolve the separate Rust task-quiescence risk. Atomic shard publication is addressed independently by #427. Related open PR #407 by @ishandhanani, "[2.0] The full 2.0 stack: schema v2, services, graceful shutdown, observability, MCP, and the migrator (#386-#406 and after)", also introduces step signaling within its broader v2/services/cleanup migration, using name lookup and a launcher fallback. This patch keeps the main-line fix limited to Tachometer with an exact launch identity; it does not change #407.

Signed-off-by: Yuewei Na <nv-yna@users.noreply.github.com>
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.89474% with 2 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@4d3e8bd). Learn more about missing BASE report.

Files with missing lines Patch % Lines
src/srtctl/core/tachometer_process.py 97.70% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #428   +/-   ##
=======================================
  Coverage        ?   75.38%           
=======================================
  Files           ?      110           
  Lines           ?    16207           
  Branches        ?        0           
=======================================
  Hits            ?    12218           
  Misses          ?     3989           
  Partials        ?        0           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@ishandhanani

Copy link
Copy Markdown
Collaborator

Closing with the 2.0 merge (#407, part 18): tachometer, every worker, frontend, and service are named Slurm steps, and cleanup delivers SIGTERM with scancel --signal=TERM --full <job>.<step>; the scraper gets shutdown_grace_secs. Same fix, different mechanism (step name rather than a recorded step id). If the exec-PID and stale-metadata coverage from this PR is still wanted, please reopen against main.

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