diff --git a/vero/src/vero/candidate_repository/git.py b/vero/src/vero/candidate_repository/git.py index d0fbb9ba..2d80c0db 100644 --- a/vero/src/vero/candidate_repository/git.py +++ b/vero/src/vero/candidate_repository/git.py @@ -137,6 +137,28 @@ async def create( raise CandidateRepositoryError( result.stderr or "failed to initialize candidate repository" ) + + # A writer killed mid-transaction (the run that died in finalization was + # SIGKILLed while capturing a candidate) leaves a stale + # refs/vero/.../.lock behind, and every later update-ref on that + # ref dies with "Unable to create ...: File exists" on a lock nobody + # holds, which is unrecoverable without a human deleting the file. + # Sweeping is only safe here and nowhere else: create() runs once while + # the session repository is being opened, before this process has any + # writer of its own, so a lock present at this instant is left over from + # a dead process. The same sweep on the capture path would be much worse + # than the bug, because it could delete the lock a concurrent writer is + # actively holding and let two ref updates clobber each other. For the + # same reason it stays inside refs/vero/candidates/, and not the whole of + # refs/vero/: the repository's own refs and packed-refs.lock are vero's to + # leave alone, and refs/vero/export/ and refs/vero/incoming/ are transient + # refs held by `vero session export` and its import counterpart, which run + # outside the session lock and so can genuinely be holding a live lock at + # the moment another process opens the repository. + candidate_refs = repository_path / "refs" / "vero" / "candidates" + for stale_lock in sorted(candidate_refs.rglob("*.lock")): + stale_lock.unlink(missing_ok=True) + records_path.mkdir(parents=True, exist_ok=True) instance = cls( root=root, diff --git a/vero/src/vero/evaluation/engine.py b/vero/src/vero/evaluation/engine.py index d4350479..ccdf6cc1 100644 --- a/vero/src/vero/evaluation/engine.py +++ b/vero/src/vero/evaluation/engine.py @@ -439,6 +439,41 @@ async def _execute_record( # original, as BudgetStore.refund already does. raise cancellation from refund_error raise + except Exception as unexpected: + # The same last line of defence, for the failure side. The evaluator + # converts everything the *backend* raises into the two typed errors + # above, but not everything it does itself: an OSError from the + # result-directory mkdir or the running-manifest write it performs + # before its own try block, or from a shielded _persist_failure + # inside its handlers, unwinds as a bare exception none of the + # handlers above match. The reservation then stays charged with no + # evaluation to show for it, so the budget is short by that amount + # for the rest of the run and every later evaluation is silently + # starved -- over a long search that is the difference between + # finishing and running out of budget mid-way. + # + # Only the refund is recoverable here: a bare exception carries no + # evaluation id, so there is no record to load, exactly as with the + # raw cancellation above. Refunding a cost the run may never have + # spent is bounded by the clamp in BudgetLedger.refund, which never + # restores more than the budget's own total. + if charged: + try: + await asyncio.shield( + self.budget_ledger.refund( + backend_id, + request.evaluation_set, + cost, + principal, + ) + ) + except Exception as refund_error: + # Same guard as the handlers above, for the same reason: + # chain the refund's own failure and re-raise the original so + # the caller still sees what actually stopped the evaluation, + # as BudgetStore.refund already does. + raise unexpected from refund_error + raise # Shielded for the same reason as the two handlers above, and one more: # the budget was charged for an evaluation that has now actually run, so # a cancellation landing inside _record would leave the ledger counting @@ -485,7 +520,7 @@ async def _execute_record( ) ) except Exception as refund_error: - # Same guard as the three handlers above, for the same + # Same guard as the four handlers above, for the same # reason: chain the refund's own failure and re-raise the # original, as BudgetStore.refund already does. raise failure from refund_error diff --git a/vero/src/vero/evaluation/store/persistence.py b/vero/src/vero/evaluation/store/persistence.py index e8c3ca49..acc23b82 100644 --- a/vero/src/vero/evaluation/store/persistence.py +++ b/vero/src/vero/evaluation/store/persistence.py @@ -44,17 +44,43 @@ def _atomic_write_json(path: Path, value: Any) -> None: ) temporary_path = Path(temporary_name) try: - with os.fdopen(descriptor, "w", encoding="utf-8") as handle: + handle = os.fdopen(descriptor, "w", encoding="utf-8") + except BaseException: + # This is the only window where we still own the raw descriptor: once + # os.fdopen succeeds the file object owns it and closes it exactly once + # when the block below exits, so closing it again from a shared failure + # path was a double close. Three writers reach this helper concurrently + # through asyncio.to_thread, and on CPython that second close can land + # on an unrelated descriptor that has since been handed the same number. + os.close(descriptor) + temporary_path.unlink(missing_ok=True) + raise + try: + with handle: json.dump(value, handle, ensure_ascii=False, indent=2) handle.write("\n") handle.flush() os.fsync(handle.fileno()) os.replace(temporary_path, path) - except BaseException: + # Fsyncing the file only makes its bytes durable; the rename that + # publishes them is a change to the parent directory, so a hard power + # loss or container kill right here can leave the old contents behind or + # no file at all. Every durable artifact vero has commits through this + # one helper (the evaluation manifest, the per-case checkpoints, + # database.json, budgets.json, the session manifest, and the disclosure + # ledger), so fsync the directory as well. Some platforms refuse to open + # a directory for fsync, and this is a durability upgrade rather than + # part of the write itself, so an OSError from either step is ignored + # instead of failing a write that already landed. try: - os.close(descriptor) + directory_descriptor = os.open(path.parent, os.O_RDONLY) + try: + os.fsync(directory_descriptor) + finally: + os.close(directory_descriptor) except OSError: pass + except BaseException: temporary_path.unlink(missing_ok=True) raise @@ -447,8 +473,33 @@ def load_reconciled( """Load the index and repair it from canonical completed evaluations.""" existed = database_path.exists() - database = cls.load_from_file(database_path) if existed else cls(id=database_id) + database: EvaluationDatabase | None = None + if existed: + try: + database = cls.load_from_file(database_path) + except Exception as error: + # A crash in the middle of a write leaves a truncated index, and + # raising here bricked the session for good: every later run of + # it died on load even though this file is only a cache of the + # per-evaluation manifests that get reconciled in below. Rebuild + # from those instead of refusing to start, and log it, since the + # operator otherwise has no way to tell that an index they will + # find rewritten on disk was ever damaged. + logger.warning( + "Rebuilding unreadable evaluation database %s from the " + "per-evaluation manifests: %s", + database_path, + error, + ) + # Treat the damaged file as absent so the repaired index is + # written back even when no new evaluation appeared on disk. + existed = False + if database is None: + database = cls(id=database_id) if database.id != database_id: + # A readable index naming a different session is a real "wrong + # session" signal (a copied or crossed session directory), never a + # torn write, so it stays a hard error rather than a rebuild. raise ValueError( f"evaluation database belongs to {database.id!r}, not {database_id!r}" ) diff --git a/vero/src/vero/gateway/inference.py b/vero/src/vero/gateway/inference.py index c024ed05..48764f31 100644 --- a/vero/src/vero/gateway/inference.py +++ b/vero/src/vero/gateway/inference.py @@ -1066,8 +1066,56 @@ async def finish() -> None: finally: await asyncio.shield(finish()) + # Step the generator once before handing it over, because a generator + # that has never run is a reservation nobody owns. StreamingResponse + # races stream_response against listen_for_disconnect inside one anyio + # task group, so a client that has already gone away cancels the scope + # while stream_response is still awaiting its first send(), and the + # body iterator is never stepped at all: the generator body never + # runs, its finally never runs, and store.complete() is never called. + # A never-started async generator is invisible to asyncio's asyncgen + # finalization too, so nothing else cleans up behind it. Each such + # disconnect silently burned one concurrency permit until reserve() + # waited on the semaphore forever, with no timeout and no traceback: + # just a gateway that stopped answering, which is how two runs died. + # One anext() leaves the generator suspended inside its try, so the + # permit belongs to a started generator whose finally does run when it + # is closed. Cost: the response headers now wait on the first SSE + # event, so time to first byte grows by one event. + stream = chunks() + head: list[bytes] = [] + replay: BaseException | None = None + try: + head.append(await anext(stream)) + except StopAsyncIteration: + # An upstream that ended without a single event: the generator has + # already run its finally, so the reservation is already settled + # and the body below is simply empty. + pass + except asyncio.CancelledError: + # The request itself is being torn down, so there is no client + # left to hand a partial stream to. finish() has already run. + raise + except BaseException as error: + # A first chunk that fails has to keep failing *inside* the + # response rather than out of this handler: raising here would + # turn the truncated 200 that client SDKs retry as a connection + # error into a 500 that none of them retry, which would quietly + # change which trials fail. finish() has already run in the + # generator's finally, so all this still owes the client is the + # same broken stream it saw before. + replay = error + + async def primed() -> AsyncIterator[bytes]: + for chunk in head: + yield chunk + if replay is not None: + raise replay + async for chunk in stream: + yield chunk + return StreamingResponse( - chunks(), + primed(), status_code=upstream.status_code, headers=response_headers, media_type="text/event-stream", diff --git a/vero/src/vero/harbor/backend.py b/vero/src/vero/harbor/backend.py index 567e5cc4..8d6aeb72 100644 --- a/vero/src/vero/harbor/backend.py +++ b/vero/src/vero/harbor/backend.py @@ -432,6 +432,14 @@ async def export_case_resources( cache = Path(configured_cache) if not (cache / "index.json").is_file(): cache.parent.mkdir(parents=True, exist_ok=True) + # Deliberately still an mkdtemp and not a deterministic sibling. A + # stable staging path would let a retry reclaim the tree a killed + # attempt abandoned, which is the idempotent thing to want, but it + # also means two processes over one configured cache path would + # rmtree each other's staging tree mid-materialization. mkdtemp is + # what keeps them apart, and losing a case-resource tree under + # another process's feet is a worse failure than leaving a stale + # `..XXXXXXXX` directory behind to be swept later. temporary = Path( tempfile.mkdtemp( dir=cache.parent, @@ -923,7 +931,24 @@ def _trial_artifacts( text = payload.decode("utf-8") sanitized = sanitize_text(text, self._secrets()) if sanitized != text: - resolved.write_text(sanitized, encoding="utf-8") + # Write a sibling and rename over the trial file. The + # in-place write truncated the record first, so a crash + # (or a full disk) part way through left a trial that + # was neither the original nor the redacted version, + # and the original was already unrecoverable. The + # rename is atomic, so whoever reads the record next + # sees exactly one of the two. + redacting = resolved.with_name( + f".{resolved.name}.vero-redacting" + ) + redacting.write_text(sanitized, encoding="utf-8") + # Carry the record's own mode over, which the in-place + # write kept for free. The agent context copies these + # files in and only ever strips write bits from them, + # so a redacted trial left at this process's umask + # could become one the optimizer cannot read. + shutil.copymode(resolved, redacting) + os.replace(redacting, resolved) except (OSError, UnicodeDecodeError): pass media_type = mimetypes.guess_type(resolved.name)[0] @@ -1532,16 +1557,51 @@ async def evaluate( "provisioning; check that every ancestor directory is " "traversable by the dropped user" ) - result = await context.workspace.sandbox.run( - command, - cwd=context.workspace.project_path, - timeout=request.limits.timeout_seconds, - env=self._environment( - context.evaluation_id, finalization=context.finalization - ), - run_as=self.config.harness_user, - ) - await staging.download("jobs", attempt_jobs_dir) + sub_run_completed = False + try: + result = await context.workspace.sandbox.run( + command, + cwd=context.workspace.project_path, + timeout=request.limits.timeout_seconds, + env=self._environment( + context.evaluation_id, finalization=context.finalization + ), + run_as=self.config.harness_user, + ) + sub_run_completed = True + finally: + # Salvage whatever trials the sub-run did finish before it + # died. This download used to sit after the run, so anything + # that raised (a sandbox that went away, or the cancellation + # quiesce_agent_evaluations delivers at finalization) skipped + # it and the staging area's teardown a few lines below deleted + # the only copy of the completed trials: exactly the evidence + # needed to diagnose the failure, and exactly the work a + # resume would want to reuse. Shielded so a cancelled task + # still gets them off the sandbox. The cost is that a failed + # sub-run now pays one extra copy off the sandbox. + try: + await asyncio.shield(staging.download("jobs", attempt_jobs_dir)) + except BaseException: + if sub_run_completed: + # Nothing is unwinding, so this is the same download + # failure that always propagated from here. + raise + # A salvage that fails on its own must never replace the + # sub-run's own failure: a dead sandbox fails this copy + # too, and that error is the symptom, not the cause the + # caller needs to see. BaseException and not Exception + # because the failure being unwound is very often a + # cancellation, and a second cancellation delivered while + # this await is parked would otherwise raise CancelledError + # out of the `finally` and take the sub-run's own error + # with it. Swallowing it here does not lose the cancel: + # the task stays in its cancelling state and the next + # await in the caller sees it again. + logger.warning( + "unable to salvage Harbor trials from a failed sub-run", + exc_info=True, + ) stdout = self.sanitize_error(result.stdout) stderr = self.sanitize_error(result.stderr) attempts.append((result, stdout, stderr)) diff --git a/vero/src/vero/harbor/build/compiler.py b/vero/src/vero/harbor/build/compiler.py index 9f2cefce..58df9f38 100644 --- a/vero/src/vero/harbor/build/compiler.py +++ b/vero/src/vero/harbor/build/compiler.py @@ -78,6 +78,16 @@ # harbor/deployment.py's own default. DEFAULT_EVALUATION_DRAIN_SECONDS = 600.0 +# Author and committer date of the baseline commit. Pinned rather than left to +# the wall clock because that commit's sha is an identity, not a timestamp: it +# is written into the Harbor session manifest as selection.baseline_version, and +# the sidecar refuses to come up against a preserved session whose manifest +# disagrees. With the date unpinned, recompiling a byte-identical baseline tree +# produced a different sha every time, so a recompile could never be brought up +# against durable state and hours of search work had nothing to resume from. +# Nobody reads this date; the value only has to be constant. +BASELINE_COMMIT_DATE = "2000-01-01T00:00:00+00:00" + def _backend_id(partition: str) -> str: return f"harbor-{partition}" @@ -182,6 +192,19 @@ def git(*arguments: str) -> str: check=True, capture_output=True, text=True, + # The identity is pinned above so the commit does not depend on the + # host's git config; the two dates are pinned here for the same + # reason, and they matter more. Git folds both into the commit + # object, so leaving them to `now` (or to whatever the caller + # happened to export) made the returned sha differ on every compile + # of identical content. That sha is selection.baseline_version in + # the session manifest, an identity used to match a resumed run + # against its preserved session, not a timestamp anyone reads. + env={ + **os.environ, + "GIT_AUTHOR_DATE": BASELINE_COMMIT_DATE, + "GIT_COMMITTER_DATE": BASELINE_COMMIT_DATE, + }, ) return result.stdout.strip() @@ -520,6 +543,46 @@ def _render(template: str, destination: Path, **context) -> None: ) +def _previous_gateway_tokens(output: Path) -> tuple[str, str, str] | None: + """The producer, evaluation and finalization tokens of the compile already + sitting in this output directory, or None when they cannot all be recovered. + + Re-minting the three tokens on every compile is what made a recompile + un-resumable: the two evaluation tokens are part of the Harbor backend + config, that config is hashed into the backend provenance the session + manifest pins, so fresh tokens mean a fresh digest and the sidecar refuses to + come up against the preserved session even though nothing about the + evaluation changed. The tradeoff of reusing them is that a token then + outlives the single compile that minted it on this host; that is bounded by + the per-run scoped volume the compiled tree lives in, which dies with the + run. All three or none: reusing a subset changes the digest anyway. + """ + launch = output / "environment/gateway/launch.json" + serve = output / "environment/sidecar/serve.json" + if not launch.is_file() or not serve.is_file(): + return None + try: + producer = json.loads(launch.read_text(encoding="utf-8"))["producer_api_key"] + # Every harbor backend in a compile carries the same pair, so the first + # one is enough. A command-backend compile never writes them at all, and + # that build simply re-mints: there is nothing on disk to match. + backend = next( + iter(json.loads(serve.read_text(encoding="utf-8"))["backends"].values()) + ) + tokens = ( + producer, + backend["inference_gateway_token"], + backend["inference_gateway_finalization_token"], + ) + except (AttributeError, KeyError, StopIteration, TypeError, ValueError): + # A half-written or differently shaped previous compile is not an error + # here, it just means this compile mints its own tokens. + return None + if not all(isinstance(token, str) and token for token in tokens): + return None + return tokens[0], tokens[1], tokens[2] + + def compile_harbor_task( config: HarborBuildConfig, output_dir: Path | str, @@ -539,11 +602,22 @@ def compile_harbor_task( task_source_path = Path(config.task_source) if task_source_path.exists(): protected.append(task_source_path.resolve()) + # Everything is written into a sibling .partial directory and swapped into + # place at the very end (see the rename below), so a compile that dies + # halfway can no longer leave a half-built tree in `output` that the next + # step reads as complete. + staging = output.parent / f"{output.name}.partial" for path in protected: if output == path or output.is_relative_to(path) or path.is_relative_to(output): raise ValueError( f"output directory {output} overlaps protected source {path}" ) + # The staging directory is wiped before use, so a protected source living + # inside it has to be rejected here for the same reason. + if staging == path or path.is_relative_to(staging): + raise ValueError( + f"output directory {output} overlaps protected source {path}" + ) # Imported here, not at module scope: deployment pulls in the whole runtime # stack, and harbor/__init__ imports this package before it, so a top-level # import would only work by accident of partial-initialization ordering. @@ -578,9 +652,12 @@ def compile_harbor_task( raise ValueError( "declared task credentials are missing: " + ", ".join(missing) ) - if output.exists(): - shutil.rmtree(output) - environment_dir = output / "environment" + # A leftover .partial is the corpse of an earlier failed compile, not state + # anyone resumes from, so it is cleared rather than reused. `output` itself is + # left alone until the swap. + if staging.exists(): + shutil.rmtree(staging) + environment_dir = staging / "environment" sidecar_dir = environment_dir / "sidecar" gateway_dir = environment_dir / "gateway" environment_dir.mkdir(parents=True) @@ -646,15 +723,31 @@ def compile_harbor_task( Path(config.task_source), sidecar_dir / "task-source", ) - producer_inference_token = ( - generate_inference_token() if config.inference_gateway is not None else None - ) - evaluation_inference_token = ( - generate_inference_token() if config.inference_gateway is not None else None - ) - finalization_inference_token = ( - generate_inference_token() if config.inference_gateway is not None else None + # Reuse the previous compile's tokens when recompiling into an output + # directory that already holds them, because minting new ones moves the + # backend config digest the session manifest pins and so locks a recompile + # out of its own preserved session. Read from `output`, which is still the + # last complete compile at this point: this one is being built in `staging` + # and does not land until the swap at the end. + previous_tokens = ( + _previous_gateway_tokens(output) + if config.inference_gateway is not None + else None ) + if config.inference_gateway is None: + producer_inference_token = None + evaluation_inference_token = None + finalization_inference_token = None + elif previous_tokens is not None: + ( + producer_inference_token, + evaluation_inference_token, + finalization_inference_token, + ) = previous_tokens + else: + producer_inference_token = generate_inference_token() + evaluation_inference_token = generate_inference_token() + finalization_inference_token = generate_inference_token() deployment = _deployment_config( config, baseline_version=baseline, @@ -808,8 +901,8 @@ def compile_harbor_task( "overlay_present": overlay_present, "overlay_excludes": overlay_excludes, } - _render("task.toml.j2", output / "task.toml", **context) - _render("instruction.md.j2", output / "instruction.md", **context) + _render("task.toml.j2", staging / "task.toml", **context) + _render("instruction.md.j2", staging / "instruction.md", **context) _render("Dockerfile.main.j2", environment_dir / "Dockerfile", **context) _render( "Dockerfile.sidecar.j2", @@ -828,13 +921,22 @@ def compile_harbor_task( **context, ) _render("seed.sh.j2", environment_dir / "main/seed.sh", **context) - _render("test.sh.j2", output / "tests/test.sh", **context) - _render("solve.sh.j2", output / "solution/solve.sh", **context) + _render("test.sh.j2", staging / "tests/test.sh", **context) + _render("solve.sh.j2", staging / "solution/solve.sh", **context) for script in ( environment_dir / "main/seed.sh", - output / "tests/test.sh", - output / "solution/solve.sh", + staging / "tests/test.sh", + staging / "solution/solve.sh", ): script.chmod(0o755) + # The tree is complete, so swap it in. Only here is the previous compile + # dropped, which is the whole point: until this line a crash leaves the last + # known-good tree in place instead of a plausible-looking ruin. The cost is + # peak disk, both compiles exist side by side for the duration of the rename, + # so a build that bakes a large task source or vero checkout needs room for + # two of them. + if output.exists(): + shutil.rmtree(output) + staging.rename(output) logger.info("Compiled Harbor task at %s from baseline %s", output, baseline) return output diff --git a/vero/src/vero/harbor/build/templates/seed.sh.j2 b/vero/src/vero/harbor/build/templates/seed.sh.j2 index 121b3c63..e2c4d262 100644 --- a/vero/src/vero/harbor/build/templates/seed.sh.j2 +++ b/vero/src/vero/harbor/build/templates/seed.sh.j2 @@ -3,17 +3,31 @@ set -eu if [ ! -d {{ layout.target_git }} ]; then cp -a {{ layout.seed_repo }}/. {{ layout.target_repo }}/ -fi {% if overlay_present %} -# Inject baked-in workspace overlay (agent definitions, skills, config, ...). -cp -a {{ layout.overlay }}/. {{ layout.target_repo }}/ + # Inject baked-in workspace overlay (agent definitions, skills, config, ...). + # Inside the first-boot guard deliberately: this script is the main service's + # command, so Docker runs it again whenever it restarts a crashed container, + # and by then the workspace volume already holds the optimizer's own work. + # Re-copying the baked overlay over that silently reverted whatever it had + # changed under these paths. + cp -a {{ layout.overlay }}/. {{ layout.target_repo }}/ {% endif %} +fi find {{ layout.target_repo }} -path {{ layout.target_evals }} -prune -o -exec chown agent:agent {} + git config --system --add safe.directory {{ layout.target_repo }} -printf '%s\n' '/.evals/' >> {{ layout.target_git_exclude }} +# The same restart re-ran these appends, so every boot added another copy of the +# same ignore lines and .git/info/exclude grew without bound for the life of a +# run. grep -qxF asks for the exact line; on the very first boot it fails with a +# missing-file error (hence the redirect of stderr) and the append below creates +# the file, exactly as it did before. +if ! grep -qxF '/.evals/' {{ layout.target_git_exclude }} 2>/dev/null; then + printf '%s\n' '/.evals/' >> {{ layout.target_git_exclude }} +fi {% for path in overlay_excludes %} -printf '%s\n' '/{{ path }}/' >> {{ layout.target_git_exclude }} +if ! grep -qxF '/{{ path }}/' {{ layout.target_git_exclude }} 2>/dev/null; then + printf '%s\n' '/{{ path }}/' >> {{ layout.target_git_exclude }} +fi {% endfor %} {% if inference_gateway %} # Codex otherwise treats the built-in OpenAI provider as WebSocket-capable. diff --git a/vero/src/vero/harbor/cli.py b/vero/src/vero/harbor/cli.py index 90562c56..9be14ae5 100644 --- a/vero/src/vero/harbor/cli.py +++ b/vero/src/vero/harbor/cli.py @@ -69,6 +69,29 @@ def _request( ) from error +def _fsync_directory(path: Path) -> None: + """Flush a directory entry so a preceding ``os.replace`` is itself durable. + + Fsyncing the temporary file only promises its *contents* survive a crash; + the rename that publishes it lives in the parent directory, so a host that + dies right after the replace can come back with the file missing entirely. + These files are the record of a run that took hours, so the extra syscall is + free by comparison. Directory fsync is not supported everywhere, and losing + an export over a filesystem that refuses it would be strictly worse than + the weaker durability, so OSError is swallowed. + """ + try: + descriptor = os.open(path, os.O_RDONLY) + except OSError: + return + try: + os.fsync(descriptor) + except OSError: + pass + finally: + os.close(descriptor) + + def _atomic_write_bytes(path: Path, payload: bytes) -> None: path.parent.mkdir(parents=True, exist_ok=True) descriptor, temporary_name = tempfile.mkstemp( @@ -83,6 +106,7 @@ def _atomic_write_bytes(path: Path, payload: bytes) -> None: file.flush() os.fsync(file.fileno()) os.replace(temporary, path) + _fsync_directory(path.parent) finally: temporary.unlink(missing_ok=True) @@ -122,6 +146,7 @@ def _download( file.flush() os.fsync(file.fileno()) os.replace(temporary, destination) + _fsync_directory(destination.parent) finally: temporary.unlink(missing_ok=True) @@ -1108,19 +1133,23 @@ def finalize_command(token_file, output): "/finalize", headers={"Authorization": f"Bearer {token}"}, ) + # Both files below go through _atomic_write_bytes rather than write_text + # because they are the *only* record of the held-out result: a crash partway + # through a plain write leaves a truncated reward.json that Harbor still + # reads, and once the sidecar is gone there is nothing left to re-derive it + # from. Same bytes as before, just published by rename instead of in place. destination = Path(output) - destination.parent.mkdir(parents=True, exist_ok=True) # reward.json keeps only the reward map Harbor consumes. - destination.write_text( - json.dumps(result["rewards"], indent=2) + "\n", - encoding="utf-8", + _atomic_write_bytes( + destination, + (json.dumps(result["rewards"], indent=2) + "\n").encode("utf-8"), ) # Persist the full verification result (the shipped flag, verifier errors, # baseline rewards) alongside it, so "did anything ship, and if not why" is # answerable without re-running — reward.json alone drops those signals. - (destination.parent / "finalization.json").write_text( - json.dumps(result, indent=2) + "\n", - encoding="utf-8", + _atomic_write_bytes( + destination.parent / "finalization.json", + (json.dumps(result, indent=2) + "\n").encode("utf-8"), ) click.echo(json.dumps(result, indent=2)) @@ -1221,17 +1250,43 @@ def export_session_command( token = read_admin_token(token_file) headers = {"Authorization": f"Bearer {token}"} - finalization = _request("POST", "/finalize", headers=headers) - status = _request("GET", "/status") output = Path(output).expanduser().resolve() report_output = Path(report_output).expanduser().resolve() status_output = Path(status_output).expanduser().resolve() finalization_output = Path(finalization_output).expanduser().resolve() + # This POST stays, even though it is the second finalize of the run: the + # generated verifier script runs `vero harbor finalize` immediately before + # this command, so reusing that file would save a call. It is not worth what + # it costs. --finalization-output defaults under /logs, and in the topology + # these benchmarks actually run (harness_user unset, so candidate harness + # code shares the sidecar uid) /logs is writable by the thing being scored, + # so a candidate could plant its own held-out result and have it flow into + # the archived record, the status file and the report. reward.json could not + # be forged that way, because finalize_command writes it from its own POST, + # but a forged archive is still a forged number. The right home for a + # reusable finalization record is the admin volume, next to the baseline.json + # written by measure_baseline, which is where the trusted copy already lives; + # that change belongs with the finalize-caching work, not here. + finalization = _request("POST", "/finalize", headers=headers) + status = _request("GET", "/status") with tempfile.TemporaryDirectory(prefix="vero-harbor-session-") as directory: temporary = Path(directory) - downloaded = temporary / "sidecar-session.tar.gz" - _download("/session/export", downloaded, headers=headers) - session = extract_harbor_session_archive(downloaded, temporary / "extracted") + # Download straight to --output instead of parking the archive in the + # temporary directory until the very end. Extraction, trace redaction + # and report generation all sit between the download and the augmented + # rewrite, and any one of them failing used to take the archive down + # with the temporary directory, losing the only durable copy of the + # winning candidate. --output therefore briefly holds the un-augmented + # sidecar archive: that is the deliberate tradeoff, a raw archive with + # candidates/repository.git and database.json in it beats no archive. + # Both writes are atomic (_download and create_harbor_session_archive + # each rename into place), so a reader never sees a partial file. + output.parent.mkdir(parents=True, exist_ok=True) + # Drop any checksum left by an earlier export before the archive under + # it changes, so a companion file never describes different bytes. + output.with_name(f"{output.name}.sha256").unlink(missing_ok=True) + _download("/session/export", output, headers=headers) + session = extract_harbor_session_archive(output, temporary / "extracted") encoded_finalization = ( json.dumps(finalization, ensure_ascii=False, indent=2) + "\n" ).encode("utf-8") @@ -1270,13 +1325,18 @@ def export_session_command( asyncio.run(generate_experiment_report(session, generated_report)) create_harbor_session_archive(session, output) digest = file_sha256(output) - _atomic_write_bytes(report_output, generated_report.read_bytes()) - _atomic_write_bytes(status_output, encoded_status) - _atomic_write_bytes(finalization_output, encoded_finalization) + # Persist the checksum first, before the report and status writes. Each + # of those can fail on its own (a full /logs, an unreadable report), and + # a digest we computed but never wrote down leaves an archive nobody can + # validate afterwards, which is the one thing the companion file exists + # to prevent. _atomic_write_bytes( output.with_name(f"{output.name}.sha256"), f"{digest} {output.name}\n".encode("ascii"), ) + _atomic_write_bytes(report_output, generated_report.read_bytes()) + _atomic_write_bytes(status_output, encoded_status) + _atomic_write_bytes(finalization_output, encoded_finalization) click.echo( json.dumps( { diff --git a/vero/src/vero/optimization/optimizer.py b/vero/src/vero/optimization/optimizer.py index 324d3996..24708639 100644 --- a/vero/src/vero/optimization/optimizer.py +++ b/vero/src/vero/optimization/optimizer.py @@ -120,22 +120,37 @@ async def evaluate( if await self.workspace.is_dirty() else await self.workspace.current_version() ) - self._count += 1 - candidate = Candidate( - id=f"{self.proposal.id}:trial:{self._count}", - version=version, - parent_id=self._last_candidate_id, - created_at=datetime.now(UTC), - description=description, - metadata={ - **self.proposal.metadata, - "producer_id": self.proposal.producer_id, - "proposal_id": self.proposal.id, - "round": self.round_number, - "trial": self._count, - }, - ) - await self.optimizer._capture_candidate(candidate, self.workspace) + # A producer re-evaluates an unchanged checkpoint routinely: a retry + # after a flaky harness, or a second look at a partition it already + # scored. Minting a fresh trial candidate each time filled the durable + # archive with distinct ids over byte-identical content, so a resume + # could not tell they were the same work. When the workspace has not + # moved off the last trial candidate's version, reuse that candidate; + # it is already captured and already the head of both the trial list + # and the parent chain, so leaving captured False keeps neither from + # gaining a duplicate entry. Only the identity is deduplicated: the + # evaluation below still runs, so no measurement is skipped. + last_trial = self._trial_candidates[-1] if self._trial_candidates else None + if last_trial is not None and last_trial.version == version: + candidate = last_trial + captured = False + else: + self._count += 1 + candidate = Candidate( + id=f"{self.proposal.id}:trial:{self._count}", + version=version, + parent_id=self._last_candidate_id, + created_at=datetime.now(UTC), + description=description, + metadata={ + **self.proposal.metadata, + "producer_id": self.proposal.producer_id, + "proposal_id": self.proposal.id, + "round": self.round_number, + "trial": self._count, + }, + ) + await self.optimizer._capture_candidate(candidate, self.workspace) request = self.optimizer._request(candidate, requested_set) try: result = await self.optimizer.engine.evaluate( diff --git a/vero/src/vero/report.py b/vero/src/vero/report.py index 59c4fc34..c8a7b270 100644 --- a/vero/src/vero/report.py +++ b/vero/src/vero/report.py @@ -6,8 +6,11 @@ import hashlib import importlib.resources import json +import logging import mimetypes +import os import subprocess +import tempfile from pathlib import Path from typing import Any @@ -19,28 +22,47 @@ from vero.sidecar.session import HarborSessionManifest from vero.sidecar.verifier import VerificationResult +logger = logging.getLogger(__name__) + _MAX_EMBEDDED_ARTIFACT_BYTES = 5_000_000 _MAX_EMBEDDED_ARTIFACTS_BYTES = 50_000_000 _MAX_DIFF_CHARACTERS = 500_000 -def _read_events(path: Path) -> list[dict[str, Any]]: +def _read_events(path: Path) -> tuple[list[dict[str, Any]], int]: + """Read the session event log, tolerating a torn tail. + + A run that is SIGKILLed mid-append leaves a half-written last line in + events.jsonl, and report generation is what session export calls, so raising + on that one torn line used to throw away the entire export including the + winning candidate. Skip whatever cannot be parsed instead, log it so the loss + is visible, and return the count alongside the events so the report can say + how many records went missing. Decoding replaces invalid bytes rather than + raising for the same reason: a SIGKILL in the middle of a multi-byte + character must cost one event, not the whole session. + """ if not path.is_file(): - return [] + return [], 0 events: list[dict[str, Any]] = [] + skipped = 0 for line_number, line in enumerate( - path.read_text(encoding="utf-8").splitlines(), 1 + path.read_text(encoding="utf-8", errors="replace").splitlines(), 1 ): if not line.strip(): continue try: event = RuntimeEvent.model_validate_json(line) except Exception as error: - raise ValueError( - f"invalid runtime event on line {line_number}: {error}" - ) from error + skipped += 1 + logger.warning( + "Skipping corrupt runtime event on line %d of %s: %s", + line_number, + path, + error, + ) + continue events.append(event.model_dump(mode="json")) - return events + return events, skipped def _git_diff( @@ -329,7 +351,7 @@ async def _build_report_data( item["artifacts"] = artifacts evaluation_data.append(item) - events = _read_events(session_dir / "events.jsonl") + events, skipped_event_lines = _read_events(session_dir / "events.jsonl") return { "schema_version": 1, "generated_from": str(session_dir), @@ -337,6 +359,10 @@ async def _build_report_data( "candidates": candidate_data, "evaluations": evaluation_data, "events": events, + # Carry the torn-line count into the payload so a report built from a + # SIGKILLed session admits that its timeline has holes instead of quietly + # presenting a truncated history as complete. + "skipped_event_lines": skipped_event_lines, "traces": traces, } @@ -524,6 +550,36 @@ async def generate_experiment_report( ) html = template.replace("__VERO_REPORT_DATA__", _safe_json(data)) destination.parent.mkdir(parents=True, exist_ok=True) - destination.write_text(html, encoding="utf-8") + # Writing the document in place truncated experiment.html first, so a death + # part-way through the write left a half-written report that a browser renders + # as a blank page while looking, to every later reader, like the export + # succeeded. Stage the whole document in a sibling temporary file and rename it + # over the destination, so the destination only ever holds a complete report + # and a previous good report survives a failed regeneration. + descriptor, temporary_name = tempfile.mkstemp( + dir=destination.parent, + prefix=f".{destination.name}.", + suffix=".tmp", + ) + temporary = Path(temporary_name) + try: + handle = os.fdopen(descriptor, "w", encoding="utf-8") + except BaseException: + # The only window where the raw descriptor is still ours to close: after + # os.fdopen succeeds the file object owns it and the with-block below + # closes it exactly once, so closing it again from a shared failure path + # would be a double close, and a second close can land on an unrelated + # descriptor that has since been handed the same number. + os.close(descriptor) + temporary.unlink(missing_ok=True) + raise + try: + with handle: + handle.write(html) + handle.flush() + os.fsync(handle.fileno()) + os.replace(temporary, destination) + finally: + temporary.unlink(missing_ok=True) return destination diff --git a/vero/src/vero/runtime/events.py b/vero/src/vero/runtime/events.py index 248fa533..3890408d 100644 --- a/vero/src/vero/runtime/events.py +++ b/vero/src/vero/runtime/events.py @@ -6,6 +6,7 @@ import inspect import json import logging +import os from collections.abc import Awaitable, Callable from datetime import UTC, datetime from pathlib import Path @@ -113,8 +114,17 @@ def __init__(self, path: Path): async def __call__(self, event: RuntimeEvent) -> None: line = json.dumps(event.model_dump(mode="json"), ensure_ascii=False) + record = f"{line}\n".encode() async with self._lock: self.path.parent.mkdir(parents=True, exist_ok=True) - with self.path.open("a", encoding="utf-8") as handle: - handle.write(line) - handle.write("\n") + # The record and its newline go out as one write to a binary append + # handle, not as two text writes: a large event whose payload flushed + # without its trailing newline fused with the next record and corrupted + # two events instead of one. The fsync then costs real latency on every + # event, and we pay it deliberately, because previously the tail of the + # log only survived to the last close and this file is the sole forensic + # record when the process vanishes without a traceback. + with self.path.open("ab") as handle: + handle.write(record) + handle.flush() + os.fsync(handle.fileno()) diff --git a/vero/src/vero/runtime/session.py b/vero/src/vero/runtime/session.py index 99978f3a..131472f0 100644 --- a/vero/src/vero/runtime/session.py +++ b/vero/src/vero/runtime/session.py @@ -3,8 +3,12 @@ from __future__ import annotations import asyncio +import fcntl import hashlib import json +import os +from collections.abc import Iterator +from contextlib import contextmanager from dataclasses import dataclass, field from datetime import UTC, datetime from enum import Enum @@ -125,6 +129,55 @@ def normalize_timestamps(cls, value: datetime) -> datetime: return value.astimezone(UTC) +@contextmanager +def _exclusive_session_lock(session_dir: Path) -> Iterator[None]: + """Hold an advisory lock so two processes cannot drive one session. + + Nothing stopped a second `vero run` from attaching to a session directory + that already had a live process in it, and an operator has every reason to + try: a run takes hours, and when it goes quiet there is no way from the + outside to tell a wedged process from a dead one. Both processes then derive + the same pending work from the same manifest, and each holds its own + in-memory copy of the budget ledger, so the same budget is spent twice, the + manifest writes overwrite one another, and the recorded history describes + neither run. + + An advisory ``flock`` is the right instrument precisely because the kernel + drops it when the holder exits, however it exits. A relaunch after a real + crash (the case this is all meant to support) acquires it immediately, and + there is no cleanup step anyone can forget. The pid written into the file is + only a breadcrumb for the refusal message: the lock itself lives in the open + file description, never in the file's contents. + """ + + lock_path = session_dir / "run.lock" + descriptor = os.open(lock_path, os.O_CREAT | os.O_RDWR, 0o644) + try: + # Only a would-block means contention. Any other OSError (a filesystem + # that cannot lock, say) propagates as itself rather than being reported + # as a second process, which would be a false diagnosis of the one thing + # this lock exists to diagnose. + try: + fcntl.flock(descriptor, fcntl.LOCK_EX | fcntl.LOCK_NB) + except BlockingIOError as error: + holder = os.read(descriptor, 64).decode(errors="replace").strip() + raise RuntimeError( + f"session {session_dir} is already being run by another process" + f" (pid {holder or 'unknown'} holds {lock_path}), and two" + " processes over one session double-spend its budgets and" + " interleave their history. If that process is gone, the kernel" + " has already released the lock and this run can be retried." + ) from error + os.ftruncate(descriptor, 0) + os.write(descriptor, f"{os.getpid()}\n".encode()) + yield + finally: + # Closing the descriptor is what releases the lock. The file itself is + # left behind deliberately: an unheld lock file is inert, and deleting + # it would race with the next acquisition it is supposed to guard. + os.close(descriptor) + + @dataclass class OptimizationSession: """Own the durable state and lifecycle of one optimization run.""" @@ -358,165 +411,182 @@ async def run( skip_baseline_evaluation: bool = False, max_proposals: int | None = None, ) -> OptimizationResult: - manifest = self.load_manifest() if self.manifest_path.exists() else None - if baseline is None: - if manifest is not None and manifest.baseline is not None: - baseline = manifest.baseline - elif self.baseline is not None: - baseline = self.baseline - else: - baseline = Candidate.from_version( - await self.optimizer.workspace.current_version() + # Advisory-locked for the whole of run(): see _exclusive_session_lock + # for why a second process on one session directory is refused here + # rather than left to corrupt the budgets and the history. + with _exclusive_session_lock(self.session_dir): + manifest = self.load_manifest() if self.manifest_path.exists() else None + if baseline is None: + if manifest is not None and manifest.baseline is not None: + baseline = manifest.baseline + elif self.baseline is not None: + baseline = self.baseline + else: + baseline = Candidate.from_version( + await self.optimizer.workspace.current_version() + ) + if manifest is None: + manifest = self._initial_manifest(baseline) + if manifest.id != self.id: + raise ValueError("session manifest ID does not match runtime session") + if manifest.backend_id != self.optimizer.backend_id: + raise ValueError("session backend does not match the persisted manifest") + if manifest.candidate_repository_family != self.candidate_repository.family: + raise ValueError( + "session candidate repository does not match the persisted manifest" ) - if manifest is None: - manifest = self._initial_manifest(baseline) - if manifest.id != self.id: - raise ValueError("session manifest ID does not match runtime session") - if manifest.backend_id != self.optimizer.backend_id: - raise ValueError("session backend does not match the persisted manifest") - if manifest.candidate_repository_family != self.candidate_repository.family: - raise ValueError( - "session candidate repository does not match the persisted manifest" - ) - if ( - manifest.candidate_repository_format_version - != self.candidate_repository.format_version - ): - raise ValueError( - "session candidate repository format does not match the " - "persisted manifest" - ) - backend = self.optimizer.engine.backends.resolve(self.optimizer.backend_id) - if manifest.backend != backend.provenance: - raise ValueError( - "session backend configuration does not match the persisted manifest" - ) - if manifest.evaluation_plan != self.optimizer.evaluation_plan: - raise ValueError( - "session evaluation plan does not match the persisted manifest" - ) - if manifest.objective != self.optimizer.objective: - raise ValueError("session objective does not match the persisted manifest") - if manifest.run != self._run_spec(): - # A session directory is bound to the protocol it was created with, - # on purpose: reusing it under another would let its recorded history - # misdescribe what produced the candidates. The usual cause is an - # edited config -- changing `optimizer.model` between two commands is - # enough, since the model is part of the producer's identity -- and - # the bare message named neither the field nor the remedy. - differing = ", ".join(self._differing_fields(manifest.run, self._run_spec())) - raise ValueError( - "session run protocol does not match the persisted manifest" - f" (differs in: {differing}). A session records the protocol it was" - " created with, so a changed strategy, producer, model, or proposal" - " count needs a fresh session directory -- or discard the existing" - f" state with `vero session clear {self.session_dir} --yes`." - ) - if manifest.parameters != self.optimizer.parameters: - raise ValueError( - "session evaluation parameters do not match the persisted manifest" - ) - if manifest.limits != self.optimizer.limits: - raise ValueError( - "session evaluation limits do not match the persisted manifest" + if ( + manifest.candidate_repository_format_version + != self.candidate_repository.format_version + ): + raise ValueError( + "session candidate repository format does not match the " + "persisted manifest" + ) + backend = self.optimizer.engine.backends.resolve(self.optimizer.backend_id) + if manifest.backend != backend.provenance: + raise ValueError( + "session backend configuration does not match the persisted manifest" + ) + if manifest.evaluation_plan != self.optimizer.evaluation_plan: + raise ValueError( + "session evaluation plan does not match the persisted manifest" + ) + if manifest.objective != self.optimizer.objective: + raise ValueError("session objective does not match the persisted manifest") + if manifest.run != self._run_spec(): + # A session directory is bound to the protocol it was created with, + # on purpose: reusing it under another would let its recorded history + # misdescribe what produced the candidates. The usual cause is an + # edited config -- changing `optimizer.model` between two commands is + # enough, since the model is part of the producer's identity -- and + # the bare message named neither the field nor the remedy. + differing = ", ".join(self._differing_fields(manifest.run, self._run_spec())) + raise ValueError( + "session run protocol does not match the persisted manifest" + f" (differs in: {differing}). A session records the protocol it was" + " created with, so a changed strategy, producer, model, or proposal" + " count needs a fresh session directory -- or discard the existing" + f" state with `vero session clear {self.session_dir} --yes`." + ) + if manifest.parameters != self.optimizer.parameters: + raise ValueError( + "session evaluation parameters do not match the persisted manifest" + ) + if manifest.limits != self.optimizer.limits: + raise ValueError( + "session evaluation limits do not match the persisted manifest" + ) + if manifest.seed != self.optimizer.seed: + raise ValueError( + "session evaluation seed does not match the persisted manifest" + ) + if manifest.baseline is None or ( + manifest.baseline.id, + manifest.baseline.version, + ) != (baseline.id, baseline.version): + raise ValueError("session baseline does not match the persisted manifest") + + # Deliberately not clearing `failure` here. This write is a rerun's very + # first durable act, so clearing it destroyed the only recorded + # explanation of the previous death before anyone had read it: the run + # that lost 67 held-out trials to an upstream budget cap had its + # diagnosis erased by the relaunch that was supposed to recover it. The + # clear happens on the COMPLETED write below instead. The tradeoff is + # that a stale failure now lingers on the manifest of a run that is + # mid-flight, which is the right way round: `status` already says + # RUNNING, so the failure reads as "how the last attempt ended", and + # keeping a stale explanation costs nothing next to losing a real one. + manifest = manifest.model_copy( + update={ + "status": SessionStatus.RUNNING, + "updated_at": datetime.now(UTC), + } ) - if manifest.seed != self.optimizer.seed: - raise ValueError( - "session evaluation seed does not match the persisted manifest" + await self._save_manifest(manifest) + assert self.events is not None + await self.events.emit( + session_id=self.id, + kind="session_started", + payload={"baseline_candidate_id": baseline.id}, ) - if manifest.baseline is None or ( - manifest.baseline.id, - manifest.baseline.version, - ) != (baseline.id, baseline.version): - raise ValueError("session baseline does not match the persisted manifest") - - manifest = manifest.model_copy( - update={ - "status": SessionStatus.RUNNING, - "updated_at": datetime.now(UTC), - "failure": None, - } - ) - await self._save_manifest(manifest) - assert self.events is not None - await self.events.emit( - session_id=self.id, - kind="session_started", - payload={"baseline_candidate_id": baseline.id}, - ) - try: - result = await self.optimizer.run( - baseline=baseline, - skip_baseline_evaluation=skip_baseline_evaluation, - max_proposals=max_proposals, - ) - except BaseException as error: - failure = SessionFailure( - type=f"{type(error).__module__}.{type(error).__name__}", - message=str(error) or type(error).__name__, - ) - await self._save_manifest( - manifest.model_copy( - update={ - "status": SessionStatus.FAILED, - "updated_at": datetime.now(UTC), - "failure": failure, - } + try: + result = await self.optimizer.run( + baseline=baseline, + skip_baseline_evaluation=skip_baseline_evaluation, + max_proposals=max_proposals, + ) + except BaseException as error: + failure = SessionFailure( + type=f"{type(error).__module__}.{type(error).__name__}", + message=str(error) or type(error).__name__, + ) + await self._save_manifest( + manifest.model_copy( + update={ + "status": SessionStatus.FAILED, + "updated_at": datetime.now(UTC), + "failure": failure, + } + ) + ) + await self.events.emit( + session_id=self.id, + kind="session_failed", + payload={"error_type": failure.type, "message": failure.message}, ) + raise + + best = result.best + completed = manifest.model_copy( + update={ + "status": SessionStatus.COMPLETED, + "updated_at": datetime.now(UTC), + # Only a completed run may drop the previous attempt's failure: + # by here the session has a result, so the old explanation is + # genuinely obsolete rather than merely inconvenient. + "failure": None, + "best_candidate_id": ( + best.request.candidate.id if best is not None else None + ), + "best_evaluation_id": best.id if best is not None else None, + "final_baseline_evaluation_id": ( + result.final_baseline.id + if result.final_baseline is not None + else None + ), + "final_evaluation_id": ( + result.final.id if result.final is not None else None + ), + } ) + await self._save_manifest(completed) await self.events.emit( session_id=self.id, - kind="session_failed", - payload={"error_type": failure.type, "message": failure.message}, + kind="session_completed", + payload={ + "best_candidate_id": completed.best_candidate_id, + "best_evaluation_id": completed.best_evaluation_id, + "evaluation_count": len(result.evaluations), + "status": "completed", + "baseline_candidate_id": result.baseline.request.candidate.id, + "baseline_objective": ( + result.baseline.objective.value + if result.baseline.objective is not None + else None + ), + "best_objective": ( + best.objective.value + if best is not None and best.objective is not None + else None + ), + "final_objective": ( + result.final.objective.value + if result.final is not None + and result.final.objective is not None + else None + ), + }, ) - raise - - best = result.best - completed = manifest.model_copy( - update={ - "status": SessionStatus.COMPLETED, - "updated_at": datetime.now(UTC), - "best_candidate_id": ( - best.request.candidate.id if best is not None else None - ), - "best_evaluation_id": best.id if best is not None else None, - "final_baseline_evaluation_id": ( - result.final_baseline.id - if result.final_baseline is not None - else None - ), - "final_evaluation_id": ( - result.final.id if result.final is not None else None - ), - } - ) - await self._save_manifest(completed) - await self.events.emit( - session_id=self.id, - kind="session_completed", - payload={ - "best_candidate_id": completed.best_candidate_id, - "best_evaluation_id": completed.best_evaluation_id, - "evaluation_count": len(result.evaluations), - "status": "completed", - "baseline_candidate_id": result.baseline.request.candidate.id, - "baseline_objective": ( - result.baseline.objective.value - if result.baseline.objective is not None - else None - ), - "best_objective": ( - best.objective.value - if best is not None and best.objective is not None - else None - ), - "final_objective": ( - result.final.objective.value - if result.final is not None - and result.final.objective is not None - else None - ), - }, - ) - return result + return result diff --git a/vero/src/vero/runtime/wandb.py b/vero/src/vero/runtime/wandb.py index 9166c724..6b2da0c6 100644 --- a/vero/src/vero/runtime/wandb.py +++ b/vero/src/vero/runtime/wandb.py @@ -8,6 +8,7 @@ import logging import os import tempfile +import threading from pathlib import Path from typing import Any from uuid import uuid4 @@ -207,10 +208,20 @@ def __call__(self, event: RuntimeEvent) -> None: evaluation_id = str(payload["evaluation_id"]) if evaluation_id in self.logged_evaluations: return - self.run.log(payload, step=self.next_step) - self.logged_evaluations.add(evaluation_id) + # Persist the step before spending it. The old order logged at + # `next_step` and only then wrote state, so a process killed inside + # that window came back believing the step was still free and handed + # W&B a step it had already used: the two points collide on one x + # value and the history is unreadable exactly where the crash was. + # This is deliberately at-most-once now -- a crash between the write + # and the log loses this telemetry point instead of duplicating one, + # and for a live-watch series a missing point is much the cheaper + # failure than a colliding one. + step = self.next_step self.next_step += 1 + self.logged_evaluations.add(evaluation_id) self._save_state() + self.run.log(payload, step=step) return if event.kind == "session_completed": self.run.summary.update(event.payload) @@ -268,6 +279,14 @@ def __init__( self.log_traces = log_traces self.artifacts = ArtifactStore(session_dir / "artifacts") self.state_path = "wandb/state.json" + # The telemetry poller calls log_inference_usage/ship_request_logs from a + # worker thread (`asyncio.to_thread`) while the evaluation listener calls + # __call__ on the loop thread, so the two genuinely interleave rather + # than merely looking like they might. Every mutate-then-save body below + # runs under this lock: without it one caller can read `next_step`, be + # descheduled, and let the other spend the same step, or a save can + # serialize a half-updated view and drop the other caller's mutation. + self._lock = threading.Lock() stored_run_id = None if self.artifacts.path(self.state_path).exists(): state = self.artifacts.read_json(self.state_path) @@ -275,11 +294,17 @@ def __init__( self.next_step = int(state.get("next_step", len(self.logged_evaluations))) stored_run_id = state.get("run_id") self._shipped_request_logs = dict(state.get("request_log_files", {})) + # The gateway usage ledger is the dedupe key for inference telemetry, + # and holding it only in memory meant a sidecar restart forgot which + # counters it had already sent: the resumed run re-logged an + # identical point at a fresh step and could not say what cumulative + # usage it had reported. It resumes from state.json like the rest. + self._last_inference_usage = dict(state.get("inference_usage", {})) else: self.logged_evaluations: set[str] = set() self.next_step = 0 self._shipped_request_logs: dict[str, int] = {} - self._last_inference_usage: dict[str, Any] = {} + self._last_inference_usage: dict[str, Any] = {} # Cumulative evaluation outcomes, keyed "/". # `diagnostics` below is a last-value field, so it can only ever answer # "has at least one evaluation been terminated", never "how many" -- and @@ -324,6 +349,7 @@ def _save_state(self) -> None: "next_step": self.next_step, "run_id": self.run_id, "request_log_files": self._shipped_request_logs, + "inference_usage": self._last_inference_usage, }, ) @@ -446,12 +472,20 @@ def log_inference_usage(self, scopes: dict[str, Any]) -> None: value = usage.get(key) if isinstance(value, (int, float)): payload[f"inference/{name}/{key}"] = value - if not payload or payload == self._last_inference_usage: - return - self.run.log(payload, step=self.next_step) - self.next_step += 1 - self._last_inference_usage = payload - self._save_state() + # The poller thread shares `next_step` and state.json with the evaluation + # listener on the loop thread, so the mutate-then-save body is serialized. + with self._lock: + if not payload or payload == self._last_inference_usage: + return + # Persist the step before spending it, as in __call__: at-most-once, + # so a crash between the write and the log drops this usage point + # rather than leaving a resumed process to reuse a step W&B has + # already been given. + step = self.next_step + self.next_step += 1 + self._last_inference_usage = payload + self._save_state() + self.run.log(payload, step=step) def ship_request_logs(self, directory: Path, *, final: bool = False) -> None: """Upload the gateway's rotated request-log files as one W&B artifact. @@ -466,29 +500,52 @@ def ship_request_logs(self, directory: Path, *, final: bool = False) -> None: if not files: return snapshot = {path.name: path.stat().st_size for path in files} - if snapshot == self._shipped_request_logs: - return + # Same shared-state lock as the other mutate-then-save bodies, but held + # only around the bookkeeping and never across the upload. log_artifact + # is a file transfer, and __call__ waits on this same lock from the + # sidecar's event loop, so spanning the upload would let a slow W&B + # transfer stall the thing that answers the agent's requests. Leaving the + # upload outside keeps the order at-least-once: a crash between shipping + # and saving re-ships next poll, and W&B folds byte-identical content + # into one artifact version, so a duplicate costs nothing while a lost + # request log cannot be recovered. + with self._lock: + if snapshot == self._shipped_request_logs: + return artifact = self._wandb.Artifact( name="inference-requests", type="inference_request_log" ) for path in files: artifact.add_file(str(path), name=path.name) self.run.log_artifact(artifact) - self._shipped_request_logs = snapshot - self._save_state() + with self._lock: + self._shipped_request_logs = snapshot + self._save_state() def __call__(self, record: EvaluationRecord) -> None: - if record.id in self.logged_evaluations: - return - self.run.log(self._payload(record), step=self.next_step) - if self.log_traces: - try: - self._log_trace(record) - except Exception: # tracing must never drop the metric log - pass - self.logged_evaluations.add(record.id) - self.next_step += 1 - self._save_state() + # Runs on the loop thread while the telemetry poller mutates the same + # counters from its worker thread, so hold the lock across the whole body. + with self._lock: + if record.id in self.logged_evaluations: + return + payload = self._payload(record) + # Persist the step before spending it. Logging first and saving + # afterwards meant a kill inside that window brought the sidecar back + # thinking the step was unused, and it then re-sent this evaluation on + # a step W&B already holds a point for, so the two collide. Written + # this way the window is deliberately at-most-once: a crash here + # loses this evaluation's point rather than duplicating one, and a + # gap in the series is the cheaper of the two failures. + step = self.next_step + self.next_step += 1 + self.logged_evaluations.add(record.id) + self._save_state() + self.run.log(payload, step=step) + if self.log_traces: + try: + self._log_trace(record) + except Exception: # tracing must never drop the metric log + pass def finish( self, diff --git a/vero/src/vero/sidecar/app.py b/vero/src/vero/sidecar/app.py index 98851094..d2d813ad 100644 --- a/vero/src/vero/sidecar/app.py +++ b/vero/src/vero/sidecar/app.py @@ -7,6 +7,7 @@ import logging import shutil import tempfile +import time from contextlib import asynccontextmanager from pathlib import Path from typing import TYPE_CHECKING, Annotated @@ -50,6 +51,36 @@ class ScoreBaselineRequest(StrictModel): replicates: int = 1 +_SESSION_EXPORT_PREFIX = "vero-harbor-export-" + + +def _sweep_stale_session_exports() -> None: + """Remove export scratch directories left behind by earlier exports. + + Each export stages its archive in a fresh ``mkdtemp`` directory and removes + it in the response's background task. That task never runs when the export + crashes or the sidecar is killed mid-stream, and a sidecar lives for the whole + run, so the leftovers accumulate until the volume fills and every later export + fails on ENOSPC with the session still unexported. Scoped to this exact prefix + inside the temporary root the exports are created in, so nothing else on the + volume can be caught by it. + """ + + # An hour is far longer than any export takes to stream, and generous on + # purpose: keeping a dead directory an extra hour costs some disk, sweeping a + # live one costs the export that is still writing into it. + cutoff = time.time() - 3600.0 + for stale in Path(tempfile.gettempdir()).glob(f"{_SESSION_EXPORT_PREFIX}*"): + try: + if stale.is_symlink() or not stale.is_dir(): + continue + if stale.stat().st_mtime >= cutoff: + continue + except OSError: + continue + shutil.rmtree(stale, ignore_errors=True) + + def _error(status_code: int, message: str, *, detail: bool = False): async def handler(_request, error): text = message or str(error) @@ -203,7 +234,11 @@ async def export_session( authorization: Annotated[str | None, Header()] = None, ): require_admin(authorization) - directory = Path(tempfile.mkdtemp(prefix="vero-harbor-export-")) + # Off the event loop like the archive build below: removing a stale + # export's tree is unbounded filesystem work and must not stall the + # agent's own requests. + await asyncio.to_thread(_sweep_stale_session_exports) + directory = Path(tempfile.mkdtemp(prefix=_SESSION_EXPORT_PREFIX)) archive = directory / "session.tar.gz" try: await asyncio.to_thread( diff --git a/vero/src/vero/sidecar/serve.py b/vero/src/vero/sidecar/serve.py index d31bc59e..37fa6d8d 100644 --- a/vero/src/vero/sidecar/serve.py +++ b/vero/src/vero/sidecar/serve.py @@ -10,7 +10,11 @@ from pathlib import Path from typing import TYPE_CHECKING, Any, Awaitable, Callable -from vero.sidecar.auth import generate_admin_token, write_admin_token +from vero.sidecar.auth import ( + generate_admin_token, + read_admin_token, + write_admin_token, +) from vero.sidecar.sidecar import EvaluationSidecar from vero.sidecar.verifier import CanonicalVerifier @@ -74,8 +78,22 @@ async def build_app( factory_path=factory_path, config_path=config_path, ) - token = generate_admin_token() - write_admin_token(admin_token_path, token) + # Reuse the admin token already on the volume instead of minting a fresh one + # on every start. Minting unconditionally meant that a sidecar restart inside + # a run silently invalidated the token the outer agent was already holding: + # its next admin call 401'd and the run was dead, even though the session + # directory, the evaluation database and the budget ledger had all survived + # the restart intact. Reuse does not widen exposure, the token file is 0400 + # inside a 0700 directory (see write_admin_token) and the volume is per-run, + # so the only readers are the ones that could already read it before. + token_path = Path(admin_token_path) + try: + token = read_admin_token(token_path) + except (OSError, ValueError): + # No token yet (first start), or one that cannot be read back as a token: + # either way the holder has nothing usable, so mint and persist one. + token = generate_admin_token() + write_admin_token(token_path, token) return create_app( sidecar=components.sidecar, verifier=components.verifier, diff --git a/vero/src/vero/sidecar/session.py b/vero/src/vero/sidecar/session.py index e2874006..1c69151d 100644 --- a/vero/src/vero/sidecar/session.py +++ b/vero/src/vero/sidecar/session.py @@ -24,6 +24,25 @@ logger = logging.getLogger(__name__) +def _fsync_path(path: Path) -> None: + """Flush a file or a directory entry, tolerating filesystems that refuse to. + + Used on both halves of the archive publish: the archive itself, so its bytes + are on disk, and its parent directory, so the rename that made the final + name visible is on disk too. + """ + try: + descriptor = os.open(path, os.O_RDONLY) + except OSError: + return + try: + os.fsync(descriptor) + except OSError: + pass + finally: + os.close(descriptor) + + class HarborSessionManifest(StrictModel): """Trusted metadata needed to interpret an exported Harbor session.""" @@ -182,7 +201,19 @@ def _text_member(archive: tarfile.TarFile, arcname: str, payload: bytes) -> None "session/vero-export-skipped.json", (json.dumps({"skipped": skipped}, indent=2) + "\n").encode("utf-8"), ) + # This archive is the only durable copy of a finished run, so getting the + # bytes as far as the page cache is not enough: a host that dies right + # after the export can publish the final name over an empty or truncated + # file, which is exactly the "the search work was unrecoverable" outcome + # the export exists to prevent. Flush the archive itself before the + # rename, then flush the parent directory after it so the rename that + # made the name visible is durable too. Both flushes swallow OSError for + # the same reason harbor/cli.py's _fsync_directory does: a filesystem + # that refuses the syscall would otherwise turn every export into a 500 + # and lose the archive outright, which is worse than weaker durability. + _fsync_path(temporary) os.replace(temporary, output) + _fsync_path(output.parent) finally: temporary.unlink(missing_ok=True) if skipped: diff --git a/vero/src/vero/sidecar/sidecar.py b/vero/src/vero/sidecar/sidecar.py index 1c8b8083..0a170618 100644 --- a/vero/src/vero/sidecar/sidecar.py +++ b/vero/src/vero/sidecar/sidecar.py @@ -31,9 +31,11 @@ EvaluationRequest, EvaluationRequestError, EvaluationSet, + EvaluationStore, EvaluationSummary, EvaluationTerminatedError, ObjectiveSpec, + RunningEvaluationManifest, project_evaluation, ) from vero.evaluation.engine import EvaluationEngine @@ -286,9 +288,62 @@ def __init__( ) self._policies[policy.key] = policy + def _running_evaluations(self) -> dict[str, RunningEvaluationManifest]: + """Load the manifests of evaluations left mid-flight by an earlier start. + + ``EvaluationDatabase.load_reconciled`` deliberately skips every + non-complete manifest, so one of these directories is the only surviving + record that its evaluation was ever started. + """ + + manifests: dict[str, RunningEvaluationManifest] = {} + evaluations_dir = self.engine.evaluator.evaluations_dir + if not evaluations_dir.is_dir(): + return manifests + for result_dir in sorted(evaluations_dir.iterdir()): + try: + manifest = RunningEvaluationManifest.model_validate_json( + (result_dir / EvaluationStore.manifest_basename).read_text( + encoding="utf-8" + ) + ) + except (OSError, ValueError): + continue # completed, corrupt, or not an evaluation directory + manifests[manifest.id] = manifest + return manifests + + def _interrupted_evaluation_id( + self, + job: SidecarEvaluationJob, + running: dict[str, RunningEvaluationManifest], + ) -> str | None: + """Name the mid-flight evaluation an interrupted job was driving. + + Matched on the identity the job already records (backend, evaluation set, + and the candidate version resolved at admission) plus the job's own start + time, since the evaluation can only have begun after the job existed. The + claimed manifest is removed from ``running`` so one evaluation cannot be + credited to two jobs, and an ambiguous match is declined outright: naming + the wrong evaluation is worse than naming none, because the reconciler + would then chase a budget reservation that belongs to another job. + """ + + matches = [ + manifest + for manifest in running.values() + if manifest.backend_id == job.backend_id + and manifest.request.evaluation_set == job.evaluation_set + and manifest.request.candidate.version == job.version + and manifest.created_at >= job.created_at + ] + if len(matches) != 1: + return None + return running.pop(matches[0].id).id + def _load_evaluation_jobs(self) -> None: """Restore terminal jobs and mark interrupted in-flight jobs explicitly.""" + running = self._running_evaluations() for path in sorted(self._evaluation_jobs_dir.glob("*.json")): try: job = SidecarEvaluationJob.model_validate_json( @@ -303,6 +358,16 @@ def _load_evaluation_jobs(self) -> None: job = job.model_copy( update={ "status": EvaluationJobStatus.FAILED, + # A job only learns its evaluation_id when the evaluation + # returns, so an interrupted one recorded none at all and + # the evaluation it was driving became unfindable: the + # budget it reserved could never be reconciled against it, + # and the cases it had already checkpointed belonged to + # nothing. Carry the id over from the manifest the + # evaluator left behind, which is the only durable link + # between the orphaned job and its evaluation. + "evaluation_id": job.evaluation_id + or self._interrupted_evaluation_id(job, running), "error": "evaluation job was interrupted by a sidecar restart", "completed_at": datetime.now(UTC), } diff --git a/vero/src/vero/sidecar/verifier.py b/vero/src/vero/sidecar/verifier.py index 2fe92f19..04aa6c7f 100644 --- a/vero/src/vero/sidecar/verifier.py +++ b/vero/src/vero/sidecar/verifier.py @@ -192,6 +192,10 @@ def result_path(self) -> Path: def submission_path(self) -> Path: return self.admin_volume / "submission.json" + @property + def baseline_path(self) -> Path: + return self.admin_volume / "baseline.json" + def _try_load_submission(self) -> Candidate | None: if not self.submission_path.exists(): return None @@ -492,6 +496,17 @@ def _aggregate(values: list[float | None]) -> dict[str, JsonValue]: target_values.append(None if error is not None else reward) targets[target.reward_key] = _aggregate(target_values) result["targets"] = targets + # Persist the aggregate before returning it. The HTTP response used to be + # the only copy, so a dropped connection threw away every replicate and + # the whole measurement had to be paid for again, which on a held-out + # partition is the most expensive scoring a run does. This is a record of + # what was measured, not a cache: nothing reads it back to skip work, so + # a re-measurement still runs and simply overwrites it. + await asyncio.to_thread( + _atomic_write_json, + self.baseline_path, + result, + ) return result async def _finalize(self) -> VerificationResult: diff --git a/vero/src/vero/workspace/git.py b/vero/src/vero/workspace/git.py index 7fd20597..b27500c8 100644 --- a/vero/src/vero/workspace/git.py +++ b/vero/src/vero/workspace/git.py @@ -12,6 +12,13 @@ logger = logging.getLogger(__name__) +# Commit dates for the automated saves below are pinned to the epoch so that +# committing the same staged tree on the same parent with the same message +# always yields the same sha. Left on the wall clock, a resumed run recommitted +# byte-identical content and got a brand new sha, so it could not recognise the +# candidate its own previous attempt had already built and paid to evaluate. +_PINNED_COMMIT_DATE = "1970-01-01T00:00:00+00:00" + def _basename(path: str) -> str: """Extract the last component of a path.""" @@ -69,11 +76,36 @@ def name(self) -> str: # ── Git helper ────────────────────────────────────────────────── - async def _git(self, *args: str) -> str: + async def _git(self, *args: str, env: dict[str, str] | None = None) -> str: """Run a git command via sandbox.run(), returning stdout. Raises on non-zero exit.""" + command = ["git", "-c", f"safe.directory={self._root}", *args] + if env: + # Extra variables go through the `env` binary rather than + # sandbox.run(env=...) because the two Sandbox implementations + # disagree about what that parameter means: LocalSandbox hands the + # dict straight to the subprocess and therefore *replaces* the whole + # environment (losing PATH, HOME and the ambient git configuration), + # while DockerSandbox adds the entries on top of the container's. + # Prefixing the command keeps "add these variables" meaning the same + # thing in every sandbox. + command = [ + "env", + *(f"{name}={value}" for name, value in env.items()), + *command, + ] result = await self._sandbox.run( - ["git", "-c", f"safe.directory={self._root}", *args], + command, cwd=self._root, + # The sandbox's 30 second default is far too short for git over a + # large candidate tree: a `git add` of a big working copy while the + # machine was loaded with concurrent trials ran past it, the timeout + # killed git mid-write, and the .git/index.lock it left behind + # destroyed that round's candidate with an error that pointed + # nowhere near the real cause. The tradeoff of 120 seconds is that a + # git process which is genuinely hung now takes four times as long + # to fail, which we accept because losing a candidate is worse than + # waiting. + timeout=120, ) if result.returncode != 0: raise RuntimeError(f"git {' '.join(args)} failed: {result.stderr}") @@ -168,7 +200,16 @@ async def save(self, message: str = "Save") -> str: else: await self._git("add", self._project_path) - # Commit (skip hooks for automated commits) + # Commit (skip hooks for automated commits). Both dates have to be + # pinned, not just the author date: the committer date is part of + # the commit object too, so leaving it on the wall clock keeps the + # sha unstable. Note this invalidates existing session directories + # exactly once, because candidates saved before this change carry + # wall-clock shas that will never be reproduced again. Candidate + # ordering does not move: the optimizer stamps Candidate.created_at + # from datetime.now(UTC) separately, and both the repository listing + # and the objective tie-break sort on that field, never on a commit + # date. await self._git( "-c", "user.name=vero", @@ -178,6 +219,10 @@ async def save(self, message: str = "Save") -> str: "-m", message, "--no-verify", + env={ + "GIT_AUTHOR_DATE": _PINNED_COMMIT_DATE, + "GIT_COMMITTER_DATE": _PINNED_COMMIT_DATE, + }, ) return await self.current_version() diff --git a/vero/tests/test_idempotency_backend.py b/vero/tests/test_idempotency_backend.py new file mode 100644 index 00000000..2b15cb8b --- /dev/null +++ b/vero/tests/test_idempotency_backend.py @@ -0,0 +1,368 @@ +"""Idempotency regressions for the Harbor backend's sub-run plumbing. + +Every test here pins work that a killed or restarted run must not have to redo: +the trials a dying sub-run already finished, a trial record half way through +redaction, and the case-resource tree a previous attempt already staged. None of +them touch scoring; the scores they assert are incidental. +""" + +from __future__ import annotations + +import asyncio +import json +import os +import sys +from datetime import UTC, datetime +from pathlib import Path +from types import SimpleNamespace + +import pytest + +from vero.candidate import Candidate +from vero.evaluation import ( + CaseCheckpointStore, + CaseIds, + EvaluationContext, + EvaluationLimits, + EvaluationRequest, + EvaluationSet, + RetryPolicy, +) +from vero.harbor import HarborBackend, HarborBackendConfig +from vero.sandbox import CommandResult, LocalSandbox + +TASK_NAME = "example/alpha" + + +def _cases(path: Path) -> Path: + path.write_text( + json.dumps( + [ + {"id": "case-a", "task_name": TASK_NAME}, + {"id": "case-b", "task_name": "example/beta"}, + ] + ), + encoding="utf-8", + ) + return path + + +def _config(tmp_path: Path, **updates) -> HarborBackendConfig: + values = { + "task_source": "example/tasks@1.0", + "agent_import_path": "candidate.agent:Agent", + "cases_path": str(_cases(tmp_path / "cases.json")), + "harbor_requirement": "harbor==0.1.17", + "evaluation_set_name": "harbor-bench", + "partition": "test", + "uv_executable": sys.executable, + "infrastructure_max_attempts": 1, + "infrastructure_retry_delay_seconds": 0, + } + values.update(updates) + return HarborBackendConfig(**values) + + +def _request(selection=None) -> EvaluationRequest: + return EvaluationRequest( + candidate=Candidate( + id="candidate", + version="version", + created_at=datetime(2026, 1, 1, tzinfo=UTC), + ), + evaluation_set=EvaluationSet( + name="harbor-bench", + partition="test", + **({"selection": selection} if selection is not None else {}), + ), + limits=EvaluationLimits( + timeout_seconds=90, + max_concurrency=4, + retry=RetryPolicy.disabled(), + ), + ) + + +async def _context(tmp_path: Path, sandbox: LocalSandbox) -> EvaluationContext: + target = tmp_path / "target" + target.mkdir(parents=True, exist_ok=True) + result_dir = tmp_path / "result" + artifact_dir = result_dir / "artifacts" + artifact_dir.mkdir(parents=True) + return EvaluationContext( + workspace=SimpleNamespace( + project_path=str(target), root=str(target), sandbox=sandbox + ), + session_id="session", + evaluation_id="evaluation", + result_dir=result_dir, + artifact_dir=artifact_dir, + case_store=CaseCheckpointStore(result_dir / "cases"), + ) + + +class TrialWritingSandbox(LocalSandbox): + """Writes one finished trial into the sub-run's jobs directory. + + ``dies`` reproduces the sub-run that goes away part way through: the trial it + already finished is on disk in the sandbox, and the run call never returns. + """ + + def __init__(self, root: Path, *, agent_files: dict[str, str], dies: bool = False): + super().__init__(root) + self.agent_files = agent_files + self.dies = dies + self.download_attempts: list[str] = [] + self.download_fails = False + self.download_error: BaseException | None = None + + async def run(self, command, cwd=None, timeout=30, env=None, run_as=None): + if not isinstance(command, list) or "--jobs-dir" not in command: + return await super().run( + command, cwd=cwd, timeout=timeout, env=env, run_as=run_as + ) + jobs_dir = Path(command[command.index("--jobs-dir") + 1]) + trial_dir = jobs_dir / "job-0" / "trial-alpha" + trial_dir.mkdir(parents=True, exist_ok=True) + (trial_dir / "result.json").write_text( + json.dumps( + { + "task_name": TASK_NAME, + "trial_name": "trial-0", + "finished_at": "2026-01-01T00:00:00Z", + "verifier_result": {"rewards": {"reward": 1.0}}, + } + ), + encoding="utf-8", + ) + for relative_path, content in self.agent_files.items(): + path = trial_dir / "agent" / relative_path + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content, encoding="utf-8") + # A distinctive mode, so a test can tell whether redaction preserved + # the record's own permissions. + path.chmod(0o640) + if self.dies: + raise RuntimeError("sandbox went away mid sub-run") + return CommandResult("harbor output", "", 0) + + async def download(self, remote_path: str, local_path: str) -> None: + self.download_attempts.append(remote_path) + if self.download_error is not None: + raise self.download_error + if self.download_fails: + raise RuntimeError("docker cp: no such container") + await super().download(remote_path, local_path) + + +@pytest.mark.asyncio +async def test_dying_sub_run_still_salvages_the_trials_it_finished(tmp_path): + sandbox = TrialWritingSandbox(tmp_path, agent_files={}, dies=True) + backend = HarborBackend(_config(tmp_path)) + context = await _context(tmp_path, sandbox) + + with pytest.raises(RuntimeError, match="sandbox went away"): + await backend.evaluate( + context=context, request=_request(CaseIds(ids=["case-a"])) + ) + + # The staging area is gone by now, so this local copy is the only surviving + # record of the trial the sub-run did finish before it died. + salvaged = sorted((context.artifact_dir / "harbor" / "jobs").rglob("result.json")) + assert len(salvaged) == 1 + assert json.loads(salvaged[0].read_text())["task_name"] == TASK_NAME + + +@pytest.mark.asyncio +async def test_failed_salvage_does_not_replace_the_sub_run_failure(tmp_path): + sandbox = TrialWritingSandbox(tmp_path, agent_files={}, dies=True) + sandbox.download_fails = True + backend = HarborBackend(_config(tmp_path)) + context = await _context(tmp_path, sandbox) + + # A sandbox that has gone away fails the salvage copy too; the caller must + # still see the sub-run's own failure, which is the diagnosable one. + with pytest.raises(RuntimeError, match="sandbox went away"): + await backend.evaluate( + context=context, request=_request(CaseIds(ids=["case-a"])) + ) + + assert sandbox.download_attempts, "the salvage download was never attempted" + + +@pytest.mark.asyncio +async def test_cancelled_salvage_does_not_replace_the_sub_run_failure(tmp_path): + """A cancellation landing on the salvage must not eat the real failure. + + The salvage sits in a ``finally`` and is shielded so a cancelled task still + gets its trials off the sandbox, which means a second cancellation can arrive + while that shielded copy is parked. Catching only ``Exception`` there let the + ``CancelledError`` out of the ``finally`` and it replaced the sub-run's own + error, so the caller was told the run was cancelled when what actually + happened was a dead sandbox. The cancellation is not lost by being swallowed: + a genuinely cancelling task sees it again at its next await. + """ + sandbox = TrialWritingSandbox(tmp_path, agent_files={}, dies=True) + sandbox.download_error = asyncio.CancelledError() + backend = HarborBackend(_config(tmp_path)) + context = await _context(tmp_path, sandbox) + + with pytest.raises(RuntimeError, match="sandbox went away"): + await backend.evaluate( + context=context, request=_request(CaseIds(ids=["case-a"])) + ) + + assert sandbox.download_attempts, "the salvage download was never attempted" + + +@pytest.mark.asyncio +async def test_interrupted_redaction_leaves_the_original_trial_record( + tmp_path, monkeypatch +): + secret = "evaluation-scope-secret" + original = json.dumps({"steps": [{"message": f"used {secret}"}]}) + sandbox = TrialWritingSandbox(tmp_path, agent_files={"trajectory.json": original}) + backend = HarborBackend(_config(tmp_path, environment={"EVALUATION_TOKEN": secret})) + context = await _context(tmp_path, sandbox) + real_replace = os.replace + + def fail_the_rename(source, destination, *args, **kwargs): + if str(destination).endswith("trajectory.json"): + raise OSError("no space left on device") + return real_replace(source, destination, *args, **kwargs) + + monkeypatch.setattr(os, "replace", fail_the_rename) + + report = await backend.evaluate( + context=context, request=_request(CaseIds(ids=["case-a"])) + ) + + trajectory = next( + context.artifact_dir / artifact.path + for artifact in report.cases[0].artifacts + if Path(artifact.path).name == "trajectory.json" + ) + # The redaction that could not complete leaves the record exactly as Harbor + # wrote it, not truncated to nothing: the report the caller sees is sanitized + # on its own path, so an intact original is recoverable while a half-written + # one is neither the original nor the redacted version. + assert trajectory.read_text(encoding="utf-8") == original + + +@pytest.mark.asyncio +async def test_completed_redaction_leaves_no_half_written_sibling(tmp_path): + secret = "evaluation-scope-secret" + sandbox = TrialWritingSandbox( + tmp_path, + agent_files={ + "trajectory.json": json.dumps({"steps": [{"message": f"used {secret}"}]}) + }, + ) + backend = HarborBackend(_config(tmp_path, environment={"EVALUATION_TOKEN": secret})) + context = await _context(tmp_path, sandbox) + + report = await backend.evaluate( + context=context, request=_request(CaseIds(ids=["case-a"])) + ) + + names = {Path(artifact.path).name for artifact in report.cases[0].artifacts} + assert names == {"result.json", "trajectory.json"} + trajectory = next( + context.artifact_dir / artifact.path + for artifact in report.cases[0].artifacts + if Path(artifact.path).name == "trajectory.json" + ) + assert "[REDACTED]" in trajectory.read_text(encoding="utf-8") + assert secret not in trajectory.read_text(encoding="utf-8") + # The rename must not hand the agent context a record with this process's + # umask in place of the record's own mode. + assert trajectory.stat().st_mode & 0o777 == 0o640 + # The sibling the rename consumed must not survive as a second copy of the + # record next to it. + assert sorted(path.name for path in trajectory.parent.iterdir()) == [ + "trajectory.json" + ] + + +@pytest.mark.asyncio +async def test_case_resource_staging_isolates_attempts_from_each_other( + tmp_path, monkeypatch +): + """A retry must stage somewhere the previous attempt cannot be holding. + + A stable staging path would be the more idempotent choice on its own, since + a retry could reclaim the tree a killed attempt abandoned. It is not the safe + one: nothing serializes two processes over one configured cache path, and the + stage clears its directory before using it, so a shared name means one + attempt can delete a tree another is still materializing into. Distinct paths + cost a stale directory after a hard kill and buy that isolation. + """ + + task_source = tmp_path / "tasks" + task_source.mkdir() + for task_name in ("alpha", "beta"): + task = task_source / task_name + task.mkdir() + (task / "task.toml").write_text(f'[task]\nname="example/{task_name}"\n') + cases_path = tmp_path / "local-cases.json" + cases_path.write_text( + json.dumps( + [ + {"id": "case-a", "task_name": "alpha"}, + {"id": "case-b", "task_name": "beta"}, + ] + ), + encoding="utf-8", + ) + cache = tmp_path / "case-resources" / "test" + backend = HarborBackend( + _config( + tmp_path, + task_source=str(task_source), + cases_path=str(cases_path), + case_resources_cache_path=str(cache), + ) + ) + evaluation_set = EvaluationSet( + name="harbor-bench", partition="test", selection=CaseIds(ids=["case-b"]) + ) + sandbox = await LocalSandbox.create(root=tmp_path) + staged: list[Path] = [] + materialize = backend._materialize_case_resources + + async def record_then_fail_once(root, cases, evaluation_set): + staged.append(root) + if len(staged) == 1: + raise OSError("dataset download died") + await materialize(root, cases, evaluation_set) + + monkeypatch.setattr(backend, "_materialize_case_resources", record_then_fail_once) + + first = tmp_path / "context-first" + first.mkdir() + with pytest.raises(OSError, match="dataset download died"): + await backend.export_case_resources( + evaluation_set=evaluation_set, destination=str(first), sandbox=sandbox + ) + + # Stand in for the tree a run killed outright leaves behind, which never + # reaches the cleanup below. + (staged[0] / "tasks").mkdir(parents=True) + (staged[0] / "tasks" / "leftover").write_text("partial\n", encoding="utf-8") + + second = tmp_path / "context-second" + second.mkdir() + await backend.export_case_resources( + evaluation_set=evaluation_set, destination=str(second), sandbox=sandbox + ) + + # Two attempts, two paths: neither can clear the other's work out from under + # it, and the abandoned partial cannot leak into what the retry exports. + assert staged[0] != staged[1] + assert staged[0].parent == staged[1].parent == cache.parent + index = json.loads((second / "index.json").read_text()) + assert [item["case_id"] for item in index["cases"]] == ["case-b"] + assert not (second / "tasks" / "leftover").exists() + # The attempt that ran to completion cleans up after itself; only the killed + # one leaves a directory behind, which is the accepted cost of the isolation. + assert not staged[1].exists() diff --git a/vero/tests/test_idempotency_compiler.py b/vero/tests/test_idempotency_compiler.py new file mode 100644 index 00000000..40717865 --- /dev/null +++ b/vero/tests/test_idempotency_compiler.py @@ -0,0 +1,250 @@ +"""Recompiling the same build has to be able to rejoin a preserved session. + +Every test here defends one half of that: the compiled tree's identities (the +baseline commit sha, the three gateway tokens) must not drift when nothing about +the build changed, and a compile that dies must not overwrite the tree that a +resume would otherwise still be able to use. +""" + +from __future__ import annotations + +import json +import subprocess +from datetime import datetime +from pathlib import Path + +import pytest + +from vero.harbor import ( + AgentAccessSpec, + HarborBuildConfig, + InferenceBudgetSpec, + InferenceGatewaySpec, + VerificationTargetSpec, +) +from vero.harbor.build import compiler + +_VERO_ROOT = Path(__file__).parents[1] + + +def _git(path: Path, *arguments: str) -> str: + result = subprocess.run( + ["git", *arguments], + cwd=path, + check=True, + text=True, + capture_output=True, + ) + return result.stdout.strip() + + +def _target_repo(path: Path) -> Path: + path.mkdir(parents=True) + _git(path, "init", "-q") + _git(path, "config", "user.name", "VeRO Test") + _git(path, "config", "user.email", "vero@example.test") + (path / "README.md").write_text("# Target\n", encoding="utf-8") + (path / "pyproject.toml").write_text( + '[project]\nname="target"\nversion="0.1.0"\n', + encoding="utf-8", + ) + _git(path, "add", ".") + _git(path, "commit", "-q", "-m", "target baseline") + return path + + +def _task_source(path: Path, names: list[str]) -> Path: + for name in names: + task = path / name + task.mkdir(parents=True) + (task / "task.toml").write_text( + f'[task]\nname="org/{name}"\n', encoding="utf-8" + ) + return path + + +def _config(root: Path, **updates) -> HarborBuildConfig: + """The smallest build that still exercises a harbor backend per partition.""" + values = { + "name": "org/optimize-program", + "description": "Improve the program", + "agent_repo": str(_target_repo(root / "target")), + "task_source": str( + _task_source(root / "protected-tasks", ["task-a", "task-b", "task-hidden"]) + ), + "agent_import_path": "target.agent:Agent", + "harbor_requirement": "harbor==0.1.17", + "partitions": {"validation": ["task-a", "task-b"], "test": ["task-hidden"]}, + "agent_access": [AgentAccessSpec(partition="validation", total_runs=5)], + "selection_partition": "validation", + "targets": [VerificationTargetSpec(partition="test")], + } + values.update(updates) + return HarborBuildConfig(**values) + + +def _gateway_config(root: Path, **updates) -> HarborBuildConfig: + return _config( + root, + inference_gateway=InferenceGatewaySpec( + producer=InferenceBudgetSpec(allowed_models=["gpt-producer"]), + evaluation=InferenceBudgetSpec(allowed_models=["gpt-target"]), + ), + **updates, + ) + + +def _baseline_version(output: Path) -> str: + serve = json.loads( + (output / "environment/sidecar/serve.json").read_text(encoding="utf-8") + ) + return serve["selection"]["baseline_version"] + + +def _tokens(output: Path) -> tuple[str, str, str]: + launch = json.loads( + (output / "environment/gateway/launch.json").read_text(encoding="utf-8") + ) + serve = json.loads( + (output / "environment/sidecar/serve.json").read_text(encoding="utf-8") + ) + backend = next(iter(serve["backends"].values())) + return ( + launch["producer_api_key"], + backend["inference_gateway_token"], + backend["inference_gateway_finalization_token"], + ) + + +def test_baseline_commit_dates_are_pinned_so_the_sha_is_content_addressed( + tmp_path, + monkeypatch, +): + """The baseline sha is selection.baseline_version, so it has to be a function + of the tree alone. Ambient GIT_*_DATE is set to two different values across + the two compiles here precisely because the compiler used to inherit it (and + otherwise the wall clock), which is how identical content produced two + different shas and locked a recompile out of its preserved session. + """ + monkeypatch.setenv("GIT_AUTHOR_DATE", "2021-01-01T00:00:00+00:00") + monkeypatch.setenv("GIT_COMMITTER_DATE", "2021-01-01T00:00:00+00:00") + first = compiler.compile_harbor_task( + _config(tmp_path / "a"), tmp_path / "a/out", vero_root=_VERO_ROOT + ) + monkeypatch.setenv("GIT_AUTHOR_DATE", "2023-06-06T06:06:06+00:00") + monkeypatch.setenv("GIT_COMMITTER_DATE", "2023-06-06T06:06:06+00:00") + second = compiler.compile_harbor_task( + _config(tmp_path / "b"), tmp_path / "b/out", vero_root=_VERO_ROOT + ) + + assert _baseline_version(first) == _baseline_version(second) + # And the pin is the documented constant, not just "the same twice". + pinned = datetime.fromisoformat(compiler.BASELINE_COMMIT_DATE) + expected = str(int(pinned.timestamp())) + stamps = _git( + first / "environment/agent-baseline", "show", "-s", "--format=%at %ct", "HEAD" + ) + assert stamps == f"{expected} {expected}" + + +def test_recompile_into_the_same_directory_reuses_the_gateway_tokens(tmp_path): + """Fresh tokens move the backend config digest the session manifest pins, so + a recompile that re-mints them can never be brought up against durable state + even though nothing about the evaluation changed. + """ + output = tmp_path / "compiled" + first = compiler.compile_harbor_task( + _gateway_config(tmp_path), output, vero_root=_VERO_ROOT + ) + before = _tokens(first) + digests_before = json.loads( + (first / "environment/gateway/config.json").read_text(encoding="utf-8") + )["scopes"] + serve_before = (first / "environment/sidecar/serve.json").read_text( + encoding="utf-8" + ) + + second = compiler.compile_harbor_task( + _gateway_config(tmp_path / "again"), output, vero_root=_VERO_ROOT + ) + + assert _tokens(second) == before + # The three stay distinct, so the optimizer still cannot spend finalization's + # reserved budget; reuse is not collapsing them into one token. + assert len(set(before)) == 3 + digests_after = json.loads( + (second / "environment/gateway/config.json").read_text(encoding="utf-8") + )["scopes"] + for scope in ("producer", "evaluation", "finalization"): + assert ( + digests_after[scope]["token_sha256"] + == digests_before[scope]["token_sha256"] + ) + # With the baseline sha pinned too, the whole sidecar config is now + # byte-identical across a recompile, which is what the manifest check wants. + serve_after = (second / "environment/sidecar/serve.json").read_text( + encoding="utf-8" + ) + assert serve_after == serve_before + + +def test_a_different_output_directory_still_mints_its_own_tokens(tmp_path): + """Reuse is keyed on the output directory, not global: two concurrent runs + must not end up sharing one gateway credential. + """ + first = compiler.compile_harbor_task( + _gateway_config(tmp_path / "a"), tmp_path / "a/out", vero_root=_VERO_ROOT + ) + second = compiler.compile_harbor_task( + _gateway_config(tmp_path / "b"), tmp_path / "b/out", vero_root=_VERO_ROOT + ) + assert set(_tokens(first)).isdisjoint(set(_tokens(second))) + + +def test_a_dead_compile_leaves_the_previous_tree_in_place(tmp_path, monkeypatch): + """The failure that motivated staging: a compile dies partway, and what is + left behind in the output directory has enough of a shape that the next step + treats it as a finished task and fails much later, somewhere unrelated. + """ + output = tmp_path / "compiled" + good = compiler.compile_harbor_task( + _config(tmp_path / "a"), output, vero_root=_VERO_ROOT + ) + task_toml = (good / "task.toml").read_text(encoding="utf-8") + # A successful compile leaves no staging directory behind. + assert not (tmp_path / "compiled.partial").exists() + + def die(*arguments, **keywords): + raise RuntimeError("compile died halfway") + + monkeypatch.setattr(compiler, "_write_cases", die) + with pytest.raises(RuntimeError, match="compile died halfway"): + compiler.compile_harbor_task( + _config(tmp_path / "b"), output, vero_root=_VERO_ROOT + ) + + # The last complete compile is untouched, so a resume still has something to + # come up against. + assert (output / "task.toml").read_text(encoding="utf-8") == task_toml + assert (output / "environment/sidecar/serve.json").is_file() + # The wreckage is parked next door, and it is obviously unfinished. + partial = tmp_path / "compiled.partial" + assert partial.is_dir() + assert not (partial / "task.toml").exists() + + +def test_a_later_compile_clears_the_wreckage_of_an_earlier_one(tmp_path): + """A stale .partial must not be mistaken for state to resume from, and must + not leak files that the new compile would never have written. + """ + output = tmp_path / "compiled" + stale = tmp_path / "compiled.partial" + (stale / "environment").mkdir(parents=True) + (stale / "leftover.txt").write_text("from a dead compile\n", encoding="utf-8") + + compiled = compiler.compile_harbor_task( + _config(tmp_path), output, vero_root=_VERO_ROOT + ) + assert (compiled / "task.toml").is_file() + assert not (compiled / "leftover.txt").exists() + assert not stale.exists() diff --git a/vero/tests/test_idempotency_engine.py b/vero/tests/test_idempotency_engine.py new file mode 100644 index 00000000..2cd5939f --- /dev/null +++ b/vero/tests/test_idempotency_engine.py @@ -0,0 +1,160 @@ +"""Budget reservations must not leak when the evaluator fails unexpectedly. + +The engine refunds a reservation on cancellation (typed and raw), on a recorded +execution failure, and on an infrastructure diagnostic, but a bare exception from +the evaluator used to escape every one of those handlers with the reservation +still charged. Nothing else notices, so the run continues against a permanently +short budget and the evaluations at the end of a long search are starved. +""" + +from __future__ import annotations + +from datetime import UTC, datetime +from pathlib import Path + +import pytest + +from vero.candidate import Candidate +from vero.evaluation import ( + BackendProvenance, + BackendRegistry, + BudgetLedger, + EvaluationBudget, + EvaluationCost, + EvaluationDatabase, + EvaluationEngine, + EvaluationReport, + EvaluationRequest, + EvaluationSet, + EvaluationStore, + Evaluator, + allow_all_evaluations, +) + + +class NeverCheckedOutRepository: + """Candidate repository stand-in: these tests fail before any checkout.""" + + family = "stub" + + def checkout(self, candidate, *, sandbox, name=None): + raise AssertionError("the running-manifest write must fail before checkout") + + +class StubBackend: + """Minimal backend: only resolve_cost is reached before the failure.""" + + @property + def provenance(self) -> BackendProvenance: + return BackendProvenance(name="stub", version="1", config_digest="0" * 64) + + async def resolve_cost(self, evaluation_set: EvaluationSet) -> EvaluationCost: + return EvaluationCost(runs=1, cases=4) + + async def evaluate(self, *, context, request) -> EvaluationReport: + raise AssertionError("the running-manifest write must fail before evaluate") + + +def request() -> EvaluationRequest: + return EvaluationRequest( + candidate=Candidate( + id="id:candidate", + version="candidate", + created_at=datetime(2026, 1, 1, tzinfo=UTC), + ), + evaluation_set=EvaluationSet(name="performance"), + ) + + +def engine_with_budget(tmp_path: Path, ledger: BudgetLedger) -> EvaluationEngine: + return EvaluationEngine( + evaluator=Evaluator( + candidate_repository=NeverCheckedOutRepository(), + sandbox=None, + session_dir=tmp_path / "sessions" / "session", + ), + backends=BackendRegistry({"default": StubBackend()}), + database=EvaluationDatabase(id="session"), + database_path=tmp_path / "database.json", + budget_ledger=ledger, + authorization_resolver=allow_all_evaluations, + ) + + +def ledger_with_two_runs(tmp_path: Path) -> BudgetLedger: + evaluation_set = request().evaluation_set + ledger = BudgetLedger( + [ + EvaluationBudget( + backend_id="default", + evaluation_set_key=evaluation_set.budget_key("default"), + total_runs=2, + total_cases=8, + ) + ], + path=tmp_path / "budgets.json", + ) + ledger.save() + return ledger + + +def fail_running_manifest(monkeypatch) -> None: + """Break the evaluator's pre-try running-manifest write. + + This is the concrete unexpected failure: write_running happens before the + evaluator's own try block, so an OSError from it is never converted into + EvaluationCancelledError or EvaluationExecutionError and arrives at the + engine as a bare exception. + """ + + def boom(self, **kwargs): + raise OSError("running manifest write failed") + + monkeypatch.setattr(EvaluationStore, "write_running", boom) + + +@pytest.mark.asyncio +async def test_unexpected_evaluator_failure_refunds_the_reservation( + tmp_path: Path, monkeypatch +): + ledger = ledger_with_two_runs(tmp_path) + evaluation_set = request().evaluation_set + engine = engine_with_budget(tmp_path, ledger) + fail_running_manifest(monkeypatch) + + # The bare exception still reaches the caller unchanged: the refund is the + # only new behaviour, so retry classification upstream is untouched. + with pytest.raises(OSError, match="running manifest write failed"): + await engine.evaluate_record(backend_id="default", request=request()) + + restored = ledger.get("default", evaluation_set) + assert restored is not None + assert restored.remaining_runs == 2 + assert restored.remaining_cases == 8 + durable = BudgetLedger.load(tmp_path / "budgets.json").get( + "default", evaluation_set + ) + assert durable is not None + assert durable.remaining_runs == 2 + assert durable.remaining_cases == 8 + + +@pytest.mark.asyncio +async def test_unexpected_failure_refund_failure_is_chained_not_substituted( + tmp_path: Path, monkeypatch +): + # A refund that fails on its own must not replace the error that actually + # stopped the evaluation, the same contract the cancellation and + # infrastructure handlers already hold themselves to. + ledger = ledger_with_two_runs(tmp_path) + engine = engine_with_budget(tmp_path, ledger) + fail_running_manifest(monkeypatch) + + async def failing_refund(*args, **kwargs): + raise RuntimeError("durable refund write failed") + + monkeypatch.setattr(ledger, "refund", failing_refund) + + with pytest.raises(OSError, match="running manifest write failed") as raised: + await engine.evaluate_record(backend_id="default", request=request()) + assert isinstance(raised.value.__cause__, RuntimeError) diff --git a/vero/tests/test_idempotency_gateway.py b/vero/tests/test_idempotency_gateway.py new file mode 100644 index 00000000..c4864332 --- /dev/null +++ b/vero/tests/test_idempotency_gateway.py @@ -0,0 +1,172 @@ +from __future__ import annotations + +import asyncio +import gc +import json + +import httpx +from fastapi.testclient import TestClient + +from vero.gateway.inference import ( + InferenceGatewayConfig, + InferenceScopeConfig, + create_inference_gateway_app, + token_digest, +) + +_STREAM = ( + 'event: response.created\ndata: {"type":"response.created"}\n\n' + 'event: response.completed\ndata: {"type":"response.completed",' + '"response":{"usage":{"input_tokens":5,"output_tokens":3,' + '"total_tokens":8}}}\n\n' +) + + +def _streaming_app(tmp_path): + """A gateway whose upstream always answers with a two-event SSE stream.""" + + def upstream(_request: httpx.Request): + return httpx.Response( + 200, + content=_STREAM, + headers={"content-type": "text/event-stream"}, + ) + + return create_inference_gateway_app( + config=InferenceGatewayConfig( + state_path=str(tmp_path / "usage.json"), + scopes={ + "producer": InferenceScopeConfig( + token_sha256=token_digest("scoped-token"), + allowed_models=["gpt-test"], + # One permit, so a single leaked reservation wedges the whole + # scope and the wedge is observable in one request. + max_concurrency=1, + ) + }, + ), + upstream_api_key="upstream-secret", + transport=httpx.MockTransport(upstream), + ) + + +def _scope(body: bytes) -> dict: + # No "spec_version" in the asgi extension, so starlette takes its pre-2.4 + # branch and races stream_response against listen_for_disconnect in an anyio + # task group. That is the branch uvicorn drives today and the one where the + # body iterator can be cancelled before its first step. + return { + "type": "http", + "asgi": {"version": "3.0"}, + "http_version": "1.1", + "method": "POST", + "scheme": "http", + "path": "/scopes/producer/optimizer/v1/responses", + "raw_path": b"/scopes/producer/optimizer/v1/responses", + "root_path": "", + "query_string": b"", + "headers": [ + (b"host", b"testserver"), + (b"authorization", b"Bearer scoped-token"), + (b"content-type", b"application/json"), + (b"content-length", str(len(body)).encode()), + ], + "client": ("testclient", 50000), + "server": ("testserver", 80), + } + + +def test_streaming_client_that_disconnects_first_returns_its_permit(tmp_path): + """A client gone before the first chunk must not keep a concurrency permit. + + This is the wedge from the audit: starlette cancels its streaming task group + the moment listen_for_disconnect sees http.disconnect, so an unprimed body + iterator is never stepped, the generator's finally never runs, and the + reservation it holds is never returned. After max_concurrency of these, + reserve() waits on the semaphore forever with no timeout and no traceback. + """ + app = _streaming_app(tmp_path) + body = json.dumps({"model": "gpt-test", "input": "hello", "stream": True}).encode() + + async def drive() -> None: + pending = [{"type": "http.request", "body": body, "more_body": False}] + + async def receive() -> dict: + # The request body first, then a client that has already hung up. + return pending.pop(0) if pending else {"type": "http.disconnect"} + + sent: list[dict] = [] + + async def send(message: dict) -> None: + # A real ASGI server suspends here while it writes to the socket, and + # that suspension is where the cancelled scope lands: anyio refuses to + # cancel a task that has not started yet, so the streaming task always + # gets as far as its first await and no further. + await asyncio.sleep(0) + sent.append(message) + + # What the app's own lifespan does. Spelled out because this test drives + # the raw ASGI callable rather than TestClient, which cannot disconnect + # before reading the response. + async with httpx.AsyncClient( + transport=httpx.MockTransport( + lambda _request: httpx.Response( + 200, + content=_STREAM, + headers={"content-type": "text/event-stream"}, + ) + ) + ) as client: + app.state.upstream = client + await app(_scope(body), receive, send) + + # The premise of the test: nothing of the stream reached the client, + # so the disconnect really did land before the first chunk. + assert not any(message.get("body") for message in sent) + + store = app.state.usage_store + limiter = store._limits["producer"] + # The permit comes back through asyncio's async-generator + # finalization, which needs the suspended generator collected first: + # the cancelled task's traceback keeps it in a cycle, so plain + # reference counting will not do it. A generator that was never + # started is not registered for finalization at all, so under the old + # behaviour no number of collections brings its permit back. + for _ in range(50): + gc.collect() + await asyncio.sleep(0) + if not limiter.locked(): + break + + assert store.ledger.scopes["producer"].active_requests == 0 + assert not limiter.locked() + # The permit itself, not just the counter. With one permit in the + # scope, a second reservation can only be granted if the first one + # really came back, and hanging here forever is precisely how the two + # lost runs presented, so the timeout is the assertion. + await asyncio.wait_for(store.reserve("producer", "optimizer"), timeout=1) + + asyncio.run(drive()) + + +def test_primed_stream_still_delivers_every_upstream_byte(tmp_path): + """Not a regression test for the leak: a guard on the priming step itself. + + Priming consumes the first SSE event inside the handler, so this pins that the + client still receives that event, once and in order. Nothing else in the suite + compares a streamed body byte for byte, and a dropped first event would change + what every target agent reads. + """ + app = _streaming_app(tmp_path) + with TestClient(app) as client: + response = client.post( + "/scopes/producer/optimizer/v1/responses", + headers={"Authorization": "Bearer scoped-token"}, + json={"model": "gpt-test", "input": "hello", "stream": True}, + ) + + assert response.status_code == 200 + assert response.text == _STREAM + usage = app.state.usage_store.ledger.scopes["producer"] + assert usage.active_requests == 0 + assert usage.total_tokens == 8 diff --git a/vero/tests/test_idempotency_gitrepo.py b/vero/tests/test_idempotency_gitrepo.py new file mode 100644 index 00000000..82d4491b --- /dev/null +++ b/vero/tests/test_idempotency_gitrepo.py @@ -0,0 +1,184 @@ +"""Regression tests for the git plumbing a resumed run depends on. + +The motivating incident: a run died late, and the resume could not recognise +work its own previous attempt had already committed, because every save +produced a fresh sha from the wall clock. The other two cases here are the +crash residue that stopped the retry from getting that far at all: a git call +killed by the sandbox's 30 second default leaving an index lock, and a ref lock +left behind by a writer that was killed mid-transaction. +""" + +from __future__ import annotations + +import asyncio +import subprocess +from pathlib import Path + +import pytest + +from vero.candidate import Candidate +from vero.candidate_repository import GitCandidateRepository +from vero.sandbox import CommandResult, LocalSandbox +from vero.workspace import GitWorkspace + + +def _git(path: Path, *arguments: str) -> str: + result = subprocess.run( + ["git", *arguments], + cwd=path, + check=True, + capture_output=True, + text=True, + ) + return result.stdout.strip() + + +def _initialize(path: Path) -> str: + _git(path, "init", "-b", "main") + (path / "main.py").write_text("x = 1\n", encoding="utf-8") + _git(path, "add", "--all") + _git( + path, + "-c", + "user.name=vero", + "-c", + "user.email=vero@localhost", + "commit", + "-m", + "baseline", + ) + return _git(path, "rev-parse", "HEAD") + + +class _TimeoutRecordingSandbox(LocalSandbox): + """Local sandbox that remembers the timeout each command was given.""" + + def __init__(self, root: Path) -> None: + super().__init__(root=root) + self.timeouts: list[int | None] = [] + + async def run( + self, + command: str | list[str], + cwd: str | None = None, + timeout: int | None = 30, + env: dict[str, str] | None = None, + run_as: str | None = None, + ) -> CommandResult: + self.timeouts.append(timeout) + return await super().run( + command, cwd=cwd, timeout=timeout, env=env, run_as=run_as + ) + + +@pytest.mark.asyncio +async def test_workspace_git_calls_get_two_minutes(tmp_path: Path): + """A git call must not inherit the sandbox's 30 second default.""" + + _initialize(tmp_path) + sandbox = _TimeoutRecordingSandbox(tmp_path) + workspace = GitWorkspace(sandbox=sandbox, root=str(tmp_path)) + + await workspace.current_version() + + assert sandbox.timeouts == [120] + + +@pytest.mark.asyncio +async def test_save_pins_both_commit_dates(tmp_path: Path): + """Author and committer dates are pinned, so the sha is a function of content.""" + + _initialize(tmp_path) + sandbox = LocalSandbox(root=tmp_path) + workspace = await GitWorkspace.from_path(sandbox, str(tmp_path)) + + (tmp_path / "main.py").write_text("x = 2\n", encoding="utf-8") + await workspace.save("candidate") + + assert _git(tmp_path, "log", "-1", "--format=%at %ct") == "0 0" + + +@pytest.mark.asyncio +async def test_save_of_identical_content_reproduces_the_same_sha(tmp_path: Path): + """The sha a resumed run recomputes has to match the one it already stored.""" + + baseline = _initialize(tmp_path) + sandbox = LocalSandbox(root=tmp_path) + workspace = await GitWorkspace.from_path(sandbox, str(tmp_path)) + + (tmp_path / "main.py").write_text("x = 2\n", encoding="utf-8") + first = await workspace.save("candidate") + + # Rewind to the same parent and redo the identical save, the way a resumed + # run replays a round it had already completed. The sleep pushes the wall + # clock past a whole second, which is the granularity a commit date is + # stored at: without it the two commits could share a timestamp and the + # test would pass even with the dates unpinned. + _git(tmp_path, "reset", "--hard", baseline) + await asyncio.sleep(1.1) + (tmp_path / "main.py").write_text("x = 2\n", encoding="utf-8") + second = await workspace.save("candidate") + + assert second == first + + +@pytest.mark.asyncio +async def test_create_sweeps_stale_vero_ref_locks_only(tmp_path: Path): + """Opening the repository clears vero's own crash residue and nothing else.""" + + source = tmp_path / "source" + source.mkdir() + baseline_version = _initialize(source) + sandbox = await LocalSandbox.create(root=tmp_path) + workspace = await GitWorkspace.from_path(sandbox, str(source)) + repository = await GitCandidateRepository.create( + tmp_path / "session" / "candidates", + workspace=workspace, + ) + + candidate = Candidate.from_version(baseline_version, candidate_id="candidate") + stale_lock = repository.repository_path / ( + repository._candidate_ref(candidate.id) + ".lock" + ) + stale_lock.parent.mkdir(parents=True, exist_ok=True) + stale_lock.write_text("", encoding="utf-8") + + # Locks git owns are not ours to remove: a sweep wide enough to take these + # could stomp on a live git process, which is worse than the bug. + foreign_locks = [ + repository.repository_path / "packed-refs.lock", + repository.repository_path / "refs" / "heads" / "main.lock", + ] + for lock in foreign_locks: + lock.parent.mkdir(parents=True, exist_ok=True) + lock.write_text("", encoding="utf-8") + + await GitCandidateRepository.create(repository.root, workspace=workspace) + + assert not stale_lock.exists() + assert all(lock.exists() for lock in foreign_locks) + + +@pytest.mark.asyncio +async def test_capture_survives_a_ref_lock_left_by_a_dead_writer(tmp_path: Path): + """The next run captures normally instead of dying on a lock nobody holds.""" + + source = tmp_path / "source" + source.mkdir() + baseline_version = _initialize(source) + sandbox = await LocalSandbox.create(root=tmp_path) + workspace = await GitWorkspace.from_path(sandbox, str(source)) + session_root = tmp_path / "session" / "candidates" + repository = await GitCandidateRepository.create(session_root, workspace=workspace) + + candidate = Candidate.from_version(baseline_version, candidate_id="candidate") + stale_lock = repository.repository_path / ( + repository._candidate_ref(candidate.id) + ".lock" + ) + stale_lock.parent.mkdir(parents=True, exist_ok=True) + stale_lock.write_text("", encoding="utf-8") + + resumed = await GitCandidateRepository.create(session_root, workspace=workspace) + await resumed.capture(candidate, workspace) + + assert resumed.get(candidate.id) == candidate diff --git a/vero/tests/test_idempotency_harborcli.py b/vero/tests/test_idempotency_harborcli.py new file mode 100644 index 00000000..3a8462de --- /dev/null +++ b/vero/tests/test_idempotency_harborcli.py @@ -0,0 +1,244 @@ +"""Durability regressions for the Harbor CLI's terminal writes. + +Every file this module is about is written once, at the end of a run that took +hours, and is the only copy of what the run produced: reward.json, the session +archive, and the archive's checksum. The tests below pin the ordering and the +durability properties that make those files survive a crash or a partial +failure, not the numbers inside them. +""" + +from __future__ import annotations + +import io +import json +import os +import stat +import urllib.request +from pathlib import Path + +import pytest +from click.testing import CliRunner + +import vero.harbor.cli as harbor_cli +import vero.report as report_module +from vero.cli import main +from vero.layout import LAYOUT +from vero.sidecar.auth import write_admin_token +from vero.sidecar.session import file_sha256 + + +def _record_fsynced_inodes(monkeypatch) -> list[int]: + """Collect the inode of every directory handed to ``os.fsync``.""" + inodes: list[int] = [] + real_fsync = os.fsync + + def recording_fsync(descriptor): + info = os.fstat(descriptor) + if stat.S_ISDIR(info.st_mode): + inodes.append(info.st_ino) + return real_fsync(descriptor) + + monkeypatch.setattr(os, "fsync", recording_fsync) + return inodes + + +def test_atomic_write_bytes_fsyncs_the_parent_directory(tmp_path, monkeypatch): + inodes = _record_fsynced_inodes(monkeypatch) + destination = tmp_path / "verifier" / "reward.json" + + harbor_cli._atomic_write_bytes(destination, b'{"reward": 0.0}\n') + + assert destination.read_bytes() == b'{"reward": 0.0}\n' + assert destination.parent.stat().st_ino in inodes + + +def test_download_fsyncs_the_parent_directory(tmp_path, monkeypatch): + monkeypatch.setenv(LAYOUT.eval_url_env, "http://sidecar") + monkeypatch.setattr( + urllib.request, + "urlopen", + lambda request: io.BytesIO(b"sidecar archive"), + ) + inodes = _record_fsynced_inodes(monkeypatch) + destination = tmp_path / "verifier" / "session.tar.gz" + + harbor_cli._download("/session/export", destination) + + assert destination.read_bytes() == b"sidecar archive" + assert destination.parent.stat().st_ino in inodes + + +def test_finalize_routes_both_records_through_the_atomic_writer(tmp_path, monkeypatch): + token_file = write_admin_token(tmp_path / "token", "admin-secret") + output = tmp_path / "verifier" / "reward.json" + written: list[Path] = [] + real_atomic = harbor_cli._atomic_write_bytes + + def recording_atomic(path, payload): + written.append(Path(path)) + return real_atomic(path, payload) + + monkeypatch.setattr(harbor_cli, "_atomic_write_bytes", recording_atomic) + monkeypatch.setattr( + harbor_cli, + "_request", + lambda method, path, *, payload=None, headers=None: { + "rewards": {"reward": 0.25}, + "baseline_rewards": {"reward": 0.1}, + "errors": {}, + }, + ) + + result = CliRunner().invoke( + main, + [ + "harbor", + "finalize", + "--token-file", + str(token_file), + "--output", + str(output), + ], + ) + + assert result.exit_code == 0, result.output + assert written == [output, output.parent / "finalization.json"] + assert json.loads(output.read_text()) == {"reward": 0.25} + finalization = json.loads((output.parent / "finalization.json").read_text()) + assert finalization["baseline_rewards"] == {"reward": 0.1} + # A rename-published write leaves no temporary behind for Harbor to trip on. + assert sorted(path.name for path in output.parent.iterdir()) == [ + "finalization.json", + "reward.json", + ] + + +class _ExportHarness: + """Mocked sidecar for ``export-session``: no HTTP, no real archive.""" + + def __init__(self, tmp_path: Path, monkeypatch) -> None: + self.requests: list[tuple[str, str]] = [] + self.archived_finalizations: list[object] = [] + self.token_file = write_admin_token(tmp_path / "token", "admin-secret") + self.output = tmp_path / "logs" / "session.tar.gz" + self.report = tmp_path / "logs" / "experiment.html" + self.status_output = tmp_path / "logs" / "status.json" + self.finalization_output = tmp_path / "logs" / "finalization.json" + self.trace = tmp_path / "trajectory.json" + self.trace.write_text("[]\n") + self.extract_error: Exception | None = None + + def fake_request(method, path, *, payload=None, headers=None): + self.requests.append((method, path)) + if path == "/finalize": + return {"candidate": "posted", "rewards": {"reward": 0.0}} + return {"submit_enabled": False, "evaluation_access": []} + + def fake_download(path, destination, *, headers=None): + destination.write_bytes(b"sidecar archive") + + def fake_extract(_archive, destination): + if self.extract_error is not None: + raise self.extract_error + session = destination / "session" + session.mkdir(parents=True) + (session / "harbor-session.json").write_text("{}\n") + return session + + async def fake_report(session, destination): + # The archived finalization is what the report is built from, so + # read it here to prove which copy the export actually used. + self.archived_finalizations.append( + json.loads((session / "harbor-finalization.json").read_text()) + ) + destination.write_text("experiment\n") + return destination + + monkeypatch.setattr(harbor_cli, "_request", fake_request) + monkeypatch.setattr(harbor_cli, "_download", fake_download) + monkeypatch.setattr(harbor_cli, "extract_harbor_session_archive", fake_extract) + monkeypatch.setattr(report_module, "generate_experiment_report", fake_report) + + def invoke(self): + return CliRunner().invoke( + main, + [ + "harbor", + "export-session", + "--token-file", + str(self.token_file), + "--output", + str(self.output), + "--report-output", + str(self.report), + "--status-output", + str(self.status_output), + "--finalization-output", + str(self.finalization_output), + "--agent-trace", + str(self.trace), + ], + ) + + +def test_export_session_keeps_the_raw_archive_when_augmentation_fails( + tmp_path, monkeypatch +): + harness = _ExportHarness(tmp_path, monkeypatch) + harness.extract_error = ValueError("unsafe Harbor session archive member: session") + + result = harness.invoke() + + assert result.exit_code != 0 + # The download landed at --output before anything could fail, so the + # operator still holds the un-augmented archive rather than nothing. + assert harness.output.read_bytes() == b"sidecar archive" + + +def test_export_session_writes_the_checksum_before_the_report(tmp_path, monkeypatch): + harness = _ExportHarness(tmp_path, monkeypatch) + real_atomic = harbor_cli._atomic_write_bytes + + def failing_atomic(path, payload): + if Path(path) == harness.report: + raise OSError("simulated /logs exhaustion") + return real_atomic(path, payload) + + monkeypatch.setattr(harbor_cli, "_atomic_write_bytes", failing_atomic) + + result = harness.invoke() + + assert result.exit_code != 0 + assert not harness.report.exists() + checksum = harness.output.with_name(f"{harness.output.name}.sha256") + assert checksum.read_text() == f"{file_sha256(harness.output)} session.tar.gz\n" + + +@pytest.mark.parametrize( + "payload", + ['{"candidate": "planted", "rewards": {"reward": 1.0}}', "{ truncated", ""], +) +def test_export_session_never_trusts_a_finalization_it_finds_on_disk( + tmp_path, monkeypatch, payload +): + """The archived held-out result must come from the sidecar, always. + + Skipping the POST when --finalization-output already exists would save the + run's second finalize, and it is tempting because the generated verifier + script writes that file moments earlier. It is also a forgery vector: the + default path is under /logs, which candidate harness code can write in the + topology these benchmarks run in, so a planted record would reach the + archive, the status file and the report. A trusted reusable copy has to live + on the admin volume instead. + """ + + harness = _ExportHarness(tmp_path, monkeypatch) + harness.finalization_output.parent.mkdir(parents=True, exist_ok=True) + harness.finalization_output.write_text(payload) + + result = harness.invoke() + + assert result.exit_code == 0, result.output + assert ("POST", "/finalize") in harness.requests + assert harness.archived_finalizations[0]["candidate"] == "posted" + assert json.loads(harness.finalization_output.read_text())["candidate"] == "posted" diff --git a/vero/tests/test_idempotency_optimizer.py b/vero/tests/test_idempotency_optimizer.py new file mode 100644 index 00000000..9a5ca403 --- /dev/null +++ b/vero/tests/test_idempotency_optimizer.py @@ -0,0 +1,252 @@ +from __future__ import annotations + +import subprocess +import sys +from pathlib import Path + +import pytest + +from vero.candidate_repository import GitCandidateRepository +from vero.evaluation import ( + BackendRegistry, + CommandBackend, + CommandBackendConfig, + EvaluationDatabase, + EvaluationEngine, + EvaluationPlan, + EvaluationSet, + Evaluator, + MetricSelector, + ObjectiveSpec, + allow_all_evaluations, +) +from vero.optimization import ( + CandidateChange, + CandidateProposal, + Optimizer, +) +from vero.sandbox import LocalSandbox +from vero.workspace import GitWorkspace + + +def initialize_repository(path: Path) -> str: + subprocess.run( + ["git", "init", "-b", "main"], + cwd=path, + check=True, + capture_output=True, + ) + subprocess.run( + ["git", "add", "--all"], + cwd=path, + check=True, + capture_output=True, + ) + subprocess.run( + [ + "git", + "-c", + "user.name=vero", + "-c", + "user.email=vero@localhost", + "commit", + "-m", + "baseline", + ], + cwd=path, + check=True, + capture_output=True, + ) + return subprocess.run( + ["git", "rev-parse", "HEAD"], + cwd=path, + check=True, + capture_output=True, + text=True, + ).stdout.strip() + + +class FixedProposalStrategy: + """Propose one proposal with a stable ID so trial IDs are predictable.""" + + async def propose(self, _context): + return [CandidateProposal(id="proposal-a", producer_id="default")] + + +class RepeatingProducer: + """Score one checkpoint twice, then edit and score the new checkpoint.""" + + def __init__(self): + self.receipts = [] + + async def produce(self, *, proposal, context, workspace, evaluation): + program = Path(workspace.project_path) / "program.txt" + program.write_text("fast\n", encoding="utf-8") + self.receipts.append( + await evaluation.evaluate( + evaluation="performance", + description="Score the fast implementation", + ) + ) + # The retry a flaky harness or an impatient agent produces: nothing in + # the workspace moved between these two calls. + self.receipts.append( + await evaluation.evaluate( + evaluation="performance", + description="Re-score the identical checkpoint", + ) + ) + program.write_text("faster\n", encoding="utf-8") + self.receipts.append( + await evaluation.evaluate( + evaluation="performance", + description="Score the faster implementation", + ) + ) + return CandidateChange(description="Finish repeating producer") + + +@pytest.mark.asyncio +async def test_repeated_agent_evaluation_reuses_the_unchanged_trial_candidate( + tmp_path: Path, +): + target = tmp_path / "target" + target.mkdir() + (target / "program.txt").write_text("slow\n", encoding="utf-8") + baseline_version = initialize_repository(target) + + runs = tmp_path / "harness-runs.txt" + harness = tmp_path / "harness" + harness.mkdir() + harness_script = harness / "evaluate.py" + harness_script.write_text( + """ +import json +import sys +from pathlib import Path + +workspace, report_path, runs_path = map(Path, sys.argv[1:]) +program = (workspace / "program.txt").read_text().strip() +latency = {"slow": 10.0, "fast": 1.0, "faster": 0.5}[program] +with runs_path.open("a", encoding="utf-8") as handle: + handle.write(program + "\\n") +report_path.write_text(json.dumps({ + "schema_version": 1, + "status": "success", + "metrics": {"latency_ms": latency}, +})) +""", + encoding="utf-8", + ) + + sandbox = await LocalSandbox.create(root=tmp_path) + workspace = await GitWorkspace.from_path(sandbox, str(target)) + session_dir = tmp_path / "sessions" / "repeat" + candidate_repository = await GitCandidateRepository.create( + session_dir / "candidates", workspace=workspace + ) + database = EvaluationDatabase(id="repeat") + engine = EvaluationEngine( + evaluator=Evaluator( + candidate_repository=candidate_repository, + sandbox=workspace.sandbox, + session_dir=session_dir, + ), + backends=BackendRegistry( + { + "command": CommandBackend( + CommandBackendConfig( + harness_root=str(harness), + command=[ + sys.executable, + str(harness_script), + "{workspace}", + "{report}", + str(runs), + ], + ) + ) + } + ), + database=database, + database_path=session_dir / "database.json", + authorization_resolver=allow_all_evaluations, + ) + producer = RepeatingProducer() + optimizer = Optimizer( + workspace=workspace, + candidate_repository=candidate_repository, + engine=engine, + backend_id="command", + evaluation_plan=EvaluationPlan.single(EvaluationSet(name="performance")), + objective=ObjectiveSpec( + selector=MetricSelector(metric="latency_ms"), + direction="minimize", + ), + strategy=FixedProposalStrategy(), + producers={"default": producer}, + max_proposals=1, + ) + + result = await optimizer.run() + + # The repeat did not skip the measurement: the harness ran once for the + # baseline and once per gateway call, and every call returned its own + # evaluation with a real score for the content it saw. + assert runs.read_text(encoding="utf-8").split() == [ + "slow", + "fast", + "fast", + "faster", + ] + assert len({receipt.evaluation_id for receipt in producer.receipts}) == 3 + assert [receipt.result.objective.value for receipt in producer.receipts] == [ + 1.0, + 1.0, + 0.5, + ] + + # The unchanged repeat reused trial 1 instead of minting a second identity + # for byte-identical content, so only the genuine edit advanced the counter. + trials = sorted( + ( + candidate + for candidate in result.candidates + if "trial" in candidate.metadata + ), + key=lambda candidate: int(candidate.metadata["trial"]), + ) + assert [candidate.id for candidate in trials] == [ + "proposal-a:trial:1", + "proposal-a:trial:2", + ] + assert [candidate.metadata["trial"] for candidate in trials] == [1, 2] + assert trials[0].parent_id == baseline_version + assert trials[1].parent_id == "proposal-a:trial:1" + # The durable archive holds the baseline plus exactly one candidate per + # distinct checkpoint, which is what lets a resume recognize the same work. + assert len(candidate_repository.list()) == 3 + + trial_records = [ + record + for record in result.evaluations + if "trial" in record.request.candidate.metadata + ] + assert len(trial_records) == 3 + candidate_ids = [record.request.candidate.id for record in trial_records] + assert candidate_ids == [ + "proposal-a:trial:1", + "proposal-a:trial:1", + "proposal-a:trial:2", + ] + versions = [record.request.candidate.version for record in trial_records] + assert versions[0] == versions[1] + assert versions[2] != versions[0] + + # Scores are untouched by the deduplication: the baseline and every trial + # keep the value their own measurement produced, and the winner is still + # the fastest checkpoint. + assert result.baseline.objective.value == 10.0 + assert [record.objective.value for record in trial_records] == [1.0, 1.0, 0.5] + assert result.best.objective.value == 0.5 + assert result.best.request.candidate.id == "proposal-a:trial:2" diff --git a/vero/tests/test_idempotency_reportevents.py b/vero/tests/test_idempotency_reportevents.py new file mode 100644 index 00000000..4c98ac79 --- /dev/null +++ b/vero/tests/test_idempotency_reportevents.py @@ -0,0 +1,242 @@ +"""Regressions for surviving a process death mid-write of the event log and report.""" + +from __future__ import annotations + +import asyncio +import json +import os +from datetime import UTC, datetime +from pathlib import Path +from typing import Any + +import pytest + +from vero.evaluation import ( + BackendProvenance, + EvaluationPlan, + EvaluationSet, + MetricSelector, + ObjectiveSpec, +) +from vero.report import build_experiment_report_data, generate_experiment_report +from vero.runtime import ( + JsonlEventSink, + OptimizationComponentSpec, + OptimizationRunSpec, + RuntimeEvent, + SessionManifest, + SessionStatus, +) + +SESSION_ID = "idempotency-reportevents" + + +def write_session(root: Path) -> Path: + """Write the smallest session a report can be generated from. + + The candidate repository family is deliberately not ``git`` so these tests + exercise the event log and the report write without paying for a real + candidate repository; the events and the HTML output are what is under test. + """ + created = datetime(2026, 1, 1, tzinfo=UTC) + component = OptimizationComponentSpec(type="test", config_digest="0" * 64) + manifest = SessionManifest( + id=SESSION_ID, + status=SessionStatus.COMPLETED, + backend_id="test", + backend=BackendProvenance.from_config(name="test", version="1", config={}), + candidate_repository_family="memory", + candidate_repository_format_version=1, + evaluation_plan=EvaluationPlan.single(EvaluationSet(name="development")), + objective=ObjectiveSpec( + selector=MetricSelector(metric="score"), direction="maximize" + ), + run=OptimizationRunSpec( + max_proposals=1, + max_rounds=1, + max_concurrency=1, + strategy=component, + producers={"test": component}, + ), + created_at=created, + updated_at=created, + ) + session_dir = root / "session" + session_dir.mkdir(parents=True) + (session_dir / "manifest.json").write_text( + manifest.model_dump_json(indent=2), encoding="utf-8" + ) + return session_dir + + +def event_line(kind: str) -> str: + return RuntimeEvent( + session_id=SESSION_ID, + kind=kind, + created_at=datetime(2026, 1, 1, tzinfo=UTC), + payload={"note": "kept"}, + ).model_dump_json() + + +def test_report_survives_a_torn_last_event_line(tmp_path: Path): + session_dir = write_session(tmp_path) + intact = event_line("evaluation_completed") + torn = event_line("verification_completed") + # A SIGKILL mid-append leaves exactly this shape: complete records, then one + # half-written record with no trailing newline. + (session_dir / "events.jsonl").write_text( + f"{intact}\n{torn[: len(torn) // 2]}", encoding="utf-8" + ) + + data = asyncio.run(build_experiment_report_data(session_dir)) + + assert [event["kind"] for event in data["events"]] == ["evaluation_completed"] + assert data["skipped_event_lines"] == 1 + + +def test_report_survives_a_torn_multibyte_character_in_the_event_log(tmp_path: Path): + session_dir = write_session(tmp_path) + intact = event_line("evaluation_completed") + # Events are written with ensure_ascii=False, so a death in the middle of a + # multi-byte character leaves undecodable bytes rather than bad JSON. + (session_dir / "events.jsonl").write_bytes( + intact.encode("utf-8") + b"\n" + '{"payload": "café'.encode()[:-1] + ) + + data = asyncio.run(build_experiment_report_data(session_dir)) + + assert [event["kind"] for event in data["events"]] == ["evaluation_completed"] + assert data["skipped_event_lines"] == 1 + + +def test_report_counts_no_skipped_lines_for_an_intact_event_log(tmp_path: Path): + session_dir = write_session(tmp_path) + (session_dir / "events.jsonl").write_text( + f"{event_line('evaluation_completed')}\n", encoding="utf-8" + ) + + data = asyncio.run(build_experiment_report_data(session_dir)) + + assert [event["kind"] for event in data["events"]] == ["evaluation_completed"] + assert data["skipped_event_lines"] == 0 + + +def test_experiment_html_write_that_dies_keeps_the_previous_report( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + session_dir = write_session(tmp_path) + destination = asyncio.run(generate_experiment_report(session_dir)) + previous = destination.read_text(encoding="utf-8") + + def failing_replace(*arguments: object, **keywords: object) -> None: + raise OSError("simulated death while publishing the report") + + monkeypatch.setattr(os, "replace", failing_replace) + + with pytest.raises(OSError): + asyncio.run(generate_experiment_report(session_dir)) + + monkeypatch.undo() + assert destination.read_text(encoding="utf-8") == previous + assert [path.name for path in session_dir.glob("*.tmp")] == [] + + +def test_experiment_html_closes_its_descriptor_when_fdopen_fails( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + """A failure between mkstemp and fdopen must close the raw descriptor once. + + Staging the report through a temporary file opens a window where the bare + descriptor is nobody's responsibility but this function's. Closing it from a + shared failure path instead would be a double close once fdopen has + succeeded, and a second close can land on a descriptor the interpreter has + since handed to something else. + """ + + session_dir = write_session(tmp_path) + closed: list[int] = [] + real_close = os.close + + def fail_fdopen(*_arguments: object, **_keywords: object) -> None: + raise RuntimeError("fdopen failed") + + def tracked_close(descriptor: int) -> None: + closed.append(descriptor) + real_close(descriptor) + + monkeypatch.setattr(os, "fdopen", fail_fdopen) + monkeypatch.setattr(os, "close", tracked_close) + + with pytest.raises(RuntimeError, match="fdopen failed"): + asyncio.run(generate_experiment_report(session_dir)) + + monkeypatch.undo() + assert len(closed) == 1 + assert [path.name for path in session_dir.glob("*.tmp")] == [] + + +class RecordingHandle: + """Delegate to a real file handle while recording every write it receives.""" + + def __init__(self, handle: Any, writes: list[Any]): + self._handle = handle + self._writes = writes + + def write(self, payload: Any) -> int: + self._writes.append(payload) + return self._handle.write(payload) + + def flush(self) -> None: + self._handle.flush() + + def fileno(self) -> int: + return self._handle.fileno() + + def __enter__(self) -> RecordingHandle: + self._handle.__enter__() + return self + + def __exit__(self, *arguments: object) -> None: + self._handle.__exit__(*arguments) + + +def test_event_sink_appends_record_and_newline_in_one_fsynced_write( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +): + path = tmp_path / "session" / "events.jsonl" + sink = JsonlEventSink(path) + modes: list[str] = [] + writes: list[Any] = [] + fsynced: list[int] = [] + real_open = Path.open + real_fsync = os.fsync + + def recording_open( + self: Path, mode: str = "r", *arguments: object, **keywords: object + ) -> Any: + handle = real_open(self, mode, *arguments, **keywords) # type: ignore[arg-type] + if self != path: + return handle + modes.append(mode) + return RecordingHandle(handle, writes) + + def recording_fsync(descriptor: int) -> None: + fsynced.append(descriptor) + real_fsync(descriptor) + + monkeypatch.setattr(Path, "open", recording_open) + monkeypatch.setattr(os, "fsync", recording_fsync) + + # A payload well past any buffer is the case that used to flush the record + # without its newline and fuse it with the next record. + event = RuntimeEvent( + session_id=SESSION_ID, kind="agent", payload={"text": "x" * 200_000} + ) + asyncio.run(sink(event)) + + monkeypatch.undo() + assert modes == ["ab"] + expected = json.dumps(event.model_dump(mode="json"), ensure_ascii=False) + assert writes == [f"{expected}\n".encode()] + assert fsynced + assert json.loads(path.read_text(encoding="utf-8"))["id"] == event.id diff --git a/vero/tests/test_idempotency_seedsh.py b/vero/tests/test_idempotency_seedsh.py new file mode 100644 index 00000000..75e71c46 --- /dev/null +++ b/vero/tests/test_idempotency_seedsh.py @@ -0,0 +1,194 @@ +"""Boot-twice behaviour of the compiled seed script. + +The seed script is the main service's compose `command`, so Docker runs it again +every time it restarts a crashed main container, against the same `agent_repo` +volume. That makes it a first-boot script that in practice boots many times, and +the two things it did unconditionally both compounded across a long run: the +`.git/info/exclude` appends duplicated themselves on every boot, and the baked +workspace overlay was copied back over work the optimizer had already done. + +These tests therefore execute the rendered script rather than grep its text: the +only way to tell "applied once" from "applied again" is to run it twice and look +at the workspace afterwards. The container paths are rewritten onto a sandbox +under tmp_path, and `chown` / `git` are stubbed on PATH because the real ones +need root and a system gitconfig that a test must not touch. +""" + +from __future__ import annotations + +import os +import subprocess +from collections.abc import Callable +from pathlib import Path + +from vero.harbor import ( + AgentAccessSpec, + HarborBuildConfig, + VerificationTargetSpec, + WorkspaceOverlaySpec, + compile_harbor_task, +) +from vero.layout import LAYOUT + +_OVERLAY_SKILL = "# baked overlay skill\n" + + +def _git(path: Path, *arguments: str) -> None: + subprocess.run( + ["git", *arguments], cwd=path, check=True, text=True, capture_output=True + ) + + +def _config(tmp_path: Path) -> HarborBuildConfig: + """The smallest build that still bakes an overlay into the workspace.""" + target = tmp_path / "target" + target.mkdir(parents=True) + _git(target, "init", "-q") + _git(target, "config", "user.name", "VeRO Test") + _git(target, "config", "user.email", "vero@example.test") + (target / "README.md").write_text("# Target\n", encoding="utf-8") + _git(target, "add", ".") + _git(target, "commit", "-q", "-m", "target baseline") + + task_source = tmp_path / "tasks" + task_source.mkdir() + for name in ("task-a", "task-b", "task-c", "task-d", "task-e", "task-hidden"): + task = task_source / name + task.mkdir() + (task / "task.toml").write_text( + f'[task]\nname="org/{name}"\n', encoding="utf-8" + ) + + bundle = tmp_path / "bundle" / "skills" / "insights" + bundle.mkdir(parents=True) + (bundle / "SKILL.md").write_text(_OVERLAY_SKILL, encoding="utf-8") + + return HarborBuildConfig( + name="org/optimize-program", + description="Improve the program", + agent_repo=str(target), + task_source=str(task_source), + agent_import_path="target.agent:Agent", + harbor_requirement="harbor==0.1.17", + partitions={ + "validation": ["task-a", "task-b", "task-c", "task-d", "task-e"], + "test": ["task-hidden"], + }, + agent_access=[ + AgentAccessSpec( + partition="validation", + expose_case_resources=True, + total_runs=5, + total_cases=25, + ) + ], + selection_partition="validation", + targets=[VerificationTargetSpec(partition="test")], + workspace_overlays=[ + WorkspaceOverlaySpec( + source=str(tmp_path / "bundle" / "skills"), dest="skills" + ) + ], + ) + + +def _sandbox(tmp_path: Path, seed_script: str) -> tuple[Path, Callable[[], None]]: + """Stage a runnable copy of the rendered seed script and return (work, boot). + + The script's paths are absolute container paths, so the rewrite below is what + makes it executable on the host at all. `exec sleep infinity` is dropped for + the obvious reason: the real script never returns. + """ + root = tmp_path / "sandbox" + seed_repo = root / "seed" + overlay = root / "overlay" + work = root / "work" + stubs = root / "bin" + for directory in (overlay, work, stubs): + directory.mkdir(parents=True) + + # The seed repo carries the .git the first-boot guard keys on, including the + # info/exclude that git itself creates, so the appends land in a real file. + (seed_repo / ".git" / "info").mkdir(parents=True) + (seed_repo / ".git" / "info" / "exclude").write_text( + "# git ls-files --others --exclude-from=.git/info/exclude\n", encoding="utf-8" + ) + (seed_repo / "README.md").write_text("# Target\n", encoding="utf-8") + (overlay / "skills" / "insights").mkdir(parents=True) + (overlay / "skills" / "insights" / "SKILL.md").write_text( + _OVERLAY_SKILL, encoding="utf-8" + ) + for stub in ("chown", "git"): + path = stubs / stub + path.write_text("#!/bin/sh\nexit 0\n", encoding="utf-8") + path.chmod(0o755) + + body = ( + seed_script.replace(LAYOUT.overlay, str(overlay)) + .replace(LAYOUT.seed_repo, str(seed_repo)) + .replace(LAYOUT.target_repo, str(work)) + .replace("exec sleep infinity\n", "") + ) + assert "/work/agent" not in body and "/opt/" not in body + script = root / "seed.sh" + script.write_text(body, encoding="utf-8") + + def boot() -> None: + subprocess.run( + ["/bin/sh", str(script)], + check=True, + text=True, + capture_output=True, + env={"PATH": f"{stubs}{os.pathsep}{os.defpath}", "HOME": str(root)}, + ) + + return work, boot + + +def test_seed_script_appends_git_excludes_once_across_boots(tmp_path): + """A restart must not append the same ignore lines a second time. + + Before this was guarded, a run that bounced the main container a dozen times + left a dozen copies of /.evals/ and /skills/ in .git/info/exclude, and the + file grew for as long as the run lasted. + """ + output = compile_harbor_task(_config(tmp_path), tmp_path / "compiled") + seed_script = (output / "environment/main/seed.sh").read_text(encoding="utf-8") + work, boot = _sandbox(tmp_path, seed_script) + + boot() + exclude = work / ".git" / "info" / "exclude" + first = exclude.read_text(encoding="utf-8").splitlines() + assert first.count("/.evals/") == 1 + assert first.count("/skills/") == 1 + + boot() + boot() + after = exclude.read_text(encoding="utf-8").splitlines() + assert after.count("/.evals/") == 1 + assert after.count("/skills/") == 1 + # Nothing else in the file moved either, so the guard is not rewriting it. + assert after == first + + +def test_seed_script_does_not_reapply_the_overlay_after_a_restart(tmp_path): + """A restart must leave the optimizer's edits under overlay paths alone. + + The overlay copy used to sit outside the first-boot guard, so a container + that came back after a crash re-applied the baked skills over whatever the + optimizer had written there, reverting its work without a word. + """ + output = compile_harbor_task(_config(tmp_path), tmp_path / "compiled") + seed_script = (output / "environment/main/seed.sh").read_text(encoding="utf-8") + work, boot = _sandbox(tmp_path, seed_script) + + boot() + skill = work / "skills" / "insights" / "SKILL.md" + assert skill.read_text(encoding="utf-8") == _OVERLAY_SKILL + + # Stand in for the optimizer editing an overlay-provided file, which is + # exactly what a second copy of the baked overlay would overwrite. + skill.write_text("# edited by the optimizer\n", encoding="utf-8") + boot() + + assert skill.read_text(encoding="utf-8") == "# edited by the optimizer\n" diff --git a/vero/tests/test_idempotency_session.py b/vero/tests/test_idempotency_session.py new file mode 100644 index 00000000..1093c348 --- /dev/null +++ b/vero/tests/test_idempotency_session.py @@ -0,0 +1,131 @@ +"""Regressions for the session-level idempotency of a rerun. + +Every case here stands for a way a relaunch after a crash used to lose work: a +first durable write that erased the previous death's diagnosis, and two live +processes silently sharing one session directory. + +The deterministic default session identity that belongs beside these was backed +out of this branch: making it stable is correct, but it turns every rerun into a +resume, and on a session whose only stored baseline record is unusable the +resumed run adopts that record and measures nothing. That needs the baseline +reuse guards, which change what gets measured, so both land together or not at +all. +""" + +from __future__ import annotations + +import fcntl +import json +import os +from pathlib import Path + +import pytest + +from tests.test_v05_runtime_session import StubOptimizer +from vero.runtime import OptimizationSession, SessionStatus + + +@pytest.mark.asyncio +async def test_running_manifest_keeps_the_previous_failure(tmp_path: Path): + """A rerun must not erase how the previous attempt died. + + The RUNNING write is a rerun's first durable act, so clearing `failure` + there destroyed the only recorded explanation of the death before anyone + read it. Only a run that actually completed may drop it. + """ + + session_dir = tmp_path / "sessions" / "kept-failure" + failing = OptimizationSession( + id="kept-failure", + session_dir=session_dir, + optimizer=StubOptimizer(session_dir, failure=RuntimeError("producer exploded")), + ) + with pytest.raises(RuntimeError, match="producer exploded"): + await failing.run() + assert failing.load_manifest().failure.message == "producer exploded" + + observed: list[dict] = [] + + class _ObservingOptimizer(StubOptimizer): + """Read the manifest exactly as the resumed run starts working.""" + + async def run(self, **kwargs): + observed.append( + json.loads((session_dir / "manifest.json").read_text(encoding="utf-8")) + ) + return await super().run(**kwargs) + + resumed = OptimizationSession( + id="kept-failure", + session_dir=session_dir, + optimizer=_ObservingOptimizer(session_dir), + ) + result = await resumed.run(skip_baseline_evaluation=True) + + assert observed[0]["status"] == SessionStatus.RUNNING.value + assert observed[0]["failure"]["message"] == "producer exploded" + # The completed write is where the obsolete explanation goes away, because + # only by then is there a result that supersedes it. + assert resumed.load_manifest().failure is None + assert result.best is not None + + +@pytest.mark.asyncio +async def test_session_refuses_a_second_process_on_one_directory(tmp_path: Path): + """A relaunch while the first run is still alive must be refused. + + Two processes over one session directory evaluate the same pending set + against separately loaded budget ledgers, so the budget is spent twice and + the manifests overwrite each other. + """ + + session_dir = tmp_path / "sessions" / "contended" + session = OptimizationSession( + id="contended", + session_dir=session_dir, + optimizer=StubOptimizer(session_dir), + ) + # Stand in for the live process: an flock held on another open file + # description is exactly what a second `vero run` would find. + holder = os.open(session_dir / "run.lock", os.O_CREAT | os.O_RDWR, 0o644) + fcntl.flock(holder, fcntl.LOCK_EX | fcntl.LOCK_NB) + os.write(holder, b"4242\n") + + with pytest.raises(RuntimeError, match="already being run by another process"): + await session.run() + # Refused before any durable write, so the live run's state is untouched. + assert not session.manifest_path.exists() + + # Closing is what a crash does too, so the relaunch this whole tranche + # exists to support proceeds, over a lock file that is still lying there. + os.close(holder) + result = await session.run() + + assert result.best is not None + assert session.load_manifest().status == SessionStatus.COMPLETED + + +@pytest.mark.asyncio +async def test_session_run_lock_names_the_holder_and_is_released_at_exit( + tmp_path: Path, +): + """The refusal has to answer "is a run still alive?" without guesswork.""" + + session_dir = tmp_path / "sessions" / "diagnosable" + session = OptimizationSession( + id="diagnosable", + session_dir=session_dir, + optimizer=StubOptimizer(session_dir), + ) + + await session.run() + + # The lock is released once run() returns, so a sequential rerun is free to + # take it; the pid breadcrumb left behind belongs to this process. + lock_path = session_dir / "run.lock" + assert lock_path.read_text(encoding="utf-8").strip() == str(os.getpid()) + descriptor = os.open(lock_path, os.O_CREAT | os.O_RDWR, 0o644) + try: + fcntl.flock(descriptor, fcntl.LOCK_EX | fcntl.LOCK_NB) + finally: + os.close(descriptor) diff --git a/vero/tests/test_idempotency_sidecar.py b/vero/tests/test_idempotency_sidecar.py new file mode 100644 index 00000000..61e565bf --- /dev/null +++ b/vero/tests/test_idempotency_sidecar.py @@ -0,0 +1,482 @@ +"""Restart- and crash-survival regressions for the sidecar process. + +Every test here stands for one way a run that had already paid for its work lost +it to a restart: an export that was named before its bytes were durable, an admin +token the outer agent could no longer use, an orphaned job whose evaluation could +not be found again, export scratch directories that filled the volume, and a +baseline measurement that existed only in one HTTP response. +""" + +from __future__ import annotations + +import json +import os +import stat +import tempfile +import time +from datetime import UTC, datetime +from pathlib import Path +from types import SimpleNamespace + +import pytest +from fastapi.testclient import TestClient + +from vero.candidate import Candidate +from vero.evaluation import ( + BackendProvenance, + BackendRegistry, + EvaluationCost, + EvaluationDatabase, + EvaluationRecord, + EvaluationReport, + EvaluationRequest, + EvaluationSet, + EvaluationStatus, + MetricSelector, + ObjectiveResult, + ObjectiveSpec, + RunningEvaluationManifest, +) +from vero.sidecar import ( + CanonicalVerifier, + EvaluationJobStatus, + EvaluationSidecar, + HarborSessionManifest, + SidecarEvaluationJob, + VerificationResult, + VerificationSelection, + VerificationTarget, +) +from vero.sidecar.app import create_app +from vero.sidecar.auth import read_admin_token, write_admin_token +from vero.sidecar.serve import SidecarComponents, build_app +from vero.sidecar.session import ( + create_harbor_session_archive, + extract_harbor_session_archive, +) + +OBJECTIVE = ObjectiveSpec( + selector=MetricSelector(metric="score"), + direction="maximize", +) +PROVENANCE = BackendProvenance(name="stub", version="1", config_digest="0" * 64) +EVALUATION_SET = EvaluationSet(name="benchmark", partition="validation") + + +def _candidate(version: str) -> Candidate: + return Candidate( + id=version, + version=version, + created_at=datetime(2026, 1, 1, tzinfo=UTC), + ) + + +class _StubBackend: + @property + def provenance(self) -> BackendProvenance: + return PROVENANCE + + async def resolve_cost(self, evaluation_set): + return EvaluationCost(cases=1) + + async def evaluate(self, *, context, request): + raise AssertionError("the fake engine answers evaluations directly") + + +class _StubSidecar: + """Only what the FastAPI transport touches on the export path.""" + + def __init__(self, session_dir: Path): + self.engine = SimpleNamespace( + evaluator=SimpleNamespace(session_dir=session_dir), + database=EvaluationDatabase(id="session"), + ) + + +class _StubVerifier: + async def finalize(self) -> VerificationResult: + return VerificationResult(rewards={"reward": 0.75}) + + +# ITEM 3c: the session archive is the only durable copy of a finished run. + + +def _session_manifest() -> HarborSessionManifest: + return HarborSessionManifest( + id="trial", + task_name="org/optimize", + created_at=datetime(2026, 7, 16, tzinfo=UTC), + backends={"validation": PROVENANCE}, + selection=VerificationSelection(mode="submit"), + targets=[ + VerificationTarget( + reward_key="reward", + backend_id="validation", + evaluation_set=EVALUATION_SET, + objective=OBJECTIVE, + ) + ], + ) + + +def test_session_archive_is_flushed_before_and_after_it_is_named( + tmp_path, + monkeypatch, +): + # The export must not publish the archive's final name over bytes that are + # still only in the page cache: fsync the archive, then rename, then fsync + # the directory that now carries the name. + session = tmp_path / "source" + session.mkdir() + (session / "harbor-session.json").write_text( + _session_manifest().model_dump_json(indent=2) + "\n" + ) + (session / "database.json").write_text('{"id":"trial"}\n') + destination = tmp_path / "out" / "session.tar.gz" + + events: list[str] = [] + real_fsync = os.fsync + real_replace = os.replace + + def record_fsync(descriptor): + kind = "directory" if stat.S_ISDIR(os.fstat(descriptor).st_mode) else "archive" + events.append(f"fsync-{kind}") + return real_fsync(descriptor) + + def record_replace(source, target): + events.append("replace") + return real_replace(source, target) + + monkeypatch.setattr(os, "fsync", record_fsync) + monkeypatch.setattr(os, "replace", record_replace) + + create_harbor_session_archive(session, destination) + + assert events == ["fsync-archive", "replace", "fsync-directory"] + # The archive itself must still be a readable session, not just durable. + extracted = extract_harbor_session_archive(destination, tmp_path / "extracted") + assert (extracted / "database.json").read_text() == '{"id":"trial"}\n' + + +# ITEM 38: a restart must not invalidate the token the outer agent already holds. + + +async def _stub_components(*, factory_path, config_path) -> SidecarComponents: + return SidecarComponents( + sidecar=_StubSidecar(Path(config_path).parent), + verifier=_StubVerifier(), + ) + + +@pytest.mark.asyncio +async def test_build_app_reuses_an_admin_token_the_agent_already_holds( + tmp_path, + monkeypatch, +): + # Minting a fresh token on every start meant a mid-run sidecar restart 401'd + # the outer agent's next admin call even though every other piece of run state + # had survived. + monkeypatch.setattr("vero.sidecar.serve.build_components", _stub_components) + token_path = tmp_path / "admin" / "token" + write_admin_token(token_path, "token-the-agent-is-holding") + + app = await build_app( + factory_path="module:attribute", + config_path=tmp_path / "config.json", + admin_token_path=token_path, + ) + + client = TestClient(app) + response = client.post( + "/finalize", + headers={"Authorization": "Bearer token-the-agent-is-holding"}, + ) + assert response.status_code == 200 + assert response.json()["rewards"] == {"reward": 0.75} + assert read_admin_token(token_path) == "token-the-agent-is-holding" + + +@pytest.mark.asyncio +async def test_build_app_still_mints_an_admin_token_on_a_first_start( + tmp_path, + monkeypatch, +): + # Companion to the reuse test above: reuse must not break the case the volume + # has no token yet, which is every run's first start. + monkeypatch.setattr("vero.sidecar.serve.build_components", _stub_components) + token_path = tmp_path / "admin" / "token" + + app = await build_app( + factory_path="module:attribute", + config_path=tmp_path / "config.json", + admin_token_path=token_path, + ) + + minted = read_admin_token(token_path) + assert minted + client = TestClient(app) + assert ( + client.post( + "/finalize", + headers={"Authorization": f"Bearer {minted}"}, + ).status_code + == 200 + ) + + +# ITEM 45: an orphaned job must keep pointing at the evaluation it was driving. + + +def _write_job( + session_dir: Path, + job_id: str, + *, + version: str | None, + created_at: datetime, + status: EvaluationJobStatus = EvaluationJobStatus.RUNNING, +) -> Path: + job = SidecarEvaluationJob( + job_id=job_id, + status=status, + backend_id="primary", + evaluation_set=EVALUATION_SET, + version=version, + created_at=created_at, + ) + path = session_dir / "evaluation-jobs" / f"{job_id}.json" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(job.model_dump_json(indent=2) + "\n") + return path + + +def _write_running_evaluation( + session_dir: Path, + evaluation_id: str, + *, + version: str, + created_at: datetime, +) -> None: + manifest = RunningEvaluationManifest( + id=evaluation_id, + request=EvaluationRequest( + candidate=_candidate(version), + evaluation_set=EVALUATION_SET, + ), + backend_id="primary", + backend=PROVENANCE, + created_at=created_at, + ) + path = session_dir / "evaluations" / evaluation_id / "evaluation.json" + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(manifest.model_dump_json(indent=2) + "\n") + + +def _restarted_sidecar(session_dir: Path) -> EvaluationSidecar: + return EvaluationSidecar( + engine=SimpleNamespace( + evaluator=SimpleNamespace( + session_dir=session_dir, + evaluations_dir=session_dir / "evaluations", + ), + backends={}, + database=EvaluationDatabase(id="session"), + budget_ledger=None, + ), + candidate_transport=None, + access_policies=[], + ) + + +def test_interrupted_job_keeps_the_evaluation_it_was_driving(tmp_path): + # Without the evaluation_id the interrupted job is a dead end: the budget its + # evaluation reserved can never be reconciled against it, and the cases it had + # already checkpointed belong to nothing. + session = tmp_path / "session" + started = datetime(2026, 7, 16, 12, 0, tzinfo=UTC) + job_path = _write_job(session, "job-1", version="candidate-1", created_at=started) + _write_running_evaluation( + session, + "evaluation-1", + version="candidate-1", + created_at=started, + ) + + job = _restarted_sidecar(session).evaluation_job("job-1") + + assert job.status == EvaluationJobStatus.FAILED + assert job.error == "evaluation job was interrupted by a sidecar restart" + assert job.evaluation_id == "evaluation-1" + assert json.loads(job_path.read_text())["evaluation_id"] == "evaluation-1" + + +def test_interrupted_job_declines_an_evaluation_it_may_not_own(tmp_path): + # Two indistinguishable mid-flight evaluations, and one that started before the + # job existed: none of them can be attributed to this job, and guessing would + # send the reconciler after a reservation another job still owns. + session = tmp_path / "session" + started = datetime(2026, 7, 16, 12, 0, tzinfo=UTC) + _write_job(session, "job-1", version="candidate-1", created_at=started) + _write_running_evaluation( + session, + "evaluation-1", + version="candidate-1", + created_at=started, + ) + _write_running_evaluation( + session, + "evaluation-2", + version="candidate-1", + created_at=started, + ) + _write_running_evaluation( + session, + "evaluation-earlier", + version="candidate-1", + created_at=datetime(2026, 7, 16, 11, 0, tzinfo=UTC), + ) + + job = _restarted_sidecar(session).evaluation_job("job-1") + + assert job.status == EvaluationJobStatus.FAILED + assert job.evaluation_id is None + + +# ITEM 47c: export scratch directories must not accumulate for a whole run. + + +def test_session_export_sweeps_stale_scratch_directories(tmp_path, monkeypatch): + # A crashed export never runs its cleanup background task, and the sidecar + # lives for the whole run, so the leftovers fill the volume until every later + # export fails with the session still unexported. + monkeypatch.setattr(tempfile, "tempdir", str(tmp_path)) + stale = tmp_path / "vero-harbor-export-crashed" + (stale / "leftovers").mkdir(parents=True) + fresh = tmp_path / "vero-harbor-export-inflight" + fresh.mkdir() + unrelated = tmp_path / "vero-inference-usage" + unrelated.mkdir() + long_ago = time.time() - 7200 + for path in (stale, unrelated): + os.utime(path, (long_ago, long_ago)) + + def create_archive(_session_dir, destination): + destination.write_bytes(b"portable-session") + return destination + + monkeypatch.setattr( + "vero.sidecar.app.create_harbor_session_archive", + create_archive, + ) + client = TestClient( + create_app( + sidecar=_StubSidecar(tmp_path / "session"), + verifier=_StubVerifier(), + admin_token="admin-secret", + ) + ) + + exported = client.get( + "/session/export", + headers={"Authorization": "Bearer admin-secret"}, + ) + + assert exported.content == b"portable-session" + assert not stale.exists() + # An export that may still be streaming to a concurrent caller, and anything + # outside the export prefix, are both left alone. + assert fresh.is_dir() + assert unrelated.is_dir() + + +# ITEM 36: a baseline measurement must survive a dropped response. + + +class _FakeEngine: + """Answers admin evaluations from a fixed score table.""" + + def __init__(self, scores: dict[tuple[str, str], list[float]]): + self.backends = BackendRegistry({"backend": _StubBackend()}) + self.database = EvaluationDatabase(id="session") + self.scores = scores + self.calls: list[tuple[str, str]] = [] + self._sequence = 0 + + async def evaluate_record( + self, + *, + backend_id, + request, + objective_spec, + authorization, + principal, + ) -> EvaluationRecord: + key = (request.candidate.version, request.evaluation_set.name) + self.calls.append(key) + score = self.scores[key].pop(0) + self._sequence += 1 + now = datetime(2026, 2, 1, tzinfo=UTC) + record = EvaluationRecord( + id=f"admin-{self._sequence}", + request=request, + report=EvaluationReport( + status=EvaluationStatus.SUCCESS, + metrics={"score": score}, + ), + backend_id=backend_id, + backend=PROVENANCE, + objective_spec=objective_spec, + objective=ObjectiveResult(value=score, feasible=True), + created_at=now, + completed_at=now, + ) + self.database.add_evaluation(record) + return record + + +@pytest.mark.asyncio +async def test_measure_baseline_persists_the_aggregate_it_returns(tmp_path): + # Held-out replicates are the most expensive scoring a run does, and the HTTP + # response used to be the only copy: a dropped connection meant paying for the + # whole measurement again. + baseline = _candidate("baseline") + engine = _FakeEngine( + { + ("baseline", "selection"): [0.5, 0.6], + ("baseline", "test"): [0.4, 0.5], + } + ) + verifier = CanonicalVerifier( + engine=engine, + selection=VerificationSelection( + mode="auto_best", + backend_id="backend", + evaluation_set=EvaluationSet(name="selection"), + objective=OBJECTIVE, + baseline_candidate=baseline, + ), + targets=[ + VerificationTarget( + reward_key="reward", + backend_id="backend", + evaluation_set=EvaluationSet(name="test"), + objective=OBJECTIVE, + max_attempts=1, + ) + ], + admin_volume=tmp_path, + ) + + result = await verifier.measure_baseline(replicates=2) + + stored = json.loads((tmp_path / "baseline.json").read_text()) + assert stored == result + # The aggregate itself is untouched by being persisted. + assert result["selection"]["mean"] == 0.55 + assert result["selection"]["n"] == 2 + assert result["targets"]["reward"]["mean"] == 0.45 + assert engine.calls == [ + ("baseline", "selection"), + ("baseline", "selection"), + ("baseline", "test"), + ("baseline", "test"), + ] diff --git a/vero/tests/test_idempotency_store.py b/vero/tests/test_idempotency_store.py new file mode 100644 index 00000000..a8aaa435 --- /dev/null +++ b/vero/tests/test_idempotency_store.py @@ -0,0 +1,165 @@ +"""Durability regressions for the evaluation store's one atomic writer and index.""" + +from __future__ import annotations + +import json +import os +import stat +import tempfile +from datetime import UTC, datetime, timedelta +from pathlib import Path + +import pytest + +import vero.evaluation.store.persistence as persistence +from vero.candidate import Candidate +from vero.evaluation import ( + BackendProvenance, + CaseResult, + CaseStatus, + EvaluationDatabase, + EvaluationRecord, + EvaluationReport, + EvaluationRequest, + EvaluationStatus, + EvaluationStore, +) + + +def record(candidate_id: str = "candidate") -> EvaluationRecord: + created_at = datetime(2026, 1, 1, tzinfo=UTC) + return EvaluationRecord( + id=f"evaluation:{candidate_id}", + request=EvaluationRequest( + candidate=Candidate( + id=candidate_id, + version=f"version:{candidate_id}", + created_at=created_at, + ) + ), + report=EvaluationReport( + status=EvaluationStatus.SUCCESS, + metrics={"score": 1.0}, + cases=[ + CaseResult( + case_id="case-one", + status=CaseStatus.SUCCESS, + metrics={"score": 1.0}, + ) + ], + ), + backend_id="default", + backend=BackendProvenance(name="stub", version="1", config_digest="0" * 64), + created_at=created_at, + completed_at=created_at + timedelta(seconds=1), + ) + + +def test_atomic_write_fsyncs_the_parent_directory_after_the_rename( + tmp_path: Path, + monkeypatch, +): + """The rename itself has to be flushed, not just the bytes it publishes.""" + + real_fsync = os.fsync + fsynced_directories: list[str] = [] + + def tracking_fsync(descriptor: int) -> None: + if stat.S_ISDIR(os.fstat(descriptor).st_mode): + fsynced_directories.append(str(descriptor)) + real_fsync(descriptor) + + monkeypatch.setattr(persistence.os, "fsync", tracking_fsync) + + target = tmp_path / "session" / "database.json" + persistence._atomic_write_json(target, {"schema_version": 1}) + monkeypatch.undo() + + assert json.loads(target.read_text(encoding="utf-8")) == {"schema_version": 1} + assert fsynced_directories, "the parent directory was never fsynced" + + +def test_atomic_write_does_not_close_a_descriptor_it_handed_to_the_file_object( + tmp_path: Path, + monkeypatch, +): + """A failure inside the write must not close the descriptor a second time. + + The file object closes the descriptor on its way out of the block, so the + old shared failure path closed it again; three writers reach this helper + concurrently through asyncio.to_thread, where that second close can land on + an unrelated descriptor that has since been handed the same number. + """ + + real_mkstemp = tempfile.mkstemp + handed_out: list[int] = [] + + def tracking_mkstemp(*args, **kwargs): + descriptor, name = real_mkstemp(*args, **kwargs) + handed_out.append(descriptor) + return descriptor, name + + real_close = os.close + closed: list[int] = [] + + def tracking_close(descriptor: int) -> None: + closed.append(descriptor) + real_close(descriptor) + + monkeypatch.setattr(persistence.tempfile, "mkstemp", tracking_mkstemp) + monkeypatch.setattr(persistence.os, "close", tracking_close) + + target = tmp_path / "evaluation.json" + with pytest.raises(TypeError): + # json.dump raises from inside the block, once the file object owns the + # descriptor. + persistence._atomic_write_json(target, {"not_serializable": object()}) + monkeypatch.undo() + + assert len(handed_out) == 1 + assert handed_out[0] not in closed + assert not target.exists() + assert not list(tmp_path.glob("*.tmp")) + + +@pytest.mark.asyncio +async def test_reconciled_load_rebuilds_a_torn_index_from_the_manifests( + tmp_path: Path, + caplog, +): + """A truncated database.json must not brick every later run of the session.""" + + value = record() + await EvaluationStore(tmp_path / "evaluations" / value.id).save(value) + database_path = tmp_path / "database.json" + EvaluationDatabase(id="session").save_to_file(database_path) + intact = database_path.read_text(encoding="utf-8") + database_path.write_text(intact[: len(intact) // 2], encoding="utf-8") + + with caplog.at_level("WARNING"): + restored = EvaluationDatabase.load_reconciled( + database_path=database_path, + evaluations_dir=tmp_path / "evaluations", + database_id="session", + ) + + assert restored.id == "session" + assert restored.get_evaluation(value.id) == value + assert "Rebuilding unreadable evaluation database" in caplog.text + # The repaired index is written back, so the next run reads a whole file. + reloaded = EvaluationDatabase.load_from_file(database_path) + assert reloaded.get_evaluation(value.id) == value + + +def test_reconciled_load_still_rejects_an_index_from_another_session(tmp_path: Path): + """A readable index naming another session stays a hard error, not a rebuild.""" + + database_path = tmp_path / "database.json" + EvaluationDatabase(id="other-session").save_to_file(database_path) + + with pytest.raises(ValueError, match="belongs to 'other-session'"): + EvaluationDatabase.load_reconciled( + database_path=database_path, + evaluations_dir=tmp_path / "evaluations", + database_id="session", + ) diff --git a/vero/tests/test_idempotency_wandb.py b/vero/tests/test_idempotency_wandb.py new file mode 100644 index 00000000..3c97e8c4 --- /dev/null +++ b/vero/tests/test_idempotency_wandb.py @@ -0,0 +1,334 @@ +"""Crash-safety of the W&B sinks' step counter and resume state. + +The sinks own two pieces of durable bookkeeping: the monotonic W&B step and the +dedupe keys that say what has already been sent. Both are written to +``artifacts/wandb/state.json``, and a run that dies mid-flight (the common case, +runs take hours) resumes from exactly that file. +""" + +from __future__ import annotations + +import json +import threading +from datetime import UTC, datetime, timedelta +from pathlib import Path + +import pytest + +from vero.candidate import Candidate +from vero.evaluation import ( + BackendProvenance, + CaseResult, + CaseStatus, + EvaluationPrincipal, + EvaluationRecord, + EvaluationReport, + EvaluationRequest, + EvaluationSet, + EvaluationStatus, + MetricSelector, + ObjectiveResult, + ObjectiveSpec, +) +from vero.runtime import RuntimeEvent, WandbEventSink +from vero.runtime.wandb import SidecarWandbSink + + +class FakeRun: + def __init__(self, *, log_raises: bool = False): + self.logged: list[tuple[dict, int]] = [] + self.summary: dict = {} + self.log_raises = log_raises + + def log(self, payload, *, step): + if self.log_raises: + # Stands in for the process dying between the state write and the + # point actually reaching W&B, which is the window under test. + raise RuntimeError("simulated crash while logging") + self.logged.append((payload, step)) + + def log_artifact(self, artifact): # pragma: no cover - not exercised here + pass + + def finish(self, *, exit_code=0): + pass + + +class FakeWandb: + def __init__(self, run: FakeRun | None = None): + self.kwargs: dict | None = None + self.run = run if run is not None else FakeRun() + + def init(self, **kwargs): + self.kwargs = kwargs + return self.run + + def Artifact(self, *, name, type): # pragma: no cover - not exercised here + raise AssertionError("no artifact should be built by these tests") + + +def _state(session_dir: Path) -> dict: + return json.loads( + (session_dir / "artifacts" / "wandb" / "state.json").read_text(encoding="utf-8") + ) + + +def _record(evaluation_id: str = "eval-1") -> EvaluationRecord: + created = datetime(2026, 1, 1, tzinfo=UTC) + return EvaluationRecord( + id=evaluation_id, + request=EvaluationRequest( + candidate=Candidate(id="cand", version="v1", created_at=created), + evaluation_set=EvaluationSet(name="benchmark", partition="validation"), + ), + report=EvaluationReport( + status=EvaluationStatus.SUCCESS, + metrics={"score": 0.5}, + cases=[ + CaseResult( + case_id="c1", status=CaseStatus.SUCCESS, metrics={"score": 0.5} + ) + ], + ), + backend_id="primary", + backend=BackendProvenance(name="harbor", version="1", config_digest="0" * 64), + principal=EvaluationPrincipal.SYSTEM, + objective_spec=ObjectiveSpec( + selector=MetricSelector(metric="score"), direction="maximize" + ), + objective=ObjectiveResult(value=0.5, feasible=True), + created_at=created, + completed_at=created + timedelta(seconds=3), + ) + + +def _evaluation_event(evaluation_id: str) -> RuntimeEvent: + return RuntimeEvent( + session_id="session", + kind="evaluation_completed", + payload={"step": 0, "evaluation_id": evaluation_id, "objective/value": 0.5}, + ) + + +def test_runtime_event_sink_persists_the_step_before_spending_it(tmp_path: Path): + """A crash at the log boundary must not leave the step free for reuse.""" + session_dir = tmp_path / "session" + crashing = FakeWandb(FakeRun(log_raises=True)) + sink = WandbEventSink( + project="v", session_id="s", session_dir=session_dir, client=crashing + ) + + with pytest.raises(RuntimeError): + sink(_evaluation_event("evaluation-1")) + + # The step was durable before it was spent, so the restart below cannot hand + # W&B step 0 a second time. + assert _state(session_dir) == { + "evaluation_ids": ["evaluation-1"], + "next_step": 1, + } + + resumed_client = FakeWandb() + resumed = WandbEventSink( + project="v", session_id="s", session_dir=session_dir, client=resumed_client + ) + resumed(_evaluation_event("evaluation-2")) + assert [step for _, step in resumed_client.run.logged] == [1] + + +def test_sidecar_sink_persists_the_step_before_spending_it(tmp_path: Path): + """Same window on the sidecar's evaluation stream.""" + session_dir = tmp_path / "session" + crashing = FakeWandb(FakeRun(log_raises=True)) + sink = SidecarWandbSink( + project="v", session_id="s", session_dir=session_dir, client=crashing + ) + + with pytest.raises(RuntimeError): + sink(_record()) + + state = _state(session_dir) + assert state["next_step"] == 1 + assert state["evaluation_ids"] == ["eval-1"] + + resumed_client = FakeWandb() + resumed = SidecarWandbSink( + project="v", session_id="s", session_dir=session_dir, client=resumed_client + ) + resumed(_record("eval-2")) + assert [step for _, step in resumed_client.run.logged] == [1] + + +def test_sidecar_inference_usage_persists_the_step_before_spending_it(tmp_path: Path): + """And on the gateway usage series the poller drives.""" + session_dir = tmp_path / "session" + crashing = FakeWandb(FakeRun(log_raises=True)) + sink = SidecarWandbSink( + project="v", session_id="s", session_dir=session_dir, client=crashing + ) + + with pytest.raises(RuntimeError): + sink.log_inference_usage({"producer": {"requests": 3, "total_tokens": 15}}) + + assert _state(session_dir)["next_step"] == 1 + + resumed_client = FakeWandb() + resumed = SidecarWandbSink( + project="v", session_id="s", session_dir=session_dir, client=resumed_client + ) + resumed(_record()) + assert [step for _, step in resumed_client.run.logged] == [1] + + +def test_sidecar_inference_usage_ledger_survives_a_restart(tmp_path: Path): + """The usage dedupe key belongs in state.json, not only in memory. + + Held in memory only, a restarted sidecar had no idea what cumulative usage it + had already reported and re-logged a byte-identical point at a fresh step. + """ + session_dir = tmp_path / "session" + first = FakeWandb() + sink = SidecarWandbSink( + project="v", session_id="s", session_dir=session_dir, client=first + ) + scopes = {"producer": {"requests": 3, "total_tokens": 15}} + sink.log_inference_usage(scopes) + assert len(first.run.logged) == 1 + assert _state(session_dir)["inference_usage"] == { + "inference/producer/requests": 3, + "inference/producer/total_tokens": 15, + } + + # A restart against the same session volume resumes the ledger: unchanged + # gateway counters are recognized as already reported. + restarted = FakeWandb() + resumed = SidecarWandbSink( + project="v", session_id="s", session_dir=session_dir, client=restarted + ) + resumed.log_inference_usage(scopes) + assert restarted.run.logged == [] + + # Movement still logs, on the step the previous process left behind. + resumed.log_inference_usage({"producer": {"requests": 4, "total_tokens": 20}}) + assert [step for _, step in restarted.run.logged] == [1] + + +class BlockingRun(FakeRun): + """A run whose first ``log`` parks, so two callers really do overlap.""" + + def __init__(self): + super().__init__() + self.entered = threading.Event() + self.release = threading.Event() + + def log(self, payload, *, step): + if not self.entered.is_set(): + self.entered.set() + assert self.release.wait(10) + self.logged.append((payload, step)) + + +def test_sidecar_sink_serializes_the_poller_against_the_evaluation_listener( + tmp_path: Path, +): + """The poller thread and the loop thread must not be inside the body at once. + + ``InferenceTelemetryPoller`` runs ``poll_once`` under ``asyncio.to_thread`` + while the engine calls the sink as a listener on the loop thread, so both + reach the same ``next_step`` and the same state.json. + """ + run = BlockingRun() + client = FakeWandb(run) + sink = SidecarWandbSink( + project="v", session_id="s", session_dir=tmp_path / "session", client=client + ) + + listener = threading.Thread(target=sink, args=(_record(),)) + listener.start() + assert run.entered.wait(10) + + poller_finished = threading.Event() + + def poll() -> None: + sink.log_inference_usage({"producer": {"requests": 1}}) + poller_finished.set() + + poller = threading.Thread(target=poll) + poller.start() + try: + # The listener is parked inside the guarded body, so the poller cannot + # get in. Released in `finally` so a failure here does not leave the two + # threads parked until interpreter exit. + assert not poller_finished.wait(0.3) + finally: + run.release.set() + listener.join(10) + poller.join(10) + assert poller_finished.is_set() + assert [step for _, step in run.logged] == [0, 1] + + +class UploadingRun(FakeRun): + """A run whose artifact upload parks, standing in for a slow transfer.""" + + def __init__(self): + super().__init__() + self.uploading = threading.Event() + self.release = threading.Event() + self.uploaded: list[object] = [] + + def log_artifact(self, artifact): + self.uploading.set() + assert self.release.wait(10) + self.uploaded.append(artifact) + + +class UploadingWandb(FakeWandb): + def Artifact(self, *, name, type): + class _Artifact: + def __init__(self) -> None: + self.files: list[str] = [] + + def add_file(self, path, *, name): + self.files.append(name) + + return _Artifact() + + +def test_shipping_request_logs_does_not_block_the_evaluation_listener(tmp_path: Path): + """The shared-state lock must not span the artifact upload. + + ``ship_request_logs`` runs on the telemetry poller's worker thread and + ``__call__`` on the sidecar's event loop, and they share the same lock over + ``next_step`` and state.json. Holding it across ``log_artifact``, which is a + file transfer, would let a slow W&B upload stall the loop that answers the + agent's requests. + """ + run = UploadingRun() + client = UploadingWandb(run) + session_dir = tmp_path / "session" + sink = SidecarWandbSink( + project="v", session_id="s", session_dir=session_dir, client=client + ) + log_dir = tmp_path / "requests" + log_dir.mkdir() + (log_dir / "requests-0.jsonl").write_text('{"model": "m"}\n', encoding="utf-8") + + shipper = threading.Thread( + target=sink.ship_request_logs, args=(log_dir,), kwargs={"final": True} + ) + shipper.start() + try: + assert run.uploading.wait(10) + # The upload is parked and the loop thread's listener still gets through. + sink(_record()) + assert [step for _, step in run.logged] == [0] + # The snapshot is not claimed until the bytes are actually shipped, so a + # crash right here re-ships next poll rather than losing the logs. + assert _state(session_dir)["request_log_files"] == {} + finally: + run.release.set() + shipper.join(10) + + assert run.uploaded, "the artifact was never uploaded" + assert _state(session_dir)["request_log_files"] == {"requests-0.jsonl": 15}