diff --git a/.greptile/files.json b/.greptile/files.json index e7ff68bfd24..1595fa1bf81 100644 --- a/.greptile/files.json +++ b/.greptile/files.json @@ -58,6 +58,19 @@ "packages/db/package.json", "packages/db/drizzle.config.ts" ] + }, + { + "path": "rules/sleep-aware-background-work.mdc", + "description": "Prevent new idle polling; require event-driven producers, real scheduler deadlines, obligation-based recovery, and lifecycle verification without forcing unrelated legacy migrations.", + "scope": [ + "server/src/**", + "packages/db/src/**", + "packages/adapter-utils/src/**", + "packages/adapters/*/src/**", + "packages/plugins/*/src/**", + "packages/paperclip-runner/src/**", + "packages/paperclip-runner/runner/**" + ] } ] } diff --git a/rules/README.md b/rules/README.md index 4fa6390aa9e..5746e5881e5 100644 --- a/rules/README.md +++ b/rules/README.md @@ -34,3 +34,10 @@ real decision. This applies to orchestration as well as UI. See the [Greptile file-reference documentation](https://www.greptile.com/docs/code-review/greptile-config-reference) for the manifest format. + +Background execution is covered by +[`sleep-aware-background-work.mdc`](sleep-aware-background-work.mdc): new or +expanded work must become quiet, use events and real scheduler deadlines, and +preserve recovery across transactions, drain, and restart. This rule covers +`server/src/index.ts` and the worker/producer paths in its scope. It does not +require unrelated legacy polling to be migrated in the same PR. diff --git a/rules/sleep-aware-background-work.mdc b/rules/sleep-aware-background-work.mdc new file mode 100644 index 00000000000..07316a53dd4 --- /dev/null +++ b/rules/sleep-aware-background-work.mdc @@ -0,0 +1,119 @@ +--- +description: Prevent new idle polling; require event-driven work, shared deadlines, and recoverable sleep-safe lifecycle behavior. +globs: "server/src/**,packages/db/src/**,packages/adapter-utils/src/**,packages/adapters/*/src/**,packages/plugins/*/src/**,packages/paperclip-runner/src/**,packages/paperclip-runner/runner/**" +alwaysApply: false +--- + +# Background work must become idle + +Apply to new or materially changed background work, its producers, and its +startup/lifecycle wiring. This includes `server/src/index.ts`, service workers, +DB connection management, adapters, and plugin or runner background activity. + +This is a prospective review requirement, not a demand to migrate every existing +loop. Do not block unrelated edits on untouched polling. Adding a duty to an +existing heartbeat or sweep, extending its lifetime, or copying its polling +pattern is new behavior and must meet this rule. An unchanged adjacent loop is +not precedent or permission. + +## Choose the trigger before the mechanism + +An idle subsystem must not repeatedly query the database or contact another +service merely to discover that it is still idle. Ask: if nothing changes, is +there a real obligation to act at a future time? + +| Obligation | Required approach | +| --- | --- | +| Local state changes or an operation completes | Direct call, event, promise, or completion callback | +| A database write creates work | Transaction-aware notification from the supported producer | +| A routine, expiry, or retry is due later | Register its actual next deadline with the shared scheduler | +| Delivery, cleanup, a claim, or a write remains unresolved | Recovery tied to that concrete obligation and its resolution | +| Work must survive restart | Reconstruct from the existing durable source of truth at startup/resume | + +Split mixed duties. Deliver on an event; schedule a retry only after failure or +while an obligation remains. Do not add `setInterval`, recursive timeouts, sleep +loops, or heartbeat callbacks that scan empty work indefinitely. Putting the +same scan behind the scheduler is still polling. A larger interval or `.unref()` +does not establish idle behavior. Configuration or retained completed history +alone must not keep an otherwise idle worker scanning. + +## Reuse the shared lifecycle + +- Server workers use [delivery-work-coordinator.ts](../server/src/services/delivery-work-coordinator.ts) + and its [work-scheduler.ts](../server/src/services/work-scheduler.ts) for + coalescing, non-overlapping execution, retries, and named deadlines. Do not + create a per-feature scheduler or independent maintenance timer in `index.ts`. + Adapter, plugin, and runner work uses the owning host's lifecycle interface; + extend that boundary when needed instead of importing server internals. +- Register, replace, and cancel the actual next deadline. No pending work, + unresolved write, or future obligation means no scheduled scan. An unconditional + `nextRunAt = now + interval` is not a deadline unless a real obligation requires it. +- Existing rows remain durable truth; notifications are hints to inspect them. + Do not add a table per timer or thread recovery bookkeeping through callers. + Keep work predicates and provider behavior in their owning services, consistent + with [module-boundaries.mdc](module-boundaries.mdc). +- Use [notifyDeliveryWork](../server/src/services/delivery-work-notifications.ts) + and [work-signals.ts](../packages/db/src/work-signals.ts) for database-produced + work. Await intent registration before the relevant write in its transaction. + A nested savepoint release is not an outer commit. Preserve uncertainty until + the transaction protocol establishes settlement; one empty query or elapsed + timeout cannot prove an uncertain write will not commit later. +- Account for every supported producer. Process-local notifications do not cover + other replicas, independent SQL writers, or external events without ingress. + Do not remove their only recovery path without replacing that contract. +- Use the shared [database client](../packages/db/src/client.ts) for idle + connection release and reconnect. Do not add permanent DB keepalives/listeners + or scattered connect/disconnect wrappers as a substitute for a wake contract. + +## Preserve obligations through drain and sleep + +A wake is consumed by successful reconciliation, not notification receipt or a +skipped run. Preserve changes arriving during a sweep, a first write during +drain, and failed work whose retry enters drain. Passive `hasPending` checks +must not reintroduce recurring SQL while `run` is paused. Resume must still +process the retained work. Stop subscriptions/timers and cancel or await active +work on shutdown. + +The in-process scheduler cannot wake stopped compute. `nextWakeAt()` covers +registered tasks; it is not proof that Cloud has persisted an alarm or that all +work is represented. Before relaxing a sleep blocker, verify the exact durable +wake and admission contract in [idle-sleep-safety.md](../doc/idle-sleep-safety.md). +Preserve unsupported obligations as blockers. Do not change a business deadline +merely because it was inspected, or discard unfinished work to claim quiet. + +## Narrow exceptions, with evidence + +A bounded readiness probe for an active operation, protocol-required renewal +for a live resource, or reconciliation of an external source with no usable +notification/replay mechanism can need polling. The PR must identify why an +event or deadline is insufficient, the active scope, cadence/backoff, stop +condition, and sleep impact. External-source reconciliation must stop when +unconfigured/disabled; being configured is not by itself an exemption. If its +contract requires activity while otherwise idle, disclose the sleep limitation +and preserve the corresponding blocker until a supported external wake exists. +Required periodic reconciliation still uses the shared scheduler; this exception +does not justify a new independent tenant maintenance interval. + +Timers inside the scheduler, timeouts for active requests, and an in-memory +empty-buffer check are not automatically sleep blockers. Assess actual SQL, +network, CPU, and connection effects; do not flag every timer mechanically. +These exceptions do not permit a permanent "just in case" recovery scan. + +## Review and verification + +Require changes to state the trigger, source of truth, pending-work predicate, +next deadline/retry, and condition that makes the worker quiet. Block added or +expanded idle scans without the narrow exception evidence above. Name the +specific recurring effect and the owning event/deadline alternative in feedback. + +For changed behavior, verify no recurring SQL/network calls after startup and +settlement in applicable states: no configuration, configured but idle, and +retained completed history. Prove the first real producer reactivates it. Cover +relevant transaction rollback/outer commit, restart reconstruction, retry, +drain/resume, concurrent wakes, and shutdown boundaries. Select tests for the +changed risks; do not require an unrelated fleet-wide migration or every matrix +case for a small change. Timer-call assertions alone do not prove quiet. + +Worker quiet, DB disconnect, and actual host suspension are separate claims. +Do not claim deployed sleep, successful external wake, or cost savings from +unit tests or an internal deadline alone.