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 c8c9c5e21113..94095a8c881a 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 @@ -14,7 +14,7 @@ producer's ordinal), nullable `label` (the human name in the summary) and a smal Release is an **unconditional floor rule**: a sealed barrier releases once no signal is still `PENDING`. No configuration can produce a hang; `policy` only ever releases *sooner* — `requiredSuccesses` (K-of-N approvals) and `releaseOnFirstFailure` (veto). Evaluation is a coalesced `BarrierJobName.EVALUATE` job on a -dedicated `BARRIER_JOBS` queue, deduplicated on the barrier id, owned by the waitpoints module. Counts are +dedicated `BARRIER_EVALUATION` queue, deduplicated on the barrier id, owned by the waitpoints module. Counts are taken once, from a single `GROUP BY status`, and handed to a pure predicate. Signals are deleted on release, in the same transaction that completes the waitpoint — summary first, then complete, then delete. @@ -51,7 +51,7 @@ all, purely because child rows appear later than the dispatch that created them. already returns the ceiling, and a shorter deadline is a `policy` input, so both are known at create. - **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 `clearEvaluationDeduplication`, and every producer commits + active, so the handler's **first statement** is `clearEvaluationDedupKey`, and every producer commits its signal row **before** enqueuing. Otherwise the final signal lands while the job meant to see it is already running, its enqueue is dropped as a duplicate, and the barrier waits out its deadline holding a run that was ready to resume. @@ -61,6 +61,31 @@ all, purely because child rows appear later than the dispatch that created them. `shouldReleaseBarrier({ policy, sealed, counts })` takes counts rather than issuing its own queries, so one `GROUP BY status` replaces up to three round-trips and the policy matrix is unit-testable without a database. +- **Identity is path-keyed, and that needed no new column.** The engine sends `loop_1:3/loop_2:0/approval` + in the existing `stepName` field, which is only ever an identity key inside `waitpoint-service`. Which + fixes a live bug on the way past: a delay or approval **inside a loop** used to reuse one waitpoint row + across every iteration, so iteration 2 found iteration 1's COMPLETED row in `createForPause`'s + pre-completed check and skipped its pause entirely. +- **Two per-run waitpoint reads stopped being sound and were fixed here.** `getByFlowRunId` became + `findCompletedWaitpointIfRunIsIdle` — **null when any PENDING row exists for the run**, else the newest COMPLETED + one — because per-iteration rows mean a run can hold a COMPLETED leftover *and* an open PENDING pause at + once, and both callers delete the row they find and enqueue a resume. `findNonFanInByFlowRunId` became + `findSubflowWaitpoint` and prefers PENDING, newest first. `findPendingByVersion` is **still** unsound + for parallel branches; nothing here creates that case. +- **The resume guards moved to the entry point rather than becoming a flag.** `resumeFromWaitpoint` + **refuses barriers, always** — by addressed waitpoint type, or, on the by-run legacy routes, by "does this + run hold any PENDING barrier". Every unscoped route goes through it, so there is no `false` to forget and + no parameter to default wrong. What the guard is *not* is structural: `resumeTrusted` and + `resumeTrustedWithoutLock` are ordinary public members of `resumeService`, one `if` away from the guard + they skip. `flow-runs-queue`'s pre-completed recovery calls the unlocked variant from outside the module, + so a module-private `resumeTrusted` would not have worked as written. Treated as acceptable because the + reachable surface — the HTTP routes — is closed; if a third caller appears, make the boundary real rather + than adding a second convention. +- **`release` is what happens to a barrier; `resume` is what happens to a run.** The two verbs sit one layer + apart — `barrierService.release` closes the barrier, then asks `resumeService` to resume the run — so a + `resumeService.releaseBarrier` named the wrong layer and hid that the same method also serves plain `DELAY` + and `WEBHOOK` waitpoints, which is exactly what `flow-runs-queue` calls it for. It is `resumeTrusted`: + resume, skipping the barrier guard, because the caller is internal rather than an HTTP route. - **An external actor never addresses the waitpoint.** Their link carries a **signal id** on `/v1/flow-runs/:id/signals/:signalId/confirm` — in the path, not the query string, because `resumePayload` is built from `{ body, headers, queryParams }` and persisted with the run. The link must never carry @@ -76,7 +101,7 @@ all, purely because child rows appear later than the dispatch that created them. - **The machinery ships able to carry K-of-N approvals; no approval piece is rewired yet.** Per-signal links and `reasonRequiredOn` exist now because retrofitting them means another migration. - **`policy` carries no deadline override.** The plan allowed "or the policy's shorter value"; nothing - produces one, so `resolveDeadline()` is `now + AP_PAUSED_FLOW_TIMEOUT_DAYS` and takes no arguments. Add the + produces one, so `defaultBarrierDeadline()` is `now + AP_PAUSED_FLOW_TIMEOUT_DAYS` and takes no arguments. Add the field when a caller for it exists, not before. - **The 2 000-character reason bound stays a handler check, not a request schema.** The confirm route is `app.all` serving an HTML form: a Zod `body` schema would also apply to the GET that renders the page, and diff --git a/brain/knowledge/decisions/000033-a-released-barrier-leaves-a-tombstone-until-the-run-ends.md b/brain/knowledge/decisions/000033-a-released-barrier-leaves-a-tombstone-until-the-run-ends.md new file mode 100644 index 000000000000..a6d563b19edb --- /dev/null +++ b/brain/knowledge/decisions/000033-a-released-barrier-leaves-a-tombstone-until-the-run-ends.md @@ -0,0 +1,57 @@ +--- +icon: 🪦 +status: accepted +--- + +# A released barrier leaves a tombstone until the run ends + +## Decision + +Delivering a resume no longer means deleting the waitpoint row. `waitpointService.consume` owns the +rule: a `BARRIER` is tombstoned at a third status, `CONSUMED`, and every other type is still deleted. +The tombstone lives until the run reaches a terminal status, where the existing `deleteByFlowRunId` +sweep clears it. The legacy by-run resume guard (`hasBarrier`) therefore asks whether the run holds a +barrier row **at all**, in any status, instead of whether one is `PENDING`. + +## Context + +A barrier's release boundary is predicate-owned: only its own predicate or its deadline may release +it. The guard enforcing that on the deprecated V0 `/:id/requests/:requestId` routes asked +`hasPendingBarrier`, which filters `status: PENDING`, so it stopped guarding the moment the barrier +was released. Two windows followed — between `closeBarrier` committing `COMPLETED` and the trusted +resume consuming it, and the much wider one after delivery, since a run stays `PAUSED` until the +engine's next progress upload. In both, a legacy resume was accepted and enqueued a second job with +a request-controlled payload, under a job id BullMQ would not collapse against the trusted one. + +The root cause was that "this resume was delivered" was encoded as *row-absence*, which makes a +delivered barrier and a pre-waitpoint legacy pause indistinguishable — both are "no rows" — while +the legacy path may only serve the second. + +## Why + +Making delivery explicit is what lets the two be told apart, and `waitpoint.status` is +`character varying` with a TypeScript-only enum, so a third value costs no migration. + +Rejected: a `flow_run.pausedWaitpointId` column — a real migration, and `pauseMetadata` was not +available to reuse (it is deprecated and pending removal). A Redis marker with a TTL — puts a +security boundary on an expiry and a flush reopens the hole. Deleting the legacy path outright — +tempting at 4.5 months past deprecation with a 30-day default pause timeout, but a self-hoster +jumping many versions would silently lose every in-flight approval. + +Tombstoning is scoped to barriers rather than all waitpoints because the barrier is the only +waitpoint whose release boundary is predicate-owned; for `DELAY` / `WEBHOOK` the unguessable link +*is* the authorization. Widening it would leak `CONSUMED` rows into `findSubflowWaitpoint`'s +any-status fallback and into `createForPause`'s unique `(flowRunId, stepName)` slot. + +## Consequences + +A run that has ever held a barrier can never use the legacy by-run resume path again. That is +correct, not a side effect: that path serves only runs which never had a waitpoint row at all. + +`hasPendingBarrier` survives alongside `hasBarrier` and `findUndeliveredCompletedWaitpoint` must keep +calling the *pending* one — switching it to existence would return null whenever a `CONSUMED` barrier +exists and hang a later non-barrier delivery on the same run. + +The invariant that a row still sitting at `COMPLETED` was never delivered is preserved, now carried +by an explicit status rather than by absence. Cost is one small row per barrier for the life of the +run; `barrierService.create` already deletes any non-`PENDING` row occupying its step's slot. diff --git a/brain/knowledge/decisions/000035-the-barrier-signal-cap-is-a-platform-configuration-row-not-an-env-var.md b/brain/knowledge/decisions/000035-the-barrier-signal-cap-is-a-platform-configuration-row-not-an-env-var.md new file mode 100644 index 000000000000..b611aba38347 --- /dev/null +++ b/brain/knowledge/decisions/000035-the-barrier-signal-cap-is-a-platform-configuration-row-not-an-env-var.md @@ -0,0 +1,78 @@ +--- +status: accepted +--- + +# The barrier signal cap is a platform configuration row, not an env var + +## Decision + +`AP_MAX_BARRIER_SIGNALS` becomes `maxBarrierSignals`, an `integer NOT NULL DEFAULT 10000` column on +`platform_configuration`, with a `Limits` section on the Configurations page to edit it. The row is +authoritative and born from the env var, exactly as [000033](000033-platform-configuration-rows-are-authoritative-and-created-on-first-read.md) +does it. The env var survives only as the birth default for a platform that has no row yet. + +Cloud is short-circuited **in the service**: `maxBarrierSignals({ platformId })` returns the system prop +and ignores the row when `edition === CLOUD`, the same shape `isProductTelemetryEnabled` already uses. +`platformId` is added to `CreateBarrierParams` so the caller supplies it rather than +`barrierService.create` resolving it from `projectId`. There is no unlimited value and no nullable +column. + +## Context + +The variable is new on `feat/barrier-service` and has never shipped: it is absent from `main`, from the +published env var table, and from every deploy surface. So this is a choice about where a dial lives +before release, with no upgrade path to protect, no breaking-change label and no docs migration. + +Its only read site is `assertSignalCountWithinLimit` in `waitpoints/barrier-service.ts`, and +`barrierService.create` has no production caller yet, so the signature is free to change. + +## Why + +000033 draws a line that this decision deliberately walks up to: configurations hold behaviour, +`platform_plan` holds entitlements, and a **cap** is the `MAX_RECORDS_PER_TABLE` shape that decision +keeps off the page, because `POST /v1/platform-configurations` carries no edition guard and a limit +living there is a self-serve quota bypass the day the page stops being Cloud-hidden. + +The cap goes there anyway because the bypass is closed where 000033 says invariants must be closed: in +the server, on the value. Hiding the page was never the enforcement, and the Cloud short-circuit in the +service makes the row unreadable as a cap on Cloud regardless of what an admin writes. + +**Rejected: `platform_plan`.** It is the correct home for a limit that is sold, and it is CE-readable +via `platformService.getOneWithPlanOrThrow`. It was rejected because nobody sells barrier width, and a +plan column would put a self-hoster's own protective cap behind an entitlement they cannot edit. + +**Rejected: env var as a `min(row, env)` ceiling.** It preserves an operator guarantee on a +multi-platform EE install, where a platform admin is not the operator and could raise a cap that +consumes shared Postgres and Redis. It was rejected as one concept too many for a case that does not +yet exist, and because it reintroduces the two-sources-of-truth shape 000033 rejected for telemetry. +This is the variant to revisit if multi-platform installs start mattering here. + +## Consequences + +- The error message must stop saying "raise `AP_MAX_BARRIER_SIGNALS`". It is read by a project member + who may not be a platform admin, so it names the limit and the page without implying the reader can + change it. +- `platformId` on `CreateBarrierParams` means every future caller has to hold platform scope. The + waitpoints module already resolves it the other way in `resume-service.ts` + (`projectService.getPlatformId`), so the two paths in one module now differ on purpose: the check runs + before the transaction that inserts up to 10 000 signal rows, and the caller was cheaper to widen + while it has no production implementation. +- **The number reaches the browser whether or not the builder uses it.** The GET response is the whole + `PlatformConfiguration` object, and `TelemetryProvider` wraps the router and already caches that row + for every signed-in user, so any builder component can read it with an existing hook and no new + endpoint. Nothing is built on that now: no fan-out step exists, and the real fan-out shape dispatches + over a runtime array, so a design-time count does not exist for the general case. +- Because Cloud ignores the row, the raw column is **not** the effective cap. Whoever adds a builder + surface must serve a computed effective value rather than recompute the edition rule in the browser, + which is how 000033's split-brain bug happened. +- **No `tryCatch` around the read, because every failure that could reach it is already fatal to + `barrierService.create`.** Postgres down kills the transaction and the signal inserts, Redis down kills + `enqueueEvaluation`, and the `distributedLock` is taken only on the create path, once per platform for + the life of the install. Fail-closed is also the do-nothing option, so there is no code to write. +- **The `Limits` section is written but not rendered.** `limits-section.tsx` exists and is wired to the + same form schema, but `configurations/index.tsx` does not mount it, so no edition shows the field yet. + The row, the API and the cap are all live regardless: the value every platform runs on is the one + `AP_MAX_BARRIER_SIGNALS` seeded, and an existing platform's row can only be changed through + `POST /v1/platform-configurations` until the section is mounted. Rendering it is one import and one + line. +- Postgres only. There is no sqlite migration for `platform_configuration`. diff --git a/brain/knowledge/engineering/ci-pr-review-hygiene.md b/brain/knowledge/engineering/ci-pr-review-hygiene.md index 93ae61d793bc..f8f33538c536 100644 --- a/brain/knowledge/engineering/ci-pr-review-hygiene.md +++ b/brain/knowledge/engineering/ci-pr-review-hygiene.md @@ -44,7 +44,7 @@ Enforcement is the **`Codeowners review` repository ruleset** (active on the def - **Linting a single server test file OOMs node at its default heap — pass `NODE_OPTIONS=--max-old-space-size=8192`.** `npx eslint packages/server/api/test/.../.test.ts` on one file died with `FATAL ERROR: Reached heap limit` after ~23s at 2GB, because the type-aware config loads the whole `packages/server/api` program regardless of how few files you name. It reads as a broken lint setup, not as a memory ceiling. The same run with an 8GB heap finishes and reports normally. - **`bun install` on a recent bun adds `"configVersion": 0` to `bun.lock`, which is not on `main`.** It rides along in any commit that touches the lockfile and reads as an unrelated change; drop the line and re-run `bun install --frozen-lockfile` to confirm the lockfile is still consistent without it. **You do not have to run `bun install` to get it** — a bare `npx vitest run ` in `packages/web` was enough to rewrite the lockfile with `configVersion` *plus* every workspace version already bumped in the branch, so `git status` after any verification run is worth a glance before committing. -- **A version bump that merges cleanly can still be wrong — check what `main`'s number *means*, not whether it conflicts.** Two branches bumping the same package to the same number do not conflict, so git takes it silently; but if `main`'s copy of `0.5.0` is another PR's content and yours adds further exports on top, you ship new exports under an already-published version and nothing catches it. Seen merging [#15001](https://github.com/activepieces/activepieces/pull/15001) after the six-providers PR landed: `core-piece-types` and `pieces-framework` auto-merged at `0.5.0` / `0.37.0` and both needed a further bump. Only a *conflicting* version (like `core/shared` `0.140.0` vs `0.141.0`) forces you to think; the clean ones are the dangerous ones. After any merge, re-check every package you bumped against `git show origin/main:/package.json`. The reverse also happens: when review makes you *delete* code, the bump it justified can become dead — after acting on review, `git diff origin/main...HEAD -- /src` and drop the bump if it is empty. On #15001 two packages ended up byte-identical to `main` while still carrying a bump, which is noise at best and a version collision at worst. +- **A version bump that merges cleanly can still be wrong — check what `main`'s number *means*, not whether it conflicts.** Two branches bumping the same package to the same number do not conflict, so git takes it silently; but if `main`'s copy of `0.5.0` is another PR's content and yours adds further exports on top, you ship new exports under an already-published version and nothing catches it. Seen merging [#15001](https://github.com/activepieces/activepieces/pull/15001) after the six-providers PR landed: `core-piece-types` and `pieces-framework` auto-merged at `0.5.0` / `0.37.0` and both needed a further bump. Only a *conflicting* version (like `core/shared` `0.140.0` vs `0.141.0`) forces you to think; the clean ones are the dangerous ones. After any merge, re-check every package you bumped against `git show origin/main:/package.json`. The reverse also happens: when review makes you *delete* code, the bump it justified can become dead — after acting on review, `git diff origin/main...HEAD -- /src` and drop the bump if it is empty. On #15001 two packages ended up byte-identical to `main` while still carrying a bump, which is noise at best and a version collision at worst. **Nothing in CI catches a *missing* bump outside `packages/pieces/`.** `validate-publishable-packages` is the only bump check and it exempts every non-piece package, so a `core/shared` change with no bump goes green on every check. Seen on [#15078](https://github.com/activepieces/activepieces/pull/15078): nine new exports added to `core/shared/src/lib/automation/mcp/mcp-oauth.ts` with `package.json` still at `0.152.0` — the same number `main` had just reached via a *different* PR (#15162), so the collision was invisible and all 17 checks passed. Reviewing a PR that touches `core/shared`, diff the version by hand. `bun.lock` mirrors workspace versions and drifts behind — it read `0.151.0` while `main` was on `0.152.0`, because bumps routinely land without it (#15162 changed `package.json` alone). Sync it with **`bun install --lockfile-only`**, which needs no `node_modules` and so works in a throwaway worktree; on #15078 that produced exactly one line and swept up no unrelated drift. The 103-line sweep noted above is a *pieces* phenomenon — don't assume it for `core/*`, just read the diff before staging. - **`@activepieces/shared` re-exports from `@activepieces/core-execution`, so a partial rebuild produces phantom "has no exported member" errors in unrelated files.** Rebuilding `core/shared` against a stale `core/execution` dist drops those re-exports, and the API typecheck then fails in `ee/agent/*` on symbols like `GetPersonalizationConfigRequest` — which live in `core/execution/src/lib/workers/worker-contract.ts`, not in shared at all. It reads exactly like a bad merge. The dependency order that actually works is `core/utils` → `core/piece-types` → `core/formula` → `core/execution` → `core/shared` → `server/utils` → `pieces/framework` → `core/ai-providers`; skipping a link silently poisons everything downstream of it. The same staleness makes an editor report missing enum members that exist in the source. **There is no api typecheck "error baseline" — rebuild first, then trust it.** A stale `core/shared/dist` reliably yields a handful of `ee/agent` errors that look permanent enough to be waved off as known, which is how a real error hides among them; after `npx turbo run build --filter=@activepieces/shared`, `tsc --noEmit -p packages/server/api/tsconfig.app.json` comes back completely clean. The same staleness invents errors in files a rebase just touched, so during a conflict resolution rebuild before concluding you resolved it wrong. - **To pull a file back out of a PR, restore it from the merge-base, never from `origin/main`.** A PR's diff is computed against the merge-base, so `git checkout origin/main -- ` does not "revert" the file — it imports every change `main` made to it since the fork and attributes them to you. Dropping one web file from [#15001](https://github.com/activepieces/activepieces/pull/15001) that way would have silently added 82 insertions / 44 deletions of somebody else's work. `git checkout $(git merge-base origin/main HEAD) -- ` makes it byte-identical to where the branch started, so it leaves the diff entirely and merges cleanly instead of conflicting. Verify with `git diff --quiet $(git merge-base origin/main HEAD) -- ` before committing, and read `git status` first — a `bun.lock` left dirty by an earlier `bun install` loves to ride along on a commit like this. - **Retargeting a stacked PR to `main` does not drop its base branch — it merges the whole thing.** A PR opened against a long-lived feature branch shows a small diff *relative to that base*, but `gh pr edit --base main` only moves the target; the branch still contains every commit of its old base. [#14593](https://github.com/activepieces/activepieces/pull/14593) read as 2 docs files against `feat/autumn-billing-integration` and as 198 commits / 211 files / +12k lines against `main`. Check with `git diff --stat origin/main...` **before** retargeting, and if it disagrees with the PR page, cherry-pick that PR's own commits onto `main` and force-push instead. A "conflict" on such a PR is often against the feature base only — those same commits can apply to `main` cleanly. @@ -67,5 +67,10 @@ Enforcement is the **`Codeowners review` repository ruleset** (active on the def - **Nothing rolls `## Unreleased` over at release time, and the docs site is unversioned — so a breaking-changes entry has to name its own version.** No workflow or script writes to `docs/install/reference/breaking-changes.mdx` (`breaking-change-check.ts` only reads it), and `git log -S"## 0.88"` on the file comes back empty: the heading has not moved since 0.87.0, so entries for work that shipped months ago still sit under "Unreleased" (PM2 removal in 0.88.2, cache pre-warm gate and workspace naming in 0.89.0, …). `docs/docs.json` has no versioning either, so there is one live page for every self-hoster whatever version they run, published on merge rather than on release — the version heading is the *only* thing telling a reader whether a change is already in their build. So before adding an entry, run `git tag --contains ` on the change it describes and file it under the release that actually shipped it; only genuinely unshipped work belongs under "Unreleased". What points self-hosters at the page in the first place is `release-drafter.yml`, which appends a "review the Breaking Changes page" line to every release body and groups `⛓️‍💥 breaking-change` PRs under their own heading — which also means a docs-only PR back-documenting an old change shows up in the *next* release's breaking-change list. - **Greptile enforces the file-order rule on *private* constants too, which CLAUDE.md only states for exported ones.** CLAUDE.md says "Exported types and constants must be placed at the end of the file" and gives the order as imports → exports → helpers → types; Greptile reads that as covering module-private constants as well, and flags a `const` sitting above the file's exported symbol (P2 on [#15226](https://github.com/activepieces/activepieces/pull/15226), for two constants only read inside the service they sat above). It has that as a stored custom-context memory, so it will keep raising it. Put private constants in the helpers section below the export — hoisting is a non-issue when they are only read at call time. - **`turbo run lint --filter=` never typechecks that package, so "0 errors" is no proof it compiles — run `turbo run build --filter=` as well.** A package's `lint` script is `eslint 'src/**/*.ts'` with no `tsc`, and the turbo `lint` task's `dependsOn: ["^build"]` builds the package's *upstream dependencies* only, never itself — the `^` is the whole story. So a type error in the package you are editing is invisible to a filtered lint run. CI still catches it, but confusingly under the **lint** job as well as **main**, because linting the whole repo makes your package an upstream of something else whose `^build` finally compiles it. A branch can therefore sit red on two jobs for one `tsc` error that a local filtered lint reported clean. +- **Never set `maxWorkers` in `packages/server/api/vitest.config.ts` — the api suites are not alone on the runner.** `ci.yml` runs `turbo run test-ce test-ee test-cloud check-migrations --filter=api` as concurrent turbo tasks, in parallel with a second `turbo run test` line and `web#build`'s vite run, all on one 4-vCPU / 16 GB `ubuntu-latest` box. Each vitest fork boots a Fastify server *and* a PGlite database, so the config's per-project worker count is really that number **times three editions**. A stray `maxWorkers: 4` on [#14777](https://github.com/activepieces/activepieces/pull/14777) raised the forks pool from its default `availableParallelism() - 1` (3) to 4 — 12 forks instead of 9 — and took the job down twice in a row with failures that never named the cause: first `execute-flow-e2e`'s `beforeAll` timing out while **1028 tests passed**, then the whole runner SIGKILLed mid-build (`web#build exited (137)`, `check-migrations` SIGTERM, api suites never finishing). Both read as "a test broke". Neither was. The tell is `The runner has received a shutdown signal` immediately above the 137, and the fix is to delete the line, not to raise a timeout. Same reason the config's `hookTimeout` is 120s: a loaded runner pushes a server boot past a minute, so a file that pins its own smaller `beforeAll` timeout (`}, 30_000)`) silently opts out of the protection and becomes the first thing to fail. Check the blast radius before calling it fixed: a config mistake made on a long-lived stack branch replicates by merge into every descendant, so deleting it from the PR in front of you leaves the rest of the stack to reintroduce it on the next rebase. This one originated once on `feat/subflow-fan-in-barrier` (2026-08-12) and reached **14 branches, 13 of them open PRs** across three unrelated stacks — `git log -S maxWorkers` over `origin/main` proved it never landed there, and a loop of `git show : | grep` found every branch still carrying it. +- **A red `api#test-ce` names one failing test, not all of them — the script runs `--bail 1`.** `test-ce-command` is `vitest run test/integration/ce --bail 1`, so CI stops at the first failure and the report is a floor, not a count. On `feat/barrier-edges` it showed one failing assertion; the file actually had three, all the same root cause. Before concluding a branch has one problem, re-run the suspect file locally *without* `--bail` — `npx vitest run ` — and fix the set. +- **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. - **Docs videos belong on `cdn.activepieces.com/videos/docs/`, never in the repo — and `docs/resources/` is the legacy path that suggests otherwise.** Every `