2 Commits

Include renames

Author	SHA1	Message	Date
DottaandPaperclip	a92f616cd0	fix(db): attribute bounded captures to execution owners (#18634) ... ## Thinking Path > - Paperclip manages agent work through shared server and database services. > - High query volume can spend main-thread time on SQL construction and result decoding. > - The existing bounded response capture counts rows and bytes, but it cannot name the operation that executed a query. > - Bundled callers can be missing, and measuring every parsed JSON value adds work to the diagnostic. > - This pull request carries a semantic execution owner into each measured query and bounds response sizing. > - Operators can rank query volume by owner before choosing a performance fix. ## Linked Issues or Issue Description Refs #17504, #17522, #17537, #17564. **What existing behavior does this improve?** The opt-in `PAPERCLIP_DB_RESPONSE_CAPTURE_WINDOW` diagnostic. **Subsystem affected** Database diagnostics and existing server execution boundaries. **Current behavior** The capture groups by table, operation, and builder caller. It cannot distinguish execution owners that reuse a prepared query. Bundled deployments need source maps for caller locations. The capture visits every returned value and serializes parsed JSON again to estimate its size. **Proposed behavior** Version 2 captures use fixed semantic owners for HTTP requests, delivery queues, execution phases, and budget work. They count completed calls, returned rows, errors, and completion wall time. They sample successful results 1, 17, 33, and so on, with fixed cell and string/byte limits. Complex values remain explicitly unmeasured. Semantic ownership works without source maps. **Reason and benefit** A short census can identify which operations issue the most queries or return the most rows. It preserves each query's execution owner through pooled connections, concurrent operations, and prepared-query reuse. It reduces diagnostic work without claiming per-query CPU attribution. **Breaking changes** Opt-in capture output moves from `entry.v1`/`summary.v1` to `entry.v2`/`summary.v2`. Sampled primitive byte lower bounds replace all-result byte estimates. Consumers must use the new fields and must not extrapolate them into a total. There is no database migration or application API change. ## What Changed - Added `withDatabaseOperation(owner, action)` to the DB package. The server owns semantic labels. Queries retain execution ownership in their completion closure. - Added fixed owners at existing HTTP, delivery, budget, and execution timing boundaries. Delivery work replaces the context of the producer that woke it. - Added complete call/row/error counters and completion wall durations within the existing bounded window. Durations include waits and are not CPU measurements. - Sampled and bounded primitive-value sizing. Complex and oversized values remain unmeasured. Known owners skip repeated stack extraction. - Preserved the deadline, group limit, paced output, query outcomes, and existing coverage exclusions. Updated the operator documentation. - Added real PostgreSQL owner-isolation and prepared-query tests, server scope tests, and plain bundled-runtime proof. - Moved projection-test and benchmark full-response sizing into an explicit test helper. Existing payload-size thresholds remain unchanged; these synthetic checks do not use the production sampler. ## Verification - Final-commit DB capture suites: 43 tests passed in 13.86 seconds against the actual PostgreSQL driver, including concurrent owners, prepared reuse, transaction failure, a dedicated client, sampling, and a query crossing the deadline. DB typecheck passed. - Server attribution and existing delivery/execution/budget suites: 53 tests passed. Final concurrent HTTP GET/JSON POST scope refinement: 5 tests passed. - Runtime bundle suite: 8 existing tests passed. The new no-source-map owner test passed after its fixture was corrected to use a driver thenable. An ordinary native Promise can bypass an overridden `.then`; the real PostgreSQL tests separately cover driver integration. - Observer-only experiment: three alternating pairs per case on Node 24.22.2, macOS arm64, 3,000 calls per run after warm-up. For an already-materialized synthetic JSON result, median capture CPU fell from 82.37 to 1.87 microseconds/call. For a 100-row primitive result, it fell from 8.10 to 4.40 microseconds/call. Every result retained identity and all calls were counted. The new diagnostic intentionally measures less byte data; these are observer costs, not database latency or a hosted application speedup. Disabled-case timings were below one microsecond/call and too small for a performance conclusion. - Final lifecycle regression: 28 unit tests and DB typecheck passed. Closing the last capture disables and replaces owner storage; overlapping captures retain it. - Final-commit full recursive typecheck passed in 38.38 seconds and full build passed in 40.81 seconds. The build stamp matches `83ed4ba9cebd68d48929c46f15ab32d5c0b9687b`. Focused server attribution tests also passed 5/5 before the test-helper-only migration. Full test acceptance will use the repository's parallel CI gates; the unsharded local command takes hours. - Verification failures retained: initial sandbox runs could not open local IPC/PostgreSQL listeners; permitted reruns exercised the real services. One old test expected JSON serialization and was updated to assert explicit unmeasured complex values. The first bundle fixture used the wrong promise boundary, as described above. A retained-callback regression also demonstrated that disabling and reusing the same AsyncLocalStorage instance could resurrect an old owner; replacing the instance after the last capture closes fixes that case. - The first CI run found older projection tests consuming the previous full-response fields. Those consumers now use the full-response test helper, preserving their byte thresholds. The two complete affected route suites plus helper tests passed 115/115 in 73.08 seconds. A temporary full-row query negative control then failed the unchanged 2,048-byte limit at 5,000,812 bytes and was restored; five other service suites passed 34/34 and the affected chat cases passed 3/3. All three previously failing CI shards passed on the corrected head. A local skill-list benchmark smoke check completed 255 captured queries with no in-flight queries or overflow; this was integration verification under concurrent test load, not a timing benchmark. - Two final-head runner jobs lost communication with GitHub before reporting a test result. One targeted retry of those jobs and their dependent aggregate passed. All 53 final-head checks passed, with 3 intentional skips; previously passing suites were preserved. - Hosted capture remains pending until the reviewed revision is deployed and a bounded window is explicitly configured. Reproduce the focused checks with Node 24 and the repository pnpm version: ```sh pnpm exec vitest run packages/db/src/response-capture.test.ts packages/db/src/response-capture-postgres.test.ts pnpm --dir server exec vitest run --config vitest.config.ts src/__tests__/database-operation-attribution.test.ts scripts/bundle-runtime.test.mjs pnpm -r typecheck pnpm build ``` ## Risks - Owner context is active while a valid future capture window is armed, up to 30 minutes before accounting starts. No-window and expired calls take the direct path. Diagnostics still add bounded work while enabled. - A nearest nested owner is reported. Broad `http.request` and unattributed groups remain visible; this does not identify every service callsite or SQL shape. - The existing census excludes tagged/raw explicit-execute/streaming paths and internal protocol commands. Its counts are not PostgreSQL transaction totals. - Completion wall time includes queue/database/network/scheduling waits. Concurrent durations overlap. Sampled bytes exclude complex values, large strings, and values beyond the sizing limits. - Activation is startup-only. Operators must choose the window after artifact preparation, allow rollout/warm-up time, and remove the expired setting afterward. ## Model Used OpenAI Codex, GPT-6 family, with code execution, repository inspection, and parallel agents. The exact deployment 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 <noreply@paperclip.ing>	2026-11-02 12:12:21 -06:00
DottaandPaperclip	c08da3e0a7	perf(server): narrow skill list reconciliation reads (#18023) ... ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The skill library lists current company skills and their agent attachments. > - Each list request also reconciles bundled skills, folders, and local files. > - These reads return full agent records and large metadata that their callers do not use. > - Conditional requests do this work before they return an unchanged response. > - This pull request selects the fields each caller needs and avoids unused folder lookups. > - The result is less database transfer and fewer round trips while reconciliation stays current. ## Linked Issues or Issue Description Refs #17941. That change reuses bundled categories within an import. This change removes other repeated work in the same list path. **What happened?** Warm skill-list requests still made 119 SQL calls in a local HTTP fixture. The folder pass returned full skill metadata. The local-source pass also returned remote skills. Attachment counts loaded full agent configurations and monthly spend. Bundled reconciliation resolved folder paths that its release check did not use. **Expected behavior** The list must preserve its current result, attachment counts, access rules, local-file reconciliation, and bundled release checks. Internal reads should return only the data their callers need. **Steps to reproduce** 1. Use a company with many remote skills and agents with large saved configurations. 2. Read its skill list, then repeat the read with its ETag. 3. Measure the queries and response values before the server returns `200` or `304`. 4. Run `pnpm --filter @paperclipai/server exec tsx scripts/benchmark-skill-list.ts --samples=5 --network-delay-ms=3.5` for a synthetic reproduction. **Paperclip version or commit** Baseline service: `e1ba77f06771a27114b3999004b0ca5e3c172dd4`. The branch is based on current master. The paired mode changes only the skill-service module; both versions use the same imported helpers. **Deployment mode** Built from source. Local HTTP server and disposable PostgreSQL 17. No hosted deployment is part of this PR's local proof. ## What Changed - Read only the desired-skill selections of nonterminated company agents for attachment counts. Resolve each agent's references once against the full visible skill list. Preserve IDs, keys, aliases, duplicate handling, and ambiguous slugs. - Read only `sourceKind` from metadata for bundled folder classification. - Limit local-source reconciliation to local-path rows. - Skip folder-path enrichment only for bundled reconciliation. Normal imports and list responses keep their folder paths. - Add real-database byte regressions and a local HTTP benchmark. The benchmark can alternate the old and new services against one database, actor, and process. It checks complete response equality and conditional responses. The company-skill service owns these reads. The query predicates and freshness checks remain at their existing owners. This change adds no cache, schema change, or new background work. ## Verification - The attachment byte regression failed before the fix: 601,418 captured response-value bytes exceeded its 2,000-byte bound. The same case passed after the fix. It checks fresh configuration edits, IDs and keys, duplicate aliases, ambiguous slugs, paused agents, terminated agents, and company scope. - 111 focused service and authorization tests passed against isolated real PostgreSQL 17 databases. They include full result equality, retained stored Markdown and metadata, local-file updates, release checks, project visibility, and import authorization. - The default embedded PostgreSQL probe skipped the first local attempt on this Mac. The completed run used a local-only bootstrap adapter for disposable databases. Queries, migrations, services, authorization, and filesystem work used their real implementations. The adapter is not part of this PR. - In repeated synthetic HTTP byte runs, calls fell from 119 to 98 per request. Result-value bytes fell from about 7.75 MB to 1.64 MB per request, about 79%. These values are measured before Drizzle decoding. They exclude protocol framing and do not measure provider billing. - A separate-process run without the byte observer did not show a latency gain. The paired experiment removes that comparison gap: it alternates both services against the same database and checks identical complete responses. On final commit `3076fe1402085f7286ff58e642d7caca892a5f80`, five observer-free pairs had median times of 2.582 s before and 1.850 s after (28.3% lower). The candidate was faster in all five pairs. Its range was 1.802–2.199 s; the baseline range was 2.327–2.725 s. Every normal response had the same complete body hash, and both versions returned the expected unchanged conditional responses. These are local service/HTTP timings with synthetic data and 3.5 ms delay per network direction. They are not hosted acceptance. - Final-head workspace typecheck and build passed. The full local test command did not complete because embedded PostgreSQL bootstrap failed on this Mac; I stopped that owned runner after 16 native-runtime cases failed. All 16 cases then passed when rerun alone against disposable PostgreSQL 17 using their existing external-database path. The 111 focused PostgreSQL 17 tests above also completed successfully. This does not constitute a completed full local suite; all current-head remote CI gates passed (55 successful checks and 2 skipped). Module-boundary and whitespace checks pass. An initial nested package command used the global pnpm; subsequent commands were pinned to pnpm 9.15.4. - The first canary dry run failed while installing an unrelated standalone package: npm returned 404 for the newly published Rollup 4.66.0 tarball. The exact download subsequently returned HTTP 200. The one retry passed without code changes. - Benchmark commands: add `--capture-values=false` for observer-free timings. Add `--baseline-ref=e1ba77f06771a27114b3999004b0ca5e3c172dd4 --capture-values=false` for paired service acceptance. The normal mode uses the production router. The paired mode uses equivalent default-list HTTP handlers and a synthetic authenticated board actor; it does not test login. ## Risks The SQL selection must stay in sync if the desired-skill format or folder classifier needs new fields. Existing parsing and reference resolution still interpret the selected data. The synthetic fixture has large unused payloads. It proves the mechanism and preserves required content. Its percentage is not a projection of whole-instance transfer or billed savings. Hosted latency still needs verification after merge and deployment. ## Model Used OpenAI GPT-6 through Codex, with repository inspection, shell execution, real PostgreSQL, and local HTTP testing. The host does not expose a precise backend revision or context-window size. ## 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 <noreply@paperclip.ing>	2026-10-28 09:07:17 -05:00