Skip to content

[2/3] fix: protect background sessions and record resource history - #457

Closed
slashdevcorpse wants to merge 9 commits into
LodyAI:mainfrom
slashdevcorpse:fix/background-session-resource-history
Closed

slashdevcorpse wants to merge 9 commits into
LodyAI:mainfrom
slashdevcorpse:fix/background-session-resource-history

Conversation

@slashdevcorpse

@slashdevcorpse slashdevcorpse commented Sep 7, 2026 •

Copy link
Copy Markdown
Contributor

Related issue

Refs #429

Problem / pressure

Idle and memory-pressure eviction can discard a session after its parent turn finishes even when a background task or ACP terminal watch remains active. Current resource snapshots also disappear without a bounded record of process identity, resource use, or cleanup outcome.

Summary

  • Inspect goals and background work from one history read per eligibility check. Protect pending/in-progress task state, canonical CronCreate/ScheduleWakeup records until explicit completion/cancellation, and pending/live ACP terminals. Elapsed fire times do not imply completion; stale task history does not pin an absent runtime.
  • Capture runtime/history identity, metadata version and activity across awaited checks. Recheck immediately before termination, then hold dispatch, execution and manager admission through document teardown and transient-state deletion. Queued RPC/meta work resumes against fresh state; direct start/continue/steer waits before document access.
  • Isolate unreadable-session errors and skip changed sessions without counting an eviction. Coalesce document destruction; failed unloads remain retryable, and reopening cannot return a partially destroyed wrapper or let an old unload invalidate its replacement.
  • Retain up to 120 observed samples for ten minutes, with at most 256 attributed processes and 100 session rows per sample. Record resource use, cleanup status, truncation and unavailable-sample receipts in bounded memory.
  • Pin Windows per-process attribution to precise creation-identity strings. Omit session process rows on POSIX where coarse lstart values cannot establish identity; aggregate resource estimates remain available.
  • Keep sampling observer-driven. The local machine/get-resource-history RPC reads without probing, rejects session-scoped access and advertises resourceHistory v1. Strict nested schemas reject unsupported statuses/fields; command lines, environment values and raw error strings never enter history.

Dependency order

Windows process lifecycle series for #429, in bottom-to-top order:

  1. [1/3] fix: bound Windows ACP shutdown and retain failed cleanup #456 — bounded shutdown and retained cleanup ownership.
  2. [2/3] fix: protect background sessions and record resource history #457 — background-aware eviction and resource history; depends on [1/3] fix: bound Windows ACP shutdown and retain failed cleanup #456.
  3. [3/3] fix: own Windows process trees with native job supervision #458 — native Windows Job Object ownership; depends on [1/3] fix: bound Windows ACP shutdown and retain failed cleanup #456 and [2/3] fix: protect background sessions and record resource history #457.

All three PRs are submitted to LodyAI/Lody. Their source branches have linear ancestry. Because GitHub does not support native cross-fork stacks, each PR targets upstream main and later PRs include predecessor changes until those land.

Review the individual layers:

The complete series is required for Windows descendant and crash ownership. The former fork PRs slashdevcorpse#2, #3 and #4 are superseded by this upstream series.

Test plan

  • Final combined run: 223 tests passed across GC, real handler integration, session/execution managers, dispatch admission, real document unload/reopen, terminal liveness and monitoring.
  • Additional focused watcher suites and document lifecycle suites passed. Regressions cover metadata/RPC arrival during preview cleanup, pending direct execution, teardown waits, failed unload retry, concurrent reopen, preserved replacements and per-session inspection failure.
  • CLI/shared typechecks, changed TypeScript formatting, whitespace checks and root check:quick passed (zero lint errors and all boundary guards). Resource-history schemas and the resourceHistory capability registration expectation are covered.
  • Resource-history tests cover bounds, immutable receipts, missing samples, strict nested privacy fields, status validation and same-millisecond Windows identity changes. POSIX coarse timestamps do not authorize per-session process attribution.
  • GitHub Static checks, Tests and Desktop E2E passed at f291ca2. Real-provider memory recovery and unobserved provider-internal task completion remain outside this evidence; Windows crash ownership is implemented in the third PR.

Context handoff

Instructions for reviewing agents

  • Review focus: Trace idle/pressure eligibility through history, metadata and terminal liveness, then follow all admission leases through document destruction and fresh reopen. Verify bounded strict resource history and local RPC scope.
  • Decisions to challenge: Explicit pending task state protects sessions conservatively; history remains observer-driven and diagnostic PID attribution never authorizes killing.
  • Plausible failures / evidence gaps: Unobserved provider-internal completion remains unknown, so live task state stays conservatively protected. Metadata-version changes can defer cleanup. Windows process identity is observational; coarse POSIX session rows are omitted.

Authoring context

  • User goal / directives: Implement a three-PR process-lifecycle stack, with background-aware eviction and resource history as the second dependent PR.
  • Constraints / non-goals: Preserve unrelated work; avoid permanent OS probes, persisted diagnostics, raw process commands/environment, and inferred cron completion.
  • Risk-bearing decisions: Active work prevents pressure eviction even when memory is low; admission leases hold through cleanup and release deferred work afterward. Failed unloads stay retryable; strict history limits truncate large observations.
  • Destructive or irreversible behavior: Existing GC termination remains the only destructive path in this slice; the new guards prevent termination and the history RPC only reads memory.
  • Deliberately not done or tested: No provider-internal scheduler API, all-provider residency claim, continuous background sampling, remote history transport, or real user-session termination.
  • Unknowns / confidence: Focused tests and source review support the scoped behavior. Native crash ownership and repeated real process lifetimes are covered by the third PR.

Latest review verification

At f291ca2, the stack includes the eight latest the first PR review resolutions from 720e84c: PTY admission/draining and retry retention, authentication cleanup ownership, startup owner admission, spawn-error handling, explicit Windows termination semantics and non-retryable cleanup failures. the third PR supplies the requested descendant Job Object ownership. Local composed verification and exact-head fork CI Static checks, Tests and Desktop E2E all passed. Upstream CI on these recreated PRs is evaluated separately. The top PR also passed Windows x64, Windows ARM64 and the same-run dual-architecture npm archive verification.

Copilot AI lite review requested due to automatic review settings September 7, 2026 05:58

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f291ca2dc1

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

): Promise<void> {
if (this.deps.platform === 'win32') {
await this.runWindowsTaskkill(pid, force);
await terminateWindowsChildProcess(child, force);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep Windows descendant processes under cleanup ownership

On Windows, every sandboxed ACP and terminal launch now routes through this call, but terminateWindowsChildProcess only waits for the root child and does not terminate descendants; the removed implementation used taskkill /T. An agent that spawned an app-server, MCP server, or shell child can therefore be reported as cleaned and removed while those processes remain alive after GC or daemon shutdown. Use spawn-time Job Object ownership or another tree-aware primitive before treating root exit as successful cleanup.

AGENTS.md reference: apps/cli/src/session/AGENTS.md:L249-L252

Useful? React with 👍 / 👎.

@github-actions github-actions Bot added the status:needs-pr-attention External PR needs contributor attention before review label Sep 8, 2026
@github-actions

github-actions Bot commented Sep 8, 2026 •

Copy link
Copy Markdown
Contributor

@slashdevcorpse, this pull request was closed because it did not meet Lody's contribution requirements for 7 days.

Open a new pull request using the current template to continue contributing this change. This pull request will not be reopened.

@github-actions github-actions Bot added the status:pr-policy-expired PR closed after contribution requirements remained unmet for seven days label Sep 15, 2026
@github-actions github-actions Bot closed this Sep 15, 2026
@github-actions github-actions Bot removed the status:needs-pr-attention External PR needs contributor attention before review label Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: cli scope: shared status:pr-policy-expired PR closed after contribution requirements remained unmet for seven days

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants