Skip to content

fix(stopAll): fail canceled worker requests + docs cleanup#57

Merged
liobrasil merged 5 commits into
navid/new-worker-archfrom
codex/stopall-fail-canceled-requests-and-docs
Feb 24, 2026
Merged

fix(stopAll): fail canceled worker requests + docs cleanup#57
liobrasil merged 5 commits into
navid/new-worker-archfrom
codex/stopall-fail-canceled-requests-and-docs

Conversation

@liobrasil
Copy link
Copy Markdown
Contributor

Summary

  • fix stopAll() so each canceled WorkerHandler request is marked FAILED before removing it from scheduledRequests
  • borrow Worker via WorkerHandler capability in stopAll() to keep access model consistent
  • align FlowYieldVaultsEVMWorkerOps.cdc scheduler/stopAll comments with implementation
  • fix cadence/scripts/get_request_details.cdc comments

Commits in this PR

  • a694d8e chore: checkpoint staged PR changes
  • db84303 docs: align scheduler stopAll comments with implementation
  • 03f34e2 docs: fix get_request_details script comments
  • ad7610a fix(cadence): fail canceled worker requests in stopAll

Base

  • navid/new-worker-arch

@claude
Copy link
Copy Markdown

claude Bot commented Feb 20, 2026

PR Review: fix(stopAll): fail canceled worker requests + docs cleanup

Summary: This PR correctly identifies and fixes a real user-funds issue — when stopAll() was called, in-flight WorkerHandler requests were silently cancelled at the scheduler level but their EVM counterparts remained stuck in PROCESSING state indefinitely, preventing refunds. The fix is directionally correct, but there is one functional bug introduced and a documentation inconsistency.


Bug: Missing precondition for WorkerHandler in stopAll()

cadence/contracts/FlowYieldVaultsEVMWorkerOps.cdc, line 273

// pre block only checks:
//   _getManagerFromStorage() != nil
//   _getFlowTokenVaultFromStorage() != nil
//   _getSchedulerHandlerFromStorage() != nil

// ...but then unconditionally:
let workerHandler = FlowYieldVaultsEVMWorkerOps._getWorkerHandlerFromStorage()!

The force-unwrap on line 273 is executed unconditionally — even when scheduledRequests is empty. If WorkerHandler is not in storage, this panics, and due to Cadence's transaction atomicity, the pauseScheduler() call on line 269 also reverts. The scheduler remains unpaused despite the caller expecting stopAll() to have succeeded.

Contrast this with SchedulerHandler.executeTransaction, which already has this guard:

pre {
    FlowYieldVaultsEVMWorkerOps._getWorkerHandlerFromStorage() != nil: "WorkerHandler resource not found"
    // ...
}

The same precondition should be added to stopAll():

FlowYieldVaultsEVMWorkerOps._getWorkerHandlerFromStorage() != nil: "WorkerHandler resource not found"

Documentation inconsistency: scheduler cancellation

cadence/contracts/FlowYieldVaultsEVMWorkerOps.cdc, line 260 and cadence/transactions/scheduler/stop_all_scheduled_transactions.cdc

The new docstring says:

"It does not cancel the next scheduler transaction ID tracked by SchedulerHandler."

But the implementation (lines 305–312) does cancel it:

// Step 3: Cancel scheduler execution
let schedulerHandler = FlowYieldVaultsEVMWorkerOps._getSchedulerHandlerFromStorage()!
if let schedulerTransactionId = schedulerHandler.nextSchedulerTransactionId {
    let refund <- manager.cancel(id: schedulerTransactionId)
    // ...
}

This docstring is incorrect as written. If the intent was to distinguish between cancelling the transaction (which does happen) versus clearing the nextSchedulerTransactionId field on the SchedulerHandler resource (which is not done), that nuance should be stated explicitly. As-is, the note is misleading.


No test coverage for the new behavior

There are no tests in cadence/tests/ covering the stopAll() fix:

  • No test verifying that cancelled requests are marked FAILED on EVM
  • No test exercising the StopAllMarkFailedSkipped event path (i.e., when markRequestAsFailed returns false)

Given that this is a critical admin function that handles user funds, test coverage would significantly increase confidence in correctness.


Minor: StopAllMarkFailedSkipped event omits failure reason

access(all) event StopAllMarkFailedSkipped(
    requestId: UInt256,
    workerTransactionId: UInt64,
)

markRequestAsFailed returns Bool but no diagnostic info. When this event fires, operators have no indication of why the EVM call failed (e.g., request was already in a terminal state, EVM call reverted). Adding a message: String field, or having Worker expose more diagnostic context, would make this easier to triage in production.


Positive observations

  • Correct access control: borrowWorker() is access(contract), consistent with how _checkForFailedWorkerRequests accesses it — external callers cannot directly borrow the Worker capability.
  • Graceful degradation: emitting StopAllMarkFailedSkipped rather than panicking on failure is the right approach; it lets the cancellation loop continue for other requests rather than reverting everything.
  • Consistent pattern: the new stopAll() logic mirrors _checkForFailedWorkerRequests (line 596), keeping the failure-marking pattern uniform across both admin-initiated and scheduler-detected panics.
  • Test refactoring: the _startProcessingBatch helper in tests is clean and the rename from test_StartProcessing_* to test_StartProcessingBatch_* is thorough.
  • Doc cleanup: the startProcessingstartProcessingBatch updates across all markdown files and scripts are consistent and complete.

@liobrasil liobrasil force-pushed the codex/stopall-fail-canceled-requests-and-docs branch from ad7610a to 56be49a Compare February 20, 2026 21:40
@nvdtf
Copy link
Copy Markdown
Member

nvdtf commented Feb 24, 2026

LGTM! does the tests pass in this PR?

@liobrasil
Copy link
Copy Markdown
Contributor Author

liobrasil commented Feb 24, 2026

LGTM! does the tests pass in this PR?

yes the tests are passing

@liobrasil liobrasil merged commit 495b4ab into navid/new-worker-arch Feb 24, 2026
1 check passed
@liobrasil liobrasil deleted the codex/stopall-fail-canceled-requests-and-docs branch February 24, 2026 09:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants