refactor!: replace run_lock with a PackageKit-style transaction system - #23
refactor!: replace run_lock with a PackageKit-style transaction system#23eatradish wants to merge 54 commits into
run_lock with a PackageKit-style transaction system#23Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eea971579e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af0a7e4779
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
To use Codex here, create an environment for this repo. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94a29ac347
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94a29ac347
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9d81adaa4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f21d0be19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b6dfa6cd6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d2337d3af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef6ba73f6c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0fa7f52799
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a767feff92
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5521a005a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad560c73bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ba4a21dad
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…shot GetTransactionList cloned the queue/running entries, released the scheduler locks, then read each state: a cloned queued transaction could be cancelled, or a cloned running transaction could finish, before the read completed. The API then returned terminal entries even though it promises only queued and running. Read each state while still holding the corresponding scheduler lock (lock order queue -> running -> state, consistent with cancel); a queued entry is then always Queued and the running slot is at most Running, so no terminal state can leak into the list.
oma_refresh::db::Event unit variants (ScanningTopic, RunInvokeScript,
Done) serialize to JSON scalars (e.g. "Done"). Wrapping that in the
internally tagged newtype Progress(serde_json::Value) made to_string
fail (internally tagged newtypes require a map payload to inject the
tag), so emit_progress errored and the refresh forwarder dropped those
events - clients never saw milestones like Done.
Change Progress to a struct variant { payload: serde_json::Value }: the
internal tag is emitted next to the field, so arbitrary JSON values
(scalars and maps) serialize and deserialize. Update the client
EventEnvelope to match. Wire format: {"type":"progress","payload":...}.
install/remove/upgrade treated every apply Result as success, printed "Client finished successfully" and exited 0 even when package resolution, commit, or the post-commit cache refresh failed and the server emitted TaskStatus::Failed. Inspect report.status in the apply Result branch and bail on Failed (same pattern as the updates_list/simulate checks), so a failed package operation is reported as an error with non-zero exit.
…ueing An unprivileged caller could submit Simulate/ApplyChanges requests whose install/remove strings are each as large as the system bus accepts (~128MB); the vectors are captured by the queued future with no item or byte limit. The queue bound only counts entries, so one UID could retain eight near-maximum messages plus the running request and exhaust the daemon memory. Validate the aggregate argument size (all install + remove string bytes, 16 MiB cap) at the top of apply_changes and simulate, before the task is constructed or enqueued; oversized requests are rejected with LimitsExceeded.
install/remove/upgrade exited via the loops trailing Ok(()) when next_event() returned None, i.e. the daemon or system-bus connection closed before a Result arrived. A package operation with no confirmed outcome was reported as success. Bail with an error when the stream closes without a successful result; only the TxEvent::Result branch returns Ok.
The byte cap only counted string contents: millions of empty strings each contribute 0 bytes yet every deserialized String occupies a 24-byte header plus Vec capacity, so a system-bus-sized message of empty strings could bypass the check and, with up to eight queued requests per UID, still exhaust daemon memory. Add MAX_TRANSACTION_ARG_ITEMS (65536) and reject when the combined install + remove element count exceeds it, alongside the existing 16 MiB byte cap. Worst-case retained memory per request is now about 19 MiB (contents + String headers + Vec).
When Refresh/ApplyChanges is awaiting PolicyKit, begin has marked the live entry as started and stored its cancellation id but has not yet enqueued it. A concurrent Cancel previously fell through to the manager and returned UnknownObject, and Destroy rejected the object as started, leaving the owner unable to dismiss the authorization prompt. Cancel now rolls back the claim (clearing started/claimed_at/ cancellation_id) and cancels the remote PolicyKit check, returning the object to dormant so it can be retried or destroyed. Destroy likewise removes claimed-but-not-enqueued objects and cancels their remote check. begin re-checks the claim under the live lock before enqueueing so a concurrently cancelled operation never executes. StartedClaim rollback is now generation-aware: it only clears the entry if the cancellation_id still matches the one this claim wrote, so a stale begin future (whose auth resolves after Cancel + re-trigger) cannot wipe out a fresh claim.
Rolling back a claim (auth failure, enqueue rejection, or Cancel) leaves the object dormant again but keeps its original created_at. Once the object has existed for DORMANT_TIMEOUT, the next reaper pass immediately classifies it as stale and removes it, so the retry promised by the rollback is available for at most one reaper interval. Track a separate dormant_since timestamp: set at creation and reset to now on every rollback (StartedClaim::rollback/Drop, cancel rollback_claim_if_not_enqueued). The reaper now measures dormant staleness against dormant_since, falling back to created_at when unset (claimed).
When a normal task fails to emit its Result signal but succeeds in emitting the subsequent Finished state, the client event loops and wait_result wait forever: result-emission errors are only logged in transaction_object.rs, and removing the path-scoped object does not close the connection-level signal stream. Because both events use the same ordered stream, observing Finished means the result is not still on its way. Add check_terminal_state: Cancelled and Finished are both terminal and now bail immediately (Finished -> "transaction finished without result"). Use it in wait_result and the install/remove/upgrade/refresh event loops, replacing the Cancelled-only check that ignored Finished.
When cancellation races with a successful authorization and the caller immediately retries the same object, the enqueue recheck accepted the replacement claim because it verified only that started is true, not that the claim still has this invocation's cancellation ID. The stale invocation could therefore enqueue the operation that Cancel reported as canceled; if the retry is later authorized, both operations are queued with the same transaction ID, and the first completion removes the object while the duplicate remains active. Pass the expected claim generation (cancellation_id) into check_claim_still_active and reject mismatches: the entry must still exist, be started, and carry this invocation's cancellation ID.
…ation ID Cancel on a claimed-but-not-enqueued Simulate/UpdatesList rolls back the claim but returned None (no polkit cancellation id), so cancel() fell through to manager.cancel, which cannot find the never-enqueued transaction and reported UnknownObject despite preventing the task. Return (rolled_back, Option<cancellation_id>) from rollback_claim_if_not_enqueued and treat rolled_back as success; only cancel the remote authorization when a cancellation id was carried out.
Previously the PolicyKit cancellation_id doubled as the claim-generation marker, so Simulate/UpdatesList (no auth_action) always stored None. If Cancel rolled back such a claim before enqueue and the caller immediately retried, the stale and replacement invocations were indistinguishable: the stale invocation could pass check_claim_still_active (None == None) and enqueue an operation cancellation reported as stopped, or its rollback could clear the replacement claim. Split the two: every begin invocation now allocates a unique claim generation (independent counter) stored in LiveTransaction/StartedClaim, while cancellation_id remains the PolicyKit-only cancel handle. All rollback paths and the enqueue recheck compare the generation instead.
claim_expired_requires_dead_sender_or_timeout unconditionally expected a session bus, panicking and failing the whole test suite in headless CI or containers without DBUS_SESSION_BUS_ADDRESS. Skip with a notice when the session bus or its DBusProxy cannot be created, mirroring the existing system-bus test; the predicate under test only needs the bus for the live-sender name_has_owner branch.
The Topic Update Metadata section referenced UpdatesList and GetTransaction returning JSON directly, but GetTransaction no longer exists and UpdatesList now lives on the transaction object with its payload delivered through the TransactionEvent result signal. Document the CreateTransaction flow plus Simulate/UpdatesList and the result envelope instead.
The four transaction entry points duplicated the same skeleton: progress forwarding (refresh/apply_changes), spawn_blocking + JoinError handling, post-refresh of the search index, status combination, and result emission. Extract run_progress_and_refresh (shared by refresh/apply_changes) and run_summary (shared by simulate/updates_list), so each method now only carries its own differences (argument validation, task closure, role and auth action). Behavior is unchanged; result-signal error messages now use the role name instead of a per-method literal.
Move the transaction system from flat files (transaction.rs + transaction_object.rs) into a src/transaction/ directory module: - mod.rs: scheduler (TransactionManager + runner) - live.rs: live transaction registry, claim lifecycle, reaper - object.rs: per-transaction D-Bus object - types.rs: shared types (role/state/event/error) - limits.rs: transaction argument size validation - tests/: unit tests extracted per implementation module Pure structural refactor, no behavior changes. All 36 unit tests pass.
The runner clears the running slot before on_done removes the live registry entry. In that window manager.contains(id) is false while the live entry is still marked started, so rollback_claim_if_not_ enqueued and remove_for_destroy misclassified an already-executed transaction as a pre-enqueue claim: Cancel could report success after ApplyChanges had committed, and Destroy could remove the object before its terminal state was emitted. Add an enqueued flag to LiveTransaction, set atomically with the enqueue inside the live lock and kept until the entry is removed. Use it (instead of started + contains) in rollback_claim_if_ not_enqueued, remove_for_destroy, claim_expired and claim_still_ abandoned. Also drop the now-unused manager parameter from remove_for_destroy. Adds regression coverage for the finished-but-not-cleaned-up window. All 37 unit tests pass.
The reaper captures `now` once per pass, then snapshots and rechecks the live registry. A transaction created, claimed, or rolled back after that capture can have a created_at/claimed_at/dormant_since later than `now`. std::time::Instant::duration_since panics when its argument is later than self (the current toolchain returns 0 instead, but that is undocumented behavior), which would permanently terminate the one-shot reaper task and let abandoned objects exhaust the live transaction quota. Use saturating_duration_since so a timestamp newer than now is treated as zero elapsed: a freshly created/claimed/rolled-back transaction is never expired on the current pass. Same fix in claim_expired. Adds regression coverage for timestamps newer than now. All 37 unit tests pass in debug and release.
b4d72ad to
dcac0e5
Compare
|
|
||
| /// 单流 `TransactionEvent` 信号载荷：一个事务的全部事件。 | ||
| /// | ||
| /// 注意：不能叫 `TransactionEvent`——zbus 的 proxy 宏会为 |
| async fn main() -> anyhow::Result<()> { | ||
| let client = TransactionClient::connect().await?; | ||
|
|
||
| // 模拟也是一个排队执行的事务：创建事务对象并订阅信号（此时休眠）， |
| println!("{}", updates_list); | ||
| } else { | ||
| bail!("Failed to refresh packages metadata: {result:#?}") | ||
| // 更新列表也是事务：从它的 ResultReport.result 里取 OmaOperation。 |
| check_arg_size(&[big], &[]), | ||
| Err(zbus::fdo::Error::LimitsExceeded(_)) | ||
| )); | ||
| // install + remove 合计超过了限制就拒绝也拒绝。 |
| assert!(check_arg_size(&["a".repeat(half)], &["b".repeat(half)]).is_err()); | ||
| // 恰好等于字节上限允许。 | ||
| assert!(check_arg_size(&["y".repeat(MAX_TRANSACTION_ARG_BYTES)], &[]).is_ok()); | ||
| // 数量超出限制（全部空字符串，字节数=0 但内存可观）拒绝。 |
| "expected AccessDenied, got {err:?}" | ||
| ); | ||
|
|
||
| // 快速成功的授权 → Ok。 |
| .expect("approved auth must succeed"); | ||
| } | ||
|
|
||
| /// Progress 事件必须能承载标量数据（oma_refresh::db::Event 的单元变体 |
2 participants
Summary
Replace the
run_lock"reject when busy" model with a PackageKit-styletransaction system, so concurrent requests queue up and run one at a time
in FIFO order instead of failing with "Another task is already running!".
Changes
src/transaction.rs: transaction scheduler (queue + running slot,state machine queued → running → finished/cancelled, cancel only applies
to queued transactions)
Refresh/ApplyChanges/Simulate/UpdatesListall become queuedtransactions; each returns a transaction id
GetTransactionrenamed toSimulateUpdatesListnow returns a transaction id; the result arrives via theResultReportsignalresultfieldGetTransactionList(queued + running only, like PackageKit) andCancelTransactionmethodsTransactionStatesignal for state changesResultReportnow carriestransaction_id,role, and optionalresultpayloadget_transaction→simulate)Breaking changes
GetTransaction→Simulate(returnsu64transaction id)UpdatesListreturnsu64instead of a JSON stringResultReportfieldrequest_id→transaction_idThe plasmoid client will need synchronized updates.