Skip to content

fix: bring from prod no longer stalls after five quiet minutes - #398

Open
hamzahalq wants to merge 2 commits into
releases/r10.0-developfrom
hamza/fix/adapter-list-speed
Open

hamzahalq wants to merge 2 commits into
releases/r10.0-developfrom
hamza/fix/adapter-list-speed

Conversation

@hamzahalq

Copy link
Copy Markdown
Contributor

Opening the bring-from-prod list took about 80 s whenever nobody had used it for five minutes.

  • Adapter lists read each adapter's metadata 16 at a time, shared across the app, instead of one by one. A cold kind list went from ~34 s to ~3–5 s.
  • An adapter's startup properties are remembered until its package changes, instead of for five minutes. Prod stopped re-starting every adapter to mask secrets on the first subscription read after that.

Tests: two new ones in AdapterCatalogTests, each failing on the old code, plus the adapter listing, describing and secret-masking suites.

@coderabbitai

coderabbitai Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

📝 Summary
  • What changed: Adapter kind listing now reads metadata for distinct adapters concurrently, with a process-wide limit of 16 reads. Adapter startup properties are cached by package hash and shared across concurrent requests. A changed package triggers a new description; unused entries expire after one day.
  • Risk: risk:medium. The changes affect adapter discovery, metadata caching, and adapter startup coordination.
  • Security-sensitive areas: Startup property descriptions include secret properties. This change affects description refresh timing, but the supplied code does not show a change to secret masking. Confirm package hash updates are visible before cached descriptions are reused.
  • Test coverage impact: Two tests were added. One checks that an adapter is not restarted after 30 minutes but is restarted after a new build. The other uses delayed storage reads to verify concurrent metadata access. Test execution results are not supplied.
  • Operational concerns: No migration is indicated. Direct-to-storage updates may not be detected until installer metadata refreshes; the caveat reports up to five minutes for packages without a manifest. No data rollback is indicated.

Adapter listings now read adapter metadata concurrently with a process-wide limit and identify adapters by normalized IDs. Adapter descriptions now use package build hashes to validate cached answers and coalesce concurrent asks. Integration tests cover both paths, and the caveat documentation describes the resulting cache behavior.

Changes: Adapter Catalog

Layer / File(s) Summary
Bounded adapter metadata listing
SW.Bitween.Api/Resources/Adapters/AdapterListing.cs, SW.Bitween.IntegrationTests/Tests/AdapterCatalogTests.cs
Listing filters nonempty, non-convention files, normalizes and deduplicates adapter IDs, then reads declared kinds in parallel under a 16-read process-wide limit. The integration test checks that delayed metadata reads overlap.
Build-aware adapter descriptions
SW.Bitween.Api/Services/ServerlessAdapterDescriber.cs, SW.Bitween.IntegrationTests/Tests/AdapterCatalogTests.cs, docs/caveats.md
Description caching now checks the package build hash, keys concurrent asks by adapter ID and hash, and stores successful answers with a one-day sliding expiration. The integration test checks reuse within a build and a new start after a build change. The caveat describes the updated cache behavior.

Priority: Normal

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Possibly related PRs

  • simplify9/Bitween-api#396: Both changes optimize adapter catalog work in mirror operations, at different points in the flow.
  • simplify9/Bitween-api#393: Both changes update the adapter catalog path and its adapter startup metadata handling.
  • simplify9/Bitween-api#392: This PR changes adapter listing and description implementations used by the mirror workflows introduced in PR #392.

Suggested labels: security, risk:high

Suggested reviewers: mmalkhatib, samerzughul

Merge Risk: Minimal · up to 1c91b

No demonstrated issue currently blocks merging. The version-only package concern remains unverified.

Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta… Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly identifies the production delay after five quiet minutes and matches the primary changes.
Description check Passed The description directly explains the adapter-list concurrency and startup-property caching changes, their performance impact, and the tests added.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 7 functions across 3 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔇 Additional comments: 1
SW.Bitween.Api/Resources/Adapters/AdapterListing.cs-186-188: 1

186-188: Major · Functional Correctness · Quick win

⚠️ Unverified finding
Verification ran but could not confirm this finding. It is shown for review, not as a verified issue.

Read metadata from a version-only package when no current package exists.

If an older installer published only adapters/{id}/{version}, AdapterIdOf removes the version before DeclaredKindsAsync calls GetMetadataAsync. For an adapter outside the infolink6.{prefix} convention, the missing current package then makes its declared kind unreadable. List("handlers") omits that adapter even when the versioned package declares Kind=handler. Retain a versioned package key as a metadata fallback when no current package exists.


Review info
Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Central YAML; Path: .coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 5d1fa6cf-686c-419c-a38e-105755253330
Commits

Reviewing files that changed from the base of the PR and between 7c3182b and 1c91b72.

Files selected for processing: 4
  • SW.Bitween.Api/Resources/Adapters/AdapterListing.cs
  • SW.Bitween.Api/Services/ServerlessAdapterDescriber.cs
  • SW.Bitween.IntegrationTests/Tests/AdapterCatalogTests.cs
  • docs/caveats.md

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant