fix: prevent stale module binder deliveries

[LIghtJUNction]

Summary

Prevent a stale module Binder delivery from publishing after the UID it belongs to has gone away.

Changes

• Give each in-flight delivery an attempt token and accept results only from the current attempt.
• Serialize delivery publication, UID removal, and death-recipient cleanup under one lock.
• Keep failure throttling per UID, and watch the exact provider/death-recipient pair so an old callback cannot clear a replacement.
• Keep blocked provider lookups off the UID observer thread so a replacement process can retry immediately.

Validation:

• git diff --check
• ./gradlew :daemon:ktfmtCheck --rerun-tasks --no-daemon --console=plain
• ./gradlew :daemon:testDebugUnitTest --no-daemon --console=plain

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[pashippercode]

Thanks for the writeup — the post-send isCurrent checks and the per-uid generation invalidation correctly close the stale-write race, and invalidating on uidGone so a replacement attempt can take over is right. A few concerns after reading through:

1. The fixed 4-thread pool globalizes the stall it was meant to bound. The comment kept just above binderExecutor says the opposite of what the change does: "One thread per module keeps that local" — sendBinder blocks in getContentProviderExternal until the app publishes or AMS gives up (8.5s+ measured), and with a cached pool only the stuck module's own thread is held. With 4 fixed threads, 4 stuck/crash-looping module apps block every module's delivery on the device, and every subsequent uidStarts queues unboundedly behind them — each queued task is a full AMS timeout. The "unbounded" problem isn't removed, it's moved from threads to the queue and made global. A replacement attempt after uidGone also queues behind the still-blocked worker it replaced. A per-module serial executor (keyed by LoadedModule) with an overall cap, or keeping the cached pool and letting the tracker dedupe, would bound the storm without globalizing the stall.

2. uidClear() is all-or-nothing and wipes the failure throttle. Any single module generation change (one module updated/removed, or a reload triggered by the obfuscation toggle) now clears uidSet for all uids, unlinks every death recipient, and — new, and not mentioned in the description — clears binderFailures entirely. The throttle exists precisely because retrying starts the process: the comment documents 14 starts in 76s from one crash-looping module. Wiping the failure run of every uid on every unrelated module update re-feeds exactly that loop. Unaffected, currently-running module apps are also left without a delivery until their next uid transition. Could the invalidation be scoped to the changed module's uids (per-uid invalidation already exists in the tracker), leaving binderFailures alone?

3. Failures during uid churn are no longer counted. In the worker, recordFailure is only reached when isCurrent still passes after the send — but a uid that disappears mid-send (app dies while launching, the exact pattern binderFailures targets) invalidates the attempt, so the failure is silently dropped. In the old code the same send counted. A crash-looping module may never accumulate the three failures needed to engage the throttle. Failure accounting should not depend on attempt currency.

4. The generation check compares against a stale copy. moduleGenerationChanged is computed from oldState.modules (the copy at the top of the rebuild), but the swap immediately below is deliberately against the current state because other writers mutate it concurrently — the comment at the swap says so. Between the check and the swap the verdict can be wrong in both directions. Computing the diff inside synchronized(this) against the state actually being replaced would make it sound.

5. The tests cover the tracker, not the machinery. The four added tests exercise DeliveryAttemptTracker bookkeeping only. The risky parts — uidClear() vs an in-flight linkDelivery, the death-recipient identity/removal races, executor queueing, uidStarts/uidGone interleavings on ModuleAppService, and the ConfigCache generation detection — have no coverage.

[liyw0205]

The latest commit addresses the global fixed-pool starvation and narrows cache invalidation, but the delivery commit is still not atomic with UID/cache invalidation. Two races can leave a live replacement without a binder until another UID transition occurs.

Validation: git diff --check passed. I could not complete the Gradle tests locally because downloading Gradle 9.6.1 repeatedly timed out; the current head also has no successful reported check (action_required).

[liyw0205]

Blocking race: invalidating attempts and clearing uidSet/deliveries are separate operations. After invalidateMatching() returns, a new-generation uidStarts() can publish a delivery; the following snapshots/removals can then delete that new delivery. In particular, if removeMatching() removes the new entry after the worker added the UID, this method unlinks it and removes uidSet, while the worker's post-link check sees that its registry entry is already gone and cannot distinguish this from a stale commit. More importantly, the returned UID set may trigger another attempt concurrently with the first new-generation attempt. Please make lifecycle invalidation and delivery commit share one synchronization/ownership boundary rather than independently sweeping three stores.

[liyw0205]

Blocking race: the isCurrent() check at line 207 is not atomic with publishing deliveries and uidSet. A possible ordering is: old worker passes line 207; uidGone() invalidates and clears the UID; old worker executes lines 215-218; the replacement process emits its only uidStarts() callback and returns at line 149 because the stale worker temporarily set uidSet; then the old worker notices invalidation at lines 221-224 and rolls itself back. Final state is empty, but the replacement process remains alive without receiving the binder and may emit no further UID edge. The attempt validation and registry/UID publication need to be committed atomically, or a failed stale commit must explicitly schedule the current replacement.

[Tiacoo]

Found one additional ordering race in the retry accounting. The two existing atomicity concerns around delivery publication/invalidation are valid; independently, stale attempt completions can still mutate BinderFailureTracker after a replacement attempt has completed.

recordFailure is deliberately allowed after an attempt has been invalidated, but its update is not ordered against a newer attempt's binderFailures.clear(uid). For example: attempt A starts and blocks; uidGone() invalidates A; replacement B starts and succeeds, clearing the failure run; then A returns null and records a failure after that success. Repeating this churn can throttle a healthy replacement. The inverse is also possible around lines 178-180: A passes isCurrent, B replaces it and records a failure, then stale A clears B's run before linkDelivery notices it is stale.

Please carry the attempt/generation into the failure tracker (or commit result accounting under the same ownership lock) so an older completion cannot overwrite a newer result.

Validation: git diff --check passed. I could not start :daemon:testDebugUnitTest locally because this environment has no JDK; the current head's Core workflow is action_required, so there is no CI test result yet.

[Tiacoo]

Found another gap in cache-generation redelivery: uidClear() can only rediscover UIDs that currently have an active attempt or a successful delivery.

A failed send (sendBinder() == null) finishes its attempt and never enters uidSet or deliveries, even though the module process/provider can remain alive—for example, provider.call may return null without killing the UID. If a new module generation is then published, that live UID is absent from invalidatedUids, so ConfigCache does not call uidStarts(uid). No later UID observer edge is guaranteed, leaving the new generation without its Binder indefinitely.

This contradicts the stated reason for immediate redelivery in ConfigCache (a running module may not emit another UID transition). Please track active module UIDs independently of attempt/delivery success and redeliver matching active UIDs on a generation change. A regression test should cover: failed send → attempt finishes while UID remains active → generation swap → a new attempt is scheduled.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[HSSkyBoy]

This PR, it commits the ultimate faux pas of concurrent state management: "State Fragmentation".

Regard closely the core variables maintaining the state in ModuleAppService.kt:

private val uidSet = ConcurrentHashMap.newKeySet<Int>()
private val sending = ConcurrentHashMap.newKeySet<Int>()
private val deliveries = ConcurrentHashMap<Int, Pair<IBinder, IBinder.DeathRecipient>>()
private val binderFailures = ConcurrentHashMap<Int, FailureRun>()

This is exactly the root cause why the previous AIs could never catch all the Race Conditions. The lifecycle of a single UID (Idle -> Sending -> Delivered / Throttled) is forcibly divided into four different ConcurrentHashMap!

To compensate for the time differential between these four collections, the author (or the AI that generated this code) has accumulated unnecessary complexity, creating three classes—DeliveryAttemptTracker, DeliveryRegistry, and BinderFailureTracker—just to perform external synchronization and identity verification. It is completely an archaic logic of patching.

But voilà, with the Kotlin philosophy, this should be clean and elegant:
We can perfectly encapsulate the state of a single UID into a Sealed Class, and centralize it into one single ConcurrentHashMap<Int, DeliveryState> for management.

sealed class DeliveryState {
    object Idle : DeliveryState()
    data class Sending(val attemptId: Long) : DeliveryState()
    data class Delivered(val provider: IBinder, val recipient: IBinder.DeathRecipient) : DeliveryState()
    data class Throttled(val count: Int, val cooldownUntil: Long) : DeliveryState()
}

As long as one ensures the use of an atomic compute block for the state modifications of a single UID, the state transitions will be absolutely secure. uidGone simply reverts the state of that UID back to Idle and unbinds the old Binder. When the background thread receives the result and wants to update, it only needs to verify if the current state is still the Sending(attemptId) from when it was initiated. If it is not, it is simply discarded.

There is absolutely no need to maintain three extra Tracker classes, and we certainly will not have the problem of being interrupted the exact second after an isCurrent check.

This PR expends a massive amount of effort constructing complex workarounds, without realizing that the underlying architectural design itself is fundamentally flawed.

[LIghtJUNction]

The “state fragmentation” criticism is valid for the current revision. The lifecycle of one UID is split between uidSet, sending, deliveries, and binderFailures, so the original isCurrent checks cannot make validation and publication one atomic transition. The three helper trackers reduce individual races, but they do not make the lifecycle itself coherent.

The PR still has a concrete purpose independent of that design flaw: sendBinder() runs asynchronously because getContentProviderExternal() may block; after uidGone() or a module-cache generation swap, a late worker can otherwise publish a stale Binder, a late death callback can clear a replacement, and an active UID whose send returned null can miss the new generation forever. Those are real stale-delivery and redelivery bugs, not just bookkeeping style issues.

The four-state sketch is directionally right but needs a little more state than shown: an active UID after a failed send must retain its failure run, an in-flight send invalidated by uidGone() needs a tombstone so its failure can still be counted while its late success cannot publish, and provider/death-recipient identity must remain part of the delivered state.

I am restructuring this around one ConcurrentHashMap<Int, DeliveryState> with atomic per-UID compute transitions. The sealed state carries active status, attempt id, failure/cooldown metadata, and the exact provider/recipient pair; the tracker classes are being removed and the race cases are covered at the state-machine level.

[LIghtJUNction]

Follow-up: the refactor is now pushed in dccaba6a (with 2335baeb as the main state-machine change). The three tracker classes are gone; delivery, invalidation, active-UID redelivery, failure cooldown, and provider identity now share the sealed state map. Six state-machine JUnit tests pass, and :daemon:ktfmtCheck --rerun-tasks passes. The full Android unit-test task now passes locally after installing the required SDK packages and accepting their licenses: 8 tests (6 DeliveryStateTest + 2 ConfigCacheTest), 0 skipped, 0 failures, and 0 errors; BUILD SUCCESSFUL with 68 actionable tasks (12 executed, 56 up-to-date). The SDK version configuration remains identical to the PR base (targetSdk = 37, compileSdk = 37, buildToolsVersion = "37.0.0").

[Tiacoo]

I re-reviewed the latest head, d68a0c4, after the state-machine rewrite. Centralizing the state fixes the previously reported split-commit races, but three lifecycle gaps remain:

[P1] A replacement attempt overwrites the uidGone() tombstone before the old failure returns.

begin() replaces an inactive Sending state, while invalidateGone() only flips active to false. Sequence: A starts; uidGone() tombstones A; B starts and replaces the state; A then returns null. recordFailure(uid, A) sees B and ignores A, so repeated crash/restart churn can still evade the three-failure throttle. The tests split the two halves: one covers replacement plus stale success, and one covers stale failure without replacement (tests). Please retain outstanding/tombstoned attempt IDs separately from the current attempt, accept an old failure only while it is registered and no newer success has superseded it, and add begin A -> uidGone -> begin B -> recordFailure(A) coverage.

[P1/P2] An active throttled UID has no wake-up after cooldown.

begin() returns no claim before cooldownUntil, and generation invalidation returns the active UID but preserves Throttled. ConfigCache calls uidStarts() once; if that lands during cooldown, nothing schedules another attempt when the deadline expires. With no later UID observer edge, the new generation can remain without a Binder indefinitely. Return a retry deadline from the transition and schedule one keyed/epoch-checked retry, or permit one generation-change attempt while retaining the failure run. Add a clock-driven test that advances past the deadline without another observer callback.

[P1/P2] A start callback racing provider death is discarded.

For Delivered, begin() only copies active=true and records no pending start; later removeIfCurrentDelivery() changes it to Idle but schedules nothing. If a replacement/start callback arrives just before the old death recipient, that only callback is lost and the replacement may stay alive without a Binder. Store a pendingStart/start epoch in Delivered, and have removal return whether one retry must be scheduled. Add commit -> begin while Delivered -> death removal -> new attempt coverage.

[JingMatrix]

Let us speak actual English, not AI style non-sense.

I do appericiate your efforts of spoting errors in my PR #893 and driving your AI agents to improve current PR, bothing showing your strong interest of contributing to Vector.

To me, only the part of uid delivery ownership is vakuable, which I summarize in commit 231822a now.

In current PR, I don't understand why we should link uid delivery with module cache changes. Please justifity it with concrete senario, otherwise, remove this part.

[LIghtJUNction]

Fair point, and sorry — my earlier replies were stiff and over-written.

The case I had in mind is simple: a module app is already running while the module is disabled. Vector sees the UID start, but getModuleByUid() returns null, so no Binder is sent. The user then enables the module. That rebuilds the cache, but it does not restart the app or produce another UID callback, so the running app will not receive IXposedService until it restarts. The cache hook was meant to retry that UID.

If Vector intentionally requires a restart after enabling a module, then the hook is unnecessary and I will remove it.

[zhuxi99]

This is a solid rewrite — centralizing the UID lifecycle in DeliveryStateStore with per-UID compute transitions closes the stale-worker / old-death-callback races the old three-map design could not. The state machine is well documented and the regression tests map 1:1 to the failure modes in the summary. A few things I found while reading:

1. linkDelivery can leak a DeathRecipient when an invalidation races the link (ModuleAppService.kt:152-166)

commitSuccess publishes Delivered(provider, recipient) before linkToDeath registers the recipient. If uidGone() or uidClear() lands in that window:

1. The invalidating thread sees Delivered, removes the state, and calls unlinkToDeath(recipient) — which fails (recipient not registered yet) and is swallowed by runCatching.
2. Our thread then calls provider.linkToDeath(recipient, 0) — succeeds, since the binder is still alive.
3. isCurrentDelivery is now false, and removeIfCurrentDelivery returns false (state is no longer Delivered), so the inner unlink is skipped.

Result: the recipient stays registered on the provider binder with no path that ever unregisters it, until the provider process dies. Each hot-reload (uidClear) or uidGone that wins this race leaks one. Note that in that branch removeIfCurrentDelivery can only return false once isCurrentDelivery is false — the nested check is effectively dead code, which is exactly why the leak is silent.

Suggested fix — unlink unconditionally once linkToDeath succeeded and we no longer own the delivery (unlinkToDeath only unregisters this recipient, so it cannot touch a replacement pair):

provider.linkToDeath(recipient, 0)
if (!deliveryState.isCurrentDelivery(uid, provider, recipient)) {
  runCatching { provider.unlinkToDeath(recipient, 0) }
}

(Linking first and committing after would reopen the mirror-image window where a provider dying between the two steps publishes a dead binder with no death callback left to clean it up, so the current order seems intentional — the minimal fix above is the safer direction.)

2. A generation change can run two concurrent sendBinder calls for the same UID — the module app sees the duplicate

invalidateMatching turns Sending(active) into Idle(active = true), and uidClear returns that UID for immediate uidStarts → begin() grants a new attempt while the old worker is still blocked inside sendBinder (getContentProviderExternal / provider.call can block for seconds). The stale result is correctly discarded by the isCurrentSending re-check, but the module app has already received a second SEND_BINDER provider call — the side effect happened even though the outcome is dropped. The old sending set serialized this; it is the deliberate price of not waiting for the stale lookup, but it might be worth a comment (or a module-side idempotency note), since a module handling onServiceBind twice concurrently could do surprising things.

3. Test coverage gaps

• finish()'s tombstone deletion path (!active && failureCount == 0 → null, DeliveryState.kt:138) is untested — e.g. begin → invalidateGone → finish should leave no entry.
• The store-level half of race #1 (invalidate removes Delivered, a subsequent commitSuccess is rejected) is covered by staleSuccessCannotPublishAfterUidGoneAndReplacement, but the ModuleAppService-side unlink behavior isn't. A test would need injectable binder fakes, so this may be acceptable — flagging it mainly so the leak above doesn't ship unnoticed.

Positive: moduleGenerationAppIds's referential comparison is consistent with the reuse path in ConfigCache.kt:256-275 (unchanged APK path → same LoadedModule instance → no invalidation), so the "only changed generations" claim holds in practice, not just in the unit test where instances are manually shared.

[pashippercode]

Fair enough — this is a rewrite rather than a patch-up.

On JingMatrix's question, the concrete scenario for tying delivery to cache changes: a module is disabled while its app process is still running. Disabling doesn't kill the process, so nothing fires afterward — no uidGone (process is alive), no death callback (the provider binder is alive), no uidStarts (no uid transition). The uid stays Delivered and the framework keeps serving a module that left the cache: requestScope can still prompt, remote files can still be written, and a later uidStarts for that uid gets swallowed by the Delivered no-op. The invalidation is what ends that service and, for an update (new LoadedModule, same app id), hands the running process the new generation immediately instead of waiting for its next restart.

Scope of the value: uninstall is already covered — force-stop kills the process, so uidGone and the death callback clean up — which means the hook only earns its keep in the "process outlives the cache change" window. If that's judged too narrow, it can be reduced to removals only: invalidate just the app ids that left the module map. That keeps the disabled-module case and drops the update re-delivery.

[zhuxi99]

Read through the rewrite — centralizing the UID lifecycle in DeliveryStateStore is a real step up from the old scattered collections, and I couldn't find a hole in the attemptId checks.

One actual issue though: linkDelivery (ModuleAppService.kt:152-166). commitSuccess publishes Delivered before linkToDeath registers the recipient, so if uidGone()/uidClear() lands in between:

• the invalidating side removes the state and calls unlinkToDeath — fails (not registered yet), swallowed
• we then linkToDeath successfully, since the binder is still alive
• isCurrentDelivery → false, so removeIfCurrentDelivery → false and the unlink never happens

The recipient is then registered on a live binder with no code path left to unregister it — it leaks until the provider process dies. The nested if (removeIfCurrentDelivery(...)) can't return true once isCurrentDelivery is false, so it's dead code and the leak stays silent.

Fix: unlink unconditionally once we no longer own the delivery:

provider.linkToDeath(recipient, 0)
if (!deliveryState.isCurrentDelivery(uid, provider, recipient)) {
  runCatching { provider.unlinkToDeath(recipient, 0) }
}

unlinkToDeath only unregisters the exact recipient passed in, so a replacement pair is untouched. (I briefly considered linking first and committing after, but a provider dying in between would commit a dead binder with no callback left to clean it up — current order is fine, just make the unlink unconditional.)

Smaller notes:

• After invalidation, a new attempt can start while the old worker is still blocked in sendBinder — the module app gets two concurrent SEND_BINDER calls for the same uid (stale result dropped, side effect already happened). Probably harmless, and the price of not waiting for a blocked lookup, but the old code serialized this — worth a comment somewhere.
• finish()'s tombstone deletion path (DeliveryState.kt:138) is untested — "begin → invalidateGone → finish leaves nothing behind" is easy to cover and would have saved me some head-scratching.

And for what it's worth: moduleGenerationAppIds's referential comparison lines up with the object-reuse path in ConfigCache.kt:256-275 (unchanged APK path → same instance → no invalidation), so the "only changed generations" claim holds in production too, not just in the unit test.

[JingMatrix]

@LIghtJUNction Please remove the test code, and simplify your changes to be minimal.