diff --git a/brain/knowledge/decisions/000015-fan-in-is-an-event-driven-waitpoint-barrier.md b/brain/knowledge/decisions/000015-fan-in-is-an-event-driven-waitpoint-barrier.md index 94095a8c881a..3018b1404f94 100644 --- a/brain/knowledge/decisions/000015-fan-in-is-an-event-driven-waitpoint-barrier.md +++ b/brain/knowledge/decisions/000015-fan-in-is-an-event-driven-waitpoint-barrier.md @@ -45,10 +45,21 @@ all, purely because child rows appear later than the dispatch that created them. - **Receiving is an upsert, last write wins.** It is not a state machine and does not reject a second write: a retried child reports its terminal status again — possibly a different one — and the barrier must reflect the latest truth, not the first. That also makes redelivery of the receive path free. +- **A human decision is the one exception: it is compare-and-set.** `receiveSignal` keeps the upsert above for + machine producers; the confirm link goes through `recordDecision`, whose `UPDATE` carries + `AND status = PENDING` and answers "already responded" when it matches nothing. A double-click, an email + client prefetching on hover, or two people on a shared inbox must not silently flip a recorded approve into + a reject. Both paths write with an `UPDATE … RETURNING "waitpointId"`, never read-then-`save()`, which + *re-inserted* a signal row the release had already deleted. `recordDecision` also takes `FOR SHARE` on the + barrier first, so `closeBarrier`'s `FOR UPDATE` waits for it: a decision is either counted in the summary or + refused, never dropped between the count and the signal delete. `receiveSignal` takes no lock; a machine + signal that commits during a close can miss the summary, which "last write wins" already accepts. - **The deadline is set at create and nothing moves it.** The floor rule needs an evaluation to fire; a barrier nobody ever signals gets none, so the deadline is the only thing between "the producer died hard" and a run paused until retention deletes it. Seal does not touch it — the clamp with no requested value already returns the ceiling, and a shorter deadline is a `policy` input, so both are known at create. + `defaultBarrierDeadline({ flowRunCreated })` anchors it to the run's start, not to the moment the barrier + opened, so a barrier inside a long-running run cannot outlive `AP_PAUSED_FLOW_TIMEOUT_DAYS`. - **Evaluation must be coalesced, and coalescing must not swallow its own last signal.** At 10 000 signals, evaluating per signal is ~100M row reads. BullMQ holds a deduplication key while the job is queued *and* active, so the handler's **first statement** is `clearEvaluationDedupKey`, and every producer commits diff --git a/brain/knowledge/engineering/ci-pr-review-hygiene.md b/brain/knowledge/engineering/ci-pr-review-hygiene.md index f8f33538c536..00a16b5ebcef 100644 --- a/brain/knowledge/engineering/ci-pr-review-hygiene.md +++ b/brain/knowledge/engineering/ci-pr-review-hygiene.md @@ -72,5 +72,6 @@ Enforcement is the **`Codeowners review` repository ruleset** (active on the def - **Fixing an infra failure can turn a branch from red to a different red, and that is progress, not a regression.** A runner killed mid-run (`shutdown signal` above `exited (137)`) reports no test results at all, so genuine failures underneath it are invisible. When the OOM is fixed the suites finally run to completion and those failures surface for the first time — on this stack, thirteen branches went from an OOM red to a real `barrier.test.ts` red the moment the runner survived. Check whether the *previous* run reached a test summary at all before blaming the change that unmasked it. - **A `beforeAll` that overrides the vitest timeout *downwards* is a slow-motion flake, and it fails in a way that blames your diff.** [`execute-flow-e2e.test.ts`](../../../packages/server/api/test/integration/ce/flows/flow-run/execute-flow-e2e.test.ts) passes `30_000` to `beforeAll` even though `packages/server/api/vitest.config.ts` sets `hookTimeout: 60000`, and spends 5s of that budget on a hardcoded `setTimeout`. Inside the remaining ~25s it boots a fresh app, runs the **whole migration chain**, starts queue consumers and a worker. That chain has grown 293 → 376 postgres migrations since the hook was written (`d2c42816013`, "feat: worker v2", #11608, 2026-03-15 — authored *after* the 60s default was already in place), so on a loaded runner it now times out. When it does, all 9 of its tests report **skipped**, not failed: the summary reads `1 failed | 84 passed` with no failing assertion anywhere, and `--bail 1` kills the rest of the CE task. Seen on [#15078](https://github.com/activepieces/activepieces/pull/15078), an MCP-OAuth-only PR that cannot reach that code. No single commit broke it, so there is nobody to blame and nothing to revert — the fix is to delete the override and poll for worker readiness instead of sleeping blind. When triaging a red CE task, check whether the failure is an assertion or a hook timeout **before** blaming the diff. - **Git hooks cannot run from a `git worktree`: `.husky/_/husky.sh` is an install artifact that only exists in the tree where `bun install` ran.** Every commit and push from a worktree dies on `.: cannot open .husky/_/husky.sh`, before the hook's own logic is reached. Don't reach for `--no-verify` — push the worktree's commit from the main checkout (`git push origin :`; worktrees share the object store), so `pre-push` actually bootstraps and its "no direct pushes to `main`" guard runs. `SKIP_CHECK=1` is the repo's own sanctioned way to skip the lint/test prompt. Note `pre-push` special-cases `CLAUDE_PUSH=yes` into running the **full** lint + unit + API suite, so an agent that silently bypasses the hook is skipping a check the repo added deliberately for it — say so out loud when you do. +- **Merging up a stacked chain, a parent's rename reaches a child's call sites as a clean auto-merge — only the typecheck finds it.** Git conflicts on lines both sides edited, so a symbol renamed on the parent merges silently into a child that merely *calls* the old name: `feat/barrier-service` renamed `barrierService.receive` → `receiveSignal`, and `feat/barrier-edges`' `resume-controller.ts` kept calling `.receive(...)` with no conflict marker. Worse, both sides can each be right and still merge wrong — the same commit renamed `findPreCompletedByFlowRunId` while the child had rewritten that function's *body* to fix a real bug, so the conflict exposed only the signature and taking either side whole silently dropped work. Two habits: typecheck **every** branch in the chain after its merge rather than trusting a conflict-free result, and before picking a side run `git log --oneline $(git merge-base HEAD MERGE_HEAD)..HEAD -- ` on both to read what each side was actually doing. Stacks here run deep (the waitpoints → process-in-batches chain was 17 PRs), so one missed rename propagates through every branch above it. - **Docs videos belong on `cdn.activepieces.com/videos/docs/`, never in the repo — and `docs/resources/` is the legacy path that suggests otherwise.** Every `