smarter pruning - #7
Merged
Merged
Conversation
…hing everytime there's a new session (unless there's memory pressure)
lukemarsden
marked this pull request as ready for review
November 18, 2023 14:44
Collaborator
Author
|
tested and working, except for the following issue which i think is unrelated when a runner has a finetuned SDXL inference modelinstance running, any requests for non-finetuned SDXL just hang - why? |
Collaborator
Author
|
for a followup issue, I also suspect there's an issue that this could cause any long-running finetunes to get killed prematurely - we should probably distinguish between ACTIVE and IDLE sessions and not kill the active ones |
lukemarsden
added a commit
that referenced
this pull request
Nov 14, 2025
PROVEN FACTS (from core dump + source analysis): Thread Flow: 1. Thread 99 = HTTPS server (wolf.cpp:187, port 47984) 2. Processing /cancel endpoint (endpoints::https::cancel) 3. Fires StopStreamEvent SYNCHRONOUSLY (event_bus.hpp:171) 4. Handler calls gst_element_send_event FROM HTTPS THREAD 5. GStreamer recursively traverses pipeline (frames #7→#5) 6. Blocks on mutex 0x70537c0062b0 in libgstbase-1.0.so.0 7. Thread 40 (audio pipeline owner) is HEALTHY in ppoll 8. Only Thread 99 waiting on this mutex - no contention GStreamer Analysis: - gst_element_send_event IS thread-safe (uses recursive STATE_LOCK) - Documented as "MT safe" - can be called from any thread - But empirically CAUSES DEADLOCK when called from HTTPS thread - GStreamer has both recursive (STATE/PAD) and NON-recursive (live_lock) mutexes The Mystery: - WHO holds mutex 0x70537c0062b0? NOT Thread 40, not any visible thread - Options: abandoned by crashed thread, corrupted, or race condition - Cannot prove exact mechanism without debugging symbols CONFIRMED FIXES: 1. HTTPS connection leak (100% certain) - add close() in error handler 2. Replace gst_element_send_event with g_main_loop_quit (80% confidence) - Eliminates cross-thread pipeline calls - g_main_loop_quit IS thread-safe (documented) - Even though gst_element_send_event claims to be safe, empirically fails Gaps in Evidence: - No debug symbols for libgstbase (can't see frame #4 function) - Core dump partially corrupted - Can't identify mutex owner - Need symbols + reproduction to prove exact mechanism
lukemarsden
added a commit
that referenced
this pull request
Feb 16, 2026
- Swap Overview and Why This Matters sections for better hook - Add full analysis of REALTIME timer mystery (fires for gui_update but not fence_poll despite identical timer_new_ms calls) - Document thread-based fence polling workaround (thread + BH) - Add "The Debugging Method" section explaining how each fix was found - Update fixes table with fix #7 (thread-based polling) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lukemarsden
added a commit
that referenced
this pull request
Mar 18, 2026
lukemarsden
added a commit
that referenced
this pull request
Mar 18, 2026
Issue #1 (stuck "Starting Desktop"): - Add defer in StartDesktop to clear external_agent_status on any error - Give waitForDesktopBridge its own 90s context decoupled from dockerCtx Issue #4 (status not cleared on stop): - StopDesktop unconditionally clears external_agent_status and status_message Issue #5 (no restart button in Starting state): - Frontend: show Stop button in "starting" state in both screenshot and stream modes - Show "may have failed to start" message after 2-minute timeout Issue #10a (duplicate sessions per spectask): - Re-read task from DB before CreateSession; skip if PlanningSessionID already set Issue #10b (scanner targets wrong sessions): - processPendingPromptsForIdleSessions now filters to canonical planning_session_id only Issue #2 (duplicate message sends): - Add ClaimPromptForSending() atomic store method (UPDATE WHERE status IN pending/failed) - Both interrupt and any-pending delivery paths use claim before send Issue #7 (promotion race gives empty zvol): - resolveDockerDataDir: acquire read lock before fresh zvol creation; re-check after Issue #3: Already handled by existing open_thread on agent_ready reconnect Issue #6: Fixed in merged PR #1947 (RecoverStaleBuilds 60s retry) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Spec-Ref: helix-specs@04b515c3c:001588_read-helixs-design2026
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.
this PR changes the logic so that rather than killing sessions as soon as they become stale (haven't been used in the timeout), they are allowed to keep running forever until something else needs the GPU memory.
in practice, this means we can reduce the timeout a lot to make new sessions more responsive, while also improving latency by, for example, tending to keep an unfinetuned mistral running continuously for new users who try it out (this doesn't enforce it but it will tend to happen now if that mistral has been recently used)