Skip to content

fix(sdk): skip work cancelled before it starts - #1733

Merged
mldangelo-oai merged 11 commits into
mainfrom
mdangelo/codex/abortable-work-admission
Oct 23, 2026
Merged

mldangelo-oai merged 11 commits into
mainfrom
mdangelo/codex/abortable-work-admission

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Skip cancellable work when its signal is aborted before the scheduled callback starts. This prevents cancelled scans from starting target preparation or model work.

Changes

  • Check the signal immediately before entering the queued operation while retaining the existing in-flight cancellation behavior.
  • Cover both already-aborted and newly-aborted signals, successful work, and errors. Boolean cases have distinct JUnit names.
  • Merge current main.

Testing

Merged main at fe9d77e1c3d9 into 2f332bf3ce96. This refresh changes only the two Docker base-image references to the Node 26.12.1 tag and digest already merged into main; all other tracked files are identical to 6096a09e3578. git diff --check passed. Container builds and runtime suites were not rerun for this refresh; fresh hosted CI is required.

Earlier Linux checks at 6096a09e3578 passed:

  • All 74 workflow, report-comparison, package-skeleton, container-workflow, and package-timeout tests.
  • All 27 native-configuration and fixture-lifecycle tests.
  • SDK build:ci, SDK/MCP types, full SDK formatting, and git diff --check.
  • Actionlint syntax/expression checks for the two refreshed workflows; ShellCheck and Pyflakes were disabled.

Those earlier checks used Node 22.23.3 and Bun 1.3.14. Dependency lockfiles and unchanged plugin sources were matched before reusing dependency installations and private copies of existing bundled/native artifacts. Native binaries were not rebuilt. Full SDK and cross-platform suites were not rerun; fresh hosted CI must qualify this head.

Earlier cancellation results remain historical. The recorded seeded SDK run was incomplete after eleven authentication assertions encountered an invalid enclosing Git marker; that record did not establish a corrected-environment rerun or a default-order full-suite pass.

Risk and rollout

No public API or configuration changes. Operations cancelled before admission now stop before their callback runs; operations already running keep the existing cancellation behavior.

Public disclosure review

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

@mldangelo-oai

mldangelo-oai commented Oct 19, 2026 •

Copy link
Copy Markdown
Collaborator Author

Local SDK validation for bccbd0c25c30c1b6ea2b5a477a33783650e637a1 is complete.

Command Seed Passed Skipped Failed Exit
pnpm run test --seed 12345 12345 4,928 65 11 1
pnpm run test 3219181869 4,928 65 11 1

Both runs failed only in the same eleven authentication cases, each matching the control's Git-root lookup error. These eleven failures also reproduce on the unchanged base in the managed worktree; all eleven pass with 46 assertions when only the working directory changes to a clean directory. The failures arise from an invalid enclosing Git directory on this test host.

Focused target tests passed (37 passed, 2 platform-specific skips), and SDK types and formatting checks passed. Hosted CI passed on the same commit.

@github-actions github-actions Bot added the bug Something isn't working label Oct 19, 2026
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-23T05:45:44.606522Z 2f332bf New commits
🔒 Security Review ✅ Completed 2026-10-23T05:46:12.488869Z 2f332bf New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@alandelong-oai alandelong-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for tightening cancellation at the shared callback boundary. The runtime change looks sound in this merge-safety and simplification review of bccbd0c25c30c1b6ea2b5a477a33783650e637a1: the built public API skips a queued repository read after cancellation, while the base still starts it; ordinary ordering and already-started settlement remain intact.

One testing issue needs correction: the new boolean cases share a JUnit name and make the scheduled test-quality inventory checker fail. Changing the title placeholder from %s to %p resolves it.

Validated 12 cancellation cases and 44 validation/deduplication caller cases on pinned Bun 1.3.14, a built Node smoke, SDK/plugin builds, types and formatting. The broader targets suite has two local enclosing-Git-marker failures; matching-head required CI is green. The duplicate-name failure was independently reproduced with the inventory checker and Bun 1.4.2.

Comment thread sdk/typescript/tests-ts/targets.test.ts Resolved

@alandelong-oai alandelong-oai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing queued cancellation and the duplicate test names. This owner-level merge-safety and simplification review of d83be71fbecf005a3ee1119baf2600aebbdbf73a found no remaining issues. The shared check skips canceled callbacks while preserving ordinary ordering and already-started settlement.

Fresh validation passed: 12 cancellation cases, 75 record-runner cases, 2 hosted-preparation cases, JUnit inventory comparison, a built Node API smoke with an exact-base control, SDK/plugin builds, types and formatting. Broader local target/hosted tests encounter fixture ownership and enclosing-Git-marker restrictions also reproduced on the base; no full-suite pass is claimed.

@mldangelo-oai
mldangelo-oai merged commit 7728491 into main Oct 23, 2026
64 checks passed
@mldangelo-oai
mldangelo-oai deleted the mdangelo/codex/abortable-work-admission branch October 23, 2026 17:26
@github-actions github-actions Bot mentioned this pull request Oct 23, 2026
3 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants