Fix capture cleanup ownership and manual conflict recovery

[rvanmelle]

Manual profiling can remain busy after a rejected start. Automatic capture cleanup can release ownership too early and stop a newer capture.

This PR fixes both lifecycle paths. It retains the backend lock until results are collected, stops successful runs once, and clears manual busy state after a rejected start.

The defects reproduce on 0.6.0 and 0.7.0. The detailed output below uses unmodified 0.7.0.
This is a profiling reliability fix. Production frequency and performance impact are not established.

Design decisions

• Put completion in BaseBackend#stop_and_results, so StackProf and Vernier use the same lifecycle.
• Call the backend's existing stop method. Preserve overrides instead of calling native collectors directly from the shared helper.
• Defer lock release only on the thread collecting results. Release in ensure, including collection failures.
• Clear the cached profiler before releasing the lock in AppProfiler.stop. Cleanup must not inspect or stop a subsequent owner.
• Set the manual server's busy flag from the actual start result.

Direct backend callers using separate stop and results calls keep their existing behavior.
This does not redesign backend switching, capture cancellation, or ownership of captures started outside app_profiler.
The lock now covers result conversion as well as capture. That extends serialization and is a deliberate tradeoff.

Validation

• Full local suite: 266 runs, 910 assertions, zero failures, errors, or skips.
• Ruby lint: 71 files inspected, no offenses.
• Local environment: Ruby 3.3.1, Rails 7.2, Rack 2.2, Vernier 1.10.1.
• Regressions cover conflict recovery, one stop per successful run, retained locks for both backends, stop overrides, cache handoff, and collection-error cleanup.
• The conflict, double-stop, collection-lock, and cache-handoff regressions failed against original code before the fixes.
• Fork CI: all 36 Ruby/Rails/Rack test combinations and lint passed.
• The upstream PR's CLA check passed. Its test matrix runs in the fork, as linked above.

Failure sequences and the corrected ownership boundary

Rejected manual start leaves the server busy

The server marks itself busy before it knows whether AppProfiler.start succeeded.
A conflict returns 409 without running the stop path that resets that flag.
The same server application then rejects later requests, even after the original capture ends.

sequenceDiagram
    participant A as Existing capture
    participant C as Manual client
    participant S as ProfileApplication
    participant P as AppProfiler
    A->>P: Start capture
    C->>S: GET /profile
    S->>S: profile_running = true
    S->>P: start
    P-->>S: false (conflict)
    S-->>C: 409
    A->>P: Stop capture
    loop Later manual attempts
        C->>S: GET /profile
        S->>S: Busy flag is still true
        S-->>C: 409
    end

Loading

Early unlock allows one capture to stop another

BaseBackend#run calls stop, reads results, then calls stop again from ensure.
The first stop releases the shared lock before result collection.
A competing capture can start in that gap. StackProf startup also clears uncollected prior results.
The first run can lose its result and stop the competing capture prematurely.

sequenceDiagram
    participant A as Capture A run
    participant L as Backend lock
    participant P as StackProf
    participant B as Capture B
    A->>L: Acquire
    A->>P: Start and run workload
    A->>P: stop
    A->>L: Release
    B->>L: Acquire
    B->>P: Clear prior results and start
    A->>P: Read results
    P-->>A: No completed A profile
    A->>P: ensure calls stop again
    Note over P,B: B is stopped prematurely

Loading

Corrected completion retains ownership

sequenceDiagram
    participant A as Capture A
    participant L as Backend lock
    participant P as Backend stop/results
    participant B as Competing capture
    A->>L: Acquire and start
    A->>A: Run workload
    A->>P: stop_and_results
    P->>P: Call overridden stop once
    Note over A,L: Defer release on the collecting thread
    B->>L: Try to acquire
    L-->>B: Rejected while results are collected
    P->>P: Read and convert results
    opt AppProfiler.stop
        P->>P: Discard cached profiler
    end
    P->>L: Ensure release
    P-->>A: Completed profile
    B->>L: A later capture can acquire

Loading

A process using low-rate sampling can still work successfully most of the time.
The failures require contention or a rejected manual start. The reproduction controls those conditions; it does not estimate their production frequency.

Standalone reproduction and captured before/after output

Save the following as /tmp/reproduce_lifecycle.rb.
Run bundle exec ruby /tmp/reproduce_lifecycle.rb from a checkout of app_profiler.
Use unmodified v0.7.0 for the baseline, then this branch for the fixed result.
No Rails app, cloud bucket, network service, or workload data is required.

require "bundler/setup"
require "active_support/all"
require "fileutils"
require "app_profiler"
require "json"

observations = { version: AppProfiler::VERSION }
app = AppProfiler::Server.const_get(:ProfileApplication).new
request = Rack::MockRequest.env_for("/profile?duration=0&mode=cpu&interval=5000")

raise "initial capture failed" unless AppProfiler.start(mode: :cpu, interval: 5000)
observations[:manual_status_during_conflict] = app.call(request).first
AppProfiler.stop
observations[:manual_status_after_conflict_clears] = app.call(request).first

first = AppProfiler::Backend::StackprofBackend.new
competitor = AppProfiler::Backend::StackprofBackend.new
first.define_singleton_method(:results) do
  observations[:lock_free_before_results] = !self.class.run_lock.locked?
  observations[:competitor_started_before_results] = competitor.start(mode: :cpu, interval: 5000)
  super()
end
first.run(mode: :cpu, interval: 5000) { 1_000.times { Math.sqrt(42) } }
observations[:competitor_still_running_after_first_run] = competitor.running?
puts JSON.pretty_generate(observations)
competitor.stop if competitor.running?

The method hook inserts capture B at A's result boundary. This makes the interleaving deterministic without sleep-based races.

Captured baseline output from unmodified 0.7.0:

{
  "version": "0.7.0",
  "manual_status_during_conflict": 409,
  "manual_status_after_conflict_clears": 409,
  "lock_free_before_results": true,
  "competitor_started_before_results": true,
  "competitor_still_running_after_first_run": false
}

Captured output with this patch:

{
  "version": "0.7.0",
  "manual_status_during_conflict": 409,
  "manual_status_after_conflict_clears": 200,
  "lock_free_before_results": false,
  "competitor_started_before_results": false,
  "competitor_still_running_after_first_run": false
}

In the baseline, B starts successfully and A subsequently stops it.
With this patch, B cannot start during collection. Its final false means it never started in that interval.

Regression commands:

bundle exec ruby -Itest test/app_profiler/profile_server_test.rb
bundle exec ruby -Itest test/app_profiler/backend/stackprof_backend_test.rb
bundle exec ruby -Itest test/app_profiler/backend/vernier_backend_test.rb
bundle exec rake test
bundle exec rubocop

Compatibility approaches considered, with generic sample fixes

An application can prepend version-specific adapters around the manual server and StackProf backend.
Those adapters depend on private server methods and duplicate collector ownership rules.
These generic excerpts illustrate the behavior considered. They are not complete installation-ready initializers.

Manual server adapter: ProfileApplicationCompatibility behavior

Inside the existing semaphore and after the busy guard, derive busy state from the actual result:

# Replacement for ProfileApplication#start_running's state update:
@semaphore.synchronize do
  return false if @profile_running

  @profile_running = !!AppProfiler.start(**stackprof_args)
end

A manual stop adapter also needs to retain the shared lock through result collection.
Resetting only the busy flag leaves the collection race present.

Backend adapter: StackprofBackendCompatibility behavior

A StackProf-only adapter can collect results before releasing its existing lock:

# Completion excerpt, entered only after a successful start with lock ownership:
begin
  StackProf.stop
  data = StackProf.results
ensure
  AppProfiler::Backend::StackprofBackend.run_lock.unlock
end
profile = AppProfiler::BaseProfile.from_stackprof(data) if data

The complete run wrapper must also stop once, release rejected starts before running application code, preserve block exceptions, and assign duration metadata.
It must not stop another capture after releasing ownership.
A native-only implementation also bypasses backend stop overrides and does not cover Vernier.

This PR moves those ownership rules into the shared backend lifecycle instead of requiring each application to maintain those adapters.
The existing stop and results methods remain the dispatch points.

Related prior work:

• #201 proposed fixing the manual busy state. It closed without merge; this PR adds recovery regression coverage.
• #139 addressed repeated unlock behavior. Avoiding an unlock warning does not prevent the second native stop.
• #218 fixed separate CORS configuration behavior. This PR does not change CORS.