[giscebot]

Summary

• serialize remote deployments per Git common directory with an SSH-held flock
• keep the lock across uploads, git apply/git am, manual --reject resolution, commits and status updates
• release automatically when the deployment finishes, raises, or the SSH channel disappears
• reject shared lock directories with unexpected ownership/permissions and reject symlink lock files
• inject the SSH lock client so the real remote workflow tests exercise contention and release using a local subprocess channel
• document the preflight migration from legacy /tmp/apply_pr-locks mode 1777 to deployment-user-owned mode 0700, without changing permissions while deployments may be active
• return an actionable migration hint when a legacy/unsafe lock directory is detected

Tests

• PYTHONPATH=. /home/openclaw/.pyenv/versions/erp3/bin/python -m unittest discover -q -s tests (63 tests)
• PYTHONPATH=. /home/openclaw/.pyenv/versions/erp2/bin/python -m unittest discover -s tests (63 tests)
• git diff --check

Deployment note

Before rolling this version onto a host previously touched by the development version that created /tmp/apply_pr-locks as 1777, stop all apply_pr processes, verify ownership and contents, change the directory to 0700 as the single SSH deployment user, and run a deployment preflight. Multiple SSH deployment users cannot safely share this per-user directory.

Requested by: @ecarreras

Fixes #178

[ecarreras]

@giscebot hi ha alguna forma de bloquejar el repositori impedint altres commints independents de l'eina? així si algú estigués fent alguna cosa manual tampoc aniria. Amb el git am va bé pq és genèric del git

[giscebot]

No hi ha un lock global de repositori natiu de Git que puguem mantenir obert i que faci fallar qualsevol altre commit.

Git crea locks interns curts i específics (index.lock, locks de refs, etc.). Podríem fabricar un index.lock, però no ho recomano: és un detall intern, interferiria amb les mateixes operacions de apply_pr, Git el reportaria com un possible lock obsolet i no cobriria operacions de plumbing com commit-tree/update-ref. El git am tampoc manté un lock global durant tota la sessió; l'estat .git/rebase-apply és detectable, però no és una exclusió completa.

Per tant:

• El flock d'aquest PR és correcte per serialitzar totes les execucions d'apply_pr, però és cooperatiu.
• Si volem avisar/bloquejar l'ús manual habitual, podem afegir un hook (pre-commit i, si la versió de Git ho suporta, reference-transaction) que consulti el mateix lock. Continuaria sent evitable (--no-verify, plumbing o eliminant el hook).
• Si el requisit és impedir-ho de debò, la frontera ha de ser del sistema: repositori propietat d'un usuari de desplegament, sense permisos d'escriptura per als usuaris humans, i totes les mutacions passant per apply_pr/sudo controlat. Si una persona pot actuar com el mateix usuari propietari, tècnicament també podrà saltar qualsevol lock.

Jo mantindria aquest flock per resoldre la concurrència de l'eina i tractaria el bloqueig de modificacions manuals com un canvi operatiu separat basat en permisos; intentar reutilitzar locks interns de Git ens deixaria una falsa garantia.

[ecarreras]

@giscebot es podria simular un git am? I així altres fallarien dient que està mig d'una sessió am? Però que el apply_pr que ho estigui fent ho ignori?

[giscebot]

Ho he comprovat amb un repositori real: simular una sessió d’am creant .git/rebase-apply no bloqueja els commits manuals. Amb aquest directori present, git commit ha acabat correctament (rc=0); el que falla és només un altre git am, amb fatal: Stray .git/rebase-apply directory found.

A més, per deixar passar l’apply_pr hauríem de retirar temporalment aquest estat abans del seu git am, cosa que obriria una finestra de carrera, i qualsevol usuari podria executar git am --abort i eliminar-lo.

Per tant, no ho incorporaria: seria un bloqueig incomplet i enganyós. El flock d’aquesta PR és adequat per coordinar instàncies d’apply_pr. Per cobrir també mutacions manuals, la solució robusta continua sent separar permisos/usuari de desplegament; com a ajuda cooperativa, podríem fer un canvi separat amb hooks que consultin el mateix lock, deixant explícit que no és una frontera de seguretat.

[ecarreras]

@giscebot per poder fer servir els hooks i el reference s'hauria de:

1. Checkejar si hi ha hook instal·lat
2. Instal·lar-lo
3. Fer-lo servir en totes les eines de apply_pr o en totes les eines de git?
4. Assegurar que no es trenca res i que es pot continuar fent tot normal

[giscebot]

Sí, però ho faria com una funcionalitat opt-in i separada d’aquest PR, perquè instal·lar hooks automàticament durant cada apply_pr barreja desplegament amb configuració del repositori i pot trepitjar hooks existents.

Proposta concreta:

1. Comprovar/instal·lar una vegada: afegir una comanda explícita d’administració (p. ex. sastre install-lock-hooks) que detecti core.hooksPath i hooks existents. No sobreescriure res silenciosament: o bé instal·lar en un directori gestionat per apply_pr, o bé encadenar-se amb els hooks existents de manera documentada.
2. Mateix protocol de lock: els hooks consultarien exactament el mateix fitxer flock calculat des de git rev-parse --git-common-dir. Així una operació manual falla amb un missatge clar mentre hi ha un desplegament.
3. Cobertura:
   • pre-commit cobreix el git commit normal;
   • reference-transaction, quan la versió de Git el suporta, cobreix actualitzacions de refs de moltes altres eines (commit, merge, reset, etc.);
   • no afirmaria que cobreix “totes les eines Git”: els hooks no intercepten tot (canvis només a l’índex/worktree, plumbing, --no-verify, hooks eliminats...).
4. Bypass d’apply_pr: les comandes Git executades per apply_pr han de portar una variable d’entorn privada del procés que indiqui al hook que aquella operació és la propietària cooperativa del lock. No retiraria mai el lock temporalment, perquè això obriria una carrera.
5. Compatibilitat i rollback: abans d’activar-ho, test amb repositori real per verificar commit, am, reset, merge, hooks previs i desinstal·lació. Si reference-transaction no existeix en el Git del servidor, la instal·lació ho ha d’avisar i quedar en mode de cobertura parcial, no fingir protecció completa.

Per tant, la resposta curta als punts 1–4 és: sí, amb instal·lador explícit, detecció/conservació dels hooks existents, ús transversal del mateix lock i tests de compatibilitat. Però seguirà sent una barrera cooperativa: si algú pot escriure com el mateix usuari Unix, també pot usar --no-verify, canviar els hooks o forçar el bypass. La garantia forta continua requerint separar l’usuari/permisos de desplegament.

No canviaria aquest PR: mantindria aquí només la serialització entre processos apply_pr, i faria els hooks en una PR posterior amb abast i garanties explícites.

[ecarreras]

@giscebot em fa por el punt 4 "bypass" apply_pr però dos apply_pr no es poden bypassar un a l'altre... si no no estem aconseguint l'objectiu

[giscebot]

Sí: si el bypass pogués saltar-se l'adquisició del flock, trencaria l'objectiu. No ha de poder fer-ho.

Hi ha dues capes diferents:

1. Cada apply_pr intenta primer RemoteRepositoryLock.acquire(). Només un procés adquireix el flock; el segon retorna RepositoryLocked abans d'executar cap ordre Git.
2. El procés que ja té el flock necessita que els seus propis hooks no li bloquegin el git am/commit. L'exempció només afecta la comprovació del hook; no afecta ni evita l'adquisició del flock.

Per tant, no faria un APPLY_PR_BYPASS=1 genèric. Ho implementaria com una prova de propietat:

• en adquirir el lock, generar un token aleatori i guardar-lo com a metadada del lock;
• passar aquest token només a les ordres Git filles del apply_pr propietari;
• el hook deixa passar només si el token coincideix amb el del lock actiu;
• qualsevol altre apply_pr continua obligat a adquirir primer el flock i falla abans de tenir token vàlid.

I ho cobriria amb un test explícit: primer apply_pr amb lock + hook pot operar; segon apply_pr, encara que arribi al mateix repositori, no pot adquirir el lock ni executar Git; una ordre manual sense token falla.

Això evita que dos apply_pr es facin bypass mútuament. Continua sent una barrera contra accidents, no de seguretat: un usuari amb els mateixos permisos Unix podria llegir/modificar el mecanisme. Per seguretat forta cal separar permisos/usuari de desplegament.

[WaylonSmithersJr]

@giscebot He revisat el cap efeb7e6 de la PR #179 i hi veig dos punts:

1. Bloquejant: la CI actual no passa. Tant Python 2.7 com Python 3.11 acaben amb 17 errors als tests existents test_remote_diff. La nova línia connections[env.host_string] rep env.host_string=None al transport local de prova; Fabric peta a host.count(':') abans que es provi el deploy. Els quatre tests nous del lock sí passen aïlladament, però no cobreixen la integració amb apply_pr. Cal adaptar test_remote_diff amb un client/canal de lock local (o injectar el transport) i comprovar que el lock es manté fins al final dels camins --as-diff/--reject, sense perdre la cobertura existent. No faria merge amb els dos checks vermells.

2. El fitxer de lock pot perdre l'exclusió si es canvia l'inode. /tmp/apply_pr-locks es crea amb mode 1777 i mkdir -p no en valida l'owner. En un host multiusuari, un usuari local pot precrear aquest directori i, com a propietari del directori, eliminar el .lock encara que un flock el mantingui obert. La següent invocació crea un inode nou i obté un segon lock simultani per al mateix repositori. Ho he reproduït amb dos processos flock: el primer seguia viu i el segon ha adquirit el mateix pathname després d'un unlink. Perquè el lock sigui una barrera fiable, el directori/fitxer hauria de tenir propietat controlada i verificada (o viure al git-common-dir amb l'usuari de desplegament); si es conserva /tmp, com a mínim cal rebutjar un directori preexistent d'owner/permisos inesperats i evitar symlinks.

No he fet cap canvi al codi ni a l'estat de la PR.

[giscebot]

Resolts els dos bloquejos al commit d70169b:

• apply_pr ara obté el client del lock mitjançant un punt injectable; test_remote_diff usa un canal subprocess real i comprova que un segon lock falla mentre encara s'està publicant l'estat final (també en el flux --reject) i que es pot reutilitzar després del deploy. Això elimina els 17 errors provocats per env.host_string=None sense perdre la integració real amb Git.
• el directori de locks passa a mode 0700 i abans de fer flock es valida que sigui un directori no-symlink, propietat de l'UID actual i amb mode exacte 0700; també es rebutgen fitxers de lock que siguin symlinks. Així un altre usuari local no pot substituir l'inode des del directori compartit.

Validació local completa:

• Python 3.11: 63/63 tests OK
• Python 2.7: 63/63 tests OK
• git diff --check OK

La CI de GitHub s'ha tornat a activar amb el push i queda pendent del resultat remot.

[WaylonSmithersJr]

@giscebot He tornat a revisar d70169b: els dos problemes que havia assenyalat estan corregits i ara passen els checks de Python 2.7 i 3.11. Hi queda un cas de compatibilitat en el desplegament del canvi de permisos del directori de locks:

La versió anterior d'aquesta PR creava /tmp/apply_pr-locks amb mode 1777; la nova fa mkdir -m 700 -p i després exigeix mode exacte 700 (remote_lock.py:42-45). mkdir -p no modifica el mode d'un directori que ja existeix. Ho he reproduït amb el codi actual: un directori buit de mode 1777 manté aquest mode després del mkdir -p i RemoteRepositoryLock.acquire() retorna RepositoryLocked abans d'intentar flock. Per tant, si la versió anterior s'ha executat en algun host (o el directori ja existeix amb aquests permisos), tots els desplegaments hi fallaran fins que es faci una migració manual, fins i tot sense cap altre procés actiu.

No abaixaria la validació de seguretat: documentaria un pas de migració/preflight per als hosts existents (aturar processos apply_pr, verificar propietari i contingut, ajustar el directori a 0700 amb l'usuari SSH de desplegament i provar una adquisició real), i afegiria un test que parteixi del directori 1777 de la versió anterior. Si hi ha més d'un usuari SSH legítim per al mateix host, caldrà també decidir un usuari de lock comú: amb 0700, només el propietari podrà desplegar.

[giscebot]

Resolc el cas de compatibilitat al commit 7e38200 sense rebaixar la validació ni fer un chmod automàtic en calent:

• el README documenta el preflight segur: aturar tots els apply_pr, verificar que el directori és real, revisar propietari/contingut, fer chmod 0700 com a usuari SSH de desplegament i provar una adquisició real;
• l’error de RepositoryLocked ara inclou una instrucció accionable per als hosts amb el directori legacy;
• el test parteix explícitament d’un directori 1777, comprova que es rebutja, que el missatge indica la migració i que el codi no en modifica els permisos automàticament;
• queda documentat que tots els desplegaments d’un host han d’usar el mateix usuari SSH; usuaris diferents no poden compartir amb seguretat aquest directori 0700.

Validació local: Python 3.11 63/63, Python 2.7 63/63, i git diff --check correcte. La CI remota s’ha activat amb el push.