From 73408cd9d11f31291010dd775a99e9f841580e15 Mon Sep 17 00:00:00 2001 From: Thomas Honeyman Date: Sat, 10 Oct 2026 12:01:38 +0000 Subject: [PATCH] Restore cached responses for duplicate publish requests --- SPEC.md | 29 ++++++------ app-e2e/src/Test/E2E/Endpoint/Publish.purs | 8 ++-- app-e2e/src/Test/E2E/Endpoint/Unpublish.purs | 12 +---- app-e2e/src/Test/E2E/Workflow.purs | 7 +-- app/src/App/SQLite.js | 24 ++++++++-- app/src/App/SQLite.purs | 23 ++++----- app/src/App/Server/Router.purs | 16 +++---- app/test/App/SQLite.js | 14 ++++++ app/test/App/SQLite.purs | 50 ++++++++++++-------- lib/src/API/V1.purs | 2 +- 10 files changed, 104 insertions(+), 81 deletions(-) diff --git a/SPEC.md b/SPEC.md index f58117bc..af6398db 100644 --- a/SPEC.md +++ b/SPEC.md @@ -123,11 +123,10 @@ The registry responds immediately with a job ID that can be polled for status: } ``` -The publish response disposition is `created` for a new job, -`duplicate-active` when an unfinished job already owns the package version, or -`already-published` when an equivalent successful publish job exists. New jobs -return HTTP 201, while `duplicate-active` and `already-published` responses -return HTTP 200. +New jobs return HTTP 201 with disposition `created`. If any publish job already +exists for the package name and version, the server returns a cached job ID with +HTTP 200 and no disposition, without starting another attempt. This includes +failed jobs and requests with changed payloads. Poll the returned job for its outcome. #### Step 2: Source Fetched and Validated @@ -867,7 +866,7 @@ After a package is successfully published to the registry, the registry attempts **Compiler Version Requirement** -Documentation can only be published to Pursuit using compiler version 0.14.7 or later. Packages published with older compilers will skip this step. If documentation publishing was skipped due to an older compiler, you can retry by resubmitting a publish operation with a supported compiler version. +Documentation can only be published to Pursuit using compiler version 0.14.7 or later. Packages published with older compilers will skip this step. If documentation publishing was skipped due to an older compiler, contact the registry trustees; resubmitting through the publish endpoint returns a cached job. **Publishing Process** @@ -879,7 +878,7 @@ To publish documentation, the registry: **Retrying Failed Documentation Uploads** -If documentation publishing fails (for example, due to a transient network error), you can retry by resubmitting a publish operation for the same package version. The registry will detect that the version is already published and skip the tarball upload, but will retry the Pursuit upload if documentation is missing. +If documentation publishing fails (for example, due to a transient network error), contact the registry trustees. Resubmitting through the publish endpoint returns a cached job rather than starting another attempt. #### 6.3 Publish to Package Sets @@ -997,7 +996,7 @@ POST to `/api/v1/publish` with a JSON body matching the [`PublishData`](#51-publ } ``` -The response contains a job ID and a publish submission disposition: +The response contains a job ID and an optional publish submission disposition: ```json { @@ -1006,12 +1005,14 @@ The response contains a job ID and a publish submission disposition: } ``` -`disposition` is one of `created`, `duplicate-active`, or -`already-published`. New jobs return HTTP 201; `duplicate-active` and -`already-published` responses return HTTP 200. An active job owns its package -name and version even if its payload differs, because registry package versions -are immutable; clients can inspect the returned job's `payload` when handling -`duplicate-active`. +New jobs return HTTP 201 with disposition `created`. If any publish job already +exists for the package name and version, the response is HTTP 200 with only its +job ID. The server prefers an active job, then a successful job with the exact +same payload that has not been invalidated by a later successful unpublish. +Otherwise it returns the oldest job for that name and version. Changed payloads +and later unpublishing do not permit a new attempt. Poll the returned job for +its original payload and outcome; a cached result is not a statement about the +package's current publication state. #### Polling Job Status diff --git a/app-e2e/src/Test/E2E/Endpoint/Publish.purs b/app-e2e/src/Test/E2E/Endpoint/Publish.purs index ce5e96db..28ae4162 100644 --- a/app-e2e/src/Test/E2E/Endpoint/Publish.purs +++ b/app-e2e/src/Test/E2E/Endpoint/Publish.purs @@ -112,14 +112,14 @@ spec = do unless (Array.all (_.jobId >>> (_ == head.jobId)) tail) do Assert.fail "Expected concurrent publish requests to return one job ID." let created = Array.filter (_.disposition >>> (_ == Just V1.Created)) responses - let duplicateActive = Array.filter (_.disposition >>> (_ == Just V1.DuplicateActive)) responses + let cached = Array.filter (_.disposition >>> isNothing) responses Assert.shouldEqual (Array.length created) 1 - Assert.shouldEqual (Array.length duplicateActive) 4 + Assert.shouldEqual (Array.length cached) 4 _ <- Env.pollJobOrFail head.jobId completedDuplicate <- Client.publish Fixtures.effectPublishData Assert.shouldEqual completedDuplicate.jobId head.jobId - Assert.shouldEqual completedDuplicate.disposition (Just V1.AlreadyPublishedSubmission) + Assert.shouldEqual completedDuplicate.disposition Nothing Spec.it "reports an already-published package as idempotent success" do created <- Client.publish Fixtures.effectAlreadyPublishedData @@ -133,4 +133,4 @@ spec = do duplicate <- Client.publish Fixtures.effectAlreadyPublishedData Assert.shouldEqual duplicate.jobId created.jobId - Assert.shouldEqual duplicate.disposition (Just V1.AlreadyPublishedSubmission) + Assert.shouldEqual duplicate.disposition Nothing diff --git a/app-e2e/src/Test/E2E/Endpoint/Unpublish.purs b/app-e2e/src/Test/E2E/Endpoint/Unpublish.purs index 5ef869c8..1f1816ea 100644 --- a/app-e2e/src/Test/E2E/Endpoint/Unpublish.purs +++ b/app-e2e/src/Test/E2E/Endpoint/Unpublish.purs @@ -53,16 +53,8 @@ spec = do Assert.fail "Expected version to be removed from manifest index after unpublish" republish <- Client.publish Fixtures.effectPublishData - Assert.shouldEqual republish.disposition (Just V1.Created) - when (republish.jobId == publishJobId) do - Assert.fail "A later unpublish must invalidate successful publish-job deduplication." - republishJob <- Env.pollJobExpectFailure republish.jobId - case republishJob of - V1.PublishJob { error: Just error } -> - unless (String.contains (String.Pattern "has been unpublished") error.message) do - Assert.fail $ "Expected an unpublished-version error, got: " <> error.message - V1.PublishJob { error: Nothing } -> Assert.fail "Expected a structured republish error." - _ -> Assert.fail "Expected a publish job." + Assert.shouldEqual republish.disposition Nothing + Assert.shouldEqual republish.jobId publishJobId -- Test race condition: submit unpublish while publish is still running. -- Job priority (Unpublish > Matrix) ensures unpublish runs before matrix jobs. diff --git a/app-e2e/src/Test/E2E/Workflow.purs b/app-e2e/src/Test/E2E/Workflow.purs index 8f93f5d1..e68cddd4 100644 --- a/app-e2e/src/Test/E2E/Workflow.purs +++ b/app-e2e/src/Test/E2E/Workflow.purs @@ -64,9 +64,10 @@ spec = do Assert.fail "Expected no tarball upload for console@6.1.0 after failed publish" retry <- Client.publish Fixtures.consolePublishData - Assert.shouldEqual retry.disposition (Just V1.Created) - when (retry.jobId == consoleJobId) do - Assert.fail "Expected a new publish job after terminal failure." + Assert.shouldEqual retry.disposition Nothing + Assert.shouldEqual retry.jobId consoleJobId + changed <- Client.publish $ Fixtures.consolePublishData { ref = "different-ref" } + Assert.shouldEqual changed.jobId consoleJobId _ <- Env.pollJobExpectFailure retry.jobId pure unit diff --git a/app/src/App/SQLite.js b/app/src/App/SQLite.js index 631da891..b3ee7548 100644 --- a/app/src/App/SQLite.js +++ b/app/src/App/SQLite.js @@ -84,12 +84,12 @@ export const insertPublishJobImpl = (db, job) => { `).get(job.packageName, job.packageVersion); if (active) { - return { jobId: active.jobId, status: 1 }; + return { jobId: active.jobId, created: false }; } // A successful job is idempotently reusable only for the exact submitted - // payload. A later successful unpublish invalidates that result so the new - // publish attempt can run and report the authoritative unpublished error. + // payload. A later successful unpublish invalidates that result; in that + // case we fall back to the cached history below without starting new work. const successful = db.prepare(` SELECT job.jobId, info.finishedAt FROM ${PUBLISH_JOBS_TABLE} job @@ -111,12 +111,26 @@ export const insertPublishJobImpl = (db, job) => { `).get(job.packageName, job.packageVersion, successful.finishedAt); if (!laterUnpublish) { - return { jobId: successful.jobId, status: 2 }; + return { jobId: successful.jobId, created: false }; } } + // Keep failed attempts cached too. Changing the payload must not bypass + // the cache and start another expensive publish for the same name/version. + const existing = db.prepare(` + SELECT job.jobId + FROM ${PUBLISH_JOBS_TABLE} job + JOIN ${JOB_INFO_TABLE} info ON job.jobId = info.jobId + WHERE job.packageName = ? AND job.packageVersion = ? + ORDER BY info.createdAt ASC LIMIT 1 + `).get(job.packageName, job.packageVersion); + + if (existing) { + return { jobId: existing.jobId, created: false }; + } + _insertJob(db, PUBLISH_JOBS_TABLE, columns, job); - return { jobId: job.jobId, status: 0 }; + return { jobId: job.jobId, created: true }; }); // Acquire the write lock before duplicate detection. This keeps the read and diff --git a/app/src/App/SQLite.purs b/app/src/App/SQLite.purs index f9d06ea3..b8d98c1e 100644 --- a/app/src/App/SQLite.purs +++ b/app/src/App/SQLite.purs @@ -9,7 +9,7 @@ module Registry.App.SQLite , InsertMatrixJob , InsertPackageSetJob , InsertPublishJob - , InsertPublishJobResult(..) + , InsertPublishJobResult , InsertTransferJob , InsertUnpublishJob , JobInfo @@ -401,13 +401,13 @@ type JSInsertPublishJob = type JSInsertPublishJobResult = { jobId :: String - , status :: Int + , created :: Boolean } -data InsertPublishJobResult - = PublishJobCreated JobId - | PublishJobDuplicateActive JobId - | PublishJobAlreadyPublished JobId +type InsertPublishJobResult = + { jobId :: JobId + , created :: Boolean + } insertPublishJobToJSRep :: JobId -> DateTime -> InsertPublishJob -> JSInsertPublishJob insertPublishJobToJSRep jobId now { payload } = @@ -420,19 +420,14 @@ insertPublishJobToJSRep jobId now { payload } = foreign import insertPublishJobImpl :: EffectFn2 SQLite JSInsertPublishJob JSInsertPublishJobResult --- | Atomically insert a publish job or return the active/equivalent successful --- | job which makes a new insertion unnecessary. +-- | Atomically insert a publish job only when no history exists for its name +-- | and version. Prefer an active or equivalent successful job when available. insertPublishJob :: SQLite -> InsertPublishJob -> Effect InsertPublishJobResult insertPublishJob db job = do jobId <- newJobId now <- nowUTC result <- Uncurried.runEffectFn2 insertPublishJobImpl db $ insertPublishJobToJSRep jobId now job - let resultJobId = JobId result.jobId - pure case result.status of - 0 -> PublishJobCreated resultJobId - 1 -> PublishJobDuplicateActive resultJobId - 2 -> PublishJobAlreadyPublished resultJobId - status -> unsafeCrashWith $ "Invalid publish job insertion status " <> show status + pure { jobId: JobId result.jobId, created: result.created } -------------------------------------------------------------------------------- -- unpublish_jobs table diff --git a/app/src/App/Server/Router.purs b/app/src/App/Server/Router.purs index fcf24d8d..b6ab7386 100644 --- a/app/src/App/Server/Router.purs +++ b/app/src/App/Server/Router.purs @@ -19,7 +19,6 @@ import Registry.App.Auth as Auth import Registry.App.Effect.Db as Db import Registry.App.Effect.Env as Env import Registry.App.Effect.Log as Log -import Registry.App.SQLite (InsertPublishJobResult(..)) import Registry.App.Server.Env (ServerEffects, ServerEnv, jsonCreated, jsonDecoder, jsonOk, runEffects) import Registry.Operation (PackageSetOperation(..)) import Registry.Operation as Operation @@ -82,15 +81,12 @@ router { route, method, body } = HTTPurple.usingCont case route, method of publish <- HTTPurple.fromJson (jsonDecoder Operation.publishCodec) body lift $ Log.info $ "Received Publish request: " <> printJson Operation.publishCodec publish - lift (Db.insertPublishJob { payload: publish }) >>= case _ of - PublishJobCreated jobId -> - jsonCreated V1.publishJobResponseCodec { jobId, disposition: Just V1.Created } - PublishJobDuplicateActive jobId -> do - lift $ Log.warn $ "Duplicate active publish job, returning existing one: " <> unwrap jobId - jsonOk V1.publishJobResponseCodec { jobId, disposition: Just V1.DuplicateActive } - PublishJobAlreadyPublished jobId -> do - lift $ Log.info $ "Equivalent publish job already succeeded, returning existing one: " <> unwrap jobId - jsonOk V1.publishJobResponseCodec { jobId, disposition: Just V1.AlreadyPublishedSubmission } + { jobId, created } <- lift $ Db.insertPublishJob { payload: publish } + if created then + jsonCreated V1.publishJobResponseCodec { jobId, disposition: Just V1.Created } + else do + lift $ Log.warn $ "Duplicate publish job, returning existing one without starting new work: " <> unwrap jobId + jsonOk V1.jobCreatedResponseCodec { jobId } Unpublish, Post -> do auth <- HTTPurple.fromJson (jsonDecoder Operation.authenticatedCodec) body diff --git a/app/test/App/SQLite.js b/app/test/App/SQLite.js index 6d547f54..a67c125b 100644 --- a/app/test/App/SQLite.js +++ b/app/test/App/SQLite.js @@ -1,5 +1,6 @@ import Database from "better-sqlite3"; import { readFileSync } from "node:fs"; +import { randomUUID } from "node:crypto"; export const withDatabase = (test) => () => { const db = new Database(":memory:"); @@ -11,3 +12,16 @@ export const withDatabase = (test) => () => { db.close(); } }; + +// Seed the failure-then-retry history that exists from before retry admission +// was disabled. The public insert operation deliberately no longer permits it. +export const insertHistoricalPublishJob = (db) => (payload) => () => { + const { name, version } = JSON.parse(payload); + const jobId = randomUUID(); + const latest = db.prepare("SELECT MAX(createdAt) AS createdAt FROM job_info").get(); + const createdAt = new Date(Date.parse(latest.createdAt) + 1).toISOString(); + db.prepare("INSERT INTO job_info (jobId, createdAt) VALUES (?, ?)").run(jobId, createdAt); + db.prepare("INSERT INTO publish_jobs (jobId, packageName, packageVersion, payload) VALUES (?, ?, ?, ?)") + .run(jobId, name, version, payload); + return jobId; +}; diff --git a/app/test/App/SQLite.purs b/app/test/App/SQLite.purs index f313ce9a..cb89233d 100644 --- a/app/test/App/SQLite.purs +++ b/app/test/App/SQLite.purs @@ -3,19 +3,22 @@ module Test.Registry.App.SQLite (spec) where import Registry.App.Prelude import Effect.Exception as Exception -import Registry.API.V1 (SortOrder(..)) +import Registry.API.V1 (JobId(..), SortOrder(..)) import Registry.API.V1 as V1 import Registry.App.SQLite (SQLite) import Registry.App.SQLite as SQLite import Registry.Location (Location(..)) +import Registry.Operation as Operation import Registry.Test.Assert as Assert import Registry.Test.Utils as Utils import Test.Spec as Spec foreign import withDatabase :: (SQLite -> Effect Unit) -> Effect Unit +foreign import insertHistoricalPublishJob :: SQLite -> String -> Effect String + spec :: Spec.Spec Unit -spec = Spec.it "allows failed publishes to be retried without losing deduplication or job history" $ liftEffect $ withDatabase \db -> do +spec = Spec.it "caches failed publishes while preserving active and successful historical attempts" $ liftEffect $ withDatabase \db -> do let payload = { name: Utils.unsafePackageName "puppy-runtime" @@ -26,41 +29,48 @@ spec = Spec.it "allows failed publishes to be retried without losing deduplicati , location: Just $ GitHub { owner: "katsujukou", repo: "puppy", subdir: Just "puppy-runtime" } } lookup = map (map (map _.jobId)) $ SQLite.selectNextPublishJob db - submit payload' = SQLite.insertPublishJob db { payload: payload' } <#> case _ of - SQLite.PublishJobCreated jobId -> Tuple V1.Created jobId - SQLite.PublishJobDuplicateActive jobId -> Tuple V1.DuplicateActive jobId - SQLite.PublishJobAlreadyPublished jobId -> Tuple V1.AlreadyPublishedSubmission jobId - create payload' = SQLite.insertPublishJob db { payload: payload' } >>= case _ of - SQLite.PublishJobCreated jobId -> pure jobId - _ -> Exception.throw "Expected a new publish job." + submit payload' = SQLite.insertPublishJob db { payload: payload' } + create payload' = do + result <- submit payload' + unless result.created $ Exception.throw "Expected a new publish job." + pure result.jobId lookup >>= Assert.shouldEqual (Right Nothing) failedId <- create payload lookup >>= Assert.shouldEqual (Right $ Just failedId) - submit payload >>= Assert.shouldEqual (Tuple V1.DuplicateActive failedId) + submit payload >>= Assert.shouldEqual { jobId: failedId, created: false } now <- nowUTC SQLite.startJob db { jobId: failedId, startedAt: now } - submit payload >>= Assert.shouldEqual (Tuple V1.DuplicateActive failedId) + submit payload >>= Assert.shouldEqual { jobId: failedId, created: false } SQLite.finishJob db { jobId: failedId, finishedAt: now, success: false, disposition: Nothing, error: Nothing } lookup >>= Assert.shouldEqual (Right Nothing) - -- An unchanged payload must also be retryable after a transient failure. - retryId <- create payload - lookup >>= Assert.shouldEqual (Right $ Just retryId) - SQLite.finishJob db { jobId: retryId, finishedAt: now, success: false, disposition: Nothing, error: Nothing } + submit payload >>= Assert.shouldEqual { jobId: failedId, created: false } + let corrected = payload { location = Just $ GitHub { owner: "katsujukou", repo: "purescript-puppy-runtime", subdir: Nothing } } + submit corrected >>= Assert.shouldEqual { jobId: failedId, created: false } + submit (corrected { ref = "different-ref" }) >>= Assert.shouldEqual { jobId: failedId, created: false } lookup >>= Assert.shouldEqual (Right Nothing) - let corrected = payload { location = Just $ GitHub { owner: "katsujukou", repo: "purescript-puppy-runtime", subdir: Nothing } } - correctedId <- create corrected + -- Production may already have a newer attempt following an older failure. + correctedId <- JobId <$> insertHistoricalPublishJob db (stringifyJson Operation.publishCodec corrected) selected <- SQLite.selectNextPublishJob db map (map _.payload) selected `Assert.shouldEqual` Right (Just corrected) - submit payload >>= Assert.shouldEqual (Tuple V1.DuplicateActive correctedId) + submit payload >>= Assert.shouldEqual { jobId: correctedId, created: false } SQLite.selectNextPublishJob db >>= \next -> map (map _.jobId) next `Assert.shouldEqual` Right (Just correctedId) SQLite.startJob db { jobId: correctedId, startedAt: now } - submit corrected >>= Assert.shouldEqual (Tuple V1.DuplicateActive correctedId) + submit corrected >>= Assert.shouldEqual { jobId: correctedId, created: false } SQLite.finishJob db { jobId: correctedId, finishedAt: now, success: true, disposition: Just V1.Published, error: Nothing } - submit corrected >>= Assert.shouldEqual (Tuple V1.AlreadyPublishedSubmission correctedId) + submit corrected >>= Assert.shouldEqual { jobId: correctedId, created: false } + submit payload >>= Assert.shouldEqual { jobId: failedId, created: false } + submit (corrected { ref = "different-ref" }) >>= Assert.shouldEqual { jobId: failedId, created: false } + lookup >>= Assert.shouldEqual (Right Nothing) + + -- A new version is independent, but a changed payload cannot rerun its success. + let nextVersion = corrected { version = Utils.unsafeVersion "0.2.1", ref = "v0.2.1" } + nextId <- create nextVersion + SQLite.finishJob db { jobId: nextId, finishedAt: now, success: true, disposition: Just V1.Published, error: Nothing } + submit (nextVersion { ref = "different-ref" }) >>= Assert.shouldEqual { jobId: nextId, created: false } lookup >>= Assert.shouldEqual (Right Nothing) -- Looking up the old failure by ID must still work. diff --git a/lib/src/API/V1.purs b/lib/src/API/V1.purs index 74071646..35e54119 100644 --- a/lib/src/API/V1.purs +++ b/lib/src/API/V1.purs @@ -143,7 +143,7 @@ jobCreatedResponseCodec = CJ.named "JobCreatedResponse" $ CJ.Record.object { job -- | The outcome of submitting a publish request. The field is optional in the -- | response codec so clients remain able to decode responses from older --- | registry servers, but current servers always provide it. +-- | registry servers. Cached submissions also omit it; poll the job for its outcome. data PublishSubmissionDisposition = Created | DuplicateActive