Skip to content

feat(insight): add async export scheduling - #702

Closed
wangyb-A wants to merge 23 commits into
mainfrom
feat/insight-async-export
Closed

feat(insight): add async export scheduling#702
wangyb-A wants to merge 23 commits into
mainfrom
feat/insight-async-export

Conversation

@wangyb-A

@wangyb-A wangyb-A commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Summary

  • move Workflow Insight rendering, truncation, export, and flush off the checkpoint thread
  • coalesce cumulative snapshots with one lazy daemon lane per exporter
  • drain and flush with one shared configurable timeout
  • prevent one exporter object from being driven by multiple live scheduler lanes

Design

  • one lazy daemon worker per exporter object
  • one in-flight record and one latest pending record per lane
  • a newer pending snapshot replaces the older one, regardless of execution ARN
  • scheduling only swaps the pending reference, starts or wakes the worker, and returns
  • custom-renderable and non-JSON values are not inspected on the checkpoint thread
  • exporter-specific copying, rendering, truncation, and export stay on the worker
  • export_timeout_seconds defaults to 5 seconds and bounds caller wait for drain plus flush
  • timed-out lanes retain at most one detached flush so buffered records can publish before idle exit
  • workers stop when idle; blocked workers are retained and never replaced
  • worker-start failures disable the lane without escaping hooks or bypassing invocation cleanup
  • exporter ownership lasts as long as its lane, including an orphaned live worker
  • no core SDK changes

Validation

  • full Insight suite: 133 passed, including local-runner e2e
  • scheduler suite: 31 passed
  • config and ownership suite: 44 passed
  • focused scheduler and async-plugin suite: 40 passed
  • latest-pending replacement, 100-record burst, opaque/deep-value scheduling, exporter isolation, shared-timeout, queued/popped detached-flush races, sizing-resource failure, startup-failure, cleanup, and ownership-finalization regressions passed
  • the queued-barrier, sizing-resource, deep-record, and concurrent-finalization regressions were proven to fail on pre-fix production code and pass with their fixes
  • mypy passed
  • Ruff lint and format passed
  • wheel and sdist built
  • full repository collection completed without errors
  • prior Workflow Insight conformance remains CloudWatch 18/18 and S3 18/18
  • exact-SHA local correctness review at afc10fe: no remaining or new Critical/High findings

Latest review fixes

  • replaced per-execution admission and retained-graph estimation with one structurally bounded latest-pending slot per exporter lane
  • removed checkpoint-thread object sizing and custom-state traversal
  • retained worker-side support for custom-renderable and non-JSON values
  • let resource failures escape JSON sizing into the worker isolation boundary instead of retrying through export
  • passed the original record to custom rendering when lane-local container copying fails
  • preserved bounded invocation-end wait and one eventual detached flush after timeout
  • coalesced canceled and detached flush markers at their rightmost queue position so buffered records remain covered
  • preserved later invocations' detached flushes when an older barrier is already in flight or still queued
  • retained strong ownership references through lock release so custom exporter finalizers cannot re-enter the ownership lock
  • contained worker-start failures so instrumentation cannot break checkpoint hooks or state cleanup
  • enforced exporter ownership across plugin instances by lane identity and lifetime

no linked issue: #687 tracks broader Workflow Insight follow-up work

@wangyb-A
wangyb-A force-pushed the feat/insight-async-export branch from 0bbf504 to 6b73e82 Compare September 2, 2026 18:43
@wangyb-A

wangyb-A commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

/ai review

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@wangyb-A

wangyb-A commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

/ai review

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A marked this pull request as ready for review September 3, 2026 18:35
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime September 3, 2026 18:36 — with GitHub Actions Inactive
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime September 3, 2026 18:52 — with GitHub Actions Inactive
@wangyb-A
wangyb-A had a problem deploying to ai-pr-review-runtime September 3, 2026 18:52 — with GitHub Actions Failure
@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A force-pushed the feat/insight-async-export branch from 9e0e717 to 2e7049c Compare September 3, 2026 21:41
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime September 3, 2026 22:17 — with GitHub Actions Inactive
@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime September 3, 2026 22:17 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A temporarily deployed to ai-pr-review-runtime September 8, 2026 21:13 — with GitHub Actions Inactive
@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A deployed to ai-pr-review-runtime September 10, 2026 03:25 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A removed this pull request from stack #715 September 10, 2026 17:39
@wangyb-A
wangyb-A deployed to ai-pr-review-runtime September 10, 2026 17:58 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A deployed to ai-pr-review-runtime September 10, 2026 19:10 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A deployed to ai-pr-review-runtime September 10, 2026 21:23 — with GitHub Actions Active
@github-actions

This comment has been minimized.

@wangyb-A
wangyb-A deployed to ai-pr-review-runtime September 10, 2026 23:10 — with GitHub Actions Active
self._queue.append((_RECORD, None))
else:
self._move_record_token_to_back()
self._pending = record

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Codex AI review · Finding arf_v1_hhdiypndoo4kuhjkuxlfcqckuf

[P2] Release superseded records after dropping the lane lock. Replacing _pending decrements the previous record while _cond is held. Supported transformed/non-JSON values may define __del__, so the checkpoint thread can run arbitrary blocking code here; a finalizer that re-enters this lane deadlocks on the non-reentrant lock. Retain the displaced record until after the with block, similarly defer pending cleanup in _disable_locked, and add a finalizer re-entry regression.

stack.extend(item)
else:
memo[identity] = item
return copy.deepcopy(record, memo)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Codex AI review · Finding arf_v1_54if3osmob4i3ya37qb2nfe6ew

[P2] Keep deeply nested records isolated between exporter lanes. This recursive deepcopy raises for sufficiently deep built-in containers, after which _export_one passes the original canonical record to every lane. Concurrent exporters can then mutate or normalize the same object, producing timing-dependent or corrupted output. Clone built-in containers iteratively while preserving aliases and cycles, and add a two-lane deep-record test where one exporter mutates its input.

@github-actions

Copy link
Copy Markdown
Contributor

Codex AI review

Two P2 scheduler issues remain in _export_scheduler.py (lines 122 and 53): checkpoint-thread lock safety and cross-lane record isolation.

Reviewed commit afc10fe129032aa3f2496bcbea3af41b17fe11bd. Workflow run

@wangyb-A

Copy link
Copy Markdown
Contributor Author

Superseded by #719, which re-authors the async exporter around one plugin worker and one latest-pending slot with a much smaller diff.

@wangyb-A wangyb-A closed this Sep 11, 2026
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.

4 participants