From 13a83dd68a751fb9e6d4a84570c5de0d782cc4ea Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 21 Oct 2026 15:11:42 -0500 Subject: [PATCH] docs: require sleep-aware background work in reviews (#17138) ## Thinking Path > - Paperclip manages agents in instances that can be idle between tasks. > - Recurring empty database and network checks can keep idle instances and databases awake. > - Worker migrations already use events, transaction-aware signals, and shared deadlines. > - New features can still add idle scans unless contributors and code reviewers share an explicit rule. > - This PR makes that rule discoverable through the repository rule loader and Greptile. > - The rule applies to added or expanded behavior without requiring unrelated legacy polling to be removed. ## Linked Issues or Issue Description Refs #15625, #17000, #17094, #17106, and #17129. **Current behavior** The repository has module ownership and runtime dependency rules, but no explicit review requirement against new idle polling. A new duty in a heartbeat tick can undo the benefits of previous worker migrations. **Proposed behavior** Require events for changed state, transaction-aware signals for database-produced work, and shared scheduling for real deadlines. Reject recurring empty scans, including scans disguised as scheduler callbacks. Review narrow polling exceptions against concrete obligations and sleep impact. **Reason and benefit** Make sleep-aware design a default review expectation. Preserve delivery, ambiguous-commit recovery, drain/resume, and external wake correctness while preventing new permanent idle traffic. Existing unrelated polling stays outside the requested change. ## What Changed - Add rules/sleep-aware-background-work.mdc for server, database, adapter, plugin, and runner background paths, including server/src/index.ts. - Require concrete triggers, pending-work predicates, quiet conditions, actual deadlines, and lifecycle evidence. - Describe narrow exceptions for active operations and external sources, with required scope, stop conditions, and sleep limitations. - Distinguish in-process scheduling from durable Cloud wake coverage. - Register the matching scopes in .greptile/files.json and link the rule from rules/README.md. - Reuse the existing module-boundary rule for ownership instead of duplicating provider policy. ## Verification - Passed: JSON parsing, required rule metadata, exact parity between frontmatter globs and Greptile scopes, and all repository links. - Passed: 15 representative path matches, including server startup, nested service workers, producer routes, tests, database signals, adapters, plugins, and both runner source trees. UI, CLI, docs, and workflow paths remain outside this rule's glob scope. - Passed: git diff --check. - Manually checked contrasting review cases: reject a new empty queue interval; reject an always-rescheduled empty scan; allow an actual retry deadline; allow a bounded active readiness probe with lifecycle cleanup; leave untouched legacy loops alone; reject sleep approval based only on an internal timer. - No runtime code changed. Application builds and tests were not rerun for this rules-only change. This validates rule structure and intended interpretation, not future reviewer compliance or deployed sleep. - Greptile: 5/5 on 0b95bbd38a468e9b0014b906ba73c5a57beb1f91, with no findings or unresolved threads. Full hosted CI passed on the same head: https://github.com/paperclipai/paperclip/actions/runs/39664730299. - An additional audit of existing rule scopes initially assumed all YAML glob strings were quoted. The audit was corrected to accept the existing unquoted form; all rule-to-manifest mappings then passed without changes to those rules. ## Risks - Broad source globs can surface the rule on unrelated edits. Its body limits enforcement to new or materially changed background behavior and explicitly excludes unrelated legacy migration demands. - A blanket ban on every timer would reject legitimate active operations. Narrow exceptions require evidence and do not exempt permanent empty scans. - Greptile and agent rule loading provide review guidance, not a mechanical proof that no polling exists. No regex timer ban or new runtime behavior is introduced. ## Model Used OpenAI GPT-6 through Codex with reasoning, source and session inspection, editing, and structural validation. Exact serving model ID and context-window size are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip --- .greptile/files.json | 13 +++ rules/README.md | 7 ++ rules/sleep-aware-background-work.mdc | 119 ++++++++++++++++++++++++++ 3 files changed, 139 insertions(+) create mode 100644 rules/sleep-aware-background-work.mdc 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.