Skip to content

feat(progress): add clear_jobs() to remove all top-level jobs#94

Merged
jdx merged 2 commits into
mainfrom
feat/clear-jobs
May 11, 2026
Merged

feat(progress): add clear_jobs() to remove all top-level jobs#94
jdx merged 2 commits into
mainfrom
feat/clear-jobs

Conversation

@jdx

@jdx jdx commented May 11, 2026

Copy link
Copy Markdown
Owner

Summary

stop() and stop_clear() end the rendering session — they stop the background thread, render/clear the display, and reset LINES — but they leave the completed jobs registered in the global JOBS Vec. With the default ProgressJobDoneBehavior::Keep, a caller that starts a fresh session by adding a new job after stop will see the next refresh re-render every previously-completed job alongside the new ones.

This bit mise in jdx/mise#9774: mise upgrade finishes installing (calls stop()), then adds an "uninstall" progress job, and the installed-tools block gets printed twice.

Adding clear_jobs() lets callers explicitly drop the registered jobs after stopping, without needing to hold Arc<ProgressJob> references to each.

What's not changing

  • stop()/stop_clear() semantics — narrow contract preserved
  • job_count() / active_jobs() — continue to reflect what's currently registered, including completed-but-still-displayed jobs

clear_jobs() is purely additive. mise (and any other caller that wants the old behavior) can opt in.

Test plan

  • cargo build
  • cargo test -- --test-threads=1 (117 passing, including new test_clear_jobs)
  • cargo clippy (no new warnings)
  • cargo fmt --check

Follow-up: jdx/mise#9779 currently has a mise-side workaround tracking jobs in a Vec; once this lands and clx is released, the mise PR will be simplified to call progress::clear_jobs() instead.

🤖 Generated with Claude Code


Note

Medium Risk
Modifies global progress lifecycle state by clearing JOBS and resetting the STOPPING latch, which could affect rendering/thread behavior across sessions if misused.

Overview
Adds a new public clear_jobs() API to explicitly drop all registered top-level progress jobs and reset the internal stopped state, allowing a new progress “session” to render/animate normally after stop()/stop_clear().

Includes new unit tests covering registry clearing behavior and verifying that clear_jobs() resets the STOPPING flag so subsequent jobs refresh correctly.

Reviewed by Cursor Bugbot for commit 0bec5e3. Bugbot is set up for automated code reviews on this repo. Configure here.

`stop()` and `stop_clear()` end the rendering session but leave completed
jobs registered in the global JOBS list (default ProgressJobDoneBehavior
is Keep). When a caller starts a fresh session after stop, the next
refresh re-renders every job still registered alongside the new ones.

Add `clear_jobs()` so callers can explicitly drop those jobs after
stopping. This is additive — `stop()`'s contract is unchanged, and
`job_count()`/`active_jobs()` continue to reflect what's currently
registered.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request introduces a clear_jobs function to the progress state to allow clearing the job registry, along with a corresponding unit test. Review feedback highlights that the current implementation is incomplete for its intended purpose of starting a fresh session; it fails to reset the STOPPING flag and other internal states like LAST_OUTPUT, which would prevent the background rendering thread from restarting for new jobs.

Comment thread src/progress/state.rs
Comment on lines +333 to +335
pub fn clear_jobs() {
JOBS.lock().unwrap().clear();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The current implementation of clear_jobs() only clears the JOBS registry but does not reset the STOPPING flag. Since stop() and stop_clear() set STOPPING to true, any subsequent attempt to start a new progress session (by adding a new job) will fail to trigger the background rendering thread because notify() and start() return early when STOPPING is true.

To support the "fresh session" use case described in the PR, clear_jobs() should reset STOPPING to false. Additionally, clearing LAST_OUTPUT and calling clear_osc_progress() ensures a completely clean state for the next session, preventing the smart-refresh optimization from potentially skipping the first frame of the new session.

Suggested change
pub fn clear_jobs() {
JOBS.lock().unwrap().clear();
}
pub fn clear_jobs() {
JOBS.lock().unwrap().clear();
STOPPING.store(false, Ordering::Relaxed);
LAST_OUTPUT.lock().unwrap().clear();
clear_osc_progress();
}

@greptile-apps

greptile-apps Bot commented May 11, 2026

Copy link
Copy Markdown

Greptile Summary

Adds a new public clear_jobs() function that drops all registered top-level jobs from the global JOBS registry and resets the STOPPING flag, enabling callers to start a fresh rendering session after stop() without stale jobs re-appearing.

  • clear_jobs() clears JOBS then sets STOPPING = false; the ordering is correct (jobs are dropped before rendering is re-armed, preventing a window where old jobs could be re-rendered).
  • Two new tests cover the core contract: test_clear_jobs confirms the count resets to zero and new jobs start cleanly; test_clear_jobs_rearms_rendering_after_stop verifies the STOPPING flag is reset.
  • clear_jobs() is re-exported from progress::mod alongside the existing control functions.

Confidence Score: 5/5

Safe to merge; purely additive API with no changes to existing stop/start semantics.

The change is a small, additive function with no modifications to existing behavior. The one noteworthy issue — that the background thread may still be sleeping when STOPPING is reset, leaving a stale thread alongside a newly spawned one — is a narrow race masked in practice by REFRESH_LOCK serialization and LAST_OUTPUT smart-refresh, and self-corrects on the next stop() call.

src/progress/state.rs — specifically the interaction between clear_jobs() and the background thread's notify_wait() sleep cycle.

Important Files Changed

Filename Overview
src/progress/state.rs Adds clear_jobs() which clears JOBS and resets STOPPING; has a narrow race where the background thread may still be alive (sleeping in notify_wait) when STOPPING is reset, potentially leaving a stale thread running alongside a newly spawned one
src/progress/mod.rs Re-exports clear_jobs() and adds two unit tests verifying job registry reset and STOPPING re-arm; test design is sound and cleans up after itself

Fix All in Claude Code

Reviews (2): Last reviewed commit: "fix(progress): reset STOPPING in clear_j..." | Re-trigger Greptile

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 396c2a5. Configure here.

Comment thread src/progress/state.rs
`stop()` and `stop_clear()` set STOPPING=true and nothing ever clears it.
Without resetting it, the documented "fresh session" workflow silently
half-breaks: notify() and start() short-circuit on STOPPING so no
background thread restarts, and update() returns early so animated
spinners and incremental messages don't render. Only synchronous
terminal-state flushes from set_status(Done/Failed/...) still get
through via refresh_once (which doesn't check STOPPING) — which is why
the symptom in mise (jdx/mise#9774) was a final-frame re-render rather
than complete silence.

Have clear_jobs also reset STOPPING so a caller who explicitly opts
into a new session actually gets one.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@jdx jdx merged commit b346840 into main May 11, 2026
8 checks passed
@jdx jdx deleted the feat/clear-jobs branch May 11, 2026 16:11
@jdx jdx mentioned this pull request May 11, 2026
jdx added a commit to jdx/mise that referenced this pull request May 11, 2026
…ng jobs

clx 2.1.0 (jdx/clx#94) ships `clear_jobs()`, which removes all top-level
jobs from clx's global JOBS registry and resets the STOPPING flag. That
makes the mise-side `tracked_jobs` Vec — and the `pub(crate)` widening of
`ProgressReport.job` it required — unnecessary.

reset_jobs() now drops `header_job` and calls `progress::clear_jobs()`.
Behavior is unchanged for the bug this PR fixes (#9774), and the
"start a fresh session after stop" workflow now works fully (animated
spinners and incremental updates render again, not just the final
terminal-state flush via refresh_once).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.

1 participant