artifact retention - #1090
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
General PR Review: artifact retentionBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review Summary Reviewed the full PR diff (14 files; no Risk triage (repo criteria, "Risk Triage And Escalation"): silence — yes (a wrong verdict is a well-formed error a runner acts on silently); durability — yes (the decision selects which c1z survives); uncontrolled dimensions — yes (stop timing, SDK-version by runner-version pair); consumer distance — the hosted runner, another repo. Verdict: HIGH, driven by the seam contract rather than the retention branch itself. Review-blind class: error-path plus multi-artifact/version-pair. The PR names and contains its instruments — the Security Issues None found. The RFC content-guidelines item below is a repository-policy issue, not a vulnerability. Correctness Issues None new. Two prior findings remain unchanged in the current tree and are repeated here so they are not lost when this summary replaces the previous one; no new inline comments were posted for them:
Suggestions
Prompt for AI agents |
| **No kill switch (decided at implementation).** An earlier draft gated the | ||
| inversion behind a hosted-side feature flag whose off-state reproduced the | ||
| old tree. Dropped, deliberately: the marginal risk the flag would guard is | ||
| already in production — `FEATURE_FLAG_ID_SYNC_PRESERVABLE_C1Z` is `SKU_ALL`, |
There was a problem hiding this comment.
🟡 Suggestion: this is a public repository (see CLAUDE.md → "Public Repository — Content Guidelines"), and this line publishes an internal hosted-platform feature-flag identifier plus its fleet rollout state (SKU_ALL), repeated at line 344. Lines 324-325 similarly name internal runner packages. Consider a generic phrasing — "the preservable-c1z allowlist is already enabled fleet-wide" — and dropping the flag constant. (confidence: high)
| // engine teardown) deliberately do not carry it — the artifact is a | ||
| // faithful commit at that point. Consumed via | ||
| // sync.ShouldDiscardSyncArtifact; test with errors.Is. | ||
| var ErrArtifactUnusable = errors.New("sync artifact unusable") |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: medium) The verdict names staleness, but ShouldDiscardSyncArtifact is consumed as unusability. Every producer's own comment says the artifact is "stale, never torn" / "the atomic save left untouched" — i.e. the on-disk c1z is still a valid, resumable commit from a previous attempt of the same sync. RFC §4.1 uses exactly this argument to exclude EndSync/CheckpointSync as producers ("a verdict would instruct a runner to discard a still-valid artifact"), yet the close/finalize producers have the same property.
Concretely, on a resumed sync where saveC1z/pebbleStore.save fails, a migrated runner discards the prior attempt's checkpoint and restarts from zero — the failure mode this RFC exists to eliminate. For the FinalizeTimeout case this is also a strict narrowing vs. today (IsSyncPreservable preserves DeadlineExceeded), which cuts against the "preserve-by-default strictly widens preservation" framing; TestRetentionPreservableCoherence pins it with the rationale "a stale artifact holds no progress from this run" — true, but it can hold progress from earlier attempts.
Worth either narrowing the verdict to genuinely-unusable artifacts (torn/corrupt output), or documenting in the RFC why losing an older-but-valid checkpoint is preferable to retrying the upload.
| // caller in the batch error regardless, and queue.abort() | ||
| // must always run — it releases workers blocked in | ||
| // queue.next(). | ||
| if preBatchCtx.Err() == nil { |
There was a problem hiding this comment.
🟡 Suggestion: (confidence: medium) On the external-cancel path a genuine action failure that races the cancel now has no log line anywhere in the SDK. This guard suppresses the worker log, and handleOperationError's compensating "sync operation failed while run duration expired" log (line 461) only exists on the DeadlineExceeded branch — the cause != nil branch returns batchErr silently. The only remaining sink is returnSyncError, which no-ops unless s.recordStats is set (pebble only), so on the sqlite engine the error is invisible.
Consider mirroring the deadline branch's log in the cancel branch of handleOperationError: log batchErr at Error when it is not context.Canceled/DeadlineExceeded, so a real connector/store bug can't hide behind a worker drain.
Freezes the behavioral plan for inverting partial-c1z retention: discard only on a typed storage verdict (ErrArtifactUnusable), never on connector or scheduling error shapes. Documents the frozen error surface and marker text the hosted runner branches on during the SDK-first window, the tandem-landing merge criteria, and the connector-on-tag skew disposition. Co-authored-by: Cursor <cursoragent@cursor.com>
Add the typed storage verdict ErrArtifactUnusable and its seam helper ShouldDiscardSyncArtifact: the only signal that permits a runner to discard a partial c1z. Both engines produce it on close/finalize failures where mutations exist but the output c1z was not rewritten (atomic temp+rename means stale, never torn); post-save teardown failures deliberately carry no verdict. The wrapper is message-preserving, so operator-facing error text is byte-identical. Fix the two cancellation-path defects behind routine data loss on external cancels: workers observing shutdown no longer log action failures (queue.abort and cancel-cause propagation unchanged), and cancel-shaped exits now take a best-effort checkpoint on a detached bounded context before returning — side-effect-only, the returned error is unchanged. IsSyncPreservable is frozen in place with its table pinned; the lambda transport marker text gains an explicit freeze note naming the hosted runner's string-matching consumer. Instruments per RFC 0009 §5: retention conformance/coherence tables, storage-verdict fault injection for both engines plus the cross-layer traversal through syncer.Close, and chaos coverage for external cancel (quiet workers, stop checkpoint, bounded exit, cold resume) with a positive control pinning that genuine action failures still log. Co-authored-by: Cursor <cursoragent@cursor.com>
…ision The marginal risk a flag would guard is already in production (the IsSyncPreservable allowlist is SKU_ALL fleet-wide) and poisoned-checkpoint escape is the budgets' job either way; the worst case flagless equals today's discard behavior. The legacy tree is deleted rather than gated, the old flag retires with it, and rollback is a one-commit revert. Co-authored-by: Cursor <cursoragent@cursor.com>
… to shutdown Review fixes on the RFC 0009 mechanical changes, plus one gap they exposed: - checkpointOnStop was bounded by dotc1z.FinalizeTimeout (an hour, sized for the finalize+upload tail). The work is one sync-token state write on a detached, caller-uninterruptible context sitting in front of Sync() returning on an external stop — a wedged store would burn the drain grace before Close() could commit the c1z. Now bounded by a one-minute stopCheckpointTimeout. The run-duration-expiry checkpoints deliberately stay on the attached caller context (still live there); documented. - The shutdown log suppression guarded on the batch cancel-cause context, which the first failing sibling also cancels — a second worker's independent genuine failure lost its log line. The guard now reads the pre-batch caller context, so every genuine failure logs and only shutdown-shaped exits are suppressed. Pinned by a deterministic rendezvous test that fails with exactly one line under the old guard. - Stress-testing the pins surfaced a third stop exit with no freshness checkpoint: the loop-top periodic checkpoint runs on the caller's context, so an external cancel landing between batches surfaced as a checkpoint failure and returned without the best-effort stop checkpoint. It now gets the same detached rescue (side-effect-only; err unchanged). - The external-cancel chaos test premise gated on ActiveOperations alone, which any healthy in-flight RPC satisfies — under load the cancel fired before the sync reached the blocked call, testing startup teardown instead. The premise now also requires the blocking rule to have fired (new Runtime.Fires accessor), and the returned-error pin accepts both frozen cancel shapes (context.Canceled and a codes.Canceled status), matching RFC 0009 §4.4. Fifteen consecutive stress iterations clean, from a one-in-three failure rate. Co-authored-by: Cursor <cursoragent@cursor.com>
…not hangs The new second-genuine-failure test parked both workers on an unconditional WaitGroup.Wait() inside their actions. That is only safe while the pool dispatches both actions concurrently: if it ever serializes, a worker blocks forever and the package dies on the go test timeout instead of reporting a failure — the worst shape for a CI signal, and scheduler-dependent, so it would not reproduce locally. The rendezvous is now bounded, with the premise asserted explicitly: if both actions are not in flight before either fails, the test says so. Co-authored-by: Cursor <cursoragent@cursor.com>
go's default timeout is 10m PER PACKAGE. pkg/sync alone takes ~7m locally under -covermode=count on 10 cores; a 4-vCPU runner executing packages concurrently pushes it past 10m, which surfaces as a timeout panic rather than a test failure — indistinguishable, at a glance, from a hang. The Windows step already carries a bound for the same class of reason. This is a headroom fix, not a mask: the suite passes end to end locally under the CI flag set, and no single test dominates (the slowest is ~57s). Co-authored-by: Cursor <cursoragent@cursor.com>
Three findings from implementing the plan, written back so the RFC stays the record rather than the original guess: - There are three stop exits in the parallel syncer, not two: the loop-top periodic checkpoint runs on the caller's context and can return before the other two. Its coverage is stated honestly as defense-in-depth without a dedicated instrument, along with why a deterministic one is not proportionate. - Retention is an obligation per exit, not per call site — close-failure exits are retention decisions too, so counting sync-error branches undercounted the runner work. - Preserve-on-cancel requires a detached commit; durability of the commit is part of the contract, not a property of whichever context a call site held. Also states the log-suppression guard reads the pre-batch context and why the batch's own context would be wrong. Co-authored-by: Cursor <cursoragent@cursor.com>
… test TestArtifactVerdictOnSaveFailure/pebble failed only on windows-latest. The body's assertions hold there — planting a directory at path+".tmp" fails pebble's os.OpenFile the same way it does on unix — so the failure came from t.TempDir()'s cleanup: after a failed save the pebble store deliberately tears NOTHING down, since the unpacked DB is the only copy of the data and it must stay open for a retried Close. Windows refuses to delete files with live handles, so RemoveAll fails and testing reports it as a subtest failure with no assertion message. CI corroborated this before the fix: TestArtifactVerdictClearsOnRetriedClose plants the same directory on the same engine and passed on Windows, differing only in that it closes cleanly at the end. Unblocking the save and closing again releases the engine and pins the advertised recovery path in the same step. The sqlite subtest needs no retry: it closes its DB handle and clears its working dir before saving, which is why it was already green on Windows. Co-authored-by: Cursor <cursoragent@cursor.com>
…le release Three review findings, all documentation/robustness rather than behavior: The suppression guard reads the pre-batch context, which in production is parallelSync's workerCtx — cancelled by run-duration expiry as well as by an external cancel. Both are stops, so suppressing there is right, but the comment claimed "every genuine failure logs" without qualification and the variable name implied the Sync caller's context. Renamed to preBatchCtx, stated the run-duration case and the failure-races-a-stop limit, and added the test for that previously uncovered cell. ErrArtifactUnusable's doc claimed producers surface through Sync() or Close(); the syncer's only store.Close call is in Close(), and the mid-sync commits return bare errors, so a Sync() error always preserves. Said so. The verdict tests released the engine handle only on the happy path, so a failing assertion would leave pebble open and turn into Windows TempDir cleanup noise — the exact failure mode that hid the last assertion message. Moved the release into t.Cleanup at the point of the plant. Co-authored-by: Cursor <cursoragent@cursor.com>
…exception Two accepted PR review findings, both instruments rather than behavior: The coherence test claimed "preservable ⊆ preserved" unconditionally, but a real production shape violates it: C1File.finalize runs under FinalizeTimeout, so a WAL checkpoint outliving that budget produces a verdict wrapping context.DeadlineExceeded — old-policy preservable AND a discard. The relation is now stated in two halves (over verdict-free errors the widening holds; a carried verdict discards regardless of the wrapped cause, because it classifies the artifact, not the cause) and the exception shape is pinned in the taxonomy and asserted explicitly. The loop-top periodic-checkpoint stop exit was dispositioned as defense-in-depth without a dedicated instrument, on the premise that determinism required a production-visible hook. Review pointed out a cheaper shape: call parallelSync directly with a pre-canceled caller context and a store stub that fails CheckpointSync exactly when the caller's context is done. Both cells now exist and are plant-validated — the detached rescue runs and succeeds on a stop, and a genuine store failure with a live caller gets exactly one write, no detached retry. The RFC change order records the corrected disposition. The third open review comment (stopCheckpointTimeout ahead of service-mode shutdown) is declined: the one-minute bound sits in front of the same path's pre-existing one-hour detached FinalizeTimeout window, and skipping the checkpoint for consumers that delete the partial would cement the RFC §4.5 defect the series plans to remove. Co-authored-by: Cursor <cursoragent@cursor.com>
Cross-model review (Sol, Grok, Kimi) found no merge-blocking bugs; this addresses every accepted quality finding: - stop_checkpoint_test pins error identity (context.Canceled on the rescue path, the injected sentinel on the guard path), not presence. - New TestArtifactVerdictAbsentOnPostSaveTeardownFailure executes the previously untested half of the verdict iff: a failure after a successful save carries no discard verdict. The save-failure test also asserts the atomic save left the output path untouched, and its sqlite comment now states the real ordering (cleanupDbDir runs after the failed save, which is also why only pebble can retry). - The external-cancel chaos test runs at worker counts 1 and 4, so bounded return also pins pool drain with siblings parked in queue.next(). - Two exits deliberately get no detached rescue, now documented in code and RFC: the post-collection forced checkpoint (a rescue would write the cleared token and force a full-plan re-run) and the supports-diff marker write (at most one interval of idempotent collection tail). - RFC corrections: producers narrowed to Close()-reachable paths (4.1), the 4.4 skew obligation restated as invariance (the finalize-deadline shape was already preservable), 5's chaos wording matched to the implemented oracles, and the queue.abort claim corrected from deadlock to promptness/hygiene (done() precedes the error return). Co-authored-by: Cursor <cursoragent@cursor.com>
…code The marker write is metadata for an unused feature; the exclusion needs one line, not a cost-benefit argument. Co-authored-by: Cursor <cursoragent@cursor.com>
… doc An external cancel can land inside the run-duration-expiry checkpoint write itself; the old parenthetical claimed the caller is always live there. The exits stay unrescued on purpose: the exposed window is one write wide and a stale token costs at most one interval of idempotent re-work, unlike the batch-wide windows the three rescues cover. Co-authored-by: Cursor <cursoragent@cursor.com>
Reverts the -timeout=30m bump. The full pkg/sync suite is ~12m of chaos corpora, soaks, and checkpoint-cut enumeration — the tiers nightly.yaml already runs under race via the sharded sweep, per its own fast-gate/nightly split. In short mode the slowest package is ~2m against go test's default 10m, the branch's deterministic pins all still run, and chaos-check keeps its bounded representative chaos slice as a PR gate. Co-authored-by: Cursor <cursoragent@cursor.com>
…tent Several comments grew a sentence per review round. Cut the restated mechanics and cost-benefit digressions; keep the invariant, the non-obvious choice, and the RFC pointer. Co-authored-by: Cursor <cursoragent@cursor.com>
Both CI edits on this branch were the wrong lever: first -timeout=30m, then -short. The harness is not this PR's to move — an unbounded test budget is the thing being protected, and PR-local slowness is a reason to make the PR's tests cheaper, not the gate looser. This PR's own cost: pkg/sync +5.5s, pkg/dotc1z +2.1s. The external cancel chaos test was 4 cells (2 transports x 2 worker counts, 8.9s); transport is duplicate coverage here (effects apply client-side, and TestChaosConnectorCancellationTerminatesAndColdResumes already runs both), and the worker count is the dimension that parks siblings in queue.next(). One cell, 3.3s. Co-authored-by: Cursor <cursoragent@cursor.com>
e8f1446 to
ccceb70
Compare
|
|
||
| closeErr := harness.Syncer.Close(ctx) | ||
| require.Error(t, closeErr) | ||
| require.ErrorIs(t, closeErr, ErrArtifactUnusable) |
There was a problem hiding this comment.
🟡 Suggestion: RFC §4.4 obligation 1 (attaching the sentinel must not change how an old runner classifies the error) is the only thing making the SDK-first window safe, but it is never asserted against a real *artifactVerdictError. The one pin — syncer_preservable_test.go's ErrArtifactUnusable row — uses fmt.Errorf("%w", ErrArtifactUnusable), a single-Unwrap chain, so it does not exercise the Unwrap() []error traversal that status.FromError/errors.As must follow through the production wrapper. This test already holds a genuine storage-produced verdict; adding require.False(t, IsSyncPreservable(closeErr)) here (and the corresponding require.True for the finalize-deadline shape) would pin the invariance directly. (confidence: high that the coverage gap is real; the invariance itself does hold today)
1 participant
Preserve-by-default artifact retention
Title:
sync: preserve-by-default artifact retentionWhy
Today a partial sync artifact survives only if the sync error matches an
allowlist (
ErrSyncNotComplete, timeout-class errors,IsSyncPreservable).Everything else — including routine external cancellation from a worker
drain, deploy, or operator stop — discards the checkpoint, and the next
attempt restarts from whatever was uploaded last, potentially hours of work
ago. Classifying connector errors to decide retention is also the wrong
dependency: connector error shapes are an uncontrolled, version-skewed
surface (see §2.4 of the RFC for the incident this caused).
This PR inverts the contract: retention is preserve-by-default, and the
only signal that permits discarding a partial artifact is a typed storage
verdict. Full design, invariants, and compatibility analysis in
docs/rfcs/0009-artifact-retention-preserve-by-default.md.The new contract
dotc1z.ErrArtifactUnusable— a sentinel attached by the storage layerwhen mutations existed but the output c1z was not rewritten: the file
on disk is stale relative to the run's progress (never torn — both engines
write atomically). Attached via a message-preserving wrapper, so operator-
facing text and existing string matching are byte-identical.
sync.ShouldDiscardSyncArtifact(err)— the seam API for runners: pass thejoin of the
Sync()andClose()errors; true only on the typed verdict.Everything else — cancellation, timeouts, all connector failures —
preserves. Escaping a poisoned checkpoint stays the retry budgets' job.
Producers are wired at the close/finalize failure paths of both storage
engines (SQLite
finalize, PebbleClose). Failures after a successfulsave (cleanup, teardown) deliberately do not carry the verdict — the
artifact is a faithful commit at that point.
Mechanical fixes in the parallel syncer (side-effect-only)
checkpoint only on
DeadlineExceededcauses now also checkpoint on cancelcauses, on a detached bounded context, so the local file is as fresh as
possible at every stop. The returned error is byte-for-byte unchanged on
every path; a checkpoint failure is logged, never joined.
longer logs "cancelling context due to error in action" — genuine action
failures are that line's only producers now. Cancel/abort semantics are
untouched.
What deliberately does not change (frozen surfaces)
Old runners vendor this SDK before their retention branch flips, so the
shapes they branch on are frozen and now pinned by tests:
IsSyncPreservable— frozen as-is (widening it would ship a silent policychange to unmigrated callers); doc comment marks it.
Sync()error shapes:ErrSyncNotCompletefor run-duration expiry,%w-wrappedErrTooManyWarnings, cancel/deadline visibility, connectorstatus codes passing through unaltered on the batch path.
lambda_transport:,logSummary:) — thehosted runner string-matches these today; the pins carry a comment naming
the consumer and lift when typed routing lands there.
The new symbols are dead code until a runner calls them, so this release is
inert for every existing consumer, including connector builds that pick up
the tag immediately (nothing here executes in a served connector).
Verification
produce, including cross-layer traversal through
Sync()/Close()joins(
artifact_retention_test.go,artifact_verdict_test.go).IsSyncPreservablebehavior table expanded withproduction error shapes; lambda marker assertions marked FROZEN
(
syncer_preservable_test.go,failure_test.go).and cold-resumes; every worker exits (no
queue.next()hang); positivecontrol that genuine action errors still log the cancel cause
(
chaos_cancellation_test.go).close; clean closes and retried closes do not.
Rollout
Lands in tandem with the hosted runner change per the RFC's merge cri