From 22f38d09e79cc7d5d9b8f62429b0b4aad0e091cb Mon Sep 17 00:00:00 2001 From: Shehab Yasser Date: Sat, 1 Aug 2026 13:25:31 +0300 Subject: [PATCH] fix: make each step of a run idempotent across a relaunch A vero run takes hours and dies mid-flight often enough that a relaunch is the normal case, not the exception. Until now a relaunch could not pick up where the previous attempt stopped, and on its way past it destroyed some of the evidence of how that attempt had died. Three classes of change, none of which touches scoring. Durable writes are atomic. Every terminal artifact now stages into a sibling temporary file, fsyncs it, renames it into place, and fsyncs the parent directory so the rename itself survives a hard kill: the evaluation manifests, the per-case checkpoints, database.json, budgets.json, the session manifest, reward.json, finalization.json, experiment.html, and the exported session archive. A torn last line in the event log is skipped and counted instead of killing report generation, an unreadable database.json is rebuilt from the per-evaluation manifests, and trial redaction publishes by rename so an interrupted redaction leaves the original record rather than a file that is neither version. A relaunch resumes instead of starting over. A session directory takes an flock, so a second run over it is refused with the holder's pid instead of two processes spending one budget ledger twice. The RUNNING manifest write no longer clears the previous failure, which was a rerun's first durable act and erased the only record of the death before anyone read it. A sub-run that dies part way through now copies the trials it already finished off the sandbox before the staging area is torn down. W&B persists its run id, step counter and dedupe keys, and the gateway usage ledger survives a sidecar restart. A rerun computes the same identity. Commit dates are pinned in the baseline repo and the workspace, so the baseline sha a session manifest is validated against is stable across recompiles; a recompile reuses the previous gateway tokens instead of rotating them and stages the build into a sibling before swapping; serve reuses an existing admin token; and the seed script's exclude lines are guarded so re-running it does not append duplicates. Co-Authored-By: Claude Opus 5 (1M context) --- vero/src/vero/candidate_repository/git.py | 22 + vero/src/vero/evaluation/engine.py | 37 +- vero/src/vero/evaluation/store/persistence.py | 59 ++- vero/src/vero/gateway/inference.py | 50 +- vero/src/vero/harbor/backend.py | 82 ++- vero/src/vero/harbor/build/compiler.py | 136 ++++- .../vero/harbor/build/templates/seed.sh.j2 | 24 +- vero/src/vero/harbor/cli.py | 90 +++- vero/src/vero/optimization/optimizer.py | 47 +- vero/src/vero/report.py | 74 ++- vero/src/vero/runtime/events.py | 16 +- vero/src/vero/runtime/session.py | 376 ++++++++------ vero/src/vero/runtime/wandb.py | 105 +++- vero/src/vero/sidecar/app.py | 37 +- vero/src/vero/sidecar/serve.py | 24 +- vero/src/vero/sidecar/session.py | 31 ++ vero/src/vero/sidecar/sidecar.py | 65 +++ vero/src/vero/sidecar/verifier.py | 15 + vero/src/vero/workspace/git.py | 51 +- vero/tests/test_idempotency_backend.py | 368 +++++++++++++ vero/tests/test_idempotency_compiler.py | 250 +++++++++ vero/tests/test_idempotency_engine.py | 160 ++++++ vero/tests/test_idempotency_gateway.py | 172 +++++++ vero/tests/test_idempotency_gitrepo.py | 184 +++++++ vero/tests/test_idempotency_harborcli.py | 244 +++++++++ vero/tests/test_idempotency_optimizer.py | 252 +++++++++ vero/tests/test_idempotency_reportevents.py | 242 +++++++++ vero/tests/test_idempotency_seedsh.py | 194 +++++++ vero/tests/test_idempotency_session.py | 131 +++++ vero/tests/test_idempotency_sidecar.py | 482 ++++++++++++++++++ vero/tests/test_idempotency_store.py | 165 ++++++ vero/tests/test_idempotency_wandb.py | 334 ++++++++++++ 32 files changed, 4253 insertions(+), 266 deletions(-) create mode 100644 vero/tests/test_idempotency_backend.py create mode 100644 vero/tests/test_idempotency_compiler.py create mode 100644 vero/tests/test_idempotency_engine.py create mode 100644 vero/tests/test_idempotency_gateway.py create mode 100644 vero/tests/test_idempotency_gitrepo.py create mode 100644 vero/tests/test_idempotency_harborcli.py create mode 100644 vero/tests/test_idempotency_optimizer.py create mode 100644 vero/tests/test_idempotency_reportevents.py create mode 100644 vero/tests/test_idempotency_seedsh.py create mode 100644 vero/tests/test_idempotency_session.py create mode 100644 vero/tests/test_idempotency_sidecar.py create mode 100644 vero/tests/test_idempotency_store.py create mode 100644 vero/tests/test_idempotency_wandb.py 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}