fix: the defects a review of everything since 1.32.0 turned up - #1484
Merged
Conversation
Writing several dotted keys that descend through the same value produced one replacement edit each, all claiming the same span of the file, and the apply loop spliced them in one after another against offsets taken from the original text. The second edit landed in the middle of what the first had just written. A CakePHP app_local.php reading its datasource from env() is enough to hit it: three keys under that one expression left a file PHP could no longer parse, and the writer reported success, so the first sign of it was every request failing. One replacement now serves every key that reaches the same node, built the same way the graft below it already collects several new keys under one absent parent.
Making the database choice the definition's to declare left SQLite declared nowhere: the option was gated on a sqlite entry under env.services and no published definition has one, so it disappeared from the Database question for every framework lerd recognises. A fresh Laravel ships DB_CONNECTION=sqlite and the wizard offered it only MySQL and PostgreSQL, so a project that kept its data in a file was moved onto a server database with no way to say no. Declaring it among the services is not the fix. Every binary already released reads that map as things to install and start, and a definition adding the key made those installs announce sqlite and then warn that the service does not exist, on every lerd env. It is also not what it is: nothing installs a file database, starts it, or draws a card for it. So the wiring gets a field of its own beside the services rather than among them, carrying the same detect rules and vars. An older binary ignores a field it does not know and behaves exactly as it does today, which is the same reason app_file sits beside file rather than replacing it. The values written now come from that declaration too, instead of the dotenv pair held in Go, which only ever reached the frameworks that happen to read DB_CONNECTION. A framework declaring none still gets those keys.
The reader draining a run's output gave up once a line passed its buffer, and nothing else reads that pipe, so the command stayed blocked writing to it and the wait for it never returned. The run sat at running for as long as lerd-ui was up: the wizard's spinner never resolved, the reopen button kept finding live work, the retention sweep skips anything unfinished, and the child was never reaped. Composer, npm and git all write progress as one line rewritten with a bare carriage return, which is exactly the shape that trips it. Reading in chunks instead emits such a line in pieces and always reaches the end of the pipe.
The retention sweep only ran when a new run started, so scaffolding one project and then leaving lerd alone kept that run, its buffered output and its place in every listing for as long as the daemon was up, well past the half hour the retention advertises. Listing runs sweeps as well now.
Every backslash in a line was doubled on its way into the event stream and nothing on the other side put it back, so a failed scaffold printed its PHP namespaces with twice the separators, in the one place a user reads to find out what went wrong. There is nothing to escape: the payload is read as it arrives. What does need handling is a carriage return, which ends a field as readily as a newline does and split one line of output into a frame the client took for another event.
The lookup sorted the cached definition files by name, so laravel@9 came out above laravel@12 and the marks were read off a definition three majors old. A machine that had ever resolved a Laravel 9 project drew that site's Reverb and Vite workers with the generic glyph and no brand colour, for good, with the right file sitting next to the one it read. Versions are numbers. The resolver next to it already sorts them that way, so it answers here too.
The dashboard only asks for a Node version where lerd manages Node, so everywhere else the answer comes back empty. Two of the three project kinds read the saved value when it does, and the PHP one did not, so reconfiguring a PHP site from the dashboard erased a node_version its .lerd.yaml pinned and dropped the site to the machine default on the next link. The fallback belongs once, before the kinds diverge, rather than in two of them.
Reconcile rewrote the quadlet for a refreshed definition and then decided whether to restart by looking at the materialised config files, which say nothing about the unit. A store change that moves a port, names a new image or adds an environment variable touches only the unit, so the new one sat on disk while the container kept running on what it started with, until something else restarted it. The generator already reports whether it changed anything, so that answer decides it. A service that is not running is left alone rather than started by a rewrite.
Working out which site owns a database goes through the site's framework declaration, which means detecting the framework, loading the definitions and evaluating its detect rules. That ran once per site per installed engine, so a machine with twenty sites and three engines paid sixty detections every time the Databases tab refreshed. The sites are walked once now and each engine reads its own share of the result.
Each named step runs in its own process that plans the directory again, and the plan is gated on live state: whether the site is secured, what a worker scan finds, a file the previous step wrote. A caller working through a list it enumerated earlier could name a step that had since stopped applying, and that failed the run with no such setup step and stopped the rest of its queue. Work that no longer needs doing is not an error. Such a step is reported and passed over, and the queue carries on.
Collecting one replacement per node stopped two keys descending through the same value from claiming its span twice, but it left the other way in: a key that names a node outright and another that writes inside it still produced two edits over the same text, and the second was spliced against an offset the first had moved. A key naming an array and another inserting into it collide the same way. Applying back to front is only valid while each edit sits wholly before the last, so that is now the condition for applying one at all, and the narrower edit that is already in wins. The tests run php over the result, since a splice landing mid-expression is a file that still reads back and that only php calls wrong.
Restoring the saved version whenever the answer came back empty kept the pin a project had, and took away the only way to remove one: the dashboard's Node question offers an entry whose value is exactly that empty string, so choosing Not pinned wrote the old version straight back and the site went on running it. The two cases are distinguishable. Where lerd manages Node the question is asked and empty is the user clearing it. Where it does not the question is never filled in, and that is the empty answer that must not erase anything.
Taking the connection values from the definition while still creating database/database.sqlite left the two disagreeing the moment a framework spelled its path anywhere else. A Symfony project got the DSN pointing at var/data.db, an empty file at Laravel's path, and a doctor finding about the real one being missing that running the fix could never clear. Site Doctor already resolves that path from the values a project holds, through the keys its own framework declares, so the same answer decides what to create. It reads a project's whole declared vocabulary to do it, which now includes the keys the file database is wired through.
Three more ways this writer damaged a file, all reporting success. A key naming a node and another key descending through it produced two edits over one span, and the overlap guard from the previous fix resolved that by dropping one, which turned corruption into a value silently never written. Grafting into an array whose last entry has no trailing comma pasted the new entry directly against it, which PHP rejects. And a negative constant like -PHP_INT_MAX parsed as the number "-" plus a phantom entry, so touching that array reprinted it broken. Each has its fix. An update key that is an ancestor of another cannot hold at the same time as it, so the deeper key wins up front and the collision never reaches the edits. A single-line array takes its new entries inline before the closing bracket instead of being reprinted whole over spans other edits sit inside. The graft supplies the comma the last entry lacks. And a sign with no digits behind it, or a number running into an identifier, is an expression and is kept whole. The deeper problem is that every one of these returned nil while writing a file PHP refuses, and lerd's own reader accepted the wreckage too, so nothing downstream could notice. The writer now proves its work before it writes: the output must parse, with entries comma-separated as PHP demands, every update must read back as written, and every key it was not asked to touch must still hold its old value. Whatever shape the next mistake takes, it becomes an error the user sees instead of a config file the application discovers.
…uded Treating every empty answer on a managed-Node machine as the user unpinning was too broad: the dashboard renders the Node question only for a PHP project, so a proxy or dev-server reconfiguration arrives with the field empty because nobody was asked, and saving that cleared a pin the project carries. The condition is now the same one that renders the question, kind and managed together. The terminal wizard's custom-container path had the older form of the same mistake, building its config without the field at all where the proxy path beside it carries the saved value through. It carries it now too.
Three ways the created file could miss the one the application opens. A framework spelling its connection as a driver class, CakePHP's Cake\Database\Driver\Sqlite or CodeIgniter's SQLite3, failed the scan for the literal word sqlite, so no file was created at all. The path was resolved before .env.lerd_override had its say, so an overridden location got the default file created beside it. And the result was joined to the project root unconditionally, ignoring the absolute-path and docroot rules the doctor already reads by, so an absolute path grew a bogus tree inside the project and a docroot-resolved one landed a directory too high. Detection now reads the framework's own sqlite detect rules first, the way every other wiring question is answered, with the generic scan kept for projects that declare none. Creation moves after the override merge, and where the file lands is the doctor's resolution: never an absolute path, which is the user's own to manage, and preferably the candidate whose parent directory the project already has, which is how Drupal's files directory under the docroot wins over a database directory it never had.
Two ways a parked wizard came back wrong. A queue resumed behind a finished setup run never reloaded the step plan, so the optional flag read false for every remaining step and one failing optional step stopped the rest, exactly what optional exists to prevent. And a scaffold reopened after its run had aged out of the registry found nothing to reattach to and fell back to the create form, whose button then failed on the directory the scaffold had already made, with no way forward. The plan is loaded before the queue drains, and the scaffold's target directory is persisted the moment the run starts, so a resume that finds the run gone still knows where the project is and carries it into the questions the way a watched run does.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A review of the 156 commits since the release surfaced ten defects, and this is all of them.
Two are the kind that break a project rather than annoy someone. The php-array writer produced one replacement edit per key when several keys descended through the same value, all claiming the same span, and spliced them over each other against stale offsets, so a CakePHP config reading its datasource from env() came out unparseable while the writer reported success. And the run reader gave up on a line longer than its buffer, leaving the command blocked writing to a pipe nobody was draining, so the run sat at running until lerd-ui was restarted.
The SQLite regression is the widest. Making the database choice the definition's to declare left it declared nowhere, so the option vanished from the wizard for every framework lerd recognises and a fresh Laravel was offered only MySQL and PostgreSQL. The wiring now has a field of its own beside the services rather than a key among them, because a file database is not a service and an already released binary reads that map as things to start. The values written come from the declaration too, instead of the dotenv pair held in Go that only ever reached the frameworks reading DB_CONNECTION. The definitions come with it in lerd-env/frameworks#43.
The rest are smaller. Worker marks were read off a definition three majors old because the cached files were sorted by name. Backslashes were doubled on their way into the run log and never put back. A pinned Node version was erased when a PHP site was reconfigured from the dashboard. A service kept running on its old unit when a store change touched only the unit. Framework detection ran once per site per engine on every Databases poll. Finished runs were never released on an idle daemon. And a queued setup step failed the whole queue when the plan, re-derived per invocation, no longer offered it.
Closes #1483