fix(intercept): embedded handle still refuses intercept() after its listener is stopped through the admin plane #316

Problem

On the embedded transport, once this handle starts an intercept over FFI (rift.intercept()), another client can stop that listener through the engine's admin plane (DELETE /intercept on rift.adminUri()). The C-ABI and the admin plane share one InterceptControl slot (since achird-labs/rift#521). RiftImpl's interceptStarted flag (rift-java-core/src/main/java/io/github/achirdlabs/rift/RiftImpl.java, intercept()/attach paths around lines 447-470) is never cleared by the plane-side stop. A second rift.intercept() then throws IllegalStateException: already started until the original Intercept handle is closed, even though no listener runs.

After the #301 fix, rift.interceptStatus() correctly reports empty, so the two disagree.

Evidence

Found by code review of the #301 fix (branch fix/rift-301-intercept-status-shared-slot).

Repro:

1. Embedded Rift, rift.intercept() succeeds.
2. HTTP DELETE <adminUri>/intercept → 204.
3. rift.interceptStatus() → empty.
4. rift.intercept() → IllegalStateException: already started.

Proposed fix

• When interceptStarted is set and transport.interceptStatus() reports no running listener, clear the flag so the new start proceeds (or let the new start proceed and invalidate the stale Intercept handle).
• Add an InterceptE2eIT case covering: FFI start → admin-plane DELETE /intercept → interceptStatus() empty → rift.intercept() succeeds.

Should this exist?

OPEN: should a plane-side stop invalidate this handle's Intercept object (so its close()/use is a no-op or an error), or should it only allow a new start to proceed?

Provenance

Found by an agent while reviewing the #301 fix (PR for #301). Out of scope for that work, filed as a follow-up. Related to #301 (status staleness) but distinct: #301 covers the status read path, this covers the start refusal.

Triage

Verdict: build now, no engine gate. The bug is real but not specific to embedded: RiftImpl's per-handle latch goes stale on every transport. The OPEN question has an answer: a plane-side stop must invalidate the stale handle. If it does not, the old handle's close() would stop the next listener. Route: Opus.

Engine facts (checked against v0.22.0 64cee158; nothing in v0.22.0..origin/master touches intercept)

• There is one intercept slot per FFI handle. RiftHandle.intercept: InterceptControl is defined at crates/rift-ffi/src/lib.rs:129, and the admin plane gets the same Arc through .with_intercept(handle.intercept.clone()) at lib.rs:2212. The slot is Arc<Mutex<Option<InterceptPlane>>> (crates/rift-http-proxy/src/intercept_control.rs:164-166). This is the shared slot from rift#521.
• DELETE /intercept calls control.stop() and returns 204 every time, including when nothing runs (admin_api/handlers/intercept.rs:157-163). rift_stop_intercept is the same call (lib.rs:1634-1643). stop() takes the plane out of the slot (intercept_control.rs:612-622).
• GET /intercept returns 200 with {interceptPort, interceptUrl}, or 404 when no listener runs (handlers/intercept.rs:149-155). It is backed by status() -> Option<SocketAddr> (intercept_control.rs:625-627). A listener has no identity or generation, only its address. A stop followed by a restart on the same port therefore looks the same as the original listener. The route exists since v0.13.0 (cc8d3778), so no version gate is needed.
• A second start while the slot is occupied is refused by the engine as AlreadyRunning, which the plane answers with 409 (intercept_control.rs:353-358, 502). The engine already prevents a double start. The SDK latch only gives a nicer error and avoids a round trip.
• Rule calls on an empty slot fail with "intercept not started" (lib.rs:1669-1671).

rift-java today (master df1cb00)

• RiftImpl.interceptStarted (rift-java-core/.../RiftImpl.java:71) is set by CAS at :450 and cleared only by the InterceptImpl.onClosed callback (:463,474,490) or a failed start (:485,515). Nothing clears it when the slot is emptied from outside.
• The bug is not embedded-only. The latch lives in RiftImpl and covers every transport. On connect/spawn/testcontainers, another client's DELETE /intercept leaves it set in the same way. Embedded is just the easy repro.
• InterceptImpl.close() (InterceptImpl.java:378-400) calls transport.stopIntercept() unconditionally when owned, and clears rules when the handle is attached. Suppose the plane stops listener A and someone (another client, or this handle once the latch is lifted) starts B. A's close() then stops B, or for an attached handle clears B's rules. The issue's option of "only allow a new start to proceed" would turn this latent hazard into the normal path.
• The status read path is already correct after fix(intercept): embedded interceptStatus() caches the FFI start and goes stale when the shared slot changes via the admin plane #301 / PR fix(intercept): embedded interceptStatus() caches the FFI start and goes stale when the shared slot changes via the admin plane #317. EmbeddedTransport.interceptStatus() defers to admin.interceptStatus() once a plane is served (EmbeddedTransport.java:333-341). RemoteTransport.interceptStatus() maps 404 to empty (RemoteTransport.java:558-567).
• InterceptImpl.engineBound holds the engine-side address for owned and discovered handles (InterceptImpl.java:86, 108). It is null for attach(host, port) (:135).
• Rift.intercept docs (Rift.java:153-158) and Intercept docs (Intercept.java:18-19, 100-107) promise "one at a time; close() frees the engine".

Decision

• Fix in RiftImpl and InterceptImpl (core), for all transports. Do not fix it in EmbeddedTransport. The premise "embedded only" is wrong.
• Should question (answered): the SDK should treat a handle as superseded when the engine slot no longer holds its listener. A superseded handle is closed without any engine call. Its close() does nothing, its rule ops and trust() throw IllegalStateException("intercept listener was stopped outside this handle"), and its address accessors still answer, as they do after a normal close. Allowing a new start without this invalidation is unsound, because the stale close() would kill the next listener.
• A handle counts as stale when interceptStatus() is empty, or when engineBound != null and the reported port differs. The second case means someone else restarted the listener. The engine will 409 the new start, but the stale handle must still never stop or clear that other listener. A restart on the same port cannot be detected because the engine has no listener identity. That limit is accepted and documented.
• If the transport cannot report status (UnsupportedOperationException, the RiftTransport default at :263), keep today's refusal.

Design

// RiftImpl: replace AtomicBoolean interceptStarted with a guarded reference
private final Object interceptLock = new Object();
private InterceptImpl openIntercept;      // null when free
private boolean interceptStarting;        // start in flight (keeps the "no transient second listener" rule)

public Intercept intercept(InterceptOptions options) {
    synchronized (interceptLock) {
        if (interceptStarting) throw alreadyStarted();
        if (openIntercept != null && !openIntercept.supersedeIfStale()) throw alreadyStarted();
        openIntercept = null;
        interceptStarting = true;
    }
    // ... existing start/attach paths unchanged; on success: set openIntercept = impl, interceptStarting = false
    //     on failure: interceptStarting = false. onClosed callback: clear openIntercept if it is that impl.
}

// InterceptImpl
/** True (and the handle closed without touching the engine) when the engine slot no longer holds
 *  this handle's listener; false when it still does, or the transport cannot tell. */
synchronized boolean supersedeIfStale() {
    if (closed.get()) return true;
    Optional<InterceptStatus> now;
    try { now = transport.interceptStatus().map(r -> InterceptStatus.fromJson(r, "intercept status")); }
    catch (UnsupportedOperationException e) { return false; }
    boolean stale = now.isEmpty() || (engineBound != null && now.get().port() != engineBound.getPort());
    if (stale) { superseded = true; closed.set(true); }   // no stopIntercept / interceptClearRules
    return stale;
}

@Override public synchronized void close() {
    if (closed.get()) return;
    if (supersedeIfStale()) { onClosed.run(); return; }  // never stop or clear a listener we don't hold
    // ... existing body
}
// requireOpen(): message says "stopped outside this handle" when superseded, else "intercept is closed".

supersedeIfStale takes the handle's own monitor, so it serialises with a concurrent close(). RiftImpl calls it under interceptLock without holding the handle's monitor in the opposite order, which avoids lock-order inversion. onClosed must not take interceptLock while it holds the handle monitor, or must be lock-free, for example compareAndSet on an AtomicReference<InterceptImpl>. The AtomicReference is the simpler choice.

Implementation plan

• rift-java-core/.../RiftImpl.java: replace the interceptStarted latch with an open-handle reference plus an in-flight flag. Check for supersession on refusal. onClosed clears only its own handle.
• rift-java-core/.../InterceptImpl.java: add supersedeIfStale() and a superseded flag. close() checks staleness before it stops or clears. Give requireOpen() a distinct message.
• Rift.java:153-158 and Intercept.java:18-19, 100-107 javadoc: a listener stopped or replaced through the admin plane (or by another client) supersedes this handle, and a new intercept() may proceed. Mention the same-port restart limit.
• CHANGELOG/README intercept section if they repeat the "one at a time" contract.
• No transport or SPI change. RiftTransport already has interceptStatus().

Tests

• Unit (rift-java-core, proxy RiftTransport in the style of InterceptAddressTest.java:98), each case one test:
  • After a start, status flips to empty. intercept() succeeds, the old handle's ops throw ISE, and its close() makes no stopIntercept call.
  • Status reports a different port. The new start reaches startIntercept (whose 409 is surfaced), and the old close() makes no stop.
  • Status still reports the same port. The call is refused as today and startIntercept is never called.
  • Status throws UnsupportedOperationException. The call is refused as today.
  • A normal close() still stops exactly once and frees the latch.
  • An attached handle that is stale: close() makes no interceptClearRules.
• Embedded IT (rift-java-embedded/.../InterceptE2eIT.java, runs under surefire), next to anFfiStartedListenerStoppedThroughTheAdminPlaneIsNoLongerReported (:366-376):
  • FFI start, DELETE <adminUri>/intercept returns 204, status is empty, rift.intercept() succeeds. The old handle's addRule throws ISE. Closing the old handle leaves the new listener running (interceptStatus() still reports its port).
  • FFI start, plane DELETE, viaAdmin.intercept(), then old close(). The admin-started listener is still reported.
• Conformance/remote IT (needs RIFT_IT=1): the same first scenario over Rift.connect with a second client doing the DELETE, to prove the fix is transport-agnostic.

Acceptance criteria

• After a plane-side or other-client stop, rift.intercept() on the same Rift succeeds on embedded and remote transports.
• A stale Intercept never stops, or clears the rules of, a listener it did not start (except the same-port restart, which is documented).
• A second intercept() while this handle's listener is still running is still refused before any engine call.
• Transports that cannot report status keep today's behaviour.
• No public signature changes, so no binary-compat overloads are needed.

Gating: none. Everything relies on GET /intercept (engine >= 0.13.0) and the shared slot, both in v0.22.0. It can land now.
rift-scala: no code change. The facade passes IllegalStateException through (bridge/.../RiftConnector.scala:70-84). Update the docs at RiftConnector.scala:70-75 and InterceptConnector.scala:18-31 to describe the superseded handle when rift-scala bumps to the release that carries this fix.