Repository navigation
feat(cdp): let a caller bound hog and liquid rendering with a deadline - #133495
trunk-io[bot] merged 13 commits into
Conversation
HogInputsService.buildInputs takes an optional deadline signal. A hog render and a liquid render stop at the earlier of their own limit and the deadline, and a passed deadline fails the inputs with the deadline error. The deadline records when it expires, so a render checks the clock as well as the signal. No caller passes a deadline yet, so no send changes. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
🤖 CI report✅ Trunk lane — non-backend laneThis PR is assigned to the non-backend lane. It does not run backend Python tests and may merge in parallel with PRs in other lanes. ✅ Duplication (Python) — cleanNew Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.
|
| First copy | Second copy | Lines | Tokens |
|---|---|---|---|
nodejs/src/cdp/services/hog-inputs.service.test.ts:419 |
nodejs/src/cdp/services/hog-inputs.service.test.ts:555 |
28 | 160 |
…es or after a hog yield A liquid input renders each of its leaves in turn, and a leaf that starts after the deadline still got a millisecond, so a large object could keep rendering toward the invocation budget. Each leaf now checks the deadline before it renders. A hog render could also finish just before the deadline and then return after it, from the event-loop yield, as a success; the deadline is now checked once more after that yield. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The test now checks how far the clock moves past the deadline, which the caller waits on, rather than how often a property was read. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Risk: No findings The only change since the last review is a merge of origin/master into the deadline branch; the PR's own deadline code (deadline.ts, hog-exec.ts, sempahore.ts, liquid.ts) is byte-identical to what was reviewed at 9d0d9e3. I reviewed the merged-in code that touches the PR's area plus the highest-risk new surfaces (credentials-service auth, admin console results, CDP integration decrypt-failure and rebuildable-inputs handling, workflows email sandbox) and found no vulnerability introduced by this change. Sentinel reviewed |
|
Review requests went to the owners of the changed code. These owners were not requested because their part of this change looks trivial:
If you were listed here and want to review, you can still assign yourself as a reviewer. |
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 5e7d68d: 1 Should fix.
See this review and its findings in PostHog Review. Comment @posthog review --deep for a Deep review. A Deep review takes longer and looks deeper than a Standard review.
There was a problem hiding this comment.
Not approved — this change needs a human reviewer.
Re-add the stamphog label to request another review once you have addressed this.
@PostHog's current-head concern about deadline enforcement during semaphore waits remains unresolved. Make semaphore acquisition deadline-aware, remove expired waiters, and add queue-contention coverage before re-requesting review.
- Author wrote 0% of the modified lines and has 9 merged PRs in these paths (familiarity MODERATE).
- PostHog Review (Standard) reviewed the current head: 1 finding, posted on the PR.
- posthog[bot] reviewed the current head.
- @PostHog, nodejs/src/cdp/utils/hog-exec.ts: The deadline is checked only inside semaphore.run, after acquisition. In nodejs/src/cdp/utils/sempahore.ts:6–11, queued acquisition waits without cancellation, so an expired render remains pending behind preceding renders. The current code does not fix the reviewer's request for abortable acquisition and removal of expired waiters.
Gate mechanics and policy version
| Gate | Result | |
|---|---|---|
| prerequisites | ✓ | all clear |
| deny-list | ✓ | no deny categories matched |
| size | ✓ | 102L, 4F substantive, 436L/6F incl. docs/generated/snapshots — within ceiling |
| tier | ✓ | T1-agent / T1d-complex (436L, 6F, single-area, feat) |
| stamphog 2.7.0 | .stamphog/policy.yml @ 5e7d68d · reviewed head 5e7d68d |
…ts deadline passes A render that queues behind another one now leaves the queue when its deadline passes, so buildInputs settles with the deadline error at once rather than after the earlier renders finish. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An input of many hog leaves queues one waiter per leaf under one deadline. Removing each with a filter made the abort quadratic and blocked the event loop; a set removes each waiter at once. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 32d13cc: 1 Should fix.
This is review round 3 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
The wall-clock bound on dropping queued renders moves to an in-band serial bench, so runner contention in the parallel suite cannot fail it. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A wall-clock bound belongs in a benchmark, not in a suite CI runs. The bench now sits with the other dev-only runs, which Jest skips in CI. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ders Each queued leaf added its own abort listener, and the signal scans its listeners on every add, so queuing thousands of leaves under one deadline blocked the event loop. Waiters on one signal now share a listener. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…akes the render queue Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…timers The leaf that held the render queue waited on a fake timer, so the tests after it found the queue taken and hung. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 024fd7b: 1 Should fix.
This is review round 4 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
A hog input that fails with the deadline's own reason rethrows it unchanged, so a second build on the same deadline no longer fails with a TypeError while stamping a different bytecode contract. Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…der-deadline Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
PostHog Review · Standard · reviewed 9702e97: 1 Must fix, 5 Should fix. This pull request is large, so the review ran in 4 parts with less depth than usual.
This is review round 5 on this pull request. When findings keep coming back in the same code, the approach is usually the problem. Before the next fix, check whether a different design would remove the whole class of findings.
…er-deadline Refs PostHog/michaelr-context#77 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
This pull request was merged into |
Deploy status
This table shows the |
Merge gate: passed.
fleet proof gatepasses on 024fd7b (shadow diff, review at HEAD, CI green, judge fail accepted by standing rule). Layer 3 of 4 in the node-store attachment stack: #133482, #131833, this PR, then #131944. The stack merges from its top PR.Problem
Note
🟢 Merge risk: low · two-way door
Changes
HogInputsService.buildInputstakes an optional deadline signal, and a passed deadline fails the inputs with the deadline error.buildInputssettles at once. Renders under one deadline share one abort listener.How did you test this code?
Test rationale: the new cases in
hog-inputs.service.test.tsdrivebuildInputswith a deadline. They fail if a render outlives the deadline, if a passed deadline is ignored while an input waits for the event loop or after a Hog input yields it, if a queued input keeps waiting behind another render after the deadline (also once one of its own leaves holds the queue), if a Liquid input of many leaves runs more than one leaf past the deadline, or if a render ends with its own error in place of the deadline error. Breaking the Liquid deadline cap, the Hog deadline passthrough, or the shared abort listener turned a case red.execHogrenders per trial, without a deadline, half under one deadline, and each under its own. Every render finished in order with its result at both commits, within trial noise; a render with its own deadline pays about 6 µs for its listener.nodejs/src/cdp/dev/hog-exec-queue-bench.test.tstimes queuing and dropping 50,000 renders under one deadline. CI skipsdev/, so it runs by hand; a filter-based removal and a listener per waiter each went past its 1 s ceiling.Release status
Docs update
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Agent: Claude Code, Claude Opus 5.5
Skills:
/writing-tests,/writing-pr-descriptions,/stacking-prsMore context for AI agents
nodejs/src/cdp/utils/hog-exec.ts, the deadline checks around a Hog run, andsempahore.ts, the waiters grouped by deadline.🤖 Generated with Claude Code