You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
fix: report a namespace rename on the registry changes feed - #2246
Renaming a namespace (ChangeNamespaceService) moves every extension's versions to the new namespace row in place, without appending anything to extension_version_change. The feed keeps reporting the old (namespace, extension, version) tuple as ACTIVE forever, and the new tuple never appears until an unrelated transition happens to touch it.
ChangeNamespaceService.changeNamespaceInDatabase now records a REMOVED entry for the old tuple of every version the feed already reported, and an ACTIVE entry for the new tuple of every version that's still active, both at the instant the rename happens.
RepositoryService.wasReportedAsAvailable(ExtensionVersion) is promoted out of a private ExtensionService helper so both call sites (delete/purge and now rename) share it.
V1_76__Repair_Renamed_Namespace_Version_Changes.sql backfills the same two entries for versions that were already renamed before this fix ships. changed_at is stamped with the migration's own run time rather than a reconstructed rename time, since no rename history is kept and a backdated entry would sort into a part of the feed a caught-up consumer has already read past.
ChangeNamespaceServiceTest (new): active version reports REMOVED then ACTIVE in order; a version never reported by the feed isn't withdrawn; an inactive version isn't announced under the new namespace.
ExtensionServiceTest updated for the wasReportedAsAvailable call boundary move.
This test does not verify the tuple snapshots that are the core of the regression. Because RepositoryService is mocked, both calls receive the same mutable extVersion; inOrder proves only REMOVED-before-ACTIVE, and the test would still pass if the namespace were changed before both calls, causing both entries to contain new. Capture the namespace value at each invocation (for example with doAnswer) or use the real repository service, then assert old/REMOVED followed by new/ACTIVE.
Addressed the "Test fails to verify namespace snapshots at each invocation" finding from the latest review (flagged under "Previously missed", no standalone thread to reply on): ChangeNamespaceServiceTest's renamingReportsAnActiveVersionAsMovedToTheNewNamespace only asserted call order, not the namespace value in effect at each call - since recordExtensionVersionChange takes no namespace argument (the real implementation reads it from the mutable Extension object at call time), an order-only assertion couldn't distinguish "REMOVED then ACTIVE, correctly against old-then-new" from "REMOVED then ACTIVE, both already against new".
Fixed in 9ba2338 with a doAnswer that captures extVersion.getExtension().getNamespace().getName() at each invocation and asserts ["old", "new"]. Verified locally that this actually catches the regression by temporarily swapping changeExtensionNamespace before recordNamespaceDeparture: the new assertion fails while the order-only one still passed.
Moved the ALTER SEQUENCE fix into #2246's own V1_77 (same table, same
migration file is the better fit anyway) rather than shipping a
second, colliding V1_77 here - Flyway can't apply two migrations at
the same version once both PRs land.
import-db-dump.sh: pg_get_serial_sequence also returns NULL for
extension_version_change on any target schema before that ownership
fix has actually run - notably a fresh target, which this script
deliberately migrates only to the dump's own Flyway version, not the
latest one. Added a fallback to the table's own <table>_seq naming
convention (every table here follows it once it has a sequence) so
the reset works regardless of merge order or which schema version the
target ends up on.
export-db-dump.sh: the two-rename publish wasn't atomic as a pair -
if the second mv failed after the first succeeded, the exit trap
deleted the old generation it had just moved aside instead of
restoring it, leaving no usable output directory at all. Track
whether publication completed and have cleanup restore the previous
directory when it didn't. Verified all three paths (failure before
any rename, failure between the two renames, success) directly:
failure now always leaves a usable directory with the right contents,
success leaves no leftovers.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Folded an additional fix into this PR's V1_77 migration: `extension_version_change_seq` (introduced earlier in this same migration's history, V1_71) was created without `OWNED BY`, unlike every other entity's sequence (V1_40 explicitly linked all of those). Without it, `pg_get_serial_sequence('extension_version_change', 'id')` returns NULL, which silently defeats tooling that relies on it to find the table's sequence - discovered while testing #2258's db-dump export/import scripts against a production copy. Added `ALTER SEQUENCE extension_version_change_seq OWNED BY public.extension_version_change.id;` to the top of this migration rather than shipping it as a separate one in #2258, which would have collided on the same V1_77 version number once both land.
…ump (#2258)
* chore: add export-db-dump.sh, cover extension_version_change in the dump
scrub-db-dump.sh and import-db-dump.sh already had a well-defined,
matched per-table dump format, but nothing in the repo produced that
dump from a live source in the first place - export-db-dump.sh fills
that gap, taking every table in one transaction/snapshot so concurrent
writes on the source can't leave the tables inconsistent with each
other.
extension_version_change (added by V1_71) was missing from both the
export and the existing import-db-dump.sh entirely, since it predates
that table. Without it, a DB copy built via this pipeline can't
exercise anything version-changes-feed related - the table is simply
empty regardless of what's actually in the app. Added it to both
scripts' table lists; import-db-dump.sh's delete/import ordering,
column adaptation and id-sequence fixup are all already driven
generically off that list, so no other logic changes were needed
there.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: address review findings on the db-dump scripts
- export-db-dump.sh: check for psql on PATH up front, matching
scrub-db-dump.sh/import-db-dump.sh, instead of failing on the last
line with an empty output directory left behind.
- export-db-dump.sh: set a restrictive umask before creating anything.
The raw dump holds personal_access_token.value,
signature_key_pair.private_key and user_data.eclipse_token until
scrub-db-dump.sh redacts them, so it shouldn't inherit whatever
umask the caller happens to have.
- export-db-dump.sh: stage \copy output next to the target directory
and only move files into place once psql has committed. \copy
writes its file directly, independent of the transaction, so a
failure partway through previously left the target with a mix of
freshly-overwritten and stale files that still passed
import-db-dump.sh's existence checks.
- import-db-dump.sh: handle a table with zero dump rows during Flyway
version auto-detection instead of crashing. Verified the crash first
(an empty CSV makes counts.most_common(1)[0] raise IndexError, which
set -e propagates through the array assignment and kills the
script), then confirmed the fix handles it: an empty dump file is
now treated as carrying no signal for that table rather than
vetoing every candidate version, and detection is refused with a
clear message only if every table is empty.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: reject too-old versions for an empty table, anchor default dump path
import-db-dump.sh: an empty dump file for a table was skipped from
version matching entirely, not just its exact column count - so a
version from before that table existed could still "match" on the
strength of every other table alone (concretely: an empty
extension_version_change.csv could select a pre-V1_71 schema). The
file existing at all means the table existed at export time, so a
candidate version where it has zero columns (doesn't exist yet) is
now rejected outright; only the exact-count comparison is skipped
when there's no row signal for it. Verified both branches directly
against a table_column_count stub.
export-db-dump.sh: the default output directory was relative to the
caller's cwd, unlike scrub-db-dump.sh/import-db-dump.sh's own db/dump
default, which both resolve against the repo root. Running this
script by path from elsewhere silently wrote the dump somewhere
neither of those two scripts would look for it. Anchored the default
to the script's own project root the same way.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: atomic dump publication, fix extension_version_change's sequence ownership
export-db-dump.sh: publish the staged dump as two directory renames
(old -> .previous, staged -> live) instead of moving each table's
file into place individually. A crash or a failed move partway
through the old per-file loop could still leave the target with some
tables on the new generation and some on the old one - exactly the
mixed state staging was introduced to prevent in the first place.
Verified both the success and failure paths directly: on failure the
target is left completely untouched, on success no staging/backup
directories are left behind.
V1_77__ExtensionVersionChange_Sequence_Ownership.sql: fixes a real,
independently-verified server-side gap this PR's own testing exposed.
extension_version_change_seq (V1_71) was created without OWNED BY,
unlike every other entity's sequence - confirmed by checking
V1_40__Entity_Sequencies.sql, which explicitly added OWNED BY for
every one of import-db-dump.sh's original 8 tables. Without it,
pg_get_serial_sequence('extension_version_change', 'id') returns
NULL, so import-db-dump.sh's existing generic per-table sequence
reset silently skips this table - reproducing the exact duplicate-key
crash hit earlier when this table's sequence wasn't reset by hand,
except now happening through the "fixed" script itself.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: merge sequence-ownership migration into #2246, harden both scripts
Moved the ALTER SEQUENCE fix into #2246's own V1_77 (same table, same
migration file is the better fit anyway) rather than shipping a
second, colliding V1_77 here - Flyway can't apply two migrations at
the same version once both PRs land.
import-db-dump.sh: pg_get_serial_sequence also returns NULL for
extension_version_change on any target schema before that ownership
fix has actually run - notably a fresh target, which this script
deliberately migrates only to the dump's own Flyway version, not the
latest one. Added a fallback to the table's own <table>_seq naming
convention (every table here follows it once it has a sequence) so
the reset works regardless of merge order or which schema version the
target ends up on.
export-db-dump.sh: the two-rename publish wasn't atomic as a pair -
if the second mv failed after the first succeeded, the exit trap
deleted the old generation it had just moved aside instead of
restoring it, leaving no usable output directory at all. Track
whether publication completed and have cleanup restore the previous
directory when it didn't. Verified all three paths (failure before
any rename, failure between the two renames, success) directly:
failure now always leaves a usable directory with the right contents,
success leaves no leftovers.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: resolve symlinked output dirs, run psql with -X
pwd (without -P) preserves a logical symlink path, so when the output
directory is a symlink the rename-based publish renamed the symlink
itself rather than the real directory it points at - the actual
target directory was silently left on the old generation forever,
and a plain directory took the symlink's place. Reproduced directly
(a symlinked "dump-link" ended up replaced by a real directory, with
the real target directory it used to point at orphaned and
untouched), then confirmed pwd -P fixes it: the symlink stays a
symlink and correctly resolves to the new generation.
Also run psql with -X, matching import-db-dump.sh's own target_psql -
without it, a ~/.psqlrc loads after -v ON_ERROR_STOP=1 and could
override it, letting a broken export get published as if it were
complete.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* fix: never touch files this script doesn't own, escape-proof \copy paths
Publishing by renaming the whole output directory moved everything in
it - not just the 9 generated CSVs - into the backup path the exit
trap then deleted. scrub-db-dump.sh deliberately leaves *.csv.bak
originals alongside the redacted files until they've been reviewed;
a whole-directory swap silently destroyed those (and anything else
already in the directory) the moment this script is re-run to refresh
a dump. Reverted to promoting each of the 9 known files individually,
which can only ever touch filenames this script itself owns. Verified
directly with a psql stub: a pre-existing .csv.bak and an unrelated
file both survive an export run untouched, while all 9 real tables
refresh.
Also stopped embedding STAGE_DIR's full path inside the generated
\copy command's single-quoted argument - that path comes from OUT_DIR
(derived from the repo checkout path or a caller-supplied directory),
and psql has no way to escape a literal "'" in it, so a path
containing one broke the quoting outright. Reproduced this directly
(a literal apostrophe in the path corrupts the generated SQL), then
fixed it by cd-ing into the staging directory and \copy-ing to bare
relative filenames instead, which can never contain a quote since
they're fixed table names under this script's own control. Verified
the generated SQL never contains the path at all, and a full export
run into an apostrophe-containing directory succeeds end to end.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* docs: call out the non-atomic per-file publish as an accepted tradeoff
The comment explained why per-file promotion was chosen over a
directory swap, but not what it gives up: a failure partway through
can leave a mixed generation. Spelled that out next to the loop
itself, not just in the PR description.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Renaming a namespace moved every extension's versions to the new
namespace row in place, without appending anything to
extension_version_change. The feed kept reporting the old
(namespace, extension, version) tuple as ACTIVE forever, and the new
tuple never appeared until an unrelated transition happened to touch
it (#2244).
ChangeNamespaceService now records a REMOVED entry for the old tuple
of every version the feed already knew about, and an ACTIVE entry for
the new tuple of every version still active, both at the same instant
the rename happens. wasReportedAsAvailable moves from ExtensionService
to RepositoryService so both call sites share it.
A new migration backfills the same two entries for versions that were
already renamed before this fix, stamping changed_at with the
migration's own run time rather than a reconstructed rename time,
since none is known and a backdated entry would sort into a part of
the feed a caught-up consumer has already read past.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A version could pick up a further, correctly-recorded transition
under its new namespace after a silent rename (e.g. an admin
deactivation, which always reads the version's current namespace),
without that ever withdrawing the stale tuple the rename abandoned.
Collapsing to one row per version, as before, hid that stale tuple:
its single latest entry already matched the current namespace, so the
old (still not-REMOVED) namespace was never even considered.
Track history per (version, namespace) for the departure side instead,
so an abandoned namespace's own latest state is checked independently
of what happened later under a different one. The arrival side keeps
using the version's single overall-latest entry, which is still the
right question for "has the feed caught up to where this version
lives now".
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
main added V1_76__Unique_Active_Review.sql after this branch was cut,
colliding with this PR's migration version - renamed the migration to
V1_77.
More importantly: once a version's changes-feed history can span more
than one namespace (a rename backfill, or two renames over its life),
the single most recent entry for a version is no longer necessarily
the one that reflects its current namespace - an entry appended later
for an abandoned namespace (e.g. this PR's own backfill, timestamped
with the migration's run time) can outrank an earlier, correctly
recorded entry under the namespace the version actually lives in now.
wasReportedAsAvailable, read that way, would then answer for the wrong
tuple and skip or duplicate a withdrawal on a later delete or rename.
Added ExtensionVersionChangeRepository#findFirstByExtensionVersionAndNamespaceOrderByChangedAtDescIdDesc
and scoped wasReportedAsAvailable to it, resolving the latest state
for the version's current namespace specifically. Covered by a new
ExtensionVersionChangeRepositoryTest exercising the exact scenario.
Documented two remaining, narrower gaps as known limitations rather
than folding them into this fix: a concurrent writer (scan completion,
admin action) racing a rename isn't locked against today, and neither
is any other transition writer - pre-existing, not introduced here,
and fixing it means auditing every writer, not just this one. A
version renamed back to a namespace it occupied before is only
repaired correctly if that namespace's own last entry already agrees
with the version's current state - not a concern for any of the four
renames #2244 was filed about.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
extension_version.target_platform has been NOT NULL since V1_22, so
persisting an ExtensionVersion without it failed CI's real-Postgres
run even though it compiled fine and the mocked unit tests never
touch the column.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A rolling deployment can have an old-version instance still serving
traffic while this migration runs on a new one. Without a lock, a
concurrent purge landing between the departures snapshot and its
insert would delete the extension_version row that snapshot already
captured the id of, failing the insert on the foreign key; a
concurrent rename landing between the departures and arrivals
snapshots would let one see the version under its old namespace and
the other under a namespace newer still, repairing neither correctly.
LOCK TABLE ... IN SHARE MODE on extension, extension_version and
extension_version_change before the first snapshot, same pattern
V1_76__Unique_Active_Review.sql already uses for the same class of
rolling-deployment race.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
recordExtensionVersionChange takes no namespace argument - the real
implementation reads extVersion.getExtension().getNamespace() at call
time, from the same mutable Extension object changeExtensionNamespace
later mutates in place. The order-only assertion couldn't tell that
apart from a namespace already changed before either call: it would
still see REMOVED-then-ACTIVE, both against the new namespace.
Capture the namespace via doAnswer at each invocation instead, and
assert it was "old" for the departure and "new" for the arrival.
Verified this catches the regression by temporarily swapping
changeExtensionNamespace before recordNamespaceDeparture locally: the
new assertion fails while the order-only one still passed.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Kept the why, cut the restating and repetition - same content in
roughly half the lines across the service, repository, migration and
tests.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Folded in from #2258 rather than shipping as its own V1_77, which
would collide with this PR's own V1_77 the moment both land - same
table, same migration file is also just a better fit: this PR is the
one already touching extension_version_change's schema semantics.
extension_version_change_seq was created (V1_71) without OWNED BY,
unlike every other entity's sequence (V1_40 added this same link for
the sequences that predated it). Without it, pg_get_serial_sequence
can't find the sequence, which silently defeats
scripts/import-db-dump.sh's generic per-table sequence reset for this
one table - discovered while testing #2258's db-dump tooling changes
against a production copy.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Ran the migration on a copy of the actual production database and verified all four renames from the issue via /api/{namespace} and the /api/-/version-changes feed.
Namespace state:
Extension
Old → new
New namespace resolves
deepseek-harness-web
sparks-dev → csparksdev
✓ downloadable: true
agentmeter, agentmeter-local
agentmeter → zjkjake
✓ downloadable: true
openfasttrace-lsp
felixgorke → fgorke
✓ downloadable: true
police-typing-chase
police-typing-chase → likelitt
✓ downloadable: true
All four old namespaces now 404, as expected.
Feed repair, checked entry-by-entry:
deepseek-harness-web: all 16 published versions got a REMOVED (old namespace) / ACTIVE (new namespace) pair, sharing one lastUpdated timestamp (the migration's run time).
agentmeter: only 0.1.0 and 0.1.1 were repaired - 0.1.2-0.1.5 were published after the historical rename and already carried the correct namespace from the ordinary publish path, so the migration correctly left them alone (no duplicate entries).
openfasttrace-lsp: only 0.7.0 (across its 5 target platforms) was repaired; 0.8.0 was published post-rename, same as above.
police-typing-chase: 1.0.3, 1.1.0, 1.2.0 repaired; 1.3.2 was published post-rename, same as above.
Every REMOVED/ACTIVE pair is 1:1 with no orphaned or duplicate entries, and nothing outside these four renames was touched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Renaming a namespace (
ChangeNamespaceService) moves every extension's versions to the new namespace row in place, without appending anything toextension_version_change. The feed keeps reporting the old(namespace, extension, version)tuple asACTIVEforever, and the new tuple never appears until an unrelated transition happens to touch it.ChangeNamespaceService.changeNamespaceInDatabasenow records aREMOVEDentry for the old tuple of every version the feed already reported, and anACTIVEentry for the new tuple of every version that's still active, both at the instant the rename happens.RepositoryService.wasReportedAsAvailable(ExtensionVersion)is promoted out of a privateExtensionServicehelper so both call sites (delete/purge and now rename) share it.V1_76__Repair_Renamed_Namespace_Version_Changes.sqlbackfills the same two entries for versions that were already renamed before this fix ships.changed_atis stamped with the migration's own run time rather than a reconstructed rename time, since no rename history is kept and a backdated entry would sort into a part of the feed a caught-up consumer has already read past.Fixes #2244.
Test plan
ChangeNamespaceServiceTest(new): active version reports REMOVED then ACTIVE in order; a version never reported by the feed isn't withdrawn; an inactive version isn't announced under the new namespace.ExtensionServiceTestupdated for thewasReportedAsAvailablecall boundary move../gradlew spotlessCheckpasses../gradlew test --tests "org.eclipse.openvsx.admin.*" --tests "org.eclipse.openvsx.ExtensionServiceTest" --tests "org.eclipse.openvsx.RegistryAPITest" --tests "org.eclipse.openvsx.publish.PublishExtensionVersionServiceTest"passes../gradlew test(Testcontainers/Postgres) — not run; no working Docker/Podman in this environment. Needs running in CI or locally before merge.🤖 Generated with Claude Code