Repository navigation
fix!: align task lifecycle and runtime contracts - #196
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c84bb84d69
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5cef1cb90
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Port poolifier/poolifier#3360 at a9c3ce083 while preserving Web events, worker UUIDs, protected hooks and runtime-specific contracts. BREAKING CHANGE: deferred task function operation handlers must retain taskFunctionOperationId or the original taskFunctionProperties reference. Replies without recoverable identity reject after the original cohort settles.
jerome-benoit
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cadd7ec827
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private chooseWorkerNode(name?: string): number | undefined { | ||
| const workerNodeKeys = this.getTaskFunctionWorkerNodeKeys(name) |
There was a problem hiding this comment.
In a dynamic pool with min = 0, executing a runtime function restricted to a later index (for example [2]) initially creates worker 0 before any live metadata exposes that affinity. If replay fails on that ineligible worker but leaves it alive, waitForWorkerTaskFunctionBootstraps() correctly suppresses the failure, yet this lookup still returns no affinity because it only consults worker metadata. Since the pool is then neither empty nor busy, no additional dynamic worker is created and selection rejects instead of reaching worker 2. Fall back to the captured taskFunctions registration here so an ineligible replay failure cannot prevent creation of the eligible worker.
AGENTS.md reference: AGENTS.md:L105-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed P2 on real Deno 2.9.7, Bun 1.4.2 and Node 26.11.1 workers. This is shared upstream behavior, not a port regression: the same lookup/growth path exists at a9c3ce083:1800-1849.
Public registration with min=0 and affinity [2] succeeds. After worker 0's real replay validation failure, execution exhausts six selection retries; only key 0 exists and a subsequent native round trip proves it remains alive. All three no-fault controls create keys [0,1,2] and execute on actual worker 2. The replay failure is deliberately introduced through standard worker validation, not a fabricated ACK. Commands and six logs are retained under /tmp/pr196-captured-affinity/.
Minimum correction: recover missing runtime affinity from the captured registration before growth/eligibility selection, reusing existing name/default resolution. This remains open; the faithful #3362 follow-up changes only CI and does not silently introduce an unrelated pool algorithm change.
| } catch (error) { | ||
| reject(error) |
There was a problem hiding this comment.
When sendToWorker throws for a later cohort member after an earlier worker has already accepted the mutation, this immediately rejects the operation and removes every acknowledgement listener. The mutation queue can then dispatch the next public mutation while the accepted request is still pending; a worker override that defers operations can apply that earlier request after the later one, breaking the documented call-order guarantee and leaving its result unobserved. Preserve the transport error but wait for already-contacted targets to acknowledge or exit before settling the operation.
AGENTS.md reference: AGENTS.md:L105-L112
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed P2 on real Deno 2.9.7, Bun 1.4.2 and Node 26.11.1 workers. The source is shared with upstream: a9c3ce083:3100-3112 immediately rejects and removes accepted-target listeners.
The controlled reproduction holds an earlier REMOVE on worker 0 and injects a one-shot protected sendToWorker exception for worker 1. The next ADD completes on both workers before the old REMOVE ACK. Releasing that genuine request then removes the new function from worker 0: subsequent calls alternate between not-found and the new result. Both before/after states and actual ACKs are observed; no ACK is mocked. This proves the exception-boundary behavior, not spontaneous failure of a stock native transport. Commands/fixtures remain under /tmp/pr196-send-cohort-failure/.
This is ordering, not transactional rollback. Retain the transport error but wait for already-contacted targets to acknowledge or exit before releasing the mutation queue. Finding remains open; no unrelated runtime fix is folded into the CI-only #3362 follow-up.
SonarCloud gate: intentional thenable adapterCI passes 8/8 jobs at The reported adapter is deliberately identical to upstream. It captures Replacing it with No workaround or analyzer suppression was added. Recommended action: accept this intentional rule exception in Sonar, with this evidence. Automated API/browser sessions have no authenticated Sonar user or issue permissions; no Sonar token is available. The external gate remains blocked pending that triage. |
Port poolifier/poolifier#3362 using the native release CLI on Ubuntu. The existing explicit structural context annotation already satisfies the public API requirement.
Keep optional browser promise inspection from rejecting valid thenables. Preserve native branding, single-read assimilation and original receivers. Align the neutral registration lookup with upstream. Closes #197
|
Fixed #197 in b4e34cb. Optional browser prototype/own-property probing now falls back to captured-then assimilation; native branding and genuine getter/constructor errors are unchanged. The neutral registration lookup now matches upstream. Verification: 110 focused Deno regression steps pass; real Chromium 155 passes 11/11 scenarios using the committed task/cleanup fixtures (the pre-fix bundle fails nine); browser/Bun builds, DOM/WebWorker declarations, JSR dry run, lint and formatting pass. No fixes for upstream #3365/#3366 are included; those algorithms remain unchanged. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4e34cb019
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Register original cohort exits before protected Web Worker hooks can terminate a later target synchronously. Refresh captured-owner indices after listener registration to avoid dispatching to replacements. Keep upstream acknowledgement and send-error semantics unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd7183c281
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (error) { | ||
| // Retired bootstraps no longer block waiting submissions. | ||
| if (this.workerNodes.includes(workerNode)) throw error | ||
| } finally { | ||
| this.workerTaskFunctionBootstraps.delete(workerNode) |
There was a problem hiding this comment.
When a live worker fails to replay a stored runtime function, this removes its bootstrap while leaving the worker permanently marked not ready. A later addTaskFunction() that replaces the bad registration can succeed on that same worker, but nothing retries the bootstrap or restores readiness, so a one-worker fixed pool still rejects every execution with no eligible worker despite now holding the valid function. Keep a recoverable failed-bootstrap state, retry replay after a corrective mutation, or retire the failed worker instead of discarding its bootstrap unconditionally.
Useful? React with 👍 / 👎.
Refresh the captured owner index after protected listener registration. Skip dispatch when a synchronous Web exit already retired that owner. Preserve acknowledgement, rejection and cleanup semantics.
|
PR Checklist
Does this PR introduce a breaking change?
Deferred worker overrides must echo
taskFunctionOperationId, or retain the originaltaskFunctionPropertiesreference through the standard transport. Replies without recoverable identity reject after the original cohort settles; identities are never guessed. Migration and runtime constraints.Upstream alignment
Ports poolifier/poolifier#3360 at
a9c3ce0832093df258e946f244dbb6c718456e8fand #3362 at778fb7adf12ff9ab6445d229f08bc0ad059a6837onto the existing port fixes.Independent pool and worker reviews repeated after both ports, with four AST comparisons. Shared algorithms follow upstream identifiers, predicates and helper structure. Remaining differences are classified Web/runtime boundaries or preserved port contracts; no unexplained semantic drift found.
Port-specific boundaries
AsyncLocalStorage, promise branding and synchronous queued-port reads. Web exit consumers drain before channel closure, including reentrant retirement and cleanup ACKs.Verification
2310e523)missing-explicit-type; restoring it passes. No upload or slow-type bypassnpm pack --dry-run2310e523)2310e523)Closes #189, closes #190, closes #191, closes #192, closes #193, closes #194, closes #195, closes #197.