Skip to content

Fix metadata breaking during overlapping or unparseable adaptor installs - #5077

Open
hirad121 wants to merge 1 commit into
OpenFn:mainfrom
hirad121:fix/5059-adaptor-service-install-race
Open

Fix metadata breaking during overlapping or unparseable adaptor installs#5077
hirad121 wants to merge 1 commit into
OpenFn:mainfrom
hirad121:fix/5059-adaptor-service-install-race

Conversation

@hirad121

Copy link
Copy Markdown

Description

Fixes a race and a raise in AdaptorService that together broke job
metadata (autocomplete/docs in the editor) whenever an adaptor was still
installing, or when its version wasn't valid semver:

  1. Overlapping installs. install/2 stored a placeholder for an
    adaptor (status: :installing, path: nil) before its npm install
    finished, with no lock around the gap. A second lookup for the same
    package during that window found the placeholder instead of waiting
    for the real result — for a pinned version this returned a pathless
    entry MetadataService couldn't use (silently failing metadata, an
    alert but no crash); for an unpinned job the placeholder's stored
    version was the literal word "latest", which isn't valid semver and
    raised when compared against.
  2. latest never got resolved before reaching the adaptor service.
    The editor's metadata channel handler passed job.adaptor straight
    through, unlike the worker run payload (RunWithOptions) and the AI
    assistant, which already resolve latest/local to a concrete
    version via AdaptorRegistry.resolve_adaptor/1 first.

Fix, three parts:

  • AdaptorService is now a GenServer instead of an Agent. An
    overlapping install for the same package/version is queued behind the
    one already in flight (Task.Supervisor.async_nolink, replies
    deferred via GenServer.reply/2) rather than started again or served
    a half-built entry. No placeholder is ever added to the adaptor list
    at all, so a lookup only ever sees a real, present adaptor or nothing.
  • Version comparisons no longer raise on a value that isn't valid
    semver — neither the requested version nor a stored one. An
    unexpected value (local, next, empty) now just fails the lookup
    cleanly instead of crashing it.
  • The editor's request_metadata channel handler resolves latest via
    AdaptorRegistry.resolve_adaptor/1 before calling MetadataService,
    matching the two other call sites that already do this.

Closes #5059

Validation steps

  1. In the editor, open a job whose adaptor isn't yet installed on that
    web pod (or restart the app so nothing is installed) — metadata
    loads normally once the install finishes, no error/alert, even if
    you trigger it from two tabs/jobs at once for the same adaptor.
  2. A job left on the default @openfn/language-common@latest now
    resolves to a concrete version before the adaptor lookup — no raise,
    no alert, real metadata.
  3. AdaptorService.find_adaptor/2/install/2 called with a version
    that isn't valid semver (local, next, empty string) returns a
    clean "not found" instead of raising.

Additional notes for the reviewer

  • 6 files: lib/lightning/adaptor_service.ex (the actual fix),
    lib/lightning_web/channels/workflow_channel.ex (10 lines — resolves
    latest before the lookup), two test files, test/test_helper.exs
    (one Mimic.copy/1 line, needed to stub MetadataService in the new
    channel-level test), CHANGELOG.md.
  • AdaptorService.install/2 has exactly one external caller in the
    whole codebase (MetadataService.get_adaptor_path/1) and
    find_adaptor/2/get_adaptors/1/installed?/2 have none outside the
    module's own tests — checked before changing the module's internals,
    so this is a contained, self-consistent change, not a public-API break.
  • Doesn't touch role-based authorization — nothing in this path is
    gated by project role, only by AdaptorRegistry's allowlist check
    (exists?/2), which is unchanged and still runs first, unconditionally,
    before anything else in install/2.
  • New tests reproduce both failure modes against the pre-fix code first
    (via git stash on just the changed file) to confirm they actually
    detect the bug, not just pass vacuously.
  • Full suite run clean; mix format, mix credo --strict --all,
    mix dialyzer, and mix sobelow all pass with no new findings
    (sobelow's output is byte-identical before/after this diff).

AI Usage

Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):

  • I have used Claude Code
  • I have used another model
  • I have not used AI

You can read more details in our
Responsible AI Policy

Pre-submission checklist

  • I have performed an AI review of my code (we recommend using /review
    with Claude Code)
  • I have implemented and tested all related authorization policies.
    (e.g., :owner, :admin, :editor, :viewer)
  • I have updated the changelog.
  • I have ticked a box in "AI usage" in this PR

AdaptorService stored a placeholder for an adaptor before its npm
install finished, marked :installing with no path. A second lookup
during that window found the placeholder instead of waiting for the
real result: for a pinned version this returned a pathless entry that
MetadataService couldn't use, and for an unpinned job the placeholder's
version was the literal word "latest", which isn't valid semver and
raised when compared.

AdaptorService is now a GenServer instead of an Agent. An overlapping
install for the same package/version is queued behind the one already
running rather than started again or served a half-built entry; no
placeholder is ever added to the adaptor list, so a lookup only ever
sees a real, present adaptor or nothing. Version comparisons no longer
raise on a value that isn't valid semver, whether it's the requested
version or one already stored, so an unexpected value like "local" or
"next" fails the lookup cleanly instead of crashing it.

The editor's metadata channel handler also resolves "latest" to a
concrete version before calling MetadataService, the same way the
worker run payload and AI assistant already do, so the common case of
an unpinned job no longer relies on the safety net at all.

Fixes OpenFn#5059
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: New Issues

Development

Successfully merging this pull request may close these issues.

Metadata breaks while an adaptor is installing, and when its version isn't a real one

1 participant