fix: revoke approvals when queued work is canceled

[sonalisrisivani]

Fixes #251.

Summary

• Make explicit queued cancellation atomically transition the transaction and revoke any unconsumed approval.
• Make the TTL sweep perform the same revocation and ApprovalRevoked event for every stale approved transaction.
• Reuse transaction-scoped helpers in both SQLite and Postgres so the state change, approval deletion, and audit event commit together.
• Preserve the no-event behavior for transactions that were never approved and the refusal to cancel Running work.

Validation

• Focused cancellation tests: 4 passed.
• Focused TTL cleanup tests: 2 passed, including three approved stale transactions.
• Postgres contract target: 6 passed; 5 live database tests skipped because no SYSKNIFE_TEST_POSTGRES_URL is configured.
• cargo check -p sysknife-daemon: passed.
• cargo fmt --all -- --check: passed.
• Full daemon library suite: 871 passed; 12 existing environment-sensitive failures on macOS (socket path length and unrelated autoremove/watermark tests).

[sonalisrisivani]

Implemented in commit d901ecf.

The fix makes explicit cancellation and TTL cleanup atomically revoke unconsumed approvals and append approval_revoked in both SQLite and Postgres. Unapproved cancellations still emit no event, and Running transactions remain uncancelable.

Validation completed:

• 4 cancellation tests passed
• 2 TTL cleanup tests passed, including three approved stale transactions
• cargo check -p sysknife-daemon passed
• cargo fmt --all -- --check passed
• Postgres contract tests passed where no live database was required; live tests remain skipped without SYSKNIFE_TEST_POSTGRES_URL
• Full daemon suite: 871 passed, with 12 unrelated macOS environment-sensitive failures

[vladimirrott]

Reviewed at d901ecfe2e41634796a096e52cd66f248822444a.

Extracting revoke_unconsumed_approval_in_tx and calling it from three places, in both stores, is the right shape. The alternative that most people reach for is a second copy of the digest-read, delete and append sequence inside cancel_queued, and this repository already carries the scars of two copies of one rule drifting apart. One helper per store, taking the open transaction, means the state change, the approval delete and the audit event commit together or not at all.

I approved the queued fork runs after reading the whole diff, and the board is now reporting.

What I verified

Everything ran in rootless podman with the repo mounted read-only and no network, because cargo test on a fork PR runs your code as this user.

Both new SQLite tests pass at your head:

$ ./pc.sh cargo test -p sysknife-daemon --lib --locked --offline -- transactions::tests::cancel_queued_revokes_an_unconsumed_approval_and_appends_event transactions::tests::cleanup_stale_queued_revokes_every_unconsumed_approval
running 2 tests
test transactions::tests::cancel_queued_revokes_an_unconsumed_approval_and_appends_event ... ok
test transactions::tests::cleanup_stale_queued_revokes_every_unconsumed_approval ... ok
test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 894 filtered out; finished in 0.06s

You changed two call sites, so I reverted them one at a time rather than together, because a pair of tests that both fail on a combined revert can still be covering one hunk twice. Dropping the revoke call from cancel_queued alone:

$ ./pc.sh cargo test -p sysknife-daemon --lib --locked --offline -- transactions::tests::cancel_queued_revokes_an_unconsumed_approval_and_appends_event transactions::tests::cleanup_stale_queued_revokes_every_unconsumed_approval
test transactions::tests::cancel_queued_revokes_an_unconsumed_approval_and_appends_event ... FAILED
test transactions::tests::cleanup_stale_queued_revokes_every_unconsumed_approval ... ok
panicked at crates/sysknife-daemon/src/transactions.rs:2507:9:
assertion `left == right` failed
  left: ["approval_granted"]
 right: ["approval_granted", "approval_revoked"]

And from cleanup_stale_queued alone:

$ ./pc.sh cargo test -p sysknife-daemon --lib --locked --offline -- transactions::tests::cancel_queued_revokes_an_unconsumed_approval_and_appends_event transactions::tests::cleanup_stale_queued_revokes_every_unconsumed_approval
test transactions::tests::cancel_queued_revokes_an_unconsumed_approval_and_appends_event ... ok
test transactions::tests::cleanup_stale_queued_revokes_every_unconsumed_approval ... FAILED
panicked at crates/sysknife-daemon/src/transactions.rs:2770:13:
assertion failed: !store.revoke_unconsumed_approval(&transaction.transaction_id).unwrap()

One test per hunk, each failing on its own. That is the coverage shape I want and it is rarer than it sounds.

You touched store/**, so the Postgres contract is required. Eleven tests, live database, all five #[ignore] ones included:

$ ./pcpg.sh env SYSKNIFE_TEST_POSTGRES_URL="$SK" SYSKNIFE_REQUIRE_POSTGRES=1 cargo test -p sysknife-daemon --test postgres_store --locked --offline -- --include-ignored
running 11 tests
test migrates_legacy_schema_and_enforces_store_contract ... ok
test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.61s

The Postgres half is held down too. Dropping the revoke call from PostgresStore::cancel_queued:

$ ./pcpg.sh env SYSKNIFE_TEST_POSTGRES_URL="$SK" SYSKNIFE_REQUIRE_POSTGRES=1 cargo test -p sysknife-daemon --test postgres_store --locked --offline -- --include-ignored
test migrates_legacy_schema_and_enforces_store_contract ... FAILED
panicked at crates/sysknife-daemon/tests/postgres_store.rs:404:5:
assertion `left == right` failed
  left: ["approval_granted", "approval_consumed", "approval_granted"]
 right: ["approval_granted", "approval_consumed", "approval_granted", "approval_revoked"]

Both entry points are live. cancel_queued is reached from dispatcher.rs:1698, behind authorize_for_transaction, and cleanup_stale_queued from the sweep task spawned at main.rs:136. Neither is a path nothing calls.

I also checked the Postgres loop for the interleaving the SQLite IMMEDIATE transaction rules out by construction. pool.begin() is READ COMMITTED, so two concurrent sweeps can select the same stale id, but the AND status = $3 re-check in the UPDATE makes the second one affect zero rows and skip the revoke. Same for a claim_approved_for_execution racing the sweep in either order. The status re-check is doing real work there, not decorating.

Nothing is blocking

Two notes worth having, neither of them a change I need.

Say what it fixes, precisely. claim_approved_for_execution already requires status = Queued, so an unconsumed receipt on a cancelled transaction was never redeemable. What was broken is the chain: the approval row survived with consumed_at IS NULL and no ApprovalRevoked event, so sysknife audit showed a receipt granted and never resolved, for as long as the database lived. Your fix closes the record, and it removes the row so that any future code that checks the approvals table without checking status cannot be fooled. That is a real defect in something this project sells, and it is worth stating as an audit-completeness fix rather than a privilege one, because the second claim would not survive a reader opening claim_approved_for_execution.

That matters for one line in your test:

assert!(
    !store
        .claim_approved_for_execution(&tx.transaction_id, &digest)
        .unwrap(),
    "canceling must revoke the unconsumed receipt"
);

It passes with your fix removed. You can see it in the first mutation above: the failure lands on the event assertion four lines down, at transactions.rs:2507, having walked straight past this one. The message claims more than the assertion checks. The test as a whole bites, so this costs nothing today, and if you keep the line I would reword it to what it does prove: that a cancelled transaction cannot be claimed at all.

A behaviour change you may not have intended. cancel_queued and cleanup_stale_queued now take audit_key and return AuditChainMissing without one. Read-only stores are built with audit_key: None at transactions.rs:339. I traced the callers and nothing reaches either method through a read-only store, so this is unreachable rather than broken, and failing closed is the right direction. Recording it so it is not a surprise later.

Where this stands

I am approving it. The one thing between it and main is the test count: your two tests move the suite to 1,839 and the recorded baseline is 1,837, which is what turned rust red:

rust	Test workspace	test_baseline: rust suite has 1839 tests, baseline says 1837.

That is not yours to fix and not your debt. Leaving tests/evidence/workspace-tests.json, README.md, docs/introduction.md and docs/distro-support.md untouched is exactly what I would rather have than a typed figure, and Vladimir regenerates them on your branch before merging. Everything else on the board is green, including postgres-contract.

Three PRs in one day, one of them a two-store transactional change with a mutation-proof test per hunk. Take the evening.

[vladimirrott]

#380 closes #251, the deeper daemon issue I offered you, and #375 already landed. Two clean fixes in the approval and packaging paths.

You still hold #234, and two more that fit the audit and approval work you have been doing, reserved for you:

• audit export publishes request_hash, an unsalted hash over unredacted params, with no statement of its sensitivity #268 — audit export publishes request_hash, an unsalted hash over unredacted input. A privacy leak in the chain you were just in.
• audit verify says OK over a wiped log, so an empty store and a deleted one read the same #338 — audit verify says OK over a wiped log, so an empty store and a deleted one read the same. A guard that should bite and does not.

The claimed label holds them until you reply on a thread (GitHub will not assign you before then). #380 is in review.

[vladimirrott]

Approving at e022401, which carries one commit of mine on top of yours; details at the end.

The part worth naming is that you made it atomic rather than sequential. Cancelling a transaction and revoking its receipt were two writes, and the tempting fix is to add a second call next to the first. Instead revoke_unconsumed_approval_in_tx takes the open transaction, so the status change, the transaction_approvals delete and the ApprovalRevoked chain entry commit together or not at all. A crash between two separate writes is exactly how a cancelled job keeps a live receipt, which is the bug.

Reading the digest before the DELETE and failing with DatabaseInvariant when rows vanish without one is the right shape too: the event has to name the receipt it retracted, and after the delete there is nothing left to name.

What I verified, on Linux and against a real database.

Both new SQLite tests bound to their own production hunk:

S0                                            2 passed
M1  remove the revoke call from cancel_queued
    -> cancel_queued_revokes_an_unconsumed_approval_and_appends_event   FAILED
M2  remove it from cleanup_stale_queued
    -> cleanup_stale_queued_revokes_every_unconsumed_approval           FAILED

The Postgres half against a throwaway postgres:17-alpine on a free port, with SYSKNIFE_REQUIRE_POSTGRES=1:

test result: ok. 11 passed; 0 failed; 0 ignored

0 ignored matters: the live tests actually ran rather than being skipped into a green. Then the same mutation on the Postgres path:

M3  remove the revoke call from PostgresStore::cancel_queued
    -> migrates_legacy_schema_and_enforces_store_contract               FAILED

I also checked the behaviour change your diff does not advertise. cancel_queued and cleanup_stale_queued now require an audit key and return AuditChainMissing without one, which would break any caller holding a read-only store. The read-only opens are all in apps/sysknife-cli/src/runner.rs for audit verify, export and history, and none of them call either function; the real callers are the daemon's writable store in main.rs and dispatcher.rs. Safe, and worth stating out loud since it is the kind of tightening that surfaces months later.

One thing I would have liked, not blocking: cleanup_stale_queued went from a single bulk UPDATE to a select-then-loop inside one immediate transaction, so it holds the write lock across N round trips. Stale queued rows should be few and the guard on status = queued in each update makes it correct under concurrency, so this is a note rather than a change request.

The commit I pushed. Your two tests moved the workspace count from 1,841 to 1,843, and that number is published in tests/evidence/workspace-tests.json plus three prose claims in README.md, docs/introduction.md and docs/distro-support.md. Moving the artifact alone turns rust green and docs-and-hygiene red. I measured the merged tree and moved all four in e022401. None of your code changed, and that coupling being invisible to contributors is my problem rather than yours.

That push dismissed my earlier approval, hence this one. Merging once CodeQL finishes; the five required checks are already green, postgres-contract included.

Nothing new from me while #379 is open on your side. I released the three issues I had reserved for you earlier today, for the reason on those threads: I had double-booked them, and you have enough in flight.