Skip to content

readValidateJSON silently swallows errors, causing crash in concurrent installs #232

@taylortom

Description

@taylortom

Summary

Multiple issues in the CLI's file handling cause crashes and data corruption when installPlugins is called concurrently (e.g. from adapt-authoring-adaptframework during course import).

Problems

1. readValidateJSON silently swallows errors (lib/util/JSONReadValidate.js)

export async function readValidateJSON (filepath, done) {
  try {
    const data = await fs.readFile(filepath, 'utf8')
    validateJSON(data, filepath)
    done?.(null, JSON.parse(data))
    return JSON.parse(data)
  } catch (err) {
    done?.(err.message)
    // BUG: returns undefined instead of re-throwing
  }
}

When the file can't be read (concurrent write, permissions, etc.), this returns undefined instead of throwing, causing downstream TypeError crashes.

2. getManifestDependencies doesn't handle missing data (lib/integration/Project.js:63-66)

async getManifestDependencies () {
  const manifest = await readValidateJSON(this.manifestFilePath)
  return manifest.dependencies  // crashes if manifest is undefined or has no dependencies key
}

3. project.add() has an unsafe read-modify-write cycle (lib/integration/Project.js:123-133)

add (plugin) {
  let manifest = { version: '0.0.0', dependencies: {} }
  if (this.containsManifestFile) {
    manifest = readValidateJSONSync(this.manifestFilePath)  // READ
  }
  manifest.dependencies[plugin.packageName] = ...            // MODIFY
  fs.writeJSONSync(this.manifestFilePath, manifest, ...)     // WRITE
}

Under concurrency, two add() calls can both read the same state, each add their plugin, and the second write silently overwrites the first plugin's entry. This is a classic lost-update race condition.

Impact

When installPlugins is called concurrently, random plugins are reported as "failed to install" even though they installed successfully — it's only the post-install manifest update that fails. Different plugins fail on each run due to non-deterministic timing.

Suggested Fixes

  1. readValidateJSON: re-throw on error

    } catch (err) {
      done?.(err.message)
      throw err
    }
  2. getManifestDependencies: defensive access

    async getManifestDependencies () {
      const manifest = await readValidateJSON(this.manifestFilePath)
      return manifest?.dependencies ?? {}
    }
  3. project.add(): use a file lock or queue writes through a single serialized method to prevent lost updates

Related: adapt-security/adapt-authoring-adaptframework#157

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type
    No fields configured for issues without a type.

    Projects

    Status

    New

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions