diff --git a/.changeset/keep-a-cut-off-run.md b/.changeset/keep-a-cut-off-run.md new file mode 100644 index 0000000..c9cca5e --- /dev/null +++ b/.changeset/keep-a-cut-off-run.md @@ -0,0 +1,11 @@ +--- +'@pdsjs/git-ci': minor +'@pdsjs/core': patch +--- + +Startup no longer adopts a ref whose latest check is still `running`. A run +cut off mid-flight leaves its check record half written, and adopting the ref +told the daemon that ref was handled, so nothing ever finished the record and +the check read `running` for good. The runner's own check records outlive its +state file, so startup reads them: a ref left mid-run stays unadopted and runs +again on the next event for that repository. diff --git a/packages/git-ci/src/cli.js b/packages/git-ci/src/cli.js index 5270662..f6f7386 100644 --- a/packages/git-ci/src/cli.js +++ b/packages/git-ci/src/cli.js @@ -8,7 +8,7 @@ import { readFile } from 'node:fs/promises'; import { resolveRepoLocation, XrpcClient } from '@pdsjs/git'; import { watch } from './daemon.js'; import { createShellExecutor } from './executor.js'; -import { createPublisher } from './publish.js'; +import { createPublisher, createUnfinishedRefsReader } from './publish.js'; import { createRunner } from './runner.js'; import { grantedSecrets, parseSecretsFile } from './secrets.js'; import { parseSpec, repoKey } from './spec.js'; @@ -109,6 +109,7 @@ async function main() { await watch({ repos, runCheck, + unfinishedRefs: createUnfinishedRefsReader({ client, did }), state: createStateStore(statePath(env), onNotice), signal: controller.signal, onNotice, diff --git a/packages/git-ci/src/daemon.js b/packages/git-ci/src/daemon.js index 7654aad..467c64f 100644 --- a/packages/git-ci/src/daemon.js +++ b/packages/git-ci/src/daemon.js @@ -38,6 +38,7 @@ export const WATCHED_COLLECTIONS = [ * @property {import('./state.js').StateStore} state * @property {AbortSignal} [signal] * @property {typeof defaultReadRepoRecord} [readRepoRecord] - how startup learns a repository's refs + * @property {(repoDid: string, repoName: string, refs: string[]) => Promise} [unfinishedRefs] - which refs the runner left mid-run * @property {(message: string) => void} [onNotice] */ @@ -144,6 +145,7 @@ export async function watch(ctx) { const { repos, runCheck, state, signal } = ctx; const onNotice = ctx.onNotice ?? (() => {}); const readRepoRecord = ctx.readRepoRecord ?? defaultReadRepoRecord; + const unfinishedRefs = ctx.unfinishedRefs; // A repository the state file says nothing about has no ref this daemon has // seen, so the first event would read every ref in the record as new and @@ -151,13 +153,30 @@ export async function watch(ctx) { // the first run come from a ref that moves after startup, which is what // starting at the live head means. A repository that cannot be read is left // unseeded: the daemon still watches it, and the first event runs it. + // + // A ref whose latest check is still `running` is left unadopted. Its run + // was cut off with the check record half written, and adopting the ref + // would leave that record `running` for good; unadopted, the ref runs again + // on the next event for the repository. for (const repo of repos) { const key = repoKey(repo.did, repo.repoName); if (Object.keys(state.refs(key)).length > 0) continue; try { const record = await readRepoRecord(repo); - state.setRefs(key, refsToState(record.refs)); - onNotice(`${repo.repoName}: starting from ${record.refs.length} refs`); + let refs = record.refs; + if (unfinishedRefs) { + const cut = await unfinishedRefs( + repo.did, + repo.repoName, + refs.map((ref) => ref.name), + ); + if (cut.length > 0) { + refs = refs.filter((ref) => !cut.includes(ref.name)); + onNotice(`${repo.repoName}: ${cut.join(', ')} was left mid-run`); + } + } + state.setRefs(key, refsToState(refs)); + onNotice(`${repo.repoName}: starting from ${refs.length} refs`); } catch (err) { onNotice( `${repo.repoName}: could not read its refs (${err instanceof Error ? err.message : err})`, diff --git a/packages/git-ci/src/index.js b/packages/git-ci/src/index.js index 18979fc..894c8d4 100644 --- a/packages/git-ci/src/index.js +++ b/packages/git-ci/src/index.js @@ -14,6 +14,7 @@ export { export { checkStatus, createPublisher, + createUnfinishedRefsReader, latestCheckEntries, truncateLogs, } from './publish.js'; diff --git a/packages/git-ci/src/publish.js b/packages/git-ci/src/publish.js index 9e4c00d..bf4eb70 100644 --- a/packages/git-ci/src/publish.js +++ b/packages/git-ci/src/publish.js @@ -72,6 +72,42 @@ export function checkStatus(results) { * @property {string} [finishedAt] */ +/** + * Which of a repository's refs the runner last left mid-run. + * + * The check records are the runner's own memory of what it was doing, and + * they outlive its state file. A daemon that starts with no state adopts the + * refs it finds so it does not rerun the whole record; a ref whose latest + * check is still `running` is the one exception, because the run it belongs + * to was cut off and nothing else will finish it. + * @param {Object} ctx + * @param {import('@pdsjs/git').XrpcClient} ctx.client - the runner's PDS + * @param {string} ctx.did - the runner's own DID + * @returns {(repoDid: string, repoName: string, refs: string[]) => Promise} + */ +export function createUnfinishedRefsReader(ctx) { + const { client, did } = ctx; + return async (repoDid, repoName, refs) => { + /** @type {string[]} */ + const unfinished = []; + for (const ref of refs) { + const rkey = latestCheckKey(repoDid, repoName, ref); + // A repository the runner has never published for reads as absent, + // which is not a cut-off run. + const current = await client.getRecord( + did, + GIT_LATEST_CHECK_COLLECTION, + rkey, + ); + const entries = latestCheckEntries(current?.value); + if (entries.some((entry) => entry.status === 'running')) { + unfinished.push(ref); + } + } + return unfinished; + }; +} + /** * The per-workflow entries of a latest-check record. A record written * before entries existed carries one workflow's fields at the top level; diff --git a/packages/git-ci/test/daemon.test.js b/packages/git-ci/test/daemon.test.js index 1ee893f..063300a 100644 --- a/packages/git-ci/test/daemon.test.js +++ b/packages/git-ci/test/daemon.test.js @@ -415,6 +415,33 @@ describe('watch startup', () => { expect(ran).toEqual([]); }); + it('leaves a ref whose run was cut off, so it runs again', async () => { + const store = createStateStore(join(dir, 'state.json')); + /** @type {string[]} */ + const notices = []; + await watch({ + repos: REPOS, + runCheck: async () => 'success', + state: store, + signal: quietSignal(), + onNotice: (message) => notices.push(message), + readRepoRecord: async () => ({ + uri: `at://${DID}/dev.pdsjs.git.repo/proj`, + cid: 'bafyreiaaa', + refs: [ + { name: 'refs/heads/main', sha: MAIN }, + { name: 'refs/tags/v1.0.0', sha: MAIN }, + ], + }), + unfinishedRefs: async () => ['refs/heads/main'], + }); + + // Adopting main would leave its half-written check record running for + // good. Unadopted, the next event for the repository runs it again. + expect(store.refs(`${DID}/proj`)).toEqual({ 'refs/tags/v1.0.0': MAIN }); + expect(notices.some((n) => n.includes('was left mid-run'))).toBe(true); + }); + it('keeps the refs it already knows', async () => { const store = createStateStore(join(dir, 'state.json')); store.setRefs(`${DID}/proj`, { 'refs/heads/main': MAIN }); diff --git a/packages/git-ci/test/publish.test.js b/packages/git-ci/test/publish.test.js index 31dedc5..8a0ad2d 100644 --- a/packages/git-ci/test/publish.test.js +++ b/packages/git-ci/test/publish.test.js @@ -4,7 +4,12 @@ import { GIT_CHECK_COLLECTION, GIT_LATEST_CHECK_COLLECTION, } from '../src/lexicon.js'; -import { checkStatus, createPublisher, truncateLogs } from '../src/publish.js'; +import { + checkStatus, + createPublisher, + createUnfinishedRefsReader, + truncateLogs, +} from '../src/publish.js'; const SUBJECT = { uri: 'at://did:plc:abc/dev.pdsjs.git.repo/proj', @@ -352,3 +357,48 @@ describe('createPublisher', () => { expect(calls()).toBe(3); }); }); + +describe('createUnfinishedRefsReader', () => { + const RUNNER = 'did:plc:runner'; + const OWNER = 'did:plc:abc'; + + /** + * @param {ReturnType} fake + * @param {string} ref + * @param {Array<{workflow: string, status: string}>} checks + */ + async function seed(fake, ref, checks) { + await fake.client.putRecord( + RUNNER, + GIT_LATEST_CHECK_COLLECTION, + `${OWNER}:proj:${ref.replace(/[^a-zA-Z0-9.:_~-]/g, '_')}`, + { $type: GIT_LATEST_CHECK_COLLECTION, ref, checks }, + null, + ); + } + + it('names only the refs an entry is still running for', async () => { + const fake = fakeClient(); + await seed(fake, 'refs/heads/main', [ + { workflow: 'ci', status: 'success' }, + { workflow: 'publish-wizard', status: 'running' }, + ]); + await seed(fake, 'refs/tags/v1.0.0', [ + { workflow: 'ci', status: 'success' }, + ]); + + const unfinished = createUnfinishedRefsReader({ + client: /** @type {*} */ (fake.client), + did: RUNNER, + }); + + expect( + await unfinished(OWNER, 'proj', [ + 'refs/heads/main', + 'refs/tags/v1.0.0', + // Never published for, so the record is absent rather than running. + 'refs/heads/topic', + ]), + ).toEqual(['refs/heads/main']); + }); +});