Skip to content

UN-4318 [FEAT] Show stuck executions and let owners abort running ones - #2415

Closed
kirtimanmishrazipstack wants to merge 7 commits into
mainfrom
UN-4318-stuck-executions-abort
Closed

kirtimanmishrazipstack wants to merge 7 commits into
mainfrom
UN-4318-stuck-executions-abort

Conversation

@kirtimanmishrazipstack

@kirtimanmishrazipstack kirtimanmishrazipstack commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

What

  • GET execution/?stuck=true narrows the execution list to stuck runs, and each row gets stuck_since.
  • GET execution/stuck-count/ returns the stuck runs per entity (API, ETL, TASK, WF) and the threshold.
  • POST execution/abort/ stops up to 100 runs and returns one outcome per id: aborted, already_finished, not_permitted or failed.

Why

  • The Frontend v1 executions page needs to show stuck runs and abort them (PB-T08).

How

  • A run is stuck when it is PENDING or EXECUTING and neither the run nor its files have changed for longer than the threshold.
  • Owners of the run's deployment, pipeline or workflow can abort it, and so can org admins and service accounts. Abort sets the run and its open file rows to STOPPED, writes the file counts, sets the pipeline's status, sends notifications and records an execution.aborted audit event.
  • Abort stops a run between files; the file being processed finishes. Workers check the run's status before each file through a new internal workflow-execution/<id>/status/ endpoint, and stop only on STOPPED.
  • Queued batches of a stopped run are skipped but still count down, and so does a batch that fails after the abort, so the callback runs. It keeps STOPPED, saves the final counts, commits usage for finished files and cleans up the execution directory.
  • Abort reports the run itself, so a dead worker's run is still reported. A pipeline's status changes only if this is its newest run, and paused pipelines are reported too. The callback leaves both to the abort.
  • A re-run ignores a stopped run's active-file markers.
  • The contract is in backend/workflow_manager/execution/README.md.

Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)

  • Runs nobody aborts behave as before. Without stuck, the list returns the same response (tested). Workers make one extra status call per file; if that API call fails, the file is processed as today. A run that hits ERROR mid-batch carries on as today.
  • These change, but only for STOPPED runs: a synchronous API deployment POST returns 422 instead of 200; GET applies its result filters; the callback keeps STOPPED, bills finished files and leaves the pipeline status and notifications to the abort; stopped file rows show their error as the status message.
  • The public API spec now names STOPPED as a 422 outcome; only those strings changed. Regenerating the Python client and CLI is an optional follow-up.
  • Known limits, in the README: a re-run right after an abort can process the file still finishing a second time. A dead worker's run gets no callback, so its execution directory and usage commit stay undone. An API caller can't get the result of the file still finishing, though it is billed.

Database Migrations

  • None.

Env Config

  • STUCK_EXECUTION_THRESHOLD_SECONDS (default 9000), also added to backend/sample.env.

Relevant Docs

  • backend/workflow_manager/execution/README.md

Related Issues or PRs

  • UN-4318, epic UN-4257
  • UN-4366 is the planned consumer. The spec lists three abort outcomes; failed is a fourth.

Dependencies Versions

  • None.

Notes on Testing

  • Deploy the backend before the workers. A new worker talking to an old backend gets a 404 from the status check and keeps processing, as today. Expose abort to users only once the workers are updated too: an older worker's callback reports an aborted run with a finished file as completed.
  • With Postgres up: backend (workflow_manager, api_v2, platform_audit, inbox, pipeline_v2, notification_v2) 675 passed, 26 skipped; workers 1779 passed, 3 skipped.
  • Running-app test with the real backend, PG workers, Postgres, Redis and MinIO, with only the per-file tool step stubbed: API and ETL runs aborted mid-run, GET after an abort, a re-run before any worker picks up the stopped batch, an old run aborted while a newer one exists, a paused pipeline, a batch crashing after the abort, and an abort landing during the callback. The worker-killed-mid-file case was run in the previous round.
  • Not tested live: a real tool or LLM run, usage billing (needs the cloud plugin) and actual webhook delivery (a notification row was confirmed queued).

Screenshots

Checklist

I have read and understood the Contribution Guidelines.

🤖 Generated with Claude Code

- GET execution/?stuck=true and execution/stuck-count/ for PENDING/EXECUTING
  runs idle past STUCK_EXECUTION_THRESHOLD_SECONDS (default 9000).
- POST execution/abort/ stops runs the caller owns (or any run, for org
  admins and service accounts), with a per-id outcome and an audit event.
- Workers check the run's status before each file and stop once it is
  STOPPED; the file in progress finishes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kirtimanmishrazipstack kirtimanmishrazipstack left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #2415 review — UN-4318 [FEAT] Show stuck executions and let owners abort running ones

Mode: INITIAL · Head: 4bca922 · Merge base: 8765a4d · Base: main (freshened this run) · 23 files, 1638 lines, count matches GitHub · Rubric: unstract 2.1.80

Signals: lenses 4, 6, 7, 8, 16, 17 raised (new endpoints and permission checks, select_for_update, new serializers, a worker-to-backend call, README and docstrings, a new threshold setting).

Verdict — REQUEST CHANGES

Summary — Critical: 0 · High: 4 · Medium: 15 · Low: 8 · Lenses run: 19/19 (4 N/A, 1 Not covered; see checklist)

Security deep-dive: done
boundary-sweep: 12 sinks traced, clean

Findings are numbered F1 to F27 and appear as inline comments on this review.

One cause sits behind F1, F2, F6, F7 and F8. This PR adds the first request-path writer of STOPPED (abort.py:L127-L129). The worker and API code around it was written assuming a terminal row means the reaper already finished the run. Patching each site is possible; the structural fix is to make STOPPED a real outcome that the callback, the skip path and the API view all handle.

Unanchored findings

  • [Low] The PR body still lists "sub-tasks UN-4548 to UN-4552" under Related Issues; the ticket's session rules say to remove sub-task numbers from the description.

Lens checklist

  1. Spec & intent: See findings F1, F2
  2. Architectural fit & precedent: See findings F8
  3. Correctness & edge cases: See findings F1, F2, F3, F7, F8, F9, F10, F11, F23
  4. Security: Clean
  5. Data integrity & migrations: See findings F8
  6. Concurrency: See findings F3
  7. API & contract compatibility: See findings F1, F6, F12, F21, F22
  8. Reliability & resilience: See findings F9
  9. Performance & cost: See findings F11
  10. Observability: Clean
  11. Operational safety: Clean
  12. LLM/agent-specific: N/A — no model calls, prompts or tools in the diff
  13. Testing: Not covered — Postgres and Redis unreachable, so 30 of 33 new backend tests did not run; gaps found by reading are F4, F5, F14 to F19, F24 to F27
  14. Dependencies & build: N/A — no dependency, lockfile or build file in the diff
  15. Code quality: Clean
  16. Doc & comment accuracy: See findings F2, F12, F13, F20
  17. Cross-document rule consistency: See findings F2, F20
  18. On-prem deployment experience: N/A — no chart, on-prem values, schema or on-prem build path in the diff
  19. Scaling & workload lifecycle: N/A — no concurrency, autoscaling, shutdown, broker or pool setting in the diff

Assessed by the orchestrator, not an agent: 4, 5, 6, 8, 11, 12, 14, 15's gate, 17, 18, 19 and lens 7's consumer search. Lens 1's PR-title check passes against CONTRIBUTION_GUIDE.md.

Notes behind those lines:

  • Lens 4: read the abort permission path, API-key tiers (read keys cannot POST) and tenant scoping. The security scan found nothing at confidence >= 8. That scan is precision-biased and excludes DoS, rate limits and docs.
  • Lens 7 consumer search: grepped the endpoint paths and stuck_since in sibling unstract-cloud and unstract-sdk, in frontend/ on this branch and on origin/main, and read unstract-python-client for F6. The new public endpoints have no consumer yet (UN-4366 is pending). The internal status endpoint's consumer is this PR's worker, and the route resolves.
  • Lens 15 gate: ruff 0.3.4 (pinned) with the hook's --force-exclude is clean on check and format, matching pre-commit.ci. mypy on the three worker files: 145 errors, none on lines this PR added. mypy on backend files was not run: it is not installed in backend/.venv.
  • Lens 17: the branch is 10 commits behind origin/main and merges without conflict. The merge-result sweep covered 13 rule-carrying identifiers; its hits are this branch's own definitions, plus the stale sites in F2 and F20. facts.sh's own pass covered the first 40 of 108 identifiers.
  • Boundary sweep: run and file messages, audit target name, summary and metadata, the abort, count, list and internal status responses, and three worker warnings. Only ids, statuses, constants, names already shown to that caller, and errors from this app's own backend reach them.

Open questions for the author

  • Should files that finished in a STOPPED run count toward usage? F1 and F2 both depend on the answer.
  • Does a storage lifecycle rule sweep the execution directories? If so, the document-retention part of F2 shrinks.
  • Does SoftTimeLimitExceeded fire on the PG queue path? If not, that part of F9 falls away.
  • What should a re-run do with a file the aborted run is still processing? F3 needs this decision.

Assumptions made

  • No abort flow was run end to end. Backend behaviour is from reading, and from probes with mocks where stated.
  • The enterprise auth plugin, cloud overrides of REST_FRAMEWORK and ingress exposure of /internal/ are outside this repo and were not checked.
  • The spec is the Confluence page linked from UN-4318 (PB-T08, version 5).

cost: agents=7 wall=19:24 lenses-examined=14 of 19 files=23 lines=1638

Unanchored findings

  • backend/api_v2/api_deployment_views.py:174 (outside the diff hunk): [Medium] [Lens 7] — F6 An API caller waiting on POST gets HTTP 200 when its run is aborted; polling the same run gets 422
  • Location: backend/api_v2/api_deployment_views.py:L174
  • Failure mode: the sync wait exits on STOPPED and builds a response with no error. has_error tests only error or "ERROR", so POST returns 200 with a null or partial result, while GET on the same run returns 422. The consumer unstract-python-client/src/unstract/api_deployments/client.py:L92, L389-L391 does not list STOPPED as in progress, so a caller reads this as finished successfully. Other external callers could not be identified.
  • Evidence: has_error = response.get("error") or execution_status == "ERROR"; workflow_helper.py:L762-L767 passes no error.
  • Suggested fix: use ExecutionStatus.is_failure(execution_status) in has_error.
  • Confidence: High on the status codes, Medium on how often an abort meets a live sync wait.
  • Verification: VERIFIED — read the view, the sync wait and the sibling client. Not executed.
  • workers/file_processing/tasks.py:332 (outside the diff hunk): [Medium] [Lens 3] — F7 A re-run after abort silently leaves out queued-batch files until the active-file marker expires
  • Location: workers/file_processing/tasks.py:L332-L339, L1527, workers/shared/processing/filter_pipeline.py:L655-L657
  • Failure mode: markers are created at discovery and removed only in pre-create. A queued batch skipped after abort never reaches pre-create, so its markers survive. The re-run's filter sees a different execution id and drops those files, with only a log line. The TTL is 300s by default and can be set up to 9000s.
  • Evidence: cached_exec_id != execution_id adds the file to the active set. clear_history_for_workflow clears these markers; abort does not.
  • Suggested fix: call cleanup_active_file_cache for the batch's files on the terminal-skip path.
  • Confidence: High
  • Verification: VERIFIED — read marker creation, removal and the filter. Not executed.

Comment thread workers/file_processing/tasks.py Resolved
Comment thread workers/file_processing/tasks.py Resolved

@kirtimanmishrazipstack kirtimanmishrazipstack left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UN-4318 [FEAT] Show stuck executions and let owners abort running ones. Head 4bca92285, merge base 8765a4dcb, 23 files, 1638 lines (count matches GitHub). Base fetched this run. The branch is 10 commits behind main and merges cleanly.

Verdict — REQUEST CHANGES

Summary — Critical: 0 · High: 5 · Medium: 9 · Low: 6 · Lenses run: 19/19

Security deep-dive: done. No findings at confidence >= 8; this is a precision-biased scan, not a clean bill of health.

boundary-sweep: 11 sinks traced, clean.

Tests run with real Postgres and Redis: backend 33 passed plus 21 subtests, workers 18 passed. MinIO and RabbitMQ were not started. The wider totals in the PR body (216 and 1467) were not re-run.

The numbered findings are in the inline comments.

Unanchored findings

  • [Low] The PR body still lists "sub-tasks UN-4548 to UN-4552"; remove them.

Lens checklist

  • Lens 1: See findings #1, #2
  • Lens 2: Clean
  • Lens 3: See findings #1, #2, #7, #8, #9, #18, #19
  • Lens 4: Clean
  • Lens 5: See findings #7
  • Lens 6: See findings #2, #4
  • Lens 7: See findings #6, #10, #16, #17
  • Lens 8: Clean
  • Lens 9: Clean
  • Lens 10: Clean
  • Lens 11: Clean
  • Lens 12: N/A — no model calls, prompts or agents touched
  • Lens 13: See findings #3, #4, #5, #13, #14, #18, #20
  • Lens 14: N/A — no dependency manifest or build file changed; new imports are existing dependencies
  • Lens 15: Clean
  • Lens 16: See findings #2, #10, #11, #12, #15
  • Lens 17: See findings #11
  • Lens 18: N/A — no chart, on-prem values, on-prem settings or on-prem build path touched
  • Lens 19: N/A — no worker concurrency, autoscaling, grace or pool setting touched

Assessed by the orchestrator: lenses 4, 5, 6, 8, 11, 12, 14, 17, 18, 19, and lens 7's consumer search.

Lens 7 search: frontend/src and the cloud repo have no caller of the new endpoints yet; the planned consumer is the Logs page ticket, UN-4366. The internal status route's consumer is the worker in this repo.

Lens 15 gate, pinned versions, check-only, on the changed files: ruff 0.3.4 check and format with the hook's --force-exclude, pycln 2.6.0 and markdownlint-cli 0.42.0 all pass. pyupgrade was not run because it has no check mode.

Lens 17: the merge-result sweep exited 1. Its hits outside this diff are the audit README and actions.py lines; only platform_audit/README.md:L200 disagrees.

Open questions for the author

  1. What should the callback's usage step do for finished files of an aborted run? It acts only on COMPLETED, so returning STOPPED in fix #1 changes that.
  2. Does a storage lifecycle rule expire execution directories? That decides how much the leftover directories in #2 matter.
  3. The spec names owners and admins; this PR also lets write-tier platform keys abort. A ticket comment on the audit recorder mentions platform keys as an actor. Is this intended?
  4. Should a remaining file already in skipped_already_completed count as failed when a batch stops? The main loop does not count it (tasks.py:L809), and the stop path does (L727).

Assumptions made

  • #1 is held at High, not Critical, because the run row stays correct and abort is a deliberate user action. If a webhook drives an irreversible downstream load, it should go up.
  • #2 is rated High rather than Medium because it covers the feature's main target and leaves customer documents in storage.
  • Findings #3, #4 and #5 are test gaps; today's code behaved correctly in each probe.

Unanchored findings

  • backend/api_v2/api_deployment_views.py:173 (outside the diff hunk): [Medium] [Lens 7] — A sync API caller gets HTTP 200 for an aborted run

  • Location: backend/api_v2/api_deployment_views.py:L173-L174, made reachable by backend/workflow_manager/execution/abort.py:L127

  • Failure mode: The sync wait loop exits on STOPPED. post treats only an error or ERROR as failure, so it returns 200, while get returns 422 for the same run. An integration keying on 2xx treats the aborted document as processed. It needs an abort inside the request's timeout, and the body does say STOPPED.

  • Evidence: has_error = response.get("error") or execution_status == "ERROR".

  • Suggested fix: Treat STOPPED as an error in post, matching get, and note it in the README.

  • Confidence: High. Searched this repo and the cloud repo; external API callers have no source here, so what they do with a 200 is not covered.

  • Verification: VERIFIED — read both handlers and workflow_helper.py:L741-L766.

  • workers/file_processing/tasks.py:336 (outside the diff hunk): [Medium] [Lens 3] — Re-running straight after an abort can silently drop files

  • Location: workers/file_processing/tasks.py:L336-L339, made reachable by backend/workflow_manager/execution/abort.py:L127

  • Failure mode: Active-file markers are cleared only during pre-create, which a skipped batch never reaches. If a batch was still queued at abort, a re-run within the marker's lifetime treats those files as active elsewhere and leaves them out. The lifetime defaults to 300s and can be raised to 9000s.

  • Evidence: Markers are written at workers/shared/processing/file_discovery.py:L255 and cleared at tasks.py:L1527. filter_pipeline.py:L655-L657 compares only the execution id and never checks whether that run has ended.

  • Suggested fix: Call cleanup_active_file_cache for the batch's files in the _TerminalExecutionSkip branch.

  • Confidence: Medium. Aborting a run with a queued batch and then re-running would confirm it.

  • Verification: VERIFIED — read the write, cleanup and filter paths. Not run end to end.

Comment thread workers/file_processing/tasks.py Resolved
Comment thread workers/file_processing/tasks.py Resolved
Comment thread workers/file_processing/tasks.py Resolved
- Callback keeps STOPPED, so an aborted run is no longer reported as completed.
- Abort sets the pipeline status, sends notifications and writes file counts,
  so a run with a dead worker is still reported.
- Skipped batches of a stopped run count down and clear their active-file
  markers, so the callback cleans up and a re-run keeps every file.
- A synchronous API call for an aborted run returns 422.
- One id failing answers `failed` instead of a 500 for the whole request.
- Add tests for org scoping, the row lock, per-id commits and the status route.
- Correct the README and API schema, and document the known limits.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kirtimanmishrazipstack

Copy link
Copy Markdown
Contributor Author

Replies for the review items that have no thread of their own. All fixed in 493fe01.

Sync API caller gets 200 for an aborted run (both reviews). Real, fixed. post now treats STOPPED as an error, so the caller gets 422, matching get. Pinned in api_v2/tests/test_deployment_aborted.py, and checked on a running app.

Re-run silently drops queued-batch files (both reviews). Real, fixed. A batch skipped for a STOPPED run clears its active-file markers. Checked on a running app with a two-batch ETL run: markers went from 2 to 0, and an immediate re-run processed all 4 files.

Sub-task numbers in the PR body (both reviews). Removed.

Open questions

  • Usage for finished files of an aborted run: billed, as for a partly failed COMPLETED run.
  • Storage lifecycle for execution directories: the unstract-cloud-platform Terraform has no rule for them (buckets outside that repo weren't checked), so a dead worker's directory stays until something else deletes it. In README Limits.
  • SoftTimeLimitExceeded on the PG path: no longer matters, since the status check now lets it through.
  • A re-run while the aborted run's file is still finishing: documented in README Limits, no code change.
  • Write-level platform keys: keep abort.
  • Files already completed when a batch stops: not counted as failed, as in the main loop.

@kirtimanmishrazipstack kirtimanmishrazipstack left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR follow-up review — UN-4318 [FEAT] Show stuck executions and let owners abort running ones

Mode: FOLLOWUP · Head: 6dc7e1e · Merge base: ed53cfd · Base: main (freshened this run; the branch is current with it) · 32 files, 2554 lines, count matches GitHub · Rubric: unstract 2.1.80

Signals: lenses 3, 4, 6, 7, 8, 13, 16, 17 raised (the abort path now writes pipeline status and sends notifications after commit; the callback, barrier and API deployment view changed; a status code changed for STOPPED runs; the README was rewritten).

Verdict — REQUEST CHANGES

Prior context fetched — followup-fetch.sh exited 0. Since-boundary: 4bca922, the head of both earlier reviews. Findings carry the F-numbers of the first review and the thread ids of the second.

Scope change — YES — new files outside the original review surface (workers/callback/tasks.py, workers/queue_backend/pg_barrier.py, workers/shared/infrastructure/logging/workflow_logger.py, backend/api_v2/api_deployment_views.py, backend/workflow_manager/file_execution/serializers.py) — re-scanned those files and everything commits 493fe01 and 6dc7e1e changed.

Summary — Critical: 0 · High: 2 · Medium: 12 · Lenses run: 19/19 (4 N/A; see checklist). Low findings (9 new) are left out of this post at the author's request, and Low threads from the earlier reviews have no reply.

Prior findings: 45 raised across the two reviews: 38 resolved, 6 partly resolved (4 distinct defects, since both reviews raised two of them), 1 not resolved. The counts above are what is open now: 5 prior defects and 18 raised by this re-scan.

Security deep-dive: done
boundary-sweep: 9 sinks traced; one finding (N3)
Tests: run with real Postgres and a throwaway Redis, removed afterwards. Backend PR tests 53 passed. Worker PR tests 47 passed; the wider worker suite, excluding integration tests, 1576 passed, 2 skipped. CI's test job is still skipped while the PR is a draft.

Prior findings status

Author replies were treated as claims. Each row was checked against the code at this head.

Still open. All five threads are marked resolved on GitHub, but the code does not fully back that, so they are flagged here and in their replies; they were not reopened:

  • F3 [High] Re-running right after an abort can process the in-flight file twice → NOT RESOLVED — backend/workflow_manager/execution/abort.py:L162-L170 and backend/workflow_manager/internal_views.py:L1365, L1393 are unchanged. The author declined and documented it at README.md:L181-L184. That is not a reviewer waiver. No re-run warning exists yet: "abort" appears nowhere in frontend/src.
  • F1, #4452398509 [Medium] (was High) Aborted run reported as COMPLETED → PARTIALLY RESOLVED — fixed at workers/callback/tasks.py:L335-L340 and pinned at workers/tests/test_abort_finishes_the_run.py:L112-L116. Residual: an abort landing after the callback's fetch still sends COMPLETED and pipeline SUCCESS, because the refused 200 is discarded at workers/callback/tasks.py:L414. Disclosed at README.md:L192-L193.
  • F2, #4452398513 [Medium] (was High) Callback never fires → PARTIALLY RESOLVED — the queued-batch case is fixed at workers/file_processing/tasks.py:L316-L317, and abort now writes status and counts at abort.py:L147-L154. Residual: a dead worker's execution directory is not cleaned and usage for its finished files is not committed. Disclosed at README.md:L185-L189, author-accepted, no reviewer waiver. The reaper's ERROR path already has the same gap. N4 is a further way to reach this.
  • F7 [Medium] Re-run silently leaves out queued-batch files → PARTIALLY RESOLVED — markers clear at workers/file_processing/tasks.py:L363-L364, but only when a worker picks up the skipped batch. A re-run before that still drops those files until the marker expires (300s by default, up to 9000s). abort.py clears nothing and the README does not mention it. The reply says fixed.
  • F11 [Medium] One POST per remaining file → PARTIALLY RESOLVED — unseen failures and the dead except are fixed at workers/file_processing/tasks.py:L755-L768. Residual: one sequential POST per file at L780-L787. The reply's reason holds only for the existing client method, which posts to a no-op route; a batch endpoint that writes exists at backend/workflow_manager/file_execution/internal_views.py:L409.

Resolved, High and Medium only (evidence at this head):

  • F4, #4452398527 [High] Cross-org abort test → RESOLVED — test_abort_executions.py:L289-L312
  • #4452398531 [High] Row lock and per-id transaction untestable → RESOLVED — test_abort_executions.py:L177-L195, test_abort_transactions.py:L63-L128
  • F18, #4452398537 [Medium] Status route not pinned → RESOLVED — test_execution_contract.py:L134-L137
  • F5 [Medium] Backend tests could not run → RESOLVED — all 53 ran and passed in this review with real services
  • F6 [Medium] Sync caller gets 200 → RESOLVED — backend/api_v2/api_deployment_views.py:L175-L178; see N3 for the GET body
  • F8, #4452398543 [Medium] No file counts → RESOLVED — abort.py:L176-L185
  • #4452398552 [Medium] Stopped file rows look running → RESOLVED — abort.py:L165-L169, backend/workflow_manager/file_execution/serializers.py:L37-L38
  • F9 [Medium] Broad catch → RESOLVED — workers/file_processing/tasks.py:L736; SoftTimeLimitExceeded now passes through
  • F10 [Medium] One id's failure loses all outcomes → RESOLVED — abort.py:L118-L124; see N1 for a false failed
  • F12, #4452398560 [Medium] 401/403 contract → RESOLVED — README.md:L153-L160, views/stuck_abort.py:L21-L26
  • F13 [Medium] README contradictions → RESOLVED — README.md:L113-L132, L179-L180, L190-L191
  • #4452398572 [Medium] Reaper and hung worker → RESOLVED — README.md:L179-L180
  • F14 [Medium] Part-way failure untested → RESOLVED — test_abort_transactions.py:L70-L95
  • F15 [Medium] Slot and inbox not pinned → RESOLVED — test_abort_executions.py:L118-L132
  • F16 [Medium] ERROR rows not pinned → RESOLVED — test_abort_executions.py:L72-L79
  • F17, #4452398583 [Medium] Clock-dependent test → RESOLVED — test_execution_contract.py:L64
  • F19 [Medium] Dict-only stop path → RESOLVED — workers/tests/test_abort_between_files.py:L104-L117, L156-L167
  • F20, #4452398567 [Medium] Who may abort → RESOLVED — views/stuck_abort.py:L60, backend/platform_audit/README.md:L200
  • #4452398577 [Medium] Five regressions unseen → RESOLVED — test_abort_executions.py:L129-L132, test_stuck_executions.py:L69-L78, test_execution_contract.py:L88-L98

New findings N1 to N9 follow as inline comments on this review; those whose lines are outside the diff are listed below.

Lens checklist

  1. Spec & intent: See findings N5
  2. Architectural fit & precedent: Clean
  3. Correctness & edge cases: See findings N1, N2, N4, N5, N6
  4. Security: Clean
  5. Data integrity & migrations: Clean
  6. Concurrency: See findings N2, F3
  7. API & contract compatibility: See findings N3, N7
  8. Reliability & resilience: Clean
  9. Performance & cost: See findings F11
  10. Observability: Clean
  11. Operational safety: Low finding only; not posted
  12. LLM/agent-specific: N/A — no model calls, prompts or tools in the diff
  13. Testing: See findings N1, N8
  14. Dependencies & build: N/A — no dependency, lockfile or build file in the diff
  15. Code quality: Low finding only; not posted
  16. Doc & comment accuracy: See findings N6, N9
  17. Cross-document rule consistency: See findings N7
  18. On-prem deployment experience: N/A — no chart, on-prem values, schema or on-prem build path in the diff
  19. Scaling & workload lifecycle: N/A — no concurrency, autoscaling, shutdown, broker or pool setting in the diff

Assessed by the orchestrator, not an agent: 4, 5, 6, 8, 11, 12, 14, 15's gate, 17, 18, 19 and lens 7's consumer search. Lens 1's PR-title check passes against CONTRIBUTION_GUIDE.md.

Notes behind those lines:

  • Lens 4: read the abort permission path, the code that now runs after commit, and what notifications carry. The security scan found nothing at confidence >= 8. It is precision-biased, excludes DoS and rate limits, and did not read cloud overlay code.
  • Lens 7 consumer search: grepped the endpoint paths and stuck_since in frontend/ and sibling unstract-cloud and unstract-sdk, and read unstract-python-client's handling of 422 and STOPPED. The new public endpoints have no consumer yet (UN-4366 is pending). External API deployment callers have no source here and could not be identified.
  • Lens 15 gate: ruff 0.3.4 with the hook's --force-exclude is clean on check and format, matching pre-commit.ci. mypy on the six changed worker files: 191 errors, none on lines this PR added. Backend mypy was not run because it is not installed in backend/.venv.
  • Lens 17: the branch is current with origin/main. The merge-result sweep covered 11 rule-carrying identifiers; its hits are this branch's own definitions, unrelated uses of the words, and the stale sites in N9.
  • Boundary sweep: the abort exception log, callback and status-check logs, _mark_file_stopped's error text, the notification payloads, the STOPPED status_msg, the final UI log, and the POST and GET bodies for a STOPPED run. Only ids, statuses, counts, names, constants and this app's own error text reach them, except the GET body in N3.
  • Usage double-commit on a redelivered STOPPED callback was checked and not filed: the cloud plugin deletes its cached usage on commit. That was read from the local unstract-cloud checkout.

Open questions for the author

  • F3: will you accept the re-run hazard in writing? A README limit is the author's note, not a waiver, so it stays open until then.
  • F2: do you accept in writing that a dead worker's directory and usage stay undone?
  • N2: should abort touch the pipeline's status when a newer run exists?
  • N5: is billing the in-flight file acceptable when an API caller can't receive it?
  • N6: should a paused pipeline's subscribers hear about an abort?

Assumptions made

  • The two earlier reviews share one GitHub login, so both are treated as this reviewer's findings.
  • The author's "checked on a running app" claims were not reproduced here. No abort was run end to end in this review.
  • Cloud overlay code was not read, apart from the subscription usage plugin.

cost: agents=8 wall=39:37 lenses-examined=15 of 19 files=32 lines=2554

Unanchored findings

  • backend/api_v2/api_deployment_views.py:257 (outside the diff hunk): [Medium] [Lens 7] — N3 GET on an aborted API run returns finished results unfiltered, while POST filters them
  • Location: backend/api_v2/api_deployment_views.py:L257-L273
  • Failure mode: process_completed_execution runs only for COMPLETED, so a STOPPED GET returns raw cached results including highlight_data, extracted_text, metadata and metrics. It ignores include_metadata=false, include_metrics=false and an org with ENABLE_HIGHLIGHT_API_DEPLOYMENT off. POST filters for every status (deployment_helper.py:L408-L429), so README.md:L146-L147's "the same as polling with GET" holds for the status code only. The caller owns the document, so this is a contract break rather than a tenant leak.
  • Evidence: api_deployment_views.py:L258 gates filtering on COMPLETED. test_deployment_aborted.py covers POST only.
  • Suggested fix: run process_completed_execution for STOPPED too, keep the 422, and add a GET test.
  • Confidence: High
  • Verification: VERIFIED — read both handlers. Not executed.
  • workers/queue_backend/pg_barrier.py:1399 (outside the diff hunk): [Medium] [Lens 3] — N4 A failed batch in an aborted run deletes the barrier, so the callback never fires
  • Location: workers/queue_backend/pg_barrier.py:L1399-L1410
  • Failure mode: any exception from a batch after an abort reaches this block. mark_execution_error posts ERROR, the backend refuses with a 200 skipped, the worker reads that as confirmed and deletes the barrier row. Later batches decrement a missing row and log a false ERROR. No callback runs, so no cleanup, usage commit or final log. README Limits covers only a dead worker.
  • Evidence: backend/workflow_manager/internal_views.py:L623-L630; convert_dict_response turned the refused body into success: True.
  • Suggested fix: read the status before tearing down; if STOPPED, decrement with a failed-batch result. The agent was unsure of the exact shape, because the decrement may be what raised.
  • Confidence: High
  • Verification: VERIFIED in part — ran convert_dict_response; the rest is traced by reading.
  • backend/workflow_manager/workflow_v2/workflow_helper.py:446 (outside the diff hunk): [Medium] [Lens 3, 1] — N5 The file still processing at abort is billed, but an API caller can never get its result
  • Location: backend/workflow_manager/workflow_v2/workflow_helper.py:L421-L425, L446-L463
  • Failure mode: STOPPED counts as complete for result delivery, so the first read after an abort returns the results so far, acknowledges them and deletes the cached list. The in-flight file then pushes its result to a key nothing reads, and later GETs answer 406. Its row ends COMPLETED, so the callback bills it. README Limits does not mention this.
  • Evidence: set_result_acknowledge calls delete_api_results; workers/file_processing/tasks.py:L1883 caches each result as it finishes.
  • Suggested fix: add it to README Limits. Holding acknowledgement until the callback runs is a larger change and a product call.
  • Confidence: High on behaviour.
  • Verification: VERIFIED — read the read, acknowledge and write paths. Not run.
  • backend/api_v2/openapi_schema.py:248 (outside the diff hunk): [Medium] [Lens 17, 7] — N7 The public API spec does not name STOPPED as a 422 outcome
  • Location: backend/api_v2/openapi_schema.py:L248-L249, L265-L267, L290-L294; generated specs/docstudio-oss.json
  • Made stale by: backend/api_v2/api_deployment_views.py:L175-L178
  • Failure mode: the spec tells pollers to "treat 422 as the normal reply and stop on 200", and says POST 422 means "finished with an error". STOPPED is never named. A client that only knows ERROR keeps polling after the first 422, but that read was the only one carrying the finished files; the next answers 406.
  • Evidence: git grep STOPPED finds nothing in either file. unstract-python-client does not treat STOPPED as in progress, so that client stops correctly; other external callers could not be identified.
  • Suggested fix: name STOPPED in all three strings and regenerate with generate_docstudio_spec.
  • Confidence: High
  • Verification: VERIFIED — read the strings and the client.

Comment thread workers/callback/tasks.py Resolved
Comment thread workers/queue_backend/pg_barrier.py Resolved
…s after abort

- Report the abort with a named hook, so a failed notification can't turn a
  committed abort into `failed` or lose its audit event.
- Only a pipeline's newest run writes its status; paused pipelines are reported.
  The callback leaves a stopped run's pipeline status and notifications to abort.
- An abort landing during the callback is reported as STOPPED, not COMPLETED.
- A batch that fails after the abort counts down, so the callback still runs.
- A re-run ignores a stopped run's active-file markers, so it keeps those files
  before any worker picks up the queued batch.
- GET filters a stopped run's results like POST, and the API spec names STOPPED.
- Correct the barrier comments and pin the callback's final log argument.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Nov 5, 2026

Copy link
Copy Markdown

@kirtimanmishrazipstack

Copy link
Copy Markdown
Contributor Author

Replaced by #2452, same branch and commits, to start with a clean review history. The review threads here stay as the record.

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.

1 participant