Repository navigation
[OMEGA-468] Carry the OmegaClaw memory and auth secret over on upgrade - #375
Conversation
There was a problem hiding this comment.
Looks good to me, but I'm concerned about what will happen if migration process crashes for some reason?
The script only check whether (excluding memory import file):
omega-memorydoesn't exist ORomegalcaw-memorydoesn't exist ORomegaclaw-memorydoesn't contain the marker
And check 3 runs after check 1, so if the migration process was already initiated and crashed for some reason, then omega-memory was created and partially filled with data, but the migration was not completed. In this case the check 1 will not pass and migration process will not be initiated, so this could lead to knowledge loss.
So I'd like to ask: should we address this, or should we just hope this never happens?
cc: @alyona-snet
|
@kabirkbr Could you please review Paul's comment and provide your opinion? |
|
Thanks @TossSky, this is what I had in mind: the manual upgrade steps from the release notes, done by the launcher. Keeping the old volume and starting re @paul-v-snet concern -- as i understand it, if the run is killed half way (Ctrl-C, a reboot, the Docker daemon restarting), We cannot just drop the " Proposal: mark the start of the migration in the old volume, next to the marker you already write at the end. On the next start, the old volume is checked first:
On a handled failure the start marker is removed again, so the old volume stays exactly as it was and your existing failure tests still hold. The code below (as well as the idea of the proposal :). was generated by my AI assistant. It is a proposal, not a finished change: please review it and take it, adapt it or do it your own way.
--- a/scripts/omega
+++ b/scripts/omega
@@ -752,17 +752,35 @@
local new_volume="omega-memory"
local memory_path="/PeTTa/repos/Omega/memory"
local marker=".migrated-to-omega"
+ local started_marker=".migration-started"
local stopped_old_container=0
+ local old_state
if [ -n "${memory_import_file}" ] \
- || docker volume inspect "${new_volume}" >/dev/null 2>&1 \
|| ! docker volume inspect "${old_volume}" >/dev/null 2>&1; then
return 0
fi
- if docker run --rm --entrypoint sh --volume "${old_volume}:/from:ro" "${image}" \
- -c "test -e /from/${marker}"; then
- return 0
- fi
+ old_state=$(docker run --rm --entrypoint sh --volume "${old_volume}:/from:ro" "${image}" \
+ -c "if [ -e /from/${marker} ]; then echo migrated; elif [ -e /from/${started_marker} ]; then echo interrupted; fi") \
+ || old_state=""
+ case "${old_state}" in
+ migrated)
+ return 0
+ ;;
+ interrupted)
+ echo "The last copy from ${old_volume} did not finish, copying again"
+ if docker volume inspect "${new_volume}" >/dev/null 2>&1 \
+ && ! docker volume rm "${new_volume}" >/dev/null; then
+ echo "Could not remove the partly copied volume ${new_volume}" >&2
+ return 1
+ fi
+ ;;
+ *)
+ if docker volume inspect "${new_volume}" >/dev/null 2>&1; then
+ return 0
+ fi
+ ;;
+ esac
echo "Copying memory from volume ${old_volume} to ${new_volume}"
if [ "$(docker inspect -f '{{.State.Running}}' "${old_container}" 2>/dev/null)" = "true" ]; then
@@ -774,7 +792,9 @@
echo "Stopped container ${old_container}"
fi
- if docker volume create "${new_volume}" >/dev/null \
+ if docker run --rm --entrypoint sh --volume "${old_volume}:/from" "${image}" \
+ -c "date -u > /from/${started_marker}" \
+ && docker volume create "${new_volume}" >/dev/null \
&& docker run --rm --entrypoint sh --volume "${new_volume}:${memory_path}" "${image}" \
-c "test -f ${memory_path}/prompt.txt" \
&& docker run --rm --entrypoint sh \
@@ -788,6 +808,8 @@
echo "Could not copy memory from ${old_volume}, the old installation is left as it was" >&2
docker volume rm "${new_volume}" >/dev/null 2>&1 || true
+ docker run --rm --entrypoint sh --volume "${old_volume}:/from" "${image}" \
+ -c "rm -f /from/${started_marker}" >/dev/null 2>&1 || true
if [ "${stopped_old_container}" = "1" ]; then
docker start "${old_container}" >/dev/null 2>&1 || true
fiTwo tests for it, using your fake docker ( --- a/tests/test_omega_launcher_migration.py
+++ b/tests/test_omega_launcher_migration.py
@@ -338,3 +338,37 @@
assert _container_state(docker_root, "omegaclaw") == "running"
assert _snapshot(old) == before
assert not _started_agent(docker_root)
+
+
+def _interrupted_migration(root, image_files):
+ old = _install_omegaclaw(root, running=False)
+ (old / ".migration-started").write_text("")
+ new = root / "volumes" / "omega-memory"
+ new.mkdir()
+ if image_files:
+ shutil.copytree(root / "image-memory", new, dirs_exist_ok=True)
+ return old, new
+
+
+def test_interrupted_copy_is_redone(docker_root):
+ old, new = _interrupted_migration(docker_root, image_files=True)
+
+ result = _launcher(docker_root, "start", "-d", IMAGE)
+
+ assert result.returncode == 0, result.stderr
+ assert _read(new / "history.metta") == "(old history)\n"
+ assert _read(new / "chroma_db" / "chroma.sqlite3") == "old long-term memory"
+ assert _read(new / "prompt.txt") == "omega prompt\n"
+ assert (old / ".migrated-to-omega").exists()
+ assert _started_agent(docker_root)
+
+
+def test_run_interrupted_before_the_image_files_is_redone(docker_root):
+ old, new = _interrupted_migration(docker_root, image_files=False)
+
+ result = _launcher(docker_root, "start", "-d", IMAGE)
+
+ assert result.returncode == 0, result.stderr
+ assert _read(new / "history.metta") == "(old history)\n"
+ assert _read(new / "prompt.txt") == "omega prompt\n"
+ assert _started_agent(docker_root)On the current PR head, both new tests fail (the migration is skipped). With the proposed change, all 14 tests in the file pass, run against the fake docker only. With a fix for this in, in whatever form, I am fine for it to go to testing. One more point for the release: it may be a good idea if the launcher say clearly that carrying memory over on upgrade is experimental, and that the old omegaclaw-memory volume should be kept until the user has checked that the agent remembers what it knew. |
|
Fixed it, see e30dc0a. I took the proposed patch and both tests as they were. I also left the start marker out of the copy and made the message after a successful copy say that the copy is experimental. Now a start killed in the middle of the copy is redone by the next start. |
| docker volume rm "${new_volume}" >/dev/null 2>&1 || true | ||
| docker run --rm --entrypoint sh --volume "${old_volume}:/from" "${image}" \ |
There was a problem hiding this comment.
If docker volume rm on line 810 fails, line 811 will still remove the migration marker. This can recreate the previously reported issue: the incomplete omega-memory volume remains, but on the next start it may be treated as a fully migrated volume.
I suggest explicitly checking that the volume has been removed successfully before removing the migration marker.
There was a problem hiding this comment.
Fixed it, see 60e922f. The start marker is now removed only when omega-memory is gone, so if the volume cannot be removed, the next start copies the memory again. The test covers it.
| old_state=$(docker run --rm --entrypoint sh --volume "${old_volume}:/from:ro" "${image}" \ | ||
| -c "if [ -e /from/${marker} ]; then echo migrated; elif [ -e /from/${started_marker} ]; then echo interrupted; fi") \ | ||
| || old_state="" | ||
| case "${old_state}" in |
There was a problem hiding this comment.
If this docker run fails for any reason, old_state is silently set to an empty string, which is then treated the same as "no migration markers found".
If omega-memory already exists after an interrupted migration, this can cause the script to skip migration and start the agent with an incomplete volume.
I suggest failing here instead of treating this case as an empty state.
There was a problem hiding this comment.
Fixed it, see 60e922f. If the markers cannot be read, start now stops with an error. The test covers it.
|
@TossSky, you need to resolve the conflicts with the main branch. I'll reapprove it after that's done. |
|
@paul-v-snet conflicts were resolved. Can you re-approve please. |
Description
An upgrade from v0.1.19 started with empty memory. The launcher mounts
omega-memory, the v0.1.19 memory stays inomegaclaw-memory, and the oldomegaclawcontainer keeps running next to the new one. A setup that starts the container itself and still passesOMEGACLAW_AUTH_SECRETran with authorization off, because the proxy reads onlyOMEGA_AUTH_SECRET.startnow copies the old memory before it creates the container. This happens only whenomegaclaw-memoryexists and has not been copied before, and--memory-importis not set. An existingomega-memoryis left alone, because the new version may already keep memory there. The launcher stopsomegaclawif it is running, fillsomega-memoryfrom the image and copies everything exceptprompt*.txtandtg_prompt.txt, which the new version ships itself.Before it creates
omega-memory, the launcher writes.migration-startedinto the old volume, and after the copy it writes.migrated-to-omega. Finding only the first marker means that the previous run was killed in the middle of the copy, so the launcher removesomega-memoryand copies again. The second marker stays in the old volume, socleanfollowed bystartgives an empty memory again. The old volume is mounted read-only for the copy and is never removed. If any step fails, the launcher removesomega-memory, startsomegaclawagain if it was running, and exits beforedocker run. It removes the start marker only onceomega-memoryis gone, so a volume it could not remove is copied again on the next start. If the markers cannot be read,startstops with an error. After a successful copy it warns that the copy is experimental. The installer goes throughstart, so it migrates as well.proxy/nginx.shfalls back toOMEGACLAW_AUTH_SECRETwhenOMEGA_AUTH_SECRETis empty and logs that the old name is deprecated. When both are set,OMEGA_AUTH_SECRETwins. The paths to the template and to the generated config can be overridden throughNGINX_TEMPLATEandNGINX_CONFIG, so the script runs in a test.How Has This Been Tested?
tests/test_omega_launcher_migration.pyruns the launcher against a fake docker that keeps volumes as directories and runs the copy for real. It covers the copy itself, the prompts, the old container,cleanfollowed bystart, an existingomega-memory,--memory-import, a failed copy, a failed file copy, an image without the Omega memory layout, a copy interrupted before and after the image files reachedomega-memory, the start marker staying out ofomega-memory, a failed copy that could not removeomega-memory, and markers that cannot be read.tests/test_proxy_auth_secret.pyrunsproxy/nginx.shwith the real template and checks the generated config. The old name alone sets the secret and logs a warning, the new name wins over the old one, and without a secret the value stays empty.tests/pytest.shgives 107 passed and 1 skipped.scripts/omega startand the installer copiedhistory.mettaandchroma_dbunchanged, used the prompts of the image and stoppedomegaclaw, and the agent recalled both facts throughquery.cleanfollowed bystartgave an empty memory.omega-memorylimited to a 64 KB tmpfs, the launcher exited with code 1, removedomega-memoryand the start marker, and startedomegaclawagain.OMEGACLAW_AUTH_SECRET=4242passed to the image, the newnginx.shgives{"enabled":true}on/auth/statusand rejects a wrong token. The current one gives{"enabled":false}and accepts any token.Checklist