morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #4356] [CLOSED] fix(cli): preserve resource-owned cleanup status #6196

New issue

Closed

opened 2026-10-31 01:12:37 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:12:37 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4356
Author: @geekwolf
Created: 10/16/2026
Status: ❌ Closed

Base: main ← Head: fix/cli-cleanup-retry-evidence

📝 Commits (1)

c84e795 fix(cli): preserve resource-owned cleanup status

📊 Changes

2 files changed (+49 additions, -2 deletions)

View changed files

📝 src/cli/finishAnalysisCommand.ts (+2 -2)
📝 tests/boundary/cli/captureCleanup.test.ts (+47 -0)

📄 Description

Upstream coverage update — 2026-10-16

Main now independently covers this defect through #4274, commit 1b97b0f370. The generic CLI finalizer no longer resolves producer cleanup status, and main includes a resource-owner regression. Relevant main workflows were rechecked on 8ab5024397: 53 tests passed, with two existing Windows-gated POSIX signal cases skipped. This draft remains open, not merged; its superseded patch is not being replayed. Historical proposal and validation follow.

Summary

Keep cleanup status owned by the resource lifecycle rather than clearing it in the generic one-shot CLI finalizer. This corrects the regression introduced by 93c620c3. No separate issue was opened; the concrete regressions are the packet-capture and signal-recording cleanup tests on current main.

Problem and change

finishAnalysisCommand treated a successful service.close() as proof that every resource referenced by an earlier failure was released. It changed cleanup_incomplete into execution_failure and removed the unresolved-resource list even when the resource owner had not resolved its error.

Remove that blanket mutation. Actual resource owners retain their existing resolveCleanup() behavior. The CLI continues awaiting close, retaining completed observations, and merging a later cleanup failure with the primary error. A public command-boundary control verifies that an owner's explicit resolution is still honored.

Scope decision: not applicable to this correction within existing lifecycle contracts. No new provider, execution authority, configuration, schema, package metadata or performance claim.

Validation

Pinned baseline: main 42fc6c53ae188978deb3bedfe88287b6b316ba22. Both existing successful-retry CLI cases failed with execution_failure instead of cleanup_incomplete and lost their resource list.
After the fix, the four focused CLI files passed all 19 tests, including late cancellation, primary/secondary errors, failed cleanup retries and explicit owner resolution.
npm run check:fast, full npm run check, scoped formatting and git diff --check passed on Windows x64, Node 24.18.0 / npm 11.16.0.
Broader controlled-producer verification: 82 tests passed, two failed before producer execution because this Windows account cannot create file symlinks (EPERM). Fourteen POSIX descriptor cases were skipped by their existing platform gates. These unavailable cases are not claimed passing. No real TShark/sigrok or POSIX signal verification was performed.
Related independent baseline fixes are #4357 and #4358. Combining these three patches locally passed 65 focused tests (two Windows-gated POSIX CLI cases skipped). This branch's hosted source-cancellation and discovery-probe failures match those two separate fixes; the branch is not CI-green. Current-head hosted CI remains required.

Author readiness

Human owner: @geekwolf

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/4356 **Author:** [@geekwolf](https://github.com/geekwolf) **Created:** 10/16/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/cli-cleanup-retry-evidence` --- ### 📝 Commits (1) - [`c84e795`](https://github.com/morluto/rea/commit/c84e795f94ba5d41f7ccee60fc57920c5f7eea22) fix(cli): preserve resource-owned cleanup status ### 📊 Changes **2 files changed** (+49 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/cli/finishAnalysisCommand.ts` (+2 -2) 📝 `tests/boundary/cli/captureCleanup.test.ts` (+47 -0) </details> ### 📄 Description ## Upstream coverage update — 2026-10-16 Main now independently covers this defect through #4274, commit 1b97b0f3707b1cf78ac98bd4ae81638b33cc286f. The generic CLI finalizer no longer resolves producer cleanup status, and main includes a resource-owner regression. Relevant main workflows were rechecked on 8ab5024397ddbd3b37ab68a13cabedc2eb7ce7dd: 53 tests passed, with two existing Windows-gated POSIX signal cases skipped. This draft remains open, not merged; its superseded patch is not being replayed. Historical proposal and validation follow. --- ## Summary Keep cleanup status owned by the resource lifecycle rather than clearing it in the generic one-shot CLI finalizer. This corrects the regression introduced by 93c620c3. No separate issue was opened; the concrete regressions are the packet-capture and signal-recording cleanup tests on current main. ## Problem and change `finishAnalysisCommand` treated a successful `service.close()` as proof that every resource referenced by an earlier failure was released. It changed `cleanup_incomplete` into `execution_failure` and removed the unresolved-resource list even when the resource owner had not resolved its error. Remove that blanket mutation. Actual resource owners retain their existing `resolveCleanup()` behavior. The CLI continues awaiting close, retaining completed observations, and merging a later cleanup failure with the primary error. A public command-boundary control verifies that an owner's explicit resolution is still honored. Scope decision: not applicable to this correction within existing lifecycle contracts. No new provider, execution authority, configuration, schema, package metadata or performance claim. ## Validation - Pinned baseline: main `42fc6c53ae188978deb3bedfe88287b6b316ba22`. Both existing successful-retry CLI cases failed with `execution_failure` instead of `cleanup_incomplete` and lost their resource list. - After the fix, the four focused CLI files passed all 19 tests, including late cancellation, primary/secondary errors, failed cleanup retries and explicit owner resolution. - `npm run check:fast`, full `npm run check`, scoped formatting and `git diff --check` passed on Windows x64, Node 24.18.0 / npm 11.16.0. - Broader controlled-producer verification: 82 tests passed, two failed before producer execution because this Windows account cannot create file symlinks (`EPERM`). Fourteen POSIX descriptor cases were skipped by their existing platform gates. These unavailable cases are not claimed passing. No real TShark/sigrok or POSIX signal verification was performed. - Related independent baseline fixes are #4357 and #4358. Combining these three patches locally passed 65 focused tests (two Windows-gated POSIX CLI cases skipped). This branch's hosted source-cancellation and discovery-probe failures match those two separate fixes; the branch is not CI-green. Current-head hosted CI remains required. ## Author readiness Human owner: @geekwolf - [ ] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [ ] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [ ] I declare this PR ready for initial maintainer review. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:12:37 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:46:04 +00:00

[PR #6196] [CLOSED] fix(analysis-view): locate malformed pcap parents at the view source #7198

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#6196

No description provided.

Rows
Columns