Audit publish commit and cleanup behavior
This commit is contained in:
@@ -101,7 +101,7 @@ These are prioritization signals, not findings:
|
||||
| 1: contract and boundary map | complete | Canonical contracts and focused internal docs read; ownership, stage-contract, lifecycle, scenario, area, and preliminary risk-to-test matrices recorded below. |
|
||||
| 2: runner and manifest | complete | Full/single-stage entry paths, every lifecycle outcome, both manifest models/transitions, save disagreement states, canonical invalidation boundaries, and runner lock lifetime reviewed. Focused app/manifest test and race commands passed. Confirmed `COR-001` and `RSK-001`; assigned `DUP-001`, `SIM-001`, `COM-001`, `TST-002`, and lock-release details to later stages. |
|
||||
| 3: paths and filesystem | complete | Canonical local/remote paths, every artifact source family, filesystem mutations, directory promotion, cleanup confinement, permissions, and lock mechanics reviewed. Focused normal and race commands passed. Confirmed `COR-002`, `COR-003`, `RSK-002`, `RSK-003`, and `RSK-004`; added `DUP-002` and `TST-003`, and refined `DUP-001`. |
|
||||
| 4: publish and cleanup | not_started | Assigned remote commit and cleanup scenarios below. |
|
||||
| 4: publish and cleanup | complete | Publish prerequisites/source families, deterministic upload order, every partial remote outcome, retry semantics, effective locks, status/restore interpretation, and automatic/manual cleanup gates reviewed. Focused stage/app/artifacts/storage tests passed. Confirmed `COR-004` through `COR-007` and `RSK-005`; added `ARC-003`, `COM-002`, and `TST-004`. |
|
||||
| 5: restore and previous state | not_started | Assigned restore and previous-cache scenarios below. |
|
||||
| 6: configuration and composition | not_started | Assigned configuration, CLI composition, and process-boundary areas below. |
|
||||
| 7: adapters and shared support | not_started | Assigned external-boundary and cancellation areas below. |
|
||||
@@ -126,7 +126,7 @@ has been mapped but not behaviorally audited.
|
||||
| Transcript stages | `internal/stage` plus tool adapters | 8 | assigned |
|
||||
| Extraction | `internal/stage`, Notarius adapter, `internal/fileops` | 9 | assigned |
|
||||
| Analyze and artifact dependencies | `internal/stage`, `internal/artifacts`, `internal/artifactpolicy` | 10 | assigned |
|
||||
| Publish and cleanup | `internal/stage`, `internal/app` | 4 | assigned |
|
||||
| Publish and cleanup | `internal/stage`, `internal/app` | 4 | reviewed |
|
||||
| Manifest state | `internal/manifest`, transition policy in `internal/app` | 2 | reviewed |
|
||||
| Artifacts, paths, and policy | `internal/artifacts`, `internal/artifactpolicy`, `internal/pathsafe` | 3 (resolution consumption revisited in 10) | reviewed |
|
||||
| Restore | `internal/app`, `internal/artifacts`, `internal/previouscache`, `internal/audio` | 5 | assigned |
|
||||
@@ -186,7 +186,7 @@ succeeded downstream work, while force pre-stales succeeded downstream work.
|
||||
| 7 `extract` | Final-trimmed source -> immutable index and configured lane outputs | Notarius executable/config/pipeline/timeout/output contracts; `notarius.Runner` | Disabled is explicit `notarius_disabled` self-skip; only current `ResumeValidator`; obsolete reruns, unsafe validation errors | Validates run-local receipt/bundle completely, promotes to unique immutable bundle, records checksums/contracts/provenance; identical repeated self-skip is stable. |
|
||||
| 8 `render` | Final and final-trimmed JSON -> two Markdown transcripts | Render and Seriatim fields; `seriatim.Runner` | Disabled returns a zero-disposition result with skip metadata, therefore runner-level success rather than explicit self-skip; no validator | Run-local text validates non-empty before canonical materialization when enabled. |
|
||||
| 9 `analyze` | Dynamic built-in, prepared, extraction, configured, and previous sources -> selected configured artifact outputs | Scriptorium artifact graph/selection; `scriptorium.Runner` | Missing config or no executable artifacts returns success with skip metadata; no validator | Topological run-local generation/reuse, validation, canonical outputs, deterministic metadata; static `Declares` omits dynamic outputs and several input families. |
|
||||
| 10 `publish` | Session/run state, selected output rules, locks, previous cache -> remote run/output/current objects | Publish/storage/selection fields; `storage.ObjectStore` | Disabled publish or run upload returns success with skip metadata; force cannot bypass locks; no validator | Deterministic uploads; `current/manifest.json` before `current/run_id.txt`; commit metadata gates cleanup. Static prerequisites omit extract because disabled extraction is valid and lane resolution enforces required extraction state when selected. |
|
||||
| 10 `publish` | Session/run state, selected output rules, locks, previous cache -> remote run/output/current objects | Publish/storage/selection fields; `storage.ObjectStore` | Disabled publish or run upload returns ordinary success with skip metadata, not a self-skip; force cannot bypass locks; no validator | Deterministic uploads; `current/manifest.json` before `current/run_id.txt`; post-commit local metadata gates cleanup. Static prerequisites omit extract because disabled extraction is valid and lane resolution enforces required extraction state when selected. |
|
||||
| 11 `notify` | No implemented persisted pipeline input/output | Optional `notify.Sender`; default no-op | Ordinary succeeded-record skip; no explicit self-skip or validator | Placeholder metadata and optional notification call; no returned output. `Declares` nevertheless advertises placeholder input/output paths. |
|
||||
|
||||
Configuration, adapters, skip policy, and dynamic outputs are not represented
|
||||
@@ -280,7 +280,7 @@ states and their later interpretation are:
|
||||
| Session `running` save fails after the run save | Run stage is `running`; session remains at its prior state and the stage does not execute. Forced downstream staleness is not durable. | A non-succeeded target retries; a prior success skips unless force/resume validation again requires replacement. |
|
||||
| Terminal session save fails after stage work | Both durable records remain `running`, although the stage may already have external or canonical effects. | The session stage reruns because `running` is not reusable. Stage-owned idempotency remains essential. |
|
||||
| Terminal run save fails after terminal session save | Session has the authoritative success, skip, failure, and downstream state; run stays `running`. | Execution resumes safely from the session, but the historical run remains inaccurate. |
|
||||
| Post-publish cleanup fails | Session stage outcomes remain terminal; a successful run save marks the invocation failed, while a failed run save leaves its prior overall `running` state. | A later invocation decides stages from the session and re-enters the cleanup gate; Stage 4 owns whether that retry is safe for every cleanup state. |
|
||||
| Post-publish cleanup fails | Session publish remains succeeded; a successful run save marks the invocation failed, while a failed run save leaves its prior overall `running` state. | A normal later invocation skips publish, so the execution-based cleanup gate does not retry. A forced publish or explicit manual clean is required; confirmed `COR-006`. |
|
||||
| Final overall run save fails | Session and per-stage run records are terminal, but overall run status remains `running`. | A later invocation skips or reruns from session state and does not repair the old run. |
|
||||
|
||||
Save errors are returned with both the stage error and persistence error when
|
||||
@@ -482,6 +482,124 @@ release failure, or assembled concurrent runners. `TST-003` assigns a narrow
|
||||
cross-package regression set to Stage 12; each confirmed finding below also
|
||||
identifies its smallest behavior-level test.
|
||||
|
||||
## Publish, Remote Commit, Lock, And Cleanup Conclusions
|
||||
|
||||
### Prerequisites, sources, and deterministic order
|
||||
|
||||
Enabled publish requires successful `prepare`, `transcribe`, `merge`, `polish`,
|
||||
`normalize`, `trim`, `render`, and `analyze` session records before any remote
|
||||
write. `extract` is deliberately absent: disabled extraction is valid, while an
|
||||
explicit required extraction output still fails artifact resolution unless the
|
||||
current extraction state is complete and trusted. Publish disabled or
|
||||
`upload_run` disabled returns ordinary stage success with `skipped` metadata;
|
||||
it is not the self-skip claimed by the focused stage document. `COM-002` records
|
||||
that contract mismatch and resolves the publish portion of `ARC-002`.
|
||||
|
||||
Publish builds all plans before uploading. Configured-artifact selection applies
|
||||
only to `narratio.artifact.*` rules; built-in and extraction rules are unaffected.
|
||||
Static and remote locks are merged before execution, with static rules winning.
|
||||
A matching lock intentionally skips an output even when required and preserves
|
||||
an existing published destination. An unselected configured output is recorded
|
||||
as unselected before lock handling. Missing unlocked optional outputs skip;
|
||||
missing unlocked required outputs fail. `--force` changes runner reuse and
|
||||
invalidation only: it never enters output resolution and cannot bypass either
|
||||
kind of lock.
|
||||
|
||||
The upload sequence is deterministic:
|
||||
|
||||
1. run files sorted by slash-normalized relative path;
|
||||
2. resolved published outputs in validated configuration order;
|
||||
3. previous-cache files sorted by relative path;
|
||||
4. the fixed `current/manifest.json`; and
|
||||
5. `current/run_id.txt`, always the final object-store call.
|
||||
|
||||
The run archive includes `manifest.json`, receipt/stderr diagnostics, and every
|
||||
other non-directory entry except exact `audio/**` and
|
||||
`extract/notarius-output/**` paths. Similarly named paths remain included.
|
||||
Audio is not uploaded. Previous-cache inclusion recursively includes every
|
||||
non-directory entry under the session's durable `previous` directory. Neither
|
||||
walk rejects symlink entries before storage opens the local path, which is the
|
||||
confidentiality defect in `COR-005`.
|
||||
|
||||
### Remote-current authority and partial outcomes
|
||||
|
||||
The storage boundary accepts explicit local paths and keys and implements upload
|
||||
as unconditional object replacement. It has no transaction, conditional write,
|
||||
generation check, rollback, or indeterminate-outcome query. Those are publish
|
||||
protocol responsibilities rather than storage policy. Every successful upload
|
||||
before the pointer is durable partial state; retry uploads the complete plan
|
||||
again and overwrites the same destinations. Partial run/session objects are not
|
||||
removed. `COR-001` separately means a later invocation can combine a new run ID
|
||||
with a stale remote run prefix.
|
||||
|
||||
`LoadCurrentState` reads the pointer first, then the fixed manifest, and rejects
|
||||
pointer/manifest run disagreement. Status reports that error as remote publish
|
||||
missing or unavailable; restore returns an error. Thus the pointer is the only
|
||||
accepted commit marker, but overwriting the fixed manifest before it does not
|
||||
preserve the prior readable commit. `COR-004` records that distinction.
|
||||
|
||||
| Publish boundary or durable remote state | Remote-current interpretation | Retry and cleanup consequence |
|
||||
| --- | --- | --- |
|
||||
| Planning/prerequisite/source resolution fails | No upload occurs; any prior pointer/manifest pair remains current. | Non-succeeded local publish reruns; cleanup is ineligible. |
|
||||
| A run, published-output, or previous-cache upload fails | Earlier objects remain or overwrite existing keys; the prior current pair is still intact because the fixed manifest was not reached. With no prior pair, no current state exists. | Retry unconditionally reuploads the whole plan. Cleanup is ineligible. |
|
||||
| Current-manifest construction fails | All planned data objects may exist, but the prior current pair remains intact. | Same retry behavior; cleanup is ineligible. |
|
||||
| Current-manifest upload definitely fails before remote acceptance | The prior pair remains intact; no pointer attempt occurs. | Same retry behavior; cleanup is ineligible. |
|
||||
| New current manifest is accepted while the old pointer remains | First publish has a manifest but no pointer and is not current. A replacement publish has old pointer/new manifest disagreement, so status and restore reject it rather than expose either run as current. | A successful retry repairs the pair; cleanup is ineligible until then. This includes the ordinary window between the final two uploads. |
|
||||
| Pointer upload returns an error | If not accepted, the disagreement above remains. If the service accepted the write but the response was lost, remote current is committed although local publish is marked failed. The interface cannot distinguish these outcomes. | Retry is conservative and overwrites again; automatic cleanup does not run for the failed local outcome. |
|
||||
| Pointer upload succeeds | Pointer and manifest identities agree and the new run is current. | The stage returns `uploaded=true` and `current_pointer_written=true`; automatic cleanup may become eligible. |
|
||||
| Remote commit succeeds, then terminal session/run persistence fails | Remote current remains committed. A session-save failure leaves local publish running and causes republish; a run-save failure leaves session publish succeeded and ordinary retry skips it. | The latter boundary can permanently miss automatic cleanup under `COR-006`; remote state itself remains authoritative. |
|
||||
|
||||
The current manifest is generated before commit with
|
||||
`current_pointer_written=false`, while the local post-upload result records the
|
||||
same field as true. Current-state readers correctly derive authority from the
|
||||
actual pointer and ignore that remote metadata, and cleanup correctly uses the
|
||||
post-commit local record. `ARC-003` records the future-consumer ambiguity rather
|
||||
than treating the precommit snapshot as remote authority.
|
||||
|
||||
### Lock authority and cleanup truth tables
|
||||
|
||||
Loaded static and remote locks are honored even for forced publish, but the
|
||||
remote lock store is an unsynchronized snapshot. Publish loads it before the
|
||||
local session lock, and lock add/remove performs an unconditional read-modify-
|
||||
write without a remote generation check or the runner's session lock. Concurrent
|
||||
operator mutations can lose one another, and a lock added after publish's load
|
||||
does not protect that in-flight upload. `RSK-005` records this limit; sequential
|
||||
lock behavior and static precedence are otherwise correct.
|
||||
|
||||
Automatic cleanup uses the following gate. “Confined” here means the existing
|
||||
lexical/final-target validation; symlinked ancestors remain the shared
|
||||
`COR-003` defect, and stale run-derived targets remain a `COR-001` consequence.
|
||||
|
||||
| Policy and local publish record | Automatic action |
|
||||
| --- | --- |
|
||||
| Both cleanup policies false | No cleanup lookup, deletion, or metadata mutation. |
|
||||
| Either policy true, but `publish` absent from this invocation's executed list | No cleanup, even if the session publish record is succeeded and records a commit. This one-shot behavior is `COR-006`. |
|
||||
| Publish executed but its session status is not succeeded | No cleanup. |
|
||||
| Publish succeeded, but publish/upload is disabled, `skipped=true`, `uploaded` is not true, `current_pointer_written` is not true, or pointer key is empty | No deletion; cleanup-skip reason is persisted. |
|
||||
| Publish succeeded with enabled upload, `uploaded=true`, pointer true/key present, and spool policy true | Validate the run-scoped spool target beneath the configured spool root, then delete it. Spool-only completion metadata is persisted. |
|
||||
| The same commit gate with workspace policy true | Validate the run-scoped work target beneath the workspace root, then delete it. Completion metadata is only mutated in memory and is lost under `COR-007`. |
|
||||
| Either requested deletion fails | Record best-effort failure metadata and fail the invocation. The succeeded publish remains reusable, so ordinary retry does not retry cleanup (`COR-006`). |
|
||||
|
||||
Manual `clean` is a separate explicit operator boundary and correctly does not
|
||||
require a prior publish or commit metadata. Session cleanup requires an explicit
|
||||
campaign/session resolution and deletes the confined session work and spool
|
||||
trees. `clean --all` rejects session/campaign selectors, deletes the confined
|
||||
workspace `work` tree and non-symlink spool-root children, and optionally clears
|
||||
the configured audio-cache namespace. Cache deletion requires `--clear-cache`;
|
||||
`--dry-run` performs validation and reports without removing. There is no clean
|
||||
force flag. Publish `--force` therefore cannot reach or weaken manual or
|
||||
automatic target validation.
|
||||
|
||||
Focused tests cover successful ordering and exact exclusions, source-family
|
||||
selection, required/optional/locked rules, force with a preloaded remote lock,
|
||||
pointer absence on selected failures, ordinary cleanup eligibility, unsafe
|
||||
final targets, and manual dry-run/scope behavior. They do not seed a prior
|
||||
current pair across a manifest/pointer failure, model ambiguous upload success,
|
||||
exercise retry after partial commit, reject symlink upload sources, assert
|
||||
workspace-cleanup metadata durability, retry failed cleanup, or model concurrent
|
||||
remote lock writers. `TST-004` assigns the smallest stateful protocol cases to
|
||||
Stage 12.
|
||||
|
||||
## Confirmed Findings
|
||||
|
||||
### `COR-001`: session identity initialization preserves stale invocation paths and accepts conflicting identity
|
||||
@@ -605,6 +723,143 @@ identifies its smallest behavior-level test.
|
||||
cleanup, restore/download, adapter, stage-materialization, and extraction
|
||||
consequences. `COR-002` owns lexical identity traversal separately.
|
||||
|
||||
### `COR-004`: precommit current-manifest replacement invalidates the prior readable commit
|
||||
|
||||
- Category: confirmed correctness/recovery defect.
|
||||
- Locations/invariant: `internal/stage/publish.go` upload order and
|
||||
`internal/artifacts/current_state.go` in `LoadCurrentState`. The pointer must
|
||||
be the sole commit point, and work before it must not destroy the previously
|
||||
committed state.
|
||||
- Evidence: publish unconditionally replaces the fixed
|
||||
`current/manifest.json`, then uploads `current/run_id.txt`. Current-state
|
||||
loading reads the pointer and fixed manifest and rejects unequal run IDs. A
|
||||
replacement publish that uploads the new manifest but has not yet written,
|
||||
or fails to write, the pointer therefore leaves old pointer/new manifest
|
||||
disagreement. Existing tests prove no pointer call follows selected failures
|
||||
but do not seed and preserve a prior current pair.
|
||||
- Realistic scenario: run A is current. Publishing run B reaches the current
|
||||
manifest, then the pointer upload fails or status/restore runs during the
|
||||
gap. Run A is no longer readable, run B is not committed, and both operator
|
||||
workflows report remote state unavailable until a later successful publish.
|
||||
An upload error after server acceptance also makes local outcome ambiguous.
|
||||
- Impact/likelihood/confidence: high recovery availability and integrity impact;
|
||||
upload failure likelihood is low per call but the disagreement window occurs
|
||||
on every replacement publish; high confidence from fixed keys, unconditional
|
||||
upload, and identity validation.
|
||||
- Estimated remediation scope and owner: medium publish/artifacts protocol
|
||||
change. Publish an immutable run-specific manifest first and make the final
|
||||
commit object select that immutable state, or use a versioned/conditional
|
||||
current representation that preserves the previous pair. Do not move commit
|
||||
policy into the generic storage adapter.
|
||||
- Test changes: use a stateful store seeded with run A, fail or pause every run
|
||||
B boundary, and assert run A remains readable until a successful final commit;
|
||||
add the first-publish and indeterminate-pointer-response cases. Existing
|
||||
pointer-last tests remain useful but are insufficient alone.
|
||||
- Dependencies: `COR-001` can misplace the immutable run prefix and must be
|
||||
fixed consistently. Stage 5 should consume the established current-state
|
||||
truth table rather than duplicate this publish root cause.
|
||||
|
||||
### `COR-005`: publish follows symlinked archive entries and can upload files outside its roots
|
||||
|
||||
- Category: confirmed correctness/security defect.
|
||||
- Locations/invariant: `internal/stage/publish.go` in
|
||||
`collectPublishRunFiles` and `collectPublishPreviousFiles`, plus
|
||||
`internal/adapters/storage` upload implementations. Run and previous archives
|
||||
must be confined to the enumerated local trees and contain eligible regular
|
||||
files only.
|
||||
- Evidence: both `WalkDir` collectors append every non-directory entry without
|
||||
rejecting symlinks or other special files. Storage then opens the recorded
|
||||
path (`os.Open` in the S3 backend and file reads in the fake), which follows a
|
||||
symlink final component. The run-manifest check also uses `os.Stat` and accepts
|
||||
any non-directory. Existing exclusion tests cover path names, not entry type.
|
||||
- Realistic scenario: an adapter, restored tree, local user, or compromised
|
||||
subprocess leaves `runs/<id>/logs/debug.log` as a symlink to a credential or
|
||||
unrelated private file. Publish uploads its contents under the apparently
|
||||
harmless run key. A symlink in durable previous cache has the same effect.
|
||||
- Impact/likelihood/confidence: critical confidentiality impact; low-to-moderate
|
||||
likelihood depending on workspace ownership and subprocess trust; high
|
||||
confidence from standard open semantics and the missing type checks.
|
||||
- Estimated remediation scope and owner: small-to-medium publish/file-boundary
|
||||
change. Enumerate and open regular files without following links, keep the
|
||||
opened object tied to the verified entry where platform support permits, and
|
||||
reject unsafe roots/ancestors consistently. Storage should continue accepting
|
||||
explicit paths rather than infer archive policy.
|
||||
- Test changes: create run, previous, and manifest symlinks to an outside
|
||||
sentinel and prove publish fails before uploading sentinel contents; include
|
||||
an entry-replacement race case if the implementation adopts path-based
|
||||
`Lstat` only. Existing exact exclusion/order tests should remain.
|
||||
- Dependencies: `COR-003` owns symlinked ancestors for mutation and cleanup;
|
||||
this finding is distinct because it is a publish read/exfiltration boundary.
|
||||
`RSK-004` affects who can create the malicious entry but is not required for
|
||||
exploitation by a trusted subprocess gone wrong.
|
||||
|
||||
### `COR-006`: automatic cleanup is one-shot and is not retried after postcommit failure
|
||||
|
||||
- Category: confirmed correctness/operational defect.
|
||||
- Locations/invariant: `internal/app/runner.go` around terminal publish saves
|
||||
and `runPostPublishCleanup`, and `post_publish_cleanup.go` in
|
||||
`publishStageRecordForCleanup`. An enabled cleanup policy should remain
|
||||
recoverable after a committed publish until its requested cleanup succeeds.
|
||||
- Evidence: cleanup requires `publish` to appear in the current invocation's
|
||||
`executed` list. Once the terminal session publish save succeeds, later
|
||||
ordinary invocations treat publish as already succeeded and omit it from that
|
||||
list. A terminal run-manifest save failure before cleanup, a cleanup deletion
|
||||
failure, or cleanup metadata-save failure can therefore leave requested data
|
||||
present while every normal retry silently bypasses cleanup. A forced publish
|
||||
or manual `clean` is the only retry path.
|
||||
- Realistic scenario: remote commit succeeds and the session publish record is
|
||||
saved, but saving the run record fails, so cleanup is never entered. The
|
||||
operator fixes the filesystem and reruns normally; publish skips and the
|
||||
sensitive spool remains despite `delete_audio_after_publish=true`. The same
|
||||
occurs when the first removal attempt itself fails.
|
||||
- Impact/likelihood/confidence: medium confidentiality/storage and operator-
|
||||
expectation impact; low-to-moderate lifetime likelihood around filesystem
|
||||
failures; high confidence from the execution-list gate and session reuse
|
||||
policy.
|
||||
- Estimated remediation scope and owner: medium app/manifest change. Persist a
|
||||
cleanup obligation/state independently of “publish executed this invocation”
|
||||
and retry it idempotently whenever the committed local record proves
|
||||
eligibility. Keep explicit policy and confinement checks on every attempt.
|
||||
- Test changes: inject a failure immediately after publish session success,
|
||||
fail each requested deletion/save once, then run normally and assert cleanup
|
||||
retries without republishing or deleting an uncommitted target. Existing
|
||||
commit-gate tests remain useful.
|
||||
- Dependencies: `TST-002` notes that run-save boundaries are not injectable;
|
||||
`COR-001` and `COR-003` still govern target identity/confinement. `COR-007`
|
||||
separately owns missing success evidence after work deletion.
|
||||
|
||||
### `COR-007`: successful workspace cleanup metadata is never persisted
|
||||
|
||||
- Category: confirmed correctness/diagnosability defect.
|
||||
- Locations/invariant: `internal/app/post_publish_cleanup.go` in the
|
||||
`workRequested` success path. Cleanup outcomes promised as manifest metadata
|
||||
must survive the invocation that performed the destructive action.
|
||||
- Evidence: spool-only cleanup sets completion metadata and saves the session
|
||||
manifest. When workspace cleanup is requested, the function removes the work
|
||||
directory, mutates `workdir_cleanup_deleted`, `cleanup_completed`, and
|
||||
`cleanup_skipped` only in memory, then returns without saving the session.
|
||||
The subsequent final run save copies identity fields, not the mutated session
|
||||
stage record. If both policies are enabled, the spool deletion metadata is
|
||||
lost with the same unsaved map. Existing cleanup tests assert paths only.
|
||||
- Realistic scenario: automatic cleanup successfully removes both run work and
|
||||
spool audio. The durable session and run records retain the pre-cleanup
|
||||
publish metadata, so status, incident review, or a future cleanup retry cannot
|
||||
distinguish completed cleanup from a path that was never considered.
|
||||
- Impact/likelihood/confidence: medium audit/recovery impact; occurs on every
|
||||
successful workspace cleanup; high confidence from the missing save and run
|
||||
record copy behavior.
|
||||
- Estimated remediation scope and owner: small app/manifest change. Persist one
|
||||
authoritative cleanup transition after requested deletions, with ordering
|
||||
that remains meaningful if saving fails after deletion. Coordinate that
|
||||
state with the retry obligation in `COR-006`.
|
||||
- Test changes: after workspace-only and combined cleanup, reload both durable
|
||||
ledgers and assert the chosen authoritative completion fields and deleted
|
||||
paths. Add a post-delete save-failure case to define retry/reporting behavior;
|
||||
no existing path-preservation test should be removed.
|
||||
- Dependencies: fixing `COR-006` and this finding together avoids inventing two
|
||||
competing cleanup state machines. `RSK-002` remains the crash-durability
|
||||
guarantee of the eventual manifest save.
|
||||
|
||||
### `RSK-001`: invocation audit records can remain indefinitely `running`
|
||||
|
||||
- Category: confirmed correctness/operational risk.
|
||||
@@ -744,6 +999,44 @@ identifies its smallest behavior-level test.
|
||||
adapter diagnostics, and Stage 13 should rank the risk against deployment
|
||||
assumptions.
|
||||
|
||||
### `RSK-005`: remote publish locks are race-prone snapshots
|
||||
|
||||
- Category: confirmed correctness/concurrency risk.
|
||||
- Locations/invariant: `internal/app/runner.go` remote-lock load order,
|
||||
`remote_locks.go`, `operator_locks.go`, and unconditional
|
||||
`storage.ObjectStore.Upload`. An operator lock intended to protect a published
|
||||
destination should not be silently lost or bypassed by concurrent control-
|
||||
plane activity.
|
||||
- Evidence: a publish-capable run loads and merges the remote lock document
|
||||
before acquiring its local session lock, then uses that in-memory slice for
|
||||
the entire invocation. Lock add/remove separately loads the full document,
|
||||
mutates it, and unconditionally uploads a replacement without a generation
|
||||
check or the runner lock. Two mutations can lose an update, and a lock added
|
||||
after publish's load cannot affect that in-flight upload. Existing tests are
|
||||
sequential and prove only static precedence, mutation validation, and loaded
|
||||
lock enforcement.
|
||||
- Realistic scenario: two operators lock different outputs at the same time;
|
||||
the last full-document upload drops the other lock. Or an operator locks a
|
||||
destination while a long run is approaching publish, but that run already
|
||||
loaded the old document and overwrites the destination despite the command
|
||||
reporting that it was locked.
|
||||
- Impact/likelihood/confidence: high protected-output integrity impact;
|
||||
low-to-moderate likelihood in multi-operator or multi-host use; high confidence
|
||||
in the lost-update/stale-read mechanics, moderate confidence that deployments
|
||||
rely on concurrent lock mutation because no concurrency contract is stated.
|
||||
- Estimated remediation scope and owner: medium app/storage-capability change.
|
||||
Define lock activation semantics and use object generation/ETag conditional
|
||||
replacement with retry, or a remote coordination primitive. Re-read or bind
|
||||
the effective generation at the publish commit boundary. Static locks remain
|
||||
immutable configuration and need no remote mutation protocol.
|
||||
- Test changes: add a version-aware stateful store and deterministic barriers
|
||||
for two add/remove writers plus add-during-publish; prove no lock is lost and
|
||||
define whether the in-flight publish aborts or observes a committed snapshot.
|
||||
Retain sequential force/static-lock tests.
|
||||
- Dependencies: `RSK-003` concerns only the local single-writer sentinel and
|
||||
cannot serialize other hosts or lock commands. `COR-004` may influence the
|
||||
appropriate remote compare-and-swap capability.
|
||||
|
||||
## Candidate Register
|
||||
|
||||
The remaining candidates require inspection by their named owners. Stages 2
|
||||
@@ -780,10 +1073,30 @@ and 3 have promoted their confirmed defects and risks into the register above.
|
||||
outcomes clear state, are reconsidered, and invalidate downstream work in the
|
||||
same way. Conversely, changing them to explicit self-skip could break valid
|
||||
pipeline continuation or cleanup semantics.
|
||||
- Confirmation owners: Stage 2 for runner truth table, Stage 4 for publish and
|
||||
cleanup, Stage 8 for ordinary disabled stages. Treat wording and behavior as
|
||||
unresolved until those flows are traced. Stage 2 confirmed that the two
|
||||
outcomes are deliberate and internally distinguishable at runner level.
|
||||
- Confirmation owners: Stage 2 confirmed the runner truth table. Stage 4
|
||||
confirmed publish's ordinary-success behavior is used deliberately by the
|
||||
cleanup gate, while `COM-002` owns its incorrect “self-skip” documentation.
|
||||
Stage 8 must decide the remaining ordinary disabled-stage behavior before
|
||||
this architectural candidate can close.
|
||||
|
||||
### `ARC-003`: committed and local manifests give `current_pointer_written` different meanings
|
||||
|
||||
- Category: architectural boundary/ownership candidate.
|
||||
- Evidence: publish must serialize `current/manifest.json` before the commit
|
||||
marker, so `publishMetadataPreview` records
|
||||
`current_pointer_written=false`. After pointer success, the local session and
|
||||
invocation results record the same field as true. Current-state readers use
|
||||
the actual pointer and ignore the remote field; automatic cleanup uses the
|
||||
local true value. Existing behavior is therefore safe for current consumers.
|
||||
- Realistic risk: a future status, restore, reconciliation, or cleanup feature
|
||||
may treat the committed remote manifest's field literally and report a valid
|
||||
commit as incomplete, while another consumer interprets the local copy as an
|
||||
assertion about remote state. Updating the fixed manifest after pointer would
|
||||
instead violate pointer-last ordering.
|
||||
- Confirmation owner: Stage 11 should decide whether this is best represented
|
||||
by distinct precommit/local metadata types, omission from the remote snapshot,
|
||||
or derivation from loaded pointer identity. Any change must preserve the
|
||||
protocol correction selected for `COR-004`; do not add a post-pointer upload.
|
||||
|
||||
### `TST-001`: full race baseline fails in the concurrent transcribe test
|
||||
|
||||
@@ -862,6 +1175,26 @@ and 3 have promoted their confirmed defects and risks into the register above.
|
||||
distinct invariant and reuse shared low-level tests across callers. Avoid
|
||||
duplicating every path spelling or persistence call sequence.
|
||||
|
||||
### `TST-004`: publish protocol tests do not preserve prior current state or exercise recovery
|
||||
|
||||
- Category: test-suite sufficiency candidate.
|
||||
- Evidence: focused publish tests strongly cover successful upload contents,
|
||||
pointer-last order, and absence of a pointer call after output/current-
|
||||
manifest failures. Cleanup tests cover ordinary commit metadata and path
|
||||
effects. They do not seed a prior pointer/manifest pair, inspect readability
|
||||
between the last two calls, retry a partial publish, model an upload accepted
|
||||
with an error response, reload workspace-cleanup metadata, retry cleanup, use
|
||||
symlink archive entries, or coordinate concurrent lock mutations.
|
||||
- Realistic risk: tests can continue proving “pointer was not advanced” while a
|
||||
failed attempt has already made the prior commit unreadable (`COR-004`), or
|
||||
can prove directories disappeared without detecting lost cleanup evidence
|
||||
and retry obligations (`COR-006`/`COR-007`). The same suite would not prevent
|
||||
partial fixes to `COR-005` or `RSK-005`.
|
||||
- Confirmation owner: Stage 12 should add the smallest stateful publish fake
|
||||
with barriers/version semantics and one behavior-level case per distinct
|
||||
invariant. Prefer extending current order/cleanup fixtures over duplicating
|
||||
all source-family tables, which already have good focused coverage.
|
||||
|
||||
### `SIM-001`: runner terminalization and persistence ordering lack a narrow owner
|
||||
|
||||
- Category: simplification candidate.
|
||||
@@ -890,6 +1223,25 @@ and 3 have promoted their confirmed defects and risks into the register above.
|
||||
named operation that makes the invariant obvious; add a concise rationale
|
||||
only where code structure cannot.
|
||||
|
||||
### `COM-002`: publish documentation calls ordinary success a self-skip
|
||||
|
||||
- Category: comment/clarity candidate.
|
||||
- Evidence: `docs/internal/stage-publish.md` says disabled publish or run upload
|
||||
can “self-skip.” The implementation returns zero disposition with skip
|
||||
metadata, so both manifests record succeeded and later normal runs reuse that
|
||||
success. True self-skip is a durable skipped outcome and is reconsidered by
|
||||
the runner. Focused tests check returned metadata but do not make the durable
|
||||
distinction obvious at the stage-document boundary.
|
||||
- Realistic risk: an operator or maintainer can expect enabling publish and
|
||||
rerunning normally to reconsider a skipped stage, or can change it to explicit
|
||||
self-skip and unintentionally alter downstream/cleanup behavior. The `publish`
|
||||
command currently forces execution, which mitigates the common explicit
|
||||
operator workflow but not the terminology.
|
||||
- Confirmation owner: documentation owner with Stage 8 coordination for the
|
||||
broader `ARC-002` vocabulary. State the durable outcome and reconsideration
|
||||
behavior precisely; change code only if a later lifecycle decision establishes
|
||||
different intended semantics.
|
||||
|
||||
## Candidate Classification Log
|
||||
|
||||
| Candidate signal | Classification | Reason |
|
||||
@@ -900,17 +1252,21 @@ and 3 have promoted their confirmed defects and risks into the register above.
|
||||
| Session `last_error` survives a later stage success | rejected as a current-state defect at Stage 2 | No production reader was found; current status and per-stage error are authoritative, so the field can serve as historical context. Reopen only if an operator surface treats it as the active error. |
|
||||
| Minimal loaded-manifest status/timestamp validation | rejected as a standalone Stage 2 finding | Unknown/non-succeeded statuses fail conservatively into execution, nil maps/records are normalized, and no realistic unsafe caller was established. Configured-versus-persisted identity conflict is separately confirmed in `COR-001`. |
|
||||
| Ignored runner lock-release error | confirmed as `RSK-003` at Stage 3 | The lock is an exclusive-create sentinel, not an OS-released lock. An unlink failure leaves the conflicting file while the runner suppresses the error; process death does the same without a release attempt. |
|
||||
| Two durable meanings of “skip” | runner portion confirmed, stage-contract conclusion deferred | Run action/status distinguishes idempotent skip from executed self-skip. Stages 4 and 8 must determine whether successful disabled outcomes and their wording are intentional. |
|
||||
| Two durable meanings of “skip” | runner and publish portions confirmed; ordinary stages deferred | Run action/status distinguishes idempotent skip from executed self-skip. Publish deliberately returns success metadata consumed by cleanup, but its focused document incorrectly calls this self-skip (`COM-002`). Stage 8 owns remaining disabled stages. |
|
||||
| Exported previous-artifact helper accepts traversal | consolidated into `COR-002` | Current production callers normalize first, but the helper's under-root contract is false in isolation. The identity/relative-segment boundary should be repaired once rather than as separate caller bugs. |
|
||||
| Built-in and previous resolvers do not re-hash manifest records | documented trust distinction; deferred to Stages 5 and 10 | Extraction explicitly requires checksum/contract/provenance validation and enforces it. Other source families explicitly use content validation and a previous-cache filesystem fallback; consumer/restore threat models must establish a stronger requirement before this becomes a finding. |
|
||||
| Promotion destination is path-based while source is handle-confined | consolidated into `COR-003` | Source hardening is strong, but destination ancestors share the same symlink/replacement root cause as ordinary writers and cleanup. |
|
||||
| Pointer is written last, so failed publish cannot advance current | rejected as sufficient atomic-commit proof at Stage 4 | The fixed current manifest is overwritten first. Pointer identity remains authoritative, but an old pointer paired with the new manifest is unreadable; confirmed `COR-004`. |
|
||||
| Remote current manifest records `current_pointer_written=false` | safe for current readers; retained as `ARC-003` | The snapshot is necessarily precommit, current-state loaders use actual pointer identity, and cleanup uses postcommit local metadata. The shared field remains ambiguous for future consumers. |
|
||||
| Post-publish cleanup is revisited by later invocations | corrected and confirmed as `COR-006` | The runner invokes the helper, but its gate requires publish in the current `executed` list. Once session publish is succeeded, ordinary retry skips publish and therefore skips cleanup. |
|
||||
| Manual clean should require publish commit metadata | rejected as a policy requirement at Stage 4 | Manual clean is explicit operator authorization with session/global scope, dry-run, cache opt-in, and confined targets. Publish execution/upload/pointer gates correctly apply only to automatic cleanup. |
|
||||
|
||||
## Unresolved Questions And Follow-Up
|
||||
|
||||
- Should startup/status reconcile abandoned `running` invocation records, mark
|
||||
them interrupted, or retain them as an explicitly accepted audit limitation?
|
||||
- Do Stage 3 path mechanics and Stage 4 publish/cleanup behavior confirm every
|
||||
concrete consequence of the stale run-derived fields in `COR-001`?
|
||||
- Which manifest model should retain run-scoped identity after `COR-001` is
|
||||
repaired, given that publish archives and automatic cleanup both consume it?
|
||||
- Which cross-platform root-relative filesystem primitive should own the
|
||||
symlink-safe mutation capability required by `COR-003` without moving stage
|
||||
policy into low-level `fileops`?
|
||||
@@ -922,13 +1278,15 @@ and 3 have promoted their confirmed defects and risks into the register above.
|
||||
- Do deployment packaging and service definitions already enforce an umask,
|
||||
ownership, or ACL that reduces `RSK-004`, and where should that contract be
|
||||
documented and tested?
|
||||
- Are disabled render/analyze/publish outcomes intentionally successful so
|
||||
pipeline continuation and optional outputs work, and do all operator views
|
||||
describe that distinction accurately?
|
||||
- Are disabled render/analyze outcomes intentionally successful so pipeline
|
||||
continuation works, and do all operator views describe that distinction
|
||||
accurately? Publish is now resolved as successful with a documentation gap.
|
||||
- Is `IODecl` intended only for display/tests, or should it own enforceable
|
||||
dependency declarations?
|
||||
- Does publish's omission of extract from its static prerequisite list combine
|
||||
safely with every configured extraction output rule and disabled extraction?
|
||||
- Should remote lock activation be snapshot-based or immediate, and what
|
||||
conditional-write capability can enforce the chosen rule across hosts?
|
||||
- Should committed remote manifests omit precommit-only metadata, or should
|
||||
local/remote publish metadata have distinct types after `COR-004` is fixed?
|
||||
- Which native CI runner limitations explain the absence of validation jobs in
|
||||
the tag-only release workflow? Stage 12 owns the automation conclusion.
|
||||
|
||||
@@ -1033,3 +1391,43 @@ No accepted risks or final audit conclusions are recorded yet.
|
||||
concrete callers. Stages 5 and 10 must decide whether ordinary manifest and
|
||||
previous-cache checksum trust is sufficient. Stages 6, 11, and 12 own
|
||||
compatibility, simplification, and durable regression coverage respectively.
|
||||
|
||||
### Stage 4
|
||||
|
||||
- Contracts and code reviewed: architecture publish/cleanup/force invariants;
|
||||
focused publish, storage, workspace, manifest, operations, CLI, and
|
||||
troubleshooting contracts; publish stage prerequisites, artifact catalog and
|
||||
output resolution, selection, locks, run/previous collection, every upload,
|
||||
current snapshot/pointer generation, storage upload semantics, effective
|
||||
remote-lock loading/mutation, current-state discovery/identity validation,
|
||||
status/restore entry interpretation, runner terminal ordering, automatic
|
||||
cleanup, cleanup target validation, and manual session/global/cache cleanup.
|
||||
- Graph/source evidence: call traces from publish and lock commands into
|
||||
storage; exact source for current-state readers, cleanup gates, lock mutation,
|
||||
and runner persistence; complete focused publish/cleanup/lock/current-state
|
||||
test inventory; fallback source/text inspection for the generic stage method,
|
||||
S3 `os.Open`, and non-code contracts where graph modeling was insufficient.
|
||||
- Validation: `go test -count=1 ./internal/stage ./internal/app
|
||||
./internal/artifacts ./internal/adapters/storage` passed (`internal/stage`
|
||||
0.440 s, `internal/app` 0.749 s, `internal/artifacts` 0.028 s,
|
||||
`internal/adapters/storage` 0.019 s; 1.86 s command wall time).
|
||||
- Conclusions: publish plans fully before writing and uploads sorted run files,
|
||||
configuration-ordered outputs, sorted previous files, current manifest, then
|
||||
the pointer last. Output family, required/optional, selection, lock, exclusion,
|
||||
retry, existing-object, and force behavior is recorded above. Remote-current
|
||||
and automatic-cleanup truth tables resolve scenarios 5 and 7 at every
|
||||
boundary; manual cleanup is correctly a separate explicit authorization.
|
||||
- Findings: confirmed `COR-004`, `COR-005`, `COR-006`, `COR-007`, and
|
||||
`RSK-005`; added `ARC-003`, `COM-002`, and `TST-004`; resolved publish's
|
||||
portion of `ARC-002` and corrected Stage 2's cleanup-retry conclusion.
|
||||
- Explicit no-finding conclusions: pointer is unequivocally the final upload
|
||||
and only accepted current marker; failure before current-manifest publication
|
||||
preserves any prior current pair; successful retry is idempotent by
|
||||
unconditional replacement; static and loaded remote locks, including required
|
||||
outputs, survive force; disabled extraction is safe because only explicit
|
||||
extraction rules resolve it; manual clean does not need publish metadata;
|
||||
storage correctly remains policy-neutral and consumes explicit paths/keys.
|
||||
- Follow-up: Stage 5 should reuse the remote-current truth table when auditing
|
||||
restore mechanics. Stage 11 owns metadata vocabulary/duplication decisions;
|
||||
Stage 12 owns the smallest stateful commit, cleanup-retry, symlink-read, and
|
||||
remote-lock concurrency tests.
|
||||
|
||||
Reference in New Issue
Block a user