Repository navigation
UN-4318 [FEAT] Show stuck executions and let owners abort running ones - #2415
kirtimanmishrazipstack wants to merge 7 commits into
Conversation
- 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
left a comment
There was a problem hiding this comment.
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
- Spec & intent: See findings F1, F2
- Architectural fit & precedent: See findings F8
- Correctness & edge cases: See findings F1, F2, F3, F7, F8, F9, F10, F11, F23
- Security: Clean
- Data integrity & migrations: See findings F8
- Concurrency: See findings F3
- API & contract compatibility: See findings F1, F6, F12, F21, F22
- Reliability & resilience: See findings F9
- Performance & cost: See findings F11
- Observability: Clean
- Operational safety: Clean
- LLM/agent-specific: N/A — no model calls, prompts or tools in the diff
- 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
- Dependencies & build: N/A — no dependency, lockfile or build file in the diff
- Code quality: Clean
- Doc & comment accuracy: See findings F2, F12, F13, F20
- Cross-document rule consistency: See findings F2, F20
- On-prem deployment experience: N/A — no chart, on-prem values, schema or on-prem build path in the diff
- 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 (
readkeys 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_sincein siblingunstract-cloudandunstract-sdk, infrontend/on this branch and onorigin/main, and readunstract-python-clientfor 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-excludeis clean on check and format, matchingpre-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 inbackend/.venv. - Lens 17: the branch is 10 commits behind
origin/mainand 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
SoftTimeLimitExceededfire 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_FRAMEWORKand 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_errortests onlyerroror"ERROR", so POST returns 200 with a null or partial result, while GET on the same run returns 422. The consumerunstract-python-client/src/unstract/api_deployments/client.py:L92,L389-L391does 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-L767passes no error. - Suggested fix: use
ExecutionStatus.is_failure(execution_status)inhas_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_idadds the file to the active set.clear_history_for_workflowclears these markers; abort does not. - Suggested fix: call
cleanup_active_file_cachefor the batch's files on the terminal-skip path. - Confidence: High
- Verification: VERIFIED — read marker creation, removal and the filter. Not executed.
kirtimanmishrazipstack
left a comment
There was a problem hiding this comment.
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
- 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.
- Does a storage lifecycle rule expire execution directories? That decides how much the leftover directories in #2 matter.
- 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?
- Should a remaining file already in
skipped_already_completedcount 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 bybackend/workflow_manager/execution/abort.py:L127 -
Failure mode: The sync wait loop exits on STOPPED.
posttreats only an error or ERROR as failure, so it returns 200, whilegetreturns 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, matchingget, 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 bybackend/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:L255and cleared attasks.py:L1527.filter_pipeline.py:L655-L657compares only the execution id and never checks whether that run has ended. -
Suggested fix: Call
cleanup_active_file_cachefor the batch's files in the_TerminalExecutionSkipbranch. -
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.
- 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>
|
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. 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
|
kirtimanmishrazipstack
left a comment
There was a problem hiding this comment.
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-L170andbackend/workflow_manager/internal_views.py:L1365,L1393are unchanged. The author declined and documented it atREADME.md:L181-L184. That is not a reviewer waiver. No re-run warning exists yet: "abort" appears nowhere infrontend/src. - F1, #4452398509 [Medium] (was High) Aborted run reported as COMPLETED → PARTIALLY RESOLVED — fixed at
workers/callback/tasks.py:L335-L340and pinned atworkers/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 atworkers/callback/tasks.py:L414. Disclosed atREADME.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 atabort.py:L147-L154. Residual: a dead worker's execution directory is not cleaned and usage for its finished files is not committed. Disclosed atREADME.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.pyclears 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
exceptare fixed atworkers/file_processing/tasks.py:L755-L768. Residual: one sequential POST per file atL780-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 atbackend/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;SoftTimeLimitExceedednow passes through - F10 [Medium] One id's failure loses all outcomes → RESOLVED —
abort.py:L118-L124; see N1 for a falsefailed - 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
- Spec & intent: See findings N5
- Architectural fit & precedent: Clean
- Correctness & edge cases: See findings N1, N2, N4, N5, N6
- Security: Clean
- Data integrity & migrations: Clean
- Concurrency: See findings N2, F3
- API & contract compatibility: See findings N3, N7
- Reliability & resilience: Clean
- Performance & cost: See findings F11
- Observability: Clean
- Operational safety: Low finding only; not posted
- LLM/agent-specific: N/A — no model calls, prompts or tools in the diff
- Testing: See findings N1, N8
- Dependencies & build: N/A — no dependency, lockfile or build file in the diff
- Code quality: Low finding only; not posted
- Doc & comment accuracy: See findings N6, N9
- Cross-document rule consistency: See findings N7
- On-prem deployment experience: N/A — no chart, on-prem values, schema or on-prem build path in the diff
- 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_sinceinfrontend/and siblingunstract-cloudandunstract-sdk, and readunstract-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-excludeis clean on check and format, matchingpre-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 inbackend/.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 STOPPEDstatus_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-cloudcheckout.
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_executionruns only for COMPLETED, so a STOPPED GET returns raw cached results includinghighlight_data,extracted_text, metadata and metrics. It ignoresinclude_metadata=false,include_metrics=falseand an org withENABLE_HIGHLIGHT_API_DEPLOYMENToff. POST filters for every status (deployment_helper.py:L408-L429), soREADME.md:L146-L147's "the same as polling withGET" 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:L258gates filtering on COMPLETED.test_deployment_aborted.pycovers POST only. - Suggested fix: run
process_completed_executionfor 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_errorposts ERROR, the backend refuses with a 200skipped, 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_responseturned the refused body intosuccess: 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_acknowledgecallsdelete_api_results;workers/file_processing/tasks.py:L1883caches 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; generatedspecs/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 STOPPEDfinds nothing in either file.unstract-python-clientdoes 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.
…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>
|
|
Replaced by #2452, same branch and commits, to start with a clean review history. The review threads here stay as the record. |



What
GET execution/?stuck=truenarrows the execution list to stuck runs, and each row getsstuck_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_permittedorfailed.Why
How
execution.abortedaudit event.workflow-execution/<id>/status/endpoint, and stop only on STOPPED.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)
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.POSTreturns 422 instead of 200;GETapplies 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.Database Migrations
Env Config
STUCK_EXECUTION_THRESHOLD_SECONDS(default 9000), also added tobackend/sample.env.Relevant Docs
backend/workflow_manager/execution/README.mdRelated Issues or PRs
failedis a fourth.Dependencies Versions
Notes on Testing
workflow_manager,api_v2,platform_audit,inbox,pipeline_v2,notification_v2) 675 passed, 26 skipped; workers 1779 passed, 3 skipped.GETafter 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.Screenshots
Checklist
I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code